[PATCH v2 0/4] ArmPlatformPkg: Add support to generate HEST ACPI table

[PATCH v2 0/4] ArmPlatformPkg: Add support to generate HEST ACPI table

[Omkar Anand Kulkarni]

Changes since v1:
- Helper added for HEST ACPI table generation.
- Rebased to the latest upstream code.

Hardware Error Source Table (HEST)[1] and Software Delegated Exception Interface
(SDEI)[2] ACPI tables are used to acomplish firmware first error handling.This
patch series introduces a framework to build and install the HEST ACPI table
dynamically.

The following figure illustrates the possible usage of the dyanamic
generation of HEST ACPI table.

                                    NS | S
+--------------------------------------+--------------------------------------+
|                                      |                                      |
|+-------------------------------------+---------------------+                |
||               +---------------------+--------------------+|                |
||               |                     |                    ||                |
|| +-----------+ |+------------------+ | +-----------------+|| +-------------+|
|| |HestTable  | ||  HestErrorSource | | | HestErrorSource ||| | DMC-620     ||
|| |  DXE      | ||        DXE       | | |  StandaloneMM   ||| |Standalone MM||
|| +-----------+ |+------------------+ | +-----------------+|| +-------------+|
||               |GHESv2               |                    ||                |
||               +---------------------+--------------------+|                |
||          +--------------------+     |                     |                |
||          |PlatformErrorHandler|     |                     |                |
||          |        DXE         |     |                     |                |
||          +--------------------+     |                     |                |
||FF FWK                               |                     |                |
|+-------------------------------------+---------------------+                |
|                                      |                                      |
+--------------------------------------+--------------------------------------+
                                       |
                   Figure: Firmware First Error Handling approach.

All the hardware error sources are added to HEST table as GHESv2[3] error source
descriptors. The framework comprises of following DXE and MM drivers:

- HestTableDxe:
  Builds HEST table header and allows appending error source descriptors to the
  HEST table. Also provides protocol interface to install the built HEST table.

- HestErrorSourceDxe & HestErrorSourceStandaloneMM:
  These two drivers together retrieve all possible error source descriptors of
  type GHESv2 from the MM drivers implementing HEST Error Source Descriptor
  protocol. Once all the descriptors are collected HestErrorSourceDxe appends
  it to HEST table using HestTableDxe driver.
  
- PlatformErrorHandlerDxe:
  Builds and installs SDEI ACPI table. This driver does not initialize(load)
  until HestErrorSourceDxe driver has finished appending all possible GHESv2
  error source descriptors to the HEST table. Once that is complete using the
  HestTableDxe driver it installs the HEST table.

This patch series provides reference implementation for DMC-620 Dynamic Memory
Controller[4] that has RAS feature enabled. This is platform code
implemented as Standalone MM driver in edk2-platforms.

References:
[1] : ACPI 6.3, Table 18-382, Hardware Error Source Table
[2] : SDEI Platform Design Document, revision b, 10 Appendix C, ACPI table
      definitions for SDEI
[3] : ACPI Reference Specification 6.3, Table 18-393 GHESv2 Structure
[4] : DMC620 Dynamic Memory Controller, revision r1p0
[5] : UEFI Reference Specification 2.8, Appendix N - Common Platform Error
      Record
[6] : UEFI Reference Specification 2.8, Section N.2.5 Memory Error Section

Link to github branch with the patches in this series -
https://github.com/omkkul01/edk2/tree/ras_firmware_first_edk2

Omkar Anand Kulkarni (4):
  ArmPlatformPkg: Allow dynamic generation of HEST ACPI table
  ArmPlatformPkg: add definition for MM_HEST_ERROR_SOURCE_DESC_PROTOCOL
  ArmPlatformPkg: retreive error source descriptors from MM
  ArmPlatformPkg: Add helpers for HEST table generation

 ArmPlatformPkg/ArmPlatformPkg.dec             |  12 +
 .../Drivers/Apei/HestDxe/HestDxe.inf          |  49 +++
 .../HestMmErrorSources/HestErrorSourceDxe.inf |  44 +++
 .../HestErrorSourceStandaloneMm.inf           |  51 +++
 .../HestMmErrorSourceCommon.h                 |  37 ++
 ArmPlatformPkg/Include/HestAcpiHeader.h       |  49 +++
 .../Include/Protocol/HestErrorSourceInfo.h    |  64 ++++
 ArmPlatformPkg/Include/Protocol/HestTable.h   |  71 ++++
 ArmPlatformPkg/Drivers/Apei/HestDxe/HestDxe.c | 354 ++++++++++++++++++
 .../HestMmErrorSources/HestErrorSourceDxe.c   | 308 +++++++++++++++
 .../HestErrorSourceStandaloneMm.c             | 312 +++++++++++++++
 11 files changed, 1351 insertions(+)
 create mode 100644 ArmPlatformPkg/Drivers/Apei/HestDxe/HestDxe.inf
 create mode 100644 ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceDxe.inf
 create mode 100644 ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceStandaloneMm.inf
 create mode 100644 ArmPlatformPkg/Drivers/HestMmErrorSources/HestMmErrorSourceCommon.h
 create mode 100644 ArmPlatformPkg/Include/HestAcpiHeader.h
 create mode 100644 ArmPlatformPkg/Include/Protocol/HestErrorSourceInfo.h
 create mode 100644 ArmPlatformPkg/Include/Protocol/HestTable.h
 create mode 100644 ArmPlatformPkg/Drivers/Apei/HestDxe/HestDxe.c
 create mode 100644 ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceDxe.c
 create mode 100644 ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceStandaloneMm.c

-- 
2.17.1

[PATCH v2 1/4] ArmPlatformPkg: Allow dynamic generation of HEST ACPI table

[Omkar Anand Kulkarni]

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>
---
 ArmPlatformPkg/ArmPlatformPkg.dec               |   4 +
 ArmPlatformPkg/Drivers/Apei/HestDxe/HestDxe.inf |  49 +++
 ArmPlatformPkg/Include/Protocol/HestTable.h     |  71 ++++
 ArmPlatformPkg/Drivers/Apei/HestDxe/HestDxe.c   | 354 ++++++++++++++++++++
 4 files changed, 478 insertions(+)

diff --git a/ArmPlatformPkg/ArmPlatformPkg.dec b/ArmPlatformPkg/ArmPlatformPkg.dec
index 3a25ddcdc8ca..e4afe5da8e11 100644
--- a/ArmPlatformPkg/ArmPlatformPkg.dec
+++ b/ArmPlatformPkg/ArmPlatformPkg.dec
@@ -127,3 +127,7 @@
   gArmPlatformTokenSpaceGuid.PcdPL031RtcPpmAccuracy|300000000|UINT32|0x00000022
 
   gArmPlatformTokenSpaceGuid.PcdWatchdogCount|0x0|UINT32|0x00000033
+
+[Protocols.common]
+  ## Arm Platform HEST table generation protocol
+  gHestTableProtocolGuid = { 0x705bdcd9, 0x8c47, 0x457e, { 0xad, 0x0d, 0xf7, 0x86, 0xf3, 0x4a, 0x0d, 0x63 } }
diff --git a/ArmPlatformPkg/Drivers/Apei/HestDxe/HestDxe.inf b/ArmPlatformPkg/Drivers/Apei/HestDxe/HestDxe.inf
new file mode 100644
index 000000000000..91c7385bf7ff
--- /dev/null
+++ b/ArmPlatformPkg/Drivers/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
+  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
+
+[LibraryClasses]
+  ArmLib
+  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/ArmPlatformPkg/Include/Protocol/HestTable.h b/ArmPlatformPkg/Include/Protocol/HestTable.h
new file mode 100644
index 000000000000..3b2e1f7d9203
--- /dev/null
+++ b/ArmPlatformPkg/Include/Protocol/HestTable.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;
+
+extern EFI_GUID gHestTableProtocolGuid;
+#endif  // HEST_TABLE_H_
diff --git a/ArmPlatformPkg/Drivers/Apei/HestDxe/HestDxe.c b/ArmPlatformPkg/Drivers/Apei/HestDxe/HestDxe.c
new file mode 100644
index 000000000000..b68e1a0c4e48
--- /dev/null
+++ b/ArmPlatformPkg/Drivers/Apei/HestDxe/HestDxe.c
@@ -0,0 +1,354 @@
+/** @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>
+#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;
+
+/**
+  Helper function to the driver.
+
+  Function that reallocates memory for every new error source descriptor info
+  added.
+
+  @param[in]       OldTableSize  Old memory pool size.
+  @param[in]       NewTableSize  Required memory pool size.
+  @param[in, out]  OldBuffer     Current memory buffer pointer holding the Hest
+                                 table data.
+
+  @return  New pointer to reallocated memory pool.
+**/
+STATIC
+VOID*
+ReallocateHestTableMemory (
+  IN     UINT32 OldTableSize,
+  IN     UINT32 NewTableSize,
+  IN OUT VOID   *OldBuffer
+  )
+{
+  return ReallocateReservedPool (
+           OldTableSize,
+           NewTableSize,
+           OldBuffer
+           );
+}
+
+/**
+  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;
+  UINT64                                          TempOemTableId;
+
+  //
+  // Allocate memory for the HEST table header.
+  //
+  mHestDriverData.HestTable =
+    ReallocateHestTableMemory (
+      0,
+      sizeof (EFI_ACPI_6_3_HARDWARE_ERROR_SOURCE_TABLE_HEADER),
+      NULL
+      );
+  if (mHestDriverData.HestTable == NULL) {
+    return EFI_OUT_OF_RESOURCES;
+  }
+
+  mHestDriverData.CurrentTableSize =
+    sizeof (EFI_ACPI_6_3_HARDWARE_ERROR_SOURCE_TABLE_HEADER);
+
+  //
+  // 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)
+    );
+  TempOemTableId = FixedPcdGet64 (PcdAcpiDefaultOemTableId);
+  CopyMem (
+    &HestHeader.Header.OemTableId,
+    &TempOemTableId,
+    sizeof (HestHeader.Header.OemTableId)
+    );
+  HestHeader.Header.OemRevision = PcdGet32 (PcdAcpiDefaultOemRevision);
+  HestHeader.Header.CreatorId = PcdGet32 (PcdAcpiDefaultCreatorId);
+  HestHeader.Header.CreatorRevision = PcdGet32 (PcdAcpiDefaultCreatorRevision);
+  HestHeader.ErrorSourceCount = 0;
+  CopyMem (mHestDriverData.HestTable, &HestHeader, sizeof (HestHeader));
+
+  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 = ReallocateHestTableMemory (
+                                mHestDriverData.CurrentTableSize,
+                                NewTableSize,
+                                mHestDriverData.HestTable
+                                );
+  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_SUCCESS;
+  }
+
+  //
+  // 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;
+  HestHeader->Header.Checksum = CalculateCheckSum8 (
+                                  (UINT8 *)HestHeader,
+                                  HestHeader->Header.Length
+                                  );
+
+  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;
+  }
+
+  //
+  // Free the HEST table buffer.
+  //
+  FreePool (mHestDriverData.HestTable);
+  DEBUG ((
+    DEBUG_INFO,
+    "HestDxe: Installed HEST table \n"
+    ));
+  return EFI_SUCCESS;
+}
+
+//
+// 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;
+  }
+
+  return EFI_SUCCESS;
+}
-- 
2.17.1

Re: [PATCH v2 1/4] ArmPlatformPkg: Allow dynamic generation of HEST ACPI table

[Sami Mujawar]

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

Hi Omkar,

Please find my response marked inline as [SAMI].

Regards,

Sami Mujawar


On 10/07/2021 05:18 PM, 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>
> ---
>   ArmPlatformPkg/ArmPlatformPkg.dec               |   4 +
>   ArmPlatformPkg/Drivers/Apei/HestDxe/HestDxe.inf |  49 +++
>   ArmPlatformPkg/Include/Protocol/HestTable.h     |  71 ++++
>   ArmPlatformPkg/Drivers/Apei/HestDxe/HestDxe.c   | 354 ++++++++++++++++++++
>   4 files changed, 478 insertions(+)
>
> diff --git a/ArmPlatformPkg/ArmPlatformPkg.dec b/ArmPlatformPkg/ArmPlatformPkg.dec
> index 3a25ddcdc8ca..e4afe5da8e11 100644
> --- a/ArmPlatformPkg/ArmPlatformPkg.dec
> +++ b/ArmPlatformPkg/ArmPlatformPkg.dec
> @@ -127,3 +127,7 @@
>     gArmPlatformTokenSpaceGuid.PcdPL031RtcPpmAccuracy|300000000|UINT32|0x00000022
>   
>     gArmPlatformTokenSpaceGuid.PcdWatchdogCount|0x0|UINT32|0x00000033
> +
> +[Protocols.common]
> +  ## Arm Platform HEST table generation protocol
> +  gHestTableProtocolGuid = { 0x705bdcd9, 0x8c47, 0x457e, { 0xad, 0x0d, 0xf7, 0x86, 0xf3, 0x4a, 0x0d, 0x63 } }
> diff --git a/ArmPlatformPkg/Drivers/Apei/HestDxe/HestDxe.inf b/ArmPlatformPkg/Drivers/Apei/HestDxe/HestDxe.inf
> new file mode 100644
> index 000000000000..91c7385bf7ff
> --- /dev/null
> +++ b/ArmPlatformPkg/Drivers/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
> +  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
> +
> +[LibraryClasses]
> +  ArmLib
> +  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/ArmPlatformPkg/Include/Protocol/HestTable.h b/ArmPlatformPkg/Include/Protocol/HestTable.h
> new file mode 100644
> index 000000000000..3b2e1f7d9203
> --- /dev/null
> +++ b/ArmPlatformPkg/Include/Protocol/HestTable.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;
> +
> +extern EFI_GUID gHestTableProtocolGuid;
> +#endif  // HEST_TABLE_H_
> diff --git a/ArmPlatformPkg/Drivers/Apei/HestDxe/HestDxe.c b/ArmPlatformPkg/Drivers/Apei/HestDxe/HestDxe.c
> new file mode 100644
> index 000000000000..b68e1a0c4e48
> --- /dev/null
> +++ b/ArmPlatformPkg/Drivers/Apei/HestDxe/HestDxe.c
> @@ -0,0 +1,354 @@
> +/** @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>
> +#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;
> +
> +/**
> +  Helper function to the driver.
> +
> +  Function that reallocates memory for every new error source descriptor info
> +  added.
> +
> +  @param[in]       OldTableSize  Old memory pool size.
> +  @param[in]       NewTableSize  Required memory pool size.
> +  @param[in, out]  OldBuffer     Current memory buffer pointer holding the Hest
> +                                 table data.
> +
> +  @return  New pointer to reallocated memory pool.
> +**/
> +STATIC
> +VOID*
> +ReallocateHestTableMemory (
> +  IN     UINT32 OldTableSize,
> +  IN     UINT32 NewTableSize,
> +  IN OUT VOID   *OldBuffer
> +  )
> +{
[SAMI] Have you considered maintaining a linked list of the error 
sources and serialising the list once InstallHestTable() is called?
> +  return ReallocateReservedPool (
> +           OldTableSize,
> +           NewTableSize,
> +           OldBuffer
> +           );
[SAMI] Is this wrapper function required? Can ReallocateReservedPool() 
be used directly instead?
> +}
> +
> +/**
> +  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)
[SAMI] VOID and closing parenthesis should be on a separate line.
> +{
> +  EFI_ACPI_6_3_HARDWARE_ERROR_SOURCE_TABLE_HEADER HestHeader;
> +  UINT64                                          TempOemTableId;
> +
> +  //
> +  // Allocate memory for the HEST table header.
> +  //
> +  mHestDriverData.HestTable =
> +    ReallocateHestTableMemory (
> +      0,
> +      sizeof (EFI_ACPI_6_3_HARDWARE_ERROR_SOURCE_TABLE_HEADER),
> +      NULL
> +      );
[SAMI] Is the Relocate version of the function required here, maybe the 
Alloc version could be used.
- Should EfiReservedMemoryType be used?
     Please see extract from section 2.3.6 AArch64 Platforms of the UEFI 
2.9 specification below:
     "Note:Previous EFI specifications allowed ACPI tables loaded at 
runtime to be in the
      EfiReservedMemoryType and there was no guidance provided for other 
EFI Configuration
     Tables. EfiReservedMemoryType is not intended to be used by 
firmware. UEFI 2.0 clarified
     the situation moving forward. "
> +  if (mHestDriverData.HestTable == NULL) {
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  mHestDriverData.CurrentTableSize =
> +    sizeof (EFI_ACPI_6_3_HARDWARE_ERROR_SOURCE_TABLE_HEADER);
> +
> +  //
> +  // 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)
> +    );
> +  TempOemTableId = FixedPcdGet64 (PcdAcpiDefaultOemTableId);
> +  CopyMem (
> +    &HestHeader.Header.OemTableId,
> +    &TempOemTableId,
> +    sizeof (HestHeader.Header.OemTableId)
> +    );
[SAMI] HestHeader.Header.OemTableId = FixedPcdGet64 
(PcdAcpiDefaultOemTableId); ?
> +  HestHeader.Header.OemRevision = PcdGet32 (PcdAcpiDefaultOemRevision);
> +  HestHeader.Header.CreatorId = PcdGet32 (PcdAcpiDefaultCreatorId);
> +  HestHeader.Header.CreatorRevision = PcdGet32 (PcdAcpiDefaultCreatorRevision);
> +  HestHeader.ErrorSourceCount = 0;
> +  CopyMem (mHestDriverData.HestTable, &HestHeader, sizeof (HestHeader));
[SAMI] Is it possible to use a local HEST table pointer to initalise the 
values instead of initialising a HEST structure and then doing memcopy?
> +
> +  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 = ReallocateHestTableMemory (
> +                                mHestDriverData.CurrentTableSize,
> +                                NewTableSize,
> +                                mHestDriverData.HestTable
> +                                );
> +  if (mHestDriverData.HestTable == NULL) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "%a: Failed to reallocate memory for HEST table\n",
> +      __FUNCTION__
> +      ));
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
[SAMI] As mentioned earlier, would it be possible to maintain a link 
list instead of relocating the memory?
> +  //
> +  // 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)
[SAMI] Please update according to coding standard.
> +{
> +  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_SUCCESS;
[SAMI] Can a suitable error code be returned here? 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;
> +  HestHeader->Header.Checksum = CalculateCheckSum8 (
> +                                  (UINT8 *)HestHeader,
> +                                  HestHeader->Header.Length
> +                                  );
[SAMI] Checksum calculation is not needed as 
ACPITableProtocol->InstallAcpiTable() does this internally.
> +
> +  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;
> +  }
> +
> +  //
> +  // Free the HEST table buffer.
> +  //
> +  FreePool (mHestDriverData.HestTable);
> +  DEBUG ((
> +    DEBUG_INFO,
> +    "HestDxe: Installed HEST table \n"
> +    ));
> +  return EFI_SUCCESS;
[SAMI] return Status;
> +}
> +
> +//
> +// HEST table generation protocol instance.
> +//
> +STATIC HEST_TABLE_PROTOCOL mHestProtocol = {
> +  AppendErrorSourceDescriptor,
> +  InstallHestAcpiTable
> +};
[SAMI] HEST is platform and processor architecture independent. 
Threfore, can this implementation be paced in a common location? 
MdeModulePkg?
> +
> +/**
> +  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;
> +  }
> +
> +  return EFI_SUCCESS;
[SAMI] return Status;
> +}


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

Re: [PATCH v2 1/4] ArmPlatformPkg: Allow dynamic generation of HEST ACPI table

[Omkar Anand Kulkarni]

Hi Sami,

Thanks for reviewing this patch. Please find my response inline.

Regards,
Omkar

Hi Omkar,
Please find my response marked inline as [SAMI].
Regards,
Sami Mujawar

On 10/07/2021 05:18 PM, 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 mailto:thomas.abraham@arm.com
Signed-off-by: Omkar Anand Kulkarni mailto:omkar.kulkarni@arm.com
---
 ArmPlatformPkg/ArmPlatformPkg.dec               |   4 +
 ArmPlatformPkg/Drivers/Apei/HestDxe/HestDxe.inf |  49 +++
 ArmPlatformPkg/Include/Protocol/HestTable.h     |  71 ++++
 ArmPlatformPkg/Drivers/Apei/HestDxe/HestDxe.c   | 354 ++++++++++++++++++++
 4 files changed, 478 insertions(+)

diff --git a/ArmPlatformPkg/ArmPlatformPkg.dec b/ArmPlatformPkg/ArmPlatformPkg.dec
index 3a25ddcdc8ca..e4afe5da8e11 100644
--- a/ArmPlatformPkg/ArmPlatformPkg.dec
+++ b/ArmPlatformPkg/ArmPlatformPkg.dec
@@ -127,3 +127,7 @@
   gArmPlatformTokenSpaceGuid.PcdPL031RtcPpmAccuracy|300000000|UINT32|0x00000022
 
   gArmPlatformTokenSpaceGuid.PcdWatchdogCount|0x0|UINT32|0x00000033
+
+[Protocols.common]
+  ## Arm Platform HEST table generation protocol
+  gHestTableProtocolGuid = { 0x705bdcd9, 0x8c47, 0x457e, { 0xad, 0x0d, 0xf7, 0x86, 0xf3, 0x4a, 0x0d, 0x63 } }
diff --git a/ArmPlatformPkg/Drivers/Apei/HestDxe/HestDxe.inf b/ArmPlatformPkg/Drivers/Apei/HestDxe/HestDxe.inf
new file mode 100644
index 000000000000..91c7385bf7ff
--- /dev/null
+++ b/ArmPlatformPkg/Drivers/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
+  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
+
+[LibraryClasses]
+  ArmLib
+  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/ArmPlatformPkg/Include/Protocol/HestTable.h b/ArmPlatformPkg/Include/Protocol/HestTable.h
new file mode 100644
index 000000000000..3b2e1f7d9203
--- /dev/null
+++ b/ArmPlatformPkg/Include/Protocol/HestTable.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;
+
+extern EFI_GUID gHestTableProtocolGuid;
+#endif  // HEST_TABLE_H_
diff --git a/ArmPlatformPkg/Drivers/Apei/HestDxe/HestDxe.c b/ArmPlatformPkg/Drivers/Apei/HestDxe/HestDxe.c
new file mode 100644
index 000000000000..b68e1a0c4e48
--- /dev/null
+++ b/ArmPlatformPkg/Drivers/Apei/HestDxe/HestDxe.c
@@ -0,0 +1,354 @@
+/** @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>
+#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;
+
+/**
+  Helper function to the driver.
+
+  Function that reallocates memory for every new error source descriptor info
+  added.
+
+  @param[in]       OldTableSize  Old memory pool size.
+  @param[in]       NewTableSize  Required memory pool size.
+  @param[in, out]  OldBuffer     Current memory buffer pointer holding the Hest
+                                 table data.
+
+  @return  New pointer to reallocated memory pool.
+**/
+STATIC
+VOID*
+ReallocateHestTableMemory (
+  IN     UINT32 OldTableSize,
+  IN     UINT32 NewTableSize,
+  IN OUT VOID   *OldBuffer
+  )
+{
[SAMI] Have you considered maintaining a linked list of the error sources and serialising the list once InstallHestTable() is called? 

This is a simple implementation which collects all the error source descriptors as buffers. Linked list will add complexity to the code. Are there any particular benefits of using linked list over this method. For the v3, linked list has not been used. Let me know you opinion.

- Omkar

+  return ReallocateReservedPool (
+           OldTableSize,
+           NewTableSize,
+           OldBuffer
+           );
[SAMI] Is this wrapper function required? Can ReallocateReservedPool() be used directly instead? 

Ack.

- Omkar

+}
+
+/**
+  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)
[SAMI] VOID and closing parenthesis should be on a separate line. 

Ack.

- Omkar

+{
+  EFI_ACPI_6_3_HARDWARE_ERROR_SOURCE_TABLE_HEADER HestHeader;
+  UINT64                                          TempOemTableId;
+
+  //
+  // Allocate memory for the HEST table header.
+  //
+  mHestDriverData.HestTable =
+    ReallocateHestTableMemory (
+      0,
+      sizeof (EFI_ACPI_6_3_HARDWARE_ERROR_SOURCE_TABLE_HEADER),
+      NULL
+      );
[SAMI] Is the Relocate version of the function required here, maybe the Alloc version could be used.
- Should EfiReservedMemoryType be used? 
    Please see extract from section 2.3.6 AArch64 Platforms of the UEFI 2.9 specification below:
    "Note:Previous EFI specifications allowed ACPI tables loaded at runtime to be in the
     EfiReservedMemoryType and there was no guidance provided for other EFI Configuration
    Tables. EfiReservedMemoryType is not intended to be used by firmware. UEFI 2.0 clarified
    the situation moving forward. "

Ack.

- Omkar

+  if (mHestDriverData.HestTable == NULL) {
+    return EFI_OUT_OF_RESOURCES;
+  }
+
+  mHestDriverData.CurrentTableSize =
+    sizeof (EFI_ACPI_6_3_HARDWARE_ERROR_SOURCE_TABLE_HEADER);
+
+  //
+  // 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)
+    );
+  TempOemTableId = FixedPcdGet64 (PcdAcpiDefaultOemTableId);
+  CopyMem (
+    &HestHeader.Header.OemTableId,
+    &TempOemTableId,
+    sizeof (HestHeader.Header.OemTableId)
+    );
[SAMI] HestHeader.Header.OemTableId = FixedPcdGet64 (PcdAcpiDefaultOemTableId); ? 
+  HestHeader.Header.OemRevision = PcdGet32 (PcdAcpiDefaultOemRevision);
+  HestHeader.Header.CreatorId = PcdGet32 (PcdAcpiDefaultCreatorId);
+  HestHeader.Header.CreatorRevision = PcdGet32 (PcdAcpiDefaultCreatorRevision);
+  HestHeader.ErrorSourceCount = 0;
+  CopyMem (mHestDriverData.HestTable, &HestHeader, sizeof (HestHeader));
[SAMI] Is it possible to use a local HEST table pointer to initalise the values instead of initialising a HEST structure and then doing memcopy?

Ack.

- Omkar

+
+  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 = ReallocateHestTableMemory (
+                                mHestDriverData.CurrentTableSize,
+                                NewTableSize,
+                                mHestDriverData.HestTable
+                                );
+  if (mHestDriverData.HestTable == NULL) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a: Failed to reallocate memory for HEST table\n",
+      __FUNCTION__
+      ));
+    return EFI_OUT_OF_RESOURCES;
+  }
+
[SAMI] As mentioned earlier, would it be possible to maintain a link list instead of relocating the memory?

This implementation is simpler over the linked list implementation. Same as the comment above, for v3, this implementation will be maintained. Let me know your opinion.

- Omkar

+  //
+  // 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)
[SAMI] Please update according to coding standard.

Ack.

- Omkar

+{
+  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_SUCCESS;
[SAMI] Can a suitable error code be returned here? EFI_NOT_FOUND?

Ack.

- Omkar

+  }
+
+  //
+  // 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;
+  HestHeader->Header.Checksum = CalculateCheckSum8 (
+                                  (UINT8 *)HestHeader,
+                                  HestHeader->Header.Length
+                                  );
[SAMI] Checksum calculation is not needed as ACPITableProtocol->InstallAcpiTable() does this internally.

Ack.

- Omkar

+
+  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;
+  }
+
+  //
+  // Free the HEST table buffer.
+  //
+  FreePool (mHestDriverData.HestTable);
+  DEBUG ((
+    DEBUG_INFO,
+    "HestDxe: Installed HEST table \n"
+    ));
+  return EFI_SUCCESS;
[SAMI] return Status;

Ack.

- Omkar

+}
+
+//
+// HEST table generation protocol instance.
+//
+STATIC HEST_TABLE_PROTOCOL mHestProtocol = {
+  AppendErrorSourceDescriptor,
+  InstallHestAcpiTable
+};
[SAMI] HEST is platform and processor architecture independent. Threfore, can this implementation be paced in a common location? MdeModulePkg?

Ack.

- Omkar

+
+/**
+  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;
+  }
+
+  return EFI_SUCCESS;
[SAMI] return Status;

Ack.

- Omkar

+}

[PATCH v2 2/4] ArmPlatformPkg: add definition for MM_HEST_ERROR_SOURCE_DESC_PROTOCOL

[Omkar Anand Kulkarni]

Add the protocol definition of the MM_HEST_ERROR_SOURCE_DESC_PROTOCOL
protocol. This protocol can be implemented by MM drivers to publish
error source descriptors that have to be populated into HEST table.

Co-authored-by: Thomas Abraham <thomas.abraham@arm.com>
Signed-off-by: Omkar Anand Kulkarni <omkar.kulkarni@arm.com>
---
 ArmPlatformPkg/ArmPlatformPkg.dec                     |  1 +
 ArmPlatformPkg/Include/Protocol/HestErrorSourceInfo.h | 64 ++++++++++++++++++++
 2 files changed, 65 insertions(+)

diff --git a/ArmPlatformPkg/ArmPlatformPkg.dec b/ArmPlatformPkg/ArmPlatformPkg.dec
index e4afe5da8e11..4f062292663b 100644
--- a/ArmPlatformPkg/ArmPlatformPkg.dec
+++ b/ArmPlatformPkg/ArmPlatformPkg.dec
@@ -131,3 +131,4 @@
 [Protocols.common]
   ## Arm Platform HEST table generation protocol
   gHestTableProtocolGuid = { 0x705bdcd9, 0x8c47, 0x457e, { 0xad, 0x0d, 0xf7, 0x86, 0xf3, 0x4a, 0x0d, 0x63 } }
+  gMmHestErrorSourceDescProtocolGuid = { 0x560bf236, 0xa4a8, 0x4d69, { 0xbc, 0xf6, 0xc2, 0x97, 0x24, 0x10, 0x9d, 0x91 } }
diff --git a/ArmPlatformPkg/Include/Protocol/HestErrorSourceInfo.h b/ArmPlatformPkg/Include/Protocol/HestErrorSourceInfo.h
new file mode 100644
index 000000000000..95afd4dffe9c
--- /dev/null
+++ b/ArmPlatformPkg/Include/Protocol/HestErrorSourceInfo.h
@@ -0,0 +1,64 @@
+/** @file
+  MM protocol to get the secure error source descriptor information.
+
+  MM Drivers must implement this protocol in order to publish secure side
+  error source descriptor information to OSPM through the HEST ACPI table.
+
+  Copyright (c) 2020 - 2021, ARM Limited. All rights reserved.
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#ifndef MM_HEST_ERROR_SOURCE_DESC_
+#define MM_HEST_ERROR_SOURCE_DESC_
+
+#define MM_HEST_ERROR_SOURCE_DESC_PROTOCOL_GUID \
+  { \
+    0x560bf236, 0xa4a8, 0x4d69, { 0xbc, 0xf6, 0xc2, 0x97, 0x24, 0x10, 0x9d, 0x91 } \
+  }
+
+typedef struct MM_HEST_ERROR_SOURCE_DESC_PROTOCOL_
+                 MM_HEST_ERROR_SOURCE_DESC_PROTOCOL;
+
+/**
+  Get HEST Secure Error Source Descriptors.
+
+  The MM drivers implementing this protocol must convey the total count and
+  total length of the error sources the driver has along with the actual error
+  source descriptor(s).
+
+  Passing NULL as Buffer parameter shall return EFI_INVALID_PARAMETR with the
+  total length and count of the error source descriptor(s) it supports.
+
+  @param[in]   This                MM_HEST_ERROR_SOURCE_DESC_PROTOCOL instance.
+  @param[out]  Buffer              Buffer to be appended with the error
+                                   source descriptors information.
+  @param[out]  ErrorSourcesLength  Total length of all the error source
+                                   descriptors.
+  @param[out]  ErrorSourceCount    Count of total error source descriptors
+                                   supported by the driver.
+
+  retval  EFI_SUCCESS           If the Buffer is valid and is filled with valid
+                                Error Source descriptor data.
+  retval  EFI_INVALID_PARAMTER  Buffer is NULL.
+  retval  Other                 If no error source descriptor information is
+                                available.
+**/
+typedef
+EFI_STATUS
+(EFIAPI *MM_HEST_GET_ERROR_SOURCE_DESCRIPTORS) (
+  IN  MM_HEST_ERROR_SOURCE_DESC_PROTOCOL *This,
+  OUT VOID                               **Buffer,
+  OUT UINTN                              *ErrorSourcesLength,
+  OUT UINTN                              *ErrorSourcesCount
+  );
+
+//
+// Protocol declaration
+//
+struct MM_HEST_ERROR_SOURCE_DESC_PROTOCOL_ {
+  MM_HEST_GET_ERROR_SOURCE_DESCRIPTORS GetHestErrorSourceDescriptors;
+};
+
+extern EFI_GUID gMmHestErrorSourceDescProtocolGuid;
+
+#endif // MM_HEST_ERROR_SOURCE_DESC_
-- 
2.17.1

