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

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

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
> >>
> >