[AMD Official Use Only - General]
Hi Nickle, I will fix them in the next version.
Thanks
Abner
> -----Original Message-----
> From: Nickle Wang <nick...@nvidia.com>
> Sent: Thursday, April 20, 2023 2:42 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>; Igor Kulchytskyy <ig...@ami.com>
> Subject: RE: [edk2-platforms][PATCH V2 01/14] ManageabilityPkg: Add more
> helper functions
>
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
>
>
> Hi Abner,
>
> Please find my comments below.
>
> Thanks,
> Nickle
>
> > -----Original Message-----
> > From: abner.ch...@amd.com <abner.ch...@amd.com>
> > Sent: Tuesday, April 18, 2023 3:16 PM
> > To: devel@edk2.groups.io
> > Cc: Isaac Oram <isaac.w.o...@intel.com>; Abdul Lateef Attar
> > <abdat...@amd.com>; Nickle Wang <nick...@nvidia.com>; Igor
> Kulchytskyy
> > <ig...@ami.com>
> > Subject: [edk2-platforms][PATCH V2 01/14] ManageabilityPkg: Add more
> > helper functions
> >
> > External email: Use caution opening links or attachments
> >
> >
> > From: Abner Chang <abner.ch...@amd.com>
> >
> > 1. Add a helper function to output payload binary
> > to debug output device.
> > 2. Add a helper function to split payload into
> > packages according to maximum transfer unit
> > of transport interface.
> > 3. Add a helper function to generate CRC8.
> >
> > 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>
> > ---
> > .../BaseManageabilityTransportHelper.inf | 1 +
> > .../Library/ManageabilityTransportHelperLib.h | 98 ++++++++
> > .../BaseManageabilityTransportHelper.c | 225 +++++++++++++++++-
> > 3 files changed, 314 insertions(+), 10 deletions(-)
> >
> > diff --git
> > a/Features/ManageabilityPkg/Library/BaseManageabilityTransportHelperLi
> > b/Ba
> > seManageabilityTransportHelper.inf
> > b/Features/ManageabilityPkg/Library/BaseManageabilityTransportHelperLi
> > b/Ba
> > seManageabilityTransportHelper.inf
> > index 5447954144..c9e5eaef60 100644
> > ---
> > a/Features/ManageabilityPkg/Library/BaseManageabilityTransportHelperLi
> > b/Ba
> > seManageabilityTransportHelper.inf
> > +++
> b/Features/ManageabilityPkg/Library/BaseManageabilityTransportHelp
> > +++ er Lib/BaseManageabilityTransportHelper.inf
> > @@ -25,6 +25,7 @@
> > [LibraryClasses]
> > BaseMemoryLib
> > DebugLib
> > + MemoryAllocationLib
> >
> > [Packages]
> > ManageabilityPkg/ManageabilityPkg.dec
> > diff --git
> > a/Features/ManageabilityPkg/Include/Library/ManageabilityTransportHelp
> > erLib
> > .h
> > b/Features/ManageabilityPkg/Include/Library/ManageabilityTransportHelp
> > erLib
> > .h
> > index 718ac34a1f..0dbf5ccb3c 100644
> > ---
> > a/Features/ManageabilityPkg/Include/Library/ManageabilityTransportHelp
> > erLib
> > .h
> > +++ b/Features/ManageabilityPkg/Include/Library/ManageabilityTransport
> > +++ He
> > +++ lperLib.h
> > @@ -11,8 +11,24 @@
> >
> > #include <Library/ManageabilityTransportLib.h>
> >
> > +#define DEBUG_MANAGEABILITY_INFO DEBUG_INFO
> > +
> > typedef struct _MANAGEABILITY_PROTOCOL_NAME
> > MANAGEABILITY_PROTOCOL_NAME;
> >
> > +typedef struct {
> > + UINT8 *PayloadPointer;
> > + UINT32 PayloadSize;
> > +} MANAGEABILITY_TRANSMISSION_PACKAGE_ATTR;
> > +
> > +//
> > +// The information of multi portions of payload it is // splitted
> > +according to transport interface Maximum // Transfer Unit.
> > +typedef struct {
> > + UINT16 NumberOfPackages; ///< Number
> > of packages
> in
> > MultiPackages.
> > + MANAGEABILITY_TRANSMISSION_PACKAGE_ATTR MultiPackages[];
> > +} MANAGEABILITY_TRANSMISSION_MULTI_PACKAGES;
> > +
> > /**
> > Helper function returns the human readable name of Manageability
> > specification.
> >
> > @@ -90,4 +106,86 @@ HelperInitManageabilityTransport (
> > OUT MANAGEABILITY_TRANSPORT_ADDITIONAL_STATUS
> > *TransportAdditionalStatus OPTIONAL
> > );
> >
> > +/**
> > + This function splits payload into multiple packages according to
> > + the given transport interface Maximum Transfer Unit (MTU).
> > +
> > + @param[in] PreambleSize The additional data size precedes
> > + each package.
> > + @param[in] PostambleSize The additional data size succeeds
> > + each package.
> > + @param[in] Payload Pointer to payload.
> > + @param[in] PayloadSize Payload size in byte.
> > + @param[in] MaximumTransferUnit MTU of transport interface.
> > + @param[out] MultiplePackages Pointer to receive
> > +
> > MANAGEABILITY_TRANSMISSION_MULTI_PACKAGES
> > + structure. Caller has to free the memory
> > + allocated for
> > MANAGEABILITY_TRANSMISSION_MULTI_PACKAGES.
> > +
> > + @retval EFI_SUCCESS
> > MANAGEABILITY_TRANSMISSION_MULTI_PACKAGES structure
> > + is returned successfully.
> > + @retval EFI_OUT_OF_RESOURCE Not enough resource to create
> > +
> > + MANAGEABILITY_TRANSMISSION_MULTI_PACKAGES
> > structure.
> > +**/
> > +EFI_STATUS
> > +HelperManageabilitySplitPayload (
> > + IN UINT16 PreambleSize,
> > + IN UINT16 PostambleSize,
> > + IN UINT8 *Payload,
> > + IN UINT32 PayloadSize,
> > + IN UINT32 MaximumTransferUnit,
> > + OUT MANAGEABILITY_TRANSMISSION_MULTI_PACKAGES
> > **MultiplePackages
> > + );
> > +
> > +/**
> > + This function generates CRC8 with given polynomial.
> > +
> > + @param[in] Polynomial Polynomial in 8-bit.
> > + @param[in] CrcInitialValue CRC initial value.
> > + @param[in] BufferStart Pointer to buffer starts the CRC
> > calculation.
> > + @param[in] BufferSize Size of buffer.
> > +
> > + @retval UINT8 CRC value.
> > +**/
> > +UINT8
> > +HelperManageabilityGenerateCrc8 (
> > + IN UINT8 Polynomial,
> > + IN UINT8 CrcInitialValue,
> > + IN UINT8 *BufferStart,
> > + IN UINT32 BufferSize
> > + );
> > +
> > +/**
> > + Print out manageability transmit payload to the debug output device.
> > +
> > + @param[in] Payload Payload to print.
> > + @param[in] PayloadSize Payload size.
> > +
> > +**/
> > +VOID
> > +EFIAPI
> > +HelperManageabilityPayLoadDebugPrint (
> > + IN VOID *Payload,
> > + IN UINT32 PayloadSize
> > + );
> > +
> > +/**
> > + Prints a debug message and manageability payload to the debug
> > +output
> > device.
> > +
> > + @param[in] Payload Payload to print.
> > + @param[in] PayloadSize Payload size.
> > + @param[in] Format The format string for the debug message to
> > print.
> > + @param[in] ... The variable argument list whose contents are
> accessed
> > + based on the format string specified by Format.
> > +
> > +**/
> > +VOID
> > +EFIAPI
> > +HelperManageabilityDebugPrint (
> > + IN VOID *Payload,
> > + IN UINT32 PayloadSize,
> > + IN CONST CHAR8 *Format,
> > + ...
> > + );
> > +
> > #endif
> > diff --git
> > a/Features/ManageabilityPkg/Library/BaseManageabilityTransportHelperLi
> > b/Ba
> > seManageabilityTransportHelper.c
> > b/Features/ManageabilityPkg/Library/BaseManageabilityTransportHelperLi
> > b/Ba
> > seManageabilityTransportHelper.c
> > index c3f35b7beb..0e241ca3fd 100644
> > ---
> > a/Features/ManageabilityPkg/Library/BaseManageabilityTransportHelperLi
> > b/Ba
> > seManageabilityTransportHelper.c
> > +++
> b/Features/ManageabilityPkg/Library/BaseManageabilityTransportHelp
> > +++ er Lib/BaseManageabilityTransportHelper.c
> > @@ -8,11 +8,12 @@
> > #include <Uefi.h>
> > #include <Library/BaseMemoryLib.h>
> > #include <Library/DebugLib.h>
> > +#include <Library/MemoryAllocationLib.h>
> > #include <Library/ManageabilityTransportHelperLib.h>
> >
> > //
> > // BaseManageabilityTransportHelper is used by PEI, DXE and SMM.
> > -// Make sure the global variables added here should be unchangable.
> > +// Make sure the global variables added here should be unchangeable.
> > //
> > MANAGEABILITY_SPECIFICATION_NAME ManageabilitySpecNameTable[]
> = {
> > { &gManageabilityTransportKcsGuid, L"KCS" },
> > @@ -47,8 +48,8 @@ HelperManageabilitySpecName (
> > return NULL;
> > }
> >
> > - if (SpecificationGuid == NULL || IsZeroGuid (SpecificationGuid)) {
> > - DEBUG((DEBUG_ERROR, "%a: Improper input GUIDs, could be NULL or
> zero
> > GUID.\n", __FUNCTION__));
> > + if ((SpecificationGuid == NULL) || IsZeroGuid (SpecificationGuid)) {
> > + DEBUG ((DEBUG_ERROR, "%a: Improper input GUIDs, could be NULL or
> > + zero GUID.\n", __FUNCTION__));
> > return NULL;
> > }
> >
> > @@ -99,12 +100,13 @@ HelperManageabilityCheckSupportedSpec (
> > return EFI_INVALID_PARAMETER;
> > }
> >
> > - if (TransportGuid == NULL ||
> > + if ((TransportGuid == NULL) ||
> > IsZeroGuid (TransportGuid) ||
> > - ManageabilityProtocolToCheck == NULL ||
> > + (ManageabilityProtocolToCheck == NULL) ||
> > IsZeroGuid (ManageabilityProtocolToCheck)
> > - ) {
> > - DEBUG((DEBUG_ERROR, "%a: Improper input GUIDs, could be NULL or
> zero
> > GUID.\n", __FUNCTION__));
> > + )
> > + {
> > + DEBUG ((DEBUG_ERROR, "%a: Improper input GUIDs, could be NULL or
> > + zero GUID.\n", __FUNCTION__));
> > return EFI_INVALID_PARAMETER;
> > }
> >
> > @@ -116,7 +118,7 @@ HelperManageabilityCheckSupportedSpec (
> > ))
> > {
> > DEBUG ((
> > - DEBUG_VERBOSE,
> > + DEBUG_MANAGEABILITY_INFO,
> > "%a: Transport interface %s supports %s manageability
> specification.\n",
> > __FUNCTION__,
> > HelperManageabilitySpecName (TransportGuid), @@ -174,7 +176,7
> > @@ HelperAcquireManageabilityTransport (
> > return EFI_UNSUPPORTED;
> > }
> >
> > - DEBUG ((DEBUG_INFO, " Manageability protocol %s is going to
> > acquire transport interface token...\n", ManageabilityProtocolName));
> > + DEBUG ((DEBUG_MANAGEABILITY_INFO, " Manageability protocol %s is
> > + going to acquire transport interface token...\n",
> > + ManageabilityProtocolName));
> >
> > Status = AcquireTransportSession (ManageabilityProtocolSpec,
> > TransportToken);
> > if (Status == EFI_UNSUPPORTED) {
> > @@ -199,7 +201,7 @@ HelperAcquireManageabilityTransport (
> > return EFI_UNSUPPORTED;
> > }
> >
> > - DEBUG ((DEBUG_INFO, "%a: This is the transfer session for %s over
> > %s\n", __FUNCTION__, ManageabilityProtocolName,
> > ManageabilityTransportName));
> > + DEBUG ((DEBUG_MANAGEABILITY_INFO, "%a: This is the transfer
> session
> > + for %s over %s\n", __FUNCTION__, ManageabilityProtocolName,
> > + ManageabilityTransportName));
> > return Status;
> > }
> >
> > @@ -259,3 +261,206 @@ HelperInitManageabilityTransport (
> >
> > return Status;
> > }
> > +
> > +/**
> > + This function generates CRC8 with given polynomial.
> > +
> > + @param[in] Polynomial Polynomial in 8-bit.
> > + @param[in] CrcInitialValue CRC initial value.
> > + @param[in] BufferStart Pointer to buffer starts the CRC
> > calculation.
> > + @param[in] BufferSize Size of buffer.
> > +
> > + @retval UINT8 CRC value.
> > +**/
> > +UINT8
> > +HelperManageabilityGenerateCrc8 (
> > + IN UINT8 Polynomial,
> > + IN UINT8 CrcInitialValue,
> > + IN UINT8 *BufferStart,
> > + IN UINT32 BufferSize
> > + )
> > +{
> > + UINT8 BitIndex;
> > + UINT32 BufferIndex;
> > +
> > + BufferIndex = 0;
> > + while (BufferIndex < BufferSize) {
> > + CrcInitialValue = CrcInitialValue ^ *(BufferStart + BufferIndex);
> > + BufferIndex++;
> > +
> > + for (BitIndex = 0; BitIndex < 8; BitIndex++) {
> > + if ((CrcInitialValue & 0x80) != 0) {
> > + CrcInitialValue = (CrcInitialValue << 1) ^ Polynomial;
> > + } else {
> > + CrcInitialValue <<= 1;
> > + }
> > + }
> > + }
> > +
> > + return CrcInitialValue;
> > +}
> > +
> > +/**
> > + This function splits payload into multiple packages according to
> > + the given transport interface Maximum Transfer Unit (MTU).
> > +
> > +
> > + @param[in] PreambleSize The additional data size precedes
> > + each package.
> > + @param[in] PostambleSize The additional data size succeeds
> > + each package.
> > + @param[in] Payload Pointer to payload.
> > + @param[in] PayloadSize Payload size in byte.
> > + @param[in] MaximumTransferUnit MTU of transport interface.
> > + @param[out] MultiplePackages Pointer to receive
> > +
> > MANAGEABILITY_TRANSMISSION_MULTI_PACKAGES
> > + structure. Caller has to free the memory
> > + allocated for
> > MANAGEABILITY_TRANSMISSION_MULTI_PACKAGES.
> > +
> > + @retval EFI_SUCCESS
> > MANAGEABILITY_TRANSMISSION_MULTI_PACKAGES structure
> > + is returned successfully.
> > + @retval EFI_OUT_OF_RESOURCE Not enough resource to create
> > +
> > + MANAGEABILITY_TRANSMISSION_MULTI_PACKAGES
> > structure.
> > +**/
> > +EFI_STATUS
> > +HelperManageabilitySplitPayload (
> > + IN UINT16 PreambleSize,
> > + IN UINT16 PostambleSize,
> > + IN UINT8 *Payload,
> > + IN UINT32 PayloadSize,
> > + IN UINT32 MaximumTransferUnit,
> > + OUT MANAGEABILITY_TRANSMISSION_MULTI_PACKAGES
> > **MultiplePackages
> > + )
> > +{
> > + UINT16 NumberOfPackages;
> > + UINT16 IndexOfPackage;
> > + UINT32 PackagePayloadSize;
> > + UINT32 TotalPayloadRemaining;
> > + MANAGEABILITY_TRANSMISSION_MULTI_PACKAGES
> *ThisMultiplePackages;
> > + MANAGEABILITY_TRANSMISSION_PACKAGE_ATTR *ThisPackage;
> > +
> > + if ((INT16)(MaximumTransferUnit - PreambleSize - PostambleSize) < 0) {
> > + DEBUG ((
> > + DEBUG_ERROR,
> > + "%a: (Preamble 0x%x + PostambleSize 0x%x) is greater than
> > MaximumTransferUnit 0x%x.\n",
> > + __FUNCTION__,
> > + PreambleSize,
> > + PostambleSize,
> > + MaximumTransferUnit
> > + ));
> > + return EFI_INVALID_PARAMETER;
> > + }
> > +
> > + PackagePayloadSize = MaximumTransferUnit -PreambleSize -
> PostambleSize;
> > + NumberOfPackages = (UINT16)((PayloadSize + (PackagePayloadSize -
> 1)) /
> > PackagePayloadSize);
> > + ThisMultiplePackages =
> (MANAGEABILITY_TRANSMISSION_MULTI_PACKAGES
> > *)AllocateZeroPool (
> > +
> > + sizeof
> > (MANAGEABILITY_TRANSMISSION_MULTI_PACKAGES) +
> > +
> > + sizeof
> > (MANAGEABILITY_TRANSMISSION_PACKAGE_ATTR) * NumberOfPackages
> > +
> > + ); if (ThisMultiplePackages == NULL) {
> > + DEBUG ((DEBUG_ERROR, "%a: Not enough memory for
> > MANAGEABILITY_TRANSMISSION_MULTI_PACKAGES\n"));
> > + return EFI_INVALID_PARAMETER;
>
> Since there is no memory to allocate, the return value should be
> EFI_OUT_OF_RESOURCE.
>
>
>
> > + }
> > +
> > + ThisMultiplePackages->NumberOfPackages = NumberOfPackages;
> > + ThisPackage =
> > (MANAGEABILITY_TRANSMISSION_PACKAGE_ATTR
> *)(ThisMultiplePackages + 1);
> > + TotalPayloadRemaining = PayloadSize;
> > + for (IndexOfPackage = 0; IndexOfPackage < NumberOfPackages;
> > IndexOfPackage++) {
> > + ThisPackage->PayloadPointer = Payload + (IndexOfPackage *
> > PackagePayloadSize);
> > + ThisPackage->PayloadSize = MIN (TotalPayloadRemaining,
> > PackagePayloadSize);
> > + TotalPayloadRemaining -= ThisPackage->PayloadSize;
> > + ThisPackage++;
> > + }
> > +
> > + if (TotalPayloadRemaining != 0) {
> > + DEBUG ((DEBUG_ERROR, "%a: Error processing multiple packages
> > (TotalPayloadRemaining != 0)\n", __FUNCTION__));
>
> Before this error return, shall we release ThisMultiplePackages buffer?
>
>
>
> > + return EFI_INVALID_PARAMETER;
> > + }
> > +
> > + *MultiplePackages = ThisMultiplePackages;
> > + return EFI_SUCCESS;
> > +}
> > +
> > +/**
> > + Print out manageability transmit payload to the debug output device.
> > +
> > + @param[in] Payload Payload to print.
> > + @param[in] PayloadSize Payload size.
> > +
> > +**/
> > +VOID
> > +EFIAPI
> > +HelperManageabilityPayLoadDebugPrint (
> > + IN VOID *Payload,
> > + IN UINT32 PayloadSize
> > + )
> > +{
> > + UINT16 Page256;
> > + UINT16 Row16;
> > + UINT16 Column16;
> > + UINT32 RemainingBytes;
> > + UINT32 TotalBytePrinted;
> > +
> > + RemainingBytes = PayloadSize;
> > + TotalBytePrinted = 0;
> > + while (TRUE) {
> > + if (TotalBytePrinted % 256 == 0) {
> > + Page256 = (UINT16)TotalBytePrinted / 256;
> > + DEBUG ((DEBUG_MANAGEABILITY_INFO, "======== Manageability
> > Payload %04xH - %04xH =========\n", Page256 * 256, Page256 * 256 +
> MIN
> > (RemainingBytes, 256) - 1));
> > + DEBUG ((DEBUG_MANAGEABILITY_INFO, " "));
> > + for (Column16 = 0; Column16 < 16; Column16++) {
> > + DEBUG ((DEBUG_MANAGEABILITY_INFO, "%02x ", Column16));
> > + }
> > +
> > + DEBUG ((DEBUG_MANAGEABILITY_INFO, "\n
> > --------------------------
> ------
> > ---------------\n"));
> > + }
> > +
> > + for (Row16 = 0; Row16 < 16; Row16++) {
> > + DEBUG ((DEBUG_MANAGEABILITY_INFO, "%04x | ", Page256 * 256 +
> > Row16 * 16));
> > + for (Column16 = 0; Column16 < MIN (RemainingBytes, 16); Column16++)
> {
> > + DEBUG ((DEBUG_MANAGEABILITY_INFO, "%02x ", *((UINT8
> *)Payload
> > + +
> > Page256 * 256 + Row16 * 16 + Column16)));
> > + }
> > +
> > + RemainingBytes -= Column16;
> > + TotalBytePrinted += Column16;
> > + if (RemainingBytes == 0) {
> > + DEBUG ((DEBUG_MANAGEABILITY_INFO, "\n\n"));
> > + return;
> > + }
> > +
> > + DEBUG ((DEBUG_MANAGEABILITY_INFO, "\n"));
> > + }
> > +
> > + DEBUG ((DEBUG_MANAGEABILITY_INFO, "\n")); }
> > +
> > + DEBUG ((DEBUG_MANAGEABILITY_INFO, "\n\n")); }
> > +
> > +/**
> > + Prints a debug message and manageability payload to the debug
> > +output
> > device.
> > +
> > + @param[in] Payload Payload to print.
> > + @param[in] PayloadSize Payload size.
> > + @param[in] Format The format string for the debug message to
> > print.
> > + @param[in] ... The variable argument list whose contents are
> accessed
> > + based on the format string specified by Format.
> > +
> > +**/
> > +VOID
> > +EFIAPI
> > +HelperManageabilityDebugPrint (
> > + IN VOID *Payload,
> > + IN UINT32 PayloadSize,
> > + IN CONST CHAR8 *Format,
> > + ...
> > + )
> > +{
> > + VA_LIST Marker;
> > +
> > + VA_START (Marker, Format);
> > + DEBUG ((DEBUG_MANAGEABILITY_INFO, "Manageability Transmission:
> "));
> > + DebugVPrint ((UINTN)DEBUG_MANAGEABILITY_INFO, Format, Marker);
> > + HelperManageabilityPayLoadDebugPrint (Payload, PayloadSize);
> > + VA_END (Marker);
> > +}
> > --
> > 2.37.1.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#103314): https://edk2.groups.io/g/devel/message/103314
Mute This Topic: https://groups.io/mt/98339112/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