Re: [PATCH v2 2/4] ArmPlatformPkg: add definition for MM_HEST_ERROR_SOURCE_DESC_PROTOCOL

[Sami Mujawar]

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

Hi Omkar,

Please find my response below marked [SAMI]

Regards,

Sami Mujawar
On 10/07/2021 05:18 PM, Omkar Anand Kulkarni wrote:
> Add the protocol definition of the MM_HEST_ERROR_SOURCE_DESC_PROTOCOL
> protocol. This protocol can be implemented by MM drivers to publish
> error source descriptors that have to be populated into HEST table.
>
> Co-authored-by: Thomas Abraham<thomas.abraham@arm.com>
> Signed-off-by: Omkar Anand Kulkarni<omkar.kulkarni@arm.com>
> ---
>   ArmPlatformPkg/ArmPlatformPkg.dec                     |  1 +
>   ArmPlatformPkg/Include/Protocol/HestErrorSourceInfo.h | 64 ++++++++++++++++++++
>   2 files changed, 65 insertions(+)
>
> diff --git a/ArmPlatformPkg/ArmPlatformPkg.dec b/ArmPlatformPkg/ArmPlatformPkg.dec
> index e4afe5da8e11..4f062292663b 100644
> --- a/ArmPlatformPkg/ArmPlatformPkg.dec
> +++ b/ArmPlatformPkg/ArmPlatformPkg.dec
> @@ -131,3 +131,4 @@
>   [Protocols.common]
>     ## Arm Platform HEST table generation protocol
>     gHestTableProtocolGuid = { 0x705bdcd9, 0x8c47, 0x457e, { 0xad, 0x0d, 0xf7, 0x86, 0xf3, 0x4a, 0x0d, 0x63 } }
> +  gMmHestErrorSourceDescProtocolGuid = { 0x560bf236, 0xa4a8, 0x4d69, { 0xbc, 0xf6, 0xc2, 0x97, 0x24, 0x10, 0x9d, 0x91 } }
> diff --git a/ArmPlatformPkg/Include/Protocol/HestErrorSourceInfo.h b/ArmPlatformPkg/Include/Protocol/HestErrorSourceInfo.h
> new file mode 100644
> index 000000000000..95afd4dffe9c
> --- /dev/null
> +++ b/ArmPlatformPkg/Include/Protocol/HestErrorSourceInfo.h
> @@ -0,0 +1,64 @@
> +/** @file
> +  MM protocol to get the secure error source descriptor information.
> +
> +  MM Drivers must implement this protocol in order to publish secure side
> +  error source descriptor information to OSPM through the HEST ACPI table.
> +
> +  Copyright (c) 2020 - 2021, ARM Limited. All rights reserved.
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +
> +#ifndef MM_HEST_ERROR_SOURCE_DESC_
> +#define MM_HEST_ERROR_SOURCE_DESC_
> +
> +#define MM_HEST_ERROR_SOURCE_DESC_PROTOCOL_GUID \
> +  { \
> +    0x560bf236, 0xa4a8, 0x4d69, { 0xbc, 0xf6, 0xc2, 0x97, 0x24, 0x10, 0x9d, 0x91 } \
> +  }
> +
> +typedef struct MM_HEST_ERROR_SOURCE_DESC_PROTOCOL_
[SAMI] Not sure if a trailing underscore would be right to use for the 
name tag. Can MmHestErrorSourceDescProtocol be used as the name tag?
Also see 
https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/56_declarations_and_types#5-6-3-2-structure-declaration-with-forward-reference-or-self-reference 

> +                 MM_HEST_ERROR_SOURCE_DESC_PROTOCOL;
> +
> +/**
> +  Get HEST Secure Error Source Descriptors.
> +
> +  The MM drivers implementing this protocol must convey the total count and
> +  total length of the error sources the driver has along with the actual error
> +  source descriptor(s).
> +
> +  Passing NULL as Buffer parameter shall return EFI_INVALID_PARAMETR with the
> +  total length and count of the error source descriptor(s) it supports.
> +
> +  @param[in]   This                MM_HEST_ERROR_SOURCE_DESC_PROTOCOL instance.
> +  @param[out]  Buffer              Buffer to be appended with the error
> +                                   source descriptors information.
> +  @param[out]  ErrorSourcesLength  Total length of all the error source
> +                                   descriptors.
> +  @param[out]  ErrorSourceCount    Count of total error source descriptors
> +                                   supported by the driver.
> +
> +  retval  EFI_SUCCESS           If the Buffer is valid and is filled with valid
> +                                Error Source descriptor data.
> +  retval  EFI_INVALID_PARAMTER  Buffer is NULL.
> +  retval  Other                 If no error source descriptor information is
> +                                available.
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *MM_HEST_GET_ERROR_SOURCE_DESCRIPTORS) (
> +  IN  MM_HEST_ERROR_SOURCE_DESC_PROTOCOL *This,
> +  OUT VOID                               **Buffer,
> +  OUT UINTN                              *ErrorSourcesLength,
> +  OUT UINTN                              *ErrorSourcesCount
> +  );
> +
> +//
> +// Protocol declaration
> +//
> +struct MM_HEST_ERROR_SOURCE_DESC_PROTOCOL_ {
> +  MM_HEST_GET_ERROR_SOURCE_DESCRIPTORS GetHestErrorSourceDescriptors;
> +};
> +
> +extern EFI_GUID gMmHestErrorSourceDescProtocolGuid;
> +
> +#endif // MM_HEST_ERROR_SOURCE_DESC_



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

Re: [PATCH v2 2/4] ArmPlatformPkg: add definition for MM_HEST_ERROR_SOURCE_DESC_PROTOCOL

[Omkar Anand Kulkarni]

Hi Sami, 

Thanks for reviewing this patch. Please find my response inline.

Regards,
Omkar

Hi Omkar,
Please find my response below marked [SAMI]
Regards,

Sami Mujawar
On 10/07/2021 05:18 PM, Omkar Anand Kulkarni wrote:
Add the protocol definition of the MM_HEST_ERROR_SOURCE_DESC_PROTOCOL
protocol. This protocol can be implemented by MM drivers to publish
error source descriptors that have to be populated into HEST table.

Co-authored-by: Thomas Abraham mailto:thomas.abraham@arm.com
Signed-off-by: Omkar Anand Kulkarni mailto:omkar.kulkarni@arm.com
---
 ArmPlatformPkg/ArmPlatformPkg.dec                     |  1 +
 ArmPlatformPkg/Include/Protocol/HestErrorSourceInfo.h | 64 ++++++++++++++++++++
 2 files changed, 65 insertions(+)

diff --git a/ArmPlatformPkg/ArmPlatformPkg.dec b/ArmPlatformPkg/ArmPlatformPkg.dec
index e4afe5da8e11..4f062292663b 100644
--- a/ArmPlatformPkg/ArmPlatformPkg.dec
+++ b/ArmPlatformPkg/ArmPlatformPkg.dec
@@ -131,3 +131,4 @@
 [Protocols.common]
   ## Arm Platform HEST table generation protocol
   gHestTableProtocolGuid = { 0x705bdcd9, 0x8c47, 0x457e, { 0xad, 0x0d, 0xf7, 0x86, 0xf3, 0x4a, 0x0d, 0x63 } }
+  gMmHestErrorSourceDescProtocolGuid = { 0x560bf236, 0xa4a8, 0x4d69, { 0xbc, 0xf6, 0xc2, 0x97, 0x24, 0x10, 0x9d, 0x91 } }
diff --git a/ArmPlatformPkg/Include/Protocol/HestErrorSourceInfo.h b/ArmPlatformPkg/Include/Protocol/HestErrorSourceInfo.h
new file mode 100644
index 000000000000..95afd4dffe9c
--- /dev/null
+++ b/ArmPlatformPkg/Include/Protocol/HestErrorSourceInfo.h
@@ -0,0 +1,64 @@
+/** @file
+  MM protocol to get the secure error source descriptor information.
+
+  MM Drivers must implement this protocol in order to publish secure side
+  error source descriptor information to OSPM through the HEST ACPI table.
+
+  Copyright (c) 2020 - 2021, ARM Limited. All rights reserved.
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#ifndef MM_HEST_ERROR_SOURCE_DESC_
+#define MM_HEST_ERROR_SOURCE_DESC_
+
+#define MM_HEST_ERROR_SOURCE_DESC_PROTOCOL_GUID \
+  { \
+    0x560bf236, 0xa4a8, 0x4d69, { 0xbc, 0xf6, 0xc2, 0x97, 0x24, 0x10, 0x9d, 0x91 } \
+  }
+
+typedef struct MM_HEST_ERROR_SOURCE_DESC_PROTOCOL_
[SAMI] Not sure if a trailing underscore would be right to use for the name tag. Can MmHestErrorSourceDescProtocol be used as the name tag? 
Also see https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/56_declarations_and_types#5-6-3-2-structure-declaration-with-forward-reference-or-self-reference 
+                 MM_HEST_ERROR_SOURCE_DESC_PROTOCOL;
+

Ack.

- Omkar

+/**
+  Get HEST Secure Error Source Descriptors.
+
+  The MM drivers implementing this protocol must convey the total count and
+  total length of the error sources the driver has along with the actual error
+  source descriptor(s).
+
+  Passing NULL as Buffer parameter shall return EFI_INVALID_PARAMETR with the
+  total length and count of the error source descriptor(s) it supports.
+
+  @param[in]   This                MM_HEST_ERROR_SOURCE_DESC_PROTOCOL instance.
+  @param[out]  Buffer              Buffer to be appended with the error
+                                   source descriptors information.
+  @param[out]  ErrorSourcesLength  Total length of all the error source
+                                   descriptors.
+  @param[out]  ErrorSourceCount    Count of total error source descriptors
+                                   supported by the driver.
+
+  retval  EFI_SUCCESS           If the Buffer is valid and is filled with valid
+                                Error Source descriptor data.
+  retval  EFI_INVALID_PARAMTER  Buffer is NULL.
+  retval  Other                 If no error source descriptor information is
+                                available.
+**/
+typedef
+EFI_STATUS
+(EFIAPI *MM_HEST_GET_ERROR_SOURCE_DESCRIPTORS) (
+  IN  MM_HEST_ERROR_SOURCE_DESC_PROTOCOL *This,
+  OUT VOID                               **Buffer,
+  OUT UINTN                              *ErrorSourcesLength,
+  OUT UINTN                              *ErrorSourcesCount
+  );
+
+//
+// Protocol declaration
+//
+struct MM_HEST_ERROR_SOURCE_DESC_PROTOCOL_ {
+  MM_HEST_GET_ERROR_SOURCE_DESCRIPTORS GetHestErrorSourceDescriptors;
+};
+
+extern EFI_GUID gMmHestErrorSourceDescProtocolGuid;
+
+#endif // MM_HEST_ERROR_SOURCE_DESC_

[PATCH v2 3/4] ArmPlatformPkg: retreive error source descriptors from MM

[Omkar Anand Kulkarni]

