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

["Sami Mujawar" <sami.mujawar@arm.com>]

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