Re: [PATCH v4 1/2] ShellPkg/AcpiView: Adds ACPI_PARSER bitfield parser

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

[-- Attachment #1: Type: text/plain, Size: 12815 bytes --]

Hi Abdul,

Thank you for providing a patch to add this feature to Acpiview.

I have some minor feedback marked inline as [SAMI].

Regards,

Sami Mujawar


On 19/12/2021 02:44 PM, Abdul Lateef Attar wrote:
> Adds ParseAcpiBitFields() which is based on
> ParseAcpi() and capable of parsing the bit fields.
> Supports parsing of UINT8, UINT16, UINT32 and UINT64 byte data.
>
> 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/AcpiParser.h |  45 +++++
>   ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c | 185 ++++++++++++++++++++
>   2 files changed, 230 insertions(+)
>
> diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
> index 5c916a4720b8..83266e8ec2d3 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
>
>   **/
>
>   
>
> @@ -251,6 +252,11 @@ typedef VOID (EFIAPI *FNPTR_FIELD_VALIDATOR)(UINT8 *Ptr, VOID *Context);
>     the field data. If the field is more complex and requires additional
>
>     processing for formatting and representation a print formatter function
>
>     can be specified in 'PrintFormatter'.
>
> +
>
> +  ParseAcpiBitFields() uses AcpiParser structure to parse the bit fields.
>
> +  It considers Length as a number of bits that need to be parsed.
>
> +  Also, the Offset field will be considered as starting offset of the bitfield.
>
> +
>
>     The PrintFormatter function may choose to use the format string
>
>     specified by 'Format' or use its own internal format string.
>
>   
>
> @@ -264,10 +270,12 @@ typedef struct AcpiParser {
>   
>
>     /// The length of the field.
>
>     /// (Byte Length column from ACPI table spec)
>
> +  /// Length(in bits) of the bitfield if used with ParseAcpiBitFields().
>
>     UINT32                   Length;
>
>   
>
>     /// The offset of the field from the start of the table.
>
>     /// (Byte Offset column from ACPI table spec)
>
> +  /// The Bit offset of the field if used with ParseAcpiBitFields().
>
>     UINT32                   Offset;
>
>   
>
>     /// Optional Print() style format string for tracing the data. If not
>
> @@ -364,6 +372,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 cb193a5ea449..94ee26f42ab9 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"
>
> @@ -752,3 +754,186 @@ 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)) {
>
> +    IncrementErrorCount ();
>
> +    Print (
>
> +      L"\nERROR: Bitfield Length(%d) is zero or exceeding the 64 bit 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;
[SAMI] Is there a use-case where the bitfield length will be zero? I 
think any unused bits would be represented as Reserved. Considering 
this, Parser[Index].Length == 0 would mean an incorrect an incorrect 
description inACPI_PARSER, right?
If so, the default case for the switch statement below would print 
"ERROR: %a: CANNOT PARSE THIS FIELD, Field Length". Can you check this, 
please?
[/SAMI]
> +    }
>
> +
>
> +    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);
>
> +
>
> +    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 + 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;
[SAMI] ACPI_PARSER.ItemPtr is used to get a reference to the field data 
in the original ACPI table data.
In the current case, Parser[Index].ItemPtr is being set to a local 
variable (i.e. Data). I don't think this is what we want. I think it 
would be better to not supportACPI_PARSER.ItemPtr for BitFields.
I would recommend adding a comment to clarify that ItemPtr is not 
supported for BitFields in this function, as well as the documentation 
for ACPI_PARSER structure.
[/SAMI]
>
> +    }
>
> +
>
> +    Offset += Parser[Index].Length;
>
> +  } // for
>
> +
>
> +  // Decrement the Indent
>
> +  gIndent -= Indent;
>
> +  return Offset;
>
> +}
>


[-- Attachment #2: Type: text/html, Size: 13333 bytes --]