Hi Abdul,

Thank you for implementing the suggestion.

Pleas find my feedback inline marked [SAMI].

Regards,

Sami Mujawar


On 17/11/2021 12:29 PM, Abdul Lateef Attar wrote:
Adds PrintFormatter function to the FADT flags field.
Prints indivisual flag name along with flag value.
Adds new parser function ParseAcpiBitFields() to handle
bit fields.

Cc: Ray Ni <ray...@intel.com>
Cc: Zhichao Gao <zhichao....@intel.com>
Cc: Sami Mujawar <sami.muja...@arm.com>
Signed-off-by: Abdul Lateef Attar <abdat...@amd.com>
---
  ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h              |  38 
+++++
  ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c              | 177 
++++++++++++++++++++
  ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c |  59 
++++++-
  3 files changed, 273 insertions(+), 1 deletion(-)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h 
b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
index 5e34a70c8bae..444a977aced8 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
@@ -2,6 +2,7 @@
    Header file for ACPI parser

    Copyright (c) 2016 - 2020, Arm Limited. All rights reserved.

+  Copyright (c) 2021, AMD Incorporated. All rights reserved.

    SPDX-License-Identifier: BSD-2-Clause-Patent

  **/

@@ -365,6 +366,43 @@ ParseAcpi (
    IN UINT32             ParserItems

    );

+/**

+  This function is used to parse an ACPI table bitfield buffer.

+

+  The ACPI table buffer is parsed using the ACPI table parser information

+  specified by a pointer to an array of ACPI_PARSER elements. This parser

+  function iterates through each item on the ACPI_PARSER array and logs the 
ACPI table bitfields.

+

+  This function can optionally be used to parse ACPI tables and fetch specific

+  field values. The ItemPtr member of the ACPI_PARSER structure (where used)

+  is updated by this parser function to point to the selected field data

+  (e.g. useful for variable length nested fields).

+

+  @param [in] Trace        Trace the ACPI fields TRUE else only parse the

+                           table.

+  @param [in] Indent       Number of spaces to indent the output.

+  @param [in] AsciiName    Optional pointer to an ASCII string that describes

+                           the table being parsed.

+  @param [in] Ptr          Pointer to the start of the buffer.

+  @param [in] Length       Length of the buffer pointed by Ptr.

+  @param [in] Parser       Pointer to an array of ACPI_PARSER structure that

+                           describes the table being parsed.

+  @param [in] ParserItems  Number of items in the ACPI_PARSER array.

+

+  @retval Number of bits parsed.

+**/

+UINT32

+EFIAPI

+ParseAcpiBitFields (

+  IN BOOLEAN            Trace,

+  IN UINT32             Indent,

+  IN CONST CHAR8*       AsciiName OPTIONAL,

+  IN UINT8*             Ptr,

+  IN UINT32             Length,

+  IN CONST ACPI_PARSER* Parser,

+  IN UINT32             ParserItems

+);

+

  /**

     This is a helper macro to pass parameters to the Parser functions.

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c 
b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
index 74056e72c359..33a9e3d1e97a 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
@@ -2,12 +2,14 @@
    ACPI parser

    Copyright (c) 2016 - 2021, Arm Limited. All rights reserved.

+  Copyright (c) 2021, AMD Incorporated. All rights reserved.

    SPDX-License-Identifier: BSD-2-Clause-Patent

  **/

  #include <Uefi.h>

  #include <Library/UefiLib.h>

  #include <Library/UefiBootServicesTableLib.h>

+#include <Library/BaseMemoryLib.h>

  #include "AcpiParser.h"

  #include "AcpiView.h"

  #include "AcpiViewConfig.h"

@@ -738,3 +740,178 @@ ParseAcpiHeader (
    return BytesParsed;

  }

+

+/**

+  This function is used to parse an ACPI table bitfield buffer.

+

+  The ACPI table buffer is parsed using the ACPI table parser information

+  specified by a pointer to an array of ACPI_PARSER elements. This parser

+  function iterates through each item on the ACPI_PARSER array and logs the 
ACPI table bitfields.

+

+  This function can optionally be used to parse ACPI tables and fetch specific

+  field values. The ItemPtr member of the ACPI_PARSER structure (where used)

+  is updated by this parser function to point to the selected field data

+  (e.g. useful for variable length nested fields).

+

+  @param [in] Trace        Trace the ACPI fields TRUE else only parse the

+                           table.

+  @param [in] Indent       Number of spaces to indent the output.

+  @param [in] AsciiName    Optional pointer to an ASCII string that describes

+                           the table being parsed.

+  @param [in] Ptr          Pointer to the start of the buffer.

+  @param [in] Length       Length in bytes of the buffer pointed by Ptr.

+  @param [in] Parser       Pointer to an array of ACPI_PARSER structure that

+                           describes the table being parsed.

+  @param [in] ParserItems  Number of items in the ACPI_PARSER array.

+

+  @retval Number of bits parsed.

+**/

