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 >> 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; >> >> +} >> >