Re: [edk2-devel] [edk2-platforms][PATCH 01/14] ManageabilityPkg: Add more helper functions

["Tinh Nguyen" <tinhnguyen@os.amperecomputing.com>]

Please find my inline comment below

On 4/3/2023 10:04 PM, Chang, Abner via groups.io wrote:
> From: Abner Chang <abner.chang@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.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>
> ---
>   .../BaseManageabilityTransportHelper.inf      |   1 +
>   .../Library/ManageabilityTransportHelperLib.h |  98 ++++++++
>   .../BaseManageabilityTransportHelper.c        | 218 +++++++++++++++++-
>   3 files changed, 310 insertions(+), 7 deletions(-)
>
> diff --git a/Features/ManageabilityPkg/Library/BaseManageabilityTransportHelperLib/BaseManageabilityTransportHelper.inf b/Features/ManageabilityPkg/Library/BaseManageabilityTransportHelperLib/BaseManageabilityTransportHelper.inf
> index 95c3362ddb..c31a89aa49 100644
> --- a/Features/ManageabilityPkg/Library/BaseManageabilityTransportHelperLib/BaseManageabilityTransportHelper.inf
> +++ b/Features/ManageabilityPkg/Library/BaseManageabilityTransportHelperLib/BaseManageabilityTransportHelper.inf
> @@ -25,6 +25,7 @@
>   [LibraryClasses]
>     BaseMemoryLib
>     DebugLib
> +  MemoryAllocationLib
>   
>   [Packages]
>     ManageabilityPkg/ManageabilityPkg.dec
> diff --git a/Features/ManageabilityPkg/Include/Library/ManageabilityTransportHelperLib.h b/Features/ManageabilityPkg/Include/Library/ManageabilityTransportHelperLib.h
> index 718ac34a1f..0dbf5ccb3c 100644
> --- a/Features/ManageabilityPkg/Include/Library/ManageabilityTransportHelperLib.h
> +++ b/Features/ManageabilityPkg/Include/Library/ManageabilityTransportHelperLib.h
> @@ -11,8 +11,24 @@
>   
>   #include <Library/ManageabilityTransportLib.h>
>   
> +#define DEBUG_MANAGEABILITY_INFO  DEBUG_INFO
Why did you redefine this macro?
I saw that you used both DEBUG_INFO and DEBUG_MANAGEABILITY_INFO 
throughout this library. Please use them consistently.

If you want to change the debug level for all statements in a function,
you can declare an argument as the debug level.
> +
>   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/BaseManageabilityTransportHelperLib/BaseManageabilityTransportHelper.c b/Features/ManageabilityPkg/Library/BaseManageabilityTransportHelperLib/BaseManageabilityTransportHelper.c
> index 81da209764..9d20ed49ff 100644
> --- a/Features/ManageabilityPkg/Library/BaseManageabilityTransportHelperLib/BaseManageabilityTransportHelper.c
> +++ b/Features/ManageabilityPkg/Library/BaseManageabilityTransportHelperLib/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;
>     }
>   
> @@ -259,3 +261,205 @@ 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;
> +  }
> +
> +  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__));
> +    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);
> +}