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 > Signed-off-by: Omkar Anand Kulkarni > --- > 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.

> # TRUE - Supports update capsule across a system reset.
> 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 > +#include [SAMI] Do we need ArmLib.h here? > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +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; > +}