From: "Sami Mujawar" <sami.mujawar@arm.com>
To: Rohit Mathew <Rohit.Mathew@arm.com>,
"devel@edk2.groups.io" <devel@edk2.groups.io>
Cc: Thomas Abraham <thomas.abraham@arm.com>,
James Morse <James.Morse@arm.com>,
Pierre Gondois <Pierre.Gondois@arm.com>,
Zhichao Gao <zhichao.gao@intel.com>,
Yeo Reum Yun <YeoReum.Yun@arm.com>, nd <nd@arm.com>
Subject: Re: [edk2-devel] [PATCH V4 3/3] ShellPkg/AcpiView: Add MPAM Parser
Date: Tue, 19 Sep 2023 12:56:51 +0000 [thread overview]
Message-ID: <F868FEF8-F6EF-4CEB-94DA-36CA1032353A@arm.com> (raw)
In-Reply-To: <AM6PR08MB37833C763D3BF64B39A8DE5A8FFAA@AM6PR08MB3783.eurprd08.prod.outlook.com>
[-- Attachment #1: Type: text/plain, Size: 68514 bytes --]
Hi Rohit,
Please see my response inline marked [SAMI].
Regards,
Sami Mujawar
From: Rohit Mathew <Rohit.Mathew@arm.com>
Date: Tuesday, 19 September 2023 at 10:40
To: "devel@edk2.groups.io" <devel@edk2.groups.io>, Rohit Mathew <Rohit.Mathew@arm.com>, Sami Mujawar <Sami.Mujawar@arm.com>
Cc: Thomas Abraham <thomas.abraham@arm.com>, James Morse <James.Morse@arm.com>, Pierre Gondois <Pierre.Gondois@arm.com>, Zhichao Gao <zhichao.gao@intel.com>, Yeo Reum Yun <YeoReum.Yun@arm.com>, nd <nd@arm.com>
Subject: RE: [edk2-devel] [PATCH V4 3/3] ShellPkg/AcpiView: Add MPAM Parser
HI Sami,
Gentle reminder on the queries.
Regards,
Rohit
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Rohit Mathew via groups.io
Sent: Tuesday, August 29, 2023 6:34 PM
To: Sami Mujawar <Sami.Mujawar@arm.com>; devel@edk2.groups.io
Cc: Thomas Abraham <thomas.abraham@arm.com>; James Morse <James.Morse@arm.com>; Pierre Gondois <Pierre.Gondois@arm.com>; Zhichao Gao <zhichao.gao@intel.com>; Yeo Reum Yun <YeoReum.Yun@arm.com>; nd <nd@arm.com>
Subject: Re: [edk2-devel] [PATCH V4 3/3] ShellPkg/AcpiView: Add MPAM Parser
Hi Sami,
Thank you for the review. Please find my comments inline.
From: Sami Mujawar <Sami.Mujawar@arm.com<mailto:Sami.Mujawar@arm.com>>
Sent: Monday, August 21, 2023 11:46 AM
To: Rohit Mathew <Rohit.Mathew@arm.com<mailto:Rohit.Mathew@arm.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Thomas Abraham <thomas.abraham@arm.com<mailto:thomas.abraham@arm.com>>; James Morse <James.Morse@arm.com<mailto:James.Morse@arm.com>>; Pierre Gondois <Pierre.Gondois@arm.com<mailto:Pierre.Gondois@arm.com>>; Zhichao Gao <zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>>; Yeo Reum Yun <YeoReum.Yun@arm.com<mailto:YeoReum.Yun@arm.com>>; nd <nd@arm.com<mailto:nd@arm.com>>
Subject: Re: [PATCH V4 3/3] ShellPkg/AcpiView: Add MPAM Parser
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 <Rohit.Mathew@arm.com><mailto:Rohit.Mathew@arm.com>
Cc: James Morese <james.Morse@arm.com><mailto:james.Morse@arm.com>
Cc: Sami Mujawar <sami.mujawar@arm.com><mailto:sami.mujawar@arm.com>
Cc: Thomas Abraham <thomas.abraham@arm.com><mailto:thomas.abraham@arm.com>
Cc: Zhichao Gao <zhichao.gao@intel.com><mailto:zhichao.gao@intel.com>
---
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 <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 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 the code 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.
-typedef VOID (EFIAPI *FNPTR_FIELD_VALIDATOR)(UINT8 *Ptr, VOID *Context);
+typedef VOID (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.
-typedef VOID (EFIAPI *FNPTR_PRINT_FORMATTER)(CONST CHAR16 *Format, UINT8 *Ptr);
+typedef VOID (EFIAPI *FNPTR_PRINT_FORMATTER)(CONST CHAR16 *Format, UINT8 *Ptr, UINTN Length);
[/SAMI]
[Rohit]
Ack.
I believe the length field for both API’s should be of UINT32 rather than UINTN to match with the parser’s definition. As UINTN is generally larger, we would never need the extra space, right?
[SAMI] I agree, UINT32 should be fine.
typedef struct AcpiParser {
/// String describing the ACPI table field
/// (Field column from ACPI table spec)
CONST CHAR16 *NameStr;
/// The length of the field.
/// (Byte Length column from ACPI table spec)
/// Length(in bits) of the bitfield if used with ParseAcpiBitFields().
UINT32 Length;
I just did a rough prototype for these changes to see the scope of impact and I can see that fieldvalidator changes would impact the following parsers.
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Aest/AestParser.c
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Erst/ErstParser.c
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Hmat/HmatParser.c
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pcct/PcctParser.c
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rsdp/RsdpParser.c
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Spcr/SpcrParser.c
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c
~~
Likewise, The printformator change would impact the following parsers.
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Aest/AestParser.c
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Erst/ErstParser.c
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Hmat/HmatParser.c
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c
~~
I’m assuming we wouldn’t need to test these parsers as the length param would stay unused for all other parsers that use these APIs. Checking as the scope of change is quite large. What is your opinion?
[SAMI] Yes, this sounds right. I think most tables would be tested when we run on any Sgi platform. If there is something that does not get tested, we should be able to catch any issues in the code review.
+ 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 ().
[Rohit] Ack.
+ )
+{
+ 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.
[Rohit] Ack.
With this format, I’m assuming with bit lengths, we would round up to the next byte. Ie for 27 bits “0 0 0 0”.
[SAMI] These would be two different functions DumpReservedBytes() and DumpReservedBits(). I think initially we should print the bits as whole bytes.
+}
+
+/**
+ 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.
[Rohit] Ack.
Sidenote - With the old changes, we could have managed without ValidateReservedBits as ParseAcpiBitFields used a UINT64 to extract the field. The address of this field is casted and passed on to the validator.
However, with length coming in as param, it makes sense to add a bit variant to make it clean.
Here is the snippet –
// extract Bitfield data for the current item
Data = RShiftU64 (BitsData, Parser[Index].Offset) & ~(LShiftU64 (~0ULL, Parser[Index].Length));
~
// Validating only makes sense if we are tracing
// the parsed table entries, to report by table name.
if (GetConsistencyChecking () &&
(Parser[Index].FieldValidator != NULL))
{
Parser[Index].FieldValidator ((UINT8 *)&Data, Parser[Index].Context);
}
~~
+};
+
+/**
+ 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
+ )
+{
<SNIP>
+ if (Format != NULL) {
+ Print (Format, *(UINT32 *)Ptr);
+ return;
+ }
</SNIP>
[SAMI] Looking at the MpamMscNodeParser definition the Format field is always NULL. Therefore, I think the above snippet can be removed.
[Rohit] Ack
+
+ 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?
[Rohit] This is a sample of how the output would look for an MPAM MSC.
MPAM :
Signature : MPAM
Length : 752
Revision : 1
Checksum : 0x88
Oem ID : ARMLTD
Oem Table ID : ARMSGI
Oem Revision : 0x20140727
Creator ID : ARM
Creator Revision : 0x99
* MSC * : 0
Length : 520
Interface type : 0x0 (MMIO)
Reserved : 0
Identifier : 0
Base address : 0x141601000
MMIO Size : 0x1004
Overflow interrupt : 0
Overflow interrupt flags : 24
Interrupt Mode : 0 (Level triggered)
Interrupt Type : 0 (Wired interrupt)
Affinity Type : 1 (Processor container affinity)
Affinity Valid : 1 (Affinity valid)
Reserved : 0 0 0 0
Reserved1 : 0 0 0 0
Overflow interrupt affinity : 0x0
Error interrupt : 81
Error interrupt flags : 24
Interrupt Mode : 0 (Level triggered)
Interrupt Type : 0 (Wired interrupt)
Affinity Type : 1 (Processor container affinity)
Affinity Valid : 1 (Affinity valid)
Reserved : 0 0 0 0
Reserved2 : 0 0 0 0
Error interrupt affinity : 0x0
MAX_NRDY_USEC : 0x0
Hardware ID of linked device : 0x43313031484D5241 (ARMH101C)
Instance ID of linked device : 0x2200
Number of resource nodes : 8
* Resource * : 0
The intermittent spacing after Reserved from the MPAM interrupt flag is what the comment talks about. The issue arises because of nesting (both ParseAcpiBitFields and ParseAcpi adds a newline back-to-back). This would happen even with ParseAcpi calling ParseAcpi internally in a nested context (as you have suggested for Locator parsing). One way to resolve this would be to have an additional parameter say, IsNested which should remain false for all default cases.
UINT32
EFIAPI
ParseAcpiBitFields (
IN BOOLEAN Trace,
IN UINT32 Indent,
IN CONST CHAR8 *AsciiName OPTIONAL,
IN UINT8 *Ptr,
IN UINT32 Length,
IN CONST ACPI_PARSER *Parser,
IN UINT32 ParserItems,
IN BOOLEAN IsNested
)
and then brace the newline print with the following check.
~~
If (!(IsNested && Index == ParserItems - 1)) {
Print (L"\n");
}
~~
However, this change has a large scope of impact I believe as we would need to verify if the alignment of all the parsers. Could we take this as different task to not club this with MPAM if its, okay? If you have any alternate suggestions, let me know.
[SAMI] Yes, this can be done separately.
+ 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?
[Rohit] Ack.
+ { 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 }
[Rohit] Ack. The one concern I have is with how nesting introduces spacing issues as I have highlighted above.
[SAMI] Can you see if we can introduce a parameter that specifies that additional line breaks should not be added.
I think we did not get back to the feedback for the series at https://edk2.groups.io/g/devel/message/80815, but can you check if it is possible to define a new flag to control line breaks.
[/SAMI]
+};
+
+/**
+ 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]
[Rohit] I have added checks within ParseInterconnectDescriptorTable. I can move them to a validator to catch the errors a bit earlier. However we would still need some form of check to see if we should proceed with the parsing (probably a single flag InterconnectOffsetValid = TRUE/FALSE) within ParseInterconnectDescriptorTable.
+ { 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) {
<SNIP>
+ case EFI_ACPI_MPAM_LOCATION_PROCESSOR_CACHE:
+ *Offset += ParseAcpi (
+ TRUE,
+ 4,
+ NULL,
+ Ptr + *Offset,
+ AcpiTableLength - *Offset,
+ PARSER_PARAMS (MpamMscProcessorCacheLocatorParser)
+ );
</SNIP>
[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]
[Rohit] Ack. Indent could be 2 as we are nesting.
+ 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.
[Rohit] Yes. All offset pointers need to IN OUT.
+ 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]
[Rohit] Ack, I'll reword the comment. Just to add , all the parsing and checks for Interconnect descriptor is in fact done with very same layout in mind.
[SAMI] You may want to include the above ASCII layout as part of your patch/code comment for reference.
+ {
+ 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;
[Rohit] Ack.
+
+ 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.
[Rohit] Ack.
+ )
+{
+ 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.
[Rohit] Ack. Highlighted one concern.
+
+ // Parse the number of functional dependency descriptors.
+ *Offset += ParseAcpi (
+ TRUE,
+ 2,
+ NULL,
+ Ptr + *Offset,
+ AcpiTableLength - *Offset,
+ PARSER_PARAMS (MpamMscFunctionalDependencyCountParser)
+ );
+
[SAMI] With my suggestion above in MpamMscResourceParser, the above can be avoided.
[Rohit] Ack.
+ 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 () .
[Rohit] Ack.
+ 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.
[Rohit] Ack.
+
+#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"
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108828): https://edk2.groups.io/g/devel/message/108828
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]
-=-=-=-=-=-=-=-=-=-=-=-
[-- Attachment #2: Type: text/html, Size: 210793 bytes --]
next prev parent reply other threads:[~2023-09-19 12:57 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-18 11:48 [edk2-devel] [PATCH V4 3/3] ShellPkg/AcpiView: Add MPAM Parser Rohit Mathew
2023-08-21 10:46 ` Sami Mujawar
2023-08-29 17:34 ` Rohit Mathew
[not found] ` <177FEB14605F923E.26157@groups.io>
2023-09-19 9:40 ` Rohit Mathew
2023-09-19 12:56 ` Sami Mujawar [this message]
2023-10-02 16:14 ` 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=F868FEF8-F6EF-4CEB-94DA-36CA1032353A@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