Add a driver that retreives error source descriptors from MM and
populates those into the HEST ACPI table. The error source descriptors
that are available from the MM side are retreived using MM Communicate 2
protocol.

The first call into the MM returns the size of MM Communicate buffer
required to hold all error source descriptor info. The communication
buffer of that size is then allocated and the second call into MM
returns the error source descriptors in the communication buffer.
The retreived error source descriptors are then appended to the HEST
table.

Co-authored-by: Thomas Abraham <thomas.abraham@arm.com>
Signed-off-by: Omkar Anand Kulkarni <omkar.kulkarni@arm.com>
---
 ArmPlatformPkg/ArmPlatformPkg.dec                                         |   7 +
 ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceDxe.inf          |  44 +++
 ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceStandaloneMm.inf |  51 ++++
 ArmPlatformPkg/Drivers/HestMmErrorSources/HestMmErrorSourceCommon.h       |  37 +++
 ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceDxe.c            | 308 +++++++++++++++++++
 ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceStandaloneMm.c   | 312 ++++++++++++++++++++
 6 files changed, 759 insertions(+)

diff --git a/ArmPlatformPkg/ArmPlatformPkg.dec b/ArmPlatformPkg/ArmPlatformPkg.dec
index 4f062292663b..846b3e863aa9 100644
--- a/ArmPlatformPkg/ArmPlatformPkg.dec
+++ b/ArmPlatformPkg/ArmPlatformPkg.dec
@@ -52,6 +52,8 @@
 
 [Guids.common]
   gArmPlatformTokenSpaceGuid   = { 0x9c0aaed4, 0x74c5, 0x4043, { 0xb4, 0x17, 0xa3, 0x22, 0x38, 0x14, 0xce, 0x76 } }
+  gArmPlatformHestErrorSourcesGuid = { 0x76b8ab43, 0x822d, 0x4b00, { 0x9f, 0xd0, 0xf4, 0xa5, 0x35, 0x82, 0x47, 0x0a } }
+  gMmHestGetErrorSourceInfoGuid = { 0x7d602951, 0x678e, 0x4cc4, { 0x98, 0xd9, 0xe3, 0x76, 0x04, 0xf6, 0x93, 0x0d } }
 
 [PcdsFeatureFlag.common]
   gArmPlatformTokenSpaceGuid.PcdSendSgiToBringUpSecondaryCores|FALSE|BOOLEAN|0x00000004
@@ -128,6 +130,11 @@
 
   gArmPlatformTokenSpaceGuid.PcdWatchdogCount|0x0|UINT32|0x00000033
 
+[PcdsFixedAtBuild, PcdsPatchableInModule]
+  ## ACPI CPER memory space
+  gArmPlatformTokenSpaceGuid.PcdGhesGenericErrorDataMmBufferBase|0x00000000|UINT64|0x00000046
+  gArmPlatformTokenSpaceGuid.PcdGhesGenericErrorDataMmBufferSize|0x00000000|UINT64|0x00000047
+
 [Protocols.common]
   ## Arm Platform HEST table generation protocol
   gHestTableProtocolGuid = { 0x705bdcd9, 0x8c47, 0x457e, { 0xad, 0x0d, 0xf7, 0x86, 0xf3, 0x4a, 0x0d, 0x63 } }
diff --git a/ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceDxe.inf b/ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceDxe.inf
new file mode 100644
index 000000000000..5227dea91630
--- /dev/null
+++ b/ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceDxe.inf
@@ -0,0 +1,44 @@
+## @file
+#  DXE driver to get secure error sources.
+#
+#  DXE driver to retrieve the error source descriptors from Standalone MM and
+#  append those to the HEST table.
+#
+#  Copyright (c) 2020 - 2021, ARM Limited. All rights reserved.
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+  INF_VERSION                    = 0x0001001A
+  BASE_NAME                      = HestMmErrorSourceDxe
+  FILE_GUID                      = 76b8ab43-822d-4b00-9fd0-f4a53582470a
+  MODULE_TYPE                    = DXE_DRIVER
+  VERSION_STRING                 = 1.0
+  ENTRY_POINT                    = HestErrorSourceInitialize
+
+[Sources.common]
+  HestErrorSourceDxe.c
+
+[Packages]
+  ArmPkg/ArmPkg.dec
+  ArmPlatformPkg/ArmPlatformPkg.dec
+  MdePkg/MdePkg.dec
+  StandaloneMmPkg/StandaloneMmPkg.dec
+
+[LibraryClasses]
+  BaseMemoryLib
+  DebugLib
+  DxeServicesTableLib
+  UefiDriverEntryPoint
+  UefiLib
+
+[Guids]
+  gMmHestGetErrorSourceInfoGuid                  ## PRODUCES
+
+[Protocols]
+  gHestTableProtocolGuid                         ## CONSUMES
+  gEfiMmCommunication2ProtocolGuid
+
+[Depex]
+  gHestTableProtocolGuid AND gEfiMmCommunication2ProtocolGuid
diff --git a/ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceStandaloneMm.inf b/ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceStandaloneMm.inf
new file mode 100644
index 000000000000..9d566de9bec3
--- /dev/null
+++ b/ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceStandaloneMm.inf
@@ -0,0 +1,51 @@
+## @file
+#  HEST error source gateway Standalone MM driver.
+#
+#  Collects HEST error source descriptors,by communicating with all the MM
+#  drivers implementing the HEST error source descriptor protocol.
+#
+#  Copyright (c) 2020 - 2021, ARM Limited. All rights reserved.
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+  INF_VERSION                    = 0x0001001A
+  BASE_NAME                      = HestErrorSourceStandaloneMm
+  FILE_GUID                      = 3ddbebcc-9841-4ef8-87fa-305843c1922d
+  MODULE_TYPE                    = MM_STANDALONE
+  VERSION_STRING                 = 1.0
+  PI_SPECIFICATION_VERSION       = 0x00010032
+  ENTRY_POINT                    = StandaloneMmHestErrorSourceInitialize
+
+[Sources]
+  HestErrorSourceStandaloneMm.c
+
+[Packages]
+  ArmPkg/ArmPkg.dec
+  ArmPlatformPkg/ArmPlatformPkg.dec
+  MdeModulePkg/MdeModulePkg.dec
+  MdePkg/MdePkg.dec
+  StandaloneMmPkg/StandaloneMmPkg.dec
+
+[LibraryClasses]
+  ArmLib
+  ArmSvcLib
+  BaseMemoryLib
+  DebugLib
+  MemoryAllocationLib
+  StandaloneMmDriverEntryPoint
+
+[Protocols]
+  gMmHestErrorSourceDescProtocolGuid
+
+[Guids]
+  gMmHestGetErrorSourceInfoGuid               ##PRODUCES
+  gEfiStandaloneMmNonSecureBufferGuid
+
+[FixedPcd]
+  gArmPlatformTokenSpaceGuid.PcdGhesGenericErrorDataMmBufferBase
+  gArmPlatformTokenSpaceGuid.PcdGhesGenericErrorDataMmBufferSize
+
+[Depex]
+  TRUE
diff --git a/ArmPlatformPkg/Drivers/HestMmErrorSources/HestMmErrorSourceCommon.h b/ArmPlatformPkg/Drivers/HestMmErrorSources/HestMmErrorSourceCommon.h
new file mode 100644
index 000000000000..6ddc6bd21922
--- /dev/null
+++ b/ArmPlatformPkg/Drivers/HestMmErrorSources/HestMmErrorSourceCommon.h
@@ -0,0 +1,37 @@
+/** @file
+  Data structures for error source descriptor information.
+
+  This data structure forms the CommBuffer part of the MM Communication
+  protocol used for communicating the Hardware Error sources form MM to
+  Non-MM environment.
+
+  Copyright (c) 2020 - 2021, ARM Limited. All rights reserved.
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#ifndef HEST_ERROR_SOURCE_DESCRIPTOR_H_
+#define HEST_ERROR_SOURCE_DESCRIPTOR_H_
+
+#define HEST_ERROR_SOURCE_DESC_INFO_SIZE \
+  (OFFSET_OF (HEST_ERROR_SOURCE_DESC_INFO, ErrSourceDescList))
+
+//
+// Data Structure to communicate the error source descriptor information from
+// Standalone MM.
+//
+typedef struct {
+  //
+  // Total count of error source descriptors.
+  //
+  UINTN ErrSourceDescCount;
+  //
+  // Total size of all the error source descriptors.
+  //
+  UINTN ErrSourceDescSize;
+  //
+  // Array of error source descriptors that is ErrSourceDescSize in size.
+  //
+  UINT8 ErrSourceDescList[1];
+} HEST_ERROR_SOURCE_DESC_INFO;
+
+#endif // HEST_ERROR_SOURCE_DESCRIPTOR_H_
diff --git a/ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceDxe.c b/ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceDxe.c
new file mode 100644
index 000000000000..acfb0fc9e838
--- /dev/null
+++ b/ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceDxe.c
@@ -0,0 +1,308 @@
+/** @file
+  Collects and appends the HEST error source descriptors from the MM drivers.
+
+  The drivers entry point locates the MM Communication protocol and calls into
+  Standalone MM to get the HEST error sources length and count. It also
+  retrieves descriptor information. The information is then used to build the
+  HEST table using the HEST table generation protocol.
+
+  This driver collects the secure error source descriptor information from the
+  MM drviers that implement HEST error source protocol. Instead of directly
+  communicating with the individual MM drivers, it calls into
+  HestErrorSourceStandaloneMM driver which is a gatway MM driver. This MM driver
+  in-turn communicates with individual MM drivers collecting the error source
+  descriptor information.
+
+  Once all the error source descriptor information is retrieved the driver
+  appends the descriptors to HEST table using the HestDxe driver.
+
+  Copyright (c) 2020 - 2021, ARM Limited. All rights reserved.
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#include <IndustryStandard/Acpi.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/DebugLib.h>
+#include <Library/DxeServicesTableLib.h>
+#include <Library/MemoryAllocationLib.h>
+#include <Library/UefiBootServicesTableLib.h>
+#include <Protocol/MmCommunication2.h>
+#include <Protocol/HestTable.h>
+#include "HestMmErrorSourceCommon.h"
+
+#define MM_COMMUNICATE_HEADER_SIZE (OFFSET_OF (EFI_MM_COMMUNICATE_HEADER, Data))
+
+STATIC HEST_TABLE_PROTOCOL *mHestProtocol;
+STATIC EFI_MM_COMMUNICATION2_PROTOCOL *mMmCommunication2;
+
+/**
+  Retrieve the error source descriptors from Standalone MM.
+
+  Initialize the MM comminication buffer by assigning the MM service to
+  invoke as gMmHestGetErrorSourceInfoGuid. Use the MM communication
+  protocol to retrieve the error source descriptors.
+
+  @param[in]       CommBuffSize  Size of communicate buffer.
+  @param[in, out]  CommBuffer    The communicate buffer.
+
+  @retval  EFI_SUCCESS  MM Communicate protocol call successful.
+  @retval  Other        MM Communicate protocol call failed.
+**/
+STATIC
+EFI_STATUS
+GetErrorSourceDescriptors (
+  IN     UINTN                     CommBuffSize,
+  IN OUT EFI_MM_COMMUNICATE_HEADER **CommBuffer
+  )
+{
+  EFI_STATUS Status;
+
+  //
+  // Initialize the CommBuffer with MM Communicate metadata.
+  //
+  CopyGuid (&(*CommBuffer)->HeaderGuid, &gMmHestGetErrorSourceInfoGuid);
+  (*CommBuffer)->MessageLength =
+    CommBuffSize -
+    sizeof ((*CommBuffer)->HeaderGuid) -
+    sizeof ((*CommBuffer)->MessageLength);
+
+  //
+  // Call into the Standalone MM using the MM Communicate protocol.
+  //
+  Status = mMmCommunication2->Communicate (
+                                mMmCommunication2,
+                                (VOID *)*CommBuffer,
+                                (VOID *)*CommBuffer,
+                                NULL
+                                );
+
+  return Status;
+}
+
+/**
+  Collect HEST error source descriptors from all Standalone MM drivers and
+  append them to the HEST table.
+
+  Use MM Communication Protocol to communicate and collect the error source
+  descriptor information from Standalone MM. Check for the required buffer size
+  returned by the MM driver. Allocate buffer of adequate size and call again
+  into MM.
+
+  @retval  EFI_SUCCESS           Successful to collect and append the error
+                                 source.
+                                 descriptors to HEST table.
+  @retval  EFI_OUT_OF_RESOURCES  Memory allocation failure.
+  @retval  Other                 For any other error.
+**/
+STATIC
+EFI_STATUS
+AppendMmErrorSources (VOID)
+{
+  EFI_MM_COMMUNICATE_HEADER   *CommunicationHeader = NULL;
+  HEST_ERROR_SOURCE_DESC_INFO *ErrorSourceDescInfo;
+  EFI_STATUS                  Status;
+  UINTN                       CommBufferSize;
+
+  //
+  // Retrieve the count, length and the actual eror source descriptors from
+  // the MM drivers. Do this by performing two MM Communicate calls, in the
+  // first call pass CommBuffer which is atleast of the size of error source
+  // descriptor info structure. Followed by another communicate call with
+  // CommBuffer allocated to required buffer size to hold all descriptors.
+  //
+  // Allocate CommBuffer atleast the size of error source descriptor info
+  // structure.
+  CommBufferSize =
+    MM_COMMUNICATE_HEADER_SIZE + HEST_ERROR_SOURCE_DESC_INFO_SIZE;
+  CommunicationHeader = AllocatePool (CommBufferSize);
+  if (CommunicationHeader == NULL) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a: Failed to allocate memory for CommunicationHeader\n",
+      __FUNCTION__
+      ));
+    return EFI_OUT_OF_RESOURCES;
+  }
+
+  //
+  // Make the first MM Communicate call to HestErrorSourceStandaloneMM gateway
+  // driver, which returns the required buffer size adequate to hold all the
+  // desctriptor information.
+  //
+  Status = GetErrorSourceDescriptors (CommBufferSize, &CommunicationHeader);
+  if ((EFI_ERROR (Status)) &&
+      (Status != EFI_BAD_BUFFER_SIZE)) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a: MM Communicate protocol call failed, status: %r\n",
+      __FUNCTION__,
+      Status
+      ));
+    FreePool (CommunicationHeader);
+    return Status;
+  }
+
+  // Check for the length of Error Source descriptors.
+  ErrorSourceDescInfo =
+    (HEST_ERROR_SOURCE_DESC_INFO *)(CommunicationHeader->Data);
+  if ((ErrorSourceDescInfo->ErrSourceDescSize == 0) ||
+      (ErrorSourceDescInfo->ErrSourceDescCount == 0)) {
+    DEBUG ((
+      DEBUG_INFO,
+      "HesErrorSourceDxe: HEST error source(s) not found\n"
+      ));
+    FreePool (CommunicationHeader);
+    return EFI_SUCCESS;
+  }
+
+  //
+  // Allocate CommBuffer of required size to accomodate all the error source
+  // descriptors. Required size of communication buffer =
+  // MM communicate metadata. + (error source desc info struct + error source
+  // descriptor size).
+  //
+  CommBufferSize =
+    MM_COMMUNICATE_HEADER_SIZE +
+    HEST_ERROR_SOURCE_DESC_INFO_SIZE +
+    ErrorSourceDescInfo->ErrSourceDescSize;
+
+  // Free old MM Communicate buffer and allocate a new buffer of required size.
+  FreePool (CommunicationHeader);
+  CommunicationHeader = AllocatePool (CommBufferSize);
+  if (CommunicationHeader == NULL) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a: Failed to allocate memory for CommunicationHeader\n",
+      __FUNCTION__
+      ));
+    return EFI_OUT_OF_RESOURCES;
+  }
+
+  //
+  // Make second MM Communicate call to HestErrorSourceStandaloneMM driver to
+  // get the error source descriptors from the MM drivers.
+  //
+  Status = GetErrorSourceDescriptors (CommBufferSize, &CommunicationHeader);
+  if (EFI_ERROR (Status)) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a: MM Communicate protocol failed, status: %r\n",
+      __FUNCTION__,
+      Status
+      ));
+    FreePool (CommunicationHeader);
+    return Status;
+  }
+
+  //
+  // Retrieve the HEST error source descriptor information. Ensure that there
+  // is a valid list of error source descriptors.
+  //
+  ErrorSourceDescInfo =
+    (HEST_ERROR_SOURCE_DESC_INFO *)(CommunicationHeader->Data);
+  if (ErrorSourceDescInfo->ErrSourceDescList == NULL) {
+    DEBUG ((
+      DEBUG_INFO,
+      "HestErrorSourceDxe: Error source descriptor list is empty"
+      ));
+    FreePool (CommunicationHeader);
+    return EFI_SUCCESS;
+  }
+
+  DEBUG ((
+    DEBUG_INFO,
+    "HestErrorSourceDxe: ErrorSources: TotalCount = %d TotalLength = %d \n",
+    ErrorSourceDescInfo->ErrSourceDescCount,
+    ErrorSourceDescInfo->ErrSourceDescSize
+    ));
+
+  //
+  // Append the error source descriptors to HEST table using the HEST table
+  // generation protocol.
+  //
+  Status = mHestProtocol->AppendErrorSourceDescriptors (
+                            ErrorSourceDescInfo->ErrSourceDescList,
+                            ErrorSourceDescInfo->ErrSourceDescSize,
+                            ErrorSourceDescInfo->ErrSourceDescCount
+                            );
+  if (EFI_ERROR (Status)) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a: Failed to append error source(s), status: %r\n",
+      __FUNCTION__,
+      Status
+      ));
+  }
+
+  FreePool (CommunicationHeader);
+  return Status;
+}
+
+/**
+  The Entry Point for HEST Error Source Dxe driver.
+
+  Locates the HEST Table generation and MM Communication2 protocols. Using the
+  MM Communication2, the driver collects the Error Source Descriptor(s) from
+  Standalone MM. It then appends those Error Source Descriptor(s) to the Hest
+  table using the HEST Table generation protocol.
+
+  @param[in]  ImageHandle  The firmware allocated handle for the Efi image.
+  @param[in]  SystemTable  A pointer to the Efi System Table.
+
+  @retval  EFI_SUCCESS  The entry point is executed successfully.
+  @retval  Other        Some error occurred when executing this entry point.
+**/
+EFI_STATUS
+EFIAPI
+HestErrorSourceInitialize (
+  IN EFI_HANDLE       ImageHandle,
+  IN EFI_SYSTEM_TABLE *SystemTable
+  )
+{
+  EFI_STATUS Status;
+
+  Status = gBS->LocateProtocol (
+                  &gHestTableProtocolGuid,
+                  NULL,
+                  (VOID **)&mHestProtocol
+                  );
+  if (EFI_ERROR (Status)) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a: Failed to locate HEST table generation protocol, status:%r\n",
+      __FUNCTION__,
+      Status
+      ));
+    return Status;
+  }
+
+  Status = gBS->LocateProtocol (
+                  &gEfiMmCommunication2ProtocolGuid,
+                  NULL,
+                  (VOID **)&mMmCommunication2
+                  );
+  if (EFI_ERROR (Status)) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a: Failed to locate MMCommunication2 driver protocol, status:%r\n",
+      __FUNCTION__,
+      Status
+      ));
+    return Status;
+  }
+
+  //
+  // Append HEST error sources retrieved from StandaloneMM, if any, into the
+  // HEST ACPI table.
+  //
+  Status = AppendMmErrorSources ();
+  if (EFI_ERROR (Status)) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a: Failed appending error source desc to HEST table, status:%r\n",
+      __FUNCTION__,
+      Status
+      ));
+  }
+  return EFI_SUCCESS;
+}
diff --git a/ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceStandaloneMm.c b/ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceStandaloneMm.c
new file mode 100644
index 000000000000..c7b2304fc494
--- /dev/null
+++ b/ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceStandaloneMm.c
@@ -0,0 +1,312 @@
+/** @file
+  MM HEST error source gateway driver.
+
+  This MM driver installs a handler which can be used to retrieve the error
+  source descriptors from the all MM drivers implementing the HEST error source
+  descriptor protocol.
+
+  The MM driver acts as a single point of contact to collect secure hardware
+  error sources from the MM drivers. It loops over all the MM drivers that
+  implement HEST error source descriptor protocol and collects error source
+  descriptor information along with the error source count and length.
+
+  Copyright (c) 2020 - 2021, ARM Limited. All rights reserved.
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#include <Base.h>
+#include <Library/BaseLib.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/DebugLib.h>
+#include <Library/MemoryAllocationLib.h>
+#include <Protocol/HestErrorSourceInfo.h>
+
+#include "HestMmErrorSourceCommon.h"
+
+STATIC EFI_MM_SYSTEM_TABLE *mMmst = NULL;
+
+/**
+  Returns an array of handles that implement the HEST error source descriptor
+  protocol.
+
+  Passing HandleBuffer as NULL will return the actual size of the buffer
+  required to hold the array of handles implementing the protocol.
+
+  @param[in, out]  HandleBufferSize  The size of the HandleBuffer.
+  @param[out]      HandleBuffer      A pointer to the buffer containing the list
+                                    of handles.
+
+  @retval  EFI_SUCCESS    The array of handles returned in HandleBuffer.
+  @retval  EFI_NOT_FOUND  No implementation present for the protocol.
+  @retval  Other          For any other error.
+**/
+STATIC
+EFI_STATUS
+GetHestErrorSourceProtocolHandles (
+  IN OUT UINTN      *HandleBufferSize,
+  OUT    EFI_HANDLE **HandleBuffer
+  )
+{
+  EFI_STATUS Status;
+
+  Status = mMmst->MmLocateHandle (
+                    ByProtocol,
+                    &gMmHestErrorSourceDescProtocolGuid,
+                    NULL,
+                    HandleBufferSize,
+                    *HandleBuffer
+                    );
+  if ((EFI_ERROR (Status)) &&
+      (Status != EFI_BUFFER_TOO_SMALL))
+  {
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a: No implementation of MmHestErrorSourceDescProtocol found, \
+       Status:%r\n",
+      __FUNCTION__,
+      Status
+      ));
+    return EFI_NOT_FOUND;
+  }
+
+  return Status;
+}
+
+/**
+  Mmi handler to retrieve HEST error source descriptor information.
+
+  Handler for Mmi service that returns the supported HEST error source
+  descriptors in MM. This handler populates the CommBuffer with the
+  list of all error source descriptors, prepended with the length and
+  the number of descriptors populated into CommBuffer.
+
+  @param[in]       DispatchHandle  The unique handle assigned to this handler by
+                                   MmiHandlerRegister().
+  @param[in]       Context         Points to an optional handler context that
+                                   is specified when the handler was registered.
+  @param[in, out]  CommBuffer      Buffer used for communication of HEST error
+                                   source descriptors.
+  @param[in, out]  CommBufferSize  The size of the CommBuffer.
+
+  @retval  EFI_SUCCESS            CommBuffer has valid data.
+  @retval  EFI_BAD_BUFFER_SIZE    CommBufferSize not adequate.
+  @retval  EFI_OUT_OF_RESOURCES   System out of memory resources.
+  @retval  EFI_INVALID_PARAMETER  Invalid CommBufferSize recieved.
+  @retval  Other                  For any other error.
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+HestErrorSourcesInfoMmiHandler (
+  IN     EFI_HANDLE DispatchHandle,
+  IN     CONST VOID *Context,       OPTIONAL
+  IN OUT VOID       *CommBuffer,    OPTIONAL
+  IN OUT UINTN      *CommBufferSize OPTIONAL
+  )
+{
+  MM_HEST_ERROR_SOURCE_DESC_PROTOCOL *HestErrSourceDescProtocolHandle;
+  HEST_ERROR_SOURCE_DESC_INFO        *ErrorSourceInfoList;
+  EFI_HANDLE                         *HandleBuffer;
+  EFI_STATUS                         Status;
+  UINTN                              HandleCount;
+  UINTN                              HandleBufferSize;
+  UINTN                              Index;
+  UINTN                              SourceCount = 0;
+  UINTN                              SourceLength = 0;
+  VOID                               *ErrorSourcePtr;
+  UINTN                              TotalSourceLength = 0;
+  UINTN                              TotalSourceCount = 0;
+
+  if (*CommBufferSize < HEST_ERROR_SOURCE_DESC_INFO_SIZE) {
+    //
+    // Ensures that the communication buffer has enough space to atleast hold
+    // the ErrSourceDescCount and ErrSourceDescSize elements of the
+    // HEST_ERROR_SOURCE_DESC_INFO structure.
+    //
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a: Invalid CommBufferSize parameter\n",
+      __FUNCTION__
+      ));
+    return EFI_INVALID_PARAMETER;
+  }
+
+  //
+  // Get all handles that implement the HEST error source descriptor protocol.
+  // Get the buffer size required to store list of handles for the protocol.
+  //
+  HandleBuffer = NULL;
+  HandleBufferSize = 0;
+  Status = GetHestErrorSourceProtocolHandles (&HandleBufferSize, &HandleBuffer);
+  if ((Status == EFI_NOT_FOUND) ||
+      (HandleBufferSize == 0))
+  {
+    return Status;
+  }
+
+  // Allocate memory for HandleBuffer of size HandleBufferSize.
+  HandleBuffer = AllocatePool (HandleBufferSize);
+  if (HandleBuffer == NULL) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a: Failed to allocate memory for HandleBuffer\n",
+      __FUNCTION__
+      ));
+    return EFI_OUT_OF_RESOURCES;
+  }
+
+  // Get the list of handles.
+  Status = GetHestErrorSourceProtocolHandles (&HandleBufferSize, &HandleBuffer);
+  if ((EFI_ERROR (Status)) ||
+      (HandleBuffer == NULL))
+  {
+    FreePool (HandleBuffer);
+    return Status;
+  }
+
+  // Count of handles for the protocol.
+  HandleCount = HandleBufferSize / sizeof (EFI_HANDLE);
+
+  //
+  // Loop to get the count and length of the error source descriptors.
+  //
+  // This loop collects and adds the length of error source descriptors and
+  // its count from all the the MM drivers implementing HEST error source.
+  // descriptor protocol. The total length and count values retrieved help
+  // to determine weather the CommBuffer is big enough to hold the descriptor
+  // information.
+  // As mentioned in the HEST error source descriptor protocol definition,
+  // Buffer parameter set to NULL ensures only length and the count values
+  // are returned from the driver and no error source information is copied to
+  // Buffer.
+  //
+  for (Index = 0; Index < HandleCount; ++Index) {
+    Status = mMmst->MmHandleProtocol (
+                      HandleBuffer[Index],
+                      &gMmHestErrorSourceDescProtocolGuid,
+                      (VOID **)&HestErrSourceDescProtocolHandle
+                      );
+    if (EFI_ERROR (Status)) {
+      continue;
+    }
+
+    //
+    // Protocol called with Buffer parameter passed as NULL, must return
+    // error source length and error count for that driver.
+    //
+    Status = HestErrSourceDescProtocolHandle->GetHestErrorSourceDescriptors (
+                                                HestErrSourceDescProtocolHandle,
+                                                NULL,
+                                                &SourceLength,
+                                                &SourceCount
+                                                );
+    if (Status == EFI_INVALID_PARAMETER) {
+      TotalSourceLength += SourceLength;
+      TotalSourceCount += SourceCount;
+    }
+  }
+
+  // Set the count and length in the error source descriptor.
+  ErrorSourceInfoList = (HEST_ERROR_SOURCE_DESC_INFO *)(CommBuffer);
+  ErrorSourceInfoList->ErrSourceDescCount = TotalSourceCount;
+  ErrorSourceInfoList->ErrSourceDescSize = TotalSourceLength;
+
+  //
+  // Check the size of CommBuffer, it should atleast be of size
+  // TotalSourceLength + HEST_ERROR_SOURCE_DESC_INFO_SIZE.
+  //
+  TotalSourceLength = TotalSourceLength + HEST_ERROR_SOURCE_DESC_INFO_SIZE;
+  if ((*CommBufferSize) < TotalSourceLength) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a: Invalid CommBufferSize parameter\n",
+      __FUNCTION__
+      ));
+    FreePool (HandleBuffer);
+    return EFI_BAD_BUFFER_SIZE;
+  }
+
+  //
+  // CommBuffer size is adequate to return all the error source descriptors.
+  // So go ahead and populate it with the error source descriptor information.
+  //
+
+  // Buffer pointer to append the Error Descriptors data.
+  ErrorSourcePtr =  ErrorSourceInfoList->ErrSourceDescList;
+
+  //
+  // Loop to retrieve error source descriptors information.
+  //
+  // Calls into each MM driver that implement the HEST error source descriptor
+  // protocol. Here the Buffer parameter passed to the protocol service is
+  // valid. So the MM driver when called copies the descriptor information.
+  //
+  for (Index = 0; Index < HandleCount; ++Index) {
+    Status = mMmst->MmHandleProtocol (
+                      HandleBuffer[Index],
+                      &gMmHestErrorSourceDescProtocolGuid,
+                      (VOID **)&HestErrSourceDescProtocolHandle
+                      );
+    if (EFI_ERROR (Status)) {
+      continue;
+    }
+
+    Status = HestErrSourceDescProtocolHandle->GetHestErrorSourceDescriptors (
+                                                HestErrSourceDescProtocolHandle,
+                                                (VOID **)&ErrorSourcePtr,
+                                                &SourceLength,
+                                                &SourceCount
+                                                );
+    if (Status == EFI_SUCCESS) {
+      ErrorSourcePtr += SourceLength;
+    }
+  }
+
+  // Free the buffer holding all the protocol handles.
+  FreePool (HandleBuffer);
+
+  return EFI_SUCCESS;
+}
+
+/**
+  Entry point for this Stanalone MM driver.
+
+  Registers an Mmi handler that retrieves the error source descriptors from all
+  the MM drivers implementing the MM_HEST_ERROR_SOURCE_DESC_PROTOCOL.
+
+  @param[in]  ImageHandle  The firmware allocated handle for the EFI image.
+  @param[in]  SystemTable  A pointer to the EFI System Table.
+
+  @retval  EFI_SUCCESS  The entry point registered handler successfully.
+  @retval  Other        Some error occurred when executing this entry point.
+**/
+EFI_STATUS
+EFIAPI
+StandaloneMmHestErrorSourceInitialize (
+  IN EFI_HANDLE          ImageHandle,
+  IN EFI_MM_SYSTEM_TABLE *SystemTable
+  )
+{
+  EFI_HANDLE DispatchHandle;
+  EFI_STATUS Status;
+
+  ASSERT (SystemTable != NULL);
+  mMmst = SystemTable;
+
+  Status = mMmst->MmiHandlerRegister (
+                    HestErrorSourcesInfoMmiHandler,
+                    &gMmHestGetErrorSourceInfoGuid,
+                    &DispatchHandle
+                    );
+  if (EFI_ERROR(Status)) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a: Mmi handler registration failed with status : %r\n",
+      __FUNCTION__,
+      Status
+      ));
+    return Status;
+  }
+
+  return EFI_SUCCESS;
+}
-- 
2.17.1