+UINT32

+EFIAPI

+ParseAcpiBitFields (

+  IN BOOLEAN            Trace,

+  IN UINT32             Indent,

+  IN CONST CHAR8*       AsciiName OPTIONAL,

+  IN UINT8*             Ptr,

+  IN UINT32             Length,

+  IN CONST ACPI_PARSER* Parser,

+  IN UINT32             ParserItems

+)

+{

+  UINT32  Index;

+  UINT32  Offset;

+  BOOLEAN HighLight;

+  UINTN   OriginalAttribute;

+

+  UINT64 Data;

+  UINT64 BitsData;

+

+  if (Length <= 0 || Length > 8) {
[SAMI] Minor, since Lenght is UINT32 this check can be changed to 'if (Length == 0 || Length > 8) {'. Similarly the error log message below can be modified to say that the length is zero or exceeding the 64 bits limit.

+    IncrementErrorCount ();

+    Print (

+        L"\nERROR: Bitfield Length(%d) is exceeding the 64bits limit \n",

+        Length

+        );

+    return 0;

+  }

+  //

+  // set local variables to suppress incorrect compiler/analyzer warnings

+  //

+  OriginalAttribute = 0;

+  Offset = 0;

+

+  // Increment the Indent

+  gIndent += Indent;

+

+  CopyMem ((VOID *)&BitsData, (VOID *)Ptr, Length);

+  if (Trace && (AsciiName != NULL)){

+    HighLight = GetColourHighlighting ();

+

+    if (HighLight) {

+      OriginalAttribute = gST->ConOut->Mode->Attribute;

+      gST->ConOut->SetAttribute (

+                     gST->ConOut,

+                     EFI_TEXT_ATTR(EFI_YELLOW,

+                       ((OriginalAttribute&(BIT4|BIT5|BIT6))>>4))

+                     );

+    }

+    Print (

+      L"%*a%-*a :\n",

+      gIndent,

+      "",

+      (OUTPUT_FIELD_COLUMN_WIDTH - gIndent),

+      AsciiName

+      );

+    if (HighLight) {

+      gST->ConOut->SetAttribute (gST->ConOut, OriginalAttribute);

+    }

+  }

+

+  for (Index = 0; Index < ParserItems; Index++) {

+    if ((Offset + Parser[Index].Length) > (Length * 8)) {

+      // For fields outside the buffer length provided, reset any pointers

+      // which were supposed to be updated by this function call

+      if (Parser[Index].ItemPtr != NULL) {

+        *Parser[Index].ItemPtr = NULL;

+      }

+

+      // We don't parse past the end of the max length specified

+      continue;

+    }

+

+    if (Parser[Index].Length == 0) {

+      // don't parse the bitfield whose length is zero

+      continue;

+    }

+

+    if (GetConsistencyChecking () &&

+        (Offset != Parser[Index].Offset)) {

+      IncrementErrorCount ();

+      Print (

+        L"\nERROR: %a: Offset Mismatch for %s\n"

+          L"CurrentOffset = %d FieldOffset = %d\n",

+        AsciiName,

+        Parser[Index].NameStr,

+        Offset,

+        Parser[Index].Offset

+        );

+    }

+

+    // extract Bitfield data for the current item

+    Data = (BitsData >> Parser[Index].Offset) & ~(~0ULL << 
Parser[Index].Length);
[SAMI] Is it possible to update the documentation for ACPI_PARSER structure fields to relect that the Offset and Length fields have a modified meaning when used with ParseAcpiBitFields(), please?

+

+    if (Trace) {

+      // if there is a Formatter function let the function handle

+      // the printing else if a Format is specified in the table use

+      // the Format for printing

+      PrintFieldName (2, Parser[Index].NameStr);

+      if (Parser[Index].PrintFormatter != NULL) {

+        Parser[Index].PrintFormatter (Parser[Index].Format, (UINT8 *)&Data);

+      } else if (Parser[Index].Format != NULL) {

+        // convert bit length to byte length

+        switch ((Parser[Index].Length / 8) + (Parser[Index].Length % 8)) {
[SAMI] Should this be 'switch ((Parser[Index].Length + 7) >> 3) {'?

+          // print the data depends on byte size

+          case 1:

+            DumpUint8 (Parser[Index].Format, (UINT8 *)&Data);

+            break;

+          case 2:

+            DumpUint16 (Parser[Index].Format, (UINT8 *)&Data);

+            break;

+          case 3:

+          case 4:

+            DumpUint32 (Parser[Index].Format, (UINT8 *)&Data);

+            break;

+          case 5:

+          case 6:

+          case 7:

+          case 8:

+            DumpUint64 (Parser[Index].Format, (UINT8 *)&Data);

+            break;

+          default:

+            Print (

+              L"\nERROR: %a: CANNOT PARSE THIS FIELD, Field Length = %d\n",

+              AsciiName,

+              Parser[Index].Length

+              );

+        } // switch

+      }

+      // Validating only makes sense if we are tracing

+      // the parsed table entries, to report by table name.

+      if (GetConsistencyChecking () &&

+          (Parser[Index].FieldValidator != NULL)) {

+        Parser[Index].FieldValidator ((UINT8 *)&Data, Parser[Index].Context);

+      }

+      Print (L"\n");

+    } // if (Trace)

+

+    if (Parser[Index].ItemPtr != NULL) {

+      *Parser[Index].ItemPtr = (VOID*)(UINT8 *)&Data;

+    }

+

+    Offset += Parser[Index].Length;

+  } // for

+

+  // Decrement the Indent

+  gIndent -= Indent;

+  return Offset;

+}

