From: "PierreGondois" <pierre.gondois@arm.com>
To: devel@edk2.groups.io, rohit.mathew@arm.com
Cc: Thomas Abraham <thomas.abraham@arm.com>,
Sami Mujawar <sami.mujawar@arm.com>,
James Morse <james.morse@arm.com>, Ray Ni <ray.ni@intel.com>,
Zhichao Gao <zhichao.gao@intel.com>
Subject: Re: [edk2-devel] [PATCH V3 3/3] ShellPkg/AcpiView: Add MPAM Parser
Date: Thu, 20 Jul 2023 17:24:50 +0200 [thread overview]
Message-ID: <e0577d2f-2b0f-eff1-771c-42293b00f5c6@arm.com> (raw)
In-Reply-To: <20230522144506.2799121-1-Rohit.Mathew@arm.com>
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.
>
> Signed-off-by: Rohit Mathew <Rohit.Mathew@arm.com>
> ---
> 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/UefiShellAcpiViewCommandLib.c | 3 +-
> ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.inf | 4 +-
> ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.uni | 3 +-
> 6 files changed, 1384 insertions(+), 3 deletions(-)
>
> 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
> );
>
> +/**
> + 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/MpamParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Mpam/MpamParser.c
> new file mode 100644
> index 0000000000..9352357318
> --- /dev/null
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Mpam/MpamParser.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 <IndustryStandard/Mpam.h>
> +#include <Library/PrintLib.h>
> +#include <Library/UefiLib.h>
> +#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 function 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 should 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 field
> + 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, cf.
ValidateSratReserved(). Maybe it would be good to re-use your generic implementation
there aswell (just a suggestion, maybe to be done in a later patch).
> +ValidateReserved (
> + IN CONST UINT64 Reserved
> + )
> +{
> + if (Reserved != 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 this
> + particular function, context holds the size of the
> + 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 requirements.
> + if (Context == NULL) {
> + return;
> + }
> +
> + // Only those cases which are handled currently have been implemented.
> + switch ((UINT64)Context) {
> + case 1:
> + Reserved = *Ptr;
> + break;
> + case 2:
> + if (((UINT64)Ptr) % 2 == 0) {
> + Reserved = *((UINT16 *)Ptr);
> + } else {
> + Reserved = (*(Ptr + 1) << 8);
> + Reserved |= *Ptr;
> + }
> +
> + break;
> + case 4:
> + if (((UINT64)Ptr) % 4 == 0) {
> + Reserved = *((UINT32 *)Ptr);
> + } else {
> + Reserved = (*(Ptr + 3) << 24);
> + Reserved |= (*(Ptr + 2) << 16);
> + Reserved |= (*(Ptr + 1) << 8);
> + Reserved |= (*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 this
> + function is a string.
> +
> + @param [in] Indent Number of spaces to add to the global table
> + indent. The global table indent is 0 by
> + default; however this value is updated on
> + entry to the ParseAcpi() by adding the indent
> + value provided to ParseAcpi() and restored
> + back on exit. Therefore the total indent 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 FieldStr.
> +**/
> +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 MPAM ACPI
> + table. MPAM ACPI specification states that the MMIO size for an MSC having 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 this
> + function, context holds the parent/double pointer 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 = (UINT8 **)Context;
I don't think these comments are necessary as this is generic for acpiview.
> + // Context - double pointer to interface type. (Refer MpamMscNodeParse
> + // table).
> + // *Context - address of object holding interface type.
> + // **Context - interface type.
> + if (*InterfaceTypeParentPtr == NULL) {
> + MpamLengthError (L"Interface type not set!");
> + return;
> + }
> +
> + InterfaceType = **InterfaceTypeParentPtr;
> +> + if (InterfaceType == EFI_ACPI_MPAM_INTERFACE_PCC) {
> + MmioSize = *((UINT32 *)Ptr);
> +
> + if (MmioSize != 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 interconnect
> + 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 this
> + function, context is ignored.
> +**/
> +STATIC
> +VOID
> +EFIAPI
> +DecodeLinkType (
> + IN UINT8 *Ptr,
> + IN VOID *Context
> + )
> +{
> + UINT8 LinkType;
> +
> + LinkType = *Ptr;
> +
> + if (LinkType == 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 == 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 table.
> + 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 this
> + 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 divisible by 8.
> + // Proceed to dereference 8 bytes in one go.
> + HardwareId = *((UINT64 *)Ptr);
> +
> + if (HardwareId != 0) {
> + Print (L"\n");
Is it necessary to print '* Hardware ID type decoded' as the field name should
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 this
> + function, context is ignored.
> +**/
> +STATIC
> +VOID
> +EFIAPI
> +DecodeInterfaceType (
> + IN UINT8 *Ptr,
> + IN VOID *Context
> + )
> +{
> + UINT8 InterfaceType;
> +
> + InterfaceType = *Ptr;
> +
> + if (InterfaceType == EFI_ACPI_MPAM_INTERFACE_MMIO) {
> + PrintDecodedField (
> + 2,
> + L"* Interface type decoded",
> + L"MMIO"
> + );
> + } else if (InterfaceType == 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 this
> + function, context holds the parent/double pointer 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 = (UINT32 **)Context;
> + // Context - double pointer to the gsiv object. (Refer MpamMscNodeParse
> + // table).
> + // *Context - pointer to gsiv object.
> + // **Context - gsiv.
> + if (*InterruptGsivParentPtr == NULL) {
> + MpamLengthError (L"Interrupt gsiv not set!");
> + return;
> + }
> +
> + InterruptGsiv = **InterruptGsivParentPtr;
> +
> + // If Interrupts are absent for some reason, the flags wouldn't need any
> + // parsing.
> + if (InterruptGsiv == 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 = *((UINT32 *)Ptr);
> +
> + // Decode interrupt mode.
> + InterruptMode = (InterruptFlag & EFI_ACPI_MPAM_INTERRUPT_MODE_MASK)
> + >> EFI_ACPI_MPAM_INTERRUPT_MODE_SHIFT;
> +
> + if (InterruptMode == 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 = (InterruptFlag & EFI_ACPI_MPAM_INTERRUPT_TYPE_MASK)
> + >> EFI_ACPI_MPAM_INTERRUPT_TYPE_SHIFT;
> +
> + if (InterruptType == 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 = (InterruptFlag
> + & EFI_ACPI_MPAM_INTERRUPT_AFFINITY_VALID_MASK)
> + >> EFI_ACPI_MPAM_INTERRUPT_AFFINITY_VALID_SHIFT;
> +
> + if (InterruptAffinityValid != 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 = (InterruptFlag
> + & EFI_ACPI_MPAM_INTERRUPT_AFFINITY_TYPE_MASK)
> + >> EFI_ACPI_MPAM_INTERRUPT_AFFINITY_TYPE_SHIFT;
> +
> + if (InterruptAffinityType == EFI_ACPI_MPAM_INTERRUPT_PROCESSOR_AFFINITY) {
> + 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 = (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[] = {
> + PARSE_ACPI_HEADER (&AcpiHdrInfo)
> +};
> +
> +/**
> + ACPI_PARSER array describing the MPAM MSC node object.
> +**/
> +STATIC CONST ACPI_PARSER MpamMscNodeParser[] = {
> + { 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 check 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 validator
> + // 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 validator
> + // 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[] = {
> + { 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[] = {
> + { L"Locator", 12, 0, NULL, NULL, (VOID **)&Locator, NULL,
> + NULL }
> +};
> +
> +/**
> + ACPI_PARSER array describing the MPAM MSC resource's functional dependencies
> + count.
> +**/
> +STATIC CONST ACPI_PARSER MpamMscFunctionalDependencyCountParser[] = {
> + { L"Number of func dependencies", 4, 0, L"%u", NULL,
> + (VOID **)&NumberOfFunctionalDependencies, NULL, NULL }
> +};
> +
> +/**
> + ACPI_PARSER array describing the MPAM MSC resource's functional dependencies.
> +**/
> +STATIC CONST ACPI_PARSER MpamMscFunctionalDependencyParser[] = {
> + { 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 associated with
> + the interconnect locator type.
> +**/
> +STATIC CONST ACPI_PARSER MpamInterconnectDescriptorTableParser[] = {
> + { L"Signature", 16, 0,
> + L"%x%x%x%x-%x%x-%x%x-%x%x-%x%x%x%x%x%x", Dump16Chars, NULL, NULL, NULL },
> + { L"Number of Interconnect desc", 4, 16,L"0x%x", NULL,
> + (VOID **)&NumberOfInterconnectDescriptors, NULL, NULL }
> +};
> +
> +/**
> + ACPI_PARSER array describing the interconnect descriptor associated with the
> + interconnect locator type. The specification includes an additional reserved
> + 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[] = {
> + { 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 by
> + default; however this value is updated on
> + entry to the ParseAcpi() by adding the indent
> + value provided to ParseAcpi() and restored
> + back on exit. Therefore the total indent in
> + the output is dependent on from where this
> + function is called.
> + @param [in] LocatorTitle Title string to be used for the locator.
> +**/
> +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 by
> + default; however this value is updated on
> + entry to the ParseAcpi() by adding the indent
> + value provided to ParseAcpi() and restored
> + back on exit. Therefore the total indent in
> + the output is dependent on from where this
> + function is called.
> + @param [in] DescriptorTitle Title string to be used for the descriptor.
> + @param [in] Descriptor64 Descriptor to be printed.
> + @param [in] Validate Boolean to indicate if the second 4 byte field
> + needs to be validated as a reserved field.
> +**/
> +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 by
> + default; however this value is updated on
> + entry to the ParseAcpi() by adding the indent
> + value provided to ParseAcpi() and restored
> + back on exit. Therefore the total indent in
> + the output is dependent on from where this
> + function is called.
> + @param [in] DescriptorTitle Title string to be used for the descriptor.
> + @param [in] Descriptor32 Descriptor to be printed.
> + @param [in] Validate Boolean to indicate if the second 4 byte field
> + needs to be validated as a reserved field.
> +**/
> +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 LocatorPtr
> + field is casted to (UINT64 *) and decoded within the macro. From the MPAM 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 and 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 by
> + default; however this value is updated on
> + entry to the ParseAcpi() by adding the indent
> + value provided to ParseAcpi() and restored
> + back on exit. Therefore the total indent in
> + the output is dependent on from where this
> + function is called.
> + @param [in] LocatorTitle Title string to be used for the locator
> + @param [in] Descriptor1Title Title string to be used for descriptor1
> + @param [in] Descriptor2Title Title string to be used for descriptor2
> + @param [in] LocatorPtr Ptr to the start of locator
> + @param [in] Validate Boolean to indicate if the second 4 byte field
> + needs to be validated as a reserved field.
> +**/
> +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 += sizeof (UINT64);
> +
> + PrintLocatorDescriptor32 (
> + Indent,
> + Descriptor2Title,
> + *((UINT64 *)LocatorPtr),
> + Validate
> + );
> +}
> +
> +/**
> + This function parses the locator field within the resource node for ACPI 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 = Locator;
> +
> + switch (*LocatorType) {
I think it would be simpler to define names as:
STATIC CONST CHAR16 *MpamLocationNames[] = {
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[] = {
{ L"SMMU interface", 8, 0, L"%lu", NULL, NULL, NULL, NULL },
{ L"Reserved ID", 4, 8, L"%u", NULL, NULL, ValidateReservedGeneric, (VOID *)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-32 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 += 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 += 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 by
> + default; however this value is updated on
> + entry to the ParseAcpi() by adding the indent
> + value provided to ParseAcpi() and restored
> + back on exit. Therefore the total indent 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 = 0;
> +
> + // Parse MPAM MSC resources within the MSC body
I m not sure this case is possible as NumberOfInterconnectDescriptors is statically defined.
> + if (NumberOfInterconnectDescriptors == 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 += ParseAcpi (
> + TRUE,
> + 4,
> + NULL,
> + Ptr + *Offset,
> + AcpiTableLength - *Offset,
> + PARSER_PARAMS (MpamInterconnectDescriptorParser)
> + );
> +
> + Reserved = *((UINT32 *)(Ptr + *Offset)) & 0x00FFFFFF;
> + PrintFieldName (6, L"Reserved");
> + Print (L"%u\n", Reserved);
> + ValidateReserved (Reserved);
> + *Offset += 3;
> +
> + InterconnectDescriptorIndex++;
> + }
> +
> + return EFI_SUCCESS;
> +}
> +
> +/**
> + This function parses the interconnect descriptor table asssociated with an
> + interconnect type locator object. It also performs necessary validation 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 table.
> +
> +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 table be
> + // placed exactly after say, functional dependency table or the resource node.
> + // Instead, the interconnect locator's descriptor field 1 gives the offset
> + // 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. Offset %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 = HeaderSize + MpamMscNodeLengthCumulative + InterconnectOffset;
> +
> + Print (L"\n");
> + PrintFieldName (6, L"* Interconnect desc table *");
> + Print (L"\n\n");
> +
> + // Parse interconnect descriptor table
> + *Offset += ParseAcpi (
> + TRUE,
> + 4,
> + NULL,
> + Ptr + *Offset,
> + AcpiTableLength - *Offset,
> + PARSER_PARAMS (MpamInterconnectDescriptorTableParser)
> + );
> +
> + Status = 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 = 0;
> +
> + // Parse MPAM MSC resources within the MSC body
I think this should be (*NumberOfFunctionalDependencies == 0)
> + if (NumberOfFunctionalDependencies == 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 += 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 = 0;
> +
> + if (NumberOfMscResources == 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 traced.
> + *Offset += ParseAcpi (
> + TRUE,
> + 2,
> + NULL,
> + Ptr + *Offset,
> + AcpiTableLength - *Offset,
> + PARSER_PARAMS (MpamMscResourceParser)
> + );
> +
I understand that the locator should be printed before the functional dependencies,
but couldnt't the MpamMscResourceParser and MpamMscResourceLocatorParser structures
be merged ?
> + // Parse MPAM MSC resources within the MSC body. These fields aren't traced.
> + // ParseLocator would trace and display the fields.
> + *Offset += 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 == NULL) || (Locator == NULL)) {
> + MpamLengthError (L"Locator type or Locator not set");
> + return EFI_BAD_BUFFER_SIZE;
> + }
> +
> + ParseLocator ();
> +
> + // Parse the number of functional dependency descriptors.
> + *Offset += ParseAcpi (
> + TRUE,
> + 2,
> + NULL,
> + Ptr + *Offset,
> + AcpiTableLength - *Offset,
> + PARSER_PARAMS (MpamMscFunctionalDependencyCountParser)
> + );
> +
> + Status = ParseMpamMscFunctionalDependencies (Ptr, AcpiTableLength, Offset);
Without empty line if possible (maybe the comment applies to other places too).
> +
> + if (Status != EFI_SUCCESS) {
> + return Status;
> + }
> +
> + // Reset locator to start of the locator descriptor.
> + InterconnectOffsetPtr = (UINT64 *)Locator;
> +
> + // If offset field has been set, parse the interconnect description table.
if ((
(without extra spaces)
> + if ( (*LocatorType == EFI_ACPI_MPAM_LOCATION_INTERCONNECT)
> + && (InterconnectOffsetPtr != NULL))
> + {
> + Status = ParseInterconnectDescriptorTable (
> + Ptr,
> + AcpiTableLength,
> + Offset,
> + *InterconnectOffsetPtr
> + );
> +
> + if (Status != 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 = 0;
> +
> + while (Offset < AcpiTableLength) {
> + PrintBlockTitle (2, L"* Msc *", MscIndex);
> + // Parse MPAM msc node
> + Offset += 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 == 0).
> + if (MpamMscNodeLength == NULL) {
> + MpamLengthError (L"MPAM MSC node length not set!");
> + return EFI_BAD_BUFFER_SIZE;
> + }
> +
> + Status = ParseMpamMscResources (Ptr, AcpiTableLength, &Offset);
Without empty line if possible.
> +
> + if (Status != EFI_SUCCESS) {
> + return Status;
> + }
> +
> + MpamMscNodeLengthCumulative += (*MpamMscNodeLength);
> + MscIndex++;
> + }
> +
> + return EFI_SUCCESS;
> +}
> +
> +/**
> + This function parses the MPAM ACPI table's generic header. It also invokes 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 = ParseAcpi (
> + TRUE,
> + 0,
> + "MPAM",
> + Ptr,
> + AcpiTableLength,
> + PARSER_PARAMS (MpamParser)
> + );
> +
> + Status = ParseMpamMscNodes (Ptr, AcpiTableLength, HeaderSize);
> +
> + if (Status == EFI_SUCCESS) {
> + // Check if the length of all MPAM MSCs with the header, matches with the
> + // ACPI table's length field.
> + if (*(AcpiHdrInfo.Length) != (MpamMscNodeLengthCumulative + HeaderSize)) {
> + IncrementErrorCount ();
> + Print (L"\nERROR: Length mismatch! : ");
> + Print (L"Msc Length total != MPAM table length.");
> + Print (
> + L"table length : %u Msc total : %u\n",
> + *(AcpiHdrInfo.Length),
> + MpamMscNodeLengthCumulative
> + );
> + }
> + }
> +}
> diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Mpam/MpamParser.h b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Mpam/MpamParser.h
> new file mode 100644
> index 0000000000..bc6eef8c42
> --- /dev/null
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Mpam/MpamParser.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 <IndustryStandard/Mpam.h>
> +
> +#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/UefiShellAcpiViewCommandLib.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.c
> index bbac125bb9..3887174361 100644
> --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.c
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.c
> @@ -2,7 +2,7 @@
> Main file for 'acpiview' Shell command function.
>
> Copyright (c) 2022, NVIDIA CORPORATION. All rights reserved.
> - Copyright (c) 2016 - 2021, Arm Limited. All rights reserved.<BR>
> + Copyright (c) 2016 - 2023, Arm Limited. All rights reserved.<BR>
> SPDX-License-Identifier: BSD-2-Clause-Patent
> **/
>
> @@ -63,6 +63,7 @@ ACPI_TABLE_PARSER ParserList[] = {
> { EFI_ACPI_6_2_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE, ParseAcpiMadt },
> { EFI_ACPI_6_2_PCI_EXPRESS_MEMORY_MAPPED_CONFIGURATION_SPACE_BASE_ADDRESS_DESCRIPTION_TABLE_SIGNATURE,
> ParseAcpiMcfg },
> + { EFI_ACPI_MEMORY_SYSTEM_RESOURCE_PARTITIONING_AND_MONITORING_TABLE_SIGNATURE, ParseAcpiMpam },
> { EFI_ACPI_6_4_PLATFORM_COMMUNICATIONS_CHANNEL_TABLE_SIGNATURE,
> ParseAcpiPcct },
> { EFI_ACPI_6_4_PROCESSOR_PROPERTIES_TOPOLOGY_TABLE_STRUCTURE_SIGNATURE,
> diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.inf b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.inf
> index a38965a4bf..f30315bbf5 100644
> --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.inf
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.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.<BR>
> +# Copyright (c) 2016 - 2023, Arm Limited. All rights reserved.<BR>
> #
> # 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/UefiShellAcpiViewCommandLib.uni b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.uni
> index e4a9dd5b40..4079a8e51e 100644
> --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.uni
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.uni
> @@ -1,6 +1,6 @@
> // /**
> //
> -// Copyright (c) 2016 - 2020, Arm Limited. All rights reserved.<BR>
> +// Copyright (c) 2016 - 2023, Arm Limited. All rights reserved.<BR>
> // 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 Table\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
-=-=-=-=-=-=-=-=-=-=-=-
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]
-=-=-=-=-=-=-=-=-=-=-=-
next prev parent reply other threads:[~2023-07-20 15:25 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-22 14:45 [PATCH V3 3/3] ShellPkg/AcpiView: Add MPAM Parser Rohit Mathew
2023-07-20 15:24 ` PierreGondois [this message]
2023-07-24 9:50 ` [edk2-devel] " Rohit Mathew
2023-07-24 14:40 ` PierreGondois
2023-07-24 16:07 ` Rohit Mathew
2023-07-25 13:55 ` PierreGondois
2023-07-31 20:14 ` Rohit Mathew
2023-08-04 11:20 ` PierreGondois
2023-08-07 12:45 ` Rohit Mathew
2023-08-08 17:28 ` Rohit Mathew
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e0577d2f-2b0f-eff1-771c-42293b00f5c6@arm.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox