* [PATCH v4 1/2] ShellPkg/AcpiView: Adds ACPI_PARSER bitfield parser [not found] <20211219144437.3721-1-abdattar@amd.com> @ 2021-12-19 14:44 ` Abdul Lateef Attar 2022-01-19 16:44 ` Sami Mujawar 2021-12-19 14:44 ` [PATCH v4 2/2] ShellPkg/AcpiView: PrintFormatter for FADT Flags field Abdul Lateef Attar [not found] ` <16C22F2D003754C4.24651@groups.io> 2 siblings, 1 reply; 12+ messages in thread From: Abdul Lateef Attar @ 2021-12-19 14:44 UTC (permalink / raw) To: devel; +Cc: Ray Ni, Zhichao Gao, Sami Mujawar 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; + } + + 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; + } + + Offset += Parser[Index].Length; + } // for + + // Decrement the Indent + gIndent -= Indent; + return Offset; +} -- 2.25.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v4 1/2] ShellPkg/AcpiView: Adds ACPI_PARSER bitfield parser 2021-12-19 14:44 ` [PATCH v4 1/2] ShellPkg/AcpiView: Adds ACPI_PARSER bitfield parser Abdul Lateef Attar @ 2022-01-19 16:44 ` Sami Mujawar 2022-01-19 16:46 ` Sami Mujawar 0 siblings, 1 reply; 12+ messages in thread From: Sami Mujawar @ 2022-01-19 16:44 UTC (permalink / raw) To: Abdul Lateef Attar, devel; +Cc: Ray Ni, Zhichao Gao, nd [-- 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 --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 1/2] ShellPkg/AcpiView: Adds ACPI_PARSER bitfield parser 2022-01-19 16:44 ` Sami Mujawar @ 2022-01-19 16:46 ` Sami Mujawar 2022-01-20 3:24 ` [edk2-devel] " Attar, AbdulLateef (Abdul Lateef) 0 siblings, 1 reply; 12+ messages in thread From: Sami Mujawar @ 2022-01-19 16:46 UTC (permalink / raw) To: Abdul Lateef Attar, devel; +Cc: Ray Ni, Zhichao Gao, nd [-- 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 --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH v4 1/2] ShellPkg/AcpiView: Adds ACPI_PARSER bitfield parser 2022-01-19 16:46 ` Sami Mujawar @ 2022-01-20 3:24 ` Attar, AbdulLateef (Abdul Lateef) 2022-01-20 8:36 ` Gao, Zhichao 0 siblings, 1 reply; 12+ messages in thread From: Attar, AbdulLateef (Abdul Lateef) @ 2022-01-20 3:24 UTC (permalink / raw) To: devel@edk2.groups.io, sami.mujawar@arm.com; +Cc: Ray Ni, Zhichao Gao, nd [-- Attachment #1: Type: text/plain, Size: 14622 bytes --] [Public] Hi Sami, Why Bugzilla ticket required? Also please see inline for my response [Abdul]. Thanks AbduL From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sami Mujawar via groups.io Sent: 19 January 2022 22:17 To: Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@amd.com>; devel@edk2.groups.io Cc: Ray Ni <ray.ni@intel.com>; Zhichao Gao <zhichao.gao@intel.com>; nd <nd@arm.com> Subject: Re: [edk2-devel] [PATCH v4 1/2] ShellPkg/AcpiView: Adds ACPI_PARSER bitfield parser [CAUTION: External Email] 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><mailto:ray.ni@intel.com> Cc: Zhichao Gao <zhichao.gao@intel.com><mailto:zhichao.gao@intel.com> Cc: Sami Mujawar <sami.mujawar@arm.com><mailto:sami.mujawar@arm.com> Signed-off-by: Abdul Lateef Attar <abdattar@amd.com><mailto: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 in ACPI_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] [Abdul] Reserved bits should be having some length, 1 to 31, it cannot be 0bit length. I tested with FADT flags field which is having Reserved bits. [/Abdul] + } + + 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 support ACPI_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: 38682 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH v4 1/2] ShellPkg/AcpiView: Adds ACPI_PARSER bitfield parser 2022-01-20 3:24 ` [edk2-devel] " Attar, AbdulLateef (Abdul Lateef) @ 2022-01-20 8:36 ` Gao, Zhichao 2022-01-20 12:31 ` Sami Mujawar 0 siblings, 1 reply; 12+ messages in thread From: Gao, Zhichao @ 2022-01-20 8:36 UTC (permalink / raw) To: Attar, AbdulLateef (Abdul Lateef), devel@edk2.groups.io, sami.mujawar@arm.com Cc: Ni, Ray, nd [-- Attachment #1: Type: text/plain, Size: 15895 bytes --] Hi, Sorry for the late response. Many works interrupt my review plan. The BZ creation and update is part of edk2 develop process EDK II Development Process * tianocore/tianocore.github.io Wiki * GitHub<https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process>. It give a good history of the code change and it can be traced by the edk2 stable release page. Back to the patch itself. Because it is a reuse the structure of the ACPI_PARSER, I think it is fine to take Length check into consideration for security thinking. For the ItemPtr, it is used to be used outside the parser. So the patch looks fine to me. Anything I miss consideration, please feel free to point out. Thanks, Zhichao From: Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@amd.com> Sent: Thursday, January 20, 2022 11:25 AM To: devel@edk2.groups.io; sami.mujawar@arm.com Cc: Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; nd <nd@arm.com> Subject: RE: [edk2-devel] [PATCH v4 1/2] ShellPkg/AcpiView: Adds ACPI_PARSER bitfield parser [Public] Hi Sami, Why Bugzilla ticket required? Also please see inline for my response [Abdul]. Thanks AbduL From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Sami Mujawar via groups.io Sent: 19 January 2022 22:17 To: Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@amd.com<mailto:AbdulLateef.Attar@amd.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io> Cc: Ray Ni <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Zhichao Gao <zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>>; nd <nd@arm.com<mailto:nd@arm.com>> Subject: Re: [edk2-devel] [PATCH v4 1/2] ShellPkg/AcpiView: Adds ACPI_PARSER bitfield parser [CAUTION: External Email] 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><mailto:ray.ni@intel.com> Cc: Zhichao Gao <zhichao.gao@intel.com><mailto:zhichao.gao@intel.com> Cc: Sami Mujawar <sami.mujawar@arm.com><mailto:sami.mujawar@arm.com> Signed-off-by: Abdul Lateef Attar <abdattar@amd.com><mailto: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 in ACPI_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] [Abdul] Reserved bits should be having some length, 1 to 31, it cannot be 0bit length. I tested with FADT flags field which is having Reserved bits. [/Abdul] + } + + 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 support ACPI_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: 41912 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH v4 1/2] ShellPkg/AcpiView: Adds ACPI_PARSER bitfield parser 2022-01-20 8:36 ` Gao, Zhichao @ 2022-01-20 12:31 ` Sami Mujawar 2022-01-21 7:29 ` Gao, Zhichao 0 siblings, 1 reply; 12+ messages in thread From: Sami Mujawar @ 2022-01-20 12:31 UTC (permalink / raw) To: Gao, Zhichao, Attar, AbdulLateef (Abdul Lateef), devel@edk2.groups.io Cc: Ni, Ray, nd [-- Attachment #1: Type: text/plain, Size: 17918 bytes --] Hi Zhichao, Abdul, Please find my response inline marked [SAMI]. Regards, Sami Mujawar From: "Gao, Zhichao" <zhichao.gao@intel.com> Date: Thursday, 20 January 2022 at 08:37 To: "Attar, AbdulLateef (Abdul Lateef)" <AbdulLateef.Attar@amd.com>, "devel@edk2.groups.io" <devel@edk2.groups.io>, Sami Mujawar <Sami.Mujawar@arm.com> Cc: "Ni, Ray" <ray.ni@intel.com>, nd <nd@arm.com> Subject: RE: [edk2-devel] [PATCH v4 1/2] ShellPkg/AcpiView: Adds ACPI_PARSER bitfield parser Hi, Sorry for the late response. Many works interrupt my review plan. The BZ creation and update is part of edk2 develop process EDK II Development Process · tianocore/tianocore.github.io Wiki · GitHub<https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process>. It give a good history of the code change and it can be traced by the edk2 stable release page. Back to the patch itself. Because it is a reuse the structure of the ACPI_PARSER, I think it is fine to take Length check into consideration for security thinking. [SAMI] I agree that the Length check is required from a security perspective. If Parser[Index].Length is 0, this means the parser has not been described correctly, and logging an error message would be helpful (which I think would happen as part of the default case in the Switch statement). I am fine with keeping this check (as done in the patch) but would recommend logging an error message before continuing. [/SAMI] For the ItemPtr, it is used to be used outside the parser. So the patch looks fine to me. [SAMI] The ItemPtr is designed to get a reference to the field data in the ACPI table memory area. This allows the ItemPtr to be used outside the function (e.g. to examine the field). This patch is setting up the ItemPtr to point to a local variable, which is not right and would result in accessing an invalid memory area. To give an example on how ItemPtr is designed to work see: * Declaration of ItemPtr for Flags field at https://github.com/tianocore/edk2/blob/master/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c#L20 * Initialisation of FadtParser with ItemPtr for Flags field at https://github.com/tianocore/edk2/blob/master/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c#L176 * Invocation of ParseAcpi() for FadtParser at https://github.com/tianocore/edk2/blob/master/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c#L226-L233 * Usage of ItemPtr for Flags field at https://github.com/tianocore/edk2/blob/master/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c#L260-L261 [/SAMI] Anything I miss consideration, please feel free to point out. Thanks, Zhichao From: Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@amd.com> Sent: Thursday, January 20, 2022 11:25 AM To: devel@edk2.groups.io; sami.mujawar@arm.com Cc: Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; nd <nd@arm.com> Subject: RE: [edk2-devel] [PATCH v4 1/2] ShellPkg/AcpiView: Adds ACPI_PARSER bitfield parser [Public] Hi Sami, Why Bugzilla ticket required? Also please see inline for my response [Abdul]. Thanks AbduL From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Sami Mujawar via groups.io Sent: 19 January 2022 22:17 To: Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@amd.com<mailto:AbdulLateef.Attar@amd.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io> Cc: Ray Ni <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Zhichao Gao <zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>>; nd <nd@arm.com<mailto:nd@arm.com>> Subject: Re: [edk2-devel] [PATCH v4 1/2] ShellPkg/AcpiView: Adds ACPI_PARSER bitfield parser [CAUTION: External Email] 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><mailto:ray.ni@intel.com> Cc: Zhichao Gao <zhichao.gao@intel.com><mailto:zhichao.gao@intel.com> Cc: Sami Mujawar <sami.mujawar@arm.com><mailto:sami.mujawar@arm.com> Signed-off-by: Abdul Lateef Attar <abdattar@amd.com><mailto: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 in ACPI_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] [Abdul] Reserved bits should be having some length, 1 to 31, it cannot be 0bit length. I tested with FADT flags field which is having Reserved bits. [/Abdul] + } + + 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 support ACPI_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: 66135 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH v4 1/2] ShellPkg/AcpiView: Adds ACPI_PARSER bitfield parser 2022-01-20 12:31 ` Sami Mujawar @ 2022-01-21 7:29 ` Gao, Zhichao 0 siblings, 0 replies; 12+ messages in thread From: Gao, Zhichao @ 2022-01-21 7:29 UTC (permalink / raw) To: Sami Mujawar, Attar, AbdulLateef (Abdul Lateef), devel@edk2.groups.io Cc: Ni, Ray, nd [-- Attachment #1: Type: text/plain, Size: 18998 bytes --] Hi Sami, Thanks for the clarification. You’re right. #1 it is better to add error message before skip this item. #2 I miss the variable check. Local variable would be release when exit the function. And there is no way to make the ItemPtr to point to the specific bit field. Abdul, can you address the issue that Sami point out? Thanks, Zhichao From: Sami Mujawar <Sami.Mujawar@arm.com> Sent: Thursday, January 20, 2022 8:32 PM To: Gao, Zhichao <zhichao.gao@intel.com>; Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@amd.com>; devel@edk2.groups.io Cc: Ni, Ray <ray.ni@intel.com>; nd <nd@arm.com> Subject: Re: [edk2-devel] [PATCH v4 1/2] ShellPkg/AcpiView: Adds ACPI_PARSER bitfield parser Hi Zhichao, Abdul, Please find my response inline marked [SAMI]. Regards, Sami Mujawar From: "Gao, Zhichao" <zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>> Date: Thursday, 20 January 2022 at 08:37 To: "Attar, AbdulLateef (Abdul Lateef)" <AbdulLateef.Attar@amd.com<mailto:AbdulLateef.Attar@amd.com>>, "devel@edk2.groups.io<mailto:devel@edk2.groups.io>" <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>, Sami Mujawar <Sami.Mujawar@arm.com<mailto:Sami.Mujawar@arm.com>> Cc: "Ni, Ray" <ray.ni@intel.com<mailto:ray.ni@intel.com>>, nd <nd@arm.com<mailto:nd@arm.com>> Subject: RE: [edk2-devel] [PATCH v4 1/2] ShellPkg/AcpiView: Adds ACPI_PARSER bitfield parser Hi, Sorry for the late response. Many works interrupt my review plan. The BZ creation and update is part of edk2 develop process EDK II Development Process · tianocore/tianocore.github.io Wiki · GitHub<https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process>. It give a good history of the code change and it can be traced by the edk2 stable release page. Back to the patch itself. Because it is a reuse the structure of the ACPI_PARSER, I think it is fine to take Length check into consideration for security thinking. [SAMI] I agree that the Length check is required from a security perspective. If Parser[Index].Length is 0, this means the parser has not been described correctly, and logging an error message would be helpful (which I think would happen as part of the default case in the Switch statement). I am fine with keeping this check (as done in the patch) but would recommend logging an error message before continuing. [/SAMI] For the ItemPtr, it is used to be used outside the parser. So the patch looks fine to me. [SAMI] The ItemPtr is designed to get a reference to the field data in the ACPI table memory area. This allows the ItemPtr to be used outside the function (e.g. to examine the field). This patch is setting up the ItemPtr to point to a local variable, which is not right and would result in accessing an invalid memory area. To give an example on how ItemPtr is designed to work see: * Declaration of ItemPtr for Flags field at https://github.com/tianocore/edk2/blob/master/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c#L20 * Initialisation of FadtParser with ItemPtr for Flags field at https://github.com/tianocore/edk2/blob/master/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c#L176 * Invocation of ParseAcpi() for FadtParser at https://github.com/tianocore/edk2/blob/master/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c#L226-L233 * Usage of ItemPtr for Flags field at https://github.com/tianocore/edk2/blob/master/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c#L260-L261 [/SAMI] Anything I miss consideration, please feel free to point out. Thanks, Zhichao From: Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@amd.com<mailto:AbdulLateef.Attar@amd.com>> Sent: Thursday, January 20, 2022 11:25 AM To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; sami.mujawar@arm.com<mailto:sami.mujawar@arm.com> Cc: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Gao, Zhichao <zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>>; nd <nd@arm.com<mailto:nd@arm.com>> Subject: RE: [edk2-devel] [PATCH v4 1/2] ShellPkg/AcpiView: Adds ACPI_PARSER bitfield parser [Public] Hi Sami, Why Bugzilla ticket required? Also please see inline for my response [Abdul]. Thanks AbduL From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Sami Mujawar via groups.io Sent: 19 January 2022 22:17 To: Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@amd.com<mailto:AbdulLateef.Attar@amd.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io> Cc: Ray Ni <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Zhichao Gao <zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>>; nd <nd@arm.com<mailto:nd@arm.com>> Subject: Re: [edk2-devel] [PATCH v4 1/2] ShellPkg/AcpiView: Adds ACPI_PARSER bitfield parser [CAUTION: External Email] 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><mailto:ray.ni@intel.com> Cc: Zhichao Gao <zhichao.gao@intel.com><mailto:zhichao.gao@intel.com> Cc: Sami Mujawar <sami.mujawar@arm.com><mailto:sami.mujawar@arm.com> Signed-off-by: Abdul Lateef Attar <abdattar@amd.com><mailto: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 in ACPI_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] [Abdul] Reserved bits should be having some length, 1 to 31, it cannot be 0bit length. I tested with FADT flags field which is having Reserved bits. [/Abdul] + } + + 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 support ACPI_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: 53972 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v4 2/2] ShellPkg/AcpiView: PrintFormatter for FADT Flags field [not found] <20211219144437.3721-1-abdattar@amd.com> 2021-12-19 14:44 ` [PATCH v4 1/2] ShellPkg/AcpiView: Adds ACPI_PARSER bitfield parser Abdul Lateef Attar @ 2021-12-19 14:44 ` Abdul Lateef Attar 2022-01-19 16:44 ` Sami Mujawar [not found] ` <16C22F2D003754C4.24651@groups.io> 2 siblings, 1 reply; 12+ messages in thread From: Abdul Lateef Attar @ 2021-12-19 14:44 UTC (permalink / raw) To: devel; +Cc: Ray Ni, Zhichao Gao, Sami Mujawar Adds PrintFormatter function to the FADT flags field. Prints indivisual flag name along with flag value. 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/Parsers/Fadt/FadtParser.c | 167 +++++++++++++------- 1 file changed, 112 insertions(+), 55 deletions(-) diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c index f8fbb4bcb8e9..5ff02f3d38a1 100644 --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c @@ -2,6 +2,7 @@ FADT table 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 @par Reference(s): @@ -130,69 +131,125 @@ 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. **/ STATIC CONST ACPI_PARSER FadtParser[] = { PARSE_ACPI_HEADER (&AcpiHdrInfo), - { L"FIRMWARE_CTRL", 4, 36, L"0x%x", NULL, (VOID **)&FirmwareCtrl, + { L"FIRMWARE_CTRL", 4, 36, L"0x%x", NULL, (VOID **)&FirmwareCtrl, ValidateFirmwareCtrl, NULL }, - { L"DSDT", 4, 40, L"0x%x", NULL, (VOID **)&DsdtAddress, NULL, NULL }, - { L"Reserved", 1, 44, L"%x", NULL, NULL, NULL, NULL }, - { L"Preferred_PM_Profile", 1, 45, L"0x%x", NULL, NULL, NULL, NULL }, - { L"SCI_INT", 2, 46, L"0x%x", NULL, NULL, NULL, NULL }, - { L"SMI_CMD", 4, 48, L"0x%x", NULL, NULL, NULL, NULL }, - { L"ACPI_ENABLE", 1, 52, L"0x%x", NULL, NULL, NULL, NULL }, - { L"ACPI_DISABLE", 1, 53, L"0x%x", NULL, NULL, NULL, NULL }, - { L"S4BIOS_REQ", 1, 54, L"0x%x", NULL, NULL, NULL, NULL }, - { L"PSTATE_CNT", 1, 55, L"0x%x", NULL, NULL, NULL, NULL }, - { L"PM1a_EVT_BLK", 4, 56, L"0x%x", NULL, NULL, NULL, NULL }, - { L"PM1b_EVT_BLK", 4, 60, L"0x%x", NULL, NULL, NULL, NULL }, - { L"PM1a_CNT_BLK", 4, 64, L"0x%x", NULL, NULL, NULL, NULL }, - { L"PM1b_CNT_BLK", 4, 68, L"0x%x", NULL, NULL, NULL, NULL }, - { L"PM2_CNT_BLK", 4, 72, L"0x%x", NULL, NULL, NULL, NULL }, - { L"PM_TMR_BLK", 4, 76, L"0x%x", NULL, NULL, NULL, NULL }, - { L"GPE0_BLK", 4, 80, L"0x%x", NULL, NULL, NULL, NULL }, - { L"GPE1_BLK", 4, 84, L"0x%x", NULL, NULL, NULL, NULL }, - { L"PM1_EVT_LEN", 1, 88, L"0x%x", NULL, NULL, NULL, NULL }, - { L"PM1_CNT_LEN", 1, 89, L"0x%x", NULL, NULL, NULL, NULL }, - { L"PM2_CNT_LEN", 1, 90, L"0x%x", NULL, NULL, NULL, NULL }, - { L"PM_TMR_LEN", 1, 91, L"0x%x", NULL, NULL, NULL, NULL }, - { L"GPE0_BLK_LEN", 1, 92, L"0x%x", NULL, NULL, NULL, NULL }, - { L"GPE1_BLK_LEN", 1, 93, L"0x%x", NULL, NULL, NULL, NULL }, - { L"GPE1_BASE", 1, 94, L"0x%x", NULL, NULL, NULL, NULL }, - { L"CST_CNT", 1, 95, L"0x%x", NULL, NULL, NULL, NULL }, - { L"P_LVL2_LAT", 2, 96, L"0x%x", NULL, NULL, NULL, NULL }, - { L"P_LVL3_LAT", 2, 98, L"0x%x", NULL, NULL, NULL, NULL }, - { L"FLUSH_SIZE", 2, 100, L"0x%x", NULL, NULL, NULL, NULL }, - { L"FLUSH_STRIDE", 2, 102, L"0x%x", NULL, NULL, NULL, NULL }, - { L"DUTY_OFFSET", 1, 104, L"0x%x", NULL, NULL, NULL, NULL }, - { L"DUTY_WIDTH", 1, 105, L"0x%x", NULL, NULL, NULL, NULL }, - { L"DAY_ALRM", 1, 106, L"0x%x", NULL, NULL, NULL, NULL }, - { L"MON_ALRM", 1, 107, L"0x%x", NULL, NULL, NULL, NULL }, - { 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"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 }, - { L"FADT Minor Version", 1, 131, L"0x%x", NULL, (VOID **)&FadtMinorRevision, + { L"DSDT", 4, 40, L"0x%x", NULL, (VOID **)&DsdtAddress, NULL, NULL }, + { L"Reserved", 1, 44, L"%x", NULL, NULL, NULL, NULL }, + { L"Preferred_PM_Profile", 1, 45, L"0x%x", NULL, NULL, NULL, NULL }, + { L"SCI_INT", 2, 46, L"0x%x", NULL, NULL, NULL, NULL }, + { L"SMI_CMD", 4, 48, L"0x%x", NULL, NULL, NULL, NULL }, + { L"ACPI_ENABLE", 1, 52, L"0x%x", NULL, NULL, NULL, NULL }, + { L"ACPI_DISABLE", 1, 53, L"0x%x", NULL, NULL, NULL, NULL }, + { L"S4BIOS_REQ", 1, 54, L"0x%x", NULL, NULL, NULL, NULL }, + { L"PSTATE_CNT", 1, 55, L"0x%x", NULL, NULL, NULL, NULL }, + { L"PM1a_EVT_BLK", 4, 56, L"0x%x", NULL, NULL, NULL, NULL }, + { L"PM1b_EVT_BLK", 4, 60, L"0x%x", NULL, NULL, NULL, NULL }, + { L"PM1a_CNT_BLK", 4, 64, L"0x%x", NULL, NULL, NULL, NULL }, + { L"PM1b_CNT_BLK", 4, 68, L"0x%x", NULL, NULL, NULL, NULL }, + { L"PM2_CNT_BLK", 4, 72, L"0x%x", NULL, NULL, NULL, NULL }, + { L"PM_TMR_BLK", 4, 76, L"0x%x", NULL, NULL, NULL, NULL }, + { L"GPE0_BLK", 4, 80, L"0x%x", NULL, NULL, NULL, NULL }, + { L"GPE1_BLK", 4, 84, L"0x%x", NULL, NULL, NULL, NULL }, + { L"PM1_EVT_LEN", 1, 88, L"0x%x", NULL, NULL, NULL, NULL }, + { L"PM1_CNT_LEN", 1, 89, L"0x%x", NULL, NULL, NULL, NULL }, + { L"PM2_CNT_LEN", 1, 90, L"0x%x", NULL, NULL, NULL, NULL }, + { L"PM_TMR_LEN", 1, 91, L"0x%x", NULL, NULL, NULL, NULL }, + { L"GPE0_BLK_LEN", 1, 92, L"0x%x", NULL, NULL, NULL, NULL }, + { L"GPE1_BLK_LEN", 1, 93, L"0x%x", NULL, NULL, NULL, NULL }, + { L"GPE1_BASE", 1, 94, L"0x%x", NULL, NULL, NULL, NULL }, + { L"CST_CNT", 1, 95, L"0x%x", NULL, NULL, NULL, NULL }, + { L"P_LVL2_LAT", 2, 96, L"0x%x", NULL, NULL, NULL, NULL }, + { L"P_LVL3_LAT", 2, 98, L"0x%x", NULL, NULL, NULL, NULL }, + { L"FLUSH_SIZE", 2, 100, L"0x%x", NULL, NULL, NULL, NULL }, + { L"FLUSH_STRIDE", 2, 102, L"0x%x", NULL, NULL, NULL, NULL }, + { L"DUTY_OFFSET", 1, 104, L"0x%x", NULL, NULL, NULL, NULL }, + { L"DUTY_WIDTH", 1, 105, L"0x%x", NULL, NULL, NULL, NULL }, + { L"DAY_ALRM", 1, 106, L"0x%x", NULL, NULL, NULL, NULL }, + { L"MON_ALRM", 1, 107, L"0x%x", NULL, NULL, NULL, NULL }, + { 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, 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 }, + { L"FADT Minor Version", 1, 131, L"0x%x", NULL, (VOID **)&FadtMinorRevision, NULL, NULL }, - { L"X_FIRMWARE_CTRL", 8, 132, L"0x%lx", NULL, (VOID **)&X_FirmwareCtrl, + { L"X_FIRMWARE_CTRL", 8, 132, L"0x%lx", NULL, (VOID **)&X_FirmwareCtrl, ValidateXFirmwareCtrl, NULL }, - { L"X_DSDT", 8, 140, L"0x%lx", NULL, (VOID **)&X_DsdtAddress, NULL, NULL }, - { L"X_PM1a_EVT_BLK", 12, 148, NULL, DumpGas, NULL, NULL, NULL }, - { L"X_PM1b_EVT_BLK", 12, 160, NULL, DumpGas, NULL, NULL, NULL }, - { L"X_PM1a_CNT_BLK", 12, 172, NULL, DumpGas, NULL, NULL, NULL }, - { L"X_PM1b_CNT_BLK", 12, 184, NULL, DumpGas, NULL, NULL, NULL }, - { L"X_PM2_CNT_BLK", 12, 196, NULL, DumpGas, NULL, NULL, NULL }, - { L"X_PM_TMR_BLK", 12, 208, NULL, DumpGas, NULL, NULL, NULL }, - { L"X_GPE0_BLK", 12, 220, NULL, DumpGas, NULL, NULL, NULL }, - { L"X_GPE1_BLK", 12, 232, NULL, DumpGas, NULL, NULL, NULL }, - { L"SLEEP_CONTROL_REG", 12, 244, NULL, DumpGas, NULL, NULL, NULL }, - { L"SLEEP_STATUS_REG", 12, 256, NULL, DumpGas, NULL, NULL, NULL }, - { L"Hypervisor VendorIdentity", 8, 268, L"%lx", NULL, NULL, NULL, NULL } + { L"X_DSDT", 8, 140, L"0x%lx", NULL, (VOID **)&X_DsdtAddress, NULL, NULL }, + { L"X_PM1a_EVT_BLK", 12, 148, NULL, DumpGas, NULL, NULL, NULL }, + { L"X_PM1b_EVT_BLK", 12, 160, NULL, DumpGas, NULL, NULL, NULL }, + { L"X_PM1a_CNT_BLK", 12, 172, NULL, DumpGas, NULL, NULL, NULL }, + { L"X_PM1b_CNT_BLK", 12, 184, NULL, DumpGas, NULL, NULL, NULL }, + { L"X_PM2_CNT_BLK", 12, 196, NULL, DumpGas, NULL, NULL, NULL }, + { L"X_PM_TMR_BLK", 12, 208, NULL, DumpGas, NULL, NULL, NULL }, + { L"X_GPE0_BLK", 12, 220, NULL, DumpGas, NULL, NULL, NULL }, + { L"X_GPE1_BLK", 12, 232, NULL, DumpGas, NULL, NULL, NULL }, + { L"SLEEP_CONTROL_REG", 12, 244, NULL, DumpGas, NULL, NULL, NULL }, + { L"SLEEP_STATUS_REG", 12, 256, NULL, DumpGas, NULL, NULL, NULL }, + { L"Hypervisor VendorIdentity", 8, 268, L"%lx", NULL, NULL, NULL, NULL } }; /** -- 2.25.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v4 2/2] ShellPkg/AcpiView: PrintFormatter for FADT Flags field 2021-12-19 14:44 ` [PATCH v4 2/2] ShellPkg/AcpiView: PrintFormatter for FADT Flags field Abdul Lateef Attar @ 2022-01-19 16:44 ` Sami Mujawar 2022-01-20 8:39 ` [edk2-devel] " Gao, Zhichao 0 siblings, 1 reply; 12+ messages in thread From: Sami Mujawar @ 2022-01-19 16:44 UTC (permalink / raw) To: Abdul Lateef Attar, devel; +Cc: Ray Ni, Zhichao Gao, nd Hi Abdul, Thank you for this patch. These changes look good to me. Reviewed-by: Sami Mujawar <sami.mujawar@arm.com> Regards, Sami Mujawar On 19/12/2021 02:44 PM, Abdul Lateef Attar wrote: > Adds PrintFormatter function to the FADT flags field. > Prints indivisual flag name along with flag value. > > 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/Parsers/Fadt/FadtParser.c | 167 +++++++++++++------- > 1 file changed, 112 insertions(+), 55 deletions(-) > > diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c > index f8fbb4bcb8e9..5ff02f3d38a1 100644 > --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c > @@ -2,6 +2,7 @@ > FADT table 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 > > > > @par Reference(s): > > @@ -130,69 +131,125 @@ 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. > > **/ > > STATIC CONST ACPI_PARSER FadtParser[] = { > > PARSE_ACPI_HEADER (&AcpiHdrInfo), > > - { L"FIRMWARE_CTRL", 4, 36, L"0x%x", NULL, (VOID **)&FirmwareCtrl, > > + { L"FIRMWARE_CTRL", 4, 36, L"0x%x", NULL, (VOID **)&FirmwareCtrl, > > ValidateFirmwareCtrl, NULL }, > > - { L"DSDT", 4, 40, L"0x%x", NULL, (VOID **)&DsdtAddress, NULL, NULL }, > > - { L"Reserved", 1, 44, L"%x", NULL, NULL, NULL, NULL }, > > - { L"Preferred_PM_Profile", 1, 45, L"0x%x", NULL, NULL, NULL, NULL }, > > - { L"SCI_INT", 2, 46, L"0x%x", NULL, NULL, NULL, NULL }, > > - { L"SMI_CMD", 4, 48, L"0x%x", NULL, NULL, NULL, NULL }, > > - { L"ACPI_ENABLE", 1, 52, L"0x%x", NULL, NULL, NULL, NULL }, > > - { L"ACPI_DISABLE", 1, 53, L"0x%x", NULL, NULL, NULL, NULL }, > > - { L"S4BIOS_REQ", 1, 54, L"0x%x", NULL, NULL, NULL, NULL }, > > - { L"PSTATE_CNT", 1, 55, L"0x%x", NULL, NULL, NULL, NULL }, > > - { L"PM1a_EVT_BLK", 4, 56, L"0x%x", NULL, NULL, NULL, NULL }, > > - { L"PM1b_EVT_BLK", 4, 60, L"0x%x", NULL, NULL, NULL, NULL }, > > - { L"PM1a_CNT_BLK", 4, 64, L"0x%x", NULL, NULL, NULL, NULL }, > > - { L"PM1b_CNT_BLK", 4, 68, L"0x%x", NULL, NULL, NULL, NULL }, > > - { L"PM2_CNT_BLK", 4, 72, L"0x%x", NULL, NULL, NULL, NULL }, > > - { L"PM_TMR_BLK", 4, 76, L"0x%x", NULL, NULL, NULL, NULL }, > > - { L"GPE0_BLK", 4, 80, L"0x%x", NULL, NULL, NULL, NULL }, > > - { L"GPE1_BLK", 4, 84, L"0x%x", NULL, NULL, NULL, NULL }, > > - { L"PM1_EVT_LEN", 1, 88, L"0x%x", NULL, NULL, NULL, NULL }, > > - { L"PM1_CNT_LEN", 1, 89, L"0x%x", NULL, NULL, NULL, NULL }, > > - { L"PM2_CNT_LEN", 1, 90, L"0x%x", NULL, NULL, NULL, NULL }, > > - { L"PM_TMR_LEN", 1, 91, L"0x%x", NULL, NULL, NULL, NULL }, > > - { L"GPE0_BLK_LEN", 1, 92, L"0x%x", NULL, NULL, NULL, NULL }, > > - { L"GPE1_BLK_LEN", 1, 93, L"0x%x", NULL, NULL, NULL, NULL }, > > - { L"GPE1_BASE", 1, 94, L"0x%x", NULL, NULL, NULL, NULL }, > > - { L"CST_CNT", 1, 95, L"0x%x", NULL, NULL, NULL, NULL }, > > - { L"P_LVL2_LAT", 2, 96, L"0x%x", NULL, NULL, NULL, NULL }, > > - { L"P_LVL3_LAT", 2, 98, L"0x%x", NULL, NULL, NULL, NULL }, > > - { L"FLUSH_SIZE", 2, 100, L"0x%x", NULL, NULL, NULL, NULL }, > > - { L"FLUSH_STRIDE", 2, 102, L"0x%x", NULL, NULL, NULL, NULL }, > > - { L"DUTY_OFFSET", 1, 104, L"0x%x", NULL, NULL, NULL, NULL }, > > - { L"DUTY_WIDTH", 1, 105, L"0x%x", NULL, NULL, NULL, NULL }, > > - { L"DAY_ALRM", 1, 106, L"0x%x", NULL, NULL, NULL, NULL }, > > - { L"MON_ALRM", 1, 107, L"0x%x", NULL, NULL, NULL, NULL }, > > - { 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"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 }, > > - { L"FADT Minor Version", 1, 131, L"0x%x", NULL, (VOID **)&FadtMinorRevision, > > + { L"DSDT", 4, 40, L"0x%x", NULL, (VOID **)&DsdtAddress, NULL, NULL }, > > + { L"Reserved", 1, 44, L"%x", NULL, NULL, NULL, NULL }, > > + { L"Preferred_PM_Profile", 1, 45, L"0x%x", NULL, NULL, NULL, NULL }, > > + { L"SCI_INT", 2, 46, L"0x%x", NULL, NULL, NULL, NULL }, > > + { L"SMI_CMD", 4, 48, L"0x%x", NULL, NULL, NULL, NULL }, > > + { L"ACPI_ENABLE", 1, 52, L"0x%x", NULL, NULL, NULL, NULL }, > > + { L"ACPI_DISABLE", 1, 53, L"0x%x", NULL, NULL, NULL, NULL }, > > + { L"S4BIOS_REQ", 1, 54, L"0x%x", NULL, NULL, NULL, NULL }, > > + { L"PSTATE_CNT", 1, 55, L"0x%x", NULL, NULL, NULL, NULL }, > > + { L"PM1a_EVT_BLK", 4, 56, L"0x%x", NULL, NULL, NULL, NULL }, > > + { L"PM1b_EVT_BLK", 4, 60, L"0x%x", NULL, NULL, NULL, NULL }, > > + { L"PM1a_CNT_BLK", 4, 64, L"0x%x", NULL, NULL, NULL, NULL }, > > + { L"PM1b_CNT_BLK", 4, 68, L"0x%x", NULL, NULL, NULL, NULL }, > > + { L"PM2_CNT_BLK", 4, 72, L"0x%x", NULL, NULL, NULL, NULL }, > > + { L"PM_TMR_BLK", 4, 76, L"0x%x", NULL, NULL, NULL, NULL }, > > + { L"GPE0_BLK", 4, 80, L"0x%x", NULL, NULL, NULL, NULL }, > > + { L"GPE1_BLK", 4, 84, L"0x%x", NULL, NULL, NULL, NULL }, > > + { L"PM1_EVT_LEN", 1, 88, L"0x%x", NULL, NULL, NULL, NULL }, > > + { L"PM1_CNT_LEN", 1, 89, L"0x%x", NULL, NULL, NULL, NULL }, > > + { L"PM2_CNT_LEN", 1, 90, L"0x%x", NULL, NULL, NULL, NULL }, > > + { L"PM_TMR_LEN", 1, 91, L"0x%x", NULL, NULL, NULL, NULL }, > > + { L"GPE0_BLK_LEN", 1, 92, L"0x%x", NULL, NULL, NULL, NULL }, > > + { L"GPE1_BLK_LEN", 1, 93, L"0x%x", NULL, NULL, NULL, NULL }, > > + { L"GPE1_BASE", 1, 94, L"0x%x", NULL, NULL, NULL, NULL }, > > + { L"CST_CNT", 1, 95, L"0x%x", NULL, NULL, NULL, NULL }, > > + { L"P_LVL2_LAT", 2, 96, L"0x%x", NULL, NULL, NULL, NULL }, > > + { L"P_LVL3_LAT", 2, 98, L"0x%x", NULL, NULL, NULL, NULL }, > > + { L"FLUSH_SIZE", 2, 100, L"0x%x", NULL, NULL, NULL, NULL }, > > + { L"FLUSH_STRIDE", 2, 102, L"0x%x", NULL, NULL, NULL, NULL }, > > + { L"DUTY_OFFSET", 1, 104, L"0x%x", NULL, NULL, NULL, NULL }, > > + { L"DUTY_WIDTH", 1, 105, L"0x%x", NULL, NULL, NULL, NULL }, > > + { L"DAY_ALRM", 1, 106, L"0x%x", NULL, NULL, NULL, NULL }, > > + { L"MON_ALRM", 1, 107, L"0x%x", NULL, NULL, NULL, NULL }, > > + { 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, 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 }, > > + { L"FADT Minor Version", 1, 131, L"0x%x", NULL, (VOID **)&FadtMinorRevision, > > NULL, NULL }, > > - { L"X_FIRMWARE_CTRL", 8, 132, L"0x%lx", NULL, (VOID **)&X_FirmwareCtrl, > > + { L"X_FIRMWARE_CTRL", 8, 132, L"0x%lx", NULL, (VOID **)&X_FirmwareCtrl, > > ValidateXFirmwareCtrl, NULL }, > > - { L"X_DSDT", 8, 140, L"0x%lx", NULL, (VOID **)&X_DsdtAddress, NULL, NULL }, > > - { L"X_PM1a_EVT_BLK", 12, 148, NULL, DumpGas, NULL, NULL, NULL }, > > - { L"X_PM1b_EVT_BLK", 12, 160, NULL, DumpGas, NULL, NULL, NULL }, > > - { L"X_PM1a_CNT_BLK", 12, 172, NULL, DumpGas, NULL, NULL, NULL }, > > - { L"X_PM1b_CNT_BLK", 12, 184, NULL, DumpGas, NULL, NULL, NULL }, > > - { L"X_PM2_CNT_BLK", 12, 196, NULL, DumpGas, NULL, NULL, NULL }, > > - { L"X_PM_TMR_BLK", 12, 208, NULL, DumpGas, NULL, NULL, NULL }, > > - { L"X_GPE0_BLK", 12, 220, NULL, DumpGas, NULL, NULL, NULL }, > > - { L"X_GPE1_BLK", 12, 232, NULL, DumpGas, NULL, NULL, NULL }, > > - { L"SLEEP_CONTROL_REG", 12, 244, NULL, DumpGas, NULL, NULL, NULL }, > > - { L"SLEEP_STATUS_REG", 12, 256, NULL, DumpGas, NULL, NULL, NULL }, > > - { L"Hypervisor VendorIdentity", 8, 268, L"%lx", NULL, NULL, NULL, NULL } > > + { L"X_DSDT", 8, 140, L"0x%lx", NULL, (VOID **)&X_DsdtAddress, NULL, NULL }, > > + { L"X_PM1a_EVT_BLK", 12, 148, NULL, DumpGas, NULL, NULL, NULL }, > > + { L"X_PM1b_EVT_BLK", 12, 160, NULL, DumpGas, NULL, NULL, NULL }, > > + { L"X_PM1a_CNT_BLK", 12, 172, NULL, DumpGas, NULL, NULL, NULL }, > > + { L"X_PM1b_CNT_BLK", 12, 184, NULL, DumpGas, NULL, NULL, NULL }, > > + { L"X_PM2_CNT_BLK", 12, 196, NULL, DumpGas, NULL, NULL, NULL }, > > + { L"X_PM_TMR_BLK", 12, 208, NULL, DumpGas, NULL, NULL, NULL }, > > + { L"X_GPE0_BLK", 12, 220, NULL, DumpGas, NULL, NULL, NULL }, > > + { L"X_GPE1_BLK", 12, 232, NULL, DumpGas, NULL, NULL, NULL }, > > + { L"SLEEP_CONTROL_REG", 12, 244, NULL, DumpGas, NULL, NULL, NULL }, > > + { L"SLEEP_STATUS_REG", 12, 256, NULL, DumpGas, NULL, NULL, NULL }, > > + { L"Hypervisor VendorIdentity", 8, 268, L"%lx", NULL, NULL, NULL, NULL } > > }; > > > > /** > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH v4 2/2] ShellPkg/AcpiView: PrintFormatter for FADT Flags field 2022-01-19 16:44 ` Sami Mujawar @ 2022-01-20 8:39 ` Gao, Zhichao 0 siblings, 0 replies; 12+ messages in thread From: Gao, Zhichao @ 2022-01-20 8:39 UTC (permalink / raw) To: devel@edk2.groups.io, sami.mujawar@arm.com, Abdul Lateef Attar Cc: Ni, Ray, nd Reviewed-by: Zhichao Gao <zhichao.gao@intel.com> Thanks, Zhichao > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sami > Mujawar > Sent: Thursday, January 20, 2022 12:45 AM > To: Abdul Lateef Attar <abdattar@amd.com>; devel@edk2.groups.io > Cc: Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; nd > <nd@arm.com> > Subject: Re: [edk2-devel] [PATCH v4 2/2] ShellPkg/AcpiView: PrintFormatter > for FADT Flags field > > Hi Abdul, > > Thank you for this patch. > > These changes look good to me. > > Reviewed-by: Sami Mujawar <sami.mujawar@arm.com> > > Regards, > > Sami Mujawar > > > On 19/12/2021 02:44 PM, Abdul Lateef Attar wrote: > > Adds PrintFormatter function to the FADT flags field. > > Prints indivisual flag name along with flag value. > > > > 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/Parsers/Fadt/FadtParser.c > | 167 +++++++++++++------- > > 1 file changed, 112 insertions(+), 55 deletions(-) > > > > diff --git > > > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser > > .c > > > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser > > .c index f8fbb4bcb8e9..5ff02f3d38a1 100644 > > --- > > > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser > > .c > > +++ > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtPa > > +++ rser.c > > @@ -2,6 +2,7 @@ > > FADT table 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 > > > > > > > > @par Reference(s): > > > > @@ -130,69 +131,125 @@ 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. > > > > **/ > > > > STATIC CONST ACPI_PARSER FadtParser[] = { > > > > PARSE_ACPI_HEADER (&AcpiHdrInfo), > > > > - { L"FIRMWARE_CTRL", 4, 36, L"0x%x", NULL, (VOID > **)&FirmwareCtrl, > > > > + { L"FIRMWARE_CTRL", 4, 36, L"0x%x", NULL, (VOID > **)&FirmwareCtrl, > > > > ValidateFirmwareCtrl, NULL }, > > > > - { L"DSDT", 4, 40, L"0x%x", NULL, (VOID **)&DsdtAddress, > NULL, NULL }, > > > > - { L"Reserved", 1, 44, L"%x", NULL, NULL, NULL, > NULL }, > > > > - { L"Preferred_PM_Profile", 1, 45, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > - { L"SCI_INT", 2, 46, L"0x%x", NULL, NULL, NULL, > NULL }, > > > > - { L"SMI_CMD", 4, 48, L"0x%x", NULL, NULL, NULL, > NULL }, > > > > - { L"ACPI_ENABLE", 1, 52, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > - { L"ACPI_DISABLE", 1, 53, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > - { L"S4BIOS_REQ", 1, 54, L"0x%x", NULL, NULL, NULL, > NULL }, > > > > - { L"PSTATE_CNT", 1, 55, L"0x%x", NULL, NULL, NULL, > NULL }, > > > > - { L"PM1a_EVT_BLK", 4, 56, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > - { L"PM1b_EVT_BLK", 4, 60, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > - { L"PM1a_CNT_BLK", 4, 64, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > - { L"PM1b_CNT_BLK", 4, 68, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > - { L"PM2_CNT_BLK", 4, 72, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > - { L"PM_TMR_BLK", 4, 76, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > - { L"GPE0_BLK", 4, 80, L"0x%x", NULL, NULL, NULL, > NULL }, > > > > - { L"GPE1_BLK", 4, 84, L"0x%x", NULL, NULL, NULL, > NULL }, > > > > - { L"PM1_EVT_LEN", 1, 88, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > - { L"PM1_CNT_LEN", 1, 89, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > - { L"PM2_CNT_LEN", 1, 90, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > - { L"PM_TMR_LEN", 1, 91, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > - { L"GPE0_BLK_LEN", 1, 92, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > - { L"GPE1_BLK_LEN", 1, 93, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > - { L"GPE1_BASE", 1, 94, L"0x%x", NULL, NULL, NULL, > NULL }, > > > > - { L"CST_CNT", 1, 95, L"0x%x", NULL, NULL, NULL, > NULL }, > > > > - { L"P_LVL2_LAT", 2, 96, L"0x%x", NULL, NULL, NULL, > NULL }, > > > > - { L"P_LVL3_LAT", 2, 98, L"0x%x", NULL, NULL, NULL, > NULL }, > > > > - { L"FLUSH_SIZE", 2, 100, L"0x%x", NULL, NULL, NULL, > NULL }, > > > > - { L"FLUSH_STRIDE", 2, 102, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > - { L"DUTY_OFFSET", 1, 104, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > - { L"DUTY_WIDTH", 1, 105, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > - { L"DAY_ALRM", 1, 106, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > - { L"MON_ALRM", 1, 107, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > - { 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"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 }, > > > > - { L"FADT Minor Version", 1, 131, L"0x%x", NULL, (VOID > **)&FadtMinorRevision, > > > > + { L"DSDT", 4, 40, L"0x%x", NULL, (VOID > **)&DsdtAddress, NULL, NULL }, > > > > + { L"Reserved", 1, 44, L"%x", NULL, NULL, NULL, > NULL }, > > > > + { L"Preferred_PM_Profile", 1, 45, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > + { L"SCI_INT", 2, 46, L"0x%x", NULL, NULL, NULL, > NULL }, > > > > + { L"SMI_CMD", 4, 48, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > + { L"ACPI_ENABLE", 1, 52, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > + { L"ACPI_DISABLE", 1, 53, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > + { L"S4BIOS_REQ", 1, 54, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > + { L"PSTATE_CNT", 1, 55, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > + { L"PM1a_EVT_BLK", 4, 56, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > + { L"PM1b_EVT_BLK", 4, 60, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > + { L"PM1a_CNT_BLK", 4, 64, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > + { L"PM1b_CNT_BLK", 4, 68, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > + { L"PM2_CNT_BLK", 4, 72, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > + { L"PM_TMR_BLK", 4, 76, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > + { L"GPE0_BLK", 4, 80, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > + { L"GPE1_BLK", 4, 84, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > + { L"PM1_EVT_LEN", 1, 88, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > + { L"PM1_CNT_LEN", 1, 89, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > + { L"PM2_CNT_LEN", 1, 90, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > + { L"PM_TMR_LEN", 1, 91, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > + { L"GPE0_BLK_LEN", 1, 92, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > + { L"GPE1_BLK_LEN", 1, 93, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > + { L"GPE1_BASE", 1, 94, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > + { L"CST_CNT", 1, 95, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > + { L"P_LVL2_LAT", 2, 96, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > + { L"P_LVL3_LAT", 2, 98, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > + { L"FLUSH_SIZE", 2, 100, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > + { L"FLUSH_STRIDE", 2, 102, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > + { L"DUTY_OFFSET", 1, 104, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > + { L"DUTY_WIDTH", 1, 105, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > + { L"DAY_ALRM", 1, 106, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > + { L"MON_ALRM", 1, 107, L"0x%x", NULL, NULL, > NULL, NULL }, > > > > + { 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, 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 }, > > > > + { L"FADT Minor Version", 1, 131, L"0x%x", NULL, (VOID > **)&FadtMinorRevision, > > > > NULL, NULL }, > > > > - { L"X_FIRMWARE_CTRL", 8, 132, L"0x%lx", NULL, (VOID > **)&X_FirmwareCtrl, > > > > + { L"X_FIRMWARE_CTRL", 8, 132, L"0x%lx", NULL, (VOID > **)&X_FirmwareCtrl, > > > > ValidateXFirmwareCtrl, NULL }, > > > > - { L"X_DSDT", 8, 140, L"0x%lx", NULL, (VOID > **)&X_DsdtAddress, NULL, NULL }, > > > > - { L"X_PM1a_EVT_BLK", 12, 148, NULL, DumpGas, NULL, > NULL, NULL }, > > > > - { L"X_PM1b_EVT_BLK", 12, 160, NULL, DumpGas, NULL, > NULL, NULL }, > > > > - { L"X_PM1a_CNT_BLK", 12, 172, NULL, DumpGas, NULL, > NULL, NULL }, > > > > - { L"X_PM1b_CNT_BLK", 12, 184, NULL, DumpGas, NULL, > NULL, NULL }, > > > > - { L"X_PM2_CNT_BLK", 12, 196, NULL, DumpGas, NULL, > NULL, NULL }, > > > > - { L"X_PM_TMR_BLK", 12, 208, NULL, DumpGas, NULL, > NULL, NULL }, > > > > - { L"X_GPE0_BLK", 12, 220, NULL, DumpGas, NULL, > NULL, NULL }, > > > > - { L"X_GPE1_BLK", 12, 232, NULL, DumpGas, NULL, > NULL, NULL }, > > > > - { L"SLEEP_CONTROL_REG", 12, 244, NULL, DumpGas, NULL, > NULL, NULL }, > > > > - { L"SLEEP_STATUS_REG", 12, 256, NULL, DumpGas, NULL, > NULL, NULL }, > > > > - { L"Hypervisor VendorIdentity", 8, 268, L"%lx", NULL, NULL, > NULL, NULL } > > > > + { L"X_DSDT", 8, 140, L"0x%lx", NULL, (VOID > **)&X_DsdtAddress, NULL, NULL }, > > > > + { L"X_PM1a_EVT_BLK", 12, 148, NULL, DumpGas, NULL, > NULL, NULL }, > > > > + { L"X_PM1b_EVT_BLK", 12, 160, NULL, DumpGas, NULL, > NULL, NULL }, > > > > + { L"X_PM1a_CNT_BLK", 12, 172, NULL, DumpGas, NULL, > NULL, NULL }, > > > > + { L"X_PM1b_CNT_BLK", 12, 184, NULL, DumpGas, NULL, > NULL, NULL }, > > > > + { L"X_PM2_CNT_BLK", 12, 196, NULL, DumpGas, NULL, > NULL, NULL }, > > > > + { L"X_PM_TMR_BLK", 12, 208, NULL, DumpGas, NULL, > NULL, NULL }, > > > > + { L"X_GPE0_BLK", 12, 220, NULL, DumpGas, NULL, > NULL, NULL }, > > > > + { L"X_GPE1_BLK", 12, 232, NULL, DumpGas, NULL, > NULL, NULL }, > > > > + { L"SLEEP_CONTROL_REG", 12, 244, NULL, DumpGas, NULL, > NULL, NULL }, > > > > + { L"SLEEP_STATUS_REG", 12, 256, NULL, DumpGas, NULL, > NULL, NULL }, > > > > + { L"Hypervisor VendorIdentity", 8, 268, L"%lx", NULL, NULL, > NULL, NULL } > > > > }; > > > > > > > > /** > > > > > > > ^ permalink raw reply [flat|nested] 12+ messages in thread
[parent not found: <16C22F2D003754C4.24651@groups.io>]
* Re: [edk2-devel] [PATCH v4 1/2] ShellPkg/AcpiView: Adds ACPI_PARSER bitfield parser [not found] ` <16C22F2D003754C4.24651@groups.io> @ 2022-01-07 2:58 ` Attar, AbdulLateef (Abdul Lateef) 2022-01-19 3:39 ` Attar, AbdulLateef (Abdul Lateef) 0 siblings, 1 reply; 12+ messages in thread From: Attar, AbdulLateef (Abdul Lateef) @ 2022-01-07 2:58 UTC (permalink / raw) To: devel@edk2.groups.io, Attar, AbdulLateef (Abdul Lateef) Cc: Ray Ni, Zhichao Gao, Sami Mujawar [AMD Official Use Only] Gentle reminder...please review and merge the changeset. -----Original Message----- From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Abdul Lateef Attar via groups.io Sent: 19 December 2021 20:15 To: devel@edk2.groups.io Cc: Ray Ni <ray.ni@intel.com>; Zhichao Gao <zhichao.gao@intel.com>; Sami Mujawar <sami.mujawar@arm.com> Subject: [edk2-devel] [PATCH v4 1/2] ShellPkg/AcpiView: Adds ACPI_PARSER bitfield parser [CAUTION: External Email] 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; + } + + 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; + } + + Offset += Parser[Index].Length; + } // for + + // Decrement the Indent + gIndent -= Indent; + return Offset; +} -- 2.25.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH v4 1/2] ShellPkg/AcpiView: Adds ACPI_PARSER bitfield parser 2022-01-07 2:58 ` [edk2-devel] [PATCH v4 1/2] ShellPkg/AcpiView: Adds ACPI_PARSER bitfield parser Attar, AbdulLateef (Abdul Lateef) @ 2022-01-19 3:39 ` Attar, AbdulLateef (Abdul Lateef) 0 siblings, 0 replies; 12+ messages in thread From: Attar, AbdulLateef (Abdul Lateef) @ 2022-01-19 3:39 UTC (permalink / raw) To: devel@edk2.groups.io; +Cc: Ray Ni, Zhichao Gao, Sami Mujawar [AMD Official Use Only] Hi Zhichao Gao, Ray Ni, Could you please review the below patches and merge the changes? Thanks AbduL -----Original Message----- From: Attar, AbdulLateef (Abdul Lateef) Sent: 07 January 2022 08:29 To: devel@edk2.groups.io; Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@amd.com> Cc: Ray Ni <ray.ni@intel.com>; Zhichao Gao <zhichao.gao@intel.com>; Sami Mujawar <sami.mujawar@arm.com> Subject: RE: [edk2-devel] [PATCH v4 1/2] ShellPkg/AcpiView: Adds ACPI_PARSER bitfield parser Gentle reminder...please review and merge the changeset. -----Original Message----- From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Abdul Lateef Attar via groups.io Sent: 19 December 2021 20:15 To: devel@edk2.groups.io Cc: Ray Ni <ray.ni@intel.com>; Zhichao Gao <zhichao.gao@intel.com>; Sami Mujawar <sami.mujawar@arm.com> Subject: [edk2-devel] [PATCH v4 1/2] ShellPkg/AcpiView: Adds ACPI_PARSER bitfield parser [CAUTION: External Email] 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; + } + + 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; + } + + Offset += Parser[Index].Length; + } // for + + // Decrement the Indent + gIndent -= Indent; + return Offset; +} -- 2.25.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-01-21 7:29 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20211219144437.3721-1-abdattar@amd.com>
2021-12-19 14:44 ` [PATCH v4 1/2] ShellPkg/AcpiView: Adds ACPI_PARSER bitfield parser Abdul Lateef Attar
2022-01-19 16:44 ` Sami Mujawar
2022-01-19 16:46 ` Sami Mujawar
2022-01-20 3:24 ` [edk2-devel] " Attar, AbdulLateef (Abdul Lateef)
2022-01-20 8:36 ` Gao, Zhichao
2022-01-20 12:31 ` Sami Mujawar
2022-01-21 7:29 ` Gao, Zhichao
2021-12-19 14:44 ` [PATCH v4 2/2] ShellPkg/AcpiView: PrintFormatter for FADT Flags field Abdul Lateef Attar
2022-01-19 16:44 ` Sami Mujawar
2022-01-20 8:39 ` [edk2-devel] " Gao, Zhichao
[not found] ` <16C22F2D003754C4.24651@groups.io>
2022-01-07 2:58 ` [edk2-devel] [PATCH v4 1/2] ShellPkg/AcpiView: Adds ACPI_PARSER bitfield parser Attar, AbdulLateef (Abdul Lateef)
2022-01-19 3:39 ` Attar, AbdulLateef (Abdul Lateef)
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox