Hi Rohit, Thank you for this patch. Please find my feedback inline marked [SAMI]. Regards, Sami Mujawar On 18/08/2023 12:48 pm, Rohit Mathew 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 > Cc: James Morese > Cc: Sami Mujawar > Cc: Thomas Abraham > Cc: Zhichao Gao > --- > ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h | 21 + > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Mpam/MpamParser.c | 1256 ++++++++++++++++++++ > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Mpam/MpamParser.h | 31 + > ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.c | 3 +- > ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.inf | 4 +- > ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.uni | 3 +- > 6 files changed, 1315 insertions(+), 3 deletions(-) > > diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h > index c9f41650d9..0c46ae7117 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 MPAM 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..43b70328b0 > --- /dev/null > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Mpam/MpamParser.c > @@ -0,0 +1,1256 @@ > +/** @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 CONST UINT8 *ResourceLocatorType; > +STATIC CONST UINT16 *MpamMscNodeLength; > +STATIC UINT32 MpamMscNodeLengthCumulative; > +STATIC UINT32 HeaderSize; > +STATIC CONST UINT32 *MscInterfaceType; > +STATIC CONST UINT32 *NumberOfMscResources; > +STATIC CONST UINT32 *NumberOfFunctionalDependencies; > +STATIC CONST UINT32 *NumberOfInterconnectDescriptors; > +STATIC CONST UINT64 *InterconnectTableOffset; > +STATIC ACPI_DESCRIPTION_HEADER_INFO AcpiHdrInfo; > + > +// Array of reserved sizes for context parameter. > +STATIC RESERVED_SIZE_T ReservedSize[] = { > + { 1 }, { 2 }, { 3 }, { 4 }, { 5 }, { 6 }, { 7 }, { 8 } > +}; > + > +// Array of locator type names. New types should be added keeping the order > +// preserved as locator type is used to index into the array while parsing. > +STATIC CONST CHAR16 *MpamMscLocatorTitles[] = { > + L"Processor cache", > + L"Memory", > + L"SMMU", > + L"Memory cache", > + L"ACPI device", > + L"Interconnect" > +}; > + > +/** > + 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 > + main error log. This string could explain the reason > + why a insufficient length error was encountered in > + the first place. > +**/ > +STATIC > +VOID > +EFIAPI > +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 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 > +ValidateReserved ( > + IN UINT8 *Ptr, [SAMI] I think it would be better to extend the FieldValidator to include the length of the field being validated.  I think that should remove the requirement for RESERVED_SIZE_T (which by the way is a very confusing name). This would mean that thecode in ParseAcpi() at line 656 in file AcpiParser.c would need the following change. -Parser[Index].FieldValidator (Ptr, Parser[Index].Context); +Parser[Index].FieldValidator (Ptr, Parser[Index].Length, Parser[Index].Context); Corresponding updates would be needed to FNPTR_FIELD_VALIDATOR and the usage across all Parsers. -typedefVOID (EFIAPI *FNPTR_FIELD_VALIDATOR)(UINT8 *Ptr, VOID *Context); +typedefVOID (EFIAPI *FNPTR_FIELD_VALIDATOR)(UINT8 *Ptr, UINTN Length, VOID *Context); I think that would be a good overall improvement to the AcpiView Parsers. It would also be beneficial to extend FNPTR_PRINT_FORMATTER on similar lines. -typedefVOID (EFIAPI *FNPTR_PRINT_FORMATTER)(CONST CHAR16 *Format, UINT8 *Ptr); +typedefVOID (EFIAPI *FNPTR_PRINT_FORMATTER)(CONST CHAR16 *Format, UINT8 *Ptr, UINTN Length); [/SAMI] > + IN VOID *Context > + ) > +{ > + UINT64 Reserved; > + RESERVED_SIZE_T *ResSize; > + > + Reserved = 0; > + > + // If Context is not passed on, don't validate. Without the length, it is not > + // possible to decode the reserved field. > + if (Context == NULL) { > + return; > + } > + > + ResSize = (RESERVED_SIZE_T *)Context; > + > + // Only those cases which are handled currently have been implemented. > + switch (ResSize->Size) { > + case 8: > + Reserved = (((UINT64)*(Ptr + 7)) << 56); > + // fall through > + case 7: > + Reserved |= (((UINT64)*(Ptr + 6)) << 48); > + // fall through > + case 6: > + Reserved |= (((UINT64)*(Ptr + 5)) << 40); > + // fall through > + case 5: > + Reserved |= (((UINT64)*(Ptr + 4)) << 32); > + // fall through > + case 4: > + Reserved |= (*(Ptr + 3) << 24); > + // fall through > + case 3: > + Reserved |= (*(Ptr + 2) << 16); > + // fall through > + case 2: > + Reserved |= (*(Ptr + 1) << 8); > + // fall through > + case 1: > + Reserved |= (*Ptr); > + break; > + default: > + return; > + } > + > + if (Reserved != 0) { > + IncrementErrorCount (); > + Print (L"\nERROR : Reserved field should be 0\n"); > + } > +} > + > +/** > + 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; > + > + 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) { > + Print ( > + L" (NUMA)" > + ); > + } else if (LinkType == EFI_ACPI_MPAM_LINK_TYPE_PROC) { > + Print ( > + 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; > + > + HardwareId = *((UINT64 *)Ptr); > + > + if (HardwareId != 0) { > + Print (L" ("); > + Dump8Chars (NULL, Ptr); > + Print (L")"); > + } > +} > + > +/** > + 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) { > + Print (L" (MMIO)"); > + } else if (InterfaceType == EFI_ACPI_MPAM_INTERFACE_PCC) { > + Print (L" (PCC)"); > + } else { > + IncrementErrorCount (); > + Print ( > + L"\nERROR: Invalid interface type - %u\n", > + (UINT32)InterfaceType > + ); > + } > +} > + > +/** > + This function decodes the interrupt mode flag for MPAM. Interrupt mode could > + either be "edge triggered" or "level triggered". > + > + @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 > +DecodeInterruptMode ( > + IN UINT8 *Ptr, > + IN VOID *Context > + ) > +{ > + UINT8 InterruptMode; > + > + InterruptMode = *Ptr; > + > + if (InterruptMode == EFI_ACPI_MPAM_INTERRUPT_LEVEL_TRIGGERED) { > + Print (L" (Level triggered)"); > + } else { > + Print (L" (Edge triggered)"); > + } > +} > + > +/** > + This function decodes the interrupt type flag for MPAM. Interrupt type could > + be "wired interrupt". Other values are reserved at this point. > + > + @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 > +DecodeInterruptType ( > + IN UINT8 *Ptr, > + IN VOID *Context > + ) > +{ > + UINT8 InterruptType; > + > + InterruptType = *Ptr; > + > + if (InterruptType == EFI_ACPI_MPAM_INTERRUPT_WIRED) { > + Print (L" (Wired interrupt)"); > + } else { > + IncrementWarningCount (); > + Print (L" (Reserved value!)"); > + } > +} > + > +/** > + This function decodes the interrupt affinity valid flag for MPAM. Interrupt > + affinity could be either be valid or not. > + > + @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 > +DecodeInterruptAffinityValid ( > + IN UINT8 *Ptr, > + IN VOID *Context > + ) > +{ > + UINT8 InterruptAffinityValid; > + > + InterruptAffinityValid = *Ptr; > + > + if (InterruptAffinityValid != EFI_ACPI_MPAM_INTERRUPT_AFFINITY_VALID) { > + Print (L" (Affinity not valid)"); > + } else { > + Print (L" (Affinity valid)"); > + } > +} > + > +/** > + This function decodes the interrupt affinity type flag for MPAM. Interrupt > + affinity type could either be "Processor affinity" or "Processor container > + affinity" > + > + @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 > +DecodeInterruptAffinityType ( > + IN UINT8 *Ptr, > + IN VOID *Context > + ) > +{ > + UINT8 InterruptAffinityType; > + > + InterruptAffinityType = *Ptr; > + > + if (InterruptAffinityType == EFI_ACPI_MPAM_INTERRUPT_PROCESSOR_AFFINITY) { > + Print (L" (Processor affinity)"); > + } else { > + Print (L" (Processor container affinity)"); > + } > +} > + > +/** > + This function decodes the locator type for a particular MPAM MSC resource. > + > + @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 > +DecodeLocatorType ( > + IN UINT8 *Ptr, > + IN VOID *Context > + ) > +{ > + UINT8 LocatorType; > + > + LocatorType = *Ptr; > + > + if (LocatorType <= EFI_ACPI_MPAM_LOCATION_INTERCONNECT) { > + Print (L" (%s)", MpamMscLocatorTitles[LocatorType]); > + } else if (LocatorType == EFI_ACPI_MPAM_LOCATION_UNKNOWN) { > + Print (L" (Unknown)"); > + } else { > + Print (L" (Reserved)"); > + } > +} > + > +/** > + This function traces 7 byte reserved fields. > + > + @param [in] Format Optional format string for tracing the data. > + @param [in] Ptr Pointer to the start of the buffer. > +**/ > +STATIC > +VOID > +EFIAPI > +Dump7Reserved ( > + IN CONST CHAR16 *Format OPTIONAL, > + IN UINT8 *Ptr [SAMI] I think my suggestion to extend FNPTR_PRINT_FORMATTER above would make it possible to have a common DumpReserved (). > + ) > +{ > + UINT64 Reserved; > + > + Reserved = (((UINT64)*(Ptr + 6)) << 48); > + Reserved |= (((UINT64)*(Ptr + 5)) << 40); > + Reserved |= (((UINT64)*(Ptr + 4)) << 32); > + Reserved |= (*(Ptr + 3) << 24); > + Reserved |= (*(Ptr + 2) << 16); > + Reserved |= (*(Ptr + 1) << 8); > + Reserved |= (*Ptr); > + > + Print (L"%lu\n", Reserved); [SAMI] I think it would be better to print the reserved field values as individual byte values, e.g. "0 0 0 0 0 0 0" in the Dump7Reserved case. > +} > + > +/** > + This function traces 3 byte reserved fields. > + > + @param [in] Format Optional format string for tracing the data. > + @param [in] Ptr Pointer to the start of the buffer. > +**/ > +STATIC > +VOID > +EFIAPI > +Dump3Reserved ( > + IN CONST CHAR16 *Format OPTIONAL, > + IN UINT8 *Ptr > + ) > +{ > + UINT32 Reserved; > + > + Reserved = (*(Ptr + 2) << 16); > + Reserved |= (*(Ptr + 1) << 8); > + Reserved |= (*Ptr); > + > + Print (L"%u\n", Reserved); > +} > + > +/** > + ACPI_PARSER array describing MPAM MSC interrupt flags. > +**/ > +STATIC CONST ACPI_PARSER MpamMscInterruptFlagParser[] = { > + { L"Interrupt Mode", 1, 0, L"%u", NULL, NULL, > + DecodeInterruptMode, NULL }, > + { L"Interrupt Type", 2, 1, L"%u", NULL, NULL, > + DecodeInterruptType, NULL }, > + { L"Affinity Type", 1, 3, L"%u", NULL, NULL, > + DecodeInterruptAffinityType, NULL }, > + { L"Affinity Valid", 1, 4, L"%u", NULL, NULL, > + DecodeInterruptAffinityValid, NULL }, > + { L"Reserved", 27, 5, L"%u", NULL, NULL, > + ValidateReserved, (VOID *)&ReservedSize[RS (4)] } [SAMI] I think you need a ValidateReservedBits () here. > +}; > + > +/** > + This function traces MPAM MSC Interrupt Flags. > + If no format string is specified the Format must be NULL. > + > + @param [in] Format Optional format string for tracing the data. > + @param [in] Ptr Pointer to the start of the buffer. > +**/ > +STATIC > +VOID > +EFIAPI > +DumpMpamMscInterruptFlags ( > + IN CONST CHAR16 *Format OPTIONAL, > + IN UINT8 *Ptr > + ) > +{ > + if (Format != NULL) { > + Print (Format, *(UINT32 *)Ptr); > + return; > + } [SAMI] Looking at theMpamMscNodeParser definition the Format field  is always NULL. Therefore,  I think the above snippet can be removed. > + > + Print (L"%u\n", *(UINT32 *)Ptr); > + // ParseAcpiBitFields would be called in a nested context (within ParseAcpi). > + // This would lead to an additional newline added after the parsing is > + // complete. Though it would be ideal to have the MSC node parsed without > + // intermittent any newline, this is not tackled at this point to keep the > + // code clean. [SAMI] I am not aware how the output looks like, but if the additional newline is an issue, can you look at fixing it in ParseAcpiBitFields(), please? > + ParseAcpiBitFields ( > + TRUE, > + 2, > + NULL, > + Ptr, > + 4, > + PARSER_PARAMS (MpamMscInterruptFlagParser) > + ); > +} > + > +/** > + 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 **)&MscInterfaceType, DecodeInterfaceType, NULL }, > + { L"Reserved", 1, 3, L"%u", NULL, > + NULL, ValidateReserved, (VOID *)&ReservedSize[RS (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 **)&MscInterfaceType }, [SAMI] The last ACPI_PARSER field is 'VOID *Context' right? Can you check if the above definition is correct, please? > + { L"Overflow interrupt", 4, 20, L"%u", NULL, > + NULL, NULL, NULL }, > + { L"Overflow interrupt flags", 4, 24, NULL, DumpMpamMscInterruptFlags, > + NULL, NULL, NULL }, > + { L"Reserved1", 4, 28, L"%u", NULL, > + NULL, ValidateReserved, (VOID *)&ReservedSize[RS (4)] }, > + { L"Overflow interrupt affinity", 4, 32, L"0x%x", NULL, > + NULL, NULL, NULL }, > + { L"Error interrupt", 4, 36, L"%u", NULL, > + NULL, NULL, NULL }, > + { L"Error interrupt flags", 4, 40, NULL, DumpMpamMscInterruptFlags, > + NULL, NULL, NULL }, > + { L"Reserved2", 4, 44, L"%u", NULL, > + NULL, ValidateReserved, (VOID *)&ReservedSize[RS (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"%u", NULL, > + NULL, ValidateReserved, (VOID *)&ReservedSize[RS (2)] }, > + { L"Locator type", 1, 7, L"0x%x", NULL, > + (VOID **)&ResourceLocatorType, > + DecodeLocatorType, NULL } [SAMI] I think this can continue to decode the Loator data and the Number of functional dependencies. e.g.  { L"Locator", 12, 8, NULL, ParseLocator, NULL, NULL, NULL}, { L"No of Func Dependencies", 4, 20, L"0x%x", NULL, (VOID **)&NumberOfFunctionalDependencies, NULL} > +}; > + > +/** > + ACPI_PARSER array describing the MPAM MSC processor cache locator field. > +**/ > +STATIC CONST ACPI_PARSER MpamMscProcessorCacheLocatorParser[] = { > + { L"Cache reference", 8, 0, L"%lu", NULL, NULL, NULL, NULL }, > + { L"Reserved", 4, 8, L"%u", NULL, NULL, > + ValidateReserved, (VOID *)&ReservedSize[RS (4)] } > +}; > + > +/** > + ACPI_PARSER array describing the MPAM MSC memory locator field. > +**/ > +STATIC CONST ACPI_PARSER MpamMscMemoryLocatorParser[] = { > + { L"Proximity domain", 8, 0, L"%lu", NULL, NULL, NULL, NULL }, > + { L"Reserved", 4, 8, L"%u", NULL, NULL, > + ValidateReserved, (VOID *)&ReservedSize[RS (4)] } > +}; > + > +/** > + ACPI_PARSER array describing the MPAM MSC SMMU locator field. > +**/ > +STATIC CONST ACPI_PARSER MpamMscSMMULocatorParser[] = { > + { L"SMMU interface", 8, 0, L"%lu", NULL, NULL, NULL, NULL }, > + { L"Reserved", 4, 8, L"%u", NULL, NULL, > + ValidateReserved, (VOID *)&ReservedSize[RS (4)] } > +}; > + > +/** > + ACPI_PARSER array describing the MPAM MSC memory cache locator field. > +**/ > +STATIC CONST ACPI_PARSER MpamMscMemoryCacheLocatorParser[] = { > + { L"Reserved", 7, 0, L"%lu", Dump7Reserved, NULL, > + ValidateReserved, (VOID *)&ReservedSize[RS (7)] }, > + { L"Level", 1, 7, L"%u", NULL, NULL,NULL, NULL }, > + { L"Reference", 4, 8, L"%u", NULL, NULL,NULL, NULL } > +}; > + > +/** > + ACPI_PARSER array describing the MPAM MSC ACPI device locator field. > +**/ > +STATIC CONST ACPI_PARSER MpamMscAcpiDeviceLocatorParser[] = { > + { L"ACPI hardware ID", 8, 0, L"0x%lx", NULL, NULL, > + DecodeHardwareId, NULL }, > + { L"ACPI unique ID", 4, 8, L"%u", NULL, NULL,NULL,NULL } > +}; > + > +/** > + ACPI_PARSER array describing the MPAM MSC interconnect locator field. > +**/ > +STATIC CONST ACPI_PARSER MpamMscInterconnectLocatorParser[] = { > + { L"Interconnect desc tbl offset", 8, 0, L"%lu", NULL, > + (VOID **)&InterconnectTableOffset, NULL, NULL }, [SAMI] I think it would be worth adding a validator to check that the InterconnectTableOffset complies with the requirement in 'Table 18: Interconnect locator descriptor', i.e. This table must be located in the resource-specific data region of the parent MPAM MSC node. See Table 4. [/SAMI] > + { L"Reserved", 4, 8, L"%u", NULL, > + NULL, ValidateReserved, (VOID *)&ReservedSize[RS (4)] } > +}; > + > +/** > + ACPI_PARSER array describing the MPAM MSC resource locator field. > +**/ > +STATIC CONST ACPI_PARSER MpamMscGenericLocatorParser[] = { > + { L"Descriptor1", 8, 0, L"%lu", NULL, NULL, NULL, NULL }, > + { L"Descriptor2", 4, 8, L"%u", NULL, NULL, 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, ValidateReserved, (VOID *)&ReservedSize[RS (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. > +**/ > +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 }, > + { L"Reserved", 3, 9, L"%lu", Dump3Reserved, NULL, > + ValidateReserved, (VOID *)&ReservedSize[RS (3)] } > +}; > + > +/** > + This function parses the locator field within the resource node for ACPI MPAM > + table. The parsing is based on the locator type field. > + > + @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. > +**/ > +STATIC > +VOID > +EFIAPI > +ParseLocator ( > + IN UINT8 *CONST Ptr, > + IN CONST UINT32 AcpiTableLength, > + IN UINT32 *CONST Offset > + ) [SAMI] This can be changed to +STATIC +VOID +EFIAPI +ParseLocator (   IN CONST CHAR16  *Format OPTIONAL,   IN UINT8         *Ptr   ) > +{ > + switch (*ResourceLocatorType) { > + case EFI_ACPI_MPAM_LOCATION_PROCESSOR_CACHE: > + *Offset += ParseAcpi ( > + TRUE, > + 4, > + NULL, > + Ptr + *Offset, > + AcpiTableLength - *Offset, > + PARSER_PARAMS (MpamMscProcessorCacheLocatorParser) > + ); [SAMI] the above can be changed to case EFI_ACPI_MPAM_LOCATION_PROCESSOR_CACHE: + ParseAcpi ( + TRUE, + 4, + NULL, + Ptr, + 12, // Length is always 12 + PARSER_PARAMS (MpamMscProcessorCacheLocatorParser) + ); A similar change would also be required for the other cases below. [/SAMI] > + break; > + case EFI_ACPI_MPAM_LOCATION_MEMORY: > + *Offset += ParseAcpi ( > + TRUE, > + 4, > + NULL, > + Ptr + *Offset, > + AcpiTableLength - *Offset, > + PARSER_PARAMS (MpamMscMemoryLocatorParser) > + ); > + break; > + case EFI_ACPI_MPAM_LOCATION_SMMU: > + *Offset += ParseAcpi ( > + TRUE, > + 4, > + NULL, > + Ptr + *Offset, > + AcpiTableLength - *Offset, > + PARSER_PARAMS (MpamMscSMMULocatorParser) > + ); > + break; > + case EFI_ACPI_MPAM_LOCATION_MEMORY_CACHE: > + *Offset += ParseAcpi ( > + TRUE, > + 4, > + NULL, > + Ptr + *Offset, > + AcpiTableLength - *Offset, > + PARSER_PARAMS (MpamMscMemoryCacheLocatorParser) > + ); > + break; > + case EFI_ACPI_MPAM_LOCATION_ACPI_DEVICE: > + *Offset += ParseAcpi ( > + TRUE, > + 4, > + NULL, > + Ptr + *Offset, > + AcpiTableLength - *Offset, > + PARSER_PARAMS (MpamMscAcpiDeviceLocatorParser) > + ); > + break; > + case EFI_ACPI_MPAM_LOCATION_INTERCONNECT: > + *Offset += ParseAcpi ( > + TRUE, > + 4, > + NULL, > + Ptr + *Offset, > + AcpiTableLength - *Offset, > + PARSER_PARAMS (MpamMscInterconnectLocatorParser) > + ); > + break; > + // For both UNKNOWN and RESERVED locator types, the locator is parsed using > + // the generic locator parser as the spec does not define any format. > + case EFI_ACPI_MPAM_LOCATION_UNKNOWN: > + *Offset += ParseAcpi ( > + TRUE, > + 4, > + NULL, > + Ptr + *Offset, > + AcpiTableLength - *Offset, > + PARSER_PARAMS (MpamMscGenericLocatorParser) > + ); > + break; > + default: > + Print (L"\nWARNING : Reserved locator type\n"); > + *Offset += ParseAcpi ( > + TRUE, > + 4, > + NULL, > + Ptr + *Offset, > + AcpiTableLength - *Offset, > + PARSER_PARAMS (MpamMscGenericLocatorParser) > + ); > + 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); > +} > + > +/** > + This function parses the interconnect descriptor(s) associated 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 *CONST Ptr, > + IN CONST UINT32 AcpiTableLength, > + IN UINT32 *CONST Offset > + ) > +{ > + UINT32 InterconnectDescriptorIndex; > + > + InterconnectDescriptorIndex = 0; > + > + // Parse MPAM MSC resources within the MSC body > + 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) > + ); > + > + InterconnectDescriptorIndex++; > + } > + > + return EFI_SUCCESS; > +} > + > +/** > + This function parses the interconnect descriptor table associated 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 *CONST Ptr, > + IN CONST UINT32 AcpiTableLength, > + IN UINT32 *CONST Offset, [SAMI] Can you check if the tag IN is correct here, please? Also check other functions in this file. > + IN CONST 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)) [SAMI] As I understand the InterconnectOffset (and therefore the InterconnectDescriptorTable data) must reside between the end of the resource node List and the end of the MSC node, i.e. within the resource-specific data region. See MPAM specification version 2.0, Table 18: Interconnect locator descriptor which states /"This table must be located in the resource-specific data region of the parent MPAM MSC node. See Table 4." / I believe the layout of MPAM table should be as below:     +---------------------+     | MPAM ACPI Header    | +---------------------+-------     |   MSC Node 0 Hdr    |   ^     | +-----------------+ |   |     | | Res Node 0      | |   |     | | +-------------+ | |   |     | | | Res Node Hdr| | |   |     | | +-------------+ | |   |     | | | Res Data    | | |   |     | | |             | | |   |     | | | +---------+ | | |   |   +---------------------------+     | | | | Locator | | | | ..|...| Interconnect locator desc |     | | | |         | | | |   |   |  Descriptor Table Offset |--points-to->--+     | | | |         | | | |   |   |  Reserved [4]             |               |     | | | +---------+ | | |   |+---------------------------+               |     | | | |FnDep Cnt| | | |   ||     | | | +---------+ | | | |                                               |     | | | |FnDep 1  | | | | |                                               |     | | | +---------+ | | | Interconnect                                      |     | | | |FnDep 2  | | | | descriptor                                       |     | | | +---------+ | | | table                                            |     | | | |FnDep n  | | | | offset                                           |     | | | +---------+ | | | value                                            |     | | +-------------+ | | |                                               |     | +-----------------+ | |                                               |     |         ...         | |                                               |     | +-----------------+ | |                                               |     | | Res Node N      | | |                                               |     | | +-------------+ | | |                                               |     | | | Res Node Hdr| | | |                                               |     | | +-------------+ | | |                                               |     | | | Res Data    | | | |                                               |     | | |             | | | |                                               |     | | | +---------+ | | | |                                               |     | | | | Locator | | | | |                                               |     | | | |         | | | | |                                               |     | | | |         | | | | |                                               |     | | | +---------+ | | | |                                               |     | | | |FnDep Cnt| | | | |                                               |     | | | +---------+ | | | |                                               |     | | | |FnDep 1  | | | | |                                               |     | | | +---------+ | | | |                                               |     | | | |FnDep 2  | | | | |                                               |     | | | +---------+ | | | |                                               |     | | | |FnDep n  | | | | |                                               |     | | | +---------+ | | | |                                               |     | | +-------------+ | | |                                               |     | +-----------------+ | |                                               |     \  Resource-specific  / v                                               |     /  data region. \<--------------------------------------------------+     \                     /     +---------------------+     | MSC Node 1 Hdr      |     |     ...             |     +---------------------+ [/SAMI] > + { > + 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; > + } > + > + // It is safe to cast InterconnectOffset to UINT32 as IntercconnectOffset can > + // never exceed the MPAM table length which is at max 2 bytes. > + *Offset = HeaderSize + MpamMscNodeLengthCumulative > + + (UINT32)InterconnectOffset; [SAMI] I think it would be cleaner if you store the MscNodeStart and then use that to compute the InterconnectDescriptorTable data location. i.e. InterconnectDescriptorTablePtr =  MscNodeStart + 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 *CONST Ptr, > + IN CONST UINT32 AcpiTableLength, > + IN UINT32 *CONST Offset > + ) > +{ > + UINT32 FunctionalDependencyIndex; > + > + FunctionalDependencyIndex = 0; > + > + // Parse MPAM MSC resources within the MSC body > + 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 *CONST Ptr, > + IN CONST UINT32 AcpiTableLength, > + IN UINT32 *CONST Offset [SAMI] Offset is an IN & OUT parameter. > + ) > +{ > + EFI_STATUS Status; > + 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) > + ); > + > + // Proceed with parsing only if a valid locator has been set. > + if ((ResourceLocatorType == NULL)) { > + MpamLengthError (L"Locator type or Locator not set"); > + return EFI_BAD_BUFFER_SIZE; > + } > + > + ParseLocator (Ptr, AcpiTableLength, Offset); [SAMI] I think the Locator is really a 12 byte field and should be parsed along with the MpamMscResourceParser  block. > + > + // Parse the number of functional dependency descriptors. > + *Offset += ParseAcpi ( > + TRUE, > + 2, > + NULL, > + Ptr + *Offset, > + AcpiTableLength - *Offset, > + PARSER_PARAMS (MpamMscFunctionalDependencyCountParser) > + ); > + [SAMI] With my suggestion above inMpamMscResourceParser, the above can be avoided. > + Status = ParseMpamMscFunctionalDependencies (Ptr, AcpiTableLength, Offset); > + if (Status != EFI_SUCCESS) { > + return Status; > + } > + > + // If offset field has been set, parse the interconnect description table. > + if ( (*ResourceLocatorType == EFI_ACPI_MPAM_LOCATION_INTERCONNECT) > + && (InterconnectTableOffset != NULL)) > + { > + Status = ParseInterconnectDescriptorTable ( > + Ptr, > + AcpiTableLength, > + Offset, > + *InterconnectTableOffset > + ); > + 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 *CONST Ptr, > + IN CONST 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) > + ); > + > + if (MpamMscNodeLength == NULL) { > + MpamLengthError (L"MPAM MSC node length not set!"); > + return EFI_BAD_BUFFER_SIZE; > + } > + > + if (*MpamMscNodeLength < sizeof (EFI_ACPI_MPAM_MSC_NODE)) { > + Print (L"\nERROR: MSC length should be at least the size of node body! "); > + Print (L"MSC Length %u\n", *MpamMscNodeLength); [SAMI] I think the error counter must be updated here, i.e. by calling  IncrementErrorCount () . > + return EFI_BAD_BUFFER_SIZE; > + } > + > + // Parse MPAM MSC resources within the MSC body > + Status = ParseMpamMscResources (Ptr, AcpiTableLength, &Offset); > + 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..89d9f5fb7d > --- /dev/null > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Mpam/MpamParser.h > @@ -0,0 +1,31 @@ > +/** @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_ > + > +// Mpam has quite a lot of reserved fields that must be zero according to the > +// specification. ACPI_PARSER's FieldValidator is used for validating these > +// fields. The FieldValidator function accepts a context paramter of VOID * > +// type. If we are to directly pass a numeric literal cast as (VOID *), we > +// would need to use uintptr_t to properly decode it within the FieldValidator. > +// There is no other portable way to cast back the size (and the CI complains if > +// this is the case). In order to deal with this in a clean way, define a type > +// (RESERVED_SIZE_T) for holding the size of the reserved field. The address of > +// objects of RESERVED_SIZE_T should be passed as parameter to the > +// FieldValidator function. > +typedef struct { > + UINT8 Size; > +} RESERVED_SIZE_T; > + > +#define RS(x) (x-1) [SAMI] I think we should try to eleminate the above. > + > +#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.
> + Copyright (c) 2016 - 2023, Arm Limited. All rights reserved.
> 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.
> +# 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/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.
> +// 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 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" -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#107915): https://edk2.groups.io/g/devel/message/107915 Mute This Topic: https://groups.io/mt/100818701/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-