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

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

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

Hi Abdul,

Please also create a bugzilla ticket and add a reference to it in the 
commit message.

Regards,

Sami Mujawar


On 19/01/2022 04:44 PM, Sami Mujawar wrote:
>
> 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: 13916 bytes --]