Re: [PATCH 2/2] RedfishPkg: Redfish discover driver improvement

["Chang, Abner" <abner.chang@amd.com>]

[AMD Official Use Only - General]

Thanks for catching these bugs.
Other comments below in line.

> -----Original Message-----
> From: Nickle Wang <nicklew@nvidia.com>
> Sent: Wednesday, February 1, 2023 11:05 AM
> To: devel@edk2.groups.io
> Cc: Chang, Abner <Abner.Chang@amd.com>; Igor Kulchytskyy
> <igork@ami.com>; Nick Ramirez <nramirez@nvidia.com>
> Subject: [PATCH 2/2] RedfishPkg: Redfish discover driver improvement
> 
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
> 
> 
> Bug fix:
> - function stack fault
> - properly handle "SubnetAddrInfoIPv6" when there is no IPv6 support
> - copy-n-paste error in RedfishGetHostInterfaceProtocolData()
> Enhancement:
> - Redfish discover driver now can configure host IP address based on
>   the information from SMBIOS type 42 record. This saves the effort of
>   configuring host IP address in setup menu.
> - Performance improvement to driver binding process. Redfish discover
>   driver will wait until all required drivers are ready and do driver
>   binding start().
> - Use CopyGuid() to copy GUID instead of intrinsic function.
> 
> Signed-off-by: Nickle Wang <nicklew@nvidia.com>
> Cc: Abner Chang <abner.chang@amd.com>
> Cc: Igor Kulchytskyy <igork@ami.com>
> Cc: Nick Ramirez <nramirez@nvidia.com>
> ---
>  .../RedfishDiscoverInternal.h                 |  4 ++
>  .../RedfishDiscoverDxe/RedfishDiscoverDxe.c   | 52 +++++++++++++++----
>  .../RedfishSmbiosHostInterface.c              |  2 +-
>  3 files changed, 47 insertions(+), 11 deletions(-)
> 
> diff --git a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h
> b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h
> index 04fa09e1cc..3e1bfcdc4d 100644
> --- a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h
> +++ b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h
> @@ -3,6 +3,7 @@
> 
>    (C) Copyright 2021 Hewlett Packard Enterprise Development LP<BR>
>    Copyright (c) 2022, AMD Incorporated. All rights reserved.
> +  Copyright (c) 2023, NVIDIA CORPORATION & AFFILIATES. All rights reserved.
> 
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> @@ -182,6 +183,9 @@ typedef struct {
>    //
>    BOOLEAN                                            HostIntfValidation; ///< Indicates whether
> to validate Redfish Host interface.
>    EFI_IP_ADDRESS                                     TargetIpAddress;    ///< Target IP address
> reported in Redfish Host interface.
> +  UINT8                                              HostAddrFormat;     ///< Unknown=00h,
> Ipv4=01h, Ipv6=02h.
> +  EFI_IP_ADDRESS                                     HostIpAddress;      ///< Host IP address
> reported in Redfish Host interface.
> +  EFI_IP_ADDRESS                                     HostSubnetMask;     ///< Host subnet
> mask address reported in Redfish Host interface.
>  } EFI_REDFISH_DISCOVERED_INTERNAL_INSTANCE;
> 
>  /**
> diff --git a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> index 042d6d5fd5..e72ab71de8 100644
> --- a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> +++ b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> @@ -4,6 +4,7 @@
> 
>    (C) Copyright 2021 Hewlett Packard Enterprise Development LP<BR>
>    Copyright (c) 2022, AMD Incorporated. All rights reserved.
> +  Copyright (c) 2023, NVIDIA CORPORATION & AFFILIATES. All rights reserved.
> 
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> @@ -270,10 +271,13 @@ Tcp6GetSubnetInfo (
> 
>    if (IpModedata.AddressCount == 0) {
>      DEBUG ((DEBUG_INFO, "%a: No IPv6 address configured.\n",
> __FUNCTION__));
> +    Instance->SubnetAddrInfoIPv6Number = 0;
> +    return EFI_SUCCESS;
>    }
> 
>    if (Instance->SubnetAddrInfoIPv6 != NULL) {
>      FreePool (Instance->SubnetAddrInfoIPv6);
> +    Instance->SubnetAddrInfoIPv6 = NULL;
>    }
> 
>    Instance->SubnetAddrInfoIPv6 = AllocateZeroPool (IpModedata.AddressCount
> * sizeof (EFI_IP6_ADDRESS_INFO)); @@ -519,6 +523,14 @@
> DiscoverRedfishHostInterface (
>        return EFI_UNSUPPORTED;
>      }
> 
> +    Instance->HostAddrFormat = Data->HostIpAddressFormat;
> +    if (Data->HostIpAddressFormat == 1) {
> +      IP4_COPY_ADDRESS ((VOID *)&Instance->HostIpAddress.v4, (VOID *)Data-
> >HostIpAddress);
> +      IP4_COPY_ADDRESS ((VOID *)&Instance->HostSubnetMask.v4, (VOID
> *)Data->HostIpMask);
> +    } else if (Data->HostIpAddressFormat == 2) {
> +      IP6_COPY_ADDRESS ((VOID *)&Instance->HostIpAddress.v6, (VOID *)Data-
> >HostIpAddress);
> +    }
> +
>      if (Data->RedfishServiceIpAddressFormat == 1) {
>        IP4_COPY_ADDRESS ((VOID *)&Instance->TargetIpAddress.v4, (VOID *)Data-
> >RedfishServiceIpAddress);
>      } else {
> @@ -849,6 +861,10 @@ AddAndSignalNewRedfishService (
>            Status = EFI_OUT_OF_RESOURCES;
>            goto EXIT_FREE_CONFIG_DATA;
>          }
> +
> +        if (Instance->HostAddrFormat == 2) {
I am thinking if we have to return error if Instance->HostAddrFormat is not equal to 2 when LocalAddressIsIPv6 is TRUE?

> +          IP6_COPY_ADDRESS (&RestExHttpConfigData-
> >HttpConfigData.AccessPoint.IPv6Node->LocalAddress, &Instance-
> >HostIpAddress.v6);
> +        }
>        } else {
>          RestExHttpConfigData->HttpConfigData.AccessPoint.IPv4Node =
> AllocateZeroPool (sizeof (EFI_HTTPv4_ACCESS_POINT));
>          if (RestExHttpConfigData->HttpConfigData.AccessPoint.IPv4Node == NULL)
> { @@ -856,7 +872,13 @@ AddAndSignalNewRedfishService (
>            goto EXIT_FREE_CONFIG_DATA;
>          }
> 
> -        RestExHttpConfigData->HttpConfigData.AccessPoint.IPv4Node-
> >UseDefaultAddress = TRUE;
> +        if (Instance->HostAddrFormat == 1) {
> +          RestExHttpConfigData->HttpConfigData.AccessPoint.IPv4Node-
> >UseDefaultAddress = FALSE;
> +          IP4_COPY_ADDRESS (&RestExHttpConfigData-
> >HttpConfigData.AccessPoint.IPv4Node->LocalAddress, &Instance-
> >HostIpAddress.v4);
> +          IP4_COPY_ADDRESS (&RestExHttpConfigData-
> >HttpConfigData.AccessPoint.IPv4Node->LocalSubnet, &Instance-
> >HostSubnetMask.v4);
> +        } else {
> +          RestExHttpConfigData->HttpConfigData.AccessPoint.IPv4Node-
> >UseDefaultAddress = TRUE;
> +        }
>        }
> 
>        Status = RestEx->Configure (
> @@ -1467,11 +1489,13 @@ TestForRequiredProtocols (
>    IN EFI_HANDLE                   ControllerHandle
>    )
>  {
> -  UINT32      Id;
> +  UINT32      *Id;
>    UINTN       Index;
>    EFI_STATUS  Status;
> +  UINTN       ListCount;
> 
> -  for (Index = 0; Index < (sizeof (gRequiredProtocol) / sizeof
> (REDFISH_DISCOVER_REQUIRED_PROTOCOL)); Index++) {
> +  ListCount = (sizeof (gRequiredProtocol) / sizeof
> + (REDFISH_DISCOVER_REQUIRED_PROTOCOL));
> +  for (Index = 0; Index < ListCount; Index++) {
>      Status = gBS->OpenProtocol (
>                      ControllerHandle,
>                      gRequiredProtocol[Index].RequiredServiceBindingProtocolGuid,
> @@ -1490,8 +1514,10 @@ TestForRequiredProtocols (
>                        EFI_OPEN_PROTOCOL_GET_PROTOCOL
>                        );
>        if (EFI_ERROR (Status)) {
> -        DEBUG ((DEBUG_ERROR, "%a: %s is found on this controller handle.\n",
> __FUNCTION__, gRequiredProtocol[Index].ProtocolName));
> -        return EFI_SUCCESS;
> +        if (Index == ListCount - 1) {
I am not quite understand this change. Could you please elaborate more about this change? 
> +          DEBUG ((DEBUG_ERROR, "%a: all required protocols are found on this
> controller handle: %p.\n", __FUNCTION__, ControllerHandle));
> +          return EFI_SUCCESS;
> +        }
>        }
>      }
>    }
> @@ -1517,7 +1543,7 @@ BuildupNetworkInterface (
>    IN EFI_HANDLE                   ControllerHandle
>    )
>  {
> -  UINT32                                           Id;
> +  UINT32                                           *Id;
>    UINT32                                           Index;
>    EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTERNAL
> *NetworkInterface;
>    BOOLEAN                                          IsNew;
> @@ -1581,10 +1607,8 @@ BuildupNetworkInterface (
>        NetworkInterface->NetworkProtocolType                       =
> gRequiredProtocol[Index].ProtocolType;
>        NetworkInterface->OpenDriverAgentHandle                     = This-
> >DriverBindingHandle;
>        NetworkInterface->OpenDriverControllerHandle                = ControllerHandle;
> -      NetworkInterface->NetworkInterfaceProtocolInfo.ProtocolGuid = \
> -        *gRequiredProtocol[Index].RequiredProtocolGuid;
> -      NetworkInterface->NetworkInterfaceProtocolInfo.ProtocolServiceGuid = \
> -        *gRequiredProtocol[Index].RequiredServiceBindingProtocolGuid;
> +      CopyGuid (&NetworkInterface-
> >NetworkInterfaceProtocolInfo.ProtocolGuid,
> gRequiredProtocol[Index].RequiredProtocolGuid);
> +      CopyGuid
> + (&NetworkInterface->NetworkInterfaceProtocolInfo.ProtocolServiceGuid,
> + gRequiredProtocol[Index].RequiredServiceBindingProtocolGuid);
>        ProtocolDiscoverIdPtr        = &NetworkInterface-
> >NetworkInterfaceProtocolInfo.ProtocolDiscoverId;
>        OpenDriverAgentHandle        = NetworkInterface->OpenDriverAgentHandle;
>        OpenDriverControllerHandle   = NetworkInterface-
> >OpenDriverControllerHandle;
> @@ -1678,6 +1702,14 @@ BuildupNetworkInterface (
>            if (EFI_ERROR (Status)) {
>              DEBUG ((DEBUG_ERROR, "%a: Fail to install
> EFI_REDFISH_DISCOVER_PROTOCOL\n", __FUNCTION__));
>            }
> +        } else {
> +          DEBUG ((DEBUG_INFO, "%a, Not REST EX, continue with next\n",
> __FUNCTION__));
> +          Index++;
Space between Index and "++".

Thanks
Abner

> +          if (Index == (sizeof (gRequiredProtocol) / sizeof
> (REDFISH_DISCOVER_REQUIRED_PROTOCOL))) {
> +            break;
> +          }
> +
> +          continue;
>          }
>        }
> 
> diff --git a/RedfishPkg/RedfishDiscoverDxe/RedfishSmbiosHostInterface.c
> b/RedfishPkg/RedfishDiscoverDxe/RedfishSmbiosHostInterface.c
> index 88cec1f416..b25a222a88 100644
> --- a/RedfishPkg/RedfishDiscoverDxe/RedfishSmbiosHostInterface.c
> +++ b/RedfishPkg/RedfishDiscoverDxe/RedfishSmbiosHostInterface.c
> @@ -71,7 +71,7 @@ RedfishGetHostInterfaceProtocolData (
>            if (*RecordTmp ==
> REDFISH_HOST_INTERFACE_DEVICE_TYPE_PCI_PCIE_V2) {
>              ASSERT (SpecificDataLen == sizeof
> (PCI_OR_PCIE_INTERFACE_DEVICE_DESCRIPTOR_V2) + 1);
>            } else {
> -            ASSERT (SpecificDataLen > sizeof
> (REDFISH_HOST_INTERFACE_DEVICE_TYPE_USB_V2) + 1);
> +            ASSERT (SpecificDataLen == sizeof
> + (USB_INTERFACE_DEVICE_DESCRIPTOR_V2) + 1);
>            }
> 
>            *DeviceDescriptor = (REDFISH_INTERFACE_DATA *)RecordTmp;
> --
> 2.39.1.windows.1