Re: [PATCH v2 3/4] ArmPlatformPkg: retreive error source descriptors from MM

[Sami Mujawar]

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

Hi Omkar,

Please find my response inline marked [SAMI].

Regards,

Sami Mujawar


On 10/07/2021 05:18 PM, Omkar Anand Kulkarni wrote:
> Add a driver that retreives error source descriptors from MM and
> populates those into the HEST ACPI table. The error source descriptors
> that are available from the MM side are retreived using MM Communicate 2
> protocol.
>
> The first call into the MM returns the size of MM Communicate buffer
> required to hold all error source descriptor info. The communication
> buffer of that size is then allocated and the second call into MM
> returns the error source descriptors in the communication buffer.
> The retreived error source descriptors are then appended to the HEST
> table.
>
> Co-authored-by: Thomas Abraham<thomas.abraham@arm.com>
> Signed-off-by: Omkar Anand Kulkarni<omkar.kulkarni@arm.com>
> ---
>   ArmPlatformPkg/ArmPlatformPkg.dec                                         |   7 +
>   ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceDxe.inf          |  44 +++
>   ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceStandaloneMm.inf |  51 ++++
>   ArmPlatformPkg/Drivers/HestMmErrorSources/HestMmErrorSourceCommon.h       |  37 +++
>   ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceDxe.c            | 308 +++++++++++++++++++
>   ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceStandaloneMm.c   | 312 ++++++++++++++++++++
[SAMI] Should this patch be split into 2?
>   6 files changed, 759 insertions(+)
>
> diff --git a/ArmPlatformPkg/ArmPlatformPkg.dec b/ArmPlatformPkg/ArmPlatformPkg.dec
> index 4f062292663b..846b3e863aa9 100644
> --- a/ArmPlatformPkg/ArmPlatformPkg.dec
> +++ b/ArmPlatformPkg/ArmPlatformPkg.dec
> @@ -52,6 +52,8 @@
>   
>   [Guids.common]
>     gArmPlatformTokenSpaceGuid   = { 0x9c0aaed4, 0x74c5, 0x4043, { 0xb4, 0x17, 0xa3, 0x22, 0x38, 0x14, 0xce, 0x76 } }
> +  gArmPlatformHestErrorSourcesGuid = { 0x76b8ab43, 0x822d, 0x4b00, { 0x9f, 0xd0, 0xf4, 0xa5, 0x35, 0x82, 0x47, 0x0a } }
> +  gMmHestGetErrorSourceInfoGuid = { 0x7d602951, 0x678e, 0x4cc4, { 0x98, 0xd9, 0xe3, 0x76, 0x04, 0xf6, 0x93, 0x0d } }
>   
>   [PcdsFeatureFlag.common]
>     gArmPlatformTokenSpaceGuid.PcdSendSgiToBringUpSecondaryCores|FALSE|BOOLEAN|0x00000004
> @@ -128,6 +130,11 @@
>   
>     gArmPlatformTokenSpaceGuid.PcdWatchdogCount|0x0|UINT32|0x00000033
>   
> +[PcdsFixedAtBuild, PcdsPatchableInModule]
> +  ## ACPI CPER memory space
> +  gArmPlatformTokenSpaceGuid.PcdGhesGenericErrorDataMmBufferBase|0x00000000|UINT64|0x00000046
> +  gArmPlatformTokenSpaceGuid.PcdGhesGenericErrorDataMmBufferSize|0x00000000|UINT64|0x00000047
> +
>   [Protocols.common]
>     ## Arm Platform HEST table generation protocol
>     gHestTableProtocolGuid = { 0x705bdcd9, 0x8c47, 0x457e, { 0xad, 0x0d, 0xf7, 0x86, 0xf3, 0x4a, 0x0d, 0x63 } }
> diff --git a/ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceDxe.inf b/ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceDxe.inf
> new file mode 100644
> index 000000000000..5227dea91630
> --- /dev/null
> +++ b/ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceDxe.inf
> @@ -0,0 +1,44 @@
> +## @file
> +#  DXE driver to get secure error sources.
> +#
> +#  DXE driver to retrieve the error source descriptors from Standalone MM and
> +#  append those to the HEST table.
> +#
> +#  Copyright (c) 2020 - 2021, ARM Limited. All rights reserved.
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 0x0001001A
> +  BASE_NAME                      = HestMmErrorSourceDxe
> +  FILE_GUID                      = 76b8ab43-822d-4b00-9fd0-f4a53582470a
> +  MODULE_TYPE                    = DXE_DRIVER
> +  VERSION_STRING                 = 1.0
> +  ENTRY_POINT                    = HestErrorSourceInitialize
> +
> +[Sources.common]
> +  HestErrorSourceDxe.c
> +
> +[Packages]
> +  ArmPkg/ArmPkg.dec
> +  ArmPlatformPkg/ArmPlatformPkg.dec
> +  MdePkg/MdePkg.dec
> +  StandaloneMmPkg/StandaloneMmPkg.dec
> +
> +[LibraryClasses]
> +  BaseMemoryLib
> +  DebugLib
> +  DxeServicesTableLib
> +  UefiDriverEntryPoint
> +  UefiLib
> +
> +[Guids]
> +  gMmHestGetErrorSourceInfoGuid                  ## PRODUCES
> +
> +[Protocols]
> +  gHestTableProtocolGuid                         ## CONSUMES
> +  gEfiMmCommunication2ProtocolGuid
> +
> +[Depex]
> +  gHestTableProtocolGuid AND gEfiMmCommunication2ProtocolGuid
> diff --git a/ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceStandaloneMm.inf b/ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceStandaloneMm.inf
> new file mode 100644
> index 000000000000..9d566de9bec3
> --- /dev/null
> +++ b/ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceStandaloneMm.inf
> @@ -0,0 +1,51 @@
> +## @file
> +#  HEST error source gateway Standalone MM driver.
> +#
> +#  Collects HEST error source descriptors,by communicating with all the MM
> +#  drivers implementing the HEST error source descriptor protocol.
> +#
> +#  Copyright (c) 2020 - 2021, ARM Limited. All rights reserved.
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 0x0001001A
> +  BASE_NAME                      = HestErrorSourceStandaloneMm
> +  FILE_GUID                      = 3ddbebcc-9841-4ef8-87fa-305843c1922d
> +  MODULE_TYPE                    = MM_STANDALONE
> +  VERSION_STRING                 = 1.0
> +  PI_SPECIFICATION_VERSION       = 0x00010032
> +  ENTRY_POINT                    = StandaloneMmHestErrorSourceInitialize
> +
> +[Sources]
> +  HestErrorSourceStandaloneMm.c
> +
> +[Packages]
> +  ArmPkg/ArmPkg.dec
> +  ArmPlatformPkg/ArmPlatformPkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  MdePkg/MdePkg.dec
> +  StandaloneMmPkg/StandaloneMmPkg.dec
> +
> +[LibraryClasses]
> +  ArmLib
> +  ArmSvcLib
> +  BaseMemoryLib
> +  DebugLib
> +  MemoryAllocationLib
> +  StandaloneMmDriverEntryPoint
> +
> +[Protocols]
> +  gMmHestErrorSourceDescProtocolGuid
> +
> +[Guids]
> +  gMmHestGetErrorSourceInfoGuid               ##PRODUCES
> +  gEfiStandaloneMmNonSecureBufferGuid
> +
> +[FixedPcd]
> +  gArmPlatformTokenSpaceGuid.PcdGhesGenericErrorDataMmBufferBase
> +  gArmPlatformTokenSpaceGuid.PcdGhesGenericErrorDataMmBufferSize
> +
> +[Depex]
> +  TRUE
> diff --git a/ArmPlatformPkg/Drivers/HestMmErrorSources/HestMmErrorSourceCommon.h b/ArmPlatformPkg/Drivers/HestMmErrorSources/HestMmErrorSourceCommon.h
> new file mode 100644
> index 000000000000..6ddc6bd21922
> --- /dev/null
> +++ b/ArmPlatformPkg/Drivers/HestMmErrorSources/HestMmErrorSourceCommon.h
> @@ -0,0 +1,37 @@
> +/** @file
> +  Data structures for error source descriptor information.
> +
> +  This data structure forms the CommBuffer part of the MM Communication
> +  protocol used for communicating the Hardware Error sources form MM to
> +  Non-MM environment.
> +
> +  Copyright (c) 2020 - 2021, ARM Limited. All rights reserved.
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +
> +#ifndef HEST_ERROR_SOURCE_DESCRIPTOR_H_
> +#define HEST_ERROR_SOURCE_DESCRIPTOR_H_
> +
> +#define HEST_ERROR_SOURCE_DESC_INFO_SIZE \
> +  (OFFSET_OF (HEST_ERROR_SOURCE_DESC_INFO, ErrSourceDescList))
[SAMI] I feel there can be a simple way to do this, see the comments below.
> +
> +//
> +// Data Structure to communicate the error source descriptor information from
> +// Standalone MM.
> +//
> +typedef struct {
> +  //
> +  // Total count of error source descriptors.
> +  //
> +  UINTN ErrSourceDescCount;
> +  //
> +  // Total size of all the error source descriptors.
> +  //
[SAMI] Does the Total size also include the size of ErrSourceDescCount 
and ErrSourceDescSize?
> +  UINTN ErrSourceDescSize;
[SAMI] Can the first 2 fields of this structure be moved to a structure 
called HEST_ERROR_SOURCE_DESC_HEADER? I think it may simplify computing 
of the size of HEST_ERROR_SOURCE_DESC_INFO.
[/SAMI]
> +  //
> +  // Array of error source descriptors that is ErrSourceDescSize in size.
> +  //
> +  UINT8 ErrSourceDescList[1];
> +} HEST_ERROR_SOURCE_DESC_INFO;
> +
> +#endif // HEST_ERROR_SOURCE_DESCRIPTOR_H_
> diff --git a/ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceDxe.c b/ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceDxe.c
> new file mode 100644
> index 000000000000..acfb0fc9e838
> --- /dev/null
> +++ b/ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceDxe.c
> @@ -0,0 +1,308 @@
> +/** @file
> +  Collects and appends the HEST error source descriptors from the MM drivers.
> +
> +  The drivers entry point locates the MM Communication protocol and calls into
> +  Standalone MM to get the HEST error sources length and count. It also
> +  retrieves descriptor information. The information is then used to build the
> +  HEST table using the HEST table generation protocol.
> +
> +  This driver collects the secure error source descriptor information from the
> +  MM drviers that implement HEST error source protocol. Instead of directly
> +  communicating with the individual MM drivers, it calls into
> +  HestErrorSourceStandaloneMM driver which is a gatway MM driver. This MM driver
> +  in-turn communicates with individual MM drivers collecting the error source
> +  descriptor information.
> +
> +  Once all the error source descriptor information is retrieved the driver
> +  appends the descriptors to HEST table using the HestDxe driver.
> +
> +  Copyright (c) 2020 - 2021, ARM Limited. All rights reserved.
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +
> +#include <IndustryStandard/Acpi.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/DxeServicesTableLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Protocol/MmCommunication2.h>
> +#include <Protocol/HestTable.h>
> +#include "HestMmErrorSourceCommon.h"
> +
> +#define MM_COMMUNICATE_HEADER_SIZE (OFFSET_OF (EFI_MM_COMMUNICATE_HEADER, Data))
[SAMI] Can this definition be moved to 
MdePkg\Include\Protocol\MmCommunication.h, please ?
> +
> +STATIC HEST_TABLE_PROTOCOL *mHestProtocol;
> +STATIC EFI_MM_COMMUNICATION2_PROTOCOL *mMmCommunication2;
> +
> +/**
> +  Retrieve the error source descriptors from Standalone MM.
> +
> +  Initialize the MM comminication buffer by assigning the MM service to
> +  invoke as gMmHestGetErrorSourceInfoGuid. Use the MM communication
> +  protocol to retrieve the error source descriptors.
> +
> +  @param[in]       CommBuffSize  Size of communicate buffer.
> +  @param[in, out]  CommBuffer    The communicate buffer.
> +
> +  @retval  EFI_SUCCESS  MM Communicate protocol call successful.
> +  @retval  Other        MM Communicate protocol call failed.
> +**/
> +STATIC
> +EFI_STATUS
> +GetErrorSourceDescriptors (
> +  IN     UINTN                     CommBuffSize,
> +  IN OUT EFI_MM_COMMUNICATE_HEADER **CommBuffer
> +  )
> +{
> +  EFI_STATUS Status;
> +
> +  //
> +  // Initialize the CommBuffer with MM Communicate metadata.
> +  //
> +  CopyGuid (&(*CommBuffer)->HeaderGuid, &gMmHestGetErrorSourceInfoGuid);
> +  (*CommBuffer)->MessageLength =
> +    CommBuffSize -
> +    sizeof ((*CommBuffer)->HeaderGuid) -
> +    sizeof ((*CommBuffer)->MessageLength);
> +
> +  //
> +  // Call into the Standalone MM using the MM Communicate protocol.
> +  //
> +  Status = mMmCommunication2->Communicate (
> +                                mMmCommunication2,
> +                                (VOID *)*CommBuffer,
> +                                (VOID *)*CommBuffer,
[SAMI] Can you check if the third parameter to Communicate() is correct, 
please?
> +                                NULL
> +                                );
> +
> +  return Status;
> +}
> +
> +/**
> +  Collect HEST error source descriptors from all Standalone MM drivers and
> +  append them to the HEST table.
> +
> +  Use MM Communication Protocol to communicate and collect the error source
> +  descriptor information from Standalone MM. Check for the required buffer size
> +  returned by the MM driver. Allocate buffer of adequate size and call again
> +  into MM.
> +
> +  @retval  EFI_SUCCESS           Successful to collect and append the error
> +                                 source.
> +                                 descriptors to HEST table.
> +  @retval  EFI_OUT_OF_RESOURCES  Memory allocation failure.
> +  @retval  Other                 For any other error.
> +**/
> +STATIC
> +EFI_STATUS
> +AppendMmErrorSources (VOID)
[SAMI] VOID and ) should be on a separate line. Can you check the other 
patches in this series as well, please?
> +{
> +  EFI_MM_COMMUNICATE_HEADER   *CommunicationHeader = NULL;
> +  HEST_ERROR_SOURCE_DESC_INFO *ErrorSourceDescInfo;
> +  EFI_STATUS                  Status;
> +  UINTN                       CommBufferSize;
> +
> +  //
> +  // Retrieve the count, length and the actual eror source descriptors from
> +  // the MM drivers. Do this by performing two MM Communicate calls, in the
> +  // first call pass CommBuffer which is atleast of the size of error source
> +  // descriptor info structure. Followed by another communicate call with
> +  // CommBuffer allocated to required buffer size to hold all descriptors.
> +  //
> +  // Allocate CommBuffer atleast the size of error source descriptor info
> +  // structure.
> +  CommBufferSize =
> +    MM_COMMUNICATE_HEADER_SIZE + HEST_ERROR_SOURCE_DESC_INFO_SIZE;
> +  CommunicationHeader = AllocatePool (CommBufferSize);
[SAMI] Would it be better to use AllocateZeroPool() ?
> +  if (CommunicationHeader == NULL) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "%a: Failed to allocate memory for CommunicationHeader\n",
> +      __FUNCTION__
> +      ));
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  //
> +  // Make the first MM Communicate call to HestErrorSourceStandaloneMM gateway
> +  // driver, which returns the required buffer size adequate to hold all the
> +  // desctriptor information.
> +  //
> +  Status = GetErrorSourceDescriptors (CommBufferSize, &CommunicationHeader);
> +  if ((EFI_ERROR (Status)) &&
> +      (Status != EFI_BAD_BUFFER_SIZE)) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "%a: MM Communicate protocol call failed, status: %r\n",
> +      __FUNCTION__,
> +      Status
> +      ));
> +    FreePool (CommunicationHeader);
> +    return Status;
> +  }
> +
> +  // Check for the length of Error Source descriptors.
> +  ErrorSourceDescInfo =
> +    (HEST_ERROR_SOURCE_DESC_INFO *)(CommunicationHeader->Data);
> +  if ((ErrorSourceDescInfo->ErrSourceDescSize == 0) ||
> +      (ErrorSourceDescInfo->ErrSourceDescCount == 0)) {
> +    DEBUG ((
> +      DEBUG_INFO,
> +      "HesErrorSourceDxe: HEST error source(s) not found\n"
> +      ));
> +    FreePool (CommunicationHeader);
> +    return EFI_SUCCESS;
[SAMI] return EFI_NOT_FOUND ?
> +  }
> +
> +  //
> +  // Allocate CommBuffer of required size to accomodate all the error source
> +  // descriptors. Required size of communication buffer =
> +  // MM communicate metadata. + (error source desc info struct + error source
> +  // descriptor size).
> +  //
> +  CommBufferSize =
> +    MM_COMMUNICATE_HEADER_SIZE +
> +    HEST_ERROR_SOURCE_DESC_INFO_SIZE +
> +    ErrorSourceDescInfo->ErrSourceDescSize;
> +
> +  // Free old MM Communicate buffer and allocate a new buffer of required size.
> +  FreePool (CommunicationHeader);
> +  CommunicationHeader = AllocatePool (CommBufferSize);
> +  if (CommunicationHeader == NULL) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "%a: Failed to allocate memory for CommunicationHeader\n",
> +      __FUNCTION__
> +      ));
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  //
> +  // Make second MM Communicate call to HestErrorSourceStandaloneMM driver to
> +  // get the error source descriptors from the MM drivers.
> +  //
> +  Status = GetErrorSourceDescriptors (CommBufferSize, &CommunicationHeader);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "%a: MM Communicate protocol failed, status: %r\n",
> +      __FUNCTION__,
> +      Status
> +      ));
> +    FreePool (CommunicationHeader);
> +    return Status;
> +  }
> +
> +  //
> +  // Retrieve the HEST error source descriptor information. Ensure that there
> +  // is a valid list of error source descriptors.
> +  //
> +  ErrorSourceDescInfo =
> +    (HEST_ERROR_SOURCE_DESC_INFO *)(CommunicationHeader->Data);
> +  if (ErrorSourceDescInfo->ErrSourceDescList == NULL) {
> +    DEBUG ((
> +      DEBUG_INFO,
> +      "HestErrorSourceDxe: Error source descriptor list is empty"
> +      ));
> +    FreePool (CommunicationHeader);
> +    return EFI_SUCCESS;
[SAMI] Can EFI_NOT_FOUND be returned here?
> +  }
> +
> +  DEBUG ((
> +    DEBUG_INFO,
> +    "HestErrorSourceDxe: ErrorSources: TotalCount = %d TotalLength = %d \n",
> +    ErrorSourceDescInfo->ErrSourceDescCount,
> +    ErrorSourceDescInfo->ErrSourceDescSize
> +    ));
> +
> +  //
> +  // Append the error source descriptors to HEST table using the HEST table
> +  // generation protocol.
> +  //
> +  Status = mHestProtocol->AppendErrorSourceDescriptors (
> +                            ErrorSourceDescInfo->ErrSourceDescList,
> +                            ErrorSourceDescInfo->ErrSourceDescSize,
> +                            ErrorSourceDescInfo->ErrSourceDescCount
> +                            );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "%a: Failed to append error source(s), status: %r\n",
> +      __FUNCTION__,
> +      Status
> +      ));
> +  }
> +
> +  FreePool (CommunicationHeader);
> +  return Status;
> +}
> +
> +/**
> +  The Entry Point for HEST Error Source Dxe driver.
> +
> +  Locates the HEST Table generation and MM Communication2 protocols. Using the
> +  MM Communication2, the driver collects the Error Source Descriptor(s) from
> +  Standalone MM. It then appends those Error Source Descriptor(s) to the Hest
> +  table using the HEST Table generation protocol.
> +
> +  @param[in]  ImageHandle  The firmware allocated handle for the Efi image.
> +  @param[in]  SystemTable  A pointer to the Efi System Table.
> +
> +  @retval  EFI_SUCCESS  The entry point is executed successfully.
> +  @retval  Other        Some error occurred when executing this entry point.
> +**/
> +EFI_STATUS
> +EFIAPI
> +HestErrorSourceInitialize (
> +  IN EFI_HANDLE       ImageHandle,
> +  IN EFI_SYSTEM_TABLE *SystemTable
> +  )
> +{
> +  EFI_STATUS Status;
> +
> +  Status = gBS->LocateProtocol (
> +                  &gHestTableProtocolGuid,
> +                  NULL,
> +                  (VOID **)&mHestProtocol
> +                  );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "%a: Failed to locate HEST table generation protocol, status:%r\n",
> +      __FUNCTION__,
> +      Status
> +      ));
> +    return Status;
> +  }
> +
> +  Status = gBS->LocateProtocol (
> +                  &gEfiMmCommunication2ProtocolGuid,
> +                  NULL,
> +                  (VOID **)&mMmCommunication2
> +                  );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "%a: Failed to locate MMCommunication2 driver protocol, status:%r\n",
> +      __FUNCTION__,
> +      Status
> +      ));
> +    return Status;
> +  }
> +
> +  //
> +  // Append HEST error sources retrieved from StandaloneMM, if any, into the
> +  // HEST ACPI table.
> +  //
> +  Status = AppendMmErrorSources ();
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "%a: Failed appending error source desc to HEST table, status:%r\n",
> +      __FUNCTION__,
> +      Status
> +      ));
> +  }
> +  return EFI_SUCCESS;
> +}
> diff --git a/ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceStandaloneMm.c b/ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceStandaloneMm.c
> new file mode 100644
> index 000000000000..c7b2304fc494
> --- /dev/null
> +++ b/ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceStandaloneMm.c
> @@ -0,0 +1,312 @@
> +/** @file
> +  MM HEST error source gateway driver.
> +
> +  This MM driver installs a handler which can be used to retrieve the error
> +  source descriptors from the all MM drivers implementing the HEST error source
> +  descriptor protocol.
> +
> +  The MM driver acts as a single point of contact to collect secure hardware
> +  error sources from the MM drivers. It loops over all the MM drivers that
> +  implement HEST error source descriptor protocol and collects error source
> +  descriptor information along with the error source count and length.
> +
> +  Copyright (c) 2020 - 2021, ARM Limited. All rights reserved.
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +
> +#include <Base.h>
> +#include <Library/BaseLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +#include <Protocol/HestErrorSourceInfo.h>
> +
> +#include "HestMmErrorSourceCommon.h"
> +
> +STATIC EFI_MM_SYSTEM_TABLE *mMmst = NULL;
> +
> +/**
> +  Returns an array of handles that implement the HEST error source descriptor
> +  protocol.
> +
> +  Passing HandleBuffer as NULL will return the actual size of the buffer
> +  required to hold the array of handles implementing the protocol.
> +
> +  @param[in, out]  HandleBufferSize  The size of the HandleBuffer.
> +  @param[out]      HandleBuffer      A pointer to the buffer containing the list
> +                                    of handles.
> +
> +  @retval  EFI_SUCCESS    The array of handles returned in HandleBuffer.
> +  @retval  EFI_NOT_FOUND  No implementation present for the protocol.
> +  @retval  Other          For any other error.
> +**/
> +STATIC
> +EFI_STATUS
> +GetHestErrorSourceProtocolHandles (
> +  IN OUT UINTN      *HandleBufferSize,
> +  OUT    EFI_HANDLE **HandleBuffer
> +  )
> +{
> +  EFI_STATUS Status;
> +
> +  Status = mMmst->MmLocateHandle (
> +                    ByProtocol,
> +                    &gMmHestErrorSourceDescProtocolGuid,
> +                    NULL,
> +                    HandleBufferSize,
> +                    *HandleBuffer
> +                    );
> +  if ((EFI_ERROR (Status)) &&
> +      (Status != EFI_BUFFER_TOO_SMALL))
> +  {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "%a: No implementation of MmHestErrorSourceDescProtocol found, \
> +       Status:%r\n",
> +      __FUNCTION__,
> +      Status
> +      ));
> +    return EFI_NOT_FOUND;
> +  }
> +
> +  return Status;
> +}
> +
> +/**
> +  Mmi handler to retrieve HEST error source descriptor information.
> +
> +  Handler for Mmi service that returns the supported HEST error source
> +  descriptors in MM. This handler populates the CommBuffer with the
> +  list of all error source descriptors, prepended with the length and
> +  the number of descriptors populated into CommBuffer.
> +
> +  @param[in]       DispatchHandle  The unique handle assigned to this handler by
> +                                   MmiHandlerRegister().
> +  @param[in]       Context         Points to an optional handler context that
> +                                   is specified when the handler was registered.
> +  @param[in, out]  CommBuffer      Buffer used for communication of HEST error
> +                                   source descriptors.
> +  @param[in, out]  CommBufferSize  The size of the CommBuffer.
> +
> +  @retval  EFI_SUCCESS            CommBuffer has valid data.
> +  @retval  EFI_BAD_BUFFER_SIZE    CommBufferSize not adequate.
> +  @retval  EFI_OUT_OF_RESOURCES   System out of memory resources.
> +  @retval  EFI_INVALID_PARAMETER  Invalid CommBufferSize recieved.
> +  @retval  Other                  For any other error.
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +HestErrorSourcesInfoMmiHandler (
> +  IN     EFI_HANDLE DispatchHandle,
> +  IN     CONST VOID *Context,       OPTIONAL
> +  IN OUT VOID       *CommBuffer,    OPTIONAL
> +  IN OUT UINTN      *CommBufferSize OPTIONAL
> +  )
> +{
> +  MM_HEST_ERROR_SOURCE_DESC_PROTOCOL *HestErrSourceDescProtocolHandle;
> +  HEST_ERROR_SOURCE_DESC_INFO        *ErrorSourceInfoList;
> +  EFI_HANDLE                         *HandleBuffer;
> +  EFI_STATUS                         Status;
> +  UINTN                              HandleCount;
> +  UINTN                              HandleBufferSize;
> +  UINTN                              Index;
> +  UINTN                              SourceCount = 0;
> +  UINTN                              SourceLength = 0;
> +  VOID                               *ErrorSourcePtr;
> +  UINTN                              TotalSourceLength = 0;
> +  UINTN                              TotalSourceCount = 0;
> +
> +  if (*CommBufferSize < HEST_ERROR_SOURCE_DESC_INFO_SIZE) {
> +    //
> +    // Ensures that the communication buffer has enough space to atleast hold
> +    // the ErrSourceDescCount and ErrSourceDescSize elements of the
> +    // HEST_ERROR_SOURCE_DESC_INFO structure.
> +    //
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "%a: Invalid CommBufferSize parameter\n",
> +      __FUNCTION__
> +      ));
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  //
> +  // Get all handles that implement the HEST error source descriptor protocol.
> +  // Get the buffer size required to store list of handles for the protocol.
> +  //
> +  HandleBuffer = NULL;
> +  HandleBufferSize = 0;
> +  Status = GetHestErrorSourceProtocolHandles (&HandleBufferSize, &HandleBuffer);
> +  if ((Status == EFI_NOT_FOUND) ||
> +      (HandleBufferSize == 0))
> +  {
> +    return Status;
> +  }
> +
> +  // Allocate memory for HandleBuffer of size HandleBufferSize.
> +  HandleBuffer = AllocatePool (HandleBufferSize);
[SAMI] AllocateZeroPool() ?
> +  if (HandleBuffer == NULL) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "%a: Failed to allocate memory for HandleBuffer\n",
> +      __FUNCTION__
> +      ));
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  // Get the list of handles.
> +  Status = GetHestErrorSourceProtocolHandles (&HandleBufferSize, &HandleBuffer);
> +  if ((EFI_ERROR (Status)) ||
> +      (HandleBuffer == NULL))
[SAMI] Is check for HandleBuffer == NULL right here?
> +  {
> +    FreePool (HandleBuffer);
> +    return Status;
> +  }
> +
> +  // Count of handles for the protocol.
> +  HandleCount = HandleBufferSize / sizeof (EFI_HANDLE);
> +
> +  //
> +  // Loop to get the count and length of the error source descriptors.
> +  //
> +  // This loop collects and adds the length of error source descriptors and
> +  // its count from all the the MM drivers implementing HEST error source.
> +  // descriptor protocol. The total length and count values retrieved help
> +  // to determine weather the CommBuffer is big enough to hold the descriptor
> +  // information.
> +  // As mentioned in the HEST error source descriptor protocol definition,
> +  // Buffer parameter set to NULL ensures only length and the count values
> +  // are returned from the driver and no error source information is copied to
> +  // Buffer.
> +  //
> +  for (Index = 0; Index < HandleCount; ++Index) {
> +    Status = mMmst->MmHandleProtocol (
> +                      HandleBuffer[Index],
> +                      &gMmHestErrorSourceDescProtocolGuid,
> +                      (VOID **)&HestErrSourceDescProtocolHandle
> +                      );
> +    if (EFI_ERROR (Status)) {
> +      continue;
> +    }
> +
> +    //
> +    // Protocol called with Buffer parameter passed as NULL, must return
> +    // error source length and error count for that driver.
> +    //
> +    Status = HestErrSourceDescProtocolHandle->GetHestErrorSourceDescriptors (
> +                                                HestErrSourceDescProtocolHandle,
> +                                                NULL,
> +                                                &SourceLength,
> +                                                &SourceCount
> +                                                );
> +    if (Status == EFI_INVALID_PARAMETER) {
[SAMI] I think the error handling in this function and the error return 
implementation in GetHestErrorSourceDescriptors() could be improved.
e.g. GetHestErrorSourceDescriptors() could first check for the 
SourceLength & SourceCount and if it is less than what is required, it 
returns EFI_BUFFER_TOO_SMALL.
The next check would be to check ErrorSourcePtr and return 
EFI_INVALID_PARAMETER if it is NULL.
  [/SAMI]
> +      TotalSourceLength += SourceLength;
> +      TotalSourceCount += SourceCount;
> +    }
> +  }
> +
> +  // Set the count and length in the error source descriptor.
> +  ErrorSourceInfoList = (HEST_ERROR_SOURCE_DESC_INFO *)(CommBuffer);
> +  ErrorSourceInfoList->ErrSourceDescCount = TotalSourceCount;
> +  ErrorSourceInfoList->ErrSourceDescSize = TotalSourceLength;
> +
> +  //
> +  // Check the size of CommBuffer, it should atleast be of size
> +  // TotalSourceLength + HEST_ERROR_SOURCE_DESC_INFO_SIZE.
> +  //
> +  TotalSourceLength = TotalSourceLength + HEST_ERROR_SOURCE_DESC_INFO_SIZE;
> +  if ((*CommBufferSize) < TotalSourceLength) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "%a: Invalid CommBufferSize parameter\n",
> +      __FUNCTION__
> +      ));
> +    FreePool (HandleBuffer);
> +    return EFI_BAD_BUFFER_SIZE;
[SAMI] Should the return code be EFI_BUFFER_TOO_SMALL? The difference 
being, the caller can attempt to call again with a larger buffer if 
EFI_BUFFER_TOO_SMALL is returned.
CommBufferSize is declared as an OUT paramter, was the intent to return 
the required buffer size?
[/SAMI]
> +  }
> +
> +  //
> +  // CommBuffer size is adequate to return all the error source descriptors.
> +  // So go ahead and populate it with the error source descriptor information.
> +  //
> +
> +  // Buffer pointer to append the Error Descriptors data.
> +  ErrorSourcePtr =  ErrorSourceInfoList->ErrSourceDescList;
> +
> +  //
> +  // Loop to retrieve error source descriptors information.
> +  //
> +  // Calls into each MM driver that implement the HEST error source descriptor
> +  // protocol. Here the Buffer parameter passed to the protocol service is
> +  // valid. So the MM driver when called copies the descriptor information.
> +  //
> +  for (Index = 0; Index < HandleCount; ++Index) {
> +    Status = mMmst->MmHandleProtocol (
> +                      HandleBuffer[Index],
> +                      &gMmHestErrorSourceDescProtocolGuid,
> +                      (VOID **)&HestErrSourceDescProtocolHandle
> +                      );
> +    if (EFI_ERROR (Status)) {
> +      continue;
> +    }
> +
> +    Status = HestErrSourceDescProtocolHandle->GetHestErrorSourceDescriptors (
> +                                                HestErrSourceDescProtocolHandle,
> +                                                (VOID **)&ErrorSourcePtr,
> +                                                &SourceLength,
> +                                                &SourceCount
> +                                                );
> +    if (Status == EFI_SUCCESS) {
> +      ErrorSourcePtr += SourceLength;
> +    }
> +  }
> +
> +  // Free the buffer holding all the protocol handles.
> +  FreePool (HandleBuffer);
> +
> +  return EFI_SUCCESS;
[SAMI] return Status of last operation.
> +}
> +
> +/**
> +  Entry point for this Stanalone MM driver.
> +
> +  Registers an Mmi handler that retrieves the error source descriptors from all
> +  the MM drivers implementing the MM_HEST_ERROR_SOURCE_DESC_PROTOCOL.
> +
> +  @param[in]  ImageHandle  The firmware allocated handle for the EFI image.
> +  @param[in]  SystemTable  A pointer to the EFI System Table.
> +
> +  @retval  EFI_SUCCESS  The entry point registered handler successfully.
> +  @retval  Other        Some error occurred when executing this entry point.
> +**/
> +EFI_STATUS
> +EFIAPI
> +StandaloneMmHestErrorSourceInitialize (
> +  IN EFI_HANDLE          ImageHandle,
> +  IN EFI_MM_SYSTEM_TABLE *SystemTable
> +  )
> +{
> +  EFI_HANDLE DispatchHandle;
> +  EFI_STATUS Status;
> +
> +  ASSERT (SystemTable != NULL);
> +  mMmst = SystemTable;
> +
> +  Status = mMmst->MmiHandlerRegister (
> +                    HestErrorSourcesInfoMmiHandler,
> +                    &gMmHestGetErrorSourceInfoGuid,
> +                    &DispatchHandle
> +                    );
> +  if (EFI_ERROR(Status)) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "%a: Mmi handler registration failed with status : %r\n",
> +      __FUNCTION__,
> +      Status
> +      ));
> +    return Status;
> +  }
> +
> +  return EFI_SUCCESS;
[SAMI] return Status of last operation.
> +}


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

