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]



> -----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/ManageabilityTransport
> > +++ 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/IpmiPpiInternal
> > .h
> > b/Features/ManageabilityPkg/Universal/IpmiProtocol/Pei/IpmiPpiInternal
> > .h
> > index bbe0c8c5cb..4b6bdc97a9 100644
> > ---
> > a/Features/ManageabilityPkg/Universal/IpmiProtocol/Pei/IpmiPpiInternal
> > .h
> > +++ b/Features/ManageabilityPkg/Universal/IpmiProtocol/Pei/IpmiPpiInte
> > +++ 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/BaseManageabilityTransportNullLib/
> > BaseManageabilityTransportNull.c
> > b/Features/ManageabilityPkg/Library/BaseManageabilityTransportNullLib/
> > BaseManageabilityTransportNull.c
> > index 49fc8c0f71..3aa68578a6 100644
> > ---
> > a/Features/ManageabilityPkg/Library/BaseManageabilityTransportNullLib/
> > 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->ManageabilityTransport
> > 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.