[AMD Official Use Only - General]
> -----Original Message-----
> From: Tinh Nguyen <tinhngu...@amperemail.onmicrosoft.com>
> Sent: Friday, April 21, 2023 2:59 PM
> To: Chang, Abner <abner.ch...@amd.com>; devel@edk2.groups.io
> Cc: Isaac Oram <isaac.w.o...@intel.com>; Attar, AbdulLateef (Abdul Lateef)
> <abdullateef.at...@amd.com>; Nickle Wang <nick...@nvidia.com>; Igor
> Kulchytskyy <ig...@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.
>
>
> On 4/21/2023 7:51 AM, Chang, Abner wrote:
> > [EXTERNAL EMAIL NOTICE: This email originated from an external sender.
> > Please be mindful of safe email handling and proprietary information
> > protection practices.]
> >
> >
> > [AMD Official Use Only - General]
> >
> >
> >
> >> -----Original Message-----
> >> From: Tinh Nguyen <tinhngu...@amperemail.onmicrosoft.com>
> >> Sent: Thursday, April 20, 2023 2:08 PM
> >> To: devel@edk2.groups.io; Chang, Abner <abner.ch...@amd.com>
> >> Cc: Isaac Oram <isaac.w.o...@intel.com>; Attar, AbdulLateef (Abdul
> >> Lateef) <abdullateef.at...@amd.com>; Nickle Wang
> >> <nick...@nvidia.com>; Igor Kulchytskyy <ig...@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.ch...@amd.com>
> >>>
> >>> Update GetTransportCapability to support Maximum Transfer Unit (MTU)
> >>> of transport interface.
> >>>
> >>> Signed-off-by: Abner Chang <abner.ch...@amd.com>
> >>> Cc: Isaac Oram <isaac.w.o...@intel.com>
> >>> Cc: Abdul Lateef Attar <abdat...@amd.com>
> >>> Cc: Nickle Wang <nick...@nvidia.com>
> >>> Cc: Igor Kulchytskyy <ig...@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.
>
> yes, that is usually, but specification does not require it. I concern that
> someone will implement another size of payload
My thought is even the MTU is not in power of 2, transport can still report a
smaller MTU which is in power of 2.
e.g, if the MTU is 1234 bytes, then Manageability transport interface can
return 8 in this field, with this the MTU is 2^10 - 1.
>
>
> >
> >> 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.
> Agree, I also just use it for validation.
> > 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.
> I think we should take note somewhere
Ok, I will have comment for PPI/DXE/SMM.
> >
> >> 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/Comm
> >> o
> >>> n/ManageabilityTransportKcs.h
> >>>
> >>
> b/Features/ManageabilityPkg/Library/ManageabilityTransportKcsLib/Comm
> >> o
> >>> n/ManageabilityTransportKcs.h
> >>> index f1758ffd8f..2cdf60ba7e 100644
> >>> ---
> >>>
> >>
> a/Features/ManageabilityPkg/Library/ManageabilityTransportKcsLib/Comm
> >> o
> >>> 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?
> I do not believe ROM size is an issue of the system which can support those
> features, so let's keep them
Ok, got it.
Thanks
Abner
> > 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.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#103378): https://edk2.groups.io/g/devel/message/103378
Mute This Topic: https://groups.io/mt/98339115/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
