From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 287CE9416C9 for ; Thu, 20 Jul 2023 15:25:01 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=zJLQE+cCCWcbnyHBbcnW+vQ7MC4G1xn1RPIlCtp1mI0=; c=relaxed/simple; d=groups.io; h=X-Received:X-Received:X-Received:X-Received:Message-ID:Date:MIME-Version:User-Agent:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Unsubscribe:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:X-Gm-Message-State:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1689866700; v=1; b=BXN7T6tBWTPTS4/+do5CTIYHqvMhXxJHvGRtjICye8svVUWj+6hX5xnGGCpyNvrT9hD0qRvA F+jJLiVLV/y38CG1OkK0dB0EwHWlyew1hm5+gSf2qIcWe29T+XfAzdHdnLg3wDj4DKLLCXTjRFp 5W7w9RkSQenHPy81+VRI+sck= X-Received: by 127.0.0.2 with SMTP id O6ECYY7687511xXurMbTVXg4; Thu, 20 Jul 2023 08:25:00 -0700 X-Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web11.15796.1689866699652736728 for ; Thu, 20 Jul 2023 08:25:00 -0700 X-Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 619922F4; Thu, 20 Jul 2023 08:25:42 -0700 (PDT) X-Received: from [10.34.100.101] (e126645.nice.arm.com [10.34.100.101]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id BE0243F738; Thu, 20 Jul 2023 08:24:57 -0700 (PDT) Message-ID: Date: Thu, 20 Jul 2023 17:24:50 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.11.0 Subject: Re: [edk2-devel] [PATCH V3 3/3] ShellPkg/AcpiView: Add MPAM Parser To: devel@edk2.groups.io, rohit.mathew@arm.com Cc: Thomas Abraham , Sami Mujawar , James Morse , Ray Ni , Zhichao Gao References: <20230522144506.2799121-1-Rohit.Mathew@arm.com> From: "PierreGondois" In-Reply-To: <20230522144506.2799121-1-Rohit.Mathew@arm.com> Precedence: Bulk List-Unsubscribe: List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,pierre.gondois@arm.com X-Gm-Message-State: nvSVfXjg08YsDCnK8y0CnZ1Kx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=BXN7T6tB; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=arm.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io Hello Rohit, On 5/22/23 16:45, Rohit Mathew via groups.io wrote: > Add a parser for the MPAM (Memory system resource partitioning and > monitoring) ACPI table. This parser would parse all MPAM related > structures embedded as part of the ACPI table. Necessary validations are > also performed where and when required. >=20 > Signed-off-by: Rohit Mathew > --- > ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h = | 21 + > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Mpam/MpamParser.c = | 1331 ++++++++++++++++++++ > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Mpam/MpamParser.h = | 25 + > ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLi= b.c | 3 +- > ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLi= b.inf | 4 +- > ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLi= b.uni | 3 +- > 6 files changed, 1384 insertions(+), 3 deletions(-) >=20 > diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h b/= ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h > index c9f41650d9..fef08e714d 100644 > --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h > @@ -825,6 +825,27 @@ ParseAcpiMcfg ( > IN UINT8 AcpiTableRevision > ); > =20 > +/** > + This function parses the ACPI MPAM table. > + When trace is enabled this function parses the MCFG table and > + traces the ACPI table fields. > + > + This function also performs validation of the ACPI table fields. > + > + @param [in] Trace If TRUE, trace the ACPI fields. > + @param [in] Ptr Pointer to the start of the buffer. > + @param [in] AcpiTableLength Length of the ACPI table. > + @param [in] AcpiTableRevision Revision of the ACPI table. > +**/ > +VOID > +EFIAPI > +ParseAcpiMpam ( > + IN BOOLEAN Trace, > + IN UINT8 *Ptr, > + IN UINT32 AcpiTableLength, > + IN UINT8 AcpiTableRevision > + ); > + > /** > This function parses the ACPI PCCT table including its sub-structures > of type 0 through 4. > diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Mpam/Mp= amParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Mpam/Mpam= Parser.c > new file mode 100644 > index 0000000000..9352357318 > --- /dev/null > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Mpam/MpamParse= r.c > @@ -0,0 +1,1331 @@ > +/** @file > + MPAM table parser > + > + Copyright (c) 2023, ARM Limited. All rights reserved. > + SPDX-License-Identifier: BSD-2-Clause-Patent > + > + @par Specification Reference: > + - [1] ACPI for Memory System Resource Partitioning and Monitoring 2.0 > + (https://developer.arm.com/documentation/den0065/latest) > + > + @par Glossary: > + - MPAM - Memory System Resource Partitioning And Monitoring > + - MSC - Memory System Component > + - PCC - Platform Communication Channel > + - RIS - Resource Instance Selection > + - SMMU - Arm System Memory Management Unit > + **/ > + > +#include > +#include > +#include > +#include "AcpiParser.h" > +#include "AcpiView.h" > +#include "AcpiViewConfig.h" > +#include "MpamParser.h" > + > +// Local variables > +STATIC UINT8 *Locator; > +STATIC CONST UINT8 *LocatorType; > +STATIC CONST UINT16 *MpamMscNodeLength; > +STATIC UINT32 MpamMscNodeLengthCumulative; > +STATIC UINT32 HeaderSize; > +STATIC CONST UINT32 *ErrorInterrupt; > +STATIC CONST UINT32 *InterfaceType; > +STATIC CONST UINT32 *NumberOfMscResources; > +STATIC CONST UINT32 *NumberOfFunctionalDependencies; > +STATIC CONST UINT32 *NumberOfInterconnectDescriptors; > +STATIC CONST UINT32 *OverflowInterrupt; > +STATIC ACPI_DESCRIPTION_HEADER_INFO AcpiHdrInfo; > + > +/** > + When the length of the table is insufficient to be parsed, this functi= on could > + be used to display an appropriate error message. > + > + @param [in] ErrorMsg Error message string that has to be appended= to the (alignment) > + main error log. This string could explain the reason > + why a insufficient length error was encountered in > + the first place. > +**/ > +STATIC > +VOID > +EFIAPI It seems this function is not used for length related issues. Maybe it shou= ld be renamed and the 'Insufficient MPAM MSC Node length ...' message be removed. > +MpamLengthError ( > + IN CONST CHAR16 *ErrorMsg > + ) > +{ > + IncrementErrorCount (); > + Print (L"\nERROR : "); > + Print (ErrorMsg); > + Print ( > + L"\nError : Insufficient MPAM MSC Node length. Table length : %u.\n"= , > + *(AcpiHdrInfo.Length) > + ); > +} > + > +/** > + This function validates the passed reserved parameter. Any reserved fi= eld > + within the MPAM specification must be 0. > + > + @param [in] Reserved Reserved param that has to be validated. > +**/ > +STATIC > +VOID > +EFIAPI It seems the 'Reserved' fields is also checked in the SRAT table parsing, c= f. ValidateSratReserved(). Maybe it would be good to re-use your generic imple= mentation there aswell (just a suggestion, maybe to be done in a later patch). > +ValidateReserved ( > + IN CONST UINT64 Reserved > + ) > +{ > + if (Reserved !=3D 0) { > + IncrementErrorCount (); > + Print (L"\nERROR : Reserved field should be 0\n"); > + } > +} > + > +/** > + This function validates reserved fields. Any reserved field within the= MPAM > + specification must be 0. > + > + @param [in] Ptr Pointer to the start of the field data. > + @param [in] Context Pointer to context specific information. For thi= s > + particular function, context holds the size of t= he > + reserved field that needs to be validated. > +**/ > +STATIC > +VOID > +EFIAPI > +ValidateReservedGeneric ( > + IN UINT8 *Ptr, > + IN VOID *Context > + ) > +{ > + UINT64 Reserved; > + > + // If Context is not passed on, don't validate. without the length, it= is not > + // possible to decode the reserved field or plan out alignment require= ments. > + if (Context =3D=3D NULL) { > + return; > + } > + > + // Only those cases which are handled currently have been implemented. > + switch ((UINT64)Context) { > + case 1: > + Reserved =3D *Ptr; > + break; > + case 2: > + if (((UINT64)Ptr) % 2 =3D=3D 0) { > + Reserved =3D *((UINT16 *)Ptr); > + } else { > + Reserved =3D (*(Ptr + 1) << 8); > + Reserved |=3D *Ptr; > + } > + > + break; > + case 4: > + if (((UINT64)Ptr) % 4 =3D=3D 0) { > + Reserved =3D *((UINT32 *)Ptr); > + } else { > + Reserved =3D (*(Ptr + 3) << 24); > + Reserved |=3D (*(Ptr + 2) << 16); > + Reserved |=3D (*(Ptr + 1) << 8); > + Reserved |=3D (*Ptr); > + } > + > + break; > + default: > + return; > + } > + > + ValidateReserved (Reserved); > +} > + > +/** > + Fields that need additional decoding could use this function to print = out the > + decoded value for a particular field. The decoded value handled in thi= s > + function is a string. > + > + @param [in] Indent Number of spaces to add to the global = table > + indent. The global table indent is 0 b= y > + default; however this value is updated= on > + entry to the ParseAcpi() by adding the= indent > + value provided to ParseAcpi() and rest= ored > + back on exit. Therefore the total ind= ent in > + the output is dependent on from where = this > + function is called. > + @param [in] FieldStr Title string for the field of interest= . > + @param [in] DecodedStr Decoded value to be printed for the Fi= eldStr. > +**/ > +STATIC > +VOID > +EFIAPI > +PrintDecodedField ( > + IN UINT32 Indent, > + IN CONST CHAR16 *FieldStr, > + IN CONST CHAR16 *DecodedStr > + ) > +{ > + Print (L"\n"); > + PrintFieldName (Indent, FieldStr); > + Print (DecodedStr); > +} > + > +/** > + This function validates the MMIO size within the MSC node body for MPA= M ACPI > + table. MPAM ACPI specification states that the MMIO size for an MSC ha= ving PCC > + type interface should be zero. > + > + @param [in] Ptr Pointer to the start of the field data. > + @param [in] Context Pointer to context specific information. For th= is > + function, context holds the parent/double point= er to a > + variable holding the interface type. Make sure = to call > + the function accordingly. > +**/ > +STATIC > +VOID > +EFIAPI > +ValidateMmioSize ( > + IN UINT8 *Ptr, > + IN VOID *Context > + ) > +{ > + UINT8 InterfaceType; > + UINT8 **InterfaceTypeParentPtr; > + UINT32 MmioSize; > + > + InterfaceTypeParentPtr =3D (UINT8 **)Context; I don't think these comments are necessary as this is generic for acpiview. > + // Context - double pointer to interface type. (Refer MpamMscNodePa= rse > + // table). > + // *Context - address of object holding interface type. > + // **Context - interface type. > + if (*InterfaceTypeParentPtr =3D=3D NULL) { > + MpamLengthError (L"Interface type not set!"); > + return; > + } > + > + InterfaceType =3D **InterfaceTypeParentPtr; > +> + if (InterfaceType =3D=3D EFI_ACPI_MPAM_INTERFACE_PCC) { > + MmioSize =3D *((UINT32 *)Ptr); > + > + if (MmioSize !=3D 0) { > + IncrementErrorCount (); > + Print ( > + L"\nERROR: MMIO size should be 0 for PCC interface type. Size - = %u\n", > + MmioSize > + ); > + } > + } > +} > + > +/** > + This function decodes and validates the link type for MPAM's interconn= ect > + descriptor. Valid links are of NUMA and PROC type. > + > + @param [in] Ptr Pointer to the start of the field data. > + @param [in] Context Pointer to context specific information. For t= his > + function, context is ignored. > +**/ > +STATIC > +VOID > +EFIAPI > +DecodeLinkType ( > + IN UINT8 *Ptr, > + IN VOID *Context > + ) > +{ > + UINT8 LinkType; > + > + LinkType =3D *Ptr; > + > + if (LinkType =3D=3D EFI_ACPI_MPAM_LINK_TYPE_NUMA) { > + PrintDecodedField ( > + 2, The name of the field should have already been printed, I m not sure I see the use of printing: "* Link type decoded". Similar comment for the PrintDecodedField() in general, I think it would be better to print the value and the interpretation of the value, as: NUMA (0) for instance. > + L"* Link type decoded", > + L"NUMA" > + ); > + } else if (LinkType =3D=3D EFI_ACPI_MPAM_LINK_TYPE_PROC) { > + PrintDecodedField ( > + 2, > + L"* Link type decoded", > + L"PROC" > + ); > + } else { > + IncrementErrorCount (); > + Print ( > + L"\nERROR: Invalid link type - %u\n", > + (UINT32)LinkType > + ); > + } > +} > + > +/** > + This function decodes the hardware ID field present within MPAM ACPI t= able. > + The specification states that the hardware ID has to be set to zero if= not > + being used. > + > + @param [in] Ptr Pointer to the start of the field data. > + @param [in] Context Pointer to context specific information. For t= his > + function, context is ignored. > +**/ > +STATIC > +VOID > +EFIAPI > +DecodeHardwareId ( > + IN UINT8 *Ptr, > + IN VOID *Context > + ) > +{ > + UINT64 HardwareId; > + I don't think this comment is necessary. > + // Hardware Id fields within MPAM table always falls on offset divisib= le by 8. > + // Proceed to dereference 8 bytes in one go. > + HardwareId =3D *((UINT64 *)Ptr); > + > + if (HardwareId !=3D 0) { > + Print (L"\n"); Is it necessary to print '* Hardware ID type decoded' as the field name sho= uld have already been printed ? > + PrintFieldName (2, L"* Hardware ID type decoded"); > + Dump8Chars (NULL, Ptr); > + } > +} > + > +/** > + This function decodes and validates the interface type for MPAM. Valid > + interfaces are of MMIO and PCC type. > + > + @param [in] Ptr Pointer to the start of the field data. > + @param [in] Context Pointer to context specific information. For t= his > + function, context is ignored. > +**/ > +STATIC > +VOID > +EFIAPI > +DecodeInterfaceType ( > + IN UINT8 *Ptr, > + IN VOID *Context > + ) > +{ > + UINT8 InterfaceType; > + > + InterfaceType =3D *Ptr; > + > + if (InterfaceType =3D=3D EFI_ACPI_MPAM_INTERFACE_MMIO) { > + PrintDecodedField ( > + 2, > + L"* Interface type decoded", > + L"MMIO" > + ); > + } else if (InterfaceType =3D=3D EFI_ACPI_MPAM_INTERFACE_PCC) { > + PrintDecodedField ( > + 2, > + L"* Interface type decoded", > + L"PCC" > + ); > + } else { > + IncrementErrorCount (); > + Print ( > + L"\nERROR: I ype - %u\n", > + (UINT32)InterfaceType > + ); > + } > +} > + > +/** > + This function decodes and validates the interrupt flags present in the= MPAM > + MSC node body. > + > + @param [in] Ptr Pointer to the start of the field data. > + @param [in] Context Pointer to context specific information. For th= is > + function, context holds the parent/double point= er to a > + variable holding the interrupt gsiv. Make sure = to call > + the function accordingly. > +**/ > +STATIC > +VOID > +EFIAPI > +DecodeInterruptFlags ( > + IN UINT8 *Ptr, > + IN VOID *Context > + ) > +{ > + UINT8 InterruptMode; > + UINT8 InterruptType; > + UINT8 InterruptAffinityType; > + UINT8 InterruptAffinityValid; > + UINT32 InterruptFlag; > + UINT32 InterruptGsiv; > + UINT32 **InterruptGsivParentPtr; > + UINT32 Reserved; > + > + InterruptGsivParentPtr =3D (UINT32 **)Context; > + // Context - double pointer to the gsiv object. (Refer MpamMscNodeP= arse > + // table). > + // *Context - pointer to gsiv object. > + // **Context - gsiv. > + if (*InterruptGsivParentPtr =3D=3D NULL) { > + MpamLengthError (L"Interrupt gsiv not set!"); > + return; > + } > + > + InterruptGsiv =3D **InterruptGsivParentPtr; > + > + // If Interrupts are absent for some reason, the flags wouldn't need a= ny > + // parsing. > + if (InterruptGsiv =3D=3D 0) { > + return; > + } I think there is something to parse bitfields in acpiview, cf: https://edk2.groups.io/g/devel/message/105029 Would it fit this field parsing aswell ? > + > + InterruptFlag =3D *((UINT32 *)Ptr); > + > + // Decode interrupt mode. > + InterruptMode =3D (InterruptFlag & EFI_ACPI_MPAM_INTERRUPT_MODE_MASK) > + >> EFI_ACPI_MPAM_INTERRUPT_MODE_SHIFT; > + > + if (InterruptMode =3D=3D EFI_ACPI_MPAM_INTERRUPT_LEVEL_TRIGGERED) { > + PrintDecodedField ( > + 2, > + L"* Interrupt mode decoded", > + L"Level triggered" > + ); > + } else { > + PrintDecodedField ( > + 2, > + L"* Interrupt mode decoded", > + L"Edge triggered" > + ); > + } > + > + // Decode interrupt type. > + InterruptType =3D (InterruptFlag & EFI_ACPI_MPAM_INTERRUPT_TYPE_MASK) > + >> EFI_ACPI_MPAM_INTERRUPT_TYPE_SHIFT; > + > + if (InterruptType =3D=3D EFI_ACPI_MPAM_INTERRUPT_WIRED) { > + PrintDecodedField ( > + 2, > + L"* Interrupt type decoded", > + L"Wired interrupt" > + ); > + } else { > + IncrementErrorCount (); > + Print (L"\n"); > + PrintFieldName (2, L"* Interrupt type decoded"); > + Print (L"%u", InterruptType); > + Print (L"\nERROR : Interrupt type reserved\n"); > + } > + > + // Decode affinity valid. > + InterruptAffinityValid =3D (InterruptFlag > + & EFI_ACPI_MPAM_INTERRUPT_AFFINITY_VALID_MAS= K) > + >> EFI_ACPI_MPAM_INTERRUPT_AFFINITY_VALID_SHI= FT; > + > + if (InterruptAffinityValid !=3D EFI_ACPI_MPAM_INTERRUPT_AFFINITY_VALID= ) { > + PrintDecodedField ( > + 2, > + L"* Interrupt affinity valid decoded", > + L"Affinity not valid" > + ); > + } else { > + PrintDecodedField ( > + 2, > + L"* Interrupt affinity valid decoded", > + L"Affinity valid" > + ); > + > + // Decode affinity type. > + InterruptAffinityType =3D (InterruptFlag > + & EFI_ACPI_MPAM_INTERRUPT_AFFINITY_TYPE_MAS= K) > + >> EFI_ACPI_MPAM_INTERRUPT_AFFINITY_TYPE_SHI= FT; > + > + if (InterruptAffinityType =3D=3D EFI_ACPI_MPAM_INTERRUPT_PROCESSOR_A= FFINITY) { > + PrintDecodedField ( > + 2, > + L"* Interrupt affinity type decoded", > + L"Processor affinity" > + ); > + } else { > + PrintDecodedField ( > + 2, > + L"* Interrupt affinity type decoded", > + L"Processor container affinity" > + ); > + } > + } > + > + // Decode reserved field. > + Reserved =3D (InterruptFlag & EFI_ACPI_MPAM_INTERRUPT_RESERVED_MASK) > + >> EFI_ACPI_MPAM_INTERRUPT_RESERVED_SHIFT; > + Print (L"\n"); > + PrintFieldName (2, L"* Reserved decoded"); > + Print (L"%u", Reserved); > + > + ValidateReserved (Reserved); > +} > + > +/** > + ACPI_PARSER array describing the Generic ACPI MPAM table header. > +**/ > +STATIC CONST ACPI_PARSER MpamParser[] =3D { > + PARSE_ACPI_HEADER (&AcpiHdrInfo) > +}; > + > +/** > + ACPI_PARSER array describing the MPAM MSC node object. > +**/ > +STATIC CONST ACPI_PARSER MpamMscNodeParser[] =3D { > + { L"Length", 2, 0, L"%u", NULL, > + (VOID **)&MpamMscNodeLength, NULL, NULL }, > + // Once Interface type is decoded, the address of interface type field= is > + // captured into InterfaceType pointer so that it could be used to che= ck if > + // MMIO Size field is set as per the specification. > + { L"Interface type", 1, 2, L"0x%x", NULL, > + (VOID **)&InterfaceType, DecodeInterfaceType, NULL }, > + { L"Reserved", 1, 3, L"0x%x", NULL, > + NULL, ValidateReservedGeneric, (VOID *)1 }, > + { L"Identifier", 4, 4, L"%u", NULL, > + NULL, NULL, NULL }, > + { L"Base address", 8, 8, L"0x%lx", NULL, > + NULL, NULL, NULL }, > + { L"MMIO Size", 4, 16, L"0x%x", NULL, > + NULL, ValidateMmioSize, (VOID **)&InterfaceType }, > + { L"Overflow interrupt", 4, 20, L"%u", NULL, > + (VOID **)&OverflowInterrupt, NULL, NULL }, > + { L"Overflow interrupt flags", 4, 24, L"0x%x", NULL, > + // Initializer list has to have constants. Pass the address of > + // OverflowInterrupt to satisfy this. While using it within the vali= dator > + // make sure to dereference accordingly. > + NULL, DecodeInterruptFlags, (VOID *)&OverflowInterrupt }, > + { L"Reserved1", 4, 28, L"0x%x", NULL, > + NULL, ValidateReservedGeneric, (VOID *)4 }, > + { L"Overflow interrupt affinity", 4, 32, L"0x%x", NULL, > + NULL, NULL, NULL }, > + { L"Error interrupt", 4, 36, L"%u", NULL, > + (VOID **)&ErrorInterrupt, NULL, NULL }, > + { L"Error interrupt flags", 4, 40, L"0x%x", NULL, > + // Initializer list has to have constants. Pass the address of > + // OverflowInterrupt to satisfy this. While using it within the vali= dator > + // make sure to dereference accordingly. > + NULL, DecodeInterruptFlags, (VOID *)&ErrorInterrupt }, > + { L"Reserved2", 4, 44, L"0x%x", NULL, > + NULL, ValidateReservedGeneric, (VOID *)4 }, > + { L"Error interrupt affinity", 4, 48, L"0x%x", NULL, > + NULL, NULL, NULL }, > + { L"MAX_NRDY_USEC", 4, 52, L"0x%x", NULL, > + NULL, NULL, NULL }, > + { L"Hardware ID of linked device", 8, 56, L"0x%lx", NULL, > + NULL, DecodeHardwareId, NULL }, > + { L"Instance ID of linked device", 4, 64, L"0x%x", NULL, > + NULL, NULL, NULL }, > + { L"Number of resource nodes", 4, 68, L"%u", NULL, > + (VOID **)&NumberOfMscResources, NULL, NULL } > +}; > + > +/** > + ACPI_PARSER array describing the MPAM MSC resource fields till locator= type. > +**/ > +STATIC CONST ACPI_PARSER MpamMscResourceParser[] =3D { > + { L"Identifier", 4, 0, L"%u", NULL, NULL, NULL, NULL }, > + { L"RIS index", 1, 4, L"%u", NULL, NULL, NULL, NULL }, > + { L"Reserved1", 2, 5, L"0x%x", NULL, > + NULL, ValidateReservedGeneric, (VOID *)2 }, > + { L"Locator type", 1, 7, L"0x%x", NULL, > + (VOID **)&LocatorType, > + NULL, NULL } > +}; > + > +/** > + ACPI_PARSER array describing the MPAM MSC resource locator field. > +**/ > +STATIC CONST ACPI_PARSER MpamMscResourceLocatorParser[] =3D { > + { L"Locator", 12, 0, NULL, NULL, (VOID **)&Locator, NULL, > + NULL } > +}; > + > +/** > + ACPI_PARSER array describing the MPAM MSC resource's functional depend= encies > + count. > +**/ > +STATIC CONST ACPI_PARSER MpamMscFunctionalDependencyCountParser[] =3D { > + { L"Number of func dependencies", 4, 0, L"%u", NULL, > + (VOID **)&NumberOfFunctionalDependencies, NULL, NULL } > +}; > + > +/** > + ACPI_PARSER array describing the MPAM MSC resource's functional depend= encies. > +**/ > +STATIC CONST ACPI_PARSER MpamMscFunctionalDependencyParser[] =3D { > + { L"Producer", 4, 0, L"0x%x", NULL, NULL, NULL, NULL }, > + { L"Reserved", 4, 4, L"0x%x", NULL, > + NULL, ValidateReservedGeneric, (VOID *)4 }, > +}; > + > +/** > + ACPI_PARSER array describing the interconnect descriptor table associa= ted with > + the interconnect locator type. > +**/ > +STATIC CONST ACPI_PARSER MpamInterconnectDescriptorTableParser[] =3D { > + { L"Signature", 16, 0, > + L"%x%x%x%x-%x%x-%x%x-%x%x-%x%x%x%x%x%x", Dump16Chars, NULL, NULL, NU= LL }, > + { L"Number of Interconnect desc", 4, 16,L"0x%x", NULL, > + (VOID **)&NumberOfInterconnectDescriptors, NULL, NULL } > +}; > + > +/** > + ACPI_PARSER array describing the interconnect descriptor associated wi= th the > + interconnect locator type. The specification includes an additional re= served > + field also within the interconnect descriptor. This is not included in= the > + parser object on purpose. The function is of 3 bytes length which the > + ParseAcpi function can't parse. This has been handled separately. > +**/ > +STATIC CONST ACPI_PARSER MpamInterconnectDescriptorParser[] =3D { > + { L"Source ID", 4, 0, L"%u", NULL, NULL, NULL, NULL }, > + { L"Destination ID", 4, 4, L"%u", NULL, NULL, NULL, NULL }, > + { L"Link type", 1, 8, L"0x%x", NULL, > + NULL, DecodeLinkType, NULL } > +}; > + > +/** > + PrintLocatorTitle could be used to print the decoded locator title. > + > + @param [in] Indent Number of spaces to add to the global = table > + indent. The global table indent is 0 b= y > + default; however this value is updated= on > + entry to the ParseAcpi() by adding the= indent > + value provided to ParseAcpi() and rest= ored > + back on exit. Therefore the total ind= ent in > + the output is dependent on from where = this > + function is called. > + @param [in] LocatorTitle Title string to be used for the locato= r. > +**/ > +STATIC > +VOID > +EFIAPI > +PrintLocatorTitle ( > + IN UINT32 Indent, > + IN CONST CHAR16 *LocatorTitle > + ) > +{ > + PrintFieldName (Indent, L"* Locator type decoded"); > + Print (LocatorTitle); > + Print (L"\n"); > + PrintFieldName (Indent, L"Locator"); > + Print (L"\n"); > +} > + > +/** > + PrintLocatorDescriptor64 could be used to print the 8 byte field > + within the 12 byte locator descriptor. > + > + @param [in] Indent Number of spaces to add to the global = table > + indent. The global table indent is 0 b= y > + default; however this value is updated= on > + entry to the ParseAcpi() by adding the= indent > + value provided to ParseAcpi() and rest= ored > + back on exit. Therefore the total ind= ent in > + the output is dependent on from where = this > + function is called. > + @param [in] DescriptorTitle Title string to be used for the descri= ptor. > + @param [in] Descriptor64 Descriptor to be printed. > + @param [in] Validate Boolean to indicate if the second 4 by= te field > + needs to be validated as a reserved fi= eld. > +**/ > +STATIC > +VOID > +EFIAPI > +PrintLocatorDescriptor64 ( > + IN UINT32 Indent, > + IN CONST CHAR16 *DescriptorTitle, > + IN CONST UINT64 Descriptor64, > + IN BOOLEAN Validate > + ) > +{ > + PrintFieldName (Indent, DescriptorTitle); > + Print (L"%lu", Descriptor64); > + > + if (Validate) { > + ValidateReserved (Descriptor64); > + } > + > + Print (L"\n"); > +} > + > +/** > + PrintLocatorDescriptor32 could be used to print the 4 byte field > + within the 12 byte locator descriptor. > + > + @param [in] Indent Number of spaces to add to the global = table > + indent. The global table indent is 0 b= y > + default; however this value is updated= on > + entry to the ParseAcpi() by adding the= indent > + value provided to ParseAcpi() and rest= ored > + back on exit. Therefore the total ind= ent in > + the output is dependent on from where = this > + function is called. > + @param [in] DescriptorTitle Title string to be used for the descri= ptor. > + @param [in] Descriptor32 Descriptor to be printed. > + @param [in] Validate Boolean to indicate if the second 4 by= te field > + needs to be validated as a reserved fi= eld. > +**/ > +STATIC > +VOID > +EFIAPI > +PrintLocatorDescriptor32 ( > + IN UINT32 Indent, > + IN CONST CHAR16 *DescriptorTitle, > + IN CONST UINT32 Descriptor32, > + IN BOOLEAN Validate > + ) > +{ > + PrintFieldName (Indent, DescriptorTitle); > + Print (L"0x%x", Descriptor32); > + > + if (Validate) { > + ValidateReserved (Descriptor32); > + } > + > + Print (L"\n"); > +} > + > +/** > + PrintGenericLocatorDescriptor should be used for decoding and printing= locator > + descriptor that could be split as two 8 and 4 byte fields. The Locator= Ptr > + field is casted to (UINT64 *) and decoded within the macro. From the M= PAM ACPI > + specification, the offset of the locator descriptor field is divisible= by 8. > + It is assumed that ACPI tables are placed at 8 byte aligned address an= d thus > + unaligned access should not be a concern. > + > + Only locators which have its second field as reserved should use this > + function. > + > + @param [in] Indent Number of spaces to add to the global = table > + indent. The global table indent is 0 b= y > + default; however this value is updated= on > + entry to the ParseAcpi() by adding the= indent > + value provided to ParseAcpi() and rest= ored > + back on exit. Therefore the total ind= ent in > + the output is dependent on from where = this > + function is called. > + @param [in] LocatorTitle Title string to be used for the locato= r > + @param [in] Descriptor1Title Title string to be used for descriptor= 1 > + @param [in] Descriptor2Title Title string to be used for descriptor= 2 > + @param [in] LocatorPtr Ptr to the start of locator > + @param [in] Validate Boolean to indicate if the second 4 by= te field > + needs to be validated as a reserved fi= eld. > +**/ > +STATIC > +VOID > +EFIAPI > +PrintGenericLocatorDescriptor ( > + IN UINT32 Indent, > + IN CONST CHAR16 *LocatorTitle, > + IN CONST CHAR16 *Descriptor1Title, > + IN CONST CHAR16 *Descriptor2Title, > + IN UINT8 *LocatorPtr, > + IN BOOLEAN Validate > + ) > +{ > + PrintLocatorTitle (Indent, LocatorTitle); > + PrintLocatorDescriptor64 ( > + Indent, > + Descriptor1Title, > + *((UINT64 *)LocatorPtr), > + FALSE > + ); > + > + // Move descriptor to point to next field. > + LocatorPtr +=3D sizeof (UINT64); > + > + PrintLocatorDescriptor32 ( > + Indent, > + Descriptor2Title, > + *((UINT64 *)LocatorPtr), > + Validate > + ); > +} > + > +/** > + This function parses the locator field within the resource node for AC= PI MPAM > + table. The parsing is based on the locator type field. > + > + This function also performs validation of the locator field. > + **/ > +STATIC > +VOID > +EFIAPI > +ParseLocator ( > + VOID > + ) > +{ > + UINT8 *LocatorPtr; > + > + LocatorPtr =3D Locator; > + > + switch (*LocatorType) { I think it would be simpler to define names as: STATIC CONST CHAR16 *MpamLocationNames[] =3D { L"Processor cache", L"Memory", ... and also to define ACPI_PARSER tables for the locator descriptors instead of using PrintGenericLocatorDescriptor(). Eg: STATIC CONST ACPI_PARSER SmmuLocatorDescriptorParser[] =3D { { L"SMMU interface", 8, 0, L"%lu", NULL, NULL, NULL, NULL }, { L"Reserved ID", 4, 8, L"%u", NULL, NULL, ValidateReservedGeneric, (VOI= D *)2 }, > + case EFI_ACPI_MPAM_LOCATION_PROCESSOR_CACHE: > + PrintGenericLocatorDescriptor ( > + 4, > + L"Processor cache", > + L"* Cache reference", > + L"* Reserved", > + LocatorPtr, > + TRUE > + ); > + break; > + case EFI_ACPI_MPAM_LOCATION_MEMORY: > + PrintGenericLocatorDescriptor ( > + 4, > + L"Memory", > + L"* Proximity domain", > + L"* Reserved", > + LocatorPtr, > + TRUE > + ); > + break; > + case EFI_ACPI_MPAM_LOCATION_SMMU: > + PrintGenericLocatorDescriptor ( > + 4, > + L"SMMU", > + L"* SMMU interface", > + L"* Reserved", > + LocatorPtr, > + TRUE > + ); > + break; > + case EFI_ACPI_MPAM_LOCATION_MEMORY_CACHE: The code would be more generic with ACPI_PARSER structures I think > + // PrintGenericLocatorDescriptor can't be used here as the fields > + // For a memory cache locator descriptor don't fall in the 64bit-3= 2 bit > + // field length division. Parse these fields manually. > + PrintLocatorTitle (4, L"Memory cache"); > + > + // Parse field 1 > + PrintLocatorDescriptor64 ( > + 4, > + L"* Reserved", > + MPAM_MEMORY_LOCATOR_EXTRACT_RESERVED_FIELD ( > + *((UINT64 *)(LocatorPtr)) > + ), > + TRUE > + ); > + > + // Parse field 2 > + PrintLocatorDescriptor64 ( > + 4, > + L"* Level", > + MPAM_MEMORY_LOCATOR_EXTRACT_LEVEL_FIELD ( > + *((UINT64 *)(LocatorPtr)) > + ), > + FALSE > + ); > + > + LocatorPtr +=3D sizeof (UINT64); > + > + // Parse field 3 > + PrintLocatorDescriptor32 ( > + 4, > + L"* Reference", > + *((UINT32 *)(LocatorPtr)), > + FALSE > + ); > + break; > + case EFI_ACPI_MPAM_LOCATION_ACPI_DEVICE: > + // ACPI hardware ID would have to printed via Dump8Chars. > + PrintLocatorTitle (4, L"ACPI device"); > + PrintFieldName (4, L"* ACPI hardware ID"); > + Dump8Chars (NULL, LocatorPtr); > + Print (L"\n"); > + > + LocatorPtr +=3D sizeof (UINT64); > + > + // Parse field 2 > + PrintLocatorDescriptor32 ( > + 4, > + L"* ACPI unique ID", > + *((UINT32 *)(LocatorPtr)), > + FALSE > + ); > + break; > + case EFI_ACPI_MPAM_LOCATION_INTERCONNECT: I m not sure I understand why ParseInterconnectDescriptorTable () is not called from here as the pointer to the struct is available here. > + PrintGenericLocatorDescriptor ( > + 4, > + L"Interconnect", > + L"* Interconnect desc tbl offset", > + L"* Reserved", > + LocatorPtr, > + TRUE > + ); > + break; > + case EFI_ACPI_MPAM_LOCATION_UNKNOWN: > + PrintGenericLocatorDescriptor ( > + 4, > + L"Unknown", > + L"* Descriptor1", > + L"* Descriptor2", > + LocatorPtr, > + FALSE > + ); > + break; > + default: > + Print (L"\nWARNING : Reserved locator type\n"); > + > + IncrementWarningCount (); > + break; > + } // switch > +} > + > +/** > + PrintBlockTitle could be used to print the title of blocks that > + appear more than once in the MPAM ACPI table. > + > + @param [in] Indent Number of spaces to add to the global = table > + indent. The global table indent is 0 b= y > + default; however this value is updated= on > + entry to the ParseAcpi() by adding the= indent > + value provided to ParseAcpi() and rest= ored > + back on exit. Therefore the total ind= ent in > + the output is dependent on from where = this > + function is called. > + @param [in] Title Title string to be used for the block. > + @param [in] Index Index of the block. > +**/ > +STATIC > +VOID > +EFIAPI > +PrintBlockTitle ( > + IN UINT32 Indent, > + IN CONST CHAR16 *Title, > + IN CONST UINT32 Index > + ) > +{ > + Print (L"\n"); > + PrintFieldName (Indent, Title); > + Print (L"%u\n\n", Index); > +} > + > +/** asssociated -> associated I think this might be caught by running the CI locally on the ShellPkg or by making a 'fake' pull request to trigger the edk2 upstream CI. > + This function parses the interconnect descriptor(s) asssociated with > + an interconnect type locator object. > + > + @param [in] Ptr Pointer to the start of the buffer. > + @param [in] AcpiTableLength Length of the ACPI table. > + @param [in] Offset Pointer to current offset within Ptr. > + > +Returns: > + > + Status > + > + EFI_SUCCESS MPAM MSC Nodes were parsed properly. > + EFI_BAD_BUFFER_SIZE The buffer pointer provided as input is= not > + long enough to be parsed correctly. > +**/ > +STATIC > +EFI_STATUS > +EFIAPI > +ParseInterconnectDescriptors ( > + IN UINT8 *Ptr, > + IN UINT32 AcpiTableLength, > + IN UINT32 *Offset > + ) > +{ > + UINT32 InterconnectDescriptorIndex; > + UINT32 Reserved; > + > + InterconnectDescriptorIndex =3D 0; > + > + // Parse MPAM MSC resources within the MSC body I m not sure this case is possible as NumberOfInterconnectDescriptors is st= atically defined. > + if (NumberOfInterconnectDescriptors =3D=3D NULL) { > + MpamLengthError (L"Number of interconnect descriptors not set!"); > + return EFI_BAD_BUFFER_SIZE; > + } > + > + while (InterconnectDescriptorIndex < *NumberOfInterconnectDescriptors)= { > + PrintBlockTitle ( > + 6, > + L"* Interconnect descriptor *", > + InterconnectDescriptorIndex > + ); > + > + // Parse interconnect descriptor > + *Offset +=3D ParseAcpi ( > + TRUE, > + 4, > + NULL, > + Ptr + *Offset, > + AcpiTableLength - *Offset, > + PARSER_PARAMS (MpamInterconnectDescriptorParser) > + ); > + > + Reserved =3D *((UINT32 *)(Ptr + *Offset)) & 0x00FFFFFF; > + PrintFieldName (6, L"Reserved"); > + Print (L"%u\n", Reserved); > + ValidateReserved (Reserved); > + *Offset +=3D 3; > + > + InterconnectDescriptorIndex++; > + } > + > + return EFI_SUCCESS; > +} > + > +/** > + This function parses the interconnect descriptor table asssociated wit= h an > + interconnect type locator object. It also performs necessary validatio= n to > + make sure the interconnect descriptor is at a valid location. > + > + @param [in] Ptr Pointer to the start of the buffer. > + @param [in] AcpiTableLength Length of the ACPI table. > + @param [in] Offset Pointer to current offset within Ptr. > + @param [in] InterconnectOffset Offset to the interconnect descriptor t= able. > + > +Returns: > + > + Status > + > + EFI_SUCCESS MPAM MSC Nodes were parsed properly. > + EFI_BAD_BUFFER_SIZE The buffer pointer provided as input is= not > + long enough to be parsed correctly. > + EFI_INVALID_PARAMETER The Offset parameter encoded within the= Ptr > + buffer is not valid. > +**/ > +STATIC > +EFI_STATUS > +EFIAPI > +ParseInterconnectDescriptorTable ( > + IN UINT8 *Ptr, > + IN UINT32 AcpiTableLength, > + IN UINT32 *Offset, > + IN UINT64 InterconnectOffset > + ) > +{ > + EFI_STATUS Status; > + > + // The specification doesn't strictly state that the interconnect tabl= e be > + // placed exactly after say, functional dependency table or the resour= ce node. > + // Instead, the interconnect locator's descriptor field 1 gives the of= fset > + // from the start of the MSC node where the table could be found. > + if (*Offset > (MpamMscNodeLengthCumulative + > + InterconnectOffset + HeaderSize)) > + { > + IncrementErrorCount (); > + Print (L"\nERROR : Parsing Interconnect descriptor table failed!\n")= ; > + Print ( > + L"ERROR : Offset overlaps with other objects within the MSC. Offse= t %u.\n", > + InterconnectOffset > + ); > + > + return EFI_INVALID_PARAMETER; > + } > + > + if (InterconnectOffset > (*MpamMscNodeLength)) { > + IncrementErrorCount (); > + Print (L"\nERROR : Parsing Interconnect descriptor table failed!\n")= ; > + Print ( > + L"ERROR : Offset falls outside MSC's space. Offset %u.\n", > + InterconnectOffset > + ); > + > + return EFI_INVALID_PARAMETER; > + } > + > + *Offset =3D HeaderSize + MpamMscNodeLengthCumulative + InterconnectOff= set; > + > + Print (L"\n"); > + PrintFieldName (6, L"* Interconnect desc table *"); > + Print (L"\n\n"); > + > + // Parse interconnect descriptor table > + *Offset +=3D ParseAcpi ( > + TRUE, > + 4, > + NULL, > + Ptr + *Offset, > + AcpiTableLength - *Offset, > + PARSER_PARAMS (MpamInterconnectDescriptorTableParser) > + ); > + > + Status =3D ParseInterconnectDescriptors (Ptr, AcpiTableLength, Offset)= ; > + return Status; > +} > + > +/** > + This function parses all the MPAM functional dependency nodes within a > + single resource node. > + > + @param [in] Ptr Pointer to the start of the buffer. > + @param [in] AcpiTableLength Length of the ACPI table. > + @param [in] Offset Pointer to current offset within Ptr. > + > +Returns: > + > + Status > + > + EFI_SUCCESS MPAM MSC Nodes were parsed properly. > + EFI_BAD_BUFFER_SIZE The buffer pointer provided as input is= not > + long enough to be parsed correctly. > +**/ > +STATIC > +EFI_STATUS > +EFIAPI > +ParseMpamMscFunctionalDependencies ( > + IN UINT8 *Ptr, > + IN UINT32 AcpiTableLength, > + IN UINT32 *Offset > + ) > +{ > + UINT32 FunctionalDependencyIndex; > + > + FunctionalDependencyIndex =3D 0; > + > + // Parse MPAM MSC resources within the MSC body I think this should be (*NumberOfFunctionalDependencies =3D=3D 0) > + if (NumberOfFunctionalDependencies =3D=3D NULL) { > + MpamLengthError (L"Number of functional dependencies not set!"); > + return EFI_BAD_BUFFER_SIZE; > + } > + > + while (FunctionalDependencyIndex < *NumberOfFunctionalDependencies) { > + PrintBlockTitle ( > + 6, > + L"* Functional dependency *", > + FunctionalDependencyIndex > + ); > + > + // Parse functional dependency > + *Offset +=3D ParseAcpi ( > + TRUE, > + 4, > + NULL, > + Ptr + *Offset, > + AcpiTableLength - *Offset, > + PARSER_PARAMS (MpamMscFunctionalDependencyParser) > + ); > + > + FunctionalDependencyIndex++; > + } > + > + return EFI_SUCCESS; > +} > + > +/** > + This function parses all the MPAM resource nodes within a single MSC > + node within the MPAM ACPI table. It also invokes helper functions to > + validate and parse locators and functional dependency descriptors. > + > + @param [in] Ptr Pointer to the start of the buffer. > + @param [in] AcpiTableLength Length of the ACPI table. > + @param [in] Offset Pointer to current offset within Ptr. > + > +Returns: > + > + Status > + > + EFI_SUCCESS MPAM MSC Nodes were parsed properly. > + EFI_BAD_BUFFER_SIZE The buffer pointer provided as input is= not > + long enough to be parsed correctly. > +**/ > +STATIC > +EFI_STATUS > +EFIAPI > +ParseMpamMscResources ( > + IN UINT8 *Ptr, > + IN UINT32 AcpiTableLength, > + IN UINT32 *Offset > + ) > +{ > + EFI_STATUS Status; > + UINT64 *InterconnectOffsetPtr; > + UINT32 ResourceIndex; > + > + ResourceIndex =3D 0; > + > + if (NumberOfMscResources =3D=3D NULL) { > + MpamLengthError (L"Number of MSC resource not set!"); > + return EFI_BAD_BUFFER_SIZE; > + } > + > + while (ResourceIndex < *NumberOfMscResources) { > + PrintBlockTitle ( > + 4, > + L"* Resource *", > + ResourceIndex > + ); > + > + // Parse MPAM MSC resources within the MSC body. This could be trace= d. > + *Offset +=3D ParseAcpi ( > + TRUE, > + 2, > + NULL, > + Ptr + *Offset, > + AcpiTableLength - *Offset, > + PARSER_PARAMS (MpamMscResourceParser) > + ); > + I understand that the locator should be printed before the functional depen= dencies, but couldnt't the MpamMscResourceParser and MpamMscResourceLocatorParser st= ructures be merged ? > + // Parse MPAM MSC resources within the MSC body. These fields aren't= traced. > + // ParseLocator would trace and display the fields. > + *Offset +=3D ParseAcpi ( > + FALSE, > + 2, > + NULL, > + Ptr + *Offset, > + AcpiTableLength - *Offset, > + PARSER_PARAMS (MpamMscResourceLocatorParser) > + ); > + Shouldn't it just be counted as an error and proceed ? > + // Proceed with parsing only if a valid locator has been set. > + if ((LocatorType =3D=3D NULL) || (Locator =3D=3D NULL)) { > + MpamLengthError (L"Locator type or Locator not set"); > + return EFI_BAD_BUFFER_SIZE; > + } > + > + ParseLocator (); > + > + // Parse the number of functional dependency descriptors. > + *Offset +=3D ParseAcpi ( > + TRUE, > + 2, > + NULL, > + Ptr + *Offset, > + AcpiTableLength - *Offset, > + PARSER_PARAMS (MpamMscFunctionalDependencyCountParser) > + ); > + > + Status =3D ParseMpamMscFunctionalDependencies (Ptr, AcpiTableLength,= Offset); Without empty line if possible (maybe the comment applies to other places t= oo). > + > + if (Status !=3D EFI_SUCCESS) { > + return Status; > + } > + > + // Reset locator to start of the locator descriptor. > + InterconnectOffsetPtr =3D (UINT64 *)Locator; > + > + // If offset field has been set, parse the interconnect description = table. if (( (without extra spaces) > + if ( (*LocatorType =3D=3D EFI_ACPI_MPAM_LOCATION_INTERCONNECT) > + && (InterconnectOffsetPtr !=3D NULL)) > + { > + Status =3D ParseInterconnectDescriptorTable ( > + Ptr, > + AcpiTableLength, > + Offset, > + *InterconnectOffsetPtr > + ); > + > + if (Status !=3D EFI_SUCCESS) { > + return Status; > + } > + } > + > + ResourceIndex++; > + } > + > + return EFI_SUCCESS; > +} > + > +/** > + This function parses all the MPAM MSC nodes within the MPAM ACPI table= . It > + also invokes a helper function to detect and parse resource nodes that= maybe > + present. > + > + @param [in] Ptr Pointer to the start of the buffer. > + @param [in] AcpiTableLength Length of the ACPI table. > + @param [in] Offset Current offset within Ptr. > + > +Returns: > + > + Status > + > + EFI_SUCCESS MPAM MSC Nodes were parsed properly. > + EFI_BAD_BUFFER_SIZE The buffer pointer provided as input is= not > + long enough to be parsed correctly. > +**/ > +STATIC > +EFI_STATUS > +EFIAPI > +ParseMpamMscNodes ( > + IN UINT8 *Ptr, > + IN UINT32 AcpiTableLength, > + IN UINT32 Offset > + ) > +{ > + EFI_STATUS Status; > + UINT32 MscIndex; > + > + MscIndex =3D 0; > + > + while (Offset < AcpiTableLength) { > + PrintBlockTitle (2, L"* Msc *", MscIndex); > + // Parse MPAM msc node > + Offset +=3D ParseAcpi ( > + TRUE, > + 0, > + NULL, > + Ptr + Offset, > + AcpiTableLength - Offset, > + PARSER_PARAMS (MpamMscNodeParser) > + ); > + > + // Parse MPAM MSC resources within the MSC body I m not sure this case is possible as MpamMscNodeLength is statically defined. Maybe it should be (*MpamMscNodeLength =3D=3D 0). > + if (MpamMscNodeLength =3D=3D NULL) { > + MpamLengthError (L"MPAM MSC node length not set!"); > + return EFI_BAD_BUFFER_SIZE; > + } > + > + Status =3D ParseMpamMscResources (Ptr, AcpiTableLength, &Offset); Without empty line if possible. > + > + if (Status !=3D EFI_SUCCESS) { > + return Status; > + } > + > + MpamMscNodeLengthCumulative +=3D (*MpamMscNodeLength); > + MscIndex++; > + } > + > + return EFI_SUCCESS; > +} > + > +/** > + This function parses the MPAM ACPI table's generic header. It also inv= okes a > + sub routine that would help with parsing rest of the table. > + > + @param [in] Trace If TRUE, trace the ACPI fields. > + @param [in] Ptr Pointer to the start of the buffer. > + @param [in] AcpiTableLength Length of the ACPI table. > + @param [in] AcpiTableRevision Revision of the ACPI table. > +**/ > +VOID > +EFIAPI > +ParseAcpiMpam ( > + IN BOOLEAN Trace, > + IN UINT8 *Ptr, > + IN UINT32 AcpiTableLength, > + IN UINT8 AcpiTableRevision > + ) > +{ > + EFI_STATUS Status; > + > + if (!Trace) { > + return; > + } > + > + // Parse generic table header > + HeaderSize =3D ParseAcpi ( > + TRUE, > + 0, > + "MPAM", > + Ptr, > + AcpiTableLength, > + PARSER_PARAMS (MpamParser) > + ); > + > + Status =3D ParseMpamMscNodes (Ptr, AcpiTableLength, HeaderSize); > + > + if (Status =3D=3D EFI_SUCCESS) { > + // Check if the length of all MPAM MSCs with the header, matches wit= h the > + // ACPI table's length field. > + if (*(AcpiHdrInfo.Length) !=3D (MpamMscNodeLengthCumulative + Header= Size)) { > + IncrementErrorCount (); > + Print (L"\nERROR: Length mismatch! : "); > + Print (L"Msc Length total !=3D MPAM table length."); > + Print ( > + L"table length : %u Msc total : %u\n", > + *(AcpiHdrInfo.Length), > + MpamMscNodeLengthCumulative > + ); > + } > + } > +} > diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Mpam/Mp= amParser.h b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Mpam/Mpam= Parser.h > new file mode 100644 > index 0000000000..bc6eef8c42 > --- /dev/null > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Mpam/MpamParse= r.h > @@ -0,0 +1,25 @@ > +/** @file > + MPAM table parser > + > + Copyright (c) 2023, ARM Limited. All rights reserved. > + SPDX-License-Identifier: BSD-2-Clause-Patent > + > + @par Specification Reference: > + - [1] ACPI for Memory System Resource Partitioning and Monitoring 2.0 > + (https://developer.arm.com/documentation/den0065/latest) > + **/ > + > +#ifndef MPAM_PARSER_H_ > +#define MPAM_PARSER_H_ > + > +#include > + > +#define MPAM_MEMORY_LOCATOR_EXTRACT_RESERVED_FIELD(field) = \ > + ((field & EFI_ACPI_MPAM_MEM_CACHE_LOCATOR_RESERVED_FIELD_MASK) >> = \ > + EFI_ACPI_MPAM_MEM_CACHE_LOCATOR_RESERVED_FIELD_SHIFT) > + > +#define MPAM_MEMORY_LOCATOR_EXTRACT_LEVEL_FIELD(field) = \ > + ((field & EFI_ACPI_MPAM_MEM_CACHE_LOCATOR_LEVEL_FIELD_MASK) >> = \ > + EFI_ACPI_MPAM_MEM_CACHE_LOCATOR_LEVEL_FIELD_SHIFT) > + > +#endif // MPAM_PARSER_H_ > diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiVi= ewCommandLib.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpi= ViewCommandLib.c > index bbac125bb9..3887174361 100644 > --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewComma= ndLib.c > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewComma= ndLib.c > @@ -2,7 +2,7 @@ > Main file for 'acpiview' Shell command function. > =20 > Copyright (c) 2022, NVIDIA CORPORATION. All rights reserved. > - Copyright (c) 2016 - 2021, Arm Limited. All rights reserved.
> + Copyright (c) 2016 - 2023, Arm Limited. All rights reserved.
> SPDX-License-Identifier: BSD-2-Clause-Patent > **/ > =20 > @@ -63,6 +63,7 @@ ACPI_TABLE_PARSER ParserList[] =3D { > { EFI_ACPI_6_2_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE, = ParseAcpiMadt }, > { EFI_ACPI_6_2_PCI_EXPRESS_MEMORY_MAPPED_CONFIGURATION_SPACE_BASE_ADD= RESS_DESCRIPTION_TABLE_SIGNATURE, > ParseAcpiMcfg }, > + { EFI_ACPI_MEMORY_SYSTEM_RESOURCE_PARTITIONING_AND_MONITORING_TABLE_SI= GNATURE, ParseAcpiMpam }, > { EFI_ACPI_6_4_PLATFORM_COMMUNICATIONS_CHANNEL_TABLE_SIGNATURE, > ParseAcpiPcct }, > { EFI_ACPI_6_4_PROCESSOR_PROPERTIES_TOPOLOGY_TABLE_STRUCTURE_SIGNATUR= E, > diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiVi= ewCommandLib.inf b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAc= piViewCommandLib.inf > index a38965a4bf..f30315bbf5 100644 > --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewComma= ndLib.inf > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewComma= ndLib.inf > @@ -2,7 +2,7 @@ > # Provides Shell 'acpiview' command functions > # > # Copyright (c) 2022, NVIDIA CORPORATION. All rights reserved. > -# Copyright (c) 2016 - 2020, Arm Limited. All rights reserved.
> +# Copyright (c) 2016 - 2023, Arm Limited. All rights reserved.
> # > # SPDX-License-Identifier: BSD-2-Clause-Patent > # > @@ -42,6 +42,8 @@ > Parsers/Madt/MadtParser.c > Parsers/Madt/MadtParser.h > Parsers/Mcfg/McfgParser.c > + Parsers/Mpam/MpamParser.c > + Parsers/Mpam/MpamParser.h > Parsers/Pcct/PcctParser.c > Parsers/Pcct/PcctParser.h > Parsers/Pptt/PpttParser.c > diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiVi= ewCommandLib.uni b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAc= piViewCommandLib.uni > index e4a9dd5b40..4079a8e51e 100644 > --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewComma= ndLib.uni > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewComma= ndLib.uni > @@ -1,6 +1,6 @@ > // /** > // > -// Copyright (c) 2016 - 2020, Arm Limited. All rights reserved.
> +// Copyright (c) 2016 - 2023, Arm Limited. All rights reserved.
> // SPDX-License-Identifier: BSD-2-Clause-Patent > // > // Module Name: > @@ -89,6 +89,7 @@ > " HMAT - Heterogeneous Memory Attributes Table\r\n" > " IORT - IO Remapping Table\r\n" > " MCFG - Memory Mapped Config Space Base Address Description Tab= le\r\n" > +" MPAM - Memory System Resource Partitioning and Monitoring Table= \r\n" > " PPTT - Processor Properties Topology Table\r\n" > " RSDP - Root System Description Pointer\r\n" > " SLIT - System Locality Information Table\r\n" Regards, Pierre -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#107102): https://edk2.groups.io/g/devel/message/107102 Mute This Topic: https://groups.io/mt/99066188/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-