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

["Omkar Anand Kulkarni" <omkar.kulkarni@arm.com>]

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

+}