Re: [PATCH v2 3/4] ArmPlatformPkg: retreive error source descriptors from MM

[Omkar Anand Kulkarni]

Hi Sami,

Thanks for the patch review. Please find my response inline.

Regards,
Omkar

Hi Omkar,
Please find my response inline marked [SAMI].
Regards,
Sami Mujawar

On 10/07/2021 05:18 PM, Omkar Anand Kulkarni wrote:
Add a driver that retreives error source descriptors from MM and
populates those into the HEST ACPI table. The error source descriptors
that are available from the MM side are retreived using MM Communicate 2
protocol.

The first call into the MM returns the size of MM Communicate buffer
required to hold all error source descriptor info. The communication
buffer of that size is then allocated and the second call into MM
returns the error source descriptors in the communication buffer.
The retreived error source descriptors are then appended to the HEST
table.

Co-authored-by: Thomas Abraham mailto:thomas.abraham@arm.com
Signed-off-by: Omkar Anand Kulkarni mailto:omkar.kulkarni@arm.com
---
 ArmPlatformPkg/ArmPlatformPkg.dec                                         |   7 +
 ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceDxe.inf          |  44 +++
 ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceStandaloneMm.inf |  51 ++++
 ArmPlatformPkg/Drivers/HestMmErrorSources/HestMmErrorSourceCommon.h       |  37 +++
 ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceDxe.c            | 308 +++++++++++++++++++
 ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceStandaloneMm.c   | 312 ++++++++++++++++++++
