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

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

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_ */