Re: [edk2-platforms][PATCH v3 1/5] MdeModulePkg: Allow dynamic generation of HEST ACPI table

["Sami Mujawar" <sami.mujawar@arm.com>]

[-- Attachment #1: Type: text/plain, Size: 18324 bytes --]

Hi Omkar,

Please find my feedback inline marked [SAMI].

Regards,

Sami Mujawar


On 24/08/2021 06:33 AM, Omkar Anand Kulkarni wrote:
> Introduce the HEST table generation protocol that allows platforms to
> build the table with multiple error source descriptors and install the
> table. The protocol provides two interfaces. The first interface allows
> for adding multiple error source descriptors into the HEST table. The
> second interface can then be used to dynamically install the fully
> populated HEST table. This allows multiple drivers and/or libraries to
> dynamically register error source descriptors into the HEST table.
>
> Co-authored-by: Thomas Abraham <thomas.abraham@arm.com>
> Signed-off-by: Omkar Anand Kulkarni <omkar.kulkarni@arm.com>
> ---
>   MdeModulePkg/MdeModulePkg.dec                   |   3 +
>   MdeModulePkg/Universal/Apei/HestDxe/HestDxe.inf |  49 +++
>   MdeModulePkg/Include/Protocol/HestTable.h       |  71 +++++
>   MdeModulePkg/Universal/Apei/HestDxe/HestDxe.c   | 318 ++++++++++++++++++++
>   4 files changed, 441 insertions(+)
>
> diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
> index ad84421cf3f7..2cb4ba69d6bf 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -663,6 +663,9 @@
>     ## Include/Protocol/VariablePolicy.h
>     gEdkiiVariablePolicyProtocolGuid = { 0x81D1675C, 0x86F6, 0x48DF, { 0xBD, 0x95, 0x9A, 0x6E, 0x4F, 0x09, 0x25, 0xC3 } }
>   
> +  ## Arm Platform HEST table generation protocol
> +  gHestTableProtocolGuid = { 0x705bdcd9, 0x8c47, 0x457e, { 0xad, 0x0d, 0xf7, 0x86, 0xf3, 0x4a, 0x0d, 0x63 } }
> +
>   [PcdsFeatureFlag]
>     ## Indicates if the platform can support update capsule across a system reset.<BR><BR>
>     #   TRUE  - Supports update capsule across a system reset.<BR>
> diff --git a/MdeModulePkg/Universal/Apei/HestDxe/HestDxe.inf b/MdeModulePkg/Universal/Apei/HestDxe/HestDxe.inf
> new file mode 100644
> index 000000000000..91c7385bf7ff
> --- /dev/null
> +++ b/MdeModulePkg/Universal/Apei/HestDxe/HestDxe.inf
> @@ -0,0 +1,49 @@
> +## @file
> +#  Dxe driver that creates and publishes the HEST table.
> +#
> +#  This driver creates HEST header and provides protocol service to append
> +#  and install the HEST table.
> +#
> +#  Copyright (c) 2020 - 2021, ARM Limited. All rights reserved.
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 0x0001001A
[SAMI] Use latest INF version.
> +  BASE_NAME                      = HestDxe
> +  FILE_GUID                      = 933099a2-ef71-4e00-82aa-a79b1e0a3b38
> +  MODULE_TYPE                    = DXE_DRIVER
> +  VERSION_STRING                 = 1.0
> +  ENTRY_POINT                    = HestInitialize
> +
> +[Sources.Common]
> +  HestDxe.c
> +
> +[Packages]
> +  ArmPkg/ArmPkg.dec
> +  ArmPlatformPkg/ArmPlatformPkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  MdePkg/MdePkg.dec
> +  Platform/ARM/SgiPkg/SgiPlatform.dec
[SAMI] Is SgiPlatform.dec, ArmPkg.dec and ArmPlatformPkg.dec needed here?
> +
> +[LibraryClasses]
> +  ArmLib
[SAMI] Is ArmLib needed here?
> +  BaseMemoryLib
> +  DebugLib
> +  UefiDriverEntryPoint
> +  UefiLib
> +
> +[Protocols]
> +  gEfiAcpiTableProtocolGuid         ## PROTOCOL ALWAYS_CONSUMED
> +  gHestTableProtocolGuid            ## PRODUCES
> +
> +[FixedPcd]
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultCreatorId
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultCreatorRevision
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultOemId
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultOemRevision
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultOemTableId
> +
> +[Depex]
> +  TRUE
> diff --git a/MdeModulePkg/Include/Protocol/HestTable.h b/MdeModulePkg/Include/Protocol/HestTable.h
> new file mode 100644
> index 000000000000..3b2e1f7d9203
> --- /dev/null
> +++ b/MdeModulePkg/Include/Protocol/HestTable.h
[SAMI] Should this file be renamed to HestTableProtocol.h?
> @@ -0,0 +1,71 @@
> +/** @file
> +  Builds and installs the HEST ACPI table.
> +
> +  Define the protocol interface that allows HEST ACPI table to be created,
> +  populated with error record descriptions and installation of the HEST ACPI
> +  table.
> +
> +  Copyright (c) 2020 - 2021, ARM Limited. All rights reserved.
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +
> +#ifndef HEST_TABLE_H_
> +#define HEST_TABLE_H_
> +
> +#define HEST_TABLE_PROTOCOL_GUID \
> +  { \
> +    0x705bdcd9, 0x8c47, 0x457e, \
> +    { 0xad, 0x0d, 0xf7, 0x86, 0xf3, 0x4a, 0x0d, 0x63 } \
> +  }
> +
> +/**
> +  Append HEST error source descriptor protocol service.
> +
> +  Protocol service used to append newly collected error source descriptors to
> +  to an already created HEST table.
> +
> +  @param[in]  ErrorSourceDescriptorList      List of Error Source Descriptors.
> +  @param[in]  ErrorSourceDescriptorListSize  Total Size of Error Source
> +                                             Descriptors.
> +  @param[in]  ErrorSourceDescriptorCount     Total count of error source
> +                                             descriptors.
> +
> +  @retval  EFI_SUCCESS            Appending the error source descriptors
> +                                  successful.
> +  @retval  EFI_OUT_OF_RESOURCES   Buffer reallocation failed for the Hest
> +                                  table.
> +  @retval  EFI_INVALID_PARAMETER  Null ErrorSourceDescriptorList param or
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *APPEND_ERROR_SOURCE_DESCRIPTOR) (
> +  IN CONST VOID *ErrorSourceDescriptorList,
> +  IN UINTN      ErrorSourceDescriptorListSize,
> +  IN UINTN      ErrorSourceDescriptorCount
> +  );
> +
> +/**
> +  Install HEST table protocol service.
> +
> +  Installs the HEST table that has been appended with the error source
> +  descriptors. The checksum of this table is calculated and updated in
> +  the table header. The HEST table is then installed.
> +
> +  @retval  EFI_SUCCESS  HEST table is installed successfully.
> +  @retval  EFI_ABORTED  HEST table is NULL.
> +  @retval  Other        Install service call failed.
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *INSTALL_HEST_TABLE) (VOID);
> +
> +//
> +// HEST_TABLE_PROTOCOL enables creation and installation of HEST table
> +//
> +typedef struct {
> +  APPEND_ERROR_SOURCE_DESCRIPTOR AppendErrorSourceDescriptors;
> +  INSTALL_HEST_TABLE             InstallHestTable;
> +} HEST_TABLE_PROTOCOL;
[SAMI] I think HEST_TABLE_PROTOCOL  should be renamed 
toEDKII_HEST_TABLE_PROTOCOL.
Similarly, the function pointers to the interfaces defined by the 
protocol should also be prefixed with EDKII_, e.g. EDKII_INSTALL_HEST_TABLE.
[/SAMI]
> +
> +extern EFI_GUID gHestTableProtocolGuid;
> +#endif  // HEST_TABLE_H_
> diff --git a/MdeModulePkg/Universal/Apei/HestDxe/HestDxe.c b/MdeModulePkg/Universal/Apei/HestDxe/HestDxe.c
> new file mode 100644
> index 000000000000..87f21acf61f4
> --- /dev/null
> +++ b/MdeModulePkg/Universal/Apei/HestDxe/HestDxe.c
> @@ -0,0 +1,318 @@
> +/** @file
> +  Builds and installs the HEST ACPI table.
> +
> +  This driver installs a protocol that can be used to create and install a HEST
> +  ACPI table. The protocol allows one or more error source producers to add the
> +  error source descriptors into the HEST table. It also allows the resulting
> +  HEST table to be installed.
> +
> +  Copyright (c) 2020 - 2021, ARM Limited. All rights reserved.
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +  @par Specification Reference:
> +    - ACPI 6.3, Table 18-382, Hardware Error Source Table
> +**/
> +
> +#include <IndustryStandard/Acpi.h>
> +#include <Library/ArmLib.h>
[SAMI] Do we need ArmLib.h here?
> +#include <Library/BaseLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Protocol/AcpiSystemDescriptionTable.h>
> +#include <Protocol/AcpiTable.h>
> +#include <Protocol/HestTable.h>
> +
> +typedef struct {
> +  VOID   *HestTable;        /// Pointer to HEST table.
> +  UINT32 CurrentTableSize;  /// Current size of HEST table.
> +} HEST_DXE_DRIVER_DATA;
> +
> +STATIC EFI_ACPI_TABLE_PROTOCOL *mAcpiTableProtocol = NULL;
> +STATIC HEST_DXE_DRIVER_DATA mHestDriverData;
> +
> +/**
> +  Allocate memory for the HEST table header and populate it.
> +
> +  Helper function for this driver, populates the HEST table header. Called once
> +  in the beginning on first invocation of append error source descriptor
> +  protocol service.
> +
> +  @retval  EFI_SUCCESS           On successful creation of HEST header.
> +  @retval  EFI_OUT_OF_RESOURCES  If system is out of memory recources.
> +**/
> +STATIC
> +EFI_STATUS
> +BuildHestHeader (
> +  VOID
> +  )
> +{
> +  EFI_ACPI_6_3_HARDWARE_ERROR_SOURCE_TABLE_HEADER *HestHeader;
> +
> +  //
> +  // Allocate memory for the HEST table header.
> +  //
> +  mHestDriverData.HestTable =
> +    AllocateZeroPool (sizeof (EFI_ACPI_6_3_HARDWARE_ERROR_SOURCE_TABLE_HEADER));
> +  if (mHestDriverData.HestTable == NULL) {
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  mHestDriverData.CurrentTableSize =
> +    sizeof (EFI_ACPI_6_3_HARDWARE_ERROR_SOURCE_TABLE_HEADER);
> +
> +  HestHeader = (EFI_ACPI_6_3_HARDWARE_ERROR_SOURCE_TABLE_HEADER *)
> +                  mHestDriverData.HestTable;
> +
> +  //
> +  // Populate the values of the HEST table header.
> +  //
> +  HestHeader->Header.Signature =
> +    EFI_ACPI_6_3_HARDWARE_ERROR_SOURCE_TABLE_SIGNATURE;
> +  HestHeader->Header.Revision =
> +    EFI_ACPI_6_3_HARDWARE_ERROR_SOURCE_TABLE_REVISION;
> +  CopyMem (
> +    &HestHeader->Header.OemId,
> +    FixedPcdGetPtr (PcdAcpiDefaultOemId),
> +    sizeof (HestHeader->Header.OemId)
> +    );
> +  HestHeader->Header.OemTableId = FixedPcdGet64 (PcdAcpiDefaultOemTableId);
> +  HestHeader->Header.OemRevision = PcdGet32 (PcdAcpiDefaultOemRevision);
> +  HestHeader->Header.CreatorId = PcdGet32 (PcdAcpiDefaultCreatorId);
> +  HestHeader->Header.CreatorRevision = PcdGet32 (PcdAcpiDefaultCreatorRevision);
> +  HestHeader->ErrorSourceCount = 0;
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  Append HEST error source descriptor protocol service.
> +
> +  Protocol service used to append newly collected error source descriptors to
> +  to an already created HEST table.
> +
> +  @param[in]  ErrorSourceDescriptorList      List of Error Source Descriptors.
> +  @param[in]  ErrorSourceDescriptorListSize  Total Size of Error Source
> +                                             Descriptors.
> +  @param[in]  ErrorSourceDescriptorCount     Total count of error source
> +                                             descriptors.
> +
> +  @retval  EFI_SUCCESS            Appending the error source descriptors
> +                                  successful.
> +  @retval  EFI_OUT_OF_RESOURCES   Buffer reallocation failed for the Hest
> +                                  table.
> +  @retval  EFI_INVALID_PARAMETER  Null ErrorSourceDescriptorList param or
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +AppendErrorSourceDescriptor (
> +  IN CONST VOID *ErrorSourceDescriptorList,
> +  IN UINTN      ErrorSourceDescriptorListSize,
> +  IN UINTN      ErrorSourceDescriptorCount
> +  )
> +{
> +  EFI_ACPI_6_3_HARDWARE_ERROR_SOURCE_TABLE_HEADER *HestHeaderPtr;
> +  EFI_STATUS                                      Status;
> +  UINT32                                          NewTableSize;
> +  VOID                                            *ErrorDescriptorPtr;
> +
> +  if ((ErrorSourceDescriptorList == NULL) ||
> +      (ErrorSourceDescriptorListSize == 0)) {
> +     return EFI_INVALID_PARAMETER;
> +  }
> +
> +  //
> +  // Create a HEST table header if not already created.
> +  //
> +  if (mHestDriverData.HestTable == NULL) {
> +    Status = BuildHestHeader ();
> +    if (Status == EFI_OUT_OF_RESOURCES) {
> +      DEBUG ((
> +        DEBUG_ERROR,
> +        "%a: Failed to build HEST header, status: %r\n",
> +        __FUNCTION__,
> +        Status
> +        ));
> +      return Status;
> +    }
> +  }
> +
> +  //
> +  // Resize the existing HEST table buffer to accommodate the incoming error
> +  // source descriptors.
> +  //
> +  NewTableSize = mHestDriverData.CurrentTableSize +
> +                 ErrorSourceDescriptorListSize;
> +  mHestDriverData.HestTable = ReallocatePool (
> +                                mHestDriverData.CurrentTableSize,
> +                                NewTableSize,
> +                                mHestDriverData.HestTable
> +                                );
[SAMI] I think a Linked list based implementation should be used here, 
instead of using ReallocatePool. This should not be too complicated as 
the HEST_TABLE_PROTOCOL design already makes this easy. The 
AppendErrorSourceDescriptor() interface should probably be renamed to 
AddErrorSourceDescriptor() and can be used to collect the error source 
information and add it to a LinkedList. The InstallHestTable() can 
iterate the linked list to collect the error sources and publist the 
ACPI table. Also, there is a LinkedList implementation in 
edk2\MdePkg\BaseLib\LinkedList.c that can be used.
  [/SAMI]
> +  if (mHestDriverData.HestTable == NULL) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "%a: Failed to reallocate memory for HEST table\n",
> +      __FUNCTION__
> +      ));
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  //
> +  // Copy the incoming error source descriptors into HEST table.
> +  //
> +  ErrorDescriptorPtr = (VOID *)mHestDriverData.HestTable +
> +                       mHestDriverData.CurrentTableSize;
> +  HestHeaderPtr = (EFI_ACPI_6_3_HARDWARE_ERROR_SOURCE_TABLE_HEADER *)
> +                  mHestDriverData.HestTable;
> +  CopyMem (
> +    ErrorDescriptorPtr,
> +    ErrorSourceDescriptorList,
> +    ErrorSourceDescriptorListSize
> +    );
> +  mHestDriverData.CurrentTableSize = NewTableSize;
> +  HestHeaderPtr->Header.Length = mHestDriverData.CurrentTableSize;
> +  HestHeaderPtr->ErrorSourceCount += ErrorSourceDescriptorCount;
> +
> +  DEBUG ((
> +    DEBUG_INFO,
> +    "HestDxe: %d Error source descriptor(s) added \n",
> +    ErrorSourceDescriptorCount
> +    ));
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  Install HEST table protocol service.
> +
> +  Installs the HEST table that has been appended with the error source
> +  descriptors. The checksum of this table is calculated and updated in
> +  the table header. The HEST table is then installed.
> +
> +  @retval  EFI_SUCCESS  HEST table is installed successfully.
> +  @retval  EFI_ABORTED  HEST table is NULL.
> +  @retval  Other        Install service call failed.
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +InstallHestAcpiTable (
> +  VOID
> +  )
> +{
> +  EFI_ACPI_6_3_HARDWARE_ERROR_SOURCE_TABLE_HEADER *HestHeader;
> +  EFI_STATUS                                      Status;
> +  UINTN                                           AcpiTableHandle;
> +
> +  //
> +  // Check if we have valid HEST table data. If not, there no hardware error
> +  // sources supported by the platform and no HEST table to publish, return.
> +  //
> +  if (mHestDriverData.HestTable == NULL) {
> +    DEBUG ((
> +      DEBUG_INFO,
> +      "HestDxe: No data available to generate HEST table\n"
> +      ));
> +    return EFI_NOT_FOUND;
> +  }
> +
> +  //
> +  // Valid data for HEST table found. Update the header checksum and install the
> +  // HEST table.
> +  //
> +  HestHeader = (EFI_ACPI_6_3_HARDWARE_ERROR_SOURCE_TABLE_HEADER *)
> +               mHestDriverData.HestTable;
> +
> +  Status = mAcpiTableProtocol->InstallAcpiTable (
> +                                 mAcpiTableProtocol,
> +                                 HestHeader,
> +                                 HestHeader->Header.Length,
> +                                 &AcpiTableHandle
> +                                 );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "%a: HEST table installation failed, status: %r\n",
> +      __FUNCTION__,
> +      Status
> +      ));
> +    return Status;
[SAMI] There would be a memory leak here as mHestDriverData.HestTableis 
not freed. So just remove the return statement from here.
> +  }
> +
> +  //
> +  // Free the HEST table buffer.
> +  //
> +  FreePool (mHestDriverData.HestTable);
> +  DEBUG ((
> +    DEBUG_INFO,
> +    "HestDxe: Installed HEST table \n"
> +    ));
> +  return Status;
> +}
> +
> +//
> +// HEST table generation protocol instance.
> +//
> +STATIC HEST_TABLE_PROTOCOL mHestProtocol = {
> +  AppendErrorSourceDescriptor,
> +  InstallHestAcpiTable
> +};
> +
> +/**
> +  The Entry Point for HEST Dxe driver.
> +
> +  This function installs the HEST table creation and installation protocol
> +  services.
> +
> +  @param[in]  ImageHandle  Handle to the Efi image.
> +  @param[in]  SystemTable  A pointer to the Efi System Table.
> +
> +  @retval EFI_SUCCESS    On successful installation of protocol services and
> +                         location the ACPI table protocol.
> +  @retval Other          On Failure to locate ACPI table protocol or install
> +                         of HEST table generation protocol.
> +**/
> +EFI_STATUS
> +EFIAPI
> +HestInitialize (
> +  IN EFI_HANDLE       ImageHandle,
> +  IN EFI_SYSTEM_TABLE *SystemTable
> +  )
> +{
> +  EFI_HANDLE Handle = NULL;
> +  EFI_STATUS Status;
> +
> +  Status = gBS->LocateProtocol (
> +                  &gEfiAcpiTableProtocolGuid,
> +                  NULL,
> +                  (VOID **)&mAcpiTableProtocol
> +                  );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "%a: Failed to locate ACPI table protocol, status: %r\n",
> +      __FUNCTION__,
> +      Status
> +      ));
> +    return Status;
> +  }
> +
> +  Status = gBS->InstallProtocolInterface (
> +                  &Handle,
> +                  &gHestTableProtocolGuid,
> +                  EFI_NATIVE_INTERFACE,
> +                  &mHestProtocol
> +                  );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "%a: Failed to install HEST table generation protocol status: %r\n",
> +      __FUNCTION__,
> +      Status
> +      ));
> +    return Status;
[SAMI] This return statement can be removed as there is already one at 
the end of the function.
> +  }
> +
> +  return Status;
> +}


[-- Attachment #2: Type: text/html, Size: 19495 bytes --]