[edk2-devel] [edk2-platforms] [PATCH v1 0/9] IpmiFeaturePkg: Add IPMI transport drivers

Michael Kubacki mikuback at linux.microsoft.com
Wed Mar 3 19:22:51 UTC 2021


Looked over the series at a high-level and the feature structure looks 
fine. For the series:
Acked-by: Michael Kubacki <michael.kubacki at microsoft.com>

I didn't look closely at implementation but there's a few things I noticed.

There's a few typos. I'd suggest running a spell check to clean those 
up. Some quick examples:

ServerManagement.h:

  "Generic Definations for Server Management Header File."

   typedef struct {
     LINERIZATION_TYPE Linearization;              // L

IpmiTransportPei.h:

   "IPMI Ttransport PPI Header File."

There was a mix of function documentation styles though that might be 
something to clean up later.

I saw some globals that did not appear necessary. For example, 
mIpmiTransport in SmmIpmiBaseLib.c seems to be located before every access.

Thanks,
Michael

On 3/1/2021 6:27 PM, Nate DeSimone wrote:
> From: Isaac Oram <isaac.w.oram at intel.com>
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3242
> 
> Implement an open source generic IPMI transport driver.
> Provides the ability to communicate with a BMC over IPMI
> in MinPlatform board packages.
> 
> New changes:
>    1. Added GenericIpmi
>    2. Added IPMI base libs
>    3. Added transport PPI and protocol
>    4. Updated IPMI command request and response data size from
>       UINT8 to UINT32 in IPMI transport layer to be compatible
>       with EDK2 industry standard IPMI commands.
>    6. Added the completion code in the first byte of all IPMI
>       response data to be compatible with EDK2 industry standard
>       IPMI commands.
> 
> Cc: Sai Chaganty <rangasai.v.chaganty at intel.com>
> Cc: Liming Gao <gaoliming at byosoft.com.cn>
> Cc: Michael Kubacki <michael.kubacki at microsoft.com>
> Signed-off-by: Isaac Oram <isaac.w.oram at intel.com>
> Co-authored-by: Nate DeSimone <nathaniel.l.desimone at intel.com>
> 
> Isaac Oram (9):
>    IpmiFeaturePkg: Add IPMI driver Include headers
>    IpmiFeaturePkg: Add IpmiBaseLib and IpmiCommandLib
>    IpmiFeaturePkg: Add IpmiInit drivers
>    IpmiFeaturePkg: Add GenericIpmi driver common code
>    IpmiFeaturePkg: Add GenericIpmi PEIM
>    IpmiFeaturePkg: Add GenericIpmi DXE Driver
>    IpmiFeaturePkg: Add GenericIpmi SMM Driver
>    IpmiFeaturePkg: Add IPMI driver build files
>    Maintainers.txt: Add IpmiFeaturePkg maintainers
> 
>   .../GenericIpmi/Common/IpmiBmc.c              | 297 +++++++++++
>   .../GenericIpmi/Common/IpmiBmc.h              |  44 ++
>   .../GenericIpmi/Common/IpmiBmcCommon.h        | 179 +++++++
>   .../GenericIpmi/Common/IpmiHooks.c            |  96 ++++
>   .../GenericIpmi/Common/IpmiHooks.h            |  81 +++
>   .../GenericIpmi/Common/IpmiPhysicalLayer.h    |  29 ++
>   .../GenericIpmi/Common/KcsBmc.c               | 483 ++++++++++++++++++
>   .../GenericIpmi/Common/KcsBmc.h               | 236 +++++++++
>   .../GenericIpmi/Dxe/GenericIpmi.c             |  46 ++
>   .../GenericIpmi/Dxe/GenericIpmi.inf           |  66 +++
>   .../IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c | 452 ++++++++++++++++
>   .../GenericIpmi/Pei/PeiGenericIpmi.c          | 362 +++++++++++++
>   .../GenericIpmi/Pei/PeiGenericIpmi.h          | 138 +++++
>   .../GenericIpmi/Pei/PeiGenericIpmi.inf        |  58 +++
>   .../GenericIpmi/Pei/PeiIpmiBmc.c              | 277 ++++++++++
>   .../GenericIpmi/Pei/PeiIpmiBmc.h              |  38 ++
>   .../GenericIpmi/Pei/PeiIpmiBmcDef.h           | 156 ++++++
>   .../GenericIpmi/Smm/SmmGenericIpmi.c          | 208 ++++++++
>   .../GenericIpmi/Smm/SmmGenericIpmi.inf        |  53 ++
>   .../IpmiFeaturePkg/Include/IpmiFeature.dsc    |   9 +-
>   .../Include/Library/IpmiBaseLib.h             |  50 ++
>   .../Include/Library/IpmiCommandLib.h          |  19 +-
>   .../Include/Ppi/IpmiTransportPpi.h            |  68 +++
>   .../Include/Protocol/IpmiTransportProtocol.h  |  75 +++
>   .../IpmiFeaturePkg/Include/ServerManagement.h | 337 ++++++++++++
>   .../IpmiFeaturePkg/Include/SmStatusCodes.h    |  98 ++++
>   .../IpmiFeaturePkg/IpmiFeaturePkg.dec         |  22 +-
>   .../IpmiFeaturePkg/IpmiInit/DxeIpmiInit.c     |   4 +-
>   .../IpmiFeaturePkg/IpmiInit/DxeIpmiInit.inf   |   6 +-
>   .../IpmiFeaturePkg/IpmiInit/PeiIpmiInit.c     |   4 +-
>   .../IpmiFeaturePkg/IpmiInit/PeiIpmiInit.inf   |   4 +-
>   .../Library/IpmiBaseLib/IpmiBaseLib.c         | 155 ++++++
>   .../Library/IpmiBaseLib/IpmiBaseLib.inf       |  28 +
>   .../Library/IpmiBaseLibNull/IpmiBaseLibNull.c |  76 +++
>   .../IpmiBaseLibNull/IpmiBaseLibNull.inf       |  36 ++
>   .../Library/IpmiCommandLib/IpmiCommandLib.inf |   4 +-
>   .../IpmiCommandLib/IpmiCommandLibNetFnApp.c   |   7 +-
>   .../IpmiCommandLibNetFnChassis.c              |  51 +-
>   .../IpmiCommandLibNetFnStorage.c              |   7 +-
>   .../IpmiCommandLibNetFnTransport.c            |   7 +-
>   .../Library/PeiIpmiBaseLib/PeiIpmiBaseLib.c   | 111 ++++
>   .../Library/PeiIpmiBaseLib/PeiIpmiBaseLib.inf |  30 ++
>   .../Library/SmmIpmiBaseLib/SmmIpmiBaseLib.c   | 180 +++++++
>   .../Library/SmmIpmiBaseLib/SmmIpmiBaseLib.inf |  29 ++
>   .../IpmiFeaturePkg/Readme.md                  |   4 +-
>   Maintainers.txt                               |   6 +
>   46 files changed, 4694 insertions(+), 32 deletions(-)
>   create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiBmc.c
>   create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiBmc.h
>   create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiBmcCommon.h
>   create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiHooks.c
>   create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiHooks.h
>   create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiPhysicalLayer.h
>   create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/KcsBmc.c
>   create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/KcsBmc.h
>   create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/GenericIpmi.c
>   create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/GenericIpmi.inf
>   create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c
>   create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiGenericIpmi.c
>   create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiGenericIpmi.h
>   create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiGenericIpmi.inf
>   create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiIpmiBmc.c
>   create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiIpmiBmc.h
>   create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiIpmiBmcDef.h
>   create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Smm/SmmGenericIpmi.c
>   create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Smm/SmmGenericIpmi.inf
>   create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Include/Library/IpmiBaseLib.h
>   create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Include/Ppi/IpmiTransportPpi.h
>   create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Include/Protocol/IpmiTransportProtocol.h
>   create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Include/ServerManagement.h
>   create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Include/SmStatusCodes.h
>   create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Library/IpmiBaseLib/IpmiBaseLib.c
>   create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Library/IpmiBaseLib/IpmiBaseLib.inf
>   create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Library/IpmiBaseLibNull/IpmiBaseLibNull.c
>   create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Library/IpmiBaseLibNull/IpmiBaseLibNull.inf
>   create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Library/PeiIpmiBaseLib/PeiIpmiBaseLib.c
>   create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Library/PeiIpmiBaseLib/PeiIpmiBaseLib.inf
>   create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Library/SmmIpmiBaseLib/SmmIpmiBaseLib.c
>   create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Library/SmmIpmiBaseLib/SmmIpmiBaseLib.inf
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#72401): https://edk2.groups.io/g/devel/message/72401
Mute This Topic: https://groups.io/mt/81016650/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-





More information about the edk2-devel-archive mailing list