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.desim...@intel.com>
Cc: Oram, Isaac W <isaac.w.o...@intel.com>; Chaganty, Rangasai V
<rangasai.v.chaga...@intel.com>; Liming Gao <gaolim...@byosoft.com.cn>; Michael Kubacki
<michael.kuba...@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.kuba...@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.o...@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.chaga...@intel.com>
Cc: Liming Gao <gaolim...@byosoft.com.cn>
Cc: Michael Kubacki <michael.kuba...@microsoft.com>
Signed-off-by: Isaac Oram <isaac.w.o...@intel.com>
Co-authored-by: Nate DeSimone <nathaniel.l.desim...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