diff --git 
a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c 
b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c
index d86718bab67d..3b59864d2c7e 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c
@@ -2,6 +2,7 @@
    FADT table parser
[SAMI] I think the FADT change can be a separate patch.
    Copyright (c) 2016 - 2020, ARM Limited. All rights reserved.

+  Copyright (c) 2021, AMD Incorporated. All rights reserved.

    SPDX-License-Identifier: BSD-2-Clause-Patent

    @par Reference(s):

@@ -127,6 +128,62 @@ ValidateFlags (
  #endif

  }

+STATIC CONST ACPI_PARSER FadtFlagParser [] = {

+  {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},

+  {L"P_LVL2_UP", 1, 3, L"%d", NULL, NULL, NULL, NULL},

+  {L"PWR_BUTTON", 1, 4, L"%d", NULL, NULL, NULL, NULL},

+  {L"SLP_BUTTON", 1, 5, L"%d", NULL, NULL, NULL, NULL},

+  {L"FIX_RTC", 1, 6, L"%d", NULL, NULL, NULL, NULL},

+  {L"RTC_S4", 1, 7, L"%d", NULL, NULL, NULL, NULL},

+  {L"TMR_VAL_EXT", 1, 8, L"%d", NULL, NULL, NULL, NULL},

+  {L"DCK_CAP", 1, 9, L"%d", NULL, NULL, NULL, NULL},

+  {L"RESET_REG_SUP", 1, 10, L"%d", NULL, NULL, NULL, NULL},

+  {L"SEALED_CASE", 1, 11, L"%d", NULL, NULL, NULL, NULL},

+  {L"HEADLESS", 1, 12, L"%d", NULL, NULL, NULL, NULL},

+  {L"CPU_SW_SLP", 1, 13, L"%d", NULL, NULL, NULL, NULL},

+  {L"PCI_EXP_WAK", 1, 14, L"%d", NULL, NULL, NULL, NULL},

+  {L"USE_PLATFORM_CLOCK", 1, 15, L"%d", NULL, NULL, NULL, NULL},

+  {L"S4_RTC_STS_VALID", 1, 16, L"%d", NULL, NULL, NULL, NULL},

+  {L"REMOTE_POWER_ON_CAPABLE", 1, 17, L"%d", NULL, NULL, NULL, NULL},

+  {L"FORCE_APIC_CLUSTER_MODEL", 1, 18, L"%d", NULL, NULL, NULL, NULL},

+  {L"FORCE_APIC_PHYSICAL_DESTINATION_MODE", 1, 19, L"%d", NULL, NULL, NULL, 
NULL},

+  {L"HW_REDUCED_ACPI", 1, 20, L"%d", NULL, NULL, NULL, NULL},

+  {L"LOW_POWER_S0_IDLE_CAPABLE", 1, 21, L"%d", NULL, NULL, NULL, NULL},

+  {L"Reserved", 10, 22, L"%d", NULL, NULL, NULL, NULL}

+};

+

+/**

+  This function traces FADT Flags fields.

+  If no format string is specified the Format must be NULL.

+

+  @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);

+  ParseAcpiBitFields (

+    TRUE,

+    2,

+    NULL,

+    Ptr,

+    4,

+    PARSER_PARAMS (FadtFlagParser)

+    );

+}

+

  /**

    An ACPI_PARSER array describing the ACPI FADT Table.

  **/

@@ -170,7 +227,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},




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#83852): https://edk2.groups.io/g/devel/message/83852
Mute This Topic: https://groups.io/mt/87117964/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-


Reply via email to