[SAMI] Should this patch be split into 2?

The reason of keep this a single patch is the 2 drivers, Dxe and its MM counterpart work together to collect the error source descriptors. 

Thought its logical to keep them together as they contribute to a single task which is collection of secure error source descriptors.

 6 files changed, 759 insertions(+)

diff --git a/ArmPlatformPkg/ArmPlatformPkg.dec b/ArmPlatformPkg/ArmPlatformPkg.dec
index 4f062292663b..846b3e863aa9 100644
--- a/ArmPlatformPkg/ArmPlatformPkg.dec
+++ b/ArmPlatformPkg/ArmPlatformPkg.dec
@@ -52,6 +52,8 @@
 
 [Guids.common]
   gArmPlatformTokenSpaceGuid   = { 0x9c0aaed4, 0x74c5, 0x4043, { 0xb4, 0x17, 0xa3, 0x22, 0x38, 0x14, 0xce, 0x76 } }
+  gArmPlatformHestErrorSourcesGuid = { 0x76b8ab43, 0x822d, 0x4b00, { 0x9f, 0xd0, 0xf4, 0xa5, 0x35, 0x82, 0x47, 0x0a } }
+  gMmHestGetErrorSourceInfoGuid = { 0x7d602951, 0x678e, 0x4cc4, { 0x98, 0xd9, 0xe3, 0x76, 0x04, 0xf6, 0x93, 0x0d } }
 
 [PcdsFeatureFlag.common]
   gArmPlatformTokenSpaceGuid.PcdSendSgiToBringUpSecondaryCores|FALSE|BOOLEAN|0x00000004
@@ -128,6 +130,11 @@
 
   gArmPlatformTokenSpaceGuid.PcdWatchdogCount|0x0|UINT32|0x00000033
 
+[PcdsFixedAtBuild, PcdsPatchableInModule]
+  ## ACPI CPER memory space
+  gArmPlatformTokenSpaceGuid.PcdGhesGenericErrorDataMmBufferBase|0x00000000|UINT64|0x00000046
+  gArmPlatformTokenSpaceGuid.PcdGhesGenericErrorDataMmBufferSize|0x00000000|UINT64|0x00000047
+
 [Protocols.common]
   ## Arm Platform HEST table generation protocol
   gHestTableProtocolGuid = { 0x705bdcd9, 0x8c47, 0x457e, { 0xad, 0x0d, 0xf7, 0x86, 0xf3, 0x4a, 0x0d, 0x63 } }
diff --git a/ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceDxe.inf b/ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceDxe.inf
new file mode 100644
index 000000000000..5227dea91630
--- /dev/null
+++ b/ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceDxe.inf
@@ -0,0 +1,44 @@
+## @file
+#  DXE driver to get secure error sources.
+#
+#  DXE driver to retrieve the error source descriptors from Standalone MM and
+#  append those to the HEST table.
+#
+#  Copyright (c) 2020 - 2021, ARM Limited. All rights reserved.
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+  INF_VERSION                    = 0x0001001A
+  BASE_NAME                      = HestMmErrorSourceDxe
+  FILE_GUID                      = 76b8ab43-822d-4b00-9fd0-f4a53582470a
+  MODULE_TYPE                    = DXE_DRIVER
+  VERSION_STRING                 = 1.0
+  ENTRY_POINT                    = HestErrorSourceInitialize
+
+[Sources.common]
+  HestErrorSourceDxe.c
+
+[Packages]
+  ArmPkg/ArmPkg.dec
+  ArmPlatformPkg/ArmPlatformPkg.dec
+  MdePkg/MdePkg.dec
+  StandaloneMmPkg/StandaloneMmPkg.dec
+
+[LibraryClasses]
+  BaseMemoryLib
+  DebugLib
+  DxeServicesTableLib
+  UefiDriverEntryPoint
+  UefiLib
+
+[Guids]
+  gMmHestGetErrorSourceInfoGuid                  ## PRODUCES
+
+[Protocols]
+  gHestTableProtocolGuid                         ## CONSUMES
+  gEfiMmCommunication2ProtocolGuid
+
+[Depex]
+  gHestTableProtocolGuid AND gEfiMmCommunication2ProtocolGuid
diff --git a/ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceStandaloneMm.inf b/ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceStandaloneMm.inf
new file mode 100644
index 000000000000..9d566de9bec3
--- /dev/null
+++ b/ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceStandaloneMm.inf
@@ -0,0 +1,51 @@
+## @file
+#  HEST error source gateway Standalone MM driver.
+#
+#  Collects HEST error source descriptors,by communicating with all the MM
+#  drivers implementing the HEST error source descriptor protocol.
+#
+#  Copyright (c) 2020 - 2021, ARM Limited. All rights reserved.
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+  INF_VERSION                    = 0x0001001A
+  BASE_NAME                      = HestErrorSourceStandaloneMm
+  FILE_GUID                      = 3ddbebcc-9841-4ef8-87fa-305843c1922d
+  MODULE_TYPE                    = MM_STANDALONE
+  VERSION_STRING                 = 1.0
+  PI_SPECIFICATION_VERSION       = 0x00010032
+  ENTRY_POINT                    = StandaloneMmHestErrorSourceInitialize
+
+[Sources]
+  HestErrorSourceStandaloneMm.c
+
+[Packages]
+  ArmPkg/ArmPkg.dec
+  ArmPlatformPkg/ArmPlatformPkg.dec
+  MdeModulePkg/MdeModulePkg.dec
+  MdePkg/MdePkg.dec
+  StandaloneMmPkg/StandaloneMmPkg.dec
+
+[LibraryClasses]
+  ArmLib
+  ArmSvcLib
+  BaseMemoryLib
+  DebugLib
+  MemoryAllocationLib
+  StandaloneMmDriverEntryPoint
+
+[Protocols]
+  gMmHestErrorSourceDescProtocolGuid
+
+[Guids]
+  gMmHestGetErrorSourceInfoGuid               ##PRODUCES
+  gEfiStandaloneMmNonSecureBufferGuid
+
+[FixedPcd]
+  gArmPlatformTokenSpaceGuid.PcdGhesGenericErrorDataMmBufferBase
+  gArmPlatformTokenSpaceGuid.PcdGhesGenericErrorDataMmBufferSize
+
+[Depex]
+  TRUE
diff --git a/ArmPlatformPkg/Drivers/HestMmErrorSources/HestMmErrorSourceCommon.h b/ArmPlatformPkg/Drivers/HestMmErrorSources/HestMmErrorSourceCommon.h
new file mode 100644
index 000000000000..6ddc6bd21922
--- /dev/null
+++ b/ArmPlatformPkg/Drivers/HestMmErrorSources/HestMmErrorSourceCommon.h
@@ -0,0 +1,37 @@
+/** @file
+  Data structures for error source descriptor information.
+
+  This data structure forms the CommBuffer part of the MM Communication
+  protocol used for communicating the Hardware Error sources form MM to
+  Non-MM environment.
+
+  Copyright (c) 2020 - 2021, ARM Limited. All rights reserved.
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#ifndef HEST_ERROR_SOURCE_DESCRIPTOR_H_
+#define HEST_ERROR_SOURCE_DESCRIPTOR_H_
+
+#define HEST_ERROR_SOURCE_DESC_INFO_SIZE \
+  (OFFSET_OF (HEST_ERROR_SOURCE_DESC_INFO, ErrSourceDescList))
[SAMI] I feel there can be a simple way to do this, see the comments below.

Okay, please see comment below.

- Omkar

+
+//
+// Data Structure to communicate the error source descriptor information from
+// Standalone MM.
+//
+typedef struct {
+  //
+  // Total count of error source descriptors.
+  //
+  UINTN ErrSourceDescCount;
+  //
+  // Total size of all the error source descriptors.
+  //
[SAMI] Does the Total size also include the size of ErrSourceDescCount and ErrSourceDescSize?  

No it only indicates the size of error source descriptors. HEST_ERROR_SOURCE_DESC_INFO struct represents the data part of the MM Communication buffer. So CommBuffSize  parameter for the MM Communication protocol takes care of the size for the entire structure.

- Omkar

+  UINTN ErrSourceDescSize;
[SAMI] Can the first 2 fields of this structure be moved to a structure called HEST_ERROR_SOURCE_DESC_HEADER? I think it may simplify computing of the size of HEST_ERROR_SOURCE_DESC_INFO. 
[/SAMI]

It was added to make code more intuitive, referring to example implementation for struct SMM_FTW_COMMUNICATE_FUNCTION_HEADER in Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmmCommon.h  

- Omkar

+  //
+  // Array of error source descriptors that is ErrSourceDescSize in size.
+  //
+  UINT8 ErrSourceDescList[1];
+} HEST_ERROR_SOURCE_DESC_INFO;
+
+#endif // HEST_ERROR_SOURCE_DESCRIPTOR_H_
diff --git a/ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceDxe.c b/ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceDxe.c
new file mode 100644
index 000000000000..acfb0fc9e838
--- /dev/null
+++ b/ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceDxe.c
@@ -0,0 +1,308 @@
+/** @file
+  Collects and appends the HEST error source descriptors from the MM drivers.
+
+  The drivers entry point locates the MM Communication protocol and calls into
+  Standalone MM to get the HEST error sources length and count. It also
+  retrieves descriptor information. The information is then used to build the
+  HEST table using the HEST table generation protocol.
+
+  This driver collects the secure error source descriptor information from the
+  MM drviers that implement HEST error source protocol. Instead of directly
+  communicating with the individual MM drivers, it calls into
+  HestErrorSourceStandaloneMM driver which is a gatway MM driver. This MM driver
+  in-turn communicates with individual MM drivers collecting the error source
+  descriptor information.
+
+  Once all the error source descriptor information is retrieved the driver
+  appends the descriptors to HEST table using the HestDxe driver.
+
+  Copyright (c) 2020 - 2021, ARM Limited. All rights reserved.
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#include <IndustryStandard/Acpi.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/DebugLib.h>
+#include <Library/DxeServicesTableLib.h>
+#include <Library/MemoryAllocationLib.h>
+#include <Library/UefiBootServicesTableLib.h>
+#include <Protocol/MmCommunication2.h>
+#include <Protocol/HestTable.h>
+#include "HestMmErrorSourceCommon.h"
+
+#define MM_COMMUNICATE_HEADER_SIZE (OFFSET_OF (EFI_MM_COMMUNICATE_HEADER, Data))
[SAMI] Can this definition be moved to MdePkg\Include\Protocol\MmCommunication.h, please ?

Ack.

- Omkar

+
+STATIC HEST_TABLE_PROTOCOL *mHestProtocol;
+STATIC EFI_MM_COMMUNICATION2_PROTOCOL *mMmCommunication2;
+
+/**
+  Retrieve the error source descriptors from Standalone MM.
+
+  Initialize the MM comminication buffer by assigning the MM service to
+  invoke as gMmHestGetErrorSourceInfoGuid. Use the MM communication
+  protocol to retrieve the error source descriptors.
+
+  @param[in]       CommBuffSize  Size of communicate buffer.
+  @param[in, out]  CommBuffer    The communicate buffer.
+
+  @retval  EFI_SUCCESS  MM Communicate protocol call successful.
+  @retval  Other        MM Communicate protocol call failed.
+**/
+STATIC
+EFI_STATUS
+GetErrorSourceDescriptors (
+  IN     UINTN                     CommBuffSize,
+  IN OUT EFI_MM_COMMUNICATE_HEADER **CommBuffer
+  )
+{
+  EFI_STATUS Status;
+
+  //
+  // Initialize the CommBuffer with MM Communicate metadata.
+  //
+  CopyGuid (&(*CommBuffer)->HeaderGuid, &gMmHestGetErrorSourceInfoGuid);
+  (*CommBuffer)->MessageLength =
+    CommBuffSize -
+    sizeof ((*CommBuffer)->HeaderGuid) -
+    sizeof ((*CommBuffer)->MessageLength);
+
+  //
+  // Call into the Standalone MM using the MM Communicate protocol.
+  //
+  Status = mMmCommunication2->Communicate (
+                                mMmCommunication2,
+                                (VOID *)*CommBuffer,
+                                (VOID *)*CommBuffer,
[SAMI] Can you check if the third parameter to Communicate() is correct, please?

Ack. Passing only 3rd parameter is sufficient as mmcommunicate2 protocol works only on virtual commbuffer address param.

- Omkar

+                                NULL
+                                );
+
+  return Status;
+}
+
+/**
+  Collect HEST error source descriptors from all Standalone MM drivers and
+  append them to the HEST table.
+
+  Use MM Communication Protocol to communicate and collect the error source
+  descriptor information from Standalone MM. Check for the required buffer size
+  returned by the MM driver. Allocate buffer of adequate size and call again
+  into MM.
+
+  @retval  EFI_SUCCESS           Successful to collect and append the error
+                                 source.
+                                 descriptors to HEST table.
+  @retval  EFI_OUT_OF_RESOURCES  Memory allocation failure.
+  @retval  Other                 For any other error.
+**/
+STATIC
+EFI_STATUS
+AppendMmErrorSources (VOID)
[SAMI] VOID and ) should be on a separate line. Can you check the other patches in this series as well, please?

Ack.

- Omkar

