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

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

[AMD Official Use Only - General]

Here is the PR https://github.com/tianocore/edk2/pull/5139 with Igor's RB for the V1.

Hi Igor, please check it again as I had moved the code around.

Thanks
Abner

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Chang,
> Abner via groups.io
> Sent: Monday, December 11, 2023 10:59 PM
> To: devel@edk2.groups.io
> Cc: Nickle Wang <nicklew@nvidia.com>; Igor Kulchytskyy <igork@ami.com>;
> Mike Maslenkin <mike.maslenkin@gmail.com>
> Subject: [edk2-devel] [PATCH V2] 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.
>
>
> 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.
>
> Change-Id: Ibb38ddcd17459ad4b23fcb4fcd8a771a0f63987a
> 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   | 109 +++++++-----------
>  1 file changed, 39 insertions(+), 70 deletions(-)
>
> diff --git a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> index 833ae2b969f..06d8d00da7f 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;
> @@ -675,6 +641,9 @@ DiscoverRedfishHostInterface (
>        IsHttps = FALSE;
>        if (Data->RedfishServiceIpPort == 443) {
>          IsHttps = TRUE;
> +        DEBUG ((DEBUG_MANAGEABILITY, "Redfish service port: 443\n"));
> +      } else {
> +        DEBUG ((DEBUG_MANAGEABILITY, "Redfish service port: 80\n"));
>        }
>
>        StrSize = sizeof (UuidStr);
> @@ -737,8 +706,6 @@ DiscoverRedfishHostInterface (
>                   IsHttps
>                   );
>      }
> -  } else {
> -    DEBUG ((DEBUG_ERROR, "%a: RedfishGetHostInterfaceProtocolData is
> failed.\n", __func__));
>    }
>
>    return Status;
> @@ -1015,9 +982,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 +1176,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))
>    {
> @@ -1242,31 +1213,27 @@ RedfishServiceGetNetworkInterface (
>
>    ThisNetworkInterfaceIntn =
> (EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTERNAL *)GetFirstNode
> (&mEfiRedfishDiscoverNetworkInterface);
>    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;
> -      }
> -
> -      ThisNetworkInterfaceIntn =
> (EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTERNAL *)GetNextNode
> (&mEfiRedfishDiscoverNetworkInterface, &ThisNetworkInterfaceIntn->Entry);
> -      continue;
> -    }
> -
>      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.
> +    //
> +    // If Get Subnet Info failed then skip this interface
> +    //
> +    Status = NetworkInterfaceGetSubnetInfo (ThisNetworkInterfaceIntn,
> ImageHandle); // Get subnet info
> +    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.
> +      }
> +
> +      ThisNetworkInterface->SubnetPrefixLength = ThisNetworkInterfaceIntn-
> >SubnetPrefixLength;
>      }
>
> -    ThisNetworkInterface->SubnetPrefixLength = ThisNetworkInterfaceIntn-
> >SubnetPrefixLength;
> -    ThisNetworkInterface->VlanId             = ThisNetworkInterfaceIntn->VlanId;
> +    ThisNetworkInterface->VlanId = ThisNetworkInterfaceIntn->VlanId;
>      RestExInstance->NumberOfNetworkInterfaces++;
>      if (IsNodeAtEnd (&mEfiRedfishDiscoverNetworkInterface,
> &ThisNetworkInterfaceIntn->Entry)) {
>        break;
> @@ -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
>
>
>
> 
>



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