[AMD Official Use Only - General]
> -----Original Message-----
> From: Oram, Isaac W <isaac.w.o...@intel.com>
> Sent: Thursday, February 16, 2023 9:48 AM
> To: Chang, Abner <abner.ch...@amd.com>; devel@edk2.groups.io
> Cc: Gao, Liming <gaolim...@byosoft.com.cn>; Desimone, Nathaniel L
> <nathaniel.l.desim...@intel.com>; Attar, AbdulLateef (Abdul Lateef)
> <abdullateef.at...@amd.com>; Nickle Wang <nick...@nvidia.com>; Igor
> Kulchytskyy <ig...@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.ch...@amd.com <abner.ch...@amd.com>
> Sent: Tuesday, February 7, 2023 8:23 AM
> To: devel@edk2.groups.io
> Cc: Gao, Liming <gaolim...@byosoft.com.cn>; Oram, Isaac W
> <isaac.w.o...@intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desim...@intel.com>; Abdul Lateef Attar
> <abdat...@amd.com>; Nickle Wang <nick...@nvidia.com>; Igor Kulchytskyy
> <ig...@ami.com>
> Subject: [edk2-platforms][PATCH 5/7] ManageabilityPkg: Implement Ipmi
> Protocol/Ppi
>
> From: Abner Chang <abner.ch...@amd.com>
>
> Add Ipmi Protocol/Ppi implementation. The underlying implementation is
> provided by IpmiBaseLib.
>
> Signed-off-by: Abner Chang <abner.ch...@amd.com>
> Cc: Liming Gao <gaolim...@byosoft.com.cn>
> Cc: Isaac Oram <isaac.w.o...@intel.com>
> Cc: Nate DeSimone <nathaniel.l.desim...@intel.com>
> Cc: Abdul Lateef Attar <abdat...@amd.com>
> Cc: Nickle Wang <nick...@nvidia.com>
> Cc: Igor Kulchytskyy <ig...@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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100305): https://edk2.groups.io/g/devel/message/100305
Mute This Topic: https://groups.io/mt/96810546/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