+{
+  EFI_MM_COMMUNICATE_HEADER   *CommunicationHeader = NULL;
+  HEST_ERROR_SOURCE_DESC_INFO *ErrorSourceDescInfo;
+  EFI_STATUS                  Status;
+  UINTN                       CommBufferSize;
+
+  //
+  // Retrieve the count, length and the actual eror source descriptors from
+  // the MM drivers. Do this by performing two MM Communicate calls, in the
+  // first call pass CommBuffer which is atleast of the size of error source
+  // descriptor info structure. Followed by another communicate call with
+  // CommBuffer allocated to required buffer size to hold all descriptors.
+  //
+  // Allocate CommBuffer atleast the size of error source descriptor info
+  // structure.
+  CommBufferSize =
+    MM_COMMUNICATE_HEADER_SIZE + HEST_ERROR_SOURCE_DESC_INFO_SIZE;
+  CommunicationHeader = AllocatePool (CommBufferSize);
[SAMI] Would it be better to use AllocateZeroPool() ?

Ack.

- Omkar

+  if (CommunicationHeader == NULL) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a: Failed to allocate memory for CommunicationHeader\n",
+      __FUNCTION__
+      ));
+    return EFI_OUT_OF_RESOURCES;
+  }
+
+  //
+  // Make the first MM Communicate call to HestErrorSourceStandaloneMM gateway
+  // driver, which returns the required buffer size adequate to hold all the
+  // desctriptor information.
+  //
+  Status = GetErrorSourceDescriptors (CommBufferSize, &CommunicationHeader);
+  if ((EFI_ERROR (Status)) &&
+      (Status != EFI_BAD_BUFFER_SIZE)) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a: MM Communicate protocol call failed, status: %r\n",
+      __FUNCTION__,
+      Status
+      ));
+    FreePool (CommunicationHeader);
+    return Status;
+  }
+
+  // Check for the length of Error Source descriptors.
+  ErrorSourceDescInfo =
+    (HEST_ERROR_SOURCE_DESC_INFO *)(CommunicationHeader->Data);
+  if ((ErrorSourceDescInfo->ErrSourceDescSize == 0) ||
+      (ErrorSourceDescInfo->ErrSourceDescCount == 0)) {
+    DEBUG ((
+      DEBUG_INFO,
+      "HesErrorSourceDxe: HEST error source(s) not found\n"
+      ));
+    FreePool (CommunicationHeader);
+    return EFI_SUCCESS;
[SAMI] return EFI_NOT_FOUND ?

Ack.

- Omkar

+  }
+
+  //
+  // Allocate CommBuffer of required size to accomodate all the error source
+  // descriptors. Required size of communication buffer =
+  // MM communicate metadata. + (error source desc info struct + error source
+  // descriptor size).
+  //
+  CommBufferSize =
+    MM_COMMUNICATE_HEADER_SIZE +
+    HEST_ERROR_SOURCE_DESC_INFO_SIZE +
+    ErrorSourceDescInfo->ErrSourceDescSize;
+
+  // Free old MM Communicate buffer and allocate a new buffer of required size.
+  FreePool (CommunicationHeader);
+  CommunicationHeader = AllocatePool (CommBufferSize);
+  if (CommunicationHeader == NULL) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a: Failed to allocate memory for CommunicationHeader\n",
+      __FUNCTION__
+      ));
+    return EFI_OUT_OF_RESOURCES;
+  }
+
+  //
+  // Make second MM Communicate call to HestErrorSourceStandaloneMM driver to
+  // get the error source descriptors from the MM drivers.
+  //
+  Status = GetErrorSourceDescriptors (CommBufferSize, &CommunicationHeader);
+  if (EFI_ERROR (Status)) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a: MM Communicate protocol failed, status: %r\n",
+      __FUNCTION__,
+      Status
+      ));
+    FreePool (CommunicationHeader);
+    return Status;
+  }
+
+  //
+  // Retrieve the HEST error source descriptor information. Ensure that there
+  // is a valid list of error source descriptors.
+  //
+  ErrorSourceDescInfo =
+    (HEST_ERROR_SOURCE_DESC_INFO *)(CommunicationHeader->Data);
+  if (ErrorSourceDescInfo->ErrSourceDescList == NULL) {
+    DEBUG ((
+      DEBUG_INFO,
+      "HestErrorSourceDxe: Error source descriptor list is empty"
+      ));
+    FreePool (CommunicationHeader);
+    return EFI_SUCCESS;
[SAMI] Can EFI_NOT_FOUND be returned here?

Ack.

- Omkar

+  }
+
+  DEBUG ((
+    DEBUG_INFO,
+    "HestErrorSourceDxe: ErrorSources: TotalCount = %d TotalLength = %d \n",
+    ErrorSourceDescInfo->ErrSourceDescCount,
+    ErrorSourceDescInfo->ErrSourceDescSize
+    ));
+
+  //
+  // Append the error source descriptors to HEST table using the HEST table
+  // generation protocol.
+  //
+  Status = mHestProtocol->AppendErrorSourceDescriptors (
+                            ErrorSourceDescInfo->ErrSourceDescList,
+                            ErrorSourceDescInfo->ErrSourceDescSize,
+                            ErrorSourceDescInfo->ErrSourceDescCount
+                            );
+  if (EFI_ERROR (Status)) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a: Failed to append error source(s), status: %r\n",
+      __FUNCTION__,
+      Status
+      ));
+  }
+
+  FreePool (CommunicationHeader);
+  return Status;
+}
+
+/**
+  The Entry Point for HEST Error Source Dxe driver.
+
+  Locates the HEST Table generation and MM Communication2 protocols. Using the
+  MM Communication2, the driver collects the Error Source Descriptor(s) from
+  Standalone MM. It then appends those Error Source Descriptor(s) to the Hest
+  table using the HEST Table generation protocol.
+
+  @param[in]  ImageHandle  The firmware allocated handle for the Efi image.
+  @param[in]  SystemTable  A pointer to the Efi System Table.
+
+  @retval  EFI_SUCCESS  The entry point is executed successfully.
+  @retval  Other        Some error occurred when executing this entry point.
+**/
+EFI_STATUS
+EFIAPI
+HestErrorSourceInitialize (
+  IN EFI_HANDLE       ImageHandle,
+  IN EFI_SYSTEM_TABLE *SystemTable
+  )
+{
+  EFI_STATUS Status;
+
+  Status = gBS->LocateProtocol (
+                  &gHestTableProtocolGuid,
+                  NULL,
+                  (VOID **)&mHestProtocol
+                  );
+  if (EFI_ERROR (Status)) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a: Failed to locate HEST table generation protocol, status:%r\n",
+      __FUNCTION__,
+      Status
+      ));
+    return Status;
+  }
+
+  Status = gBS->LocateProtocol (
+                  &gEfiMmCommunication2ProtocolGuid,
+                  NULL,
+                  (VOID **)&mMmCommunication2
+                  );
+  if (EFI_ERROR (Status)) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a: Failed to locate MMCommunication2 driver protocol, status:%r\n",
+      __FUNCTION__,
+      Status
+      ));
+    return Status;
+  }
+
+  //
+  // Append HEST error sources retrieved from StandaloneMM, if any, into the
+  // HEST ACPI table.
+  //
+  Status = AppendMmErrorSources ();
+  if (EFI_ERROR (Status)) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a: Failed appending error source desc to HEST table, status:%r\n",
+      __FUNCTION__,
+      Status
+      ));
+  }
+  return EFI_SUCCESS;
+}
diff --git a/ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceStandaloneMm.c b/ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceStandaloneMm.c
new file mode 100644
index 000000000000..c7b2304fc494
--- /dev/null
+++ b/ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceStandaloneMm.c
@@ -0,0 +1,312 @@
+/** @file
+  MM HEST error source gateway driver.
+
+  This MM driver installs a handler which can be used to retrieve the error
+  source descriptors from the all MM drivers implementing the HEST error source
+  descriptor protocol.
+
+  The MM driver acts as a single point of contact to collect secure hardware
+  error sources from the MM drivers. It loops over all the MM drivers that
+  implement HEST error source descriptor protocol and collects error source
+  descriptor information along with the error source count and length.
+
+  Copyright (c) 2020 - 2021, ARM Limited. All rights reserved.
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#include <Base.h>
+#include <Library/BaseLib.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/DebugLib.h>
+#include <Library/MemoryAllocationLib.h>
+#include <Protocol/HestErrorSourceInfo.h>
+
+#include "HestMmErrorSourceCommon.h"
+
+STATIC EFI_MM_SYSTEM_TABLE *mMmst = NULL;
+
+/**
+  Returns an array of handles that implement the HEST error source descriptor
+  protocol.
+
+  Passing HandleBuffer as NULL will return the actual size of the buffer
+  required to hold the array of handles implementing the protocol.
+
+  @param[in, out]  HandleBufferSize  The size of the HandleBuffer.
+  @param[out]      HandleBuffer      A pointer to the buffer containing the list
+                                    of handles.
+
+  @retval  EFI_SUCCESS    The array of handles returned in HandleBuffer.
+  @retval  EFI_NOT_FOUND  No implementation present for the protocol.
+  @retval  Other          For any other error.
+**/
+STATIC
+EFI_STATUS
+GetHestErrorSourceProtocolHandles (
+  IN OUT UINTN      *HandleBufferSize,
+  OUT    EFI_HANDLE **HandleBuffer
+  )
+{
+  EFI_STATUS Status;
+
+  Status = mMmst->MmLocateHandle (
+                    ByProtocol,
+                    &gMmHestErrorSourceDescProtocolGuid,
+                    NULL,
+                    HandleBufferSize,
+                    *HandleBuffer
+                    );
+  if ((EFI_ERROR (Status)) &&
+      (Status != EFI_BUFFER_TOO_SMALL))
+  {
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a: No implementation of MmHestErrorSourceDescProtocol found, \
+       Status:%r\n",
+      __FUNCTION__,
+      Status
+      ));
+    return EFI_NOT_FOUND;
+  }
+
+  return Status;
+}
+
+/**
+  Mmi handler to retrieve HEST error source descriptor information.
+
+  Handler for Mmi service that returns the supported HEST error source
+  descriptors in MM. This handler populates the CommBuffer with the
+  list of all error source descriptors, prepended with the length and
+  the number of descriptors populated into CommBuffer.
+
+  @param[in]       DispatchHandle  The unique handle assigned to this handler by
+                                   MmiHandlerRegister().
+  @param[in]       Context         Points to an optional handler context that
+                                   is specified when the handler was registered.
+  @param[in, out]  CommBuffer      Buffer used for communication of HEST error
+                                   source descriptors.
+  @param[in, out]  CommBufferSize  The size of the CommBuffer.
+
+  @retval  EFI_SUCCESS            CommBuffer has valid data.
+  @retval  EFI_BAD_BUFFER_SIZE    CommBufferSize not adequate.
+  @retval  EFI_OUT_OF_RESOURCES   System out of memory resources.
+  @retval  EFI_INVALID_PARAMETER  Invalid CommBufferSize recieved.
+  @retval  Other                  For any other error.
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+HestErrorSourcesInfoMmiHandler (
+  IN     EFI_HANDLE DispatchHandle,
+  IN     CONST VOID *Context,       OPTIONAL
+  IN OUT VOID       *CommBuffer,    OPTIONAL
+  IN OUT UINTN      *CommBufferSize OPTIONAL
+  )
+{
+  MM_HEST_ERROR_SOURCE_DESC_PROTOCOL *HestErrSourceDescProtocolHandle;
+  HEST_ERROR_SOURCE_DESC_INFO        *ErrorSourceInfoList;
+  EFI_HANDLE                         *HandleBuffer;
+  EFI_STATUS                         Status;
+  UINTN                              HandleCount;
+  UINTN                              HandleBufferSize;
+  UINTN                              Index;
+  UINTN                              SourceCount = 0;
+  UINTN                              SourceLength = 0;
+  VOID                               *ErrorSourcePtr;
+  UINTN                              TotalSourceLength = 0;
+  UINTN                              TotalSourceCount = 0;
+
+  if (*CommBufferSize < HEST_ERROR_SOURCE_DESC_INFO_SIZE) {
+    //
+    // Ensures that the communication buffer has enough space to atleast hold
+    // the ErrSourceDescCount and ErrSourceDescSize elements of the
+    // HEST_ERROR_SOURCE_DESC_INFO structure.
+    //
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a: Invalid CommBufferSize parameter\n",
+      __FUNCTION__
+      ));
+    return EFI_INVALID_PARAMETER;
+  }
+
+  //
+  // Get all handles that implement the HEST error source descriptor protocol.
+  // Get the buffer size required to store list of handles for the protocol.
+  //
+  HandleBuffer = NULL;
+  HandleBufferSize = 0;
+  Status = GetHestErrorSourceProtocolHandles (&HandleBufferSize, &HandleBuffer);
+  if ((Status == EFI_NOT_FOUND) ||
+      (HandleBufferSize == 0))
+  {
+    return Status;
+  }
+
+  // Allocate memory for HandleBuffer of size HandleBufferSize.
+  HandleBuffer = AllocatePool (HandleBufferSize);
[SAMI] AllocateZeroPool() ?

Ack.

+  if (HandleBuffer == NULL) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a: Failed to allocate memory for HandleBuffer\n",
+      __FUNCTION__
+      ));
+    return EFI_OUT_OF_RESOURCES;
+  }
+
+  // Get the list of handles.
+  Status = GetHestErrorSourceProtocolHandles (&HandleBufferSize, &HandleBuffer);
+  if ((EFI_ERROR (Status)) ||
+      (HandleBuffer == NULL))
[SAMI] Is check for HandleBuffer == NULL right here? 

Ack.

- Omkar

+  {
+    FreePool (HandleBuffer);
+    return Status;
+  }
+
+  // Count of handles for the protocol.
+  HandleCount = HandleBufferSize / sizeof (EFI_HANDLE);
+
+  //
+  // Loop to get the count and length of the error source descriptors.
+  //
+  // This loop collects and adds the length of error source descriptors and
+  // its count from all the the MM drivers implementing HEST error source.
+  // descriptor protocol. The total length and count values retrieved help
+  // to determine weather the CommBuffer is big enough to hold the descriptor
+  // information.
+  // As mentioned in the HEST error source descriptor protocol definition,
+  // Buffer parameter set to NULL ensures only length and the count values
+  // are returned from the driver and no error source information is copied to
+  // Buffer.
+  //
+  for (Index = 0; Index < HandleCount; ++Index) {
+    Status = mMmst->MmHandleProtocol (
+                      HandleBuffer[Index],
+                      &gMmHestErrorSourceDescProtocolGuid,
+                      (VOID **)&HestErrSourceDescProtocolHandle
+                      );
+    if (EFI_ERROR (Status)) {
+      continue;
+    }
+
+    //
+    // Protocol called with Buffer parameter passed as NULL, must return
+    // error source length and error count for that driver.
+    //
+    Status = HestErrSourceDescProtocolHandle->GetHestErrorSourceDescriptors (
+                                                HestErrSourceDescProtocolHandle,
+                                                NULL,
+                                                &SourceLength,
+                                                &SourceCount
+                                                );
+    if (Status == EFI_INVALID_PARAMETER) {
[SAMI] I think the error handling in this function and the error return implementation in GetHestErrorSourceDescriptors() could be improved. 
e.g. GetHestErrorSourceDescriptors() could first check for the SourceLength & SourceCount and if it is less than what is required, it returns EFI_BUFFER_TOO_SMALL.
The next check would be to check ErrorSourcePtr and return EFI_INVALID_PARAMETER if it is NULL.
 [/SAMI]

This also considers the case where there are multiple MM drivers that can return error source descriptors. In case of multiple implementations for MmHestErrorSourceDescProtocol protocol by MM drivers this can lead to unnecessary calls between secure and non-secure world via MM Communicate. Instead as mentioned by the protocol definition for MmHestErrorSourceDescProtocol, first get the count and length of descriptors from all the MM drivers implementing protocol MmHestErrorSourceDescProtocol by passing “Buffer” param as NULL. This call fails by returning EFI_INVALID_PARAMETER but returns count and size of the error source descriptors it supports.

- Omkar

+      TotalSourceLength += SourceLength;
+      TotalSourceCount += SourceCount;
+    }
+  }
+
+  // Set the count and length in the error source descriptor.
+  ErrorSourceInfoList = (HEST_ERROR_SOURCE_DESC_INFO *)(CommBuffer);
+  ErrorSourceInfoList->ErrSourceDescCount = TotalSourceCount;
+  ErrorSourceInfoList->ErrSourceDescSize = TotalSourceLength;
+
+  //
+  // Check the size of CommBuffer, it should atleast be of size
+  // TotalSourceLength + HEST_ERROR_SOURCE_DESC_INFO_SIZE.
+  //
+  TotalSourceLength = TotalSourceLength + HEST_ERROR_SOURCE_DESC_INFO_SIZE;
+  if ((*CommBufferSize) < TotalSourceLength) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a: Invalid CommBufferSize parameter\n",
+      __FUNCTION__
+      ));
+    FreePool (HandleBuffer);
+    return EFI_BAD_BUFFER_SIZE;
[SAMI] Should the return code be EFI_BUFFER_TOO_SMALL? The difference being, the caller can attempt to call again with a larger buffer if EFI_BUFFER_TOO_SMALL is returned. 
CommBufferSize is declared as an OUT paramter, was the intent to return the required buffer size?
[/SAMI]

Ack. ErrSourceDescSize is used to pass the required buffer size.

- Omkar

+  }
+
+  //
+  // CommBuffer size is adequate to return all the error source descriptors.
+  // So go ahead and populate it with the error source descriptor information.
+  //
+
+  // Buffer pointer to append the Error Descriptors data.
+  ErrorSourcePtr =  ErrorSourceInfoList->ErrSourceDescList;
+
+  //
+  // Loop to retrieve error source descriptors information.
+  //
+  // Calls into each MM driver that implement the HEST error source descriptor
+  // protocol. Here the Buffer parameter passed to the protocol service is
+  // valid. So the MM driver when called copies the descriptor information.
+  //
+  for (Index = 0; Index < HandleCount; ++Index) {
+    Status = mMmst->MmHandleProtocol (
+                      HandleBuffer[Index],
+                      &gMmHestErrorSourceDescProtocolGuid,
+                      (VOID **)&HestErrSourceDescProtocolHandle
+                      );
+    if (EFI_ERROR (Status)) {
+      continue;
+    }
+
+    Status = HestErrSourceDescProtocolHandle->GetHestErrorSourceDescriptors (
+                                                HestErrSourceDescProtocolHandle,
+                                                (VOID **)&ErrorSourcePtr,
+                                                &SourceLength,
+                                                &SourceCount
+                                                );
+    if (Status == EFI_SUCCESS) {
+      ErrorSourcePtr += SourceLength;
+    }
+  }
+
+  // Free the buffer holding all the protocol handles.
+  FreePool (HandleBuffer);
+
+  return EFI_SUCCESS;
[SAMI] return Status of last operation.

Ack.

- Omkar

+}
+
+/**
+  Entry point for this Stanalone MM driver.
+
+  Registers an Mmi handler that retrieves the error source descriptors from all
+  the MM drivers implementing the MM_HEST_ERROR_SOURCE_DESC_PROTOCOL.
+
+  @param[in]  ImageHandle  The firmware allocated handle for the EFI image.
+  @param[in]  SystemTable  A pointer to the EFI System Table.
+
+  @retval  EFI_SUCCESS  The entry point registered handler successfully.
+  @retval  Other        Some error occurred when executing this entry point.
+**/
+EFI_STATUS
+EFIAPI
+StandaloneMmHestErrorSourceInitialize (
+  IN EFI_HANDLE          ImageHandle,
+  IN EFI_MM_SYSTEM_TABLE *SystemTable
+  )
+{
+  EFI_HANDLE DispatchHandle;
+  EFI_STATUS Status;
+
+  ASSERT (SystemTable != NULL);
+  mMmst = SystemTable;
+
+  Status = mMmst->MmiHandlerRegister (
+                    HestErrorSourcesInfoMmiHandler,
+                    &gMmHestGetErrorSourceInfoGuid,
+                    &DispatchHandle
+                    );
+  if (EFI_ERROR(Status)) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a: Mmi handler registration failed with status : %r\n",
+      __FUNCTION__,
+      Status
+      ));
+    return Status;
+  }
+
+  return EFI_SUCCESS;
[SAMI] return Status of last operation.

Ack.

- Omkar

+}

[PATCH v2 4/4] ArmPlatformPkg: Add helpers for HEST table generation

[Omkar Anand Kulkarni]

Add helper macros for the generation of the HEST ACPI table. Macros to
initialize the HEST GHESv2 Notification Structure and Error Status
Structure are introduced.

Signed-off-by: Omkar Anand Kulkarni <omkar.kulkarni@arm.com>
---
 ArmPlatformPkg/Include/HestAcpiHeader.h | 49 ++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/ArmPlatformPkg/Include/HestAcpiHeader.h b/ArmPlatformPkg/Include/HestAcpiHeader.h
new file mode 100644
index 000000000000..5112ee5b22c5
--- /dev/null
+++ b/ArmPlatformPkg/Include/HestAcpiHeader.h
@@ -0,0 +1,49 @@
+/** @file
+  HEST table helper macros.
+
+  Macro definitions to initialize the HEST ACPI table specific structures.
+
+  Copyright (c) 2021, ARM Limited. All rights reserved.
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+  @par Specification Reference:
+    - ACPI Reference Specification 6.3
+    - UEFI Reference Specification 2.8
+**/
+
+#ifndef HEST_ACPI_HEADER_
+#define HEST_ACPI_HEADER_
+
+#include <IndustryStandard/Acpi.h>
+
+//
+// HEST table GHESv2 type related structures.
+//
+// Helper Macro to initialize the HEST GHESv2 Notification Structure.
+// Refer Table 18-394 in ACPI Specification, Version 6.3.
+#define EFI_ACPI_6_3_HARDWARE_ERROR_NOTIFICATION_STRUCTURE_INIT(Type,         \
+  PollInterval, EventId)                                                      \
+  {                                                                           \
+    Type,                                                                     \
+    sizeof (EFI_ACPI_6_3_HARDWARE_ERROR_NOTIFICATION_STRUCTURE),              \
+    {0, 0, 0, 0, 0, 0, 0}, /* ConfigurationWriteEnable */                     \
+    PollInterval,                                                             \
+    EventId,                                                                  \
+    0,                    /* Poll Interval Threshold Value  */                \
+    0,                    /* Poll Interval Threshold Window */                \
+    0,                    /* Error Threshold Value          */                \
+    0                     /* Error Threshold Window         */                \
+  }
+
+// Helper Macro to initialize the HEST GHESv2 Error Status Structure.
+// Refer Section 5.2.3.2 in ACPI Specification, Version 6.3.
+#define EFI_ACPI_6_3_GENERIC_ERROR_STATUS_STRUCTURE_INIT(Address)     \
+  {                                                                   \
+    0,        /* UINT8 Address Space ID */                            \
+    64,       /* Register Bit Width     */                            \
+    0,        /* Register Bit Offset    */                            \
+    4,        /* Access Size            */                            \
+    Address   /* CPER/Read Ack Addr     */                            \
+  }
+
+#endif /* HEST_ACPI_HEADER_ */
-- 
2.17.1

Re: [PATCH v2 4/4] ArmPlatformPkg: Add helpers for HEST table generation

[Sami Mujawar]

Hi Omkar,

Thank you for this patch.

I have a minor suggestion marked inline as [SAMI], other than that this 
patch looks good to me.

Reviewed-by: Sami Mujawar<sami.mujawar@arm.com>

Regards,

Sami Mujawar


On 10/07/2021 05:18 PM, Omkar Anand Kulkarni wrote:
> Add helper macros for the generation of the HEST ACPI table. Macros to
> initialize the HEST GHESv2 Notification Structure and Error Status
> Structure are introduced.
>
> Signed-off-by: Omkar Anand Kulkarni <omkar.kulkarni@arm.com>
> ---
>   ArmPlatformPkg/Include/HestAcpiHeader.h | 49 ++++++++++++++++++++
>   1 file changed, 49 insertions(+)
>
> diff --git a/ArmPlatformPkg/Include/HestAcpiHeader.h b/ArmPlatformPkg/Include/HestAcpiHeader.h
> new file mode 100644
> index 000000000000..5112ee5b22c5
> --- /dev/null
> +++ b/ArmPlatformPkg/Include/HestAcpiHeader.h
> @@ -0,0 +1,49 @@
> +/** @file
> +  HEST table helper macros.
> +
> +  Macro definitions to initialize the HEST ACPI table specific structures.
> +
> +  Copyright (c) 2021, ARM Limited. All rights reserved.
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +  @par Specification Reference:
> +    - ACPI Reference Specification 6.3
> +    - UEFI Reference Specification 2.8
> +**/
> +
> +#ifndef HEST_ACPI_HEADER_
> +#define HEST_ACPI_HEADER_
> +
> +#include <IndustryStandard/Acpi.h>
> +
> +//
> +// HEST table GHESv2 type related structures.
> +//
> +// Helper Macro to initialize the HEST GHESv2 Notification Structure.
> +// Refer Table 18-394 in ACPI Specification, Version 6.3.
> +#define EFI_ACPI_6_3_HARDWARE_ERROR_NOTIFICATION_STRUCTURE_INIT(Type,         \
> +  PollInterval, EventId)                                                      \
> +  {                                                                           \
> +    Type,                                                                     \
> +    sizeof (EFI_ACPI_6_3_HARDWARE_ERROR_NOTIFICATION_STRUCTURE),              \
> +    {0, 0, 0, 0, 0, 0, 0}, /* ConfigurationWriteEnable */                     \
> +    PollInterval,                                                             \
> +    EventId,                                                                  \
> +    0,                    /* Poll Interval Threshold Value  */                \
> +    0,                    /* Poll Interval Threshold Window */                \
> +    0,                    /* Error Threshold Value          */                \
> +    0                     /* Error Threshold Window         */                \
> +  }
> +
> +// Helper Macro to initialize the HEST GHESv2 Error Status Structure.
> +// Refer Section 5.2.3.2 in ACPI Specification, Version 6.3.
> +#define EFI_ACPI_6_3_GENERIC_ERROR_STATUS_STRUCTURE_INIT(Address)     \
[SAMI] Would it be possible to define ARM_GAS64() in 
EmbeddedPkg\Include\Library\AcpiLib.h instead of this macro?
Similarly, can EFI_ACPI_6_3_HARDWARE_ERROR_NOTIFICATION_STRUCTURE_INIT() 
macro also be placed in EmbeddedPkg\Include\Library\AcpiLib.h
[/SAMI]
> +  {                                                                   \
> +    0,        /* UINT8 Address Space ID */                            \
> +    64,       /* Register Bit Width     */                            \
> +    0,        /* Register Bit Offset    */                            \
> +    4,        /* Access Size            */                            \
> +    Address   /* CPER/Read Ack Addr     */                            \
> +  }
> +
> +#endif /* HEST_ACPI_HEADER_ */

Re: [PATCH v2 4/4] ArmPlatformPkg: Add helpers for HEST table generation

[Omkar Anand Kulkarni]

Hi Sami,

Thanks for reviewing this patch. Please find my response inline.

Regards,
Omkar

> Hi Omkar,
> 
> Thank you for this patch.
> 
> I have a minor suggestion marked inline as [SAMI], other than that this patch
> looks good to me.
> 
> Reviewed-by: Sami Mujawar<sami.mujawar@arm.com>
> 
> Regards,
> 
> Sami Mujawar
> 
> 
> On 10/07/2021 05:18 PM, Omkar Anand Kulkarni wrote:
> > Add helper macros for the generation of the HEST ACPI table. Macros to
> > initialize the HEST GHESv2 Notification Structure and Error Status
> > Structure are introduced.
> >
> > Signed-off-by: Omkar Anand Kulkarni <omkar.kulkarni@arm.com>
> > ---
> >   ArmPlatformPkg/Include/HestAcpiHeader.h | 49 ++++++++++++++++++++
> >   1 file changed, 49 insertions(+)
> >
> > diff --git a/ArmPlatformPkg/Include/HestAcpiHeader.h
> > b/ArmPlatformPkg/Include/HestAcpiHeader.h
> > new file mode 100644
> > index 000000000000..5112ee5b22c5
> > --- /dev/null
> > +++ b/ArmPlatformPkg/Include/HestAcpiHeader.h
> > @@ -0,0 +1,49 @@
> > +/** @file
> > +  HEST table helper macros.
> > +
> > +  Macro definitions to initialize the HEST ACPI table specific structures.
> > +
> > +  Copyright (c) 2021, ARM Limited. All rights reserved.
> > +  SPDX-License-Identifier: BSD-2-Clause-Patent
> > +
> > +  @par Specification Reference:
> > +    - ACPI Reference Specification 6.3
> > +    - UEFI Reference Specification 2.8 **/
> > +
> > +#ifndef HEST_ACPI_HEADER_
> > +#define HEST_ACPI_HEADER_
> > +
> > +#include <IndustryStandard/Acpi.h>
> > +
> > +//
> > +// HEST table GHESv2 type related structures.
> > +//
> > +// Helper Macro to initialize the HEST GHESv2 Notification Structure.
> > +// Refer Table 18-394 in ACPI Specification, Version 6.3.
> > +#define
> EFI_ACPI_6_3_HARDWARE_ERROR_NOTIFICATION_STRUCTURE_INIT(Type,
> \
> > +  PollInterval, EventId)                                                      \
> > +  {                                                                           \
> > +    Type,                                                                     \
> > +    sizeof (EFI_ACPI_6_3_HARDWARE_ERROR_NOTIFICATION_STRUCTURE),
> \
> > +    {0, 0, 0, 0, 0, 0, 0}, /* ConfigurationWriteEnable */                     \
> > +    PollInterval,                                                             \
> > +    EventId,                                                                  \
> > +    0,                    /* Poll Interval Threshold Value  */                \
> > +    0,                    /* Poll Interval Threshold Window */                \
> > +    0,                    /* Error Threshold Value          */                \
> > +    0                     /* Error Threshold Window         */                \
> > +  }
> > +
> > +// Helper Macro to initialize the HEST GHESv2 Error Status Structure.
> > +// Refer Section 5.2.3.2 in ACPI Specification, Version 6.3.
> > +#define
> EFI_ACPI_6_3_GENERIC_ERROR_STATUS_STRUCTURE_INIT(Address)     \
> [SAMI] Would it be possible to define ARM_GAS64() in
> EmbeddedPkg\Include\Library\AcpiLib.h instead of this macro?
> Similarly, can
> EFI_ACPI_6_3_HARDWARE_ERROR_NOTIFICATION_STRUCTURE_INIT()
> macro also be placed in EmbeddedPkg\Include\Library\AcpiLib.h
> [/SAMI]

