Hi Igor,
On Wed, Nov 15, 2023 at 9:37 PM Igor Kulchytskyy <ig...@ami.com> wrote:
>
> Hi Mike,
> My implementation of checking the IpType supposed to be like this:
> /**
> Check if Network Protocol Type matches with SMBIOS Type 42 IP Address Type.
>
> @param[in] NetworkProtocolType The Network Protocol Type to check with.
> @param[in] IpType The Host IP Address Type from SMBIOS Type
> 42.
> **/
> STATIC
> BOOLEAN
> IsTcpTypeMatch (
> IN UINT32 NetworkProtocolType,
NetworkProtocolType has defined type -
NETWORK_INTERFACE_PROTOCOL_TYPE, why don't you use it.
Ah. I see it is declared as UINT32 in REDFISH_DISCOVER_REQUIRED_PROTOCOL... Ok.
It's not clear why, because it is internal structure.
> IN UINT8 IpType
> )
> {
> if ((NetworkProtocolType == ProtocolTypeTcp4) && (IpType ==
> REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP4)) {
> return TRUE;
> }
> if ((NetworkProtocolType == ProtocolTypeTcp6) && (IpType ==
> REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP6)) {
> return TRUE;
> }
> return FALSE;
> }
>
> And then in BuildupNetworkInterface function.
>
> // 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 (!IsTcpTypeMatch (mRequiredProtocol[Index].ProtocolType, IpType)) {
> continue;
> }
So, where is REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_UNKNOWN handled now?
If IpType == REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_UNKNOWN
IsTcpTypeMatch() always returns FALSE,
so !IsTcpTypeMatch() gives TRUE and we skip this iteration.
Effectively this means
if (IpType == REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_UNKNOWN ) {
return EFI_DEVICE_ERROR
}
Just because there is nothing after this loop.
And this is not what we wanted, and recognized as my mistake previously.
>
> -----Original Message-----
> From: Mike Maslenkin <mike.maslen...@gmail.com>
> Sent: Wednesday, November 15, 2023 1:27 PM
> To: Igor Kulchytskyy <ig...@ami.com>
> Cc: Leif Lindholm <quic_llind...@quicinc.com>; devel@edk2.groups.io; Abner
> Chang <abner.ch...@amd.com>; Nickle Wang <nick...@nvidia.com>
> Subject: Re: [EXTERNAL] Re: [edk2-devel] [PATCH v5 2/2] RedfishPkg:
> RedfishDiscoverDxe: Optimize the Redfish Discover flow
>
> On Wed, Nov 15, 2023 at 4:52 PM Igor Kulchytskyy <ig...@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;
> }
> return false;
> }
>
> 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)) {
> continue;
> }
>
> Regards,
> Mike.
> -The information contained in this message may be confidential and
> proprietary to American Megatrends (AMI). This communication is intended to
> be read only by the individual or entity to whom it is addressed or by their
> designee. If the reader of this message is not the intended recipient, you
> are on notice that any distribution of this message, in any form, is strictly
> prohibited. Please promptly notify the sender by reply e-mail or by telephone
> at 770-246-8600, and then delete or destroy all copies of the transmission.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111290): https://edk2.groups.io/g/devel/message/111290
Mute This Topic: https://groups.io/mt/102584140/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
