Re: [PATCH 3/5] RedfishPkg: Create RestEx child on selected interface

[Minh Nguyen OS <minhnguyen@os.amperecomputing.com>]

[-- Attachment #1: Type: text/plain, Size: 11799 bytes --]

Hi Chang,

  It seems like my email has the problem, please expect the delay for my v2 patches while my email's fixing. Thanks for your patience.

Thanks,
Minh Nguyen
________________________________
From: Chang, Abner <Abner.Chang@amd.com>
Sent: Sunday, April 16, 2023 11:36 AM
To: Minh Nguyen OS <minhnguyen@os.amperecomputing.com>; devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Open Source Submission <patches@amperecomputing.com>; nicklew@nvidia.com <nicklew@nvidia.com>; igork@ami.com <igork@ami.com>; Nhi Pham OS <nhi@os.amperecomputing.com>; Tinh Nguyen OS <tinhnguyen@os.amperecomputing.com>; Vu Nguyen OS <vunguyen@os.amperecomputing.com>
Subject: RE: [PATCH 3/5] RedfishPkg: Create RestEx child on selected interface

[AMD Official Use Only - General]

Hi Minh,
That is weird I can't find this patch set on group.io, and I can't apply this patch from email neither. Could you please check it?

Thanks
Abner

> -----Original Message-----
> From: Minh Nguyen <minhnguyen@os.amperecomputing.com>
> Sent: Friday, April 14, 2023 4:19 PM
> To: devel@edk2.groups.io
> Cc: patches@amperecomputing.com; Chang, Abner
> <Abner.Chang@amd.com>; nicklew@nvidia.com; igork@ami.com;
> nhi@os.amperecomputing.com; tinhnguyen@os.amperecomputing.com; Vu
> Nguyen <vunguyen@os.amperecomputing.com>; Minh Nguyen
> <minhnguyen@os.amperecomputing.com>
> Subject: [PATCH 3/5] RedfishPkg: Create RestEx child on selected interface
>
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
>
>
> From: Vu Nguyen <vunguyen@os.amperecomputing.com>
>
> When a MAC address matching interface is found, a RestEx child will be
> created to provide the Redfish communication on that interface.
> Currently, It will try to locate all RestEx binding services and choose the first
> satisfied instance without taking care about current selected interface. This
> might raise an issue on the system with multiple network devices that the
> RestEx child was installed on wrong interface.
>
> Signed-off-by: Minh Nguyen <minhnguyen@os.amperecomputing.com>
> ---
>  RedfishPkg/Include/Library/RestExLib.h             |   3 +
>  RedfishPkg/Library/DxeRestExLib/DxeRestExLib.c     | 153 ++++++++----------
> --
>  RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c |   1 +
>  3 files changed, 63 insertions(+), 94 deletions(-)
>
> diff --git a/RedfishPkg/Include/Library/RestExLib.h
> b/RedfishPkg/Include/Library/RestExLib.h
> index bc4e4ca6caa7..2c32c3684cf4 100644
> --- a/RedfishPkg/Include/Library/RestExLib.h
> +++ b/RedfishPkg/Include/Library/RestExLib.h
> @@ -2,6 +2,7 @@
>    This library provides help functions for REST EX Protocol.
>
>    (C) Copyright 2020 Hewlett Packard Enterprise Development LP<BR>
> +  Copyright (c) 2023, Ampere Computing LLC. All rights reserved.<BR>
>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
>
> @@ -20,6 +21,7 @@
>    This function allows the caller to create child handle for specific
>    REST server.
>
> +  @param[in]  Controller           The controller handle used of selected
> interface.
>    @param[in]  Image                The image handle used to open service.
>    @param[in]  AccessMode           Access mode of REST server.
>    @param[in]  ConfigType           Underlying configuration to communicate
> with REST server.
> @@ -32,6 +34,7 @@
>  **/
>  EFI_STATUS
>  RestExLibCreateChild (
> +  IN EFI_HANDLE                       Controller,
>    IN EFI_HANDLE                       Image,
>    IN EFI_REST_EX_SERVICE_ACCESS_MODE  AccessMode,
>    IN EFI_REST_EX_CONFIG_TYPE          ConfigType,
> diff --git a/RedfishPkg/Library/DxeRestExLib/DxeRestExLib.c
> b/RedfishPkg/Library/DxeRestExLib/DxeRestExLib.c
> index d9acad24dec1..6002be93c54e 100644
> --- a/RedfishPkg/Library/DxeRestExLib/DxeRestExLib.c
> +++ b/RedfishPkg/Library/DxeRestExLib/DxeRestExLib.c
> @@ -2,6 +2,7 @@
>    This library provides help functions for REST EX Protocol.
>
>    (C) Copyright 2020 Hewlett Packard Enterprise Development LP<BR>
> +  Copyright (c) 2023, Ampere Computing LLC. All rights reserved.<BR>
>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
>
> @@ -9,6 +10,7 @@
>
>  #include <Uefi.h>
>  #include <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>
>  #include <Library/MemoryAllocationLib.h>  #include <Library/NetLib.h>
> #include <Library/UefiBootServicesTableLib.h>
> @@ -21,6 +23,7 @@
>    This function allows the caller to create child handle for specific
>    REST server.
>
> +  @param[in]  Controller           The controller handle used of selected
> interface.
>    @param[in]  Image                The image handle used to open service.
>    @param[in]  AccessMode           Access mode of REST server.
>    @param[in]  ConfigType           Underlying configuration to communicate
> with REST server.
> @@ -33,6 +36,7 @@
>  **/
>  EFI_STATUS
>  RestExLibCreateChild (
> +  IN EFI_HANDLE                       Controller,
>    IN EFI_HANDLE                       Image,
>    IN EFI_REST_EX_SERVICE_ACCESS_MODE  AccessMode,
>    IN EFI_REST_EX_CONFIG_TYPE          ConfigType,
> @@ -41,8 +45,6 @@ RestExLibCreateChild (
>    )
>  {
>    EFI_STATUS                Status;
> -  UINTN                     NoBuffer;
> -  EFI_HANDLE                *Handle;
>    EFI_HANDLE                ChildHandle;
>    EFI_REST_EX_PROTOCOL      *RestEx;
>    EFI_REST_EX_SERVICE_INFO  *RestExServiceInfo; @@ -59,116 +61,79 @@
> RestExLibCreateChild (
>    }
>
>    *ChildInstanceHandle = NULL;
> -  //
> -  // Locate all REST EX binding service.
> -  //
> -  Handle   = NULL;
> -  NoBuffer = 0;
> -  Status   = gBS->LocateHandleBuffer (
> -                    ByProtocol,
> -                    &gEfiRestExServiceBindingProtocolGuid,
> -                    NULL,
> -                    &NoBuffer,
> -                    &Handle
> -                    );
> -  if (EFI_ERROR (Status) && (Status != EFI_BUFFER_TOO_SMALL)) {
> +
> +  ChildHandle = NULL;
> +  Status = NetLibCreateServiceChild (
> +             Controller,
> +             Image,
> +             &gEfiRestExServiceBindingProtocolGuid,
> +             &ChildHandle
> +             );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "%a: Failed to create service child - %r \n",
> +      __FUNCTION__,
> +      Status
> +      ));
>      return Status;
>    }
> -
> -  Handle = (EFI_HANDLE *)AllocateZeroPool (sizeof (EFI_HANDLE) *
> NoBuffer);
> -  if (Handle == NULL) {
> -    return EFI_OUT_OF_RESOURCES;
> -  }
> -
> -  Status = gBS->LocateHandleBuffer (
> -                  ByProtocol,
> -                  &gEfiRestExServiceBindingProtocolGuid,
> +  Status = gBS->OpenProtocol (
> +                  ChildHandle,
> +                  &gEfiRestExProtocolGuid,
> +                  (VOID **)&RestEx,
> +                  Image,
>                    NULL,
> -                  &NoBuffer,
> -                  &Handle
> +                  EFI_OPEN_PROTOCOL_GET_PROTOCOL
>                    );
>    if (EFI_ERROR (Status)) {
> -    FreePool (Handle);
> -    return Status;
> +    goto ON_ERROR;
>    }
>
>    //
> -  // Search for the proper REST EX instance.
> +  // Get the information of REST service provided by this EFI REST EX
> + driver
>    //
> -  while (NoBuffer != 0) {
> -    ChildHandle = NULL;
> -    Status      = NetLibCreateServiceChild (
> -                    *(Handle + (NoBuffer - 1)),
> -                    Image,
> -                    &gEfiRestExServiceBindingProtocolGuid,
> -                    &ChildHandle
> -                    );
> -    if (!EFI_ERROR (Status)) {
> -      Status = gBS->OpenProtocol (
> -                      ChildHandle,
> -                      &gEfiRestExProtocolGuid,
> -                      (VOID **)&RestEx,
> -                      Image,
> -                      NULL,
> -                      EFI_OPEN_PROTOCOL_GET_PROTOCOL
> -                      );
> -      if (EFI_ERROR (Status)) {
> -        goto ON_ERROR;
> -      }
> -
> -      //
> -      // Get the information of REST service provided by this EFI REST EX driver
> -      //
> -      Status = RestEx->GetService (
> -                         RestEx,
> -                         &RestExServiceInfo
> -                         );
> -      if (EFI_ERROR (Status)) {
> -        goto ON_ERROR;
> -      }
> -
> -      //
> -      // Check REST EX property.
> -      //
> -      switch (ConfigType) {
> -        case EfiRestExConfigHttp:
> -          LenOfConfig = sizeof (EFI_REST_EX_HTTP_CONFIG_DATA);
> -          break;
> -
> -        case EfiRestExConfigUnspecific:
> -          LenOfConfig = REST_EX_CONFIG_DATA_LEN_UNKNOWN;
> -          break;
> +  Status = RestEx->GetService (
> +                     RestEx,
> +                     &RestExServiceInfo
> +                     );
> +  if (EFI_ERROR (Status)) {
> +    goto ON_ERROR;
> +  }
> +  //
> +  // Check REST EX property.
> +  //
> +  switch (ConfigType) {
> +  case EfiRestExConfigHttp:
> +    LenOfConfig = sizeof (EFI_REST_EX_HTTP_CONFIG_DATA);
> +    break;
>
> -        default:
> -          goto ON_ERROR;
> -      }
> +  case EfiRestExConfigUnspecific:
> +    LenOfConfig = REST_EX_CONFIG_DATA_LEN_UNKNOWN;
> +    break;
>
> -      if ((RestExServiceInfo-
> >EfiRestExServiceInfoV10.RestServiceAccessMode != AccessMode) ||
> -          (RestExServiceInfo->EfiRestExServiceInfoV10.RestServiceType !=
> ServiceType) ||
> -          (RestExServiceInfo->EfiRestExServiceInfoV10.RestExConfigType !=
> ConfigType) ||
> -          ((LenOfConfig != REST_EX_CONFIG_DATA_LEN_UNKNOWN) &&
> (RestExServiceInfo->EfiRestExServiceInfoV10.RestExConfigDataLength !=
> LenOfConfig)))
> -      {
> -        goto ON_ERROR;
> -      }
> -    }
> +  default:
> +    goto ON_ERROR;
> +  }
> +  if (RestExServiceInfo->EfiRestExServiceInfoV10.RestServiceAccessMode !=
> AccessMode ||
> +      RestExServiceInfo->EfiRestExServiceInfoV10.RestServiceType !=
> ServiceType ||
> +      RestExServiceInfo->EfiRestExServiceInfoV10.RestExConfigType !=
> ConfigType ||
> +      ((LenOfConfig != REST_EX_CONFIG_DATA_LEN_UNKNOWN) &&
> (RestExServiceInfo->EfiRestExServiceInfoV10.RestExConfigDataLength !=
> LenOfConfig))) {
> +    goto ON_ERROR;
> +  }
>
> -    //
> -    // This is proper REST EX instance.
> -    //
> -    *ChildInstanceHandle = ChildHandle;
> -    FreePool (Handle);
> -    return EFI_SUCCESS;
> +  //
> +  // This is proper REST EX instance.
> +  //
> +  *ChildInstanceHandle = ChildHandle;
> +  return EFI_SUCCESS;
>
>  ON_ERROR:;
>      NetLibDestroyServiceChild (
> -      *(Handle + (NoBuffer - 1)),
> +      Controller,
>        Image,
>        &gEfiRestExServiceBindingProtocolGuid,
>        ChildHandle
>        );
> -    NoBuffer--;
> -  }
> -
> -  FreePool (Handle);
>    return EFI_NOT_FOUND;
>  }
> diff --git a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> index 29e7b0624ebf..86ec88ddafc4 100644
> --- a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> +++ b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> @@ -87,6 +87,7 @@ CreateRestExInstance (
>    EFI_STATUS  Status;
>
>    Status = RestExLibCreateChild (
> +             Instance->NetworkInterface->OpenDriverControllerHandle,
>               Instance->Owner,
>               FixedPcdGetBool (PcdRedfishDiscoverAccessModeInBand) ?
> EfiRestExServiceInBandAccess : EfiRestExServiceOutOfBandAccess,
>               EfiRestExConfigHttp,
> --
> 2.39.0

[-- Attachment #2: Type: text/html, Size: 24119 bytes --]