Ack.

- Omkar

> > +  {                                                                   \
> > +    0,        /* UINT8 Address Space ID */                            \
> > +    64,       /* Register Bit Width     */                            \
> > +    0,        /* Register Bit Offset    */                            \
> > +    4,        /* Access Size            */                            \
> > +    Address   /* CPER/Read Ack Addr     */                            \
> > +  }
> > +
> > +#endif /* HEST_ACPI_HEADER_ */

Re: [PATCH v2 0/4] ArmPlatformPkg: Add support to generate HEST ACPI table

[Sami Mujawar]

Hi Omkar,

Thank you for this patch series and for the clear explaination below.

The explaination below is very useful for anyone who is trying to 
understand the code.

Since the cover letter will not be part of the patch commit messages, 
would it be possible to include this explanation:

1.  as part of a commit message for one of the patches in this series 
(patch 2/4 or 3/4).

Or

2. in a Readme.md file

Regards,

Sami Mujawar


On 10/07/2021 05:18 PM, Omkar Anand Kulkarni wrote:
> Changes since v1:
> - Helper added for HEST ACPI table generation.
> - Rebased to the latest upstream code.
>
> Hardware Error Source Table (HEST)[1] and Software Delegated Exception Interface
> (SDEI)[2] ACPI tables are used to acomplish firmware first error handling.This
> patch series introduces a framework to build and install the HEST ACPI table
> dynamically.
>
> The following figure illustrates the possible usage of the dyanamic
> generation of HEST ACPI table.
>
>                                      NS | S
> +--------------------------------------+--------------------------------------+
> |                                      |                                      |
> |+-------------------------------------+---------------------+                |
> ||               +---------------------+--------------------+|                |
> ||               |                     |                    ||                |
> || +-----------+ |+------------------+ | +-----------------+|| +-------------+|
> || |HestTable  | ||  HestErrorSource | | | HestErrorSource ||| | DMC-620     ||
> || |  DXE      | ||        DXE       | | |  StandaloneMM   ||| |Standalone MM||
> || +-----------+ |+------------------+ | +-----------------+|| +-------------+|
> ||               |GHESv2               |                    ||                |
> ||               +---------------------+--------------------+|                |
> ||          +--------------------+     |                     |                |
> ||          |PlatformErrorHandler|     |                     |                |
> ||          |        DXE         |     |                     |                |
> ||          +--------------------+     |                     |                |
> ||FF FWK                               |                     |                |
> |+-------------------------------------+---------------------+                |
> |                                      |                                      |
> +--------------------------------------+--------------------------------------+
>                                         |
>                     Figure: Firmware First Error Handling approach.
>
> All the hardware error sources are added to HEST table as GHESv2[3] error source
> descriptors. The framework comprises of following DXE and MM drivers:
>
> - HestTableDxe:
>    Builds HEST table header and allows appending error source descriptors to the
>    HEST table. Also provides protocol interface to install the built HEST table.
>
> - HestErrorSourceDxe & HestErrorSourceStandaloneMM:
>    These two drivers together retrieve all possible error source descriptors of
>    type GHESv2 from the MM drivers implementing HEST Error Source Descriptor
>    protocol. Once all the descriptors are collected HestErrorSourceDxe appends
>    it to HEST table using HestTableDxe driver.
>    
> - PlatformErrorHandlerDxe:
>    Builds and installs SDEI ACPI table. This driver does not initialize(load)
>    until HestErrorSourceDxe driver has finished appending all possible GHESv2
>    error source descriptors to the HEST table. Once that is complete using the
>    HestTableDxe driver it installs the HEST table.
>
> This patch series provides reference implementation for DMC-620 Dynamic Memory
> Controller[4] that has RAS feature enabled. This is platform code
> implemented as Standalone MM driver in edk2-platforms.
>
> References:
> [1] : ACPI 6.3, Table 18-382, Hardware Error Source Table
> [2] : SDEI Platform Design Document, revision b, 10 Appendix C, ACPI table
>        definitions for SDEI
> [3] : ACPI Reference Specification 6.3, Table 18-393 GHESv2 Structure
> [4] : DMC620 Dynamic Memory Controller, revision r1p0
> [5] : UEFI Reference Specification 2.8, Appendix N - Common Platform Error
>        Record
> [6] : UEFI Reference Specification 2.8, Section N.2.5 Memory Error Section
>
> Link to github branch with the patches in this series -
> https://github.com/omkkul01/edk2/tree/ras_firmware_first_edk2
>
> Omkar Anand Kulkarni (4):
>    ArmPlatformPkg: Allow dynamic generation of HEST ACPI table
>    ArmPlatformPkg: add definition for MM_HEST_ERROR_SOURCE_DESC_PROTOCOL
>    ArmPlatformPkg: retreive error source descriptors from MM
>    ArmPlatformPkg: Add helpers for HEST table generation
>
>   ArmPlatformPkg/ArmPlatformPkg.dec             |  12 +
>   .../Drivers/Apei/HestDxe/HestDxe.inf          |  49 +++
>   .../HestMmErrorSources/HestErrorSourceDxe.inf |  44 +++
>   .../HestErrorSourceStandaloneMm.inf           |  51 +++
>   .../HestMmErrorSourceCommon.h                 |  37 ++
>   ArmPlatformPkg/Include/HestAcpiHeader.h       |  49 +++
>   .../Include/Protocol/HestErrorSourceInfo.h    |  64 ++++
>   ArmPlatformPkg/Include/Protocol/HestTable.h   |  71 ++++
>   ArmPlatformPkg/Drivers/Apei/HestDxe/HestDxe.c | 354 ++++++++++++++++++
>   .../HestMmErrorSources/HestErrorSourceDxe.c   | 308 +++++++++++++++
>   .../HestErrorSourceStandaloneMm.c             | 312 +++++++++++++++
>   11 files changed, 1351 insertions(+)
>   create mode 100644 ArmPlatformPkg/Drivers/Apei/HestDxe/HestDxe.inf
>   create mode 100644 ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceDxe.inf
>   create mode 100644 ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceStandaloneMm.inf
>   create mode 100644 ArmPlatformPkg/Drivers/HestMmErrorSources/HestMmErrorSourceCommon.h
>   create mode 100644 ArmPlatformPkg/Include/HestAcpiHeader.h
>   create mode 100644 ArmPlatformPkg/Include/Protocol/HestErrorSourceInfo.h
>   create mode 100644 ArmPlatformPkg/Include/Protocol/HestTable.h
>   create mode 100644 ArmPlatformPkg/Drivers/Apei/HestDxe/HestDxe.c
>   create mode 100644 ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceDxe.c
>   create mode 100644 ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceStandaloneMm.c
>

Re: [PATCH v2 0/4] ArmPlatformPkg: Add support to generate HEST ACPI table

[Sami Mujawar]

Hi Omkar,

How did you check that the HEST table is populated correctly?

There is no HEST parser in Acpiview at the moment. Do you plan to add an 
HEST parser to Acpiview?

Regards,

Sami Mujawar


On 02/08/2021 01:49 PM, Sami Mujawar wrote:
> Hi Omkar,
>
> Thank you for this patch series and for the clear explaination below.
>
> The explaination below is very useful for anyone who is trying to 
> understand the code.
>
> Since the cover letter will not be part of the patch commit messages, 
> would it be possible to include this explanation:
>
> 1.  as part of a commit message for one of the patches in this series 
> (patch 2/4 or 3/4).
>
> Or
>
> 2. in a Readme.md file
>
> Regards,
>
> Sami Mujawar
>
>
> On 10/07/2021 05:18 PM, Omkar Anand Kulkarni wrote:
>> Changes since v1:
>> - Helper added for HEST ACPI table generation.
>> - Rebased to the latest upstream code.
>>
>> Hardware Error Source Table (HEST)[1] and Software Delegated 
>> Exception Interface
>> (SDEI)[2] ACPI tables are used to acomplish firmware first error 
>> handling.This
>> patch series introduces a framework to build and install the HEST 
>> ACPI table
>> dynamically.
>>
>> The following figure illustrates the possible usage of the dyanamic
>> generation of HEST ACPI table.
>>
>>                                      NS | S
>> +--------------------------------------+--------------------------------------+ 
>>
>> | |                                      |
>> |+-------------------------------------+---------------------+ |
>> || +---------------------+--------------------+|                |
>> ||               |                     | ||                |
>> || +-----------+ |+------------------+ | +-----------------+|| 
>> +-------------+|
>> || |HestTable  | ||  HestErrorSource | | | HestErrorSource ||| | 
>> DMC-620     ||
>> || |  DXE      | ||        DXE       | | |  StandaloneMM   ||| 
>> |Standalone MM||
>> || +-----------+ |+------------------+ | +-----------------+|| 
>> +-------------+|
>> ||               |GHESv2               | ||                |
>> || +---------------------+--------------------+|                |
>> ||          +--------------------+     | |                |
>> ||          |PlatformErrorHandler|     | |                |
>> ||          |        DXE         |     | |                |
>> ||          +--------------------+     | |                |
>> ||FF FWK                               | |                |
>> |+-------------------------------------+---------------------+ |
>> | |                                      |
>> +--------------------------------------+--------------------------------------+ 
>>
>>                                         |
>>                     Figure: Firmware First Error Handling approach.
>>
>> All the hardware error sources are added to HEST table as GHESv2[3] 
>> error source
>> descriptors. The framework comprises of following DXE and MM drivers:
>>
>> - HestTableDxe:
>>    Builds HEST table header and allows appending error source 
>> descriptors to the
>>    HEST table. Also provides protocol interface to install the built 
>> HEST table.
>>
>> - HestErrorSourceDxe & HestErrorSourceStandaloneMM:
>>    These two drivers together retrieve all possible error source 
>> descriptors of
>>    type GHESv2 from the MM drivers implementing HEST Error Source 
>> Descriptor
>>    protocol. Once all the descriptors are collected 
>> HestErrorSourceDxe appends
>>    it to HEST table using HestTableDxe driver.
>>    - PlatformErrorHandlerDxe:
>>    Builds and installs SDEI ACPI table. This driver does not 
>> initialize(load)
>>    until HestErrorSourceDxe driver has finished appending all 
>> possible GHESv2
>>    error source descriptors to the HEST table. Once that is complete 
>> using the
>>    HestTableDxe driver it installs the HEST table.
>>
>> This patch series provides reference implementation for DMC-620 
>> Dynamic Memory
>> Controller[4] that has RAS feature enabled. This is platform code
>> implemented as Standalone MM driver in edk2-platforms.
>>
>> References:
>> [1] : ACPI 6.3, Table 18-382, Hardware Error Source Table
>> [2] : SDEI Platform Design Document, revision b, 10 Appendix C, ACPI 
>> table
>>        definitions for SDEI
>> [3] : ACPI Reference Specification 6.3, Table 18-393 GHESv2 Structure
>> [4] : DMC620 Dynamic Memory Controller, revision r1p0
>> [5] : UEFI Reference Specification 2.8, Appendix N - Common Platform 
>> Error
>>        Record
>> [6] : UEFI Reference Specification 2.8, Section N.2.5 Memory Error 
>> Section
>>
>> Link to github branch with the patches in this series -
>> https://github.com/omkkul01/edk2/tree/ras_firmware_first_edk2
>>
>> Omkar Anand Kulkarni (4):
>>    ArmPlatformPkg: Allow dynamic generation of HEST ACPI table
>>    ArmPlatformPkg: add definition for MM_HEST_ERROR_SOURCE_DESC_PROTOCOL
>>    ArmPlatformPkg: retreive error source descriptors from MM
>>    ArmPlatformPkg: Add helpers for HEST table generation
>>
>>   ArmPlatformPkg/ArmPlatformPkg.dec             |  12 +
>>   .../Drivers/Apei/HestDxe/HestDxe.inf          |  49 +++
>>   .../HestMmErrorSources/HestErrorSourceDxe.inf |  44 +++
>>   .../HestErrorSourceStandaloneMm.inf           |  51 +++
>>   .../HestMmErrorSourceCommon.h                 |  37 ++
>>   ArmPlatformPkg/Include/HestAcpiHeader.h       |  49 +++
>>   .../Include/Protocol/HestErrorSourceInfo.h    |  64 ++++
>>   ArmPlatformPkg/Include/Protocol/HestTable.h   |  71 ++++
>>   ArmPlatformPkg/Drivers/Apei/HestDxe/HestDxe.c | 354 ++++++++++++++++++
>>   .../HestMmErrorSources/HestErrorSourceDxe.c   | 308 +++++++++++++++
>>   .../HestErrorSourceStandaloneMm.c             | 312 +++++++++++++++
>>   11 files changed, 1351 insertions(+)
>>   create mode 100644 ArmPlatformPkg/Drivers/Apei/HestDxe/HestDxe.inf
>>   create mode 100644 
>> ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceDxe.inf
>>   create mode 100644 
>> ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceStandaloneMm.inf
>>   create mode 100644 
>> ArmPlatformPkg/Drivers/HestMmErrorSources/HestMmErrorSourceCommon.h
>>   create mode 100644 ArmPlatformPkg/Include/HestAcpiHeader.h
>>   create mode 100644 
>> ArmPlatformPkg/Include/Protocol/HestErrorSourceInfo.h
>>   create mode 100644 ArmPlatformPkg/Include/Protocol/HestTable.h
>>   create mode 100644 ArmPlatformPkg/Drivers/Apei/HestDxe/HestDxe.c
>>   create mode 100644 
>> ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceDxe.c
>>   create mode 100644 
>> ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceStandaloneMm.c
>>
>

Re: [PATCH v2 0/4] ArmPlatformPkg: Add support to generate HEST ACPI table

[Omkar Anand Kulkarni]

Hi Sami,

Thanks for reviewing this patch series. Please find my response inline.

Regards,
Omkar

> Hi Omkar,
> 
> How did you check that the HEST table is populated correctly?
> 
> There is no HEST parser in Acpiview at the moment. Do you plan to add an
> HEST parser to Acpiview?
> 
> Regards,
> 
> Sami Mujawar
> 

Yes, we will follow up on the HEST parser patch and post it soon.

- Omkar

> 
> On 02/08/2021 01:49 PM, Sami Mujawar wrote:
> > Hi Omkar,
> >
> > Thank you for this patch series and for the clear explaination below.
> >
> > The explaination below is very useful for anyone who is trying to
> > understand the code.
> >
> > Since the cover letter will not be part of the patch commit messages,
> > would it be possible to include this explanation:
> >
> > 1.  as part of a commit message for one of the patches in this series
> > (patch 2/4 or 3/4).
> >
> > Or
> >
> > 2. in a Readme.md file
> >
> > Regards,
> >
> > Sami Mujawar
> >

Ack.

- Omkar

> >
> > On 10/07/2021 05:18 PM, Omkar Anand Kulkarni wrote:
> >> Changes since v1:
> >> - Helper added for HEST ACPI table generation.
> >> - Rebased to the latest upstream code.
> >>
> >> Hardware Error Source Table (HEST)[1] and Software Delegated
> >> Exception Interface (SDEI)[2] ACPI tables are used to acomplish
> >> firmware first error handling.This patch series introduces a
> >> framework to build and install the HEST ACPI table dynamically.
> >>
> >> The following figure illustrates the possible usage of the dyanamic
> >> generation of HEST ACPI table.
> >>
> >>                                      NS | S
> >> +--------------------------------------+--------------------------------------+
> >>
> >> | |                                      |
> >> |+-------------------------------------+---------------------+ |
> >> || +---------------------+--------------------+|                |
> >> ||               |                     | ||                |
> >> || +-----------+ |+------------------+ | +-----------------+||
> >> +-------------+|
> >> || |HestTable  | ||  HestErrorSource | | | HestErrorSource ||| |
> >> DMC-620     ||
> >> || |  DXE      | ||        DXE       | | |  StandaloneMM   |||
> >> |Standalone MM||
> >> || +-----------+ |+------------------+ | +-----------------+||
> >> +-------------+|
> >> ||               |GHESv2               | ||                |
> >> || +---------------------+--------------------+|                |
> >> ||          +--------------------+     | |                |
> >> ||          |PlatformErrorHandler|     | |                |
> >> ||          |        DXE         |     | |                |
> >> ||          +--------------------+     | |                |
> >> ||FF FWK                               | |                |
> >> |+-------------------------------------+---------------------+ |
> >> | |                                      |
> >> +--------------------------------------+--------------------------------------+
> >>
> >>                                         |
> >>                     Figure: Firmware First Error Handling approach.
> >>
> >> All the hardware error sources are added to HEST table as GHESv2[3]
> >> error source descriptors. The framework comprises of following DXE
> >> and MM drivers:
> >>
> >> - HestTableDxe:
> >>    Builds HEST table header and allows appending error source
> >> descriptors to the
> >>    HEST table. Also provides protocol interface to install the built
> >> HEST table.
> >>
> >> - HestErrorSourceDxe & HestErrorSourceStandaloneMM:
> >>    These two drivers together retrieve all possible error source
> >> descriptors of
> >>    type GHESv2 from the MM drivers implementing HEST Error Source
> >> Descriptor
> >>    protocol. Once all the descriptors are collected
> >> HestErrorSourceDxe appends
> >>    it to HEST table using HestTableDxe driver.
> >>    - PlatformErrorHandlerDxe:
> >>    Builds and installs SDEI ACPI table. This driver does not
> >> initialize(load)
> >>    until HestErrorSourceDxe driver has finished appending all
> >> possible GHESv2
> >>    error source descriptors to the HEST table. Once that is complete
> >> using the
> >>    HestTableDxe driver it installs the HEST table.
> >>
> >> This patch series provides reference implementation for DMC-620
> >> Dynamic Memory Controller[4] that has RAS feature enabled. This is
> >> platform code implemented as Standalone MM driver in edk2-platforms.
> >>
> >> References:
> >> [1] : ACPI 6.3, Table 18-382, Hardware Error Source Table [2] : SDEI
> >> Platform Design Document, revision b, 10 Appendix C, ACPI table
> >>        definitions for SDEI
> >> [3] : ACPI Reference Specification 6.3, Table 18-393 GHESv2 Structure
> >> [4] : DMC620 Dynamic Memory Controller, revision r1p0 [5] : UEFI
> >> Reference Specification 2.8, Appendix N - Common Platform Error
> >>        Record
> >> [6] : UEFI Reference Specification 2.8, Section N.2.5 Memory Error
> >> Section
> >>
> >> Link to github branch with the patches in this series -
> >> https://github.com/omkkul01/edk2/tree/ras_firmware_first_edk2
> >>
> >> Omkar Anand Kulkarni (4):
> >>    ArmPlatformPkg: Allow dynamic generation of HEST ACPI table
> >>    ArmPlatformPkg: add definition for
> MM_HEST_ERROR_SOURCE_DESC_PROTOCOL
> >>    ArmPlatformPkg: retreive error source descriptors from MM
> >>    ArmPlatformPkg: Add helpers for HEST table generation
> >>
> >>   ArmPlatformPkg/ArmPlatformPkg.dec             |  12 +
> >>   .../Drivers/Apei/HestDxe/HestDxe.inf          |  49 +++
> >>   .../HestMmErrorSources/HestErrorSourceDxe.inf |  44 +++
> >>   .../HestErrorSourceStandaloneMm.inf           |  51 +++
> >>   .../HestMmErrorSourceCommon.h                 |  37 ++
> >>   ArmPlatformPkg/Include/HestAcpiHeader.h       |  49 +++
> >>   .../Include/Protocol/HestErrorSourceInfo.h    |  64 ++++
> >>   ArmPlatformPkg/Include/Protocol/HestTable.h   |  71 ++++
> >>   ArmPlatformPkg/Drivers/Apei/HestDxe/HestDxe.c | 354
> ++++++++++++++++++
> >>   .../HestMmErrorSources/HestErrorSourceDxe.c   | 308 +++++++++++++++
> >>   .../HestErrorSourceStandaloneMm.c             | 312 +++++++++++++++
> >>   11 files changed, 1351 insertions(+)
> >>   create mode 100644 ArmPlatformPkg/Drivers/Apei/HestDxe/HestDxe.inf
> >>   create mode 100644
> >> ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceDxe.inf
> >>   create mode 100644
> >>
> ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceStandalone
> Mm.inf
> >>   create mode 100644
> >>
> ArmPlatformPkg/Drivers/HestMmErrorSources/HestMmErrorSourceCommo
> n.h
> >>   create mode 100644 ArmPlatformPkg/Include/HestAcpiHeader.h
> >>   create mode 100644
> >> ArmPlatformPkg/Include/Protocol/HestErrorSourceInfo.h
> >>   create mode 100644 ArmPlatformPkg/Include/Protocol/HestTable.h
> >>   create mode 100644 ArmPlatformPkg/Drivers/Apei/HestDxe/HestDxe.c
> >>   create mode 100644
> >> ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceDxe.c
> >>   create mode 100644
> >>
> ArmPlatformPkg/Drivers/HestMmErrorSources/HestErrorSourceStandalone
> Mm
> >> .c
> >>
> >