From: Isaac Oram <isaac.w.oram@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@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Michael Kubacki <michael.kubacki@microsoft.com>
Signed-off-by: Isaac Oram <isaac.w.oram@intel.com>
Co-authored-by: Nate DeSimone <nathaniel.l.desimone@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

-- 
2.27.0.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#72308): https://edk2.groups.io/g/devel/message/72308
Mute This Topic: https://groups.io/mt/81016650/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Looked over the series at a high-level and the feature structure looks 
fine. For the series:
Acked-by: Michael Kubacki <michael.kubacki@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@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@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Michael Kubacki <michael.kubacki@microsoft.com>
> Signed-off-by: Isaac Oram <isaac.w.oram@intel.com>
> Co-authored-by: Nate DeSimone <nathaniel.l.desimone@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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Michael,

Thanks for the feedback.

I was torn between aligning with the proprietary version and cleaning it up.
My concern is if we do too much cleanup, it will delay adoption.  

I did the minimum that we know is required for ECC tool to pass coding style tool and avoid EFI prefixes.  

I would like delay refactoring and cleaning up until it is in the open where people can easily follow the code evolution from proprietary to open source.  I am looking to develop some maintainers for this feature package, and the cleanup would be a good way to ramp them into active open participation and put them on the path to becoming maintainers.

Anyway, your feedback is noted and I will put those on the to do list for completing this.

Regards,
Isaac

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael Kubacki
Sent: Wednesday, March 3, 2021 11:23 AM
To: devel@edk2.groups.io; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>
Cc: Oram, Isaac W <isaac.w.oram@intel.com>; Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Michael Kubacki <michael.kubacki@microsoft.com>
Subject: Re: [edk2-devel] [edk2-platforms] [PATCH v1 0/9] IpmiFeaturePkg: Add IPMI transport drivers

Looked over the series at a high-level and the feature structure looks fine. For the series:
Acked-by: Michael Kubacki <michael.kubacki@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@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@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Michael Kubacki <michael.kubacki@microsoft.com>
> Signed-off-by: Isaac Oram <isaac.w.oram@intel.com>
> Co-authored-by: Nate DeSimone <nathaniel.l.desimone@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/SmmIpmiBaseL
> ib/SmmIpmiBaseLib.inf
> 







-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#72455): https://edk2.groups.io/g/devel/message/72455
Mute This Topic: https://groups.io/mt/81016650/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Isaac,

We - Ampere Computing  are implementing IPMI over SSIF transport (per 
IPMI 2.0 spec). We also have this ready for submission, soon.
However, in light of this review, we would be interested in co-work with 
Intel so the same design can be used on non-KCS platform, instead of 
keeping each implementation proprietary.

Thanks,
Nhi

On 3/5/21 06:33, Oram, Isaac W via groups.io wrote:
> Michael,
>
> Thanks for the feedback.
>
> I was torn between aligning with the proprietary version and cleaning it up.
> My concern is if we do too much cleanup, it will delay adoption.
>
> I did the minimum that we know is required for ECC tool to pass coding style tool and avoid EFI prefixes.
>
> I would like delay refactoring and cleaning up until it is in the open where people can easily follow the code evolution from proprietary to open source.  I am looking to develop some maintainers for this feature package, and the cleanup would be a good way to ramp them into active open participation and put them on the path to becoming maintainers.
>
> Anyway, your feedback is noted and I will put those on the to do list for completing this.
>
> Regards,
> Isaac
>
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael Kubacki
> Sent: Wednesday, March 3, 2021 11:23 AM
> To: devel@edk2.groups.io; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>
> Cc: Oram, Isaac W <isaac.w.oram@intel.com>; Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Michael Kubacki <michael.kubacki@microsoft.com>
> Subject: Re: [edk2-devel] [edk2-platforms] [PATCH v1 0/9] IpmiFeaturePkg: Add IPMI transport drivers
>
> Looked over the series at a high-level and the feature structure looks fine. For the series:
> Acked-by: Michael Kubacki <michael.kubacki@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@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@intel.com>
>> Cc: Liming Gao <gaoliming@byosoft.com.cn>
>> Cc: Michael Kubacki <michael.kubacki@microsoft.com>
>> Signed-off-by: Isaac Oram <isaac.w.oram@intel.com>
>> Co-authored-by: Nate DeSimone <nathaniel.l.desimone@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/SmmIpmiBaseL
>> ib/SmmIpmiBaseLib.inf
>>
>
>
>
>
>
>
> 
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#72489): https://edk2.groups.io/g/devel/message/72489
Mute This Topic: https://groups.io/mt/81016650/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Overall structure looks good. For the series:
Reviewed-by: Sai Chaganty <rangasai.v.chaganty@intel.com>

There are few, rather general comments which I think can be queued up as on-going improvement after the initial check-in.
1. Copyright year for newly added files should begin with the current year // this is probably should be taken care before the first checkin
2. Ensure new GUIDs are created for newly added modules and interfaces, to avoid any potential conflicts. 
3. Null instance libraries (e.g. IpmiBaseLibNull.inf) has classes defined that are not necessarily in use (e.g DebugLib). While it may not be a functional failure, it's a good practice to declare only those classes that are being used in the implementation. 
4. ReadMe needs to be filled out completely. 
5. Function description are found to be placed after the function (e.g. UpdateErrorStatus(), IpmiSendCommandToBmc() in IpmiBmc.c). The description comment block should precede the function.
6. Found instances where return status is marked as "Status" in description. Suggest to add possible return values in the function description 
7. Found cases where we have two APIs with different names but with similar implementation - e.g. IpmiSendCommandToBmc() and PeiIpmiSendCommandToBmc(). Can these be converged?
8. Please check and address relative path issue in . inf (e.g. GenericIpmi.inf)
9. In IpmiInit.c, Please remove the following, if not applicable
	#ifdef FAST_VIDEO_SUPPORT
	  #include <Protocol/VideoPrint.h>
	#endif
10. Consider adding .fdf definition under \Include that lists the IPMI modules to be dispatched. This will simplify the integration of this feature on a platform. 
11. Need to make sure package build is working.

Regards,
Sai

-----Original Message-----
From: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com> 
Sent: Monday, March 01, 2021 6:28 PM
To: devel@edk2.groups.io
Cc: Oram, Isaac W <isaac.w.oram@intel.com>; Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Michael Kubacki <michael.kubacki@microsoft.com>
Subject: [edk2-platforms] [PATCH v1 0/9] IpmiFeaturePkg: Add IPMI transport drivers

From: Isaac Oram <isaac.w.oram@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@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Michael Kubacki <michael.kubacki@microsoft.com>
Signed-off-by: Isaac Oram <isaac.w.oram@intel.com>
Co-authored-by: Nate DeSimone <nathaniel.l.desimone@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

--
2.27.0.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#72681): https://edk2.groups.io/g/devel/message/72681
Mute This Topic: https://groups.io/mt/81016650/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-