[AMD Official Use Only - General]
Hi Isaac,
Thanks for spending time on this. Please see my feedback below,
> -----Original Message-----
> From: Oram, Isaac W <isaac.w.oram@intel.com>
> Sent: Thursday, February 16, 2023 9:42 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>; Nickle Wang <nicklew@nvidia.com>;
> Igor Kulchytskyy <igork@ami.com>; Attar, AbdulLateef (Abdul Lateef)
> <AbdulLateef.Attar@amd.com>; Leif Lindholm <quic_llindhol@quicinc.com>;
> Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: RE: [edk2-platforms][PATCH 0/7] Implementation of IPMI Protocol
>
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
>
>
> In looking at the code, this is what I see:
>
> edk2:
> There is an IpmiLib.h library class which abstracts the
> IpmiPpi.h/IpmiProtocol.h use
> There is an IpmiPpi.h/IpmiProtocol.h which provide an interface to send
> a command "via IPMI"
> Current IpmiPpi.h/IpmiProtocol.h implementations use IpmiBaseLib
> library class to send commands
>
> edk2-platforms/Features/Intel/OutOfBandManagement/IpmiFeaturePkg:
> There is an IpmiBaseLib library class which abstracts
> IpmiTransportPpi.h/IpmiTransportProtocol.h use
> There is an IpmiTransportPpi.h/IpmiTransportProtocol.h that abstracts
> the transport protocol.
> Currently IpmiTransportPpi.h/IpmiTransportProtocol.h are implemented
> in "GenericIpmi".
>
> It appears to me that they are meant to be the same API. The
> IpmiFeaturePkg transport version adds a function to GetBmcStatus, but it
> seems like IpmiSubmitCommand is meant to be the same function
> throughout.
> I don't find any use of the GetBmcStatus function that is in the
> IpmiFeaturePkg but not the edk2 API. I am not sure what to make of that. I
> think we need more feedback from current consumers to resolve that.
>
>
> When I look at our internal reference version which is an ancestor to the
> edk2-platforms/Feature/Intel.../IpmiFeaturePkg version, there is no use of
> the IpmiLib.h/IpmiPpi.h/IpmiProtocol.h. There are additional fields in
> interfaces, like GetBmcStatus and data values, but they seem to have been
> redesigned out when open sourcing the interfaces.
I don't really know the history of IpmiFeaturePkg and no idea why the implementation is sort of duplicated with the Ipmi stuff defined in edk2.
>
> My main questions:
> To your understanding, are the IpmiLib.h/IpmiPpi.h/IpmiProtocol.h and the
> IpmiBaseLib.h/IpmiTransportPpi.h/IpmiTransportProtocol.h simply two
> versions of basically the same with a library API abstracting a dynamic phase
> specific IPMI transport API?
> Why does ManageabilityPkg use IpmiBaseLib and not IpmiLib? Is it more
> than a temporary solution?
So what I was done is to connect the implementation between edk2 and edk2-platforms to avoid the design changes on both side.
There are IpmiLib implementation under MdeModulePkg/Library, those are
DxeIpmiLibIpmiProtocol Library
PeiIpmiLibIpmiPpi Library
SmmIpmiLibSmmIpmiProtocol Library
These libraries provide the abstract library of IpmiProtocol and this implementation makes sense to me for the usage of IPMI high level driver.
The drivers under ManageabilityPkg is the implementation of IpmiProtocol, so it uses IpmiBaseLib as IpmiBaseLib is the abstract API of IpmiTransport protocol.
So that is,
IpmiLib->IpmiProtocol->IpmiBaseLib->IpmiTransport (GenericIpmi). Again, the implementation of IpmiProtocol under ManageabilityPkg is the connection between existing IpmiLib and IpmiBaseLib.
More feedbacks in the each patch.
Thanks
Abner
>
>
> High level feedback:
> 0001 - I think we should not make this change. It impacts any existing users
> for no reason I can justify.
> 0002 - Not sure - per my understanding of the stack, it doesn't quite fit the
> code in an obvious way.
> 0003 - More description of the design and duplication, the roles of the library
> class and the PPI/protocols, and the relationship to other IPMI code in the
> repos.
> 0004 - Minor feedback only
> 0005/0006 - I think that we should skip the step of using IpmiBaseLib and just
> implement the modified version of GenericIpmi to support the edk2 API
> stack design.
>
> I will send more specific details in response to each commit shortly.
>
> Regards,
> Isaac
>
> -----Original Message-----
> From: abner.chang@amd.com <abner.chang@amd.com>
> Sent: Tuesday, February 7, 2023 8:22 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>; Nickle Wang <nicklew@nvidia.com>;
> Igor Kulchytskyy <igork@ami.com>; Abdul Lateef Attar
> <abdattar@amd.com>; Leif Lindholm <quic_llindhol@quicinc.com>; Kinney,
> Michael D <michael.d.kinney@intel.com>
> Subject: [edk2-platforms][PATCH 0/7] Implementation of IPMI Protocol
>
> From: Abner Chang <abner.chang@amd.com>
>
> This change implementes IPMI Protocol and PPI in the new introduced
> ManageabilityPkg (described in below email)
> https://edk2.groups.io/g/devel/message/95579?p=%2C%2C%2C20%2C0%2C
> 0%2C0%3A%3ACreated%2C%2CManageability%2C20%2C2%2C0%2C94572748
>
> BZ #4336:
> The change also fixes the confusion (Patch 1/7) of IpmiSubmitCommand()
> deinfed in IPMI Transport protocol.
>
> You can skip reviewing on patch 2/7 as it is an image file.
>
> 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: Nickle Wang <nicklew@nvidia.com>
> Cc: Igor Kulchytskyy <igork@ami.com>
> Cc: Abdul Lateef Attar <abdattar@amd.com>
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>
> Abner Chang (7):
> IpmiFeaturePkg: Rename IpmiSubmitCommand function
> ManageabilityPkg: Add diagrams
> ManageabilityPkg: Add Readme file
> ManageabilityPkg: Initial package
> ManageabilityPkg: Implement Ipmi Protocol/Ppi
> IpmiProtocol: Add to Manageability Package
> edk2-platforms: Maintainers.txt
>
> .../ManageabilityPkg/ManageabilityPkg.dec | 18 ++++
> .../Include/CommonLibs.dsc.inc | 52 +++++++++
> .../ManageabilityPkg/ManageabilityPkg.dsc | 27 +++++
> .../IpmiProtocol/Dxe/IpmiProtocolDxe.inf | 37 +++++++
> .../Universal/IpmiProtocol/Pei/IpmiPpiPei.inf | 38 +++++++
> .../IpmiProtocol/Smm/IpmiProtocolSmm.inf | 40 +++++++
> .../Include/Library/IpmiBaseLib.h | 2 +-
> .../Include/Ppi/IpmiTransportPpi.h | 2 +-
> .../Include/Protocol/IpmiTransportProtocol.h | 2 +-
> .../IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c | 2 +-
> .../GenericIpmi/Pei/PeiGenericIpmi.c | 2 +-
> .../GenericIpmi/Smm/SmmGenericIpmi.c | 2 +-
> .../Library/IpmiBaseLib/IpmiBaseLib.c | 4 +-
> .../Library/IpmiBaseLibNull/IpmiBaseLibNull.c | 2 +-
> .../IpmiCommandLib/IpmiCommandLibNetFnApp.c | 26 ++---
> .../IpmiCommandLibNetFnChassis.c | 12 +--
> .../IpmiCommandLibNetFnStorage.c | 24 ++---
> .../IpmiCommandLibNetFnTransport.c | 8 +-
> .../Library/PeiIpmiBaseLib/PeiIpmiBaseLib.c | 4 +-
> .../Library/SmmIpmiBaseLib/SmmIpmiBaseLib.c | 4 +-
> .../Universal/IpmiProtocol/Dxe/IpmiProtocol.c | 97 +++++++++++++++++
> .../Universal/IpmiProtocol/Pei/IpmiPpi.c | 102 ++++++++++++++++++
> .../Universal/IpmiProtocol/Smm/IpmiProtocol.c | 98 +++++++++++++++++
> .../Ipmi/Library/IpmiLibKcs/IpmiLibKcs.c | 12 +--
> Features/ManageabilityPkg/Readme.md | 37 +++++++
> .../Media/ManageabilityDriverStack.svg | 1 +
> Maintainers.txt | 9 +-
> 27 files changed, 608 insertions(+), 56 deletions(-) create mode 100644
> Features/ManageabilityPkg/ManageabilityPkg.dec
> create mode 100644
> Features/ManageabilityPkg/Include/CommonLibs.dsc.inc
> create mode 100644 Features/ManageabilityPkg/ManageabilityPkg.dsc
> 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
> create mode 100644 Features/ManageabilityPkg/Readme.md
> create mode 100644
> Features/ManageabilityPkg/Documents/Media/ManageabilityDriverStack.sv
> g
>
> --
> 2.37.1.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100301): https://edk2.groups.io/g/devel/message/100301
Mute This Topic: https://groups.io/mt/96810540/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-