Re: [edk2-devel] [PATCH v5 2/2] RedfishPkg: RedfishDiscoverDxe: Optimize the Redfish Discover flow

["Leif Lindholm" <quic_llindhol@quicinc.com>]

On 2023-11-15 18:27, Mike Maslenkin wrote:
> On Wed, Nov 15, 2023 at 4:52 PM Igor Kulchytskyy <igork@ami.com> wrote:
>>
>> Hello Leif and Mike,
>> Let me try to explain the idea of the filtering IP.
>> That filtering should work only if we know exactly that IP is IPv4 or IPv6 in SMBIOS Type 42.
> Hm. I've already composed a reply below, but then a returned to this
> statement...
> 
> Is this a difference in condition between v3 and v5? I came to the
> conclusion that at the place we are discussing
> SMBIOS table 42h can be absent because
> PlatformHostInterfaceInformationReady hasn't been called yet,
> so REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_UNKNOWN is expected.
> 
> 
>> And it just helping to reduce the work in case we know the exact type of IP, which supposed to be used in BIOS BMC communication.
>> In that case there is no need to build network interface for the unused IP Type.
>> On some systems IP address could be dynamic and we will not be able to know the version of IP from SMBIOS.
>> If you see I check HostIpAssignmentType in GetHiIpProtocolType function. And return IP type UNKNOWN if it is not static.
>> If we get an unknown IP type, we should not return from BuildupNetworkInterface function, but just proceed and build the network interface for all IPs.
>> So, later Redfish Discover driver can find the right one.
>> Based on this logic I'm going to prepare the patch v6.
>> Thank you,
>> Igor
> 
> Agree.. I was focused on edk2 implementation of
> RedfishPlatformHostInterfaceLib and PlatformHostInterfaceBmcUsbNicLib
> where HostIpAddressFormat is specified (hardcoded). I guess
> HostIpAddressFormat  as well as SMBIOS table 42h must be available
> by the time RedfishServiceAcquireService()->DiscoverRedfishHostInterface()
> call, while it might be not available at the moment
> RedfishDiscoverDriverBindingStart()->BuildupNetworkInterface(). So,
> condition from v3 looks correct to me.
> 
> My main concern was introduction of defines. Those don't look great.
> Those are huge (it even doesn't fit into the screen) and misleading a
> bit.
> For example:
> +#define MAC_COMPARE(ThisNetworkInterface, TargetNetworkInterface)
> (CompareMem ((VOID *)&ThisNetworkInterface->MacAddress,
> &TargetNetworkInterface->MacAddress,
> ThisNetworkInterface->HwAddressSize))
> 
> The proposed variant is equal to #define MAC_COMPARE(A, B)
> (CompareMem ((VOID *)&ThisNetworkInterface->MacAddress,
> &TargetNetworkInterface->MacAddress,
> ThisNetworkInterface->HwAddressSize)), i.e a bit useless.
> 
> I would expect it could be declared at least as:
> #define MAC_COMPARE(This, Target)  CompareMem ((VOID
> *)&(This)->MacAddress, &(Target)->MacAddress, (This)->HwAddressSize)
> I.e define should really replace some arguments  also reducing the line length.
> 
> BTW: there is a place in ValidateTargetNetworkInterface() where
> CompareMem  can be replaced with MAC_COMPARE too.
> 
> Also, I found IP6_LINK_EQUAL(Mac1, Mac2) define, that is unused in
> edk2. But according to that one, please consider moving "== 0" check
> to #define declaration.
> But I do not think this macro is required at all, because there are 5
> MAC compares left in this module. So, it just brings some
> inconsistency.
> 
> Agreed that static helper function would be the best.
> 
> Leif, do you expect something like this?
> STATIC
> BOOLEAN
> FilterInterface (
>    IN NETWORK_INTERFACE_PROTOCOL_TYPE ProtocolType,
>    IN UINT8 HostIpAddressFormat
>    )
> {
>    // This is based on v5, but according to the comments above
>    // v3 is correct as it allows
> REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_UNKNOWN
> 
>    if (ProtocolType == ProtocolTypeTcp4) {
>      return IpType != REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP4;
>    } else if (ProtocolType == ProtocolTypeTcp6) {
>      return IpType != REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP6;
>    }

Yes, this looks ideal.

>    return false;

Although this should be FALSE (upper-case).

> }
> 
> and then::
> 
> // Get IP Type to filter out unnecessary network protocol if possible
> IpType = GetHiIpProtocolType ();
> 
> for (Index = 0; Index < ListCount; Index++) {
>    // Check IP Type and skip an unnecessary network protocol if does not match
>   if (FilterInterface (gRequiredProtocol[Index].ProtocolType, IpType)) {

This gives us a big but still readable chunk here, and much neater tests 
in the helper than they *could* be either in place or in macros.

>      continue;
>    }

Ship it.

/
     Leif

> Regards,
> Mike.
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111314): https://edk2.groups.io/g/devel/message/111314
Mute This Topic: https://groups.io/mt/102584140/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-