Hi Abdul,

Thank you for contributing this patch.

Is it possible to introduce a new functionParseAcpiFieldBits () that can 
trace the bit fields, please? It may also be possible to reuse the 
ACPI_PARSER structure to specify the Field bits.

e.g. Keeping the meaning of the existing ACPI_PARSER field the same, the 
meaning of a few fields can be extended to also include the following.

i.e. when ACPI_PARSER is used to describe a Bit Field, then the 
following would apply.

ACPI_PARSER.Length => Length of the field bits in bits.

ACPI_PARSER.Offset => Field bit start position.

With this the FADT Flag bits can be described in an ACPI_PARSER array 
(similar to how the ACPI table fields are described).

e.g.

STATIC CONST ACPI_PARSER FadtFlagBitsParser[]= {
   {L"WBINVD", 1, 0, L"%d", NULL, NULL, NULL, NULL},
   {L"WBINVD_FLUSH", 1, 1, L"%d", NULL, NULL, NULL, NULL},
   {L"PROC_C1", 1, 2,L"%d", NULL, NULL, NULL, NULL},
...
};

The DumpFadtFlags() function could call ParseAcpiFieldBits 
(...,PARSER_PARAMS (FadtFlagBitsParser)).

ParseAcpiFieldBits() could then extract (mask and shift) the bits and 
print using the print formatter.

I think introducing ParseAcpiFieldBits() would allow the possibility of 
reusing the code as well as adding validations for the field bits.

Please do let me know if you have any queries and I would be happy to help.

Regards,

Sami Mujawar


On 12/11/2021 01:27 PM, Abdul Lateef Attar wrote:
> Adds PrintFormatter function to the FADT flags field.
> Prints indivisual flag name along with flag value.
>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Zhichao Gao <zhichao.gao@intel.com>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> Signed-off-by: Abdul Lateef Attar <abdattar@amd.com>
> ---
>   ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c | 208 +++++++++++++++++++-
>   1 file changed, 207 insertions(+), 1 deletion(-)
>
> diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c
> index d86718bab67d..7f01412f1a9b 100644
> --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c
> @@ -2,6 +2,7 @@
>     FADT table parser
>
>   
>
>     Copyright (c) 2016 - 2020, ARM Limited. All rights reserved.
>
> +  Copyright (c) 2021, AMD Incorporated. All rights reserved.<BR>
>
>     SPDX-License-Identifier: BSD-2-Clause-Patent
>
>   
>
>     @par Reference(s):
>
> @@ -127,6 +128,211 @@ ValidateFlags (
>   #endif
>
>   }
>
>   
>
> +/**
>
> +  This function traces FADT Flags field.
>
> +
>
> +  @param [in] Format  Optional format string for tracing the data.
>
> +  @param [in] Ptr     Pointer to the start of the buffer.
>
> +**/
>
> +VOID
>
> +EFIAPI
>
> +DumpFadtFlags (
>
> +  IN CONST CHAR16* Format OPTIONAL,
>
> +  IN UINT8*        Ptr
>
> +  )
>
> +{
>
> +  if (Format != NULL) {
>
> +    Print (Format, *(UINT32*)Ptr);
>
> +    return;
>
> +  }
>
> +
>
> +  Print (L"0x%X\n", *(UINT32*)Ptr);
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +     L"WBINVD",
>
> +    *(UINT32*)Ptr & BIT0 ? 1 : 0
>
> +    );
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +    L"WBINVD_FLUSH",
>
> +    *(UINT32*)Ptr & BIT1 ? 1 : 0
>
> +    );
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +    L"PROC_C1",
>
> +    *(UINT32*)Ptr & BIT2 ? 1 : 0
>
> +    );
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +    L"P_LVL2_UP",
>
> +    *(UINT32*)Ptr & BIT3 ? 1 : 0
>
> +    );
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +    L"PWR_BUTTON",
>
> +    *(UINT32*)Ptr & BIT4 ? 1 : 0
>
> +    );
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +    L"SLP_BUTTON",
>
> +    *(UINT32*)Ptr & BIT5 ? 1 : 0
>
> +   );
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +    L"FIX_RTC",
>
> +    *(UINT32*)Ptr & BIT6 ? 1 : 0
>
> +   );
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +    L"RTC_S4",
>
> +    *(UINT32*)Ptr & BIT7 ? 1 : 0
>
> +   );
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +    L"TMR_VAL_EXT",
>
> +    *(UINT32*)Ptr & BIT8 ? 1 : 0
>
> +    );
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +    L"DCK_CAP",
>
> +    *(UINT32*)Ptr & BIT9 ? 1 : 0
>
> +    );
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +    L"RESET_REG_SUP",
>
> +    *(UINT32*)Ptr & BIT10 ? 1 : 0
>
> +    );
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +    L"SEALED_CASE",
>
> +    *(UINT32*)Ptr & BIT11 ? 1 : 0
>
> +    );
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +    L"HEADLESS",
>
> +    *(UINT32*)Ptr & BIT12 ? 1 : 0
>
> +    );
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +    L"CPU_SW_SLP",
>
> +    *(UINT32*)Ptr & BIT13 ? 1 : 0
>
> +    );
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +    L"PCI_EXP_WAK",
>
> +    *(UINT32*)Ptr & BIT14 ? 1 : 0
>
> +    );
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +    L"USE_PLATFORM_CLOCK",
>
> +    *(UINT32*)Ptr & BIT15 ? 1 : 0
>
> +    );
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +    L"S4_RTC_STS_VALID",
>
> +    *(UINT32*)Ptr & BIT16 ? 1 : 0
>
> +    );
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +    L"REMOTE_POWER_ON_CAPABLE",
>
> +    *(UINT32*)Ptr & BIT17 ? 1 : 0
>
> +    );
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +    L"FORCE_APIC_CLUSTER_MODEL",
>
> +    *(UINT32*)Ptr & BIT18 ? 1 : 0
>
> +    );
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +    L"FORCE_APIC_PHYSICAL_DESTINATION_MODE",
>
> +    *(UINT32*)Ptr & BIT19 ? 1 : 0
>
> +    );
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +    L"HW_REDUCED_ACPI",
>
> +    *(UINT32*)Ptr & BIT20 ? 1 : 0
>
> +    );
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +    L"LOW_POWER_S0_IDLE_CAPABLE",
>
> +    *(UINT32*)Ptr & BIT21 ? 1 : 0
>
> +    );
>
> +  Print (
>
> +    L"%*a%-*s : %d\n",
>
> +    4,
>
> +    "",
>
> +    (OUTPUT_FIELD_COLUMN_WIDTH - 4),
>
> +    L"Reserved",
>
> +    *(UINT32*)Ptr & BIT22 ? 1 : 0
>
> +    );
>
> +}
>
> +
>
>   /**
>
>     An ACPI_PARSER array describing the ACPI FADT Table.
>
>   **/
>
> @@ -170,7 +376,7 @@ STATIC CONST ACPI_PARSER FadtParser[] = {
>     {L"CENTURY", 1, 108, L"0x%x", NULL, NULL, NULL, NULL},
>
>     {L"IAPC_BOOT_ARCH", 2, 109, L"0x%x", NULL, NULL, NULL, NULL},
>
>     {L"Reserved", 1, 111, L"0x%x", NULL, NULL, NULL, NULL},
>
> -  {L"Flags", 4, 112, L"0x%x", NULL, (VOID**)&Flags, ValidateFlags, NULL},
>
> +  {L"Flags", 4, 112, NULL, DumpFadtFlags, (VOID**)&Flags, ValidateFlags, NULL},
>
>     {L"RESET_REG", 12, 116, NULL, DumpGas, NULL, NULL, NULL},
>
>     {L"RESET_VALUE", 1, 128, L"0x%x", NULL, NULL, NULL, NULL},
>
>     {L"ARM_BOOT_ARCH", 2, 129, L"0x%x", NULL, NULL, NULL, NULL},
>