Re: [edk2-platforms][PATCH 5/7] ManageabilityPkg: Implement Ipmi Protocol/Ppi

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

[AMD Official Use Only - General]



> -----Original Message-----
> From: Oram, Isaac W <isaac.w.oram@intel.com>
> Sent: Thursday, February 16, 2023 9:48 AM
> To: Chang, Abner <Abner.Chang@amd.com>; devel@edk2.groups.io
> Cc: Gao, Liming <gaoliming@byosoft.com.cn>; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>; Attar, AbdulLateef (Abdul Lateef)
> <AbdulLateef.Attar@amd.com>; Nickle Wang <nicklew@nvidia.com>; Igor
> Kulchytskyy <igork@ami.com>
> Subject: RE: [edk2-platforms][PATCH 5/7] ManageabilityPkg: Implement Ipmi
> Protocol/Ppi
> 
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
> 
> 
> I find the code to be misleading because source code looks like it is using the
> edk2 API but it actually invokes the IpmiFeaturePkg implementations of the
> IpmiTransport* API.  we end up with things like a depex that says TRUE, but
> really includes a library that carries a dependency on a different protocol than
> the one in the INF.  It includes IpmiProtocol.h, but actually invokes a library
> that locates and uses IpmiTransportProtocol.h.
> 
> I think that we should have these implement IpmiPpi.h and IpmiProtocol.h
> instead of chaining them into the old implementation.
Agree as my feedback given to patch 1/7.
Abner

> 
> If we don't want to implement them directly, I would prefer to obviously
> have the IpmiPpi and IpmiProtocol implementations look up and invoke
> IpmiTransportPpi and IpmiTransportProtocol directly so that the code is
> obvious about what it is doing.  It seems that IpmiBaseLib only abstracts the
> phase for calling code.  Is there any other value for phase specific code to
> obfuscate what is really happening by adding the library layer?  If the library is
> there to avoid source code collisions, then I guess detailed commenting at all
> the misleading places is probably the only answer besides implementing
> directly.
> 
> Regards,
> Isaac
> 
> -----Original Message-----
> From: abner.chang@amd.com <abner.chang@amd.com>
> Sent: Tuesday, February 7, 2023 8:23 AM
> To: devel@edk2.groups.io
> Cc: Gao, Liming <gaoliming@byosoft.com.cn>; Oram, Isaac W
> <isaac.w.oram@intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>; Abdul Lateef Attar
> <abdattar@amd.com>; Nickle Wang <nicklew@nvidia.com>; Igor Kulchytskyy
> <igork@ami.com>
> Subject: [edk2-platforms][PATCH 5/7] ManageabilityPkg: Implement Ipmi
> Protocol/Ppi
> 
> From: Abner Chang <abner.chang@amd.com>
> 
> Add Ipmi Protocol/Ppi implementation. The underlying implementation is
> provided by IpmiBaseLib.
> 
> Signed-off-by: Abner Chang <abner.chang@amd.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Isaac Oram <isaac.w.oram@intel.com>
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Cc: Abdul Lateef Attar <abdattar@amd.com>
> Cc: Nickle Wang <nicklew@nvidia.com>
> Cc: Igor Kulchytskyy <igork@ami.com>
> ---
>  .../IpmiProtocol/Dxe/IpmiProtocolDxe.inf      |  37 +++++++
>  .../Universal/IpmiProtocol/Pei/IpmiPpiPei.inf |  38 +++++++
>  .../IpmiProtocol/Smm/IpmiProtocolSmm.inf      |  40 +++++++
>  .../Universal/IpmiProtocol/Dxe/IpmiProtocol.c |  97 +++++++++++++++++
>  .../Universal/IpmiProtocol/Pei/IpmiPpi.c      | 102 ++++++++++++++++++
>  .../Universal/IpmiProtocol/Smm/IpmiProtocol.c |  98 +++++++++++++++++
>  6 files changed, 412 insertions(+)
>  create mode 100644
> Features/ManageabilityPkg/Universal/IpmiProtocol/Dxe/IpmiProtocolDxe.in
> f
>  create mode 100644
> Features/ManageabilityPkg/Universal/IpmiProtocol/Pei/IpmiPpiPei.inf
>  create mode 100644
> Features/ManageabilityPkg/Universal/IpmiProtocol/Smm/IpmiProtocolSmm
> .inf
>  create mode 100644
> Features/ManageabilityPkg/Universal/IpmiProtocol/Dxe/IpmiProtocol.c
>  create mode 100644
> Features/ManageabilityPkg/Universal/IpmiProtocol/Pei/IpmiPpi.c
>  create mode 100644
> Features/ManageabilityPkg/Universal/IpmiProtocol/Smm/IpmiProtocol.c
> 
> diff --git
> a/Features/ManageabilityPkg/Universal/IpmiProtocol/Dxe/IpmiProtocolDxe.
> inf
> b/Features/ManageabilityPkg/Universal/IpmiProtocol/Dxe/IpmiProtocolDxe
> .inf
> new file mode 100644
> index 0000000000..0737a5ad8f
> --- /dev/null
> +++
> b/Features/ManageabilityPkg/Universal/IpmiProtocol/Dxe/IpmiProtocolD
> +++ xe.inf
> @@ -0,0 +1,37 @@
> +## @file
> +# IPMI Protocol DXE Driver.
> +#
> +# Copyright (C) 2023 Advanced Micro Devices, Inc. All rights
> +reserved.<BR> # SPDX-License-Identifier: BSD-2-Clause-Patent ##
> +
> +[Defines]
> +  INF_VERSION                    = 0x0001001d
> +  BASE_NAME                      = IpmiDxe
> +  FILE_GUID                      = BC41B0C2-9D8A-42B5-A28F-02CE0D4A6C28
> +  MODULE_TYPE                    = DXE_DRIVER
> +  VERSION_STRING                 = 1.0
> +  ENTRY_POINT                    = IpmiEntry
> +
> +#
> +#  VALID_ARCHITECTURES           = IA32 X64 ARM AARCH64
> +#
> +
> +[Sources]
> +  IpmiProtocol.c
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  IpmiFeaturePkg/IpmiFeaturePkg.dec
> +
> +[LibraryClasses]
> +  IpmiBaseLib
> +  UefiDriverEntryPoint
> +  UefiBootServicesTableLib
> +
> +[Protocols]
> +  gIpmiProtocolGuid               # PROTOCOL ALWAYS_PRODUCED
> +
> +[Depex]
> +  TRUE
> diff --git
> a/Features/ManageabilityPkg/Universal/IpmiProtocol/Pei/IpmiPpiPei.inf
> b/Features/ManageabilityPkg/Universal/IpmiProtocol/Pei/IpmiPpiPei.inf
> new file mode 100644
> index 0000000000..7ba8584f84
> --- /dev/null
> +++ b/Features/ManageabilityPkg/Universal/IpmiProtocol/Pei/IpmiPpiPei.in
> +++ f
> @@ -0,0 +1,38 @@
> +## @file
> +# IPMI Protocol PEI Driver.
> +#
> +# Copyright (C) 2023 Advanced Micro Devices, Inc. All rights
> +reserved.<BR> # SPDX-License-Identifier: BSD-2-Clause-Patent ##
> +
> +[Defines]
> +  INF_VERSION                    = 0x0001001d
> +  BASE_NAME                      = IpmiPei
> +  FILE_GUID                      = 7832F989-CB72-4715-ADCA-35C0B031856C
> +  MODULE_TYPE                    = PEIM
> +  VERSION_STRING                 = 1.0
> +  ENTRY_POINT                    = IpmiEntry
> +
> +#
> +# The following information is for reference only and not required by the
> build tools.
> +#
> +#  VALID_ARCHITECTURES           = IA32 X64 ARM AARCH64
> +#
> +
> +[Sources]
> +  IpmiPpi.c
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  IpmiFeaturePkg/IpmiFeaturePkg.dec
> +
> +[LibraryClasses]
> +  IpmiBaseLib
> +  PeimEntryPoint
> +
> +[Ppis]
> +  gPeiIpmiPpiGuid       # PPI ALWAYS PRODUCED
> +
> +[Depex]
> +  TRUE
> diff --git
> a/Features/ManageabilityPkg/Universal/IpmiProtocol/Smm/IpmiProtocolSm
> m.inf
> b/Features/ManageabilityPkg/Universal/IpmiProtocol/Smm/IpmiProtocolSm
> m.inf
> new file mode 100644
> index 0000000000..25a5771cb3
> --- /dev/null
> +++
> b/Features/ManageabilityPkg/Universal/IpmiProtocol/Smm/IpmiProtocolS
> +++ mm.inf
> @@ -0,0 +1,40 @@
> +## @file
> +# IPMI Protocol SMM Driver.
> +#
> +# Copyright (C) 2023 Advanced Micro Devices, Inc. All rights
> +reserved.<BR> # SPDX-License-Identifier: BSD-2-Clause-Patent ##
> +
> +#
> +# The following information is for reference only and not required by the
> build tools.
> +#
> +#  VALID_ARCHITECTURES           = IA32 X64 ARM AARCH64
> +#
> +[Defines]
> +  INF_VERSION                         = 0x0001001d
> +  BASE_NAME                           = IpmiSmm
> +  FILE_GUID                           = CDD5D1DE-E3D3-4B1F-8689-DCC661561BB4
> +  MODULE_TYPE                         = DXE_SMM_DRIVER
> +  PI_SPECIFICATION_VERSION            = 0x0001000A
> +  VERSION_STRING                      = 1.0
> +  ENTRY_POINT                         = IpmiEntry
> +
> +[Sources]
> +  IpmiProtocol.c
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  IpmiFeaturePkg/IpmiFeaturePkg.dec
> +
> +[LibraryClasses]
> +  IpmiBaseLib
> +  UefiDriverEntryPoint
> +  UefiBootServicesTableLib
> +  SmmServicesTableLib
> +
> +[Protocols]
> +  gSmmIpmiProtocolGuid               # PROTOCOL ALWAYS_PRODUCED
> +
> +[Depex]
> +  TRUE
> diff --git
> a/Features/ManageabilityPkg/Universal/IpmiProtocol/Dxe/IpmiProtocol.c
> b/Features/ManageabilityPkg/Universal/IpmiProtocol/Dxe/IpmiProtocol.c
> new file mode 100644
> index 0000000000..1f0b88052e
> --- /dev/null
> +++ b/Features/ManageabilityPkg/Universal/IpmiProtocol/Dxe/IpmiProtocol.
> +++ c
> @@ -0,0 +1,97 @@
> +/** @file
> +  This file provides IPMI Protocol implementation.
> +
> +  Copyright (C) 2023 Advanced Micro Devices, Inc. All rights
> +reserved.<BR>
> +  SPDX-License-Identifier: BSD-2-Clause-Patent **/
> +
> +#include <PiDxe.h>
> +#include <Library/DebugLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Library/IpmiBaseLib.h>
> +#include <Protocol/IpmiProtocol.h>
> +
> +/**
> +  This service enables submitting commands via Ipmi.
> +
> +  @param[in]         This              This point for IPMI_PROTOCOL structure.
> +  @param[in]         NetFunction       Net function of the command.
> +  @param[in]         Command           IPMI Command.
> +  @param[in]         RequestData       Command Request Data.
> +  @param[in]         RequestDataSize   Size of Command Request Data.
> +  @param[out]        ResponseData      Command Response Data. The
> completion code is the first byte of response data.
> +  @param[in, out]    ResponseDataSize  Size of Command Response Data.
> +
> +  @retval EFI_SUCCESS            The command byte stream was successfully
> submit to the device and a response was successfully received.
> +  @retval EFI_NOT_FOUND          The command was not successfully sent to
> the device or a response was not successfully received from the device.
> +  @retval EFI_NOT_READY          Ipmi Device is not ready for Ipmi command
> access.
> +  @retval EFI_DEVICE_ERROR       Ipmi Device hardware error.
> +  @retval EFI_TIMEOUT            The command time out.
> +  @retval EFI_UNSUPPORTED        The command was not successfully sent to
> the device.
> +  @retval EFI_OUT_OF_RESOURCES   The resource allcation is out of
> resource or data size error.
> +**/
> +EFI_STATUS
> +EFIAPI
> +IpmiSubmitCommand (
> +  IN     IPMI_PROTOCOL  *This,
> +  IN     UINT8          NetFunction,
> +  IN     UINT8          Command,
> +  IN     UINT8          *RequestData,
> +  IN     UINT32         RequestDataSize,
> +  OUT    UINT8          *ResponseData,
> +  IN OUT UINT32         *ResponseDataSize
> +  )
> +{
> +  EFI_STATUS  Status;
> +
> +  Status = IpmiSendCommand (
> +             NetFunction,
> +             Command,
> +             RequestData,
> +             RequestDataSize,
> +             ResponseData,
> +             ResponseDataSize
> +             );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "%a: Failed to send IPMI command in DXE -
> + %r\n", __FUNCTION__, Status));  }
> +
> +  return Status;
> +}
> +
> +static IPMI_PROTOCOL  mIpmiProtocol = {
> +  IpmiSubmitCommand
> +};
> +
> +/**
> +  The entry point of the Ipmi DXE driver.
> +
> +  @param[in] ImageHandle - Handle of this driver image  @param[in]
> + SystemTable - Table containing standard EFI services
> +
> +  @retval EFI_SUCCESS    - IPMI Protocol is installed successfully.
> +  @retval Otherwise      - Other errors.
> +**/
> +EFI_STATUS
> +EFIAPI
> +IpmiEntry (
> +  IN EFI_HANDLE        ImageHandle,
> +  IN EFI_SYSTEM_TABLE  *SystemTable
> +  )
> +{
> +  EFI_STATUS  Status;
> +  EFI_HANDLE  Handle;
> +
> +  Handle = NULL;
> +  Status = gBS->InstallProtocolInterface (
> +                  &Handle,
> +                  &gIpmiProtocolGuid,
> +                  EFI_NATIVE_INTERFACE,
> +                  (VOID **)&mIpmiProtocol
> +                  );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "%a: Failed to install IPMI protocol - %r\n",
> + __FUNCTION__, Status));  }
> +
> +  return Status;
> +}
> diff --git a/Features/ManageabilityPkg/Universal/IpmiProtocol/Pei/IpmiPpi.c
> b/Features/ManageabilityPkg/Universal/IpmiProtocol/Pei/IpmiPpi.c
> new file mode 100644
> index 0000000000..913a9b0811
> --- /dev/null
> +++ b/Features/ManageabilityPkg/Universal/IpmiProtocol/Pei/IpmiPpi.c
> @@ -0,0 +1,102 @@
> +/** @file
> +  This file provides IPMI PPI implementation.
> +
> +  Copyright (C) 2023 Advanced Micro Devices, Inc. All rights
> + reserved.<BR>
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <PiPei.h>
> +#include <Library/DebugLib.h>
> +#include <Library/PeiServicesLib.h>
> +#include <Library/IpmiBaseLib.h>
> +#include <Ppi/IpmiPpi.h>
> +
> +/**
> +  This service enables submitting commands via Ipmi.
> +
> +  @param[in]         This              This point for PEI_IPMI_PPI structure.
> +  @param[in]         NetFunction       Net function of the command.
> +  @param[in]         Command           IPMI Command.
> +  @param[in]         RequestData       Command Request Data.
> +  @param[in]         RequestDataSize   Size of Command Request Data.
> +  @param[out]        ResponseData      Command Response Data. The
> completion code is the first byte of response data.
> +  @param[in, out]    ResponseDataSize  Size of Command Response Data.
> +
> +  @retval EFI_SUCCESS            The command byte stream was successfully
> submit to the device and a response was successfully received.
> +  @retval EFI_NOT_FOUND          The command was not successfully sent to
> the device or a response was not successfully received from the device.
> +  @retval EFI_NOT_READY          Ipmi Device is not ready for Ipmi command
> access.
> +  @retval EFI_DEVICE_ERROR       Ipmi Device hardware error.
> +  @retval EFI_TIMEOUT            The command time out.
> +  @retval EFI_UNSUPPORTED        The command was not successfully sent to
> the device.
> +  @retval EFI_OUT_OF_RESOURCES   The resource allcation is out of
> resource or data size error.
> +**/
> +EFI_STATUS
> +EFIAPI
> +IpmiSubmitCommand (
> +  IN     PEI_IPMI_PPI  *This,
> +  IN     UINT8         NetFunction,
> +  IN     UINT8         Command,
> +  IN     UINT8         *RequestData,
> +  IN     UINT32        RequestDataSize,
> +  OUT    UINT8         *ResponseData,
> +  IN OUT UINT32        *ResponseDataSize
> +  )
> +{
> +  EFI_STATUS  Status;
> +
> +  Status = IpmiSendCommand (
> +             NetFunction,
> +             Command,
> +             RequestData,
> +             RequestDataSize,
> +             ResponseData,
> +             ResponseDataSize
> +             );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "%a: Failed to send IPMI command in PEI-
> + %r\n", __FUNCTION__, Status));  }
> +
> +  return Status;
> +}
> +
> +static PEI_IPMI_PPI  mPeiIpmiPpi = {
> +  IpmiSubmitCommand
> +};
> +
> +static EFI_PEI_PPI_DESCRIPTOR  mIpmiPpiList[] = {
> +  {
> +    (EFI_PEI_PPI_DESCRIPTOR_PPI |
> EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST),
> +    &gPeiIpmiPpiGuid,
> +    &mPeiIpmiPpi
> +  }
> +};
> +
> +/**
> +  The entry point of the Ipmi PPI PEIM.
> +
> +  @param  FileHandle  Handle of the file being invoked.
> +  @param  PeiServices Describes the list of possible PEI Services.
> +
> +  @retval EFI_SUCCESS   Indicates that Ipmi initialization completed
> successfully.
> +  @retval Others        Indicates that Ipmi initialization could not complete
> successfully.
> +**/
> +EFI_STATUS
> +EFIAPI
> +IpmiEntry (
> +  IN       EFI_PEI_FILE_HANDLE  FileHandle,
> +  IN CONST EFI_PEI_SERVICES     **PeiServices
> +  )
> +{
> +  EFI_STATUS  Status;
> +
> +  //
> +  // Install IPMI PPI.
> +  //
> +  Status = PeiServicesInstallPpi (&mIpmiPpiList[0]);  if (EFI_ERROR
> + (Status)) {
> +    DEBUG ((DEBUG_ERROR, "%a: Failed to install IPMI PPI - %r\n",
> + __FUNCTION__, Status));  }
> +
> +  return Status;
> +}
> diff --git
> a/Features/ManageabilityPkg/Universal/IpmiProtocol/Smm/IpmiProtocol.c
> b/Features/ManageabilityPkg/Universal/IpmiProtocol/Smm/IpmiProtocol.c
> new file mode 100644
> index 0000000000..ed14f9fbd1
> --- /dev/null
> +++
> b/Features/ManageabilityPkg/Universal/IpmiProtocol/Smm/IpmiProtocol.
> +++ c
> @@ -0,0 +1,98 @@
> +/** @file
> +  This file provides IPMI SMM Protocol implementation.
> +
> +  Copyright (C) 2023 Advanced Micro Devices, Inc. All rights
> +reserved.<BR>
> +  SPDX-License-Identifier: BSD-2-Clause-Patent **/
> +
> +#include <PiDxe.h>
> +#include <Library/DebugLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Library/SmmServicesTableLib.h> #include
> +<Library/IpmiBaseLib.h> #include <Protocol/IpmiProtocol.h>
> +
> +/**
> +  This service enables submitting commands via Ipmi.
> +
> +  @param[in]         This              This point for IPMI_PROTOCOL structure.
> +  @param[in]         NetFunction       Net function of the command.
> +  @param[in]         Command           IPMI Command.
> +  @param[in]         RequestData       Command Request Data.
> +  @param[in]         RequestDataSize   Size of Command Request Data.
> +  @param[out]        ResponseData      Command Response Data. The
> completion code is the first byte of response data.
> +  @param[in, out]    ResponseDataSize  Size of Command Response Data.
> +
> +  @retval EFI_SUCCESS            The command byte stream was successfully
> submit to the device and a response was successfully received.
> +  @retval EFI_NOT_FOUND          The command was not successfully sent to
> the device or a response was not successfully received from the device.
> +  @retval EFI_NOT_READY          Ipmi Device is not ready for Ipmi command
> access.
> +  @retval EFI_DEVICE_ERROR       Ipmi Device hardware error.
> +  @retval EFI_TIMEOUT            The command time out.
> +  @retval EFI_UNSUPPORTED        The command was not successfully sent to
> the device.
> +  @retval EFI_OUT_OF_RESOURCES   The resource allcation is out of
> resource or data size error.
> +**/
> +EFI_STATUS
> +EFIAPI
> +IpmiSubmitCommand (
> +  IN     IPMI_PROTOCOL  *This,
> +  IN     UINT8          NetFunction,
> +  IN     UINT8          Command,
> +  IN     UINT8          *RequestData,
> +  IN     UINT32         RequestDataSize,
> +  OUT    UINT8          *ResponseData,
> +  IN OUT UINT32         *ResponseDataSize
> +  )
> +{
> +  EFI_STATUS  Status;
> +
> +  Status = IpmiSendCommand (
> +             NetFunction,
> +             Command,
> +             RequestData,
> +             RequestDataSize,
> +             ResponseData,
> +             ResponseDataSize
> +             );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "%a: Failed to send IPMI command in DXE -
> + %r\n", __FUNCTION__, Status));  }
> +
> +  return Status;
> +}
> +
> +static IPMI_PROTOCOL  mIpmiProtocol = {
> +  IpmiSubmitCommand
> +};
> +
> +/**
> +  The entry point of the Ipmi DXE driver.
> +
> +  @param[in] ImageHandle - Handle of this driver image  @param[in]
> + SystemTable - Table containing standard EFI services
> +
> +  @retval EFI_SUCCESS    - IPMI Protocol is installed successfully.
> +  @retval Otherwise      - Other errors.
> +**/
> +EFI_STATUS
> +EFIAPI
> +IpmiEntry (
> +  IN EFI_HANDLE        ImageHandle,
> +  IN EFI_SYSTEM_TABLE  *SystemTable
> +  )
> +{
> +  EFI_STATUS  Status;
> +  EFI_HANDLE  Handle;
> +
> +  Handle = NULL;
> +  Status = gSmst->SmmInstallProtocolInterface (
> +                    &Handle,
> +                    &gSmmIpmiProtocolGuid,
> +                    EFI_NATIVE_INTERFACE,
> +                    (VOID **)&mIpmiProtocol
> +                    );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "%a: Failed to install IPMI SMM protocol -
> + %r\n", __FUNCTION__, Status));  }
> +
> +  return Status;
> +}
> --
> 2.37.1.windows.1