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 > Cc: Zhichao Gao > Cc: Sami Mujawar > Signed-off-by: Abdul Lateef Attar > --- > 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 > > #include > > #include > > +#include > > #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; > > +} >