Re: [edk2-devel] [PATCH] RedfishPkg/RedfishDicovery: Remedy Redfish service discovery flow

["Chang, Abner via groups.io" <abner.chang=amd.com@groups.io>]

[AMD Official Use Only - General]

Hi Mike, below is my answer,

> -----Original Message-----
> From: Mike Maslenkin <mike.maslenkin@gmail.com>
> Sent: Thursday, December 7, 2023 9:37 PM
> To: Chang, Abner <Abner.Chang@amd.com>
> Cc: devel@edk2.groups.io; Nickle Wang <nicklew@nvidia.com>; Igor
> Kulchytskyy <igork@ami.com>
> Subject: Re: [PATCH] RedfishPkg/RedfishDicovery: Remedy Redfish service
> discovery flow
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> Hi Abner,
>
> please see my comments below
>
> On Wed, Dec 6, 2023 at 6:06 PM <abner.chang@amd.com> wrote:
> >
> > From: Abner Chang <abner.chang@amd.com>
> >
> > Remedy Redfish service discovery flow changes made
> > in commit 8736b8fd.
> >
> > The above fix creates the dependency with SMBIOS 42h record,
> > which has a problem as SMBIOS 42h may not be created when
> > RedfishDiscovery.Supported() is invoked even all of the
> > required protocols are ready on the ControllerHandle. We can’t
> > guarantee SMBIOS 42 structure will be always created before
> > ConnectController(). USB NIC maybe detected late and it means
> > PlatformHostInterfaceBmcUsbNicLib can populate SMBIOS 42h
> > information late as well. Calling to
> > RedfishServiceGetNetworkInterface with the previous fix may
> > result in no network interface for BMC-exposed NIC as SMBIOS
> > 42h is not ready yet.This is the first issue.
> >
> > Second, to skip the network interface when
> > NetworkInterfaceGetSubnetInfo() returns a failure also has
> > problem, as the NIC may be configured via RestEx->Configure().
> > This happens after the Host interface is discovered, as at this
> > moment we have the sufficient network information to configure
> > BMC-exposed NIC.
> >
> > Base on Redfish spec in 31.1.5.2, “EFI Redfish Client may provide
> > selection UI of network interfaces for Redfish service discovery.",
> > This means edk2 Redfish client gets all network interfaces
> > through RedfishServiceGetNetworkInterface and choose the desired
> > network interface at its discretion for Redfish service.
> >
> > So the fix here is:
> > 1. In BuildNetworkInterface(), we don’t skip any network
> >    interface. In RedfishServiceGetNetworkInterface, we don’t
> >    skip any network interface even the subnet information is not
> >    retrieved. We will still return all of network interfaces to
> >    client.
> > 2. In RedfishServiceAcquireService for
> >    EFI_REDFISH_RISCOVER_HOST_INTERFACE case, we don’t skip any
> >    network interface even the subnet information is not
> >    retrieved.
> >
> > 3. Added some more debug information.
> >
> > Note: The subnet information is used for the scenario the system
> > is managed by a centralized Redfish service (not on BMC), says
> > the multiple Redfish computer system instances. As it mentions
> > in 31.1.5.2, Redfish client they may have to know the subnet
> > information so they can know the network domain the NIC is
> > connected. There may have multiple subnets in the corporation
> > network environment. So the subnet information provides client
> > an idea when they choose the network interface, so does VLAN ID.
> >
> > Signed-off-by: Abner Chang <abner.chang@amd.com>
> > Cc: Nickle Wang <nicklew@nvidia.com>
> > Cc: Igor Kulchytskyy <igork@ami.com>
> > Cc: Mike Maslenkin <mike.maslenkin@gmail.com>
> > ---
> >  .../RedfishDiscoverDxe/RedfishDiscoverDxe.c   | 105 ++++++------------
> >  1 file changed, 37 insertions(+), 68 deletions(-)
> >
> > diff --git a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> > index 833ae2b969f..1cfcbcdc794 100644
> > --- a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> > +++ b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> > @@ -487,43 +487,6 @@ CheckIsIpVersion6 (
> >    return FALSE;
> >  }
> >
> > -/**
> > -  This function returns the  IP type supported by the Host Interface.
> > -
> > -  @retval 00h is Unknown
> > -          01h is Ipv4
> > -          02h is Ipv6
> > -
> > -**/
> > -STATIC
> > -UINT8
> > -GetHiIpProtocolType (
> > -  VOID
> > -  )
> > -{
> > -  EFI_STATUS                     Status;
> > -  REDFISH_OVER_IP_PROTOCOL_DATA  *Data;
> > -  REDFISH_INTERFACE_DATA         *DeviceDescriptor;
> > -
> > -  Data             = NULL;
> > -  DeviceDescriptor = NULL;
> > -  if (mSmbios == NULL) {
> > -    Status = gBS->LocateProtocol (&gEfiSmbiosProtocolGuid, NULL, (VOID
> **)&mSmbios);
> > -    if (EFI_ERROR (Status)) {
> > -      return
> REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_UNKNOWN;
> > -    }
> > -  }
> > -
> > -  Status = RedfishGetHostInterfaceProtocolData (mSmbios,
> &DeviceDescriptor, &Data); // Search for SMBIOS type 42h
> > -  if (!EFI_ERROR (Status) && (Data != NULL) &&
> > -      (Data->HostIpAssignmentType == RedfishHostIpAssignmentStatic))
> > -  {
> > -    return Data->HostIpAddressFormat;
> > -  }
> > -
> > -  return
> REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_UNKNOWN;
> > -}
> > -
> >  /**
> >    Check if Network Protocol Type matches with SMBIOS Type 42 IP Address
> Type.
> >
> > @@ -583,7 +546,10 @@ DiscoverRedfishHostInterface (
> >    }
> >
> >    Status = RedfishGetHostInterfaceProtocolData (mSmbios,
> &DeviceDescriptor, &Data); // Search for SMBIOS type 42h
> > -  if (!EFI_ERROR (Status) && (Data != NULL) && (DeviceDescriptor != NULL))
> {
> > +  if (EFI_ERROR (Status) || (Data == NULL) || (DeviceDescriptor == NULL)) {
> > +    DEBUG ((DEBUG_ERROR, "%a: RedfishGetHostInterfaceProtocolData is
> failed.\n", __func__));
> > +    return Status;
> > +  } else {
> >      // Check IP Type and skip an unnecessary network protocol if does not
> match
> >      if (FilterProtocol (Instance->NetworkInterface->NetworkProtocolType,
> Data->HostIpAddressFormat)) {
> >        return EFI_UNSUPPORTED;
> > @@ -737,10 +703,7 @@ DiscoverRedfishHostInterface (
> >                   IsHttps
> >                   );
> >      }
> > -  } else {
> > -    DEBUG ((DEBUG_ERROR, "%a: RedfishGetHostInterfaceProtocolData is
> failed.\n", __func__));
> >    }
> > -
> >    return Status;
> >  }
> >
> > @@ -891,6 +854,12 @@ AddAndSignalNewRedfishService (
> >        DEBUG ((DEBUG_MANAGEABILITY, "Redfish service location: %s.\n",
> DiscoveredInstance->Information.Location));
> >      }
> >
> > +    if (UseHttps) {
> > +      DEBUG ((DEBUG_MANAGEABILITY, "Redfish service port: 443\n"));
> > +    } else {
> > +      DEBUG ((DEBUG_MANAGEABILITY, "Redfish service port: 80\n"));
> > +    }
> > +
>
> Do we really need such trace?
> It could be a bit misleading, because PcdRedfishServicePort  was
> introduced previously.
> May be we could move this to the caller function
> DiscoverRedfishHostInterface() where IsHttps was initialized?
Yes, move this to DiscoverRedfishHostInterface makes more sense to me as well.

>
>
> >      if (Uuid != NULL) {
> >        DiscoveredInstance->Information.Uuid = (CHAR16 *)AllocatePool
> (AsciiStrSize ((const CHAR8 *)Uuid) * sizeof (CHAR16));
> >        AsciiStrToUnicodeStrS ((const CHAR8 *)Uuid, DiscoveredInstance-
> >Information.Uuid, AsciiStrSize ((const CHAR8 *)Uuid) * sizeof (CHAR16));
> > @@ -1015,9 +984,11 @@ AddAndSignalNewRedfishService (
> >                           (EFI_REST_EX_CONFIG_DATA)(UINT8 *)RestExHttpConfigData
> >                           );
> >        if (EFI_ERROR (Status)) {
> > -        DEBUG ((DEBUG_ERROR, "%a:REST EX configured..\n", __func__));
> > +        DEBUG ((DEBUG_ERROR, "%a: REST EX is not configured..\n",
> __func__));
> >          DeleteRestEx = TRUE;
> >          goto EXIT_FREE_ALL;
> > +      } else {
> > +        DEBUG ((DEBUG_MANAGEABILITY, "%a: REST EX is configured..\n",
> __func__));
> >        }
> >
> >        //
> > @@ -1207,6 +1178,8 @@ RedfishServiceGetNetworkInterface (
> >    EFI_REDFISH_DISCOVER_NETWORK_INTERFACE
> *ThisNetworkInterface;
> >    EFI_REDFISH_DISCOVER_REST_EX_INSTANCE_INTERNAL   *RestExInstance;
> >
> > +  DEBUG ((DEBUG_MANAGEABILITY, "%a: Entry.\n", __func__));
> > +
> >    if ((This == NULL) || (NetworkIntfInstances == NULL) ||
> (NumberOfNetworkIntfs == NULL) ||
> >        (ImageHandle == NULL))
> >    {
> > @@ -1244,29 +1217,23 @@ RedfishServiceGetNetworkInterface (
> >    while (TRUE) {
> >      // If Get Subnet Info failed then skip this interface
> >      Status = NetworkInterfaceGetSubnetInfo (ThisNetworkInterfaceIntn,
> ImageHandle); // Get subnet info
> > -    if (EFI_ERROR (Status)) {
> > -      if (IsNodeAtEnd (&mEfiRedfishDiscoverNetworkInterface,
> &ThisNetworkInterfaceIntn->Entry)) {
> > -        break;
> > +    if (!EFI_ERROR (Status)) {
> > +      if (!ThisNetworkInterface->IsIpv6) {
> > +        IP4_COPY_ADDRESS (&ThisNetworkInterface->SubnetId.v4,
> &ThisNetworkInterfaceIntn->SubnetAddr.v4); // IPv4 subnet information.
> > +      } else {
> > +        IP6_COPY_ADDRESS (&ThisNetworkInterface->SubnetId.v6,
> &ThisNetworkInterfaceIntn->SubnetAddr.v6); // IPv6 subnet information in
> IPv6 address information.
> >        }
> >
> > -      ThisNetworkInterfaceIntn =
> (EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTERNAL *)GetNextNode
> (&mEfiRedfishDiscoverNetworkInterface, &ThisNetworkInterfaceIntn->Entry);
> > -      continue;
> > +      ThisNetworkInterface->SubnetPrefixLength = ThisNetworkInterfaceIntn-
> >SubnetPrefixLength;
> >      }
> >
> > +    CopyMem ((VOID *)&ThisNetworkInterface->MacAddress,
> &ThisNetworkInterfaceIntn->MacAddress, ThisNetworkInterfaceIntn-
> >HwAddressSize);
> >      ThisNetworkInterface->IsIpv6 = FALSE;
> >      if (CheckIsIpVersion6 (ThisNetworkInterfaceIntn)) {
> >        ThisNetworkInterface->IsIpv6 = TRUE;
> >      }
> >
> > -    CopyMem ((VOID *)&ThisNetworkInterface->MacAddress,
> &ThisNetworkInterfaceIntn->MacAddress, ThisNetworkInterfaceIntn-
> >HwAddressSize);
> > -    if (!ThisNetworkInterface->IsIpv6) {
> > -      IP4_COPY_ADDRESS (&ThisNetworkInterface->SubnetId.v4,
> &ThisNetworkInterfaceIntn->SubnetAddr.v4); // IPv4 subnet information.
> > -    } else {
> > -      IP6_COPY_ADDRESS (&ThisNetworkInterface->SubnetId.v6,
> &ThisNetworkInterfaceIntn->SubnetAddr.v6); // IPv6 subnet information in
> IPv6 address information.
> > -    }
> > -
> > -    ThisNetworkInterface->SubnetPrefixLength = ThisNetworkInterfaceIntn-
> >SubnetPrefixLength;
> > -    ThisNetworkInterface->VlanId             = ThisNetworkInterfaceIntn->VlanId;
> > +    ThisNetworkInterface->VlanId = ThisNetworkInterfaceIntn->VlanId;
> >      RestExInstance->NumberOfNetworkInterfaces++;
> >      if (IsNodeAtEnd (&mEfiRedfishDiscoverNetworkInterface,
> &ThisNetworkInterfaceIntn->Entry)) {
> >        break;
>
> As can be observed from this hunk the check " if
> (!ThisNetworkInterface->IsIpv6) " was moved before the
> ThisNetworkInterface->IsIpv6 initialization. Also I would consider
> removing IsIpv6 at all.
> ThisNetworkInterface->IsIpv6  depends on CheckIsIpVersion6() that is
> (ThisNetworkInterface->NetworkProtocolType == ProtocolTypeTcp6),
> so why do we need an additional variable with  the same meaning?
> ThisNetworkInterface->NetworkProtocolType is initialized early on
> RedfishDiscoverDriverBindingStart()->BuildupNetworkInterface() path,
> so it always has the correct value.
This is a mistake when I move around the code, we should set ThisNetworkInterface->IsIpv6 before copying subnet ID. We don’t encounter the problem because ThisNetworkInterface->IsIpv6 is initiated as 0 when AllocateZeroPool and we don’t have the Redfish service in IPv6 case, I guess.
Regarding the two variables with same meaning, one is for the output data according to the structure defined in spec (EFI_REDFISH_DISCOVER_NETWORK_INTERFACE ) and another one is the internal record of network interface.
I will submit the next version if this makes sense to you.

Thanks
Abner
>
>
> > @@ -1378,9 +1345,16 @@ RedfishServiceAcquireService (
> >        //
> >        Status1 = NetworkInterfaceGetSubnetInfo
> (TargetNetworkInterfaceInternal, ImageHandle);
> >        if (EFI_ERROR (Status1)) {
> > -        DEBUG ((DEBUG_ERROR, "%a: Get subnet information fail.\n",
> __func__));
> > -        FreePool (Instance);
> > -        continue;
> > +        //
> > +        // Get subnet information could be failed for
> EFI_REDFISH_DISCOVER_HOST_INTERFACE case.
> > +        // We will configure network in AddAndSignalNewRedfishService. So
> don't skip this
> > +        // target network interface.
> > +        //
> > +        if ((Flags & EFI_REDFISH_DISCOVER_HOST_INTERFACE) == 0) {
> > +          DEBUG ((DEBUG_ERROR, "%a: Get subnet information fail.\n",
> __func__));
> > +          FreePool (Instance);
> > +          continue;
> > +        }
> >        }
> >
> >        NewInstance = TRUE;
> > @@ -1448,6 +1422,7 @@ RedfishServiceAbortAcquire (
> >    IN EFI_REDFISH_DISCOVER_NETWORK_INTERFACE
> *TargetNetworkInterface OPTIONAL
> >    )
> >  {
> > +  DEBUG ((DEBUG_MANAGEABILITY, "%a: Entry.\n", __func__));
> >    // This function is used to abort Redfish service discovery through SSDP
> >    // on the network interface. SSDP is optionally suppoted by
> EFI_REDFISH_DISCOVER_PROTOCOL,
> >    // we dont have implementation for SSDP now.
> > @@ -1477,6 +1452,8 @@ RedfishServiceReleaseService (
> >    EFI_REDFISH_DISCOVERED_INSTANCE       *ThisRedfishInstance;
> >    EFI_REDFISH_DISCOVERED_INTERNAL_LIST  *DiscoveredRedfishInstance;
> >
> > +  DEBUG ((DEBUG_MANAGEABILITY, "%a: Entry.\n", __func__));
> > +
> >    if (IsListEmpty (&mRedfishInstanceList)) {
> >      DEBUG ((DEBUG_ERROR, "%a:No any discovered Redfish service.\n",
> __func__));
> >      return EFI_NOT_FOUND;
> > @@ -1723,22 +1700,13 @@ BuildupNetworkInterface (
> >    EFI_REDFISH_DISCOVER_REST_EX_INSTANCE_INTERNAL   *RestExInstance;
> >    EFI_TPL                                          OldTpl;
> >    BOOLEAN                                          NewNetworkInterfaceInstalled;
> > -  UINT8                                            IpType;
> >    UINTN                                            ListCount;
> >
> >    ListCount                    = (sizeof (mRequiredProtocol) / sizeof
> (REDFISH_DISCOVER_REQUIRED_PROTOCOL));
> >    NewNetworkInterfaceInstalled = FALSE;
> >    Index                        = 0;
> >
> > -  // 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 (FilterProtocol (mRequiredProtocol[Index].ProtocolType, IpType)) {
> > -      continue;
> > -    }
> > -
> >      Status = gBS->OpenProtocol (
> >                      // Already in list?
> >                      ControllerHandle,
> > @@ -2164,6 +2132,7 @@ RedfishDiscoverDriverBindingStart (
> >    IN EFI_DEVICE_PATH_PROTOCOL     *RemainingDevicePath OPTIONAL
> >    )
> >  {
> > +  DEBUG ((DEBUG_MANAGEABILITY, "%a:Entry.\n", __func__));
> >    return BuildupNetworkInterface (This, ControllerHandle);
> >  }
> >
> > --
> > 2.37.1.windows.1
> >
>
> I've tested this patch and it works for me.
>
> Regards,
> Mike.


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