Re: [edk2-devel] [edk2-platforms][PATCH V2 02/14] ManageabilityPkg: Support Maximum Transfer Unit

["Chang, Abner" <abner.chang@amd.com>]

[AMD Official Use Only - General]

Hi Tinh,
V3 patch just sent, I will wait for your feedback on my response below.

Thanks
Abner

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Chang,
> Abner via groups.io
> Sent: Friday, April 21, 2023 8:51 AM
> To: Tinh Nguyen <tinhnguyen@amperemail.onmicrosoft.com>;
> devel@edk2.groups.io
> Cc: Isaac Oram <isaac.w.oram@intel.com>; Attar, AbdulLateef (Abdul Lateef)
> <AbdulLateef.Attar@amd.com>; Nickle Wang <nicklew@nvidia.com>; Igor
> Kulchytskyy <igork@ami.com>
> Subject: Re: [edk2-devel] [edk2-platforms][PATCH V2 02/14]
> ManageabilityPkg: Support Maximum Transfer Unit
> 
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
> 
> 
> [AMD Official Use Only - General]
> 
> 
> 
> > -----Original Message-----
> > From: Tinh Nguyen <tinhnguyen@amperemail.onmicrosoft.com>
> > Sent: Thursday, April 20, 2023 2:08 PM
> > To: devel@edk2.groups.io; Chang, Abner <Abner.Chang@amd.com>
> > Cc: Isaac Oram <isaac.w.oram@intel.com>; Attar, AbdulLateef (Abdul
> > Lateef) <AbdulLateef.Attar@amd.com>; Nickle Wang
> <nicklew@nvidia.com>;
> > Igor Kulchytskyy <igork@ami.com>
> > Subject: Re: [edk2-devel] [edk2-platforms][PATCH V2 02/14]
> > ManageabilityPkg: Support Maximum Transfer Unit
> >
> > Caution: This message originated from an External Source. Use proper
> > caution when opening attachments, clicking links, or responding.
> >
> >
> > Hi Abner,
> >
> > I have some inline comments below
> >
> > On 18/04/2023 14:15, Chang, Abner via groups.io wrote:
> > > From: Abner Chang <abner.chang@amd.com>
> > >
> > > Update GetTransportCapability to support Maximum Transfer Unit (MTU)
> > > of transport interface.
> > >
> > > Signed-off-by: Abner Chang <abner.chang@amd.com>
> > > Cc: Isaac Oram <isaac.w.oram@intel.com>
> > > Cc: Abdul Lateef Attar <abdattar@amd.com>
> > > Cc: Nickle Wang <nicklew@nvidia.com>
> > > Cc: Igor Kulchytskyy <igork@ami.com>
> > > ---
> > >   .../Library/ManageabilityTransportLib.h       | 33 ++++++++---
> > >   .../Common/ManageabilityTransportKcs.h        |  2 +
> > >   .../IpmiProtocol/Pei/IpmiPpiInternal.h        |  8 ++-
> > >   .../BaseManageabilityTransportNull.c          | 18 ++++--
> > >   .../Dxe/ManageabilityTransportKcs.c           | 57 +++++++++++--------
> > >   .../Universal/IpmiProtocol/Dxe/IpmiProtocol.c | 24 ++++++--
> > >   .../Universal/IpmiProtocol/Pei/IpmiPpi.c      | 51 ++++++++++-------
> > >   .../Universal/IpmiProtocol/Smm/IpmiProtocol.c | 24 ++++++--
> > >   8 files changed, 145 insertions(+), 72 deletions(-)
> > >
> > > diff --git
> > > a/Features/ManageabilityPkg/Include/Library/ManageabilityTransportLib.
> > > h
> > > b/Features/ManageabilityPkg/Include/Library/ManageabilityTransportLib.
> > > h
> > > index c022b4ac5c..d86d0d87d5 100644
> > > ---
> > > a/Features/ManageabilityPkg/Include/Library/ManageabilityTransportLib.
> > > h
> > > +++ b/Features/ManageabilityPkg/Include/Library/ManageabilityTranspo
> > > +++ rt
> > > +++ Lib.h
> > > @@ -14,6 +14,9 @@
> > >   #define MANAGEABILITY_TRANSPORT_TOKEN_VERSION
> > ((MANAGEABILITY_TRANSPORT_TOKEN_VERSION_MAJOR << 8) |\
> > >
> > > MANAGEABILITY_TRANSPORT_TOKEN_VERSION_MINOR)
> > >
> > > +#define
> > MANAGEABILITY_TRANSPORT_PAYLOAD_SIZE_FROM_CAPABILITY(a)  (1
> << ((a &
> > MANAGEABILITY_TRANSPORT_CAPABILITY_MAXIMUM_PAYLOAD_MASK)
> > >>\
> > > +
> > >
> >
> +MANAGEABILITY_TRANSPORT_CAPABILITY_MAXIMUM_PAYLOAD_BIT_PO
> > SITION))
> > > +
> > >   typedef struct  _MANAGEABILITY_TRANSPORT_FUNCTION_V1_0
> > MANAGEABILITY_TRANSPORT_FUNCTION_V1_0;
> > >   typedef struct  _MANAGEABILITY_TRANSPORT
> > MANAGEABILITY_TRANSPORT;
> > >   typedef struct  _MANAGEABILITY_TRANSPORT_TOKEN
> > MANAGEABILITY_TRANSPORT_TOKEN;
> > > @@ -68,8 +71,17 @@ typedef UINT32
> > MANAGEABILITY_TRANSPORT_ADDITIONAL_STATUS;
> > >   /// Additional transport interface features.
> > >   ///
> > >   typedef UINT32 MANAGEABILITY_TRANSPORT_CAPABILITY;
> > > +/// Bit 0
> > >   #define
> >
> MANAGEABILITY_TRANSPORT_CAPABILITY_MULTIPLE_TRANSFER_TOKENS
> > 0x00000001
> > > -#define
> > MANAGEABILITY_TRANSPORT_CAPABILITY_ASYNCHRONOUS_TRANSFER
> > 0x00000002
> > > +/// Bit 1
> > > +#define
> > MANAGEABILITY_TRANSPORT_CAPABILITY_ASYNCHRONOUS_TRANSFER
> > 0x00000002
> > > +/// Bit 2   - Reserved
> > > +/// Bit 7:3 - Transport interface maximum payload size, which is (2
> > > +^ bit[7:3]
> > - 1)
> > > +///           bit[7:3] means no maximum payload.
> >
> > I am confused with your definition here.
> >
> > Why does it have to be a power of 2?
> Usually the maximum/minimum is in  power of 2 and use power of 2 has less
> bits occupied from MANAGEABILITY_TRANSPORT_CAPABILITY.
> 
> >
> > And we should separate request payload size and response payload size
> >
> > Can you clarify more about that?
> Do we really need the maximum size for response? Response is initiated by
> target endpoint and suppose the payload header should have some fields
> that indicate the return payload is only part of response. The size of payload
> returned is actually maximum transfer size that target endpoint can handle.
> Do you know any case that receiver has no idea about if the payload sent
> back from target endpoint is a partial of response or not?  We should have
> MTU response if it is required for the transport interface.
> 
> >
> > Another question, only PEI_IPMI_PPI_INTERNAL contains MaxPayloadSize,
> PPI has MaxPayloadSize in structure is because we can't define a global
> variable for PEI module as module is executed in place.
> 
> > how do IPMI/MCTP/PLDM protocol provide Maxpayloadsize
> For DXE drivers, the Maxpayloadsize is defined as global variable.
> 
> >
> > to caller?
> >
> > > +#define
> > MANAGEABILITY_TRANSPORT_CAPABILITY_MAXIMUM_PAYLOAD_MASK
> > 0x000000f8
> > > +#define
> >
> MANAGEABILITY_TRANSPORT_CAPABILITY_MAXIMUM_PAYLOAD_BIT_POS
> > ITION   3
> > > +#define
> > >
> >
> +MANAGEABILITY_TRANSPORT_CAPABILITY_MAXIMUM_PAYLOAD_NOT_A
> > VAILABLE
> > > +0x00 /// Bit 8:31 - Reserved
> > >
> > >   ///
> > >   /// Definitions of Manageability transport interface functions.
> > > @@ -187,15 +199,20 @@ AcquireTransportSession (
> > >     );
> > >
> > >   /**
> > > -  This function returns the transport capabilities.
> > > -
> > > -  @param [out]  TransportFeature        Pointer to receive transport
> > capabilities.
> > > -                                        See the definitions of
> > > -                                        MANAGEABILITY_TRANSPORT_CAPABILITY.
> > > -
> > > +  This function returns the transport capabilities according to
> > > + the manageability protocol.
> > > +
> > > +  @param [in]   TransportToken             Transport token acquired from
> > manageability
> > > +                                           transport library.
> > > +  @param [out]  TransportFeature           Pointer to receive transport
> > capabilities.
> > > +                                           See the definitions of
> > > +                                           MANAGEABILITY_TRANSPORT_CAPABILITY.
> > > +  @retval       EFI_SUCCESS                TransportCapability is returned
> > successfully.
> > > +  @retval       EFI_INVALID_PARAMETER      TransportToken is not a valid
> > token.
> > >   **/
> > > -VOID
> > > +EFI_STATUS
> > >   GetTransportCapability (
> > > +  IN MANAGEABILITY_TRANSPORT_TOKEN        *TransportToken,
> > >     OUT MANAGEABILITY_TRANSPORT_CAPABILITY  *TransportCapability
> > >     );
> > >
> > > diff --git
> > >
> >
> a/Features/ManageabilityPkg/Library/ManageabilityTransportKcsLib/Commo
> > > n/ManageabilityTransportKcs.h
> > >
> >
> b/Features/ManageabilityPkg/Library/ManageabilityTransportKcsLib/Comm
> > o
> > > n/ManageabilityTransportKcs.h
> > > index f1758ffd8f..2cdf60ba7e 100644
> > > ---
> > >
> >
> a/Features/ManageabilityPkg/Library/ManageabilityTransportKcsLib/Commo
> > > n/ManageabilityTransportKcs.h
> > > +++ b/Features/ManageabilityPkg/Library/ManageabilityTransportKcsLib
> > > +++ /C ommon/ManageabilityTransportKcs.h
> > > @@ -32,6 +32,8 @@ typedef struct {
> > >   #define IPMI_KCS_GET_STATE(s)  (s >> 6)
> > >   #define IPMI_KCS_SET_STATE(s)  (s << 6)
> > >
> > > +#define MCTP_KCS_MTU_IN_POWER_OF_2  8
> > > +
> > >   /// 5 sec, according to IPMI spec
> > >   #define IPMI_KCS_TIMEOUT_5_SEC  5000*1000
> > >   #define IPMI_KCS_TIMEOUT_1MS    1000
> > > diff --git
> > > a/Features/ManageabilityPkg/Universal/IpmiProtocol/Pei/IpmiPpiIntern
> > > al
> > > .h
> > > b/Features/ManageabilityPkg/Universal/IpmiProtocol/Pei/IpmiPpiIntern
> > > al
> > > .h
> > > index bbe0c8c5cb..4b6bdc97a9 100644
> > > ---
> > > a/Features/ManageabilityPkg/Universal/IpmiProtocol/Pei/IpmiPpiIntern
> > > al
> > > .h
> > > +++ b/Features/ManageabilityPkg/Universal/IpmiProtocol/Pei/IpmiPpiIn
> > > +++ te
> > > +++ rnal.h
> > > @@ -17,9 +17,11 @@
> > >   #define MANAGEABILITY_IPMI_PPI_INTERNAL_FROM_LINK(a)  CR (a,
> > > PEI_IPMI_PPI_INTERNAL, PeiIpmiPpi,
> > > MANAGEABILITY_IPMI_PPI_INTERNAL_SIGNATURE)
> > >
> > >   typedef struct {
> > > -  UINT32                         Signature;
> > > -  MANAGEABILITY_TRANSPORT_TOKEN  *TransportToken;
> > > -  PEI_IPMI_PPI                   PeiIpmiPpi;
> > > +  UINT32                                Signature;
> > > +  MANAGEABILITY_TRANSPORT_TOKEN         *TransportToken;
> > > +  MANAGEABILITY_TRANSPORT_CAPABILITY    TransportCapability;
> > > +  UINT32                                TransportMaximumPayload;
> > > +  PEI_IPMI_PPI                          PeiIpmiPpi;
> > >   } PEI_IPMI_PPI_INTERNAL;
> > >
> > >   #endif // MANAGEABILITY_IPMI_PPI_INTERNAL_H_
> > > diff --git
> > > a/Features/ManageabilityPkg/Library/BaseManageabilityTransportNullLi
> > > b/
> > > BaseManageabilityTransportNull.c
> > > b/Features/ManageabilityPkg/Library/BaseManageabilityTransportNullLi
> > > b/
> > > BaseManageabilityTransportNull.c
> > > index 49fc8c0f71..3aa68578a6 100644
> > > ---
> > > a/Features/ManageabilityPkg/Library/BaseManageabilityTransportNullLi
> > > b/
> > > BaseManageabilityTransportNull.c
> > > +++
> > b/Features/ManageabilityPkg/Library/BaseManageabilityTransportNull
> > > +++ Lib/BaseManageabilityTransportNull.c
> > > @@ -31,19 +31,25 @@ AcquireTransportSession (
> > >   }
> > >
> > >   /**
> > > -  This function returns the transport capabilities.
> > > -
> > > -  @param [out]  TransportFeature        Pointer to receive transport
> > capabilities.
> > > -                                        See the definitions of
> > > -                                        MANAGEABILITY_TRANSPORT_CAPABILITY.
> > > +  This function returns the transport capabilities according to
> > > + the manageability protocol.
> > >
> > > +  @param [in]   TransportToken             Transport token acquired from
> > manageability
> > > +                                           transport library.
> > > +  @param [out]  TransportFeature           Pointer to receive transport
> > capabilities.
> > > +                                           See the definitions of
> > > +                                           MANAGEABILITY_TRANSPORT_CAPABILITY.
> > > +  @retval       EFI_SUCCESS                TransportCapability is returned
> > successfully.
> > > +  @retval       EFI_INVALID_PARAMETER      TransportToken is not a valid
> > token.
> > >   **/
> > > -VOID
> > > +EFI_STATUS
> > >   GetTransportCapability (
> > > +  IN MANAGEABILITY_TRANSPORT_TOKEN        *TransportToken,
> > >     OUT MANAGEABILITY_TRANSPORT_CAPABILITY  *TransportCapability
> > >     )
> > >   {
> > >     *TransportCapability = 0;
> > > +  return EFI_SUCCESS;
> > >   }
> > >
> > >   /**
> > > diff --git
> > >
> >
> a/Features/ManageabilityPkg/Library/ManageabilityTransportKcsLib/Dxe/M
> > > anageabilityTransportKcs.c
> > >
> >
> b/Features/ManageabilityPkg/Library/ManageabilityTransportKcsLib/Dxe/M
> > > anageabilityTransportKcs.c
> > > index ab416e5449..7d85378fc1 100644
> > > ---
> > >
> >
> a/Features/ManageabilityPkg/Library/ManageabilityTransportKcsLib/Dxe/M
> > > anageabilityTransportKcs.c
> > > +++ b/Features/ManageabilityPkg/Library/ManageabilityTransportKcsLib
> > > +++ /D
> > > +++ xe/ManageabilityTransportKcs.c
> > > @@ -62,7 +62,7 @@ KcsTransportInit (
> > >     }
> > >
> > >     if (HardwareInfo.Kcs == NULL) {
> > > -    DEBUG ((DEBUG_INFO, "%a: Hardware information is not provided,
> use
> > dfault settings.\n", __FUNCTION__));
> > > +    DEBUG ((DEBUG_MANAGEABILITY_INFO, "%a: Hardware information
> is
> > > + not provided, use dfault settings.\n", __FUNCTION__));
> > >       mKcsHardwareInfo.MemoryMap                    =
> > MANAGEABILITY_TRANSPORT_KCS_IO_MAP_IO;
> > >       mKcsHardwareInfo.IoBaseAddress.IoAddress16    = PcdGet16
> > (PcdIpmiKcsIoBaseAddress);
> > >       mKcsHardwareInfo.IoDataInAddress.IoAddress16  =
> > > mKcsHardwareInfo.IoBaseAddress.IoAddress16 +
> > IPMI_KCS_DATA_IN_REGISTER_OFFSET; @@ -81,21 +81,21 @@
> KcsTransportInit
> > (
> > >     // Get protocol specification name.
> > >     ManageabilityProtocolName = HelperManageabilitySpecName
> > > (TransportToken->ManageabilityProtocolSpecification);
> > >
> > > -  DEBUG ((DEBUG_INFO, "%a: KCS transport hardware for %s is:\n",
> > > __FUNCTION__, ManageabilityProtocolName));
> > > +  DEBUG ((DEBUG_MANAGEABILITY_INFO, "%a: KCS transport hardware
> > for
> > > + %s is:\n", __FUNCTION__, ManageabilityProtocolName));
> > >     if (mKcsHardwareInfo.MemoryMap) {
> > > -    DEBUG ((DEBUG_INFO, "Memory Map I/O\n", __FUNCTION__));
> > > -    DEBUG ((DEBUG_INFO, "Base Memory Address : 0x%08x\n",
> > mKcsHardwareInfo.IoBaseAddress.IoAddress32));
> > > -    DEBUG ((DEBUG_INFO, "Data in Address     : 0x%08x\n",
> > mKcsHardwareInfo.IoDataInAddress.IoAddress32));
> > > -    DEBUG ((DEBUG_INFO, "Data out Address    : 0x%08x\n",
> > mKcsHardwareInfo.IoDataOutAddress.IoAddress32));
> > > -    DEBUG ((DEBUG_INFO, "Command Address     : 0x%08x\n",
> > mKcsHardwareInfo.IoCommandAddress.IoAddress32));
> > > -    DEBUG ((DEBUG_INFO, "Status Address      : 0x%08x\n",
> > mKcsHardwareInfo.IoStatusAddress.IoAddress32));
> > > +    DEBUG ((DEBUG_MANAGEABILITY_INFO, "Memory Map I/O\n",
> > __FUNCTION__));
> > > +    DEBUG ((DEBUG_MANAGEABILITY_INFO, "Base Memory Address :
> > 0x%08x\n", mKcsHardwareInfo.IoBaseAddress.IoAddress32));
> > > +    DEBUG ((DEBUG_MANAGEABILITY_INFO, "Data in Address     :
> > 0x%08x\n", mKcsHardwareInfo.IoDataInAddress.IoAddress32));
> > > +    DEBUG ((DEBUG_MANAGEABILITY_INFO, "Data out Address    :
> > 0x%08x\n", mKcsHardwareInfo.IoDataOutAddress.IoAddress32));
> > > +    DEBUG ((DEBUG_MANAGEABILITY_INFO, "Command Address     :
> > 0x%08x\n", mKcsHardwareInfo.IoCommandAddress.IoAddress32));
> > > +    DEBUG ((DEBUG_MANAGEABILITY_INFO, "Status Address      :
> > 0x%08x\n", mKcsHardwareInfo.IoStatusAddress.IoAddress32));
> > >     } else {
> > > -    DEBUG ((DEBUG_INFO, "I/O Map I/O\n"));
> > > -    DEBUG ((DEBUG_INFO, "Base I/O port    : 0x%04x\n",
> > mKcsHardwareInfo.IoBaseAddress.IoAddress16));
> > > -    DEBUG ((DEBUG_INFO, "Data in I/O port : 0x%04x\n",
> > mKcsHardwareInfo.IoDataInAddress.IoAddress16));
> > > -    DEBUG ((DEBUG_INFO, "Data out I/O port: 0x%04x\n",
> > mKcsHardwareInfo.IoDataOutAddress.IoAddress16));
> > > -    DEBUG ((DEBUG_INFO, "Command I/O port : 0x%04x\n",
> > mKcsHardwareInfo.IoCommandAddress.IoAddress16));
> > > -    DEBUG ((DEBUG_INFO, "Status I/O port  : 0x%04x\n",
> > mKcsHardwareInfo.IoStatusAddress.IoAddress16));
> > > +    DEBUG ((DEBUG_MANAGEABILITY_INFO, "I/O Map I/O\n"));
> > > +    DEBUG ((DEBUG_MANAGEABILITY_INFO, "Base I/O port    :
> 0x%04x\n",
> > mKcsHardwareInfo.IoBaseAddress.IoAddress16));
> > > +    DEBUG ((DEBUG_MANAGEABILITY_INFO, "Data in I/O port :
> > > + 0x%04x\n",
> > mKcsHardwareInfo.IoDataInAddress.IoAddress16));
> > > +    DEBUG ((DEBUG_MANAGEABILITY_INFO, "Data out I/O port:
> > 0x%04x\n", mKcsHardwareInfo.IoDataOutAddress.IoAddress16));
> > > +    DEBUG ((DEBUG_MANAGEABILITY_INFO, "Command I/O port :
> > 0x%04x\n", mKcsHardwareInfo.IoCommandAddress.IoAddress16));
> > > +    DEBUG ((DEBUG_MANAGEABILITY_INFO, "Status I/O port  :
> > > + 0x%04x\n", mKcsHardwareInfo.IoStatusAddress.IoAddress16));
> > >     }
> > if those code is just for debugging, you should put them into
> > DEBUG_CODE_BEGIN() and DEBUG_CODE_END()
> DEBUG_MANAGEABILITY is got approval and we can use that to enable the
> print error level for Manageability stuff. Use DEBUG_CODE_BEGIN requires
> user to enable DEBUG_PROPERTY_DEBUG_CODE_ENABLED additionally, it
> saves ROM size though.
> How do you think? Which way is convenient to users and also have a good
> code structure?
> 
> Thanks
> Abner
> 
> > >
> > >     return EFI_SUCCESS;
> > > @@ -221,7 +221,7 @@ KcsTransportTransmitReceive (
> > >     EFI_STATUS                           Status;
> > >     MANAGEABILITY_IPMI_TRANSPORT_HEADER  *TransmitHeader;
> > >
> > > -  if (TransportToken == NULL || TransferToken == NULL) {
> > > +  if ((TransportToken == NULL) || (TransferToken == NULL)) {
> > >       DEBUG ((DEBUG_ERROR, "%a: Invalid transport token or transfer
> > token.\n", __FUNCTION__));
> > >       return;
> > >     }
> > > @@ -298,6 +298,7 @@ AcquireTransportSession (
> > >       DEBUG ((DEBUG_ERROR, "%a: Fail to allocate memory for
> > MANAGEABILITY_TRANSPORT_KCS\n", __FUNCTION__));
> > >       return EFI_OUT_OF_RESOURCES;
> > >     }
> > > +
> > >     KcsTransportToken->Token.Transport = AllocateZeroPool (sizeof
> > (MANAGEABILITY_TRANSPORT));
> > >     if (KcsTransportToken->Token.Transport == NULL) {
> > >       FreePool (KcsTransportToken);
> > > @@ -329,21 +330,29 @@ AcquireTransportSession (
> > >   }
> > >
> > >   /**
> > > -  This function returns the transport capabilities.
> > > -
> > > -  @param [out]  TransportFeature        Pointer to receive transport
> > capabilities.
> > > -                                        See the definitions of
> > > -                                        MANAGEABILITY_TRANSPORT_CAPABILITY.
> > > -
> > > +  This function returns the transport capabilities according to
> > > + the manageability protocol.
> > > +
> > > +  @param [in]   TransportToken             Transport token acquired from
> > manageability
> > > +                                           transport library.
> > > +  @param [out]  TransportFeature           Pointer to receive transport
> > capabilities.
> > > +                                           See the definitions of
> > > +                                           MANAGEABILITY_TRANSPORT_CAPABILITY.
> > > +  @retval       EFI_SUCCESS                TransportCapability is returned
> > successfully.
> > > +  @retval       EFI_INVALID_PARAMETER      TransportToken is not a valid
> > token.
> > >   **/
> > > -VOID
> > > +EFI_STATUS
> > >   GetTransportCapability (
> > > +  IN MANAGEABILITY_TRANSPORT_TOKEN        *TransportToken,
> > >     OUT MANAGEABILITY_TRANSPORT_CAPABILITY  *TransportCapability
> > >     )
> > >   {
> > > -  if (TransportCapability != NULL) {
> > > -    *TransportCapability = 0;
> > > +  if ((TransportToken == NULL) || (TransportCapability == NULL)) {
> > > +    return EFI_INVALID_PARAMETER;
> > >     }
> > > +
> > > +  *TransportCapability = 0;
> > > +  return EFI_SUCCESS;
> > >   }
> > >
> > >   /**
> > > diff --git
> > > a/Features/ManageabilityPkg/Universal/IpmiProtocol/Dxe/IpmiProtocol.
> > > c
> > > b/Features/ManageabilityPkg/Universal/IpmiProtocol/Dxe/IpmiProtocol.
> > > c
> > > index 05175ee448..51d5c7f0ba 100644
> > > ---
> > > a/Features/ManageabilityPkg/Universal/IpmiProtocol/Dxe/IpmiProtocol.
> > > c
> > > +++
> > b/Features/ManageabilityPkg/Universal/IpmiProtocol/Dxe/IpmiProtoco
> > > +++ l.c
> > > @@ -17,9 +17,9 @@
> > >
> > >   #include "IpmiProtocolCommon.h"
> > >
> > > -MANAGEABILITY_TRANSPORT_TOKEN  *mTransportToken = NULL;
> > > -CHAR16                         *mTransportName;
> > > -
> > > +MANAGEABILITY_TRANSPORT_TOKEN                 *mTransportToken =
> NULL;
> > > +CHAR16                                        *mTransportName;
> > > +UINT32                                        TransportMaximumPayload;
> > >   MANAGEABILITY_TRANSPORT_HARDWARE_INFORMATION
> > mHardwareInformation;
> > >
> > >   /**
> > > @@ -92,8 +92,6 @@ DxeIpmiEntry (
> > >     MANAGEABILITY_TRANSPORT_CAPABILITY         TransportCapability;
> > >     MANAGEABILITY_TRANSPORT_ADDITIONAL_STATUS
> > > TransportAdditionalStatus;
> > >
> > > -  GetTransportCapability (&TransportCapability);
> > > -
> > >     Status = HelperAcquireManageabilityTransport (
> > >                &gManageabilityProtocolIpmiGuid,
> > >                &mTransportToken
> > > @@ -103,8 +101,22 @@ DxeIpmiEntry (
> > >       return Status;
> > >     }
> > >
> > > +  Status = GetTransportCapability (mTransportToken,
> > > + &TransportCapability);  if (EFI_ERROR (Status)) {
> > > +    DEBUG ((DEBUG_ERROR, "%a: Failed to
> > > + GetTransportCapability().\n",
> > __FUNCTION__));
> > > +    return Status;
> > > +  }
> > > +
> > > +  TransportMaximumPayload =
> > > + MANAGEABILITY_TRANSPORT_PAYLOAD_SIZE_FROM_CAPABILITY
> > (TransportCapability);  if (TransportMaximumPayload == (1 <<
> >
> MANAGEABILITY_TRANSPORT_CAPABILITY_MAXIMUM_PAYLOAD_NOT_AV
> > AILABLE)) {
> > > +    DEBUG ((DEBUG_MANAGEABILITY_INFO, "%a: Transport interface
> > > + maximum payload is undefined.\n", __FUNCTION__));  } else {
> > > +    TransportMaximumPayload -= 1;
> > > +    DEBUG ((DEBUG_MANAGEABILITY_INFO, "%a: Transport interface for
> > > + IPMI protocol has maximum payload %x.\n", __FUNCTION__,
> > > + TransportMaximumPayload));  }
> > > +
> > >     mTransportName = HelperManageabilitySpecName
> > > (mTransportToken->Transport->ManageabilityTransportSpecification);
> > > -  DEBUG ((DEBUG_INFO, "%a: IPMI protocol over %s.\n",
> __FUNCTION__,
> > > mTransportName));
> > > +  DEBUG ((DEBUG_MANAGEABILITY_INFO, "%a: IPMI protocol over
> > %s.\n",
> > > + __FUNCTION__, mTransportName));
> > >
> > >     //
> > >     // Setup hardware information according to the transport interface.
> > > diff --git
> > > a/Features/ManageabilityPkg/Universal/IpmiProtocol/Pei/IpmiPpi.c
> > > b/Features/ManageabilityPkg/Universal/IpmiProtocol/Pei/IpmiPpi.c
> > > index f839cd7387..8bf1e794f0 100644
> > > --- a/Features/ManageabilityPkg/Universal/IpmiProtocol/Pei/IpmiPpi.c
> > > +++ b/Features/ManageabilityPkg/Universal/IpmiProtocol/Pei/IpmiPpi.c
> > > @@ -51,19 +51,19 @@ PeiIpmiSubmitCommand (
> > >     IN OUT UINT32        *ResponseDataSize
> > >     )
> > >   {
> > > -  EFI_STATUS            Status;
> > > -  PEI_IPMI_PPI_INTERNAL *PeiIpmiPpiinternal;
> > > -
> > > -  PeiIpmiPpiinternal =
> > > MANAGEABILITY_IPMI_PPI_INTERNAL_FROM_LINK(This);
> > > -  Status = CommonIpmiSubmitCommand (
> > > -             PeiIpmiPpiinternal->TransportToken,
> > > -             NetFunction,
> > > -             Command,
> > > -             RequestData,
> > > -             RequestDataSize,
> > > -             ResponseData,
> > > -             ResponseDataSize
> > > -             );
> > > +  EFI_STATUS             Status;
> > > +  PEI_IPMI_PPI_INTERNAL  *PeiIpmiPpiinternal;
> > > +
> > > +  PeiIpmiPpiinternal =
> MANAGEABILITY_IPMI_PPI_INTERNAL_FROM_LINK
> > (This);
> > > +  Status             = CommonIpmiSubmitCommand (
> > > +                         PeiIpmiPpiinternal->TransportToken,
> > > +                         NetFunction,
> > > +                         Command,
> > > +                         RequestData,
> > > +                         RequestDataSize,
> > > +                         ResponseData,
> > > +                         ResponseDataSize
> > > +                         );
> > >     return Status;
> > >   }
> > >
> > > @@ -87,29 +87,28 @@ PeiIpmiEntry (
> > >     CHAR16                                        *TransportName;
> > >     PEI_IPMI_PPI_INTERNAL                         *PeiIpmiPpiinternal;
> > >     EFI_PEI_PPI_DESCRIPTOR                        *PpiDescriptor;
> > > -  MANAGEABILITY_TRANSPORT_CAPABILITY            TransportCapability;
> > >     MANAGEABILITY_TRANSPORT_ADDITIONAL_STATUS
> > TransportAdditionalStatus;
> > >     MANAGEABILITY_TRANSPORT_HARDWARE_INFORMATION
> > HardwareInformation;
> > >
> > > -  PeiIpmiPpiinternal = (PEI_IPMI_PPI_INTERNAL *)AllocateZeroPool
> > > (sizeof(PEI_IPMI_PPI_INTERNAL));
> > > +  PeiIpmiPpiinternal = (PEI_IPMI_PPI_INTERNAL *)AllocateZeroPool
> > > + (sizeof (PEI_IPMI_PPI_INTERNAL));
> > >     if (PeiIpmiPpiinternal == NULL) {
> > >       DEBUG ((DEBUG_ERROR, "%a: Not enough memory for
> > PEI_IPMI_PPI_INTERNAL.\n", __FUNCTION__));
> > >       return EFI_OUT_OF_RESOURCES;
> > >     }
> > > -  PpiDescriptor = (EFI_PEI_PPI_DESCRIPTOR *)AllocateZeroPool
> > > (sizeof(EFI_PEI_PPI_DESCRIPTOR));
> > > +
> > > +  PpiDescriptor = (EFI_PEI_PPI_DESCRIPTOR *)AllocateZeroPool
> > > + (sizeof (EFI_PEI_PPI_DESCRIPTOR));
> > >     if (PpiDescriptor == NULL) {
> > >       DEBUG ((DEBUG_ERROR, "%a: Not enough memory for
> > EFI_PEI_PPI_DESCRIPTOR.\n", __FUNCTION__));
> > >       return EFI_OUT_OF_RESOURCES;
> > >     }
> > >
> > > -  PeiIpmiPpiinternal->Signature =
> > > MANAGEABILITY_IPMI_PPI_INTERNAL_SIGNATURE;
> > > +  PeiIpmiPpiinternal->Signature                    =
> > MANAGEABILITY_IPMI_PPI_INTERNAL_SIGNATURE;
> > >     PeiIpmiPpiinternal->PeiIpmiPpi.IpmiSubmitCommand =
> > > PeiIpmiSubmitCommand;
> > >
> > >     PpiDescriptor->Flags = EFI_PEI_PPI_DESCRIPTOR_PPI |
> > EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST;
> > >     PpiDescriptor->Guid  = &gPeiIpmiPpiGuid;
> > >     PpiDescriptor->Ppi   = &PeiIpmiPpiinternal->PeiIpmiPpi;
> > >
> > > -  GetTransportCapability (&TransportCapability);
> > >     Status = HelperAcquireManageabilityTransport (
> > >                &gManageabilityProtocolIpmiGuid,
> > >                &PeiIpmiPpiinternal->TransportToken
> > > @@ -119,8 +118,22 @@ PeiIpmiEntry (
> > >       return Status;
> > >     }
> > >
> > > +  Status = GetTransportCapability
> > > + (PeiIpmiPpiinternal->TransportToken,
> > > + &PeiIpmiPpiinternal->TransportCapability);
> > > +  if (EFI_ERROR (Status)) {
> > > +    DEBUG ((DEBUG_ERROR, "%a: Failed to
> > > + GetTransportCapability().\n",
> > __FUNCTION__));
> > > +    return Status;
> > > +  }
> > > +
> > > +  PeiIpmiPpiinternal->TransportMaximumPayload =
> > > + MANAGEABILITY_TRANSPORT_PAYLOAD_SIZE_FROM_CAPABILITY
> > > + (PeiIpmiPpiinternal->TransportCapability);
> > > +  if (PeiIpmiPpiinternal->TransportMaximumPayload  == (1 <<
> >
> MANAGEABILITY_TRANSPORT_CAPABILITY_MAXIMUM_PAYLOAD_NOT_AV
> > AILABLE)) {
> > > +    DEBUG ((DEBUG_MANAGEABILITY_INFO, "%a: Transport interface
> > > + maximum payload is undefined.\n", __FUNCTION__));  } else {
> > > +    PeiIpmiPpiinternal->TransportMaximumPayload -= 1;
> > > +    DEBUG ((DEBUG_MANAGEABILITY_INFO, "%a: Transport interface for
> > > + IPMI protocol has maximum payload 0x%x.\n", __FUNCTION__,
> > > + PeiIpmiPpiinternal->TransportMaximumPayload));
> > > +  }
> > > +
> > >     TransportName = HelperManageabilitySpecName
> > > (PeiIpmiPpiinternal->TransportToken->Transport->ManageabilityTranspo
> > > rt
> > > Specification);
> > > -  DEBUG ((DEBUG_INFO, "%a: IPMI protocol over %s.\n",
> __FUNCTION__,
> > > TransportName));
> > > +  DEBUG ((DEBUG_MANAGEABILITY_INFO, "%a: IPMI protocol over
> > %s.\n",
> > > + __FUNCTION__, TransportName));
> > >
> > >     //
> > >     // Setup hardware information according to the transport interface.
> > > diff --git
> > >
> a/Features/ManageabilityPkg/Universal/IpmiProtocol/Smm/IpmiProtocol.
> > > c
> > >
> >
> b/Features/ManageabilityPkg/Universal/IpmiProtocol/Smm/IpmiProtocol.c
> > > index 87a5436bdf..e4cd166b7a 100644
> > > ---
> > >
> a/Features/ManageabilityPkg/Universal/IpmiProtocol/Smm/IpmiProtocol.
> > > c
> > > +++
> > b/Features/ManageabilityPkg/Universal/IpmiProtocol/Smm/IpmiProtoco
> > > +++ l.c
> > > @@ -18,9 +18,9 @@
> > >
> > >   #include "IpmiProtocolCommon.h"
> > >
> > > -MANAGEABILITY_TRANSPORT_TOKEN  *mTransportToken = NULL;
> > > -CHAR16                         *mTransportName;
> > > -
> > > +MANAGEABILITY_TRANSPORT_TOKEN                 *mTransportToken =
> NULL;
> > > +CHAR16                                        *mTransportName;
> > > +UINT32                                        TransportMaximumPayload;
> > >   MANAGEABILITY_TRANSPORT_HARDWARE_INFORMATION
> > mHardwareInformation;
> > >
> > >   /**
> > > @@ -93,8 +93,6 @@ SmmIpmiEntry (
> > >     MANAGEABILITY_TRANSPORT_CAPABILITY         TransportCapability;
> > >     MANAGEABILITY_TRANSPORT_ADDITIONAL_STATUS
> > > TransportAdditionalStatus;
> > >
> > > -  GetTransportCapability (&TransportCapability);
> > > -
> > >     Status = HelperAcquireManageabilityTransport (
> > >                &gManageabilityProtocolIpmiGuid,
> > >                &mTransportToken
> > > @@ -104,8 +102,22 @@ SmmIpmiEntry (
> > >       return Status;
> > >     }
> > >
> > > +  Status = GetTransportCapability (mTransportToken,
> > > + &TransportCapability);  if (EFI_ERROR (Status)) {
> > > +    DEBUG ((DEBUG_ERROR, "%a: Failed to
> > > + GetTransportCapability().\n",
> > __FUNCTION__));
> > > +    return Status;
> > > +  }
> > > +
> > > +  TransportMaximumPayload =
> > > + MANAGEABILITY_TRANSPORT_PAYLOAD_SIZE_FROM_CAPABILITY
> > (TransportCapability);  if (TransportMaximumPayload == (1 <<
> >
> MANAGEABILITY_TRANSPORT_CAPABILITY_MAXIMUM_PAYLOAD_NOT_AV
> > AILABLE)) {
> > > +    DEBUG ((DEBUG_MANAGEABILITY_INFO, "%a: Transport interface
> > > + maximum payload is undefined.\n", __FUNCTION__));  } else {
> > > +    TransportMaximumPayload -= 1;
> > > +    DEBUG ((DEBUG_MANAGEABILITY_INFO, "%a: Transport interface for
> > > + IPMI protocol has maximum payload 0x%x.\n", __FUNCTION__,
> > > + TransportMaximumPayload));  }
> > > +
> > >     mTransportName = HelperManageabilitySpecName
> > > (mTransportToken->Transport->ManageabilityTransportSpecification);
> > > -  DEBUG ((DEBUG_INFO, "%a: IPMI protocol over %s.\n",
> __FUNCTION__,
> > > mTransportName));
> > > +  DEBUG ((DEBUG_MANAGEABILITY_INFO, "%a: IPMI protocol over
> > %s.\n",
> > > + __FUNCTION__, mTransportName));
> > >
> > >     //
> > >     // Setup hardware information according to the transport interface.
> 
> 
> 
>