Krzysztof has moved on to other pastures, I'll respin the patches accordingly.
Cheers,
Tom
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gao, Zhichao via
groups.io
Sent: 11 June 2020 08:43
To: Krzysztof Koch <krzysztof.k...@arm.com>; devel@edk2.groups.io
Cc: Ni, Ray <ray...@intel.com>; Sami Mujawar <sami.muja...@arm.com>;
Laura.Moretta@arm.commatteo.carl...@arm.com; nd <n...@arm.com>
Subject: Re: [edk2-devel] [PATCH v1 1/6] ShellPkg: acpiview: Add interface for
data-driven table parsing
(1) The ASSERT only works with DEBUG build. I think we need add if condition
after the ASSERT to return the error code to avoid the null pointer dereference.
(2) It is suggested to use the const (lower-case) instead of CONST. same to
static.
Others are fine to me.
Thanks,
Zhichao
> -----Original Message-----
> From: Krzysztof Koch <krzysztof.k...@arm.com>
> Sent: Tuesday, May 5, 2020 11:46 PM
> To: devel@edk2.groups.io
> Cc: Ni, Ray <ray...@intel.com>; Gao, Zhichao <zhichao....@intel.com>;
> sami.muja...@arm.com; Laura.Moretta@arm.commatteo.carl...@arm.com;
> n...@arm.com
> Subject: [PATCH v1 1/6] ShellPkg: acpiview: Add interface for
> data-driven table parsing
>
> Define and implement an interface to streamline metadata collection
> and validation for structures present in each ACPI table.
>
> Most ACPI tables define substructures which constitute the table.
> These substructures are identified by their 'Type' field value. The
> range of possible 'Type' values is defined on a per-table basis.
>
> For more sophisticated ACPI table validation, additional data about
> each structure type needs to be maintained. This patch defines a new
> ACPI_STRUCT_INFO structure. It stores additional metadata about a
> building block of an ACPI table. ACPI_STRUCT_INFO's are organised into
> ACPI_STRUCT_DATABASE's. ACPI_STRUCT_DATABASE is an array of
> ACPI_STRUCT_INFO elements which are indexed using structure's type value.
>
> For example, in the Multiple APIC Description Table (MADT) all
> Interrupt Controller Structure types form a single database. In the
> database, the GIC CPU Interface (GICC) structure's metadata is the 11th entry
> (i.e. Type = 0xB).
>
> ACPI_STRUCT_INFO structure consists of:
> - ASCII name of the structure
> - ACPI-defined stucture Type
> - bitmask defining the validity of the structure for various
> architectures
> - instance counter
> - a handler for the structure (ACPI_STRUCT_HANDLER)
>
> The bitmask allows detection of structures in a table which are not
> compatible with the target platform.
>
> For example, the Multiple APIC Description Table (MADT) contains
> Interrupt Controller Structure definitions which apply to either the
> Advanced Programmable Interrupt Controller (APIC) model or the Generic
> Interrupt Controller (GIC) model. Presence of APIC-related structures
> on an Arm-based platform is a bug which is now detected and reported by
> acpiview.
>
> This patch adds support for compatibility checks with the Arm architecture
> only.
> However, provisions are made to allow extensions to other architectures.
>
> ACPI_STRUCT_HANDLER describes how the contents of the structure can be
> parsed. The possible options are:
> - An ACPI_PARSER array which can be passed to the ParseAcpi() function
> - A dedicated function for parsing the structure
> (ACPI_STRUCT_PARSER_FUNC)
>
> If neither of these options is provided, it is assumed that the
> parsing logic is not implemented.
>
> ACPI_STRUCT_PARSER_FUNC expects the the first two arguments to be the
> pointer to the start of the structure to parse and the length of structure's
> buffer.
> The remaining two optional arguments are context specific.
>
> This patch adds methods for:
> - Resetting the instance count for all structure types in a table.
> - Getting the combined instance count for all types in a table.
> - Validating the compatibility of a structure with the target arch.
> - Printing structure counts for the types which are compatible with
> the target architecture and validating that the non-compatible
> structures are not present in the table.
> - Parsing the structure according to the information provided by its
> handle.
>
> Finally, define a helper PrintAcpiStructName () function to streamline
> the printing of ACPI structure name together with the structure's current
> occurrence count.
>
> References:
> - ACPI 6.3, January 2019
>
> Signed-off-by: Krzysztof Koch <krzysztof.k...@arm.com>
> ---
>
> Notes:
> v1:
> - Add interface for data-driven table parsing [Krzysztof]
>
> ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c | 263
> ++++++++++++++++++++
> ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h | 234
> +++++++++++++++++
> 2 files changed, 497 insertions(+)
>
> diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
> index
> 3f12a33050a4e4ab3be2187c90ef8dcf0882283d..32566101e2de2eec3ccf44563ee
> 79379404bff62 100644
> --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
> @@ -6,6 +6,8 @@
> **/
>
> #include <Uefi.h>
> +#include <Library/DebugLib.h>
> +#include <Library/PrintLib.h>
> #include <Library/UefiLib.h>
> #include <Library/UefiBootServicesTableLib.h>
> #include "AcpiParser.h"
> @@ -466,6 +468,267 @@ PrintFieldName (
> );
> }
>
> +/**
> + Produce a Null-terminated ASCII string with the name and index of
> +an
> + ACPI structure.
> +
> + The output string is in the following format: <Name> [<Index>]
> +
> + @param [in] Name Structure name.
> + @param [in] Index Structure index.
> + @param [in] BufferSize The size, in bytes, of the output buffer.
> + @param [out] Buffer Buffer for the output string.
> +
> + @return The number of bytes written to the buffer (not including
> Null-byte)
> +**/
> +UINTN
> +EFIAPI
> +PrintAcpiStructName (
> + IN CONST CHAR8* Name,
> + IN UINT32 Index,
> + IN UINTN BufferSize,
> + OUT CHAR8* Buffer
> + )
> +{
> + ASSERT (Name != NULL);
> + ASSERT (Buffer != NULL);
> +
> + return AsciiSPrint (Buffer, BufferSize, "%a [%d]", Name , Index); }
> +
> +/**
> + Set all ACPI structure instance counts to 0.
> +
> + @param [in,out] StructDb ACPI structure database with counts to reset.
> +**/
> +VOID
> +EFIAPI
> +ResetAcpiStructCounts (
> + IN OUT ACPI_STRUCT_DATABASE* StructDb
> + )
> +{
> + UINT32 Type;
> +
> + ASSERT (StructDb != NULL);
> + ASSERT (StructDb->Entries != NULL);
> +
> + for (Type = 0; Type < StructDb->EntryCount; Type++) {
> + StructDb->Entries[Type].Count = 0;
> + }
> +}
> +
> +/**
> + Sum all ACPI structure instance counts.
> +
> + @param [in] StructDb ACPI structure database with per-type counts to
> sum.
> +
> + @return Total number of structure instances recorded in the database.
> +**/
> +UINT32
> +EFIAPI
> +SumAcpiStructCounts (
> + IN CONST ACPI_STRUCT_DATABASE* StructDb
> + )
> +{
> + UINT32 Type;
> + UINT32 Total;
> +
> + ASSERT (StructDb != NULL);
> + ASSERT (StructDb->Entries != NULL);
> +
> + Total = 0;
> +
> + for (Type = 0; Type < StructDb->EntryCount; Type++) {
> + Total += StructDb->Entries[Type].Count; }
> +
> + return Total;
> +}
> +
> +/**
> + Validate that a structure with a given type value is defined for
> +the given
> + ACPI table and target architecture.
> +
> + The target architecture is evaluated from the firmare build parameters.
> +
> + @param [in] Type ACPI-defined structure type.
> + @param [in] StructDb ACPI structure database with architecture
> + compatibility info.
> +
> + @retval TRUE Structure is valid.
> + @retval FALSE Structure is not valid.
> +**/
> +BOOLEAN
> +EFIAPI
> +IsAcpiStructTypeValid (
> + IN UINT32 Type,
> + IN CONST ACPI_STRUCT_DATABASE* StructDb
> + )
> +{
> + UINT32 Compatible;
> +
> + ASSERT (StructDb != NULL);
> + ASSERT (StructDb->Entries != NULL);
> +
> + if (Type >= StructDb->EntryCount) {
> + return FALSE;
> + }
> +
> +#if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
> + Compatible = StructDb->Entries[Type].CompatArch &
> + (ARCH_COMPAT_ARM | ARCH_COMPAT_AARCH64); #else
> + Compatible = StructDb->Entries[Type].CompatArch;
> +#endif
> +
> + return (Compatible != 0);
> +}
> +
> +/**
> + Print the instance count of each structure in an ACPI table that is
> + compatible with the target architecture.
> +
> + For structures which are not allowed for the target architecture,
> + validate that their instance counts are 0.
> +
> + @param [in] StructDb ACPI structure database with counts to validate.
> +
> + @retval TRUE All structures are compatible.
> + @retval FALSE One or more incompatible structures present.
> +**/
> +BOOLEAN
> +EFIAPI
> +ValidateAcpiStructCounts (
> + IN CONST ACPI_STRUCT_DATABASE* StructDb
> + )
> +{
> + BOOLEAN AllValid;
> + UINT32 Type;
> +
> + ASSERT (StructDb != NULL);
> + ASSERT (StructDb->Entries != NULL);
> +
> + AllValid = TRUE;
> + Print (L"\nTable Breakdown:\n");
> +
> + for (Type = 0; Type < StructDb->EntryCount; Type++) {
> + ASSERT (Type == StructDb->Entries[Type].Type);
> +
> + if (IsAcpiStructTypeValid (Type, StructDb)) {
> + Print (
> + L"%*a%-*a : %d\n",
> + INSTANCE_COUNT_INDENT,
> + "",
> + OUTPUT_FIELD_COLUMN_WIDTH - INSTANCE_COUNT_INDENT,
> + StructDb->Entries[Type].Name,
> + StructDb->Entries[Type].Count
> + );
> + } else if (StructDb->Entries[Type].Count > 0) {
> + AllValid = FALSE;
> + IncrementErrorCount ();
> + Print (
> + L"ERROR: %a Structure is not valid for the target architecture " \
> + L"(found %d)\n",
> + StructDb->Entries[Type].Name,
> + StructDb->Entries[Type].Count
> + );
> + }
> + }
> +
> + return AllValid;
> +}
> +
> +/**
> + Parse the ACPI structure with the type value given according to
> +instructions
> + defined in the ACPI structure database.
> +
> + If the input structure type is defined in the database, increment
> + structure's instance count.
> +
> + If ACPI_PARSER array is used to parse the input structure, the
> + index of the structure (instance count for the type before update)
> + gets printed alongside the structure name. This helps debugging if
> + there are many instances of the type in a table. For
> + ACPI_STRUCT_PARSER_FUNC, the printing of the index must be
> + implemented
> separately.
> +
> + @param [in] Indent Number of spaces to indent the output.
> + @param [in] Ptr Ptr to the start of the structure.
> + @param [in,out] StructDb ACPI structure database with instructions on how
> + parse every structure type.
> + @param [in] Offset Structure offset from the start of the table.
> + @param [in] Type ACPI-defined structure type.
> + @param [in] Length Length of the structure in bytes.
> + @param [in] OptArg0 First optional argument to pass to parser
> function.
> + @param [in] OptArg1 Second optional argument to pass to parser
> function.
> +
> + @retval TRUE ACPI structure parsed successfully.
> + @retval FALSE Undefined structure type or insufficient data to parse.
> +**/
> +BOOLEAN
> +EFIAPI
> +ParseAcpiStruct (
> + IN UINT32 Indent,
> + IN UINT8* Ptr,
> + IN OUT ACPI_STRUCT_DATABASE* StructDb,
> + IN UINT32 Offset,
> + IN UINT32 Type,
> + IN UINT32 Length,
> + IN CONST VOID* OptArg0 OPTIONAL,
> + IN CONST VOID* OptArg1 OPTIONAL
> + )
> +{
> + ACPI_STRUCT_PARSER_FUNC ParserFunc;
> + CHAR8 Buffer[80];
> +
> + ASSERT (Ptr != NULL);
> + ASSERT (StructDb != NULL);
> + ASSERT (StructDb->Entries != NULL); ASSERT (StructDb->Name !=
> + NULL);
> +
> + PrintFieldName (Indent, L"* Offset *"); Print (L"0x%x\n", Offset);
> +
> + if (Type >= StructDb->EntryCount) {
> + IncrementErrorCount ();
> + Print (L"ERROR: Unknown %a. Type = %d\n", StructDb->Name, Type);
> + return FALSE;
> + }
> +
> + if (StructDb->Entries[Type].Handler.ParserFunc != NULL) {
> + ParserFunc = StructDb->Entries[Type].Handler.ParserFunc;
> + ParserFunc (Ptr, Length, OptArg0, OptArg1); } else if
> + (StructDb->Entries[Type].Handler.ParserArray != NULL) {
> + ASSERT (StructDb->Entries[Type].Handler.Elements != 0);
> +
> + PrintAcpiStructName (
> + StructDb->Entries[Type].Name,
> + StructDb->Entries[Type].Count,
> + sizeof (Buffer),
> + Buffer
> + );
> +
> + ParseAcpi (
> + TRUE,
> + Indent,
> + Buffer,
> + Ptr,
> + Length,
> + StructDb->Entries[Type].Handler.ParserArray,
> + StructDb->Entries[Type].Handler.Elements
> + );
> + } else {
> + StructDb->Entries[Type].Count++;
> + Print (
> + L"ERROR: Parsing of %a Structure is not implemented\n",
> + StructDb->Entries[Type].Name
> + );
> + return FALSE;
> + }
> +
> + StructDb->Entries[Type].Count++;
> + return TRUE;
> +}
> +
> /**
> This function is used to parse an ACPI table buffer.
>
> diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
> index
> f81ccac7e118378aa185db4b625e5bcd75f78347..70e540b3a76de0ff9ce70bcabed
> 8548063bea0ff 100644
> --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
> @@ -300,6 +300,240 @@ typedef struct AcpiParser {
> VOID* Context;
> } ACPI_PARSER;
>
> +/**
> + Produce a Null-terminated ASCII string with the name and index of
> +an
> + ACPI structure.
> +
> + The output string is in the following format: <Name> [<Index>]
> +
> + @param [in] Name Structure name.
> + @param [in] Index Structure index.
> + @param [in] BufferSize The size, in bytes, of the output buffer.
> + @param [out] Buffer Buffer for the output string.
> +
> + @return The number of bytes written to the buffer (not including
> Null-byte)
> +**/
> +UINTN
> +EFIAPI
> +PrintAcpiStructName (
> + IN CONST CHAR8* Name,
> + IN UINT32 Index,
> + IN UINTN BufferSize,
> + OUT CHAR8* Buffer
> + );
> +
> +/**
> + Indentation for printing instance counts for structures in an ACPI table.
> +**/
> +#define INSTANCE_COUNT_INDENT 2
> +
> +/**
> + Common signature for functions which parse ACPI structures.
> +
> + @param [in] Ptr Pointer to the start of structure's buffer.
> + @param [in] Length Length of the buffer.
> + @param [in] OptArg0 First optional argument.
> + @param [in] OptArg1 Second optional argument.
> +*/
> +typedef VOID (*ACPI_STRUCT_PARSER_FUNC) (
> + IN UINT8* Ptr,
> + IN UINT32 Length,
> + IN CONST VOID* OptArg0 OPTIONAL,
> + IN CONST VOID* OptArg1 OPTIONAL
> + );
> +
> +/**
> + Description of how an ACPI structure should be parsed.
> +
> + One of ParserFunc or ParserArray should be not NULL. Otherwise, it
> +is
> + assumed that parsing of an ACPI structure is not supported. If both
> + ParserFunc and ParserArray are defined, ParserFunc is used.
> +**/
> +typedef struct AcpiStructHandler {
> + /// Dedicated function for parsing an ACPI structure
> + ACPI_STRUCT_PARSER_FUNC ParserFunc;
> + /// Array of instructions on how each structure field should be parsed
> + CONST ACPI_PARSER* ParserArray;
> + /// Number of elements in ParserArray if ParserArray is defined
> + UINT32 Elements;
> +} ACPI_STRUCT_HANDLER;
> +
> +/**
> + ACPI structure compatiblity with various architectures.
> +
> + Some ACPI tables define structures which are, for example, only
> + valid in the X64 or Arm context. For instance, the Multiple APIC
> + Description Table
> + (MADT) describes both APIC and GIC interrupt models.
> +
> + These definitions provide means to describe the belonging of a
> +structure
> + in an ACPI table to a particular architecture. This way,
> +incompatible
> + structures can be detected.
> +**/
> +#define ARCH_COMPAT_IA32 BIT0
> +#define ARCH_COMPAT_X64 BIT1
> +#define ARCH_COMPAT_ARM BIT2
> +#define ARCH_COMPAT_AARCH64 BIT3
> +#define ARCH_COMPAT_RISCV64 BIT4
> +
> +/**
> + Information about a structure which constitutes an ACPI table **/
> +typedef struct AcpiStructInfo {
> + /// ACPI-defined structure Name
> + CONST CHAR8* Name;
> + /// ACPI-defined structure Type
> + CONST UINT32 Type;
> + /// Architecture(s) for which this structure is valid
> + CONST UINT32 CompatArch;
> + /// Structure's instance count in a table
> + UINT32 Count;
> + /// Information on how to handle the structure
> + CONST ACPI_STRUCT_HANDLER Handler;
> +} ACPI_STRUCT_INFO;
> +
> +/**
> + Macro for defining ACPI structure info when an ACPI_PARSER array
> +must
> + be used to parse the structure.
> +**/
> +#define ADD_ACPI_STRUCT_INFO_ARRAY(Name, Type, Compat, Array) \
> +{ \
> + Name, Type, Compat, 0, {NULL, Array, ARRAY_SIZE (Array)} \
> +}
> +
> +/**
> + Macro for defining ACPI structure info when an
> +ACPI_STRUCT_PARSER_FUNC
> + must be used to parse the structure.
> +**/
> +#define ADD_ACPI_STRUCT_INFO_FUNC(Name, Type, Compat, Func) \
> +{ \
> + Name, Type, Compat, 0, {Func, NULL, 0} \
> +}
> +
> +/**
> + Macro for defining ACPI structure info when the structure is
> +defined in
> + the ACPI spec but no parsing information is provided.
> +**/
> +#define ACPI_STRUCT_INFO_PARSER_NOT_IMPLEMENTED(Name, Type,
> Compat) \
> +{ \
> + Name, Type, Compat, 0, {NULL, NULL, 0} \
> +}
> +
> +/**
> + Database collating information about every structure type defined
> +by
> + an ACPI table.
> +**/
> +typedef struct AcpiStructDatabase {
> + /// ACPI-defined name for the structures being described in the database
> + CONST CHAR8* Name;
> + /// Per-structure-type information. The list must be ordered by the
> +types
> + /// defined for the table. All entries must be unique and there
> +should be
> + /// no gaps.
> + ACPI_STRUCT_INFO* Entries;
> + /// Total number of unique types defined for the table
> + CONST UINT32 EntryCount;
> +} ACPI_STRUCT_DATABASE;
> +
> +/**
> + Set all ACPI structure instance counts to 0.
> +
> + @param [in,out] StructDb ACPI structure database with counts to reset.
> +**/
> +VOID
> +EFIAPI
> +ResetAcpiStructCounts (
> + IN OUT ACPI_STRUCT_DATABASE* StructDb
> + );
> +
> +/**
> + Sum all ACPI structure instance counts.
> +
> + @param [in] StructDb ACPI structure database with per-type counts to
> sum.
> +
> + @return Total number of structure instances recorded in the database.
> +**/
> +UINT32
> +EFIAPI
> +SumAcpiStructCounts (
> + IN CONST ACPI_STRUCT_DATABASE* StructDb
> + );
> +
> +/**
> + Validate that a structure with a given type value is defined for
> +the given
> + ACPI table and target architecture.
> +
> + The target architecture is evaluated from the firmare build parameters.
> +
> + @param [in] Type ACPI-defined structure type.
> + @param [in] StructDb ACPI structure database with architecture
> + compatibility info.
> +
> + @retval TRUE Structure is valid.
> + @retval FALSE Structure is not valid.
> +**/
> +BOOLEAN
> +EFIAPI
> +IsAcpiStructTypeValid (
> + IN UINT32 Type,
> + IN CONST ACPI_STRUCT_DATABASE* StructDb
> + );
> +
> +/**
> + Print the instance count of each structure in an ACPI table that is
> + compatible with the target architecture.
> +
> + For structures which are not allowed for the target architecture,
> + validate that their instance counts are 0.
> +
> + @param [in] StructDb ACPI structure database with counts to validate.
> +
> + @retval TRUE All structures are compatible.
> + @retval FALSE One or more incompatible structures present.
> +**/
> +BOOLEAN
> +EFIAPI
> +ValidateAcpiStructCounts (
> + IN CONST ACPI_STRUCT_DATABASE* StructDb
> + );
> +
> +/**
> + Parse the ACPI structure with the type value given according to
> +instructions
> + defined in the ACPI structure database.
> +
> + If the input structure type is defined in the database, increment
> + structure's instance count.
> +
> + If ACPI_PARSER array is used to parse the input structure, the
> + index of the structure (instance count for the type before update)
> + gets printed alongside the structure name. This helps debugging if
> + there are many instances of the type in a table. For
> + ACPI_STRUCT_PARSER_FUNC, the printing of the index must be
> + implemented
> separately.
> +
> + @param [in] Indent Number of spaces to indent the output.
> + @param [in] Ptr Ptr to the start of the structure.
> + @param [in,out] StructDb ACPI structure database with instructions on how
> + parse every structure type.
> + @param [in] Offset Structure offset from the start of the table.
> + @param [in] Type ACPI-defined structure type.
> + @param [in] Length Length of the structure in bytes.
> + @param [in] OptArg0 First optional argument to pass to parser
> function.
> + @param [in] OptArg1 Second optional argument to pass to parser
> function.
> +
> + @retval TRUE ACPI structure parsed successfully.
> + @retval FALSE Undefined structure type or insufficient data to parse.
> +**/
> +BOOLEAN
> +EFIAPI
> +ParseAcpiStruct (
> + IN UINT32 Indent,
> + IN UINT8* Ptr,
> + IN OUT ACPI_STRUCT_DATABASE* StructDb,
> + IN UINT32 Offset,
> + IN UINT32 Type,
> + IN UINT32 Length,
> + IN CONST VOID* OptArg0 OPTIONAL,
> + IN CONST VOID* OptArg1 OPTIONAL
> + );
> +
> /**
> A structure used to store the pointers to the members of the
> ACPI description header structure that was parsed.
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#61137): https://edk2.groups.io/g/devel/message/61137
Mute This Topic: https://groups.io/mt/74000905/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
