[PATCH 1/2] Mde Pkg: Support for MPAM ACPI Table

[PATCH 1/2] Mde Pkg: Support for MPAM ACPI Table

[Name]

From: Swatisri Kantamsetti <swatisrik@nvidia.com>

Added MPAM table header, MSC and Resource Node
info structures

Signed-off-by: Swatisri Kantamsetti <swatisrik@nvidia.com>
---
 MdePkg/Include/IndustryStandard/Acpi64.h |  5 ++
 MdePkg/Include/IndustryStandard/Mpam.h   | 69 ++++++++++++++++++++++++
 2 files changed, 74 insertions(+)
 create mode 100644 MdePkg/Include/IndustryStandard/Mpam.h

diff --git a/MdePkg/Include/IndustryStandard/Acpi64.h b/MdePkg/Include/IndustryStandard/Acpi64.h
index fe5ebfac2b..e54f631186 100644
--- a/MdePkg/Include/IndustryStandard/Acpi64.h
+++ b/MdePkg/Include/IndustryStandard/Acpi64.h
@@ -2952,6 +2952,11 @@ typedef struct {
 ///
 #define EFI_ACPI_6_4_PROCESSOR_PROPERTIES_TOPOLOGY_TABLE_STRUCTURE_SIGNATURE  SIGNATURE_32('P', 'P', 'T', 'T')
 
+///
+/// "MPAM" Memory System Resource Partitioning And Monitoring Table
+///
+#define EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORING_TABLE_STRUCTURE_SIGNATURE  SIGNATURE_32('M', 'P', 'A', 'M')
+
 ///
 /// "PSDT" Persistent System Description Table
 ///
diff --git a/MdePkg/Include/IndustryStandard/Mpam.h b/MdePkg/Include/IndustryStandard/Mpam.h
new file mode 100644
index 0000000000..e0f75f0114
--- /dev/null
+++ b/MdePkg/Include/IndustryStandard/Mpam.h
@@ -0,0 +1,69 @@
+/** @file
+  ACPI Memory System Resource Partitioning And Monitoring (MPAM)
+  as specified in ARM spec DEN0065
+
+  Copyright (c) 2022, NVIDIA CORPORATION. All rights reserved.
+  Copyright (c) 2022, ARM Limited. All rights reserved.
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#ifndef _MPAM_H_
+#define _MPAM_H_
+
+#pragma pack(1)
+
+///
+/// Memory System Resource Partitioning and Monitoring Table (MPAM)
+///
+typedef struct {
+  EFI_ACPI_DESCRIPTION_HEADER    Header;
+  UINT32                         NumNodes;
+  UINT32                         NodeOffset;
+  UINT32                         Reserved;
+} EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORING_TABLE_HEADER;
+
+///
+/// MPAM Revision (as defined in ACPI 6.4 spec.)
+///
+#define EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORING_TABLE_REVISION  0x01
+
+///
+/// Memory System Controller Node Structure
+///
+
+typedef struct {
+  UINT16    Length;
+  UINT16    Reserved;
+  UINT32    Identifier;
+  UINT64    BaseAddress;
+  UINT32    MmioSize;
+  UINT32    OverflowInterrupt;
+  UINT32    OverflowInterruptFlags;
+  UINT32    Reserved1;
+  UINT32    OverflowInterruptAff;
+  UINT32    ErrorInterrupt;
+  UINT32    ErrorInterruptFlags;
+  UINT32    Reserved2;
+  UINT32    ErrorInterruptAff;
+  UINT32    MaxNRdyUsec;
+  UINT64    LinkedDeviceHwId;
+  UINT32    LinkedDeviceInstanceHwId;
+  UINT32    NumResourceNodes;
+} EFI_ACPI_6_4_MPAM_MSC_NODE;
+
+///
+/// Resource Node Structure
+///
+
+typedef struct {
+  UINT32    Identifier;
+  UINT8     RisIndex;
+  UINT16    Reserved1;
+  UINT8     LocatorType;
+  UINT64    Locator;
+  UINT32    NumFuncDep;
+} EFI_ACPI_6_4_MPAM_RESOURCE_NODE;
+
+#pragma pack()
+
+#endif
-- 
2.17.1

[PATCH 2/2] Dynamic Tbl Mgr: MPAM: MPAM Generator and supporting files

[Name]

From: Swatisri Kantamsetti <swatisrik@nvidia.com>

ACPI header, MSC and Resource Nodes are populated
in the MPAM Table

Signed-off-by: Swatisri Kantamsetti <swatisrik@nvidia.com>
---
 DynamicTablesPkg/DynamicTables.dsc.inc        |   2 +
 DynamicTablesPkg/Include/AcpiTableGenerator.h |   1 +
 .../Include/ArmNameSpaceObjects.h             |  68 ++
 .../Arm/AcpiMpamLibArm/AcpiMpamLibArm.inf     |  30 +
 .../Acpi/Arm/AcpiMpamLibArm/MpamGenerator.c   | 649 ++++++++++++++++++
 .../Acpi/Arm/AcpiMpamLibArm/MpamGenerator.h   |  47 ++
 6 files changed, 797 insertions(+)
 create mode 100644 DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/AcpiMpamLibArm.inf
 create mode 100644 DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/MpamGenerator.c
 create mode 100644 DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/MpamGenerator.h

diff --git a/DynamicTablesPkg/DynamicTables.dsc.inc b/DynamicTablesPkg/DynamicTables.dsc.inc
index 3d4fa0c4c4..745d5f0633 100644
--- a/DynamicTablesPkg/DynamicTables.dsc.inc
+++ b/DynamicTablesPkg/DynamicTables.dsc.inc
@@ -29,6 +29,7 @@
   DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/AcpiIortLibArm.inf
   DynamicTablesPkg/Library/Acpi/Arm/AcpiMadtLibArm/AcpiMadtLibArm.inf
   DynamicTablesPkg/Library/Acpi/Arm/AcpiMcfgLibArm/AcpiMcfgLibArm.inf
+  DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/AcpiMpamLibArm.inf
   DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/AcpiPpttLibArm.inf
   DynamicTablesPkg/Library/Acpi/Arm/AcpiRawLibArm/AcpiRawLibArm.inf
   DynamicTablesPkg/Library/Acpi/Arm/AcpiSpcrLibArm/AcpiSpcrLibArm.inf
@@ -54,6 +55,7 @@
       NULL|DynamicTablesPkg/Library/Acpi/Arm/AcpiMadtLibArm/AcpiMadtLibArm.inf
       NULL|DynamicTablesPkg/Library/Acpi/Arm/AcpiMcfgLibArm/AcpiMcfgLibArm.inf
       NULL|DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/AcpiPpttLibArm.inf
+      NULL|DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/AcpiMpamLibArm.inf
       NULL|DynamicTablesPkg/Library/Acpi/Arm/AcpiRawLibArm/AcpiRawLibArm.inf
       NULL|DynamicTablesPkg/Library/Acpi/Arm/AcpiSpcrLibArm/AcpiSpcrLibArm.inf
       NULL|DynamicTablesPkg/Library/Acpi/Arm/AcpiSratLibArm/AcpiSratLibArm.inf
diff --git a/DynamicTablesPkg/Include/AcpiTableGenerator.h b/DynamicTablesPkg/Include/AcpiTableGenerator.h
index f962dbff57..56d7375b4a 100644
--- a/DynamicTablesPkg/Include/AcpiTableGenerator.h
+++ b/DynamicTablesPkg/Include/AcpiTableGenerator.h
@@ -94,6 +94,7 @@ typedef enum StdAcpiTableId {
   EStdAcpiTableIdIort,                          ///< IORT Generator
   EStdAcpiTableIdPptt,                          ///< PPTT Generator
   EStdAcpiTableIdSrat,                          ///< SRAT Generator
+  EStdAcpiTableIdMpam,                          ///< MPAM Generator
   EStdAcpiTableIdSsdtSerialPort,                ///< SSDT Serial-Port Generator
   EStdAcpiTableIdSsdtCmn600,                    ///< SSDT Cmn-600 Generator
   EStdAcpiTableIdSsdtCpuTopology,               ///< SSDT Cpu Topology
diff --git a/DynamicTablesPkg/Include/ArmNameSpaceObjects.h b/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
index 102e0f96be..39a14ed0b3 100644
--- a/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
+++ b/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
@@ -63,6 +63,8 @@ typedef enum ArmObjectID {
   EArmObjPciInterruptMapInfo,          ///< 39 - Pci Interrupt Map Info
   EArmObjRmr,                          ///< 40 - Reserved Memory Range Node
   EArmObjMemoryRangeDescriptor,        ///< 41 - Memory Range Descriptor
+  EArmObjMscNodeInfo,                  ///< 40 - Msc Memory System Controller Node Info
+  EArmObjResNodeInfo,                  ///< 41 - Res Resource Node Info
   EArmObjMax
 } EARM_OBJECT_ID;
 
@@ -1070,6 +1072,72 @@ typedef struct CmArmRmrDescriptor {
   UINT64    Length;
 } CM_ARM_MEMORY_RANGE_DESCRIPTOR;
 
+/** A structure that describes Memory System Controller Node.
+
+    MPAM Memory System Component Nodes are described by
+    this object.
+
+  ID: EArmObjMscNodeInfo
+*/
+typedef struct CmArmMscNodeInfo {
+  /// An unique token used to identify this object
+  CM_OBJECT_TOKEN      Token;
+
+  /// MPAM Base Address
+  UINT64               BaseAddress;
+  /// MMIO Size
+  UINT32               MmioSize;
+  /// Overflow Interrupt
+  UINT32               OverflowInterrupt;
+  /// Overflow Interrupt Flags
+  UINT32               OverflowInterruptFlags;
+  /// Overflow Interrupt Affinity
+  UINT32               OverflowInterruptAff;
+  /// Error Interrupt
+  UINT32               ErrorInterrupt;
+  /// Error Interrupt Flags
+  UINT32               ErrorInterruptFlags;
+  /// Error Interrupt Affinity
+  UINT32               ErrorInterruptAff;
+  /// Not Ready Signal time
+  UINT32               MaxNRdyUsec;
+  /// Linked Device HWID
+  UINT64               LinkedDeviceHwId;
+  /// Linked Device Instance ID
+  UINT32               LinkedDeviceInstanceHwId;
+  /// Number of Resource nodes
+  UINT32               NumResourceNodes;
+  /// Reference token for the list of resource nodes
+  //CM_OBJECT_TOKEN    ResourceNodeListToken;
+
+} CM_ARM_MSC_NODE_INFO;
+
+/** A structure that describes Memory System Controller Node.
+
+    MPAM Memory System Component Nodes are described by
+    this object.
+
+  ID: EArmObjResNodeInfo
+*/
+typedef struct CmArmResNodeInfo {
+  /// An unique token used to identify this object
+  CM_OBJECT_TOKEN    Token;
+
+  /// Identifier
+  UINT32             Identifier;
+  /// RIS Index
+  UINT8              RisIndex;
+  /// Locator Type
+  UINT8              LocatorType;
+  /// Locator
+  UINT64             Locator;
+  /// Num functional dependencies
+  UINT32             NumFuncDep;
+  /// Reference token for the list of resource nodes
+  CM_OBJECT_TOKEN    FuncDepListToken;
+
+} CM_ARM_RESOURCE_NODE_INFO;
+
 #pragma pack()
 
 #endif // ARM_NAMESPACE_OBJECTS_H_
diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/AcpiMpamLibArm.inf b/DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/AcpiMpamLibArm.inf
new file mode 100644
index 0000000000..480130dc21
--- /dev/null
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/AcpiMpamLibArm.inf
@@ -0,0 +1,30 @@
+## @file
+#  MPAM Table Generator Inf file
+#
+#  Copyright (c) 2022, NVIDIA CORPORATION. All rights reserved.
+#  Copyright (c) 2022, ARM Limited. All rights reserved.
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+##
+
+[Defines]
+  INF_VERSION    = 0x0001001B
+  BASE_NAME      = AcpiMpamLibArm
+  FILE_GUID      = 02d0c79f-41cd-45c9-9835-781229c619d1
+  VERSION_STRING = 1.0
+  MODULE_TYPE    = DXE_DRIVER
+  LIBRARY_CLASS  = NULL|DXE_DRIVER
+  CONSTRUCTOR    = AcpiMpamLibConstructor
+  DESTRUCTOR     = AcpiMpamLibDestructor
+
+[Sources]
+  MpamGenerator.c
+  MpamGenerator.h
+
+[Packages]
+  EmbeddedPkg/EmbeddedPkg.dec
+  DynamicTablesPkg/DynamicTablesPkg.dec
+  MdeModulePkg/MdeModulePkg.dec
+  MdePkg/MdePkg.dec
+
+[LibraryClasses]
+  BaseLib
diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/MpamGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/MpamGenerator.c
new file mode 100644
index 0000000000..db3e8e95bc
--- /dev/null
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/MpamGenerator.c
@@ -0,0 +1,649 @@
+/** @file
+  MPAM Table Generator
+
+  Copyright (c) 2022, NVIDIA CORPORATION. All rights reserved.
+  Copyright (c) 2022, ARM Limited. All rights reserved.
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+  @par Reference(s):
+  - ACPI 6.4 Specification, January 2021
+
+  @par Glossary:
+  - Cm or CM   - Configuration Manager
+  - Obj or OBJ - Object
+**/
+
+#include <IndustryStandard/Mpam.h>
+#include <Library/AcpiLib.h>
+#include <Library/BaseLib.h>
+#include <Library/DebugLib.h>
+#include <Library/MemoryAllocationLib.h>
+#include <Protocol/AcpiTable.h>
+
+// Module specific include files.
+#include <AcpiTableGenerator.h>
+#include <ConfigurationManagerObject.h>
+#include <ConfigurationManagerHelper.h>
+#include <Library/TableHelperLib.h>
+#include <Protocol/ConfigurationManagerProtocol.h>
+
+#include "MpamGenerator.h"
+
+/**
+  ARM standard MPAM Generator
+
+  Requirements:
+    The following Configuration Manager Object(s) are used by this Generator:
+    - EArmObjMscNodeInfo (REQUIRED)
+    - EArmObjResNodeInfo
+*/
+
+/**
+  This macro expands to a function that retrieves the MSC Node information
+  from the Configuration Manager.
+*/
+GET_OBJECT_LIST (
+  EObjNameSpaceArm,
+  EArmObjMscNodeInfo,
+  CM_ARM_MSC_NODE_INFO
+  );
+
+/**
+  This macro expands to a function that retrieves the Resource Node
+  information from the Configuration Manager.
+*/
+GET_OBJECT_LIST (
+  EObjNameSpaceArm,
+  EArmObjResNodeInfo,
+  CM_ARM_RESOURCE_NODE_INFO
+  );
+
+/**
+  Returns the size of the MPAM Memory System Controller (MSC) Node
+
+  @param [in]  Node     Pointer to MSC Node Info CM object
+
+  @retval               Size of the MSC Node in bytes.
+**/
+STATIC
+UINT32
+GetMscNodeSize (
+  IN  CONST CM_ARM_MSC_NODE_INFO  *Node
+  )
+{
+  ASSERT (Node != NULL);
+
+  // <size of Memory System Controller Node>
+  return (UINT32)(sizeof (EFI_ACPI_6_4_MPAM_MSC_NODE) +
+                  Node->NumResourceNodes * sizeof (EFI_ACPI_6_4_MPAM_RESOURCE_NODE));
+}
+
+/** Returns the total size required for the MSC and
+    updates the Node Indexer.
+
+    This function calculates the size required for the node group
+    and also populates the Node Indexer array with offsets for the
+    individual nodes.
+
+    @param [in]       NodeStartOffset Offset from the start of the
+                                      MPAM where this node group starts.
+    @param [in]       NodeList        Pointer to MSC Group node list.
+    @param [in]       NodeCount       Count of the MSC Group nodes.
+    @param [in, out]  NodeIndexer     Pointer to the next Node Indexer.
+
+    @retval Total size of the ITS Group Nodes.
+**/
+STATIC
+UINT64
+GetSizeofMscGroupNodes (
+  IN      CONST UINT32                         NodeStartOffset,
+  IN      CONST CM_ARM_MSC_NODE_INFO           *NodeList,
+  IN            UINT32                         NodeCount,
+  IN OUT        MPAM_NODE_INDEXER     **CONST  NodeIndexer
+  )
+{
+  UINT64  Size;
+
+  ASSERT (NodeList != NULL);
+
+  Size = 0;
+  while (NodeCount-- != 0) {
+    (*NodeIndexer)->Token  = NodeList->Token;
+    (*NodeIndexer)->Object = (VOID *)NodeList;
+    (*NodeIndexer)->Offset = (UINT32)(Size + NodeStartOffset);
+    DEBUG ((
+      DEBUG_INFO,
+      "MPAM: MSC Node Indexer = %p, Token = %p, Object = %p, Offset = 0x%x\n",
+      *NodeIndexer,
+      (*NodeIndexer)->Token,
+      (*NodeIndexer)->Object,
+      (*NodeIndexer)->Offset
+      ));
+
+    Size += GetMscNodeSize (NodeList);
+
+    (*NodeIndexer)++;
+    NodeList++;
+  }
+
+  return Size;
+}
+
+/** Update the MSC Group Node Information.
+
+    @param [in]     This             Pointer to the table Generator.
+    @param [in]     CfgMgrProtocol   Pointer to the Configuration Manager
+                                     Protocol Interface.
+    @param [in]     Mpam             Pointer to MPAM table structure.
+    @param [in]     NodesStartOffset Offset for the start of the Msc Group
+                                     Nodes.
+    @param [in]     NodeList         Pointer to an array of Msc Group Node
+                                     Objects.
+    @param [in]     NodeCount        Number of Msc Group Node Objects.
+
+    @retval EFI_SUCCESS           Table generated successfully.
+    @retval EFI_INVALID_PARAMETER A parameter is invalid.
+    @retval EFI_NOT_FOUND         The required object was not found.
+**/
+STATIC
+EFI_STATUS
+AddMscNodes (
+  IN      CONST ACPI_TABLE_GENERATOR                                                       *CONST  This,
+  IN      CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL                                       *CONST  CfgMgrProtocol,
+  IN      CONST EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORING_TABLE_HEADER           *Mpam,
+  IN      CONST UINT32                                                                             NodesStartOffset,
+  IN      CONST CM_ARM_MSC_NODE_INFO                                                               *NodeList,
+  IN            UINT32                                                                             NodeCount
+  )
+{
+  EFI_STATUS                       Status;
+  EFI_ACPI_6_4_MPAM_MSC_NODE       *MscNode;
+  EFI_ACPI_6_4_MPAM_RESOURCE_NODE  *ResourceNodeArray;
+  CM_ARM_RESOURCE_NODE_INFO        *ResourceNodeList;
+  UINT64                           NodeLength;
+  UINT32                           ResourceNodeCount;
+
+  ASSERT (Mpam != NULL);
+
+  MscNode = (EFI_ACPI_6_4_MPAM_MSC_NODE *)((UINT8 *)Mpam + NodesStartOffset);
+
+  // Get the Resource Node info to update the MPAM MSC node
+  Status = GetEArmObjResNodeInfo (
+             CfgMgrProtocol,
+             CM_NULL_TOKEN,
+             &ResourceNodeList,
+             &ResourceNodeCount
+             );
+  if (EFI_ERROR (Status)) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "ERROR: MPAM: Failed to add resource nodes info. Status = %r\n",
+      Status
+      ));
+    return Status;
+  }
+
+  while (NodeCount-- != 0) {
+    NodeLength = GetMscNodeSize (NodeList);
+
+    if (NodeLength > MAX_UINT16) {
+      Status = EFI_INVALID_PARAMETER;
+      DEBUG ((
+        DEBUG_ERROR,
+        "ERROR: MPAM: MSC Node length 0x%lx > MAX_UINT16. Status = %r\n",
+        NodeLength,
+        Status
+        ));
+      return Status;
+    }
+
+    // Populate the node header
+    MscNode->Length                   = (UINT16)NodeLength;
+    MscNode->Reserved                 = EFI_ACPI_RESERVED_WORD;
+    MscNode->Identifier               = EFI_ACPI_RESERVED_DWORD;
+    MscNode->BaseAddress              = NodeList->BaseAddress;
+    MscNode->MmioSize                 = NodeList->MmioSize;
+    MscNode->OverflowInterrupt        = NodeList->OverflowInterrupt;
+    MscNode->OverflowInterruptFlags   = NodeList->OverflowInterruptFlags;
+    MscNode->Reserved1                = EFI_ACPI_RESERVED_DWORD;
+    MscNode->OverflowInterruptAff     = NodeList->OverflowInterruptAff;
+    MscNode->ErrorInterrupt           = NodeList->ErrorInterrupt;
+    MscNode->ErrorInterruptFlags      = NodeList->ErrorInterruptFlags;
+    MscNode->Reserved2                = EFI_ACPI_RESERVED_DWORD;
+    MscNode->ErrorInterruptAff        = NodeList->ErrorInterruptAff;
+    MscNode->MaxNRdyUsec              = NodeList->MaxNRdyUsec;
+    MscNode->LinkedDeviceHwId         = NodeList->LinkedDeviceHwId;
+    MscNode->LinkedDeviceInstanceHwId = NodeList->LinkedDeviceInstanceHwId;
+    MscNode->NumResourceNodes         = NodeList->NumResourceNodes;
+
+    // ResourceNode List for each MSC
+    if (MscNode->NumResourceNodes > 0) {
+      // Resource Node array for this Msc node
+      ResourceNodeArray = (EFI_ACPI_6_4_MPAM_RESOURCE_NODE *)((UINT8 *)MscNode + sizeof (EFI_ACPI_6_4_MPAM_MSC_NODE));
+      // Adding Resource Node content
+      while ( MscNode->NumResourceNodes-- != 0 ) {
+        ResourceNodeArray->Identifier  = ResourceNodeList->Identifier;
+        ResourceNodeArray->RisIndex    = ResourceNodeList->RisIndex;
+        ResourceNodeArray->Reserved1   = EFI_ACPI_RESERVED_WORD;
+        ResourceNodeArray->LocatorType = ResourceNodeList->LocatorType;
+        ResourceNodeArray->Locator     = ResourceNodeList->Locator;
+        ResourceNodeArray->NumFuncDep  = ResourceNodeList->NumFuncDep;
+
+        if (EFI_ERROR (Status)) {
+          DEBUG ((
+            DEBUG_ERROR,
+            "ERROR: MPAM: Failed to add Resource Node List. Status = %r\n",
+            Status
+            ));
+          return Status;
+        }
+
+        ResourceNodeList++;
+        ResourceNodeArray++;
+      }
+    }
+
+    // Next MSC Node
+    MscNode = (EFI_ACPI_6_4_MPAM_MSC_NODE *)((UINT8 *)MscNode + MscNode->Length);
+    NodeList++;
+  } // Msc Node
+
+  return EFI_SUCCESS;
+}
+
+/**
+  Construct the MPAM ACPI table.
+
+  This function invokes the Configuration Manager protocol interface
+  to get the required hardware information for generating the ACPI
+  table.
+
+  If this function allocates any resources then they must be freed
+  in the FreeXXXXTableResources function.
+
+  @param [in]  This                 Pointer to the table generator.
+  @param [in]  AcpiTableInfo        Pointer to the ACPI table generator to be used.
+  @param [in]  CfgMgrProtocol       Pointer to the Configuration Manager
+                                    Protocol Interface.
+  @param [out] Table                Pointer to the constructed ACPI Table.
+
+  @retval EFI_SUCCESS               Table generated successfully.
+  @retval EFI_INVALID_PARAMETER     A parameter is invalid.
+  @retval EFI_NOT_FOUND             The required object was not found.
+  @retval EFI_BAD_BUFFER_SIZE       The size returned by the Configuration
+                                    Manager is less than the Object size for
+                                    the requested object.
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+BuildMpamTable (
+  IN  CONST ACPI_TABLE_GENERATOR                  *CONST  This,
+  IN  CONST CM_STD_OBJ_ACPI_TABLE_INFO            *CONST  AcpiTableInfo,
+  IN  CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  *CONST  CfgMgrProtocol,
+  OUT       EFI_ACPI_DESCRIPTION_HEADER          **CONST  Table
+  )
+{
+  EFI_STATUS  Status;
+  UINT64      TableSize;
+  UINT64      NodeSize;
+  UINT32      MpamNodeCount;
+
+  UINT32  MscNodeCount;
+  UINT32  MscNodeOffset;
+
+  EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORING_TABLE_HEADER  *Mpam;
+  ACPI_MPAM_GENERATOR                                                       *Generator;
+  CM_ARM_MSC_NODE_INFO                                                      *MscNodeList;
+  MPAM_NODE_INDEXER                                                         *NodeIndexer;
+
+  ASSERT (
+    (This != NULL) &&
+    (AcpiTableInfo != NULL) &&
+    (CfgMgrProtocol != NULL) &&
+    (Table != NULL) &&
+    (AcpiTableInfo->TableGeneratorId == This->GeneratorID) &&
+    (AcpiTableInfo->AcpiTableSignature == This->AcpiTableSignature)
+    );
+
+  DEBUG ((
+    DEBUG_ERROR,
+    "DEBUG PRINT: MPAM: Requested table revision = %d\n",
+    AcpiTableInfo->AcpiTableRevision
+    ));
+
+  if ((AcpiTableInfo->AcpiTableRevision < This->MinAcpiTableRevision) ||
+      (AcpiTableInfo->AcpiTableRevision > This->AcpiTableRevision))
+  {
+    DEBUG ((
+      DEBUG_ERROR,
+      "ERROR: MPAM: Requested table revision = %d is not supported. "
+      "Supported table revisions: Minimum = %d. Maximum = %d\n",
+      AcpiTableInfo->AcpiTableRevision,
+      This->MinAcpiTableRevision,
+      This->AcpiTableRevision
+      ));
+    return EFI_INVALID_PARAMETER;
+  }
+
+  Generator = (ACPI_MPAM_GENERATOR *)This;
+  *Table    = NULL;
+
+  // Get the Memory System Controller Node info and update the MPAM
+  // structure count with MSC Node count (Type 0)
+  Status = GetEArmObjMscNodeInfo (
+             CfgMgrProtocol,
+             CM_NULL_TOKEN,
+             &MscNodeList,
+             &MscNodeCount
+             );
+  if (EFI_ERROR (Status)) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "ERROR: MPAM: Failed to get memory system controller node info. Status = %r\n",
+      Status
+      ));
+    goto error_handler;
+  }
+
+  MpamNodeCount           = MscNodeCount;
+  Generator->MscNodeCount = MscNodeCount;
+
+  // Allocate Node Indexer array
+  NodeIndexer = (MPAM_NODE_INDEXER *)AllocateZeroPool (
+                                       sizeof (MPAM_NODE_INDEXER) *
+                                       MpamNodeCount
+                                       );
+  if (NodeIndexer == NULL) {
+    Status = EFI_OUT_OF_RESOURCES;
+    DEBUG ((
+      DEBUG_ERROR,
+      "ERROR: MPAM: Failed to allocate memory for Node Indexer. Status = %r\n ",
+      Status
+      ));
+    goto error_handler;
+  }
+
+  DEBUG ((DEBUG_INFO, "MPAM INFO: NodeIndexer = %p\n", NodeIndexer));
+  Generator->MpamNodeCount = MpamNodeCount;
+  Generator->NodeIndexer   = NodeIndexer;
+
+  // Calculate the size of the MPAM table
+  TableSize = sizeof (EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORING_TABLE_HEADER);
+
+  // Include the size of MSC Nodes and index them
+  if (Generator->MscNodeCount != 0) {
+    MscNodeOffset = TableSize;
+    // Size of MSC nodes.
+    NodeSize = GetSizeofMscGroupNodes (
+                 MscNodeOffset,
+                 MscNodeList,
+                 Generator->MscNodeCount,
+                 &NodeIndexer
+                 );
+    if (NodeSize > MAX_UINT32) {
+      Status = EFI_INVALID_PARAMETER;
+      DEBUG ((
+        DEBUG_ERROR,
+        "ERROR: MPAM: Invalid Size of Group Nodes. Status = %r\n",
+        Status
+        ));
+      goto error_handler;
+    }
+
+    TableSize += NodeSize;
+
+    DEBUG ((
+      DEBUG_INFO,
+      " MscNodeCount = %d\n" \
+      " MscNodeOffset = 0x%x\n",
+      Generator->MscNodeCount,
+      MscNodeOffset
+      ));
+  }
+
+  DEBUG ((
+    DEBUG_INFO,
+    "INFO: MPAM:\n" \
+    " MpamNodeCount = %d\n" \
+    " TableSize = 0x%X\n",
+    MpamNodeCount,
+    TableSize
+    ));
+
+  if (TableSize > MAX_UINT32) {
+    Status = EFI_INVALID_PARAMETER;
+    DEBUG ((
+      DEBUG_ERROR,
+      "ERROR: MPAM: MPAM Table Size 0x%lx > MAX_UINT32," \
+      " Status = %r\n",
+      TableSize,
+      Status
+      ));
+    goto error_handler;
+  }
+
+  // Allocate the Buffer for the MPAM table
+  *Table = (EFI_ACPI_DESCRIPTION_HEADER *)AllocateZeroPool (TableSize);
+  if (*Table == NULL) {
+    Status = EFI_OUT_OF_RESOURCES;
+    DEBUG ((
+      DEBUG_ERROR,
+      "ERROR: MPAM: Failed to allocate memory for MPAM Table. " \
+      "Size = %d. Status = %r\n",
+      TableSize,
+      Status
+      ));
+    goto error_handler;
+  }
+
+  Mpam = (EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORING_TABLE_HEADER *)*Table;
+
+  DEBUG ((
+    DEBUG_INFO,
+    "MPAM: Mpam = 0x%p. TableSize = 0x%x\n",
+    Mpam,
+    TableSize
+    ));
+
+  // Add ACPI header
+  Status = AddAcpiHeader (
+             CfgMgrProtocol,
+             This,
+             &Mpam->Header,
+             AcpiTableInfo,
+             TableSize
+             );
+  if (EFI_ERROR (Status)) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "ERROR: MPAM: Failed to add ACPI header. Status = %r\n",
+      Status
+      ));
+    goto error_handler;
+  }
+
+  // Update MPAM table
+  Mpam->NumNodes   = MscNodeCount;
+  Mpam->NodeOffset = sizeof (EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORING_TABLE_HEADER);
+  Mpam->Reserved   = EFI_ACPI_RESERVED_DWORD;
+
+  // Add MSC Nodes to the generated table
+  if (Mpam->NumNodes != 0) {
+    Status = AddMscNodes (
+               This,
+               CfgMgrProtocol,
+               Mpam,
+               MscNodeOffset,
+               MscNodeList,
+               MscNodeCount
+               );
+    if (EFI_ERROR (Status)) {
+      DEBUG ((
+        DEBUG_ERROR,
+        "ERROR: MPAM: Failed to add MSC Nodes. Status = %r\n",
+        Status
+        ));
+      goto error_handler;
+    }
+  }
+
+  return Status;
+
+error_handler:
+  if (Generator->NodeIndexer != NULL) {
+    FreePool (Generator->NodeIndexer);
+    Generator->NodeIndexer = NULL;
+  }
+
+  if (*Table != NULL) {
+    FreePool (*Table);
+    *Table = NULL;
+  }
+
+  return Status;
+}
+
+/** Free any resources allocated for constructing the MPAM
+
+  @param [in]      This           Pointer to the table generator.
+  @param [in]      AcpiTableInfo  Pointer to the ACPI Table Info.
+  @param [in]      CfgMgrProtocol Pointer to the Configuration Manager
+                                  Protocol Interface.
+  @param [in, out] Table          Pointer to the ACPI Table.
+
+  @retval EFI_SUCCESS           The resources were freed successfully.
+  @retval EFI_INVALID_PARAMETER The table pointer is NULL or invalid.
+**/
+STATIC
+EFI_STATUS
+FreeMpamTableResources (
+  IN      CONST ACPI_TABLE_GENERATOR                  *CONST  This,
+  IN      CONST CM_STD_OBJ_ACPI_TABLE_INFO            *CONST  AcpiTableInfo,
+  IN      CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  *CONST  CfgMgrProtocol,
+  IN OUT        EFI_ACPI_DESCRIPTION_HEADER          **CONST  Table
+  )
+{
+  ACPI_MPAM_GENERATOR  *Generator;
+
+  ASSERT (This != NULL);
+  ASSERT (AcpiTableInfo != NULL);
+  ASSERT (CfgMgrProtocol != NULL);
+  ASSERT (AcpiTableInfo->TableGeneratorId == This->GeneratorID);
+  ASSERT (AcpiTableInfo->AcpiTableSignature == This->AcpiTableSignature);
+
+  Generator = (ACPI_MPAM_GENERATOR *)This;
+
+  // Free any memory allocated by the generator
+  if (Generator->NodeIndexer != NULL) {
+    FreePool (Generator->NodeIndexer);
+    Generator->NodeIndexer = NULL;
+  }
+
+  if ((Table == NULL) || (*Table == NULL)) {
+    DEBUG ((DEBUG_ERROR, "ERROR: MPAM: Invalid Table Pointer\n"));
+    return EFI_INVALID_PARAMETER;
+  }
+
+  FreePool (*Table);
+  *Table = NULL;
+
+  return EFI_SUCCESS;
+}
+
+/** The MPAM Table Generator revision.
+*/
+#define MPAM_GENERATOR_REVISION  CREATE_REVISION (1, 0)
+
+/** The interface for the MPAM Table Generator.
+*/
+STATIC
+ACPI_MPAM_GENERATOR  MpamGenerator = {
+  // ACPI table generator header
+  {
+    // Generator ID
+    CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdMpam),
+    // Generator Description
+    L"ACPI.STD.MPAM.GENERATOR",
+    // ACPI Table Signature
+    EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORING_TABLE_STRUCTURE_SIGNATURE,
+    // ACPI Table Revision supported by this Generator
+    EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORING_TABLE_REVISION,
+    // Minimum supported ACPI Table Revision
+    EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORING_TABLE_REVISION,
+    // Creator ID
+    TABLE_GENERATOR_CREATOR_ID_ARM,
+    // Creator Revision
+    MPAM_GENERATOR_REVISION,
+    // Build Table function
+    BuildMpamTable,
+    // Free Resource function
+    FreeMpamTableResources,
+    // Extended build function not needed
+    NULL,
+    // Extended build function not implemented by the generator.
+    // Hence extended free resource function is not required.
+    NULL
+  },
+
+  // MPAM Generator private data
+
+  // MPAM node count
+  0,
+  // MSC node count
+  0,
+
+  // Pointer to MPAM Node Indexer
+  NULL
+};
+
+/**
+  Register the Generator with the ACPI Table Factory.
+
+  @param [in]  ImageHandle        The handle to the image.
+  @param [in]  SystemTable        Pointer to the System Table.
+
+  @retval EFI_SUCCESS             The Generator is registered.
+  @retval EFI_INVALID_PARAMETER   A parameter is invalid.
+  @retval EFI_ALREADY_STARTED     The Generator for the Table ID
+                                  is already registered.
+**/
+EFI_STATUS
+EFIAPI
+AcpiMpamLibConstructor (
+  IN  EFI_HANDLE        ImageHandle,
+  IN  EFI_SYSTEM_TABLE  *SystemTable
+  )
+{
+  EFI_STATUS  Status;
+
+  Status = RegisterAcpiTableGenerator (&MpamGenerator.Header);
+  DEBUG ((DEBUG_INFO, "MPAM: Register Generator. Status = %r\n", Status));
+  ASSERT_EFI_ERROR (Status);
+  return Status;
+}
+
+/**
+  Deregister the Generator from the ACPI Table Factory.
+
+  @param [in]  ImageHandle        The handle to the image.
+  @param [in]  SystemTable        Pointer to the System Table.
+
+  @retval EFI_SUCCESS             The Generator is deregistered.
+  @retval EFI_INVALID_PARAMETER   A parameter is invalid.
+  @retval EFI_NOT_FOUND           The Generator is not registered.
+**/
+EFI_STATUS
+EFIAPI
+AcpiMpamLibDestructor (
+  IN  EFI_HANDLE        ImageHandle,
+  IN  EFI_SYSTEM_TABLE  *SystemTable
+  )
+{
+  EFI_STATUS  Status;
+
+  Status = DeregisterAcpiTableGenerator (&MpamGenerator.Header);
+  DEBUG ((DEBUG_INFO, "MPAM: Deregister Generator. Status = %r\n", Status));
+  ASSERT_EFI_ERROR (Status);
+  return Status;
+}
diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/MpamGenerator.h b/DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/MpamGenerator.h
new file mode 100644
index 0000000000..1075bd8c6c
--- /dev/null
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/MpamGenerator.h
@@ -0,0 +1,47 @@
+/** @file
+  Header file for the dynamic MPAM generator
+
+  Copyright (c) 2022, NVIDIA CORPORATION. All rights reserved.
+  Copyright (c) 2022, ARM Limited. All rights reserved.
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+  @par Reference(s):
+  - ACPI 6.4 Specification, January 2021
+  - ARM Architecture Reference Manual ARMv8
+
+  @par Glossary:
+    - Cm or CM   - Configuration Manager
+    - Obj or OBJ - Object
+**/
+
+#ifndef MPAM_GENERATOR_H_
+#define MPAM_GENERATOR_H_
+
+#pragma pack(1)
+
+/** A structure that describes the Node indexer
+    used for indexing the MPAM MSC nodes.
+*/
+typedef struct MpamNodeIndexer {
+  /// Index token for the Node
+  CM_OBJECT_TOKEN    Token;
+  /// Pointer to the node
+  VOID               *Object;
+  /// Node offset from the start of the MPAM table
+  UINT32             Offset;
+} MPAM_NODE_INDEXER;
+
+typedef struct AcpiMpamGenerator {
+  /// ACPI Table generator header
+  ACPI_TABLE_GENERATOR    Header;
+  /// MPAM structure count
+  UINT32                  MpamNodeCount;
+  /// Count of Msc Nodes
+  UINT32                  MscNodeCount;
+  /// List of indexed CM objects for MPAM generation
+  MPAM_NODE_INDEXER       *NodeIndexer;
+} ACPI_MPAM_GENERATOR;
+
+#pragma pack()
+
+#endif // MPAM_GENERATOR_H_
-- 
2.17.1

Re: [edk2-devel] [PATCH 2/2] Dynamic Tbl Mgr: MPAM: MPAM Generator and supporting files

[Rohit Mathew]

Hi Swatisri,

Thanks for the patch. Please find my comments inline marked [Rohit] - 

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Name
> via groups.io
> Sent: 16 August 2022 21:18
> To: devel@edk2.groups.io; Sami Mujawar <Sami.Mujawar@arm.com>;
> Alexei Fedorov <Alexei.Fedorov@arm.com>; michael.d.kinney@intel.com;
> gaoliming@byosoft.com.cn; zhiguang.liu@intel.com
> Cc: Swatisri Kantamsetti <swatisrik@nvidia.com>
> Subject: [edk2-devel] [PATCH 2/2] Dynamic Tbl Mgr: MPAM: MPAM
> Generator and supporting files
> 
> From: Swatisri Kantamsetti <swatisrik@nvidia.com>
> 
> ACPI header, MSC and Resource Nodes are populated in the MPAM Table
> 
> Signed-off-by: Swatisri Kantamsetti <swatisrik@nvidia.com>
> ---
>  DynamicTablesPkg/DynamicTables.dsc.inc        |   2 +
>  DynamicTablesPkg/Include/AcpiTableGenerator.h |   1 +
>  .../Include/ArmNameSpaceObjects.h             |  68 ++
>  .../Arm/AcpiMpamLibArm/AcpiMpamLibArm.inf     |  30 +
>  .../Acpi/Arm/AcpiMpamLibArm/MpamGenerator.c   | 649
> ++++++++++++++++++
>  .../Acpi/Arm/AcpiMpamLibArm/MpamGenerator.h   |  47 ++
>  6 files changed, 797 insertions(+)
>  create mode 100644
> DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/AcpiMpamLibArm.in
> f
>  create mode 100644
> DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/MpamGenerator.c
>  create mode 100644
> DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/MpamGenerator.h
> 
> diff --git a/DynamicTablesPkg/DynamicTables.dsc.inc
> b/DynamicTablesPkg/DynamicTables.dsc.inc
> index 3d4fa0c4c4..745d5f0633 100644
> --- a/DynamicTablesPkg/DynamicTables.dsc.inc
> +++ b/DynamicTablesPkg/DynamicTables.dsc.inc
> @@ -29,6 +29,7 @@
>    DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/AcpiIortLibArm.inf
>    DynamicTablesPkg/Library/Acpi/Arm/AcpiMadtLibArm/AcpiMadtLibArm.inf
>    DynamicTablesPkg/Library/Acpi/Arm/AcpiMcfgLibArm/AcpiMcfgLibArm.inf
> +
> DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/AcpiMpamLibArm.in
> f
>    DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/AcpiPpttLibArm.inf
>    DynamicTablesPkg/Library/Acpi/Arm/AcpiRawLibArm/AcpiRawLibArm.inf
>    DynamicTablesPkg/Library/Acpi/Arm/AcpiSpcrLibArm/AcpiSpcrLibArm.inf
> @@ -54,6 +55,7 @@
> 
> NULL|DynamicTablesPkg/Library/Acpi/Arm/AcpiMadtLibArm/AcpiMadtLibAr
> m.inf
> 
> NULL|DynamicTablesPkg/Library/Acpi/Arm/AcpiMcfgLibArm/AcpiMcfgLibAr
> m.inf
> 
> NULL|DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/AcpiPpttLibArm.
> inf
> +
> +
> NULL|DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/AcpiMpamLib
> Arm.i
> + nf
> 
> NULL|DynamicTablesPkg/Library/Acpi/Arm/AcpiRawLibArm/AcpiRawLibArm.
> inf
> 
> NULL|DynamicTablesPkg/Library/Acpi/Arm/AcpiSpcrLibArm/AcpiSpcrLibArm.
> inf
> 
> NULL|DynamicTablesPkg/Library/Acpi/Arm/AcpiSratLibArm/AcpiSratLibArm.i
> nf
> diff --git a/DynamicTablesPkg/Include/AcpiTableGenerator.h
> b/DynamicTablesPkg/Include/AcpiTableGenerator.h
> index f962dbff57..56d7375b4a 100644
> --- a/DynamicTablesPkg/Include/AcpiTableGenerator.h
> +++ b/DynamicTablesPkg/Include/AcpiTableGenerator.h
> @@ -94,6 +94,7 @@ typedef enum StdAcpiTableId {
>    EStdAcpiTableIdIort,                          ///< IORT Generator
>    EStdAcpiTableIdPptt,                          ///< PPTT Generator
>    EStdAcpiTableIdSrat,                          ///< SRAT Generator
> +  EStdAcpiTableIdMpam,                          ///< MPAM Generator
>    EStdAcpiTableIdSsdtSerialPort,                ///< SSDT Serial-Port Generator
>    EStdAcpiTableIdSsdtCmn600,                    ///< SSDT Cmn-600 Generator
>    EStdAcpiTableIdSsdtCpuTopology,               ///< SSDT Cpu Topology
> diff --git a/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
> b/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
> index 102e0f96be..39a14ed0b3 100644
> --- a/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
> +++ b/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
> @@ -63,6 +63,8 @@ typedef enum ArmObjectID {
>    EArmObjPciInterruptMapInfo,          ///< 39 - Pci Interrupt Map Info
>    EArmObjRmr,                          ///< 40 - Reserved Memory Range Node
>    EArmObjMemoryRangeDescriptor,        ///< 41 - Memory Range Descriptor
> +  EArmObjMscNodeInfo,                  ///< 40 - Msc Memory System Controller
> Node Info
> +  EArmObjResNodeInfo,                  ///< 41 - Res Resource Node Info
>    EArmObjMax
>  } EARM_OBJECT_ID;
> 
> @@ -1070,6 +1072,72 @@ typedef struct CmArmRmrDescriptor {
>    UINT64    Length;
>  } CM_ARM_MEMORY_RANGE_DESCRIPTOR;
> 
> +/** A structure that describes Memory System Controller Node.
> +
> +    MPAM Memory System Component Nodes are described by
> +    this object.
> +
> +  ID: EArmObjMscNodeInfo
> +*/
> +typedef struct CmArmMscNodeInfo {
> +  /// An unique token used to identify this object
> +  CM_OBJECT_TOKEN      Token;
> +
> +  /// MPAM Base Address
> +  UINT64               BaseAddress;
> +  /// MMIO Size
> +  UINT32               MmioSize;
> +  /// Overflow Interrupt
> +  UINT32               OverflowInterrupt;
> +  /// Overflow Interrupt Flags
> +  UINT32               OverflowInterruptFlags;

[Rohit] I have added a comment on Flag's type in [PATCH 1/2]. Same comment here.

> +  /// Overflow Interrupt Affinity
> +  UINT32               OverflowInterruptAff;
> +  /// Error Interrupt
> +  UINT32               ErrorInterrupt;
> +  /// Error Interrupt Flags
> +  UINT32               ErrorInterruptFlags;
> +  /// Error Interrupt Affinity
> +  UINT32               ErrorInterruptAff;
> +  /// Not Ready Signal time
> +  UINT32               MaxNRdyUsec;
> +  /// Linked Device HWID
> +  UINT64               LinkedDeviceHwId;
> +  /// Linked Device Instance ID
> +  UINT32               LinkedDeviceInstanceHwId;
> +  /// Number of Resource nodes
> +  UINT32               NumResourceNodes;
> +  /// Reference token for the list of resource nodes
> +  //CM_OBJECT_TOKEN    ResourceNodeListToken;
> +
> +} CM_ARM_MSC_NODE_INFO;
> +
> +/** A structure that describes Memory System Controller Node.
> +
> +    MPAM Memory System Component Nodes are described by
> +    this object.
> +
> +  ID: EArmObjResNodeInfo
> +*/
> +typedef struct CmArmResNodeInfo {
> +  /// An unique token used to identify this object
> +  CM_OBJECT_TOKEN    Token;
> +
> +  /// Identifier
> +  UINT32             Identifier;
> +  /// RIS Index
> +  UINT8              RisIndex;
> +  /// Locator Type
> +  UINT8              LocatorType;
> +  /// Locator
> +  UINT64             Locator;

[Rohit] I have added a comment on Locator's type and size in [PATCH 1/2]. Same comment here.

> +  /// Num functional dependencies
> +  UINT32             NumFuncDep;
> +  /// Reference token for the list of resource nodes
> +  CM_OBJECT_TOKEN    FuncDepListToken;
> +
> +} CM_ARM_RESOURCE_NODE_INFO;
> +
>  #pragma pack()
> 
>  #endif // ARM_NAMESPACE_OBJECTS_H_
> diff --git
> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/AcpiMpamLibArm
> .inf
> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/AcpiMpamLibArm
> .inf
> new file mode 100644
> index 0000000000..480130dc21
> --- /dev/null
> +++
> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/AcpiMpamLibArm
> .in
> +++ f
> @@ -0,0 +1,30 @@
> +## @file
> +#  MPAM Table Generator Inf file
> +#
> +#  Copyright (c) 2022, NVIDIA CORPORATION. All rights reserved.
> +#  Copyright (c) 2022, ARM Limited. All rights reserved.
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent ##
> +
> +[Defines]
> +  INF_VERSION    = 0x0001001B
> +  BASE_NAME      = AcpiMpamLibArm
> +  FILE_GUID      = 02d0c79f-41cd-45c9-9835-781229c619d1
> +  VERSION_STRING = 1.0
> +  MODULE_TYPE    = DXE_DRIVER
> +  LIBRARY_CLASS  = NULL|DXE_DRIVER
> +  CONSTRUCTOR    = AcpiMpamLibConstructor
> +  DESTRUCTOR     = AcpiMpamLibDestructor
> +
> +[Sources]
> +  MpamGenerator.c
> +  MpamGenerator.h
> +
> +[Packages]
> +  EmbeddedPkg/EmbeddedPkg.dec
> +  DynamicTablesPkg/DynamicTablesPkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  MdePkg/MdePkg.dec
> +
> +[LibraryClasses]
> +  BaseLib
> diff --git
> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/MpamGenerator.
> c
> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/MpamGenerator.
> c
> new file mode 100644
> index 0000000000..db3e8e95bc
> --- /dev/null
> +++
> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/MpamGenerator.
> c
> @@ -0,0 +1,649 @@
> +/** @file
> +  MPAM Table Generator
> +
> +  Copyright (c) 2022, NVIDIA CORPORATION. All rights reserved.
> +  Copyright (c) 2022, ARM Limited. All rights reserved.
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +  @par Reference(s):
> +  - ACPI 6.4 Specification, January 2021
> +
> +  @par Glossary:
> +  - Cm or CM   - Configuration Manager
> +  - Obj or OBJ - Object
> +**/
> +
> +#include <IndustryStandard/Mpam.h>
> +#include <Library/AcpiLib.h>
> +#include <Library/BaseLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/MemoryAllocationLib.h> #include
> +<Protocol/AcpiTable.h>
> +
> +// Module specific include files.
> +#include <AcpiTableGenerator.h>
> +#include <ConfigurationManagerObject.h> #include
> +<ConfigurationManagerHelper.h> #include <Library/TableHelperLib.h>
> +#include <Protocol/ConfigurationManagerProtocol.h>
> +
> +#include "MpamGenerator.h"
> +
> +/**
> +  ARM standard MPAM Generator
> +
> +  Requirements:
> +    The following Configuration Manager Object(s) are used by this
> Generator:
> +    - EArmObjMscNodeInfo (REQUIRED)
> +    - EArmObjResNodeInfo
> +*/
> +
> +/**
> +  This macro expands to a function that retrieves the MSC Node
> +information
> +  from the Configuration Manager.
> +*/
> +GET_OBJECT_LIST (
> +  EObjNameSpaceArm,
> +  EArmObjMscNodeInfo,
> +  CM_ARM_MSC_NODE_INFO
> +  );
> +
> +/**
> +  This macro expands to a function that retrieves the Resource Node
> +  information from the Configuration Manager.
> +*/
> +GET_OBJECT_LIST (
> +  EObjNameSpaceArm,
> +  EArmObjResNodeInfo,
> +  CM_ARM_RESOURCE_NODE_INFO
> +  );
> +
> +/**
> +  Returns the size of the MPAM Memory System Controller (MSC) Node
> +
> +  @param [in]  Node     Pointer to MSC Node Info CM object
> +
> +  @retval               Size of the MSC Node in bytes.
> +**/
> +STATIC
> +UINT32
> +GetMscNodeSize (
> +  IN  CONST CM_ARM_MSC_NODE_INFO  *Node
> +  )
> +{
> +  ASSERT (Node != NULL);
> +
> +  // <size of Memory System Controller Node>
> +  return (UINT32)(sizeof (EFI_ACPI_6_4_MPAM_MSC_NODE) +
> +                  Node->NumResourceNodes * sizeof
> +(EFI_ACPI_6_4_MPAM_RESOURCE_NODE));

[Rohit] Shouldn't the node size include the size of functional dependency descriptor * number of functional dependency descriptor?

> +}
> +
> +/** Returns the total size required for the MSC and
> +    updates the Node Indexer.
> +
> +    This function calculates the size required for the node group
> +    and also populates the Node Indexer array with offsets for the
> +    individual nodes.
> +
> +    @param [in]       NodeStartOffset Offset from the start of the
> +                                      MPAM where this node group starts.

[Rohit] Should it be "start of the MPAM table where .."?

> +    @param [in]       NodeList        Pointer to MSC Group node list.
> +    @param [in]       NodeCount       Count of the MSC Group nodes.
> +    @param [in, out]  NodeIndexer     Pointer to the next Node Indexer.
> +
> +    @retval Total size of the ITS Group Nodes.
> +**/
> +STATIC
> +UINT64
> +GetSizeofMscGroupNodes (
> +  IN      CONST UINT32                         NodeStartOffset,
> +  IN      CONST CM_ARM_MSC_NODE_INFO           *NodeList,
> +  IN            UINT32                         NodeCount,
> +  IN OUT        MPAM_NODE_INDEXER     **CONST  NodeIndexer
> +  )
> +{
> +  UINT64  Size;
> +
> +  ASSERT (NodeList != NULL);
> +
> +  Size = 0;
> +  while (NodeCount-- != 0) {
> +    (*NodeIndexer)->Token  = NodeList->Token;
> +    (*NodeIndexer)->Object = (VOID *)NodeList;
> +    (*NodeIndexer)->Offset = (UINT32)(Size + NodeStartOffset);
> +    DEBUG ((
> +      DEBUG_INFO,
> +      "MPAM: MSC Node Indexer = %p, Token = %p, Object = %p, Offset =
> 0x%x\n",
> +      *NodeIndexer,
> +      (*NodeIndexer)->Token,
> +      (*NodeIndexer)->Object,
> +      (*NodeIndexer)->Offset
> +      ));
> +
> +    Size += GetMscNodeSize (NodeList);
> +
> +    (*NodeIndexer)++;
> +    NodeList++;
> +  }
> +
> +  return Size;
> +}
> +
> +/** Update the MSC Group Node Information.
> +
> +    @param [in]     This             Pointer to the table Generator.
> +    @param [in]     CfgMgrProtocol   Pointer to the Configuration Manager
> +                                     Protocol Interface.
> +    @param [in]     Mpam             Pointer to MPAM table structure.
> +    @param [in]     NodesStartOffset Offset for the start of the Msc Group
> +                                     Nodes.
> +    @param [in]     NodeList         Pointer to an array of Msc Group Node
> +                                     Objects.
> +    @param [in]     NodeCount        Number of Msc Group Node Objects.
> +
> +    @retval EFI_SUCCESS           Table generated successfully.
> +    @retval EFI_INVALID_PARAMETER A parameter is invalid.
> +    @retval EFI_NOT_FOUND         The required object was not found.
> +**/
> +STATIC
> +EFI_STATUS
> +AddMscNodes (
> +  IN      CONST ACPI_TABLE_GENERATOR                                                       *CONST
> This,
> +  IN      CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL
> *CONST  CfgMgrProtocol,
> +  IN      CONST
> EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORIN
> G_TABLE_HEADER           *Mpam,
> +  IN      CONST UINT32
> NodesStartOffset,
> +  IN      CONST CM_ARM_MSC_NODE_INFO
> *NodeList,
> +  IN            UINT32                                                                             NodeCount
> +  )
> +{
> +  EFI_STATUS                       Status;
> +  EFI_ACPI_6_4_MPAM_MSC_NODE       *MscNode;
> +  EFI_ACPI_6_4_MPAM_RESOURCE_NODE  *ResourceNodeArray;
> +  CM_ARM_RESOURCE_NODE_INFO        *ResourceNodeList;
> +  UINT64                           NodeLength;
> +  UINT32                           ResourceNodeCount;
> +
> +  ASSERT (Mpam != NULL);
> +
> +  MscNode = (EFI_ACPI_6_4_MPAM_MSC_NODE *)((UINT8 *)Mpam +
> + NodesStartOffset);
> +
> +  // Get the Resource Node info to update the MPAM MSC node  Status =
> + GetEArmObjResNodeInfo (
> +             CfgMgrProtocol,
> +             CM_NULL_TOKEN,
> +             &ResourceNodeList,
> +             &ResourceNodeCount
> +             );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "ERROR: MPAM: Failed to add resource nodes info. Status = %r\n",
> +      Status
> +      ));
> +    return Status;
> +  }
> +
> +  while (NodeCount-- != 0) {
> +    NodeLength = GetMscNodeSize (NodeList);
> +
> +    if (NodeLength > MAX_UINT16) {
> +      Status = EFI_INVALID_PARAMETER;
> +      DEBUG ((
> +        DEBUG_ERROR,
> +        "ERROR: MPAM: MSC Node length 0x%lx > MAX_UINT16. Status =
> %r\n",
> +        NodeLength,
> +        Status
> +        ));
> +      return Status;
> +    }
> +
> +    // Populate the node header
> +    MscNode->Length                   = (UINT16)NodeLength;
> +    MscNode->Reserved                 = EFI_ACPI_RESERVED_WORD;
> +    MscNode->Identifier               = EFI_ACPI_RESERVED_DWORD;

[Rohit] MPAM ACPI 1.0, section 2.1, table 4 - MSC node body, defines identifier as a unique value. For systems with multiple MSCs, the OS could expect identifiers to be unique. Could this be addressed, please?

> +    MscNode->BaseAddress              = NodeList->BaseAddress;
> +    MscNode->MmioSize                 = NodeList->MmioSize;
> +    MscNode->OverflowInterrupt        = NodeList->OverflowInterrupt;
> +    MscNode->OverflowInterruptFlags   = NodeList->OverflowInterruptFlags;
> +    MscNode->Reserved1                = EFI_ACPI_RESERVED_DWORD;
> +    MscNode->OverflowInterruptAff     = NodeList->OverflowInterruptAff;
> +    MscNode->ErrorInterrupt           = NodeList->ErrorInterrupt;
> +    MscNode->ErrorInterruptFlags      = NodeList->ErrorInterruptFlags;
> +    MscNode->Reserved2                = EFI_ACPI_RESERVED_DWORD;
> +    MscNode->ErrorInterruptAff        = NodeList->ErrorInterruptAff;
> +    MscNode->MaxNRdyUsec              = NodeList->MaxNRdyUsec;
> +    MscNode->LinkedDeviceHwId         = NodeList->LinkedDeviceHwId;
> +    MscNode->LinkedDeviceInstanceHwId = NodeList-
> >LinkedDeviceInstanceHwId;
> +    MscNode->NumResourceNodes         = NodeList->NumResourceNodes;
> +
> +    // ResourceNode List for each MSC
> +    if (MscNode->NumResourceNodes > 0) {
> +      // Resource Node array for this Msc node
> +      ResourceNodeArray = (EFI_ACPI_6_4_MPAM_RESOURCE_NODE
> *)((UINT8 *)MscNode + sizeof (EFI_ACPI_6_4_MPAM_MSC_NODE));
> +      // Adding Resource Node content
> +      while ( MscNode->NumResourceNodes-- != 0 ) {
> +        ResourceNodeArray->Identifier  = ResourceNodeList->Identifier;
> +        ResourceNodeArray->RisIndex    = ResourceNodeList->RisIndex;
> +        ResourceNodeArray->Reserved1   = EFI_ACPI_RESERVED_WORD;
> +        ResourceNodeArray->LocatorType = ResourceNodeList->LocatorType;
> +        ResourceNodeArray->Locator     = ResourceNodeList->Locator;
> +        ResourceNodeArray->NumFuncDep  = ResourceNodeList-
> >NumFuncDep;

[Rohit] If NumFuncDep > 0, shouldn't we need an additional loop to populate functional dependency list?

> +
> +        if (EFI_ERROR (Status)) {
> +          DEBUG ((
> +            DEBUG_ERROR,
> +            "ERROR: MPAM: Failed to add Resource Node List. Status = %r\n",
> +            Status
> +            ));
> +          return Status;
> +        }
> +
> +        ResourceNodeList++;
> +        ResourceNodeArray++;
> +      }
> +    }
> +
> +    // Next MSC Node
> +    MscNode = (EFI_ACPI_6_4_MPAM_MSC_NODE *)((UINT8 *)MscNode +
> MscNode->Length);
> +    NodeList++;
> +  } // Msc Node
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  Construct the MPAM ACPI table.
> +
> +  This function invokes the Configuration Manager protocol interface
> + to get the required hardware information for generating the ACPI
> + table.
> +
> +  If this function allocates any resources then they must be freed  in
> + the FreeXXXXTableResources function.
> +
> +  @param [in]  This                 Pointer to the table generator.
> +  @param [in]  AcpiTableInfo        Pointer to the ACPI table generator to be
> used.
> +  @param [in]  CfgMgrProtocol       Pointer to the Configuration Manager
> +                                    Protocol Interface.
> +  @param [out] Table                Pointer to the constructed ACPI Table.
> +
> +  @retval EFI_SUCCESS               Table generated successfully.
> +  @retval EFI_INVALID_PARAMETER     A parameter is invalid.
> +  @retval EFI_NOT_FOUND             The required object was not found.
> +  @retval EFI_BAD_BUFFER_SIZE       The size returned by the Configuration
> +                                    Manager is less than the Object size for
> +                                    the requested object.
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +BuildMpamTable (
> +  IN  CONST ACPI_TABLE_GENERATOR                  *CONST  This,
> +  IN  CONST CM_STD_OBJ_ACPI_TABLE_INFO            *CONST  AcpiTableInfo,
> +  IN  CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  *CONST
> CfgMgrProtocol,
> +  OUT       EFI_ACPI_DESCRIPTION_HEADER          **CONST  Table
> +  )
> +{
> +  EFI_STATUS  Status;
> +  UINT64      TableSize;
> +  UINT64      NodeSize;
> +  UINT32      MpamNodeCount;
> +
> +  UINT32  MscNodeCount;
> +  UINT32  MscNodeOffset;
> +
> +
> EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORIN
> G_TABLE_HEADER  *Mpam;
> +  ACPI_MPAM_GENERATOR                                                       *Generator;
> +  CM_ARM_MSC_NODE_INFO                                                      *MscNodeList;
> +  MPAM_NODE_INDEXER                                                         *NodeIndexer;
> +
> +  ASSERT (
> +    (This != NULL) &&
> +    (AcpiTableInfo != NULL) &&
> +    (CfgMgrProtocol != NULL) &&
> +    (Table != NULL) &&
> +    (AcpiTableInfo->TableGeneratorId == This->GeneratorID) &&
> +    (AcpiTableInfo->AcpiTableSignature == This->AcpiTableSignature)
> +    );
> +
> +  DEBUG ((
> +    DEBUG_ERROR,
> +    "DEBUG PRINT: MPAM: Requested table revision = %d\n",
> +    AcpiTableInfo->AcpiTableRevision
> +    ));
> +
> +  if ((AcpiTableInfo->AcpiTableRevision < This->MinAcpiTableRevision) ||
> +      (AcpiTableInfo->AcpiTableRevision > This->AcpiTableRevision))  {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "ERROR: MPAM: Requested table revision = %d is not supported. "
> +      "Supported table revisions: Minimum = %d. Maximum = %d\n",
> +      AcpiTableInfo->AcpiTableRevision,
> +      This->MinAcpiTableRevision,
> +      This->AcpiTableRevision
> +      ));
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  Generator = (ACPI_MPAM_GENERATOR *)This;
> +  *Table    = NULL;
> +
> +  // Get the Memory System Controller Node info and update the MPAM  //
> + structure count with MSC Node count (Type 0)  Status =
> + GetEArmObjMscNodeInfo (
> +             CfgMgrProtocol,
> +             CM_NULL_TOKEN,
> +             &MscNodeList,
> +             &MscNodeCount
> +             );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "ERROR: MPAM: Failed to get memory system controller node info.
> Status = %r\n",
> +      Status
> +      ));
> +    goto error_handler;
> +  }
> +
> +  MpamNodeCount           = MscNodeCount;
> +  Generator->MscNodeCount = MscNodeCount;
> +
> +  // Allocate Node Indexer array
> +  NodeIndexer = (MPAM_NODE_INDEXER *)AllocateZeroPool (
> +                                       sizeof (MPAM_NODE_INDEXER) *
> +                                       MpamNodeCount
> +                                       );  if (NodeIndexer == NULL) {
> +    Status = EFI_OUT_OF_RESOURCES;
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "ERROR: MPAM: Failed to allocate memory for Node Indexer. Status =
> %r\n ",
> +      Status
> +      ));
> +    goto error_handler;
> +  }
> +
> +  DEBUG ((DEBUG_INFO, "MPAM INFO: NodeIndexer = %p\n",
> NodeIndexer));
> + Generator->MpamNodeCount = MpamNodeCount;
> +  Generator->NodeIndexer   = NodeIndexer;
> +
> +  // Calculate the size of the MPAM table  TableSize = sizeof
> +
> (EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORIN
> G_TABLE_HEA
> + DER);
> +
> +  // Include the size of MSC Nodes and index them  if
> + (Generator->MscNodeCount != 0) {
> +    MscNodeOffset = TableSize;
> +    // Size of MSC nodes.
> +    NodeSize = GetSizeofMscGroupNodes (
> +                 MscNodeOffset,
> +                 MscNodeList,
> +                 Generator->MscNodeCount,
> +                 &NodeIndexer
> +                 );

[Rohit] MscNodeOffset is sizeof (EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORING_TABLE_HEADER), which moves past EFI_ACPI_DESCRIPTION_HEADER by 12 bytes. Should we be having these fields in the table type?
  UINT32                         NumNodes;
  UINT32                         NodeOffset;
  UINT32                         Reserved;

I had added a similar comment on [PATCH 1/2].
[/Rohit]

> +    if (NodeSize > MAX_UINT32) {
> +      Status = EFI_INVALID_PARAMETER;
> +      DEBUG ((
> +        DEBUG_ERROR,
> +        "ERROR: MPAM: Invalid Size of Group Nodes. Status = %r\n",
> +        Status
> +        ));
> +      goto error_handler;
> +    }
> +
> +    TableSize += NodeSize;
> +
> +    DEBUG ((
> +      DEBUG_INFO,
> +      " MscNodeCount = %d\n" \
> +      " MscNodeOffset = 0x%x\n",
> +      Generator->MscNodeCount,
> +      MscNodeOffset
> +      ));
> +  }
> +
> +  DEBUG ((
> +    DEBUG_INFO,
> +    "INFO: MPAM:\n" \
> +    " MpamNodeCount = %d\n" \
> +    " TableSize = 0x%X\n",
> +    MpamNodeCount,
> +    TableSize
> +    ));
> +
> +  if (TableSize > MAX_UINT32) {
> +    Status = EFI_INVALID_PARAMETER;
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "ERROR: MPAM: MPAM Table Size 0x%lx > MAX_UINT32," \
> +      " Status = %r\n",
> +      TableSize,
> +      Status
> +      ));
> +    goto error_handler;
> +  }
> +
> +  // Allocate the Buffer for the MPAM table  *Table =
> + (EFI_ACPI_DESCRIPTION_HEADER *)AllocateZeroPool (TableSize);  if
> + (*Table == NULL) {
> +    Status = EFI_OUT_OF_RESOURCES;
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "ERROR: MPAM: Failed to allocate memory for MPAM Table. " \
> +      "Size = %d. Status = %r\n",
> +      TableSize,
> +      Status
> +      ));
> +    goto error_handler;
> +  }
> +
> +  Mpam =
> +
> (EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORIN
> G_TABLE_HEA
> + DER *)*Table;
> +
> +  DEBUG ((
> +    DEBUG_INFO,
> +    "MPAM: Mpam = 0x%p. TableSize = 0x%x\n",
> +    Mpam,
> +    TableSize
> +    ));
> +
> +  // Add ACPI header
> +  Status = AddAcpiHeader (
> +             CfgMgrProtocol,
> +             This,
> +             &Mpam->Header,
> +             AcpiTableInfo,
> +             TableSize
> +             );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "ERROR: MPAM: Failed to add ACPI header. Status = %r\n",
> +      Status
> +      ));
> +    goto error_handler;
> +  }
> +
> +  // Update MPAM table
> +  Mpam->NumNodes   = MscNodeCount;
> +  Mpam->NodeOffset = sizeof
> (EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORIN
> G_TABLE_HEADER);
> +  Mpam->Reserved   = EFI_ACPI_RESERVED_DWORD;
> +
> +  // Add MSC Nodes to the generated table  if (Mpam->NumNodes != 0) {
> +    Status = AddMscNodes (
> +               This,
> +               CfgMgrProtocol,
> +               Mpam,
> +               MscNodeOffset,
> +               MscNodeList,
> +               MscNodeCount
> +               );

[Rohit] Same comment as above

> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((
> +        DEBUG_ERROR,
> +        "ERROR: MPAM: Failed to add MSC Nodes. Status = %r\n",
> +        Status
> +        ));
> +      goto error_handler;
> +    }
> +  }
> +
> +  return Status;
> +
> +error_handler:
> +  if (Generator->NodeIndexer != NULL) {
> +    FreePool (Generator->NodeIndexer);
> +    Generator->NodeIndexer = NULL;
> +  }
> +
> +  if (*Table != NULL) {
> +    FreePool (*Table);
> +    *Table = NULL;
> +  }
> +
> +  return Status;
> +}
> +
> +/** Free any resources allocated for constructing the MPAM

[Rohit] Should this be "constructing the MPAM table"?

> +
> +  @param [in]      This           Pointer to the table generator.
> +  @param [in]      AcpiTableInfo  Pointer to the ACPI Table Info.
> +  @param [in]      CfgMgrProtocol Pointer to the Configuration Manager
> +                                  Protocol Interface.
> +  @param [in, out] Table          Pointer to the ACPI Table.
> +
> +  @retval EFI_SUCCESS           The resources were freed successfully.
> +  @retval EFI_INVALID_PARAMETER The table pointer is NULL or invalid.
> +**/
> +STATIC
> +EFI_STATUS
> +FreeMpamTableResources (
> +  IN      CONST ACPI_TABLE_GENERATOR                  *CONST  This,
> +  IN      CONST CM_STD_OBJ_ACPI_TABLE_INFO            *CONST
> AcpiTableInfo,
> +  IN      CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  *CONST
> CfgMgrProtocol,
> +  IN OUT        EFI_ACPI_DESCRIPTION_HEADER          **CONST  Table
> +  )
> +{
> +  ACPI_MPAM_GENERATOR  *Generator;
> +
> +  ASSERT (This != NULL);
> +  ASSERT (AcpiTableInfo != NULL);
> +  ASSERT (CfgMgrProtocol != NULL);
> +  ASSERT (AcpiTableInfo->TableGeneratorId == This->GeneratorID);
> + ASSERT (AcpiTableInfo->AcpiTableSignature ==
> + This->AcpiTableSignature);
> +
> +  Generator = (ACPI_MPAM_GENERATOR *)This;
> +
> +  // Free any memory allocated by the generator  if
> + (Generator->NodeIndexer != NULL) {
> +    FreePool (Generator->NodeIndexer);
> +    Generator->NodeIndexer = NULL;
> +  }
> +
> +  if ((Table == NULL) || (*Table == NULL)) {
> +    DEBUG ((DEBUG_ERROR, "ERROR: MPAM: Invalid Table Pointer\n"));
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  FreePool (*Table);
> +  *Table = NULL;
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/** The MPAM Table Generator revision.
> +*/
> +#define MPAM_GENERATOR_REVISION  CREATE_REVISION (1, 0)
> +
> +/** The interface for the MPAM Table Generator.
> +*/
> +STATIC
> +ACPI_MPAM_GENERATOR  MpamGenerator = {
> +  // ACPI table generator header
> +  {
> +    // Generator ID
> +    CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdMpam),
> +    // Generator Description
> +    L"ACPI.STD.MPAM.GENERATOR",
> +    // ACPI Table Signature
> +
> EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORIN
> G_TABLE_STRUCTURE_SIGNATURE,
> +    // ACPI Table Revision supported by this Generator
> +
> EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORIN
> G_TABLE_REVISION,
> +    // Minimum supported ACPI Table Revision
> +
> EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORIN
> G_TABLE_REVISION,
> +    // Creator ID
> +    TABLE_GENERATOR_CREATOR_ID_ARM,
> +    // Creator Revision
> +    MPAM_GENERATOR_REVISION,
> +    // Build Table function
> +    BuildMpamTable,
> +    // Free Resource function
> +    FreeMpamTableResources,
> +    // Extended build function not needed
> +    NULL,
> +    // Extended build function not implemented by the generator.
> +    // Hence extended free resource function is not required.
> +    NULL
> +  },
> +
> +  // MPAM Generator private data
> +
> +  // MPAM node count
> +  0,
> +  // MSC node count
> +  0,
> +
> +  // Pointer to MPAM Node Indexer
> +  NULL
> +};
> +
> +/**
> +  Register the Generator with the ACPI Table Factory.
> +
> +  @param [in]  ImageHandle        The handle to the image.
> +  @param [in]  SystemTable        Pointer to the System Table.
> +
> +  @retval EFI_SUCCESS             The Generator is registered.
> +  @retval EFI_INVALID_PARAMETER   A parameter is invalid.
> +  @retval EFI_ALREADY_STARTED     The Generator for the Table ID
> +                                  is already registered.
> +**/
> +EFI_STATUS
> +EFIAPI
> +AcpiMpamLibConstructor (
> +  IN  EFI_HANDLE        ImageHandle,
> +  IN  EFI_SYSTEM_TABLE  *SystemTable
> +  )
> +{
> +  EFI_STATUS  Status;
> +
> +  Status = RegisterAcpiTableGenerator (&MpamGenerator.Header);
> +  DEBUG ((DEBUG_INFO, "MPAM: Register Generator. Status = %r\n",
> +Status));
> +  ASSERT_EFI_ERROR (Status);
> +  return Status;
> +}
> +
> +/**
> +  Deregister the Generator from the ACPI Table Factory.
> +
> +  @param [in]  ImageHandle        The handle to the image.
> +  @param [in]  SystemTable        Pointer to the System Table.
> +
> +  @retval EFI_SUCCESS             The Generator is deregistered.
> +  @retval EFI_INVALID_PARAMETER   A parameter is invalid.
> +  @retval EFI_NOT_FOUND           The Generator is not registered.
> +**/
> +EFI_STATUS
> +EFIAPI
> +AcpiMpamLibDestructor (
> +  IN  EFI_HANDLE        ImageHandle,
> +  IN  EFI_SYSTEM_TABLE  *SystemTable
> +  )
> +{
> +  EFI_STATUS  Status;
> +
> +  Status = DeregisterAcpiTableGenerator (&MpamGenerator.Header);
> +  DEBUG ((DEBUG_INFO, "MPAM: Deregister Generator. Status = %r\n",
> +Status));
> +  ASSERT_EFI_ERROR (Status);
> +  return Status;
> +}
> diff --git
> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/MpamGenerator.
> h
> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/MpamGenerator.
> h
> new file mode 100644
> index 0000000000..1075bd8c6c
> --- /dev/null
> +++
> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/MpamGenerator.
> h
> @@ -0,0 +1,47 @@
> +/** @file
> +  Header file for the dynamic MPAM generator
> +
> +  Copyright (c) 2022, NVIDIA CORPORATION. All rights reserved.
> +  Copyright (c) 2022, ARM Limited. All rights reserved.
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +  @par Reference(s):
> +  - ACPI 6.4 Specification, January 2021
> +  - ARM Architecture Reference Manual ARMv8
> +
> +  @par Glossary:
> +    - Cm or CM   - Configuration Manager
> +    - Obj or OBJ - Object
> +**/
> +
> +#ifndef MPAM_GENERATOR_H_
> +#define MPAM_GENERATOR_H_
> +
> +#pragma pack(1)
> +
> +/** A structure that describes the Node indexer
> +    used for indexing the MPAM MSC nodes.
> +*/
> +typedef struct MpamNodeIndexer {
> +  /// Index token for the Node
> +  CM_OBJECT_TOKEN    Token;
> +  /// Pointer to the node
> +  VOID               *Object;
> +  /// Node offset from the start of the MPAM table
> +  UINT32             Offset;
> +} MPAM_NODE_INDEXER;
> +
> +typedef struct AcpiMpamGenerator {
> +  /// ACPI Table generator header
> +  ACPI_TABLE_GENERATOR    Header;
> +  /// MPAM structure count
> +  UINT32                  MpamNodeCount;
> +  /// Count of Msc Nodes
> +  UINT32                  MscNodeCount;
> +  /// List of indexed CM objects for MPAM generation
> +  MPAM_NODE_INDEXER       *NodeIndexer;
> +} ACPI_MPAM_GENERATOR;
> +
> +#pragma pack()
> +
> +#endif // MPAM_GENERATOR_H_
> --
> 2.17.1
> 
> 
> 
> 
> 

Regards,
Rohit

Re: [edk2-devel] [PATCH 2/2] Dynamic Tbl Mgr: MPAM: MPAM Generator and supporting files

[Rohit Mathew]

Hi Swatisri,

I have dropped comments on the Flag's type based on Andrew's feedback.

Regards,
Rohit

> -----Original Message-----
> From: Rohit Mathew
> Sent: 19 August 2022 09:26
> To: devel@edk2.groups.io; username@nvidia.com; Sami Mujawar
> <Sami.Mujawar@arm.com>; Alexei Fedorov <Alexei.Fedorov@arm.com>;
> michael.d.kinney@intel.com; gaoliming@byosoft.com.cn;
> zhiguang.liu@intel.com
> Cc: Swatisri Kantamsetti <swatisrik@nvidia.com>; Thomas Abraham
> <thomas.abraham@arm.com>; Thanu Rangarajan
> <Thanu.Rangarajan@arm.com>; nd <nd@arm.com>
> Subject: RE: [edk2-devel] [PATCH 2/2] Dynamic Tbl Mgr: MPAM: MPAM
> Generator and supporting files
> 
> Hi Swatisri,
> 
> Thanks for the patch. Please find my comments inline marked [Rohit] -
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Name
> > via groups.io
> > Sent: 16 August 2022 21:18
> > To: devel@edk2.groups.io; Sami Mujawar <Sami.Mujawar@arm.com>;
> Alexei
> > Fedorov <Alexei.Fedorov@arm.com>; michael.d.kinney@intel.com;
> > gaoliming@byosoft.com.cn; zhiguang.liu@intel.com
> > Cc: Swatisri Kantamsetti <swatisrik@nvidia.com>
> > Subject: [edk2-devel] [PATCH 2/2] Dynamic Tbl Mgr: MPAM: MPAM
> > Generator and supporting files
> >
> > From: Swatisri Kantamsetti <swatisrik@nvidia.com>
> >
> > ACPI header, MSC and Resource Nodes are populated in the MPAM Table
> >
> > Signed-off-by: Swatisri Kantamsetti <swatisrik@nvidia.com>
> > ---
> >  DynamicTablesPkg/DynamicTables.dsc.inc        |   2 +
> >  DynamicTablesPkg/Include/AcpiTableGenerator.h |   1 +
> >  .../Include/ArmNameSpaceObjects.h             |  68 ++
> >  .../Arm/AcpiMpamLibArm/AcpiMpamLibArm.inf     |  30 +
> >  .../Acpi/Arm/AcpiMpamLibArm/MpamGenerator.c   | 649
> > ++++++++++++++++++
> >  .../Acpi/Arm/AcpiMpamLibArm/MpamGenerator.h   |  47 ++
> >  6 files changed, 797 insertions(+)
> >  create mode 100644
> >
> DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/AcpiMpamLibArm.in
> > f
> >  create mode 100644
> > DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/MpamGenerator.c
> >  create mode 100644
> >
> DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/MpamGenerator.h
> >
> > diff --git a/DynamicTablesPkg/DynamicTables.dsc.inc
> > b/DynamicTablesPkg/DynamicTables.dsc.inc
> > index 3d4fa0c4c4..745d5f0633 100644
> > --- a/DynamicTablesPkg/DynamicTables.dsc.inc
> > +++ b/DynamicTablesPkg/DynamicTables.dsc.inc
> > @@ -29,6 +29,7 @@
> >    DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/AcpiIortLibArm.inf
> >
> DynamicTablesPkg/Library/Acpi/Arm/AcpiMadtLibArm/AcpiMadtLibArm.inf
> >
> DynamicTablesPkg/Library/Acpi/Arm/AcpiMcfgLibArm/AcpiMcfgLibArm.inf
> > +
> >
> DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/AcpiMpamLibArm.in
> > f
> >    DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/AcpiPpttLibArm.inf
> >    DynamicTablesPkg/Library/Acpi/Arm/AcpiRawLibArm/AcpiRawLibArm.inf
> >    DynamicTablesPkg/Library/Acpi/Arm/AcpiSpcrLibArm/AcpiSpcrLibArm.inf
> > @@ -54,6 +55,7 @@
> >
> >
> NULL|DynamicTablesPkg/Library/Acpi/Arm/AcpiMadtLibArm/AcpiMadtLibAr
> > m.inf
> >
> >
> NULL|DynamicTablesPkg/Library/Acpi/Arm/AcpiMcfgLibArm/AcpiMcfgLibAr
> > m.inf
> >
> >
> NULL|DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/AcpiPpttLibArm.
> > inf
> > +
> > +
> >
> NULL|DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/AcpiMpamLib
> > Arm.i
> > + nf
> >
> >
> NULL|DynamicTablesPkg/Library/Acpi/Arm/AcpiRawLibArm/AcpiRawLibArm.
> > inf
> >
> >
> NULL|DynamicTablesPkg/Library/Acpi/Arm/AcpiSpcrLibArm/AcpiSpcrLibArm.
> > inf
> >
> >
> NULL|DynamicTablesPkg/Library/Acpi/Arm/AcpiSratLibArm/AcpiSratLibArm.i
> > nf
> > diff --git a/DynamicTablesPkg/Include/AcpiTableGenerator.h
> > b/DynamicTablesPkg/Include/AcpiTableGenerator.h
> > index f962dbff57..56d7375b4a 100644
> > --- a/DynamicTablesPkg/Include/AcpiTableGenerator.h
> > +++ b/DynamicTablesPkg/Include/AcpiTableGenerator.h
> > @@ -94,6 +94,7 @@ typedef enum StdAcpiTableId {
> >    EStdAcpiTableIdIort,                          ///< IORT Generator
> >    EStdAcpiTableIdPptt,                          ///< PPTT Generator
> >    EStdAcpiTableIdSrat,                          ///< SRAT Generator
> > +  EStdAcpiTableIdMpam,                          ///< MPAM Generator
> >    EStdAcpiTableIdSsdtSerialPort,                ///< SSDT Serial-Port Generator
> >    EStdAcpiTableIdSsdtCmn600,                    ///< SSDT Cmn-600 Generator
> >    EStdAcpiTableIdSsdtCpuTopology,               ///< SSDT Cpu Topology
> > diff --git a/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
> > b/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
> > index 102e0f96be..39a14ed0b3 100644
> > --- a/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
> > +++ b/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
> > @@ -63,6 +63,8 @@ typedef enum ArmObjectID {
> >    EArmObjPciInterruptMapInfo,          ///< 39 - Pci Interrupt Map Info
> >    EArmObjRmr,                          ///< 40 - Reserved Memory Range Node
> >    EArmObjMemoryRangeDescriptor,        ///< 41 - Memory Range
> Descriptor
> > +  EArmObjMscNodeInfo,                  ///< 40 - Msc Memory System Controller
> > Node Info
> > +  EArmObjResNodeInfo,                  ///< 41 - Res Resource Node Info
> >    EArmObjMax
> >  } EARM_OBJECT_ID;
> >
> > @@ -1070,6 +1072,72 @@ typedef struct CmArmRmrDescriptor {
> >    UINT64    Length;
> >  } CM_ARM_MEMORY_RANGE_DESCRIPTOR;
> >
> > +/** A structure that describes Memory System Controller Node.
> > +
> > +    MPAM Memory System Component Nodes are described by
> > +    this object.
> > +
> > +  ID: EArmObjMscNodeInfo
> > +*/
> > +typedef struct CmArmMscNodeInfo {
> > +  /// An unique token used to identify this object
> > +  CM_OBJECT_TOKEN      Token;
> > +
> > +  /// MPAM Base Address
> > +  UINT64               BaseAddress;
> > +  /// MMIO Size
> > +  UINT32               MmioSize;
> > +  /// Overflow Interrupt
> > +  UINT32               OverflowInterrupt;
> > +  /// Overflow Interrupt Flags
> > +  UINT32               OverflowInterruptFlags;
> 
> [Rohit] I have added a comment on Flag's type in [PATCH 1/2]. Same
> comment here.

[Rohit] Please ignore this.

> 
> > +  /// Overflow Interrupt Affinity
> > +  UINT32               OverflowInterruptAff;
> > +  /// Error Interrupt
> > +  UINT32               ErrorInterrupt;
> > +  /// Error Interrupt Flags
> > +  UINT32               ErrorInterruptFlags;
> > +  /// Error Interrupt Affinity
> > +  UINT32               ErrorInterruptAff;
> > +  /// Not Ready Signal time
> > +  UINT32               MaxNRdyUsec;
> > +  /// Linked Device HWID
> > +  UINT64               LinkedDeviceHwId;
> > +  /// Linked Device Instance ID
> > +  UINT32               LinkedDeviceInstanceHwId;
> > +  /// Number of Resource nodes
> > +  UINT32               NumResourceNodes;
> > +  /// Reference token for the list of resource nodes
> > +  //CM_OBJECT_TOKEN    ResourceNodeListToken;
> > +
> > +} CM_ARM_MSC_NODE_INFO;
> > +
> > +/** A structure that describes Memory System Controller Node.
> > +
> > +    MPAM Memory System Component Nodes are described by
> > +    this object.
> > +
> > +  ID: EArmObjResNodeInfo
> > +*/
> > +typedef struct CmArmResNodeInfo {
> > +  /// An unique token used to identify this object
> > +  CM_OBJECT_TOKEN    Token;
> > +
> > +  /// Identifier
> > +  UINT32             Identifier;
> > +  /// RIS Index
> > +  UINT8              RisIndex;
> > +  /// Locator Type
> > +  UINT8              LocatorType;
> > +  /// Locator
> > +  UINT64             Locator;
> 
> [Rohit] I have added a comment on Locator's type and size in [PATCH 1/2].
> Same comment here.
> 
> > +  /// Num functional dependencies
> > +  UINT32             NumFuncDep;
> > +  /// Reference token for the list of resource nodes
> > +  CM_OBJECT_TOKEN    FuncDepListToken;
> > +
> > +} CM_ARM_RESOURCE_NODE_INFO;
> > +
> >  #pragma pack()
> >
> >  #endif // ARM_NAMESPACE_OBJECTS_H_
> > diff --git
> >
> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/AcpiMpamLibArm
> > .inf
> >
> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/AcpiMpamLibArm
> > .inf
> > new file mode 100644
> > index 0000000000..480130dc21
> > --- /dev/null
> > +++
> >
> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/AcpiMpamLibArm
> > .in
> > +++ f
> > @@ -0,0 +1,30 @@
> > +## @file
> > +#  MPAM Table Generator Inf file
> > +#
> > +#  Copyright (c) 2022, NVIDIA CORPORATION. All rights reserved.
> > +#  Copyright (c) 2022, ARM Limited. All rights reserved.
> > +#  SPDX-License-Identifier: BSD-2-Clause-Patent ##
> > +
> > +[Defines]
> > +  INF_VERSION    = 0x0001001B
> > +  BASE_NAME      = AcpiMpamLibArm
> > +  FILE_GUID      = 02d0c79f-41cd-45c9-9835-781229c619d1
> > +  VERSION_STRING = 1.0
> > +  MODULE_TYPE    = DXE_DRIVER
> > +  LIBRARY_CLASS  = NULL|DXE_DRIVER
> > +  CONSTRUCTOR    = AcpiMpamLibConstructor
> > +  DESTRUCTOR     = AcpiMpamLibDestructor
> > +
> > +[Sources]
> > +  MpamGenerator.c
> > +  MpamGenerator.h
> > +
> > +[Packages]
> > +  EmbeddedPkg/EmbeddedPkg.dec
> > +  DynamicTablesPkg/DynamicTablesPkg.dec
> > +  MdeModulePkg/MdeModulePkg.dec
> > +  MdePkg/MdePkg.dec
> > +
> > +[LibraryClasses]
> > +  BaseLib
> > diff --git
> >
> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/MpamGenerator.
> > c
> >
> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/MpamGenerator.
> > c
> > new file mode 100644
> > index 0000000000..db3e8e95bc
> > --- /dev/null
> > +++
> >
> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/MpamGenerator.
> > c
> > @@ -0,0 +1,649 @@
> > +/** @file
> > +  MPAM Table Generator
> > +
> > +  Copyright (c) 2022, NVIDIA CORPORATION. All rights reserved.
> > +  Copyright (c) 2022, ARM Limited. All rights reserved.
> > +  SPDX-License-Identifier: BSD-2-Clause-Patent
> > +
> > +  @par Reference(s):
> > +  - ACPI 6.4 Specification, January 2021
> > +
> > +  @par Glossary:
> > +  - Cm or CM   - Configuration Manager
> > +  - Obj or OBJ - Object
> > +**/
> > +
> > +#include <IndustryStandard/Mpam.h>
> > +#include <Library/AcpiLib.h>
> > +#include <Library/BaseLib.h>
> > +#include <Library/DebugLib.h>
> > +#include <Library/MemoryAllocationLib.h> #include
> > +<Protocol/AcpiTable.h>
> > +
> > +// Module specific include files.
> > +#include <AcpiTableGenerator.h>
> > +#include <ConfigurationManagerObject.h> #include
> > +<ConfigurationManagerHelper.h> #include <Library/TableHelperLib.h>
> > +#include <Protocol/ConfigurationManagerProtocol.h>
> > +
> > +#include "MpamGenerator.h"
> > +
> > +/**
> > +  ARM standard MPAM Generator
> > +
> > +  Requirements:
> > +    The following Configuration Manager Object(s) are used by this
> > Generator:
> > +    - EArmObjMscNodeInfo (REQUIRED)
> > +    - EArmObjResNodeInfo
> > +*/
> > +
> > +/**
> > +  This macro expands to a function that retrieves the MSC Node
> > +information
> > +  from the Configuration Manager.
> > +*/
> > +GET_OBJECT_LIST (
> > +  EObjNameSpaceArm,
> > +  EArmObjMscNodeInfo,
> > +  CM_ARM_MSC_NODE_INFO
> > +  );
> > +
> > +/**
> > +  This macro expands to a function that retrieves the Resource Node
> > +  information from the Configuration Manager.
> > +*/
> > +GET_OBJECT_LIST (
> > +  EObjNameSpaceArm,
> > +  EArmObjResNodeInfo,
> > +  CM_ARM_RESOURCE_NODE_INFO
> > +  );
> > +
> > +/**
> > +  Returns the size of the MPAM Memory System Controller (MSC) Node
> > +
> > +  @param [in]  Node     Pointer to MSC Node Info CM object
> > +
> > +  @retval               Size of the MSC Node in bytes.
> > +**/
> > +STATIC
> > +UINT32
> > +GetMscNodeSize (
> > +  IN  CONST CM_ARM_MSC_NODE_INFO  *Node
> > +  )
> > +{
> > +  ASSERT (Node != NULL);
> > +
> > +  // <size of Memory System Controller Node>
> > +  return (UINT32)(sizeof (EFI_ACPI_6_4_MPAM_MSC_NODE) +
> > +                  Node->NumResourceNodes * sizeof
> > +(EFI_ACPI_6_4_MPAM_RESOURCE_NODE));
> 
> [Rohit] Shouldn't the node size include the size of functional dependency
> descriptor * number of functional dependency descriptor?
> 
> > +}
> > +
> > +/** Returns the total size required for the MSC and
> > +    updates the Node Indexer.
> > +
> > +    This function calculates the size required for the node group
> > +    and also populates the Node Indexer array with offsets for the
> > +    individual nodes.
> > +
> > +    @param [in]       NodeStartOffset Offset from the start of the
> > +                                      MPAM where this node group starts.
> 
> [Rohit] Should it be "start of the MPAM table where .."?
> 
> > +    @param [in]       NodeList        Pointer to MSC Group node list.
> > +    @param [in]       NodeCount       Count of the MSC Group nodes.
> > +    @param [in, out]  NodeIndexer     Pointer to the next Node Indexer.
> > +
> > +    @retval Total size of the ITS Group Nodes.
> > +**/
> > +STATIC
> > +UINT64
> > +GetSizeofMscGroupNodes (
> > +  IN      CONST UINT32                         NodeStartOffset,
> > +  IN      CONST CM_ARM_MSC_NODE_INFO           *NodeList,
> > +  IN            UINT32                         NodeCount,
> > +  IN OUT        MPAM_NODE_INDEXER     **CONST  NodeIndexer
> > +  )
> > +{
> > +  UINT64  Size;
> > +
> > +  ASSERT (NodeList != NULL);
> > +
> > +  Size = 0;
> > +  while (NodeCount-- != 0) {
> > +    (*NodeIndexer)->Token  = NodeList->Token;
> > +    (*NodeIndexer)->Object = (VOID *)NodeList;
> > +    (*NodeIndexer)->Offset = (UINT32)(Size + NodeStartOffset);
> > +    DEBUG ((
> > +      DEBUG_INFO,
> > +      "MPAM: MSC Node Indexer = %p, Token = %p, Object = %p, Offset =
> > 0x%x\n",
> > +      *NodeIndexer,
> > +      (*NodeIndexer)->Token,
> > +      (*NodeIndexer)->Object,
> > +      (*NodeIndexer)->Offset
> > +      ));
> > +
> > +    Size += GetMscNodeSize (NodeList);
> > +
> > +    (*NodeIndexer)++;
> > +    NodeList++;
> > +  }
> > +
> > +  return Size;
> > +}
> > +
> > +/** Update the MSC Group Node Information.
> > +
> > +    @param [in]     This             Pointer to the table Generator.
> > +    @param [in]     CfgMgrProtocol   Pointer to the Configuration Manager
> > +                                     Protocol Interface.
> > +    @param [in]     Mpam             Pointer to MPAM table structure.
> > +    @param [in]     NodesStartOffset Offset for the start of the Msc Group
> > +                                     Nodes.
> > +    @param [in]     NodeList         Pointer to an array of Msc Group Node
> > +                                     Objects.
> > +    @param [in]     NodeCount        Number of Msc Group Node Objects.
> > +
> > +    @retval EFI_SUCCESS           Table generated successfully.
> > +    @retval EFI_INVALID_PARAMETER A parameter is invalid.
> > +    @retval EFI_NOT_FOUND         The required object was not found.
> > +**/
> > +STATIC
> > +EFI_STATUS
> > +AddMscNodes (
> > +  IN      CONST ACPI_TABLE_GENERATOR
> *CONST
> > This,
> > +  IN      CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL
> > *CONST  CfgMgrProtocol,
> > +  IN      CONST
> >
> EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORIN
> > G_TABLE_HEADER           *Mpam,
> > +  IN      CONST UINT32
> > NodesStartOffset,
> > +  IN      CONST CM_ARM_MSC_NODE_INFO
> > *NodeList,
> > +  IN            UINT32                                                                             NodeCount
> > +  )
> > +{
> > +  EFI_STATUS                       Status;
> > +  EFI_ACPI_6_4_MPAM_MSC_NODE       *MscNode;
> > +  EFI_ACPI_6_4_MPAM_RESOURCE_NODE  *ResourceNodeArray;
> > +  CM_ARM_RESOURCE_NODE_INFO        *ResourceNodeList;
> > +  UINT64                           NodeLength;
> > +  UINT32                           ResourceNodeCount;
> > +
> > +  ASSERT (Mpam != NULL);
> > +
> > +  MscNode = (EFI_ACPI_6_4_MPAM_MSC_NODE *)((UINT8 *)Mpam +
> > + NodesStartOffset);
> > +
> > +  // Get the Resource Node info to update the MPAM MSC node  Status =
> > + GetEArmObjResNodeInfo (
> > +             CfgMgrProtocol,
> > +             CM_NULL_TOKEN,
> > +             &ResourceNodeList,
> > +             &ResourceNodeCount
> > +             );
> > +  if (EFI_ERROR (Status)) {
> > +    DEBUG ((
> > +      DEBUG_ERROR,
> > +      "ERROR: MPAM: Failed to add resource nodes info. Status = %r\n",
> > +      Status
> > +      ));
> > +    return Status;
> > +  }
> > +
> > +  while (NodeCount-- != 0) {
> > +    NodeLength = GetMscNodeSize (NodeList);
> > +
> > +    if (NodeLength > MAX_UINT16) {
> > +      Status = EFI_INVALID_PARAMETER;
> > +      DEBUG ((
> > +        DEBUG_ERROR,
> > +        "ERROR: MPAM: MSC Node length 0x%lx > MAX_UINT16. Status =
> > %r\n",
> > +        NodeLength,
> > +        Status
> > +        ));
> > +      return Status;
> > +    }
> > +
> > +    // Populate the node header
> > +    MscNode->Length                   = (UINT16)NodeLength;
> > +    MscNode->Reserved                 = EFI_ACPI_RESERVED_WORD;
> > +    MscNode->Identifier               = EFI_ACPI_RESERVED_DWORD;
> 
> [Rohit] MPAM ACPI 1.0, section 2.1, table 4 - MSC node body, defines
> identifier as a unique value. For systems with multiple MSCs, the OS could
> expect identifiers to be unique. Could this be addressed, please?
> 
> > +    MscNode->BaseAddress              = NodeList->BaseAddress;
> > +    MscNode->MmioSize                 = NodeList->MmioSize;
> > +    MscNode->OverflowInterrupt        = NodeList->OverflowInterrupt;
> > +    MscNode->OverflowInterruptFlags   = NodeList-
> >OverflowInterruptFlags;
> > +    MscNode->Reserved1                = EFI_ACPI_RESERVED_DWORD;
> > +    MscNode->OverflowInterruptAff     = NodeList->OverflowInterruptAff;
> > +    MscNode->ErrorInterrupt           = NodeList->ErrorInterrupt;
> > +    MscNode->ErrorInterruptFlags      = NodeList->ErrorInterruptFlags;
> > +    MscNode->Reserved2                = EFI_ACPI_RESERVED_DWORD;
> > +    MscNode->ErrorInterruptAff        = NodeList->ErrorInterruptAff;
> > +    MscNode->MaxNRdyUsec              = NodeList->MaxNRdyUsec;
> > +    MscNode->LinkedDeviceHwId         = NodeList->LinkedDeviceHwId;
> > +    MscNode->LinkedDeviceInstanceHwId = NodeList-
> > >LinkedDeviceInstanceHwId;
> > +    MscNode->NumResourceNodes         = NodeList->NumResourceNodes;
> > +
> > +    // ResourceNode List for each MSC
> > +    if (MscNode->NumResourceNodes > 0) {
> > +      // Resource Node array for this Msc node
> > +      ResourceNodeArray = (EFI_ACPI_6_4_MPAM_RESOURCE_NODE
> > *)((UINT8 *)MscNode + sizeof (EFI_ACPI_6_4_MPAM_MSC_NODE));
> > +      // Adding Resource Node content
> > +      while ( MscNode->NumResourceNodes-- != 0 ) {
> > +        ResourceNodeArray->Identifier  = ResourceNodeList->Identifier;
> > +        ResourceNodeArray->RisIndex    = ResourceNodeList->RisIndex;
> > +        ResourceNodeArray->Reserved1   = EFI_ACPI_RESERVED_WORD;
> > +        ResourceNodeArray->LocatorType = ResourceNodeList-
> >LocatorType;
> > +        ResourceNodeArray->Locator     = ResourceNodeList->Locator;
> > +        ResourceNodeArray->NumFuncDep  = ResourceNodeList-
> > >NumFuncDep;
> 
> [Rohit] If NumFuncDep > 0, shouldn't we need an additional loop to populate
> functional dependency list?
> 
> > +
> > +        if (EFI_ERROR (Status)) {
> > +          DEBUG ((
> > +            DEBUG_ERROR,
> > +            "ERROR: MPAM: Failed to add Resource Node List. Status = %r\n",
> > +            Status
> > +            ));
> > +          return Status;
> > +        }
> > +
> > +        ResourceNodeList++;
> > +        ResourceNodeArray++;
> > +      }
> > +    }
> > +
> > +    // Next MSC Node
> > +    MscNode = (EFI_ACPI_6_4_MPAM_MSC_NODE *)((UINT8 *)MscNode
> +
> > MscNode->Length);
> > +    NodeList++;
> > +  } // Msc Node
> > +
> > +  return EFI_SUCCESS;
> > +}
> > +
> > +/**
> > +  Construct the MPAM ACPI table.
> > +
> > +  This function invokes the Configuration Manager protocol interface
> > + to get the required hardware information for generating the ACPI
> > + table.
> > +
> > +  If this function allocates any resources then they must be freed
> > + in the FreeXXXXTableResources function.
> > +
> > +  @param [in]  This                 Pointer to the table generator.
> > +  @param [in]  AcpiTableInfo        Pointer to the ACPI table generator to be
> > used.
> > +  @param [in]  CfgMgrProtocol       Pointer to the Configuration Manager
> > +                                    Protocol Interface.
> > +  @param [out] Table                Pointer to the constructed ACPI Table.
> > +
> > +  @retval EFI_SUCCESS               Table generated successfully.
> > +  @retval EFI_INVALID_PARAMETER     A parameter is invalid.
> > +  @retval EFI_NOT_FOUND             The required object was not found.
> > +  @retval EFI_BAD_BUFFER_SIZE       The size returned by the
> Configuration
> > +                                    Manager is less than the Object size for
> > +                                    the requested object.
> > +**/
> > +STATIC
> > +EFI_STATUS
> > +EFIAPI
> > +BuildMpamTable (
> > +  IN  CONST ACPI_TABLE_GENERATOR                  *CONST  This,
> > +  IN  CONST CM_STD_OBJ_ACPI_TABLE_INFO            *CONST
> AcpiTableInfo,
> > +  IN  CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  *CONST
> > CfgMgrProtocol,
> > +  OUT       EFI_ACPI_DESCRIPTION_HEADER          **CONST  Table
> > +  )
> > +{
> > +  EFI_STATUS  Status;
> > +  UINT64      TableSize;
> > +  UINT64      NodeSize;
> > +  UINT32      MpamNodeCount;
> > +
> > +  UINT32  MscNodeCount;
> > +  UINT32  MscNodeOffset;
> > +
> > +
> >
> EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORIN
> > G_TABLE_HEADER  *Mpam;
> > +  ACPI_MPAM_GENERATOR                                                       *Generator;
> > +  CM_ARM_MSC_NODE_INFO                                                      *MscNodeList;
> > +  MPAM_NODE_INDEXER                                                         *NodeIndexer;
> > +
> > +  ASSERT (
> > +    (This != NULL) &&
> > +    (AcpiTableInfo != NULL) &&
> > +    (CfgMgrProtocol != NULL) &&
> > +    (Table != NULL) &&
> > +    (AcpiTableInfo->TableGeneratorId == This->GeneratorID) &&
> > +    (AcpiTableInfo->AcpiTableSignature == This->AcpiTableSignature)
> > +    );
> > +
> > +  DEBUG ((
> > +    DEBUG_ERROR,
> > +    "DEBUG PRINT: MPAM: Requested table revision = %d\n",
> > +    AcpiTableInfo->AcpiTableRevision
> > +    ));
> > +
> > +  if ((AcpiTableInfo->AcpiTableRevision < This->MinAcpiTableRevision) ||
> > +      (AcpiTableInfo->AcpiTableRevision > This->AcpiTableRevision))  {
> > +    DEBUG ((
> > +      DEBUG_ERROR,
> > +      "ERROR: MPAM: Requested table revision = %d is not supported. "
> > +      "Supported table revisions: Minimum = %d. Maximum = %d\n",
> > +      AcpiTableInfo->AcpiTableRevision,
> > +      This->MinAcpiTableRevision,
> > +      This->AcpiTableRevision
> > +      ));
> > +    return EFI_INVALID_PARAMETER;
> > +  }
> > +
> > +  Generator = (ACPI_MPAM_GENERATOR *)This;
> > +  *Table    = NULL;
> > +
> > +  // Get the Memory System Controller Node info and update the MPAM
> > + // structure count with MSC Node count (Type 0)  Status =
> > + GetEArmObjMscNodeInfo (
> > +             CfgMgrProtocol,
> > +             CM_NULL_TOKEN,
> > +             &MscNodeList,
> > +             &MscNodeCount
> > +             );
> > +  if (EFI_ERROR (Status)) {
> > +    DEBUG ((
> > +      DEBUG_ERROR,
> > +      "ERROR: MPAM: Failed to get memory system controller node info.
> > Status = %r\n",
> > +      Status
> > +      ));
> > +    goto error_handler;
> > +  }
> > +
> > +  MpamNodeCount           = MscNodeCount;
> > +  Generator->MscNodeCount = MscNodeCount;
> > +
> > +  // Allocate Node Indexer array
> > +  NodeIndexer = (MPAM_NODE_INDEXER *)AllocateZeroPool (
> > +                                       sizeof (MPAM_NODE_INDEXER) *
> > +                                       MpamNodeCount
> > +                                       );  if (NodeIndexer == NULL) {
> > +    Status = EFI_OUT_OF_RESOURCES;
> > +    DEBUG ((
> > +      DEBUG_ERROR,
> > +      "ERROR: MPAM: Failed to allocate memory for Node Indexer.
> > + Status =
> > %r\n ",
> > +      Status
> > +      ));
> > +    goto error_handler;
> > +  }
> > +
> > +  DEBUG ((DEBUG_INFO, "MPAM INFO: NodeIndexer = %p\n",
> > NodeIndexer));
> > + Generator->MpamNodeCount = MpamNodeCount;
> > +  Generator->NodeIndexer   = NodeIndexer;
> > +
> > +  // Calculate the size of the MPAM table  TableSize = sizeof
> > +
> >
> (EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORIN
> > G_TABLE_HEA
> > + DER);
> > +
> > +  // Include the size of MSC Nodes and index them  if
> > + (Generator->MscNodeCount != 0) {
> > +    MscNodeOffset = TableSize;
> > +    // Size of MSC nodes.
> > +    NodeSize = GetSizeofMscGroupNodes (
> > +                 MscNodeOffset,
> > +                 MscNodeList,
> > +                 Generator->MscNodeCount,
> > +                 &NodeIndexer
> > +                 );
> 
> [Rohit] MscNodeOffset is sizeof
> (EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORIN
> G_TABLE_HEADER), which moves past EFI_ACPI_DESCRIPTION_HEADER by
> 12 bytes. Should we be having these fields in the table type?
>   UINT32                         NumNodes;
>   UINT32                         NodeOffset;
>   UINT32                         Reserved;
> 
> I had added a similar comment on [PATCH 1/2].
> [/Rohit]
> 
> > +    if (NodeSize > MAX_UINT32) {
> > +      Status = EFI_INVALID_PARAMETER;
> > +      DEBUG ((
> > +        DEBUG_ERROR,
> > +        "ERROR: MPAM: Invalid Size of Group Nodes. Status = %r\n",
> > +        Status
> > +        ));
> > +      goto error_handler;
> > +    }
> > +
> > +    TableSize += NodeSize;
> > +
> > +    DEBUG ((
> > +      DEBUG_INFO,
> > +      " MscNodeCount = %d\n" \
> > +      " MscNodeOffset = 0x%x\n",
> > +      Generator->MscNodeCount,
> > +      MscNodeOffset
> > +      ));
> > +  }
> > +
> > +  DEBUG ((
> > +    DEBUG_INFO,
> > +    "INFO: MPAM:\n" \
> > +    " MpamNodeCount = %d\n" \
> > +    " TableSize = 0x%X\n",
> > +    MpamNodeCount,
> > +    TableSize
> > +    ));
> > +
> > +  if (TableSize > MAX_UINT32) {
> > +    Status = EFI_INVALID_PARAMETER;
> > +    DEBUG ((
> > +      DEBUG_ERROR,
> > +      "ERROR: MPAM: MPAM Table Size 0x%lx > MAX_UINT32," \
> > +      " Status = %r\n",
> > +      TableSize,
> > +      Status
> > +      ));
> > +    goto error_handler;
> > +  }
> > +
> > +  // Allocate the Buffer for the MPAM table  *Table =
> > + (EFI_ACPI_DESCRIPTION_HEADER *)AllocateZeroPool (TableSize);  if
> > + (*Table == NULL) {
> > +    Status = EFI_OUT_OF_RESOURCES;
> > +    DEBUG ((
> > +      DEBUG_ERROR,
> > +      "ERROR: MPAM: Failed to allocate memory for MPAM Table. " \
> > +      "Size = %d. Status = %r\n",
> > +      TableSize,
> > +      Status
> > +      ));
> > +    goto error_handler;
> > +  }
> > +
> > +  Mpam =
> > +
> >
> (EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORIN
> > G_TABLE_HEA
> > + DER *)*Table;
> > +
> > +  DEBUG ((
> > +    DEBUG_INFO,
> > +    "MPAM: Mpam = 0x%p. TableSize = 0x%x\n",
> > +    Mpam,
> > +    TableSize
> > +    ));
> > +
> > +  // Add ACPI header
> > +  Status = AddAcpiHeader (
> > +             CfgMgrProtocol,
> > +             This,
> > +             &Mpam->Header,
> > +             AcpiTableInfo,
> > +             TableSize
> > +             );
> > +  if (EFI_ERROR (Status)) {
> > +    DEBUG ((
> > +      DEBUG_ERROR,
> > +      "ERROR: MPAM: Failed to add ACPI header. Status = %r\n",
> > +      Status
> > +      ));
> > +    goto error_handler;
> > +  }
> > +
> > +  // Update MPAM table
> > +  Mpam->NumNodes   = MscNodeCount;
> > +  Mpam->NodeOffset = sizeof
> >
> (EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORIN
> > G_TABLE_HEADER);
> > +  Mpam->Reserved   = EFI_ACPI_RESERVED_DWORD;
> > +
> > +  // Add MSC Nodes to the generated table  if (Mpam->NumNodes != 0) {
> > +    Status = AddMscNodes (
> > +               This,
> > +               CfgMgrProtocol,
> > +               Mpam,
> > +               MscNodeOffset,
> > +               MscNodeList,
> > +               MscNodeCount
> > +               );
> 
> [Rohit] Same comment as above
> 
> > +    if (EFI_ERROR (Status)) {
> > +      DEBUG ((
> > +        DEBUG_ERROR,
> > +        "ERROR: MPAM: Failed to add MSC Nodes. Status = %r\n",
> > +        Status
> > +        ));
> > +      goto error_handler;
> > +    }
> > +  }
> > +
> > +  return Status;
> > +
> > +error_handler:
> > +  if (Generator->NodeIndexer != NULL) {
> > +    FreePool (Generator->NodeIndexer);
> > +    Generator->NodeIndexer = NULL;
> > +  }
> > +
> > +  if (*Table != NULL) {
> > +    FreePool (*Table);
> > +    *Table = NULL;
> > +  }
> > +
> > +  return Status;
> > +}
> > +
> > +/** Free any resources allocated for constructing the MPAM
> 
> [Rohit] Should this be "constructing the MPAM table"?
> 
> > +
> > +  @param [in]      This           Pointer to the table generator.
> > +  @param [in]      AcpiTableInfo  Pointer to the ACPI Table Info.
> > +  @param [in]      CfgMgrProtocol Pointer to the Configuration Manager
> > +                                  Protocol Interface.
> > +  @param [in, out] Table          Pointer to the ACPI Table.
> > +
> > +  @retval EFI_SUCCESS           The resources were freed successfully.
> > +  @retval EFI_INVALID_PARAMETER The table pointer is NULL or invalid.
> > +**/
> > +STATIC
> > +EFI_STATUS
> > +FreeMpamTableResources (
> > +  IN      CONST ACPI_TABLE_GENERATOR                  *CONST  This,
> > +  IN      CONST CM_STD_OBJ_ACPI_TABLE_INFO            *CONST
> > AcpiTableInfo,
> > +  IN      CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  *CONST
> > CfgMgrProtocol,
> > +  IN OUT        EFI_ACPI_DESCRIPTION_HEADER          **CONST  Table
> > +  )
> > +{
> > +  ACPI_MPAM_GENERATOR  *Generator;
> > +
> > +  ASSERT (This != NULL);
> > +  ASSERT (AcpiTableInfo != NULL);
> > +  ASSERT (CfgMgrProtocol != NULL);
> > +  ASSERT (AcpiTableInfo->TableGeneratorId == This->GeneratorID);
> > + ASSERT (AcpiTableInfo->AcpiTableSignature ==
> > + This->AcpiTableSignature);
> > +
> > +  Generator = (ACPI_MPAM_GENERATOR *)This;
> > +
> > +  // Free any memory allocated by the generator  if
> > + (Generator->NodeIndexer != NULL) {
> > +    FreePool (Generator->NodeIndexer);
> > +    Generator->NodeIndexer = NULL;
> > +  }
> > +
> > +  if ((Table == NULL) || (*Table == NULL)) {
> > +    DEBUG ((DEBUG_ERROR, "ERROR: MPAM: Invalid Table Pointer\n"));
> > +    return EFI_INVALID_PARAMETER;
> > +  }
> > +
> > +  FreePool (*Table);
> > +  *Table = NULL;
> > +
> > +  return EFI_SUCCESS;
> > +}
> > +
> > +/** The MPAM Table Generator revision.
> > +*/
> > +#define MPAM_GENERATOR_REVISION  CREATE_REVISION (1, 0)
> > +
> > +/** The interface for the MPAM Table Generator.
> > +*/
> > +STATIC
> > +ACPI_MPAM_GENERATOR  MpamGenerator = {
> > +  // ACPI table generator header
> > +  {
> > +    // Generator ID
> > +    CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdMpam),
> > +    // Generator Description
> > +    L"ACPI.STD.MPAM.GENERATOR",
> > +    // ACPI Table Signature
> > +
> >
> EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORIN
> > G_TABLE_STRUCTURE_SIGNATURE,
> > +    // ACPI Table Revision supported by this Generator
> > +
> >
> EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORIN
> > G_TABLE_REVISION,
> > +    // Minimum supported ACPI Table Revision
> > +
> >
> EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORIN
> > G_TABLE_REVISION,
> > +    // Creator ID
> > +    TABLE_GENERATOR_CREATOR_ID_ARM,
> > +    // Creator Revision
> > +    MPAM_GENERATOR_REVISION,
> > +    // Build Table function
> > +    BuildMpamTable,
> > +    // Free Resource function
> > +    FreeMpamTableResources,
> > +    // Extended build function not needed
> > +    NULL,
> > +    // Extended build function not implemented by the generator.
> > +    // Hence extended free resource function is not required.
> > +    NULL
> > +  },
> > +
> > +  // MPAM Generator private data
> > +
> > +  // MPAM node count
> > +  0,
> > +  // MSC node count
> > +  0,
> > +
> > +  // Pointer to MPAM Node Indexer
> > +  NULL
> > +};
> > +
> > +/**
> > +  Register the Generator with the ACPI Table Factory.
> > +
> > +  @param [in]  ImageHandle        The handle to the image.
> > +  @param [in]  SystemTable        Pointer to the System Table.
> > +
> > +  @retval EFI_SUCCESS             The Generator is registered.
> > +  @retval EFI_INVALID_PARAMETER   A parameter is invalid.
> > +  @retval EFI_ALREADY_STARTED     The Generator for the Table ID
> > +                                  is already registered.
> > +**/
> > +EFI_STATUS
> > +EFIAPI
> > +AcpiMpamLibConstructor (
> > +  IN  EFI_HANDLE        ImageHandle,
> > +  IN  EFI_SYSTEM_TABLE  *SystemTable
> > +  )
> > +{
> > +  EFI_STATUS  Status;
> > +
> > +  Status = RegisterAcpiTableGenerator (&MpamGenerator.Header);
> > +  DEBUG ((DEBUG_INFO, "MPAM: Register Generator. Status = %r\n",
> > +Status));
> > +  ASSERT_EFI_ERROR (Status);
> > +  return Status;
> > +}
> > +
> > +/**
> > +  Deregister the Generator from the ACPI Table Factory.
> > +
> > +  @param [in]  ImageHandle        The handle to the image.
> > +  @param [in]  SystemTable        Pointer to the System Table.
> > +
> > +  @retval EFI_SUCCESS             The Generator is deregistered.
> > +  @retval EFI_INVALID_PARAMETER   A parameter is invalid.
> > +  @retval EFI_NOT_FOUND           The Generator is not registered.
> > +**/
> > +EFI_STATUS
> > +EFIAPI
> > +AcpiMpamLibDestructor (
> > +  IN  EFI_HANDLE        ImageHandle,
> > +  IN  EFI_SYSTEM_TABLE  *SystemTable
> > +  )
> > +{
> > +  EFI_STATUS  Status;
> > +
> > +  Status = DeregisterAcpiTableGenerator (&MpamGenerator.Header);
> > +  DEBUG ((DEBUG_INFO, "MPAM: Deregister Generator. Status = %r\n",
> > +Status));
> > +  ASSERT_EFI_ERROR (Status);
> > +  return Status;
> > +}
> > diff --git
> >
> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/MpamGenerator.
> > h
> >
> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/MpamGenerator.
> > h
> > new file mode 100644
> > index 0000000000..1075bd8c6c
> > --- /dev/null
> > +++
> >
> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/MpamGenerator.
> > h
> > @@ -0,0 +1,47 @@
> > +/** @file
> > +  Header file for the dynamic MPAM generator
> > +
> > +  Copyright (c) 2022, NVIDIA CORPORATION. All rights reserved.
> > +  Copyright (c) 2022, ARM Limited. All rights reserved.
> > +  SPDX-License-Identifier: BSD-2-Clause-Patent
> > +
> > +  @par Reference(s):
> > +  - ACPI 6.4 Specification, January 2021
> > +  - ARM Architecture Reference Manual ARMv8
> > +
> > +  @par Glossary:
> > +    - Cm or CM   - Configuration Manager
> > +    - Obj or OBJ - Object
> > +**/
> > +
> > +#ifndef MPAM_GENERATOR_H_
> > +#define MPAM_GENERATOR_H_
> > +
> > +#pragma pack(1)
> > +
> > +/** A structure that describes the Node indexer
> > +    used for indexing the MPAM MSC nodes.
> > +*/
> > +typedef struct MpamNodeIndexer {
> > +  /// Index token for the Node
> > +  CM_OBJECT_TOKEN    Token;
> > +  /// Pointer to the node
> > +  VOID               *Object;
> > +  /// Node offset from the start of the MPAM table
> > +  UINT32             Offset;
> > +} MPAM_NODE_INDEXER;
> > +
> > +typedef struct AcpiMpamGenerator {
> > +  /// ACPI Table generator header
> > +  ACPI_TABLE_GENERATOR    Header;
> > +  /// MPAM structure count
> > +  UINT32                  MpamNodeCount;
> > +  /// Count of Msc Nodes
> > +  UINT32                  MscNodeCount;
> > +  /// List of indexed CM objects for MPAM generation
> > +  MPAM_NODE_INDEXER       *NodeIndexer;
> > +} ACPI_MPAM_GENERATOR;
> > +
> > +#pragma pack()
> > +
> > +#endif // MPAM_GENERATOR_H_
> > --
> > 2.17.1
> >
> >
> >
> > 
> >
> 
> Regards,
> Rohit

Re: [PATCH 2/2] Dynamic Tbl Mgr: MPAM: MPAM Generator and supporting files

[Swatisri Kantamsetti]

[-- Attachment #1: Type: text/plain, Size: 30843 bytes --]

Hello,
Just a reminder to provide feedback on this patch.

Thanks,
Swati

swatisrik@nvidia.com
________________________________
From: Name <username@nvidia.com>
Sent: Tuesday, August 16, 2022 2:18 PM
To: devel@edk2.groups.io <devel@edk2.groups.io>; Sami.Mujawar@arm.com <Sami.Mujawar@arm.com>; Alexei.Fedorov@arm.com <Alexei.Fedorov@arm.com>; michael.d.kinney@intel.com <michael.d.kinney@intel.com>; gaoliming@byosoft.com.cn <gaoliming@byosoft.com.cn>; zhiguang.liu@intel.com <zhiguang.liu@intel.com>
Cc: Swatisri Kantamsetti <swatisrik@nvidia.com>
Subject: [PATCH 2/2] Dynamic Tbl Mgr: MPAM: MPAM Generator and supporting files

From: Swatisri Kantamsetti <swatisrik@nvidia.com>

ACPI header, MSC and Resource Nodes are populated
in the MPAM Table

Signed-off-by: Swatisri Kantamsetti <swatisrik@nvidia.com>
---
 DynamicTablesPkg/DynamicTables.dsc.inc        |   2 +
 DynamicTablesPkg/Include/AcpiTableGenerator.h |   1 +
 .../Include/ArmNameSpaceObjects.h             |  68 ++
 .../Arm/AcpiMpamLibArm/AcpiMpamLibArm.inf     |  30 +
 .../Acpi/Arm/AcpiMpamLibArm/MpamGenerator.c   | 649 ++++++++++++++++++
 .../Acpi/Arm/AcpiMpamLibArm/MpamGenerator.h   |  47 ++
 6 files changed, 797 insertions(+)
 create mode 100644 DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/AcpiMpamLibArm.inf
 create mode 100644 DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/MpamGenerator.c
 create mode 100644 DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/MpamGenerator.h

diff --git a/DynamicTablesPkg/DynamicTables.dsc.inc b/DynamicTablesPkg/DynamicTables.dsc.inc
index 3d4fa0c4c4..745d5f0633 100644
--- a/DynamicTablesPkg/DynamicTables.dsc.inc
+++ b/DynamicTablesPkg/DynamicTables.dsc.inc
@@ -29,6 +29,7 @@
   DynamicTablesPkg/Library/Acpi/Arm/AcpiIortLibArm/AcpiIortLibArm.inf
   DynamicTablesPkg/Library/Acpi/Arm/AcpiMadtLibArm/AcpiMadtLibArm.inf
   DynamicTablesPkg/Library/Acpi/Arm/AcpiMcfgLibArm/AcpiMcfgLibArm.inf
+  DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/AcpiMpamLibArm.inf
   DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/AcpiPpttLibArm.inf
   DynamicTablesPkg/Library/Acpi/Arm/AcpiRawLibArm/AcpiRawLibArm.inf
   DynamicTablesPkg/Library/Acpi/Arm/AcpiSpcrLibArm/AcpiSpcrLibArm.inf
@@ -54,6 +55,7 @@
       NULL|DynamicTablesPkg/Library/Acpi/Arm/AcpiMadtLibArm/AcpiMadtLibArm.inf
       NULL|DynamicTablesPkg/Library/Acpi/Arm/AcpiMcfgLibArm/AcpiMcfgLibArm.inf
       NULL|DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/AcpiPpttLibArm.inf
+      NULL|DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/AcpiMpamLibArm.inf
       NULL|DynamicTablesPkg/Library/Acpi/Arm/AcpiRawLibArm/AcpiRawLibArm.inf
       NULL|DynamicTablesPkg/Library/Acpi/Arm/AcpiSpcrLibArm/AcpiSpcrLibArm.inf
       NULL|DynamicTablesPkg/Library/Acpi/Arm/AcpiSratLibArm/AcpiSratLibArm.inf
diff --git a/DynamicTablesPkg/Include/AcpiTableGenerator.h b/DynamicTablesPkg/Include/AcpiTableGenerator.h
index f962dbff57..56d7375b4a 100644
--- a/DynamicTablesPkg/Include/AcpiTableGenerator.h
+++ b/DynamicTablesPkg/Include/AcpiTableGenerator.h
@@ -94,6 +94,7 @@ typedef enum StdAcpiTableId {
   EStdAcpiTableIdIort,                          ///< IORT Generator
   EStdAcpiTableIdPptt,                          ///< PPTT Generator
   EStdAcpiTableIdSrat,                          ///< SRAT Generator
+  EStdAcpiTableIdMpam,                          ///< MPAM Generator
   EStdAcpiTableIdSsdtSerialPort,                ///< SSDT Serial-Port Generator
   EStdAcpiTableIdSsdtCmn600,                    ///< SSDT Cmn-600 Generator
   EStdAcpiTableIdSsdtCpuTopology,               ///< SSDT Cpu Topology
diff --git a/DynamicTablesPkg/Include/ArmNameSpaceObjects.h b/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
index 102e0f96be..39a14ed0b3 100644
--- a/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
+++ b/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
@@ -63,6 +63,8 @@ typedef enum ArmObjectID {
   EArmObjPciInterruptMapInfo,          ///< 39 - Pci Interrupt Map Info
   EArmObjRmr,                          ///< 40 - Reserved Memory Range Node
   EArmObjMemoryRangeDescriptor,        ///< 41 - Memory Range Descriptor
+  EArmObjMscNodeInfo,                  ///< 40 - Msc Memory System Controller Node Info
+  EArmObjResNodeInfo,                  ///< 41 - Res Resource Node Info
   EArmObjMax
 } EARM_OBJECT_ID;

@@ -1070,6 +1072,72 @@ typedef struct CmArmRmrDescriptor {
   UINT64    Length;
 } CM_ARM_MEMORY_RANGE_DESCRIPTOR;

+/** A structure that describes Memory System Controller Node.
+
+    MPAM Memory System Component Nodes are described by
+    this object.
+
+  ID: EArmObjMscNodeInfo
+*/
+typedef struct CmArmMscNodeInfo {
+  /// An unique token used to identify this object
+  CM_OBJECT_TOKEN      Token;
+
+  /// MPAM Base Address
+  UINT64               BaseAddress;
+  /// MMIO Size
+  UINT32               MmioSize;
+  /// Overflow Interrupt
+  UINT32               OverflowInterrupt;
+  /// Overflow Interrupt Flags
+  UINT32               OverflowInterruptFlags;
+  /// Overflow Interrupt Affinity
+  UINT32               OverflowInterruptAff;
+  /// Error Interrupt
+  UINT32               ErrorInterrupt;
+  /// Error Interrupt Flags
+  UINT32               ErrorInterruptFlags;
+  /// Error Interrupt Affinity
+  UINT32               ErrorInterruptAff;
+  /// Not Ready Signal time
+  UINT32               MaxNRdyUsec;
+  /// Linked Device HWID
+  UINT64               LinkedDeviceHwId;
+  /// Linked Device Instance ID
+  UINT32               LinkedDeviceInstanceHwId;
+  /// Number of Resource nodes
+  UINT32               NumResourceNodes;
+  /// Reference token for the list of resource nodes
+  //CM_OBJECT_TOKEN    ResourceNodeListToken;
+
+} CM_ARM_MSC_NODE_INFO;
+
+/** A structure that describes Memory System Controller Node.
+
+    MPAM Memory System Component Nodes are described by
+    this object.
+
+  ID: EArmObjResNodeInfo
+*/
+typedef struct CmArmResNodeInfo {
+  /// An unique token used to identify this object
+  CM_OBJECT_TOKEN    Token;
+
+  /// Identifier
+  UINT32             Identifier;
+  /// RIS Index
+  UINT8              RisIndex;
+  /// Locator Type
+  UINT8              LocatorType;
+  /// Locator
+  UINT64             Locator;
+  /// Num functional dependencies
+  UINT32             NumFuncDep;
+  /// Reference token for the list of resource nodes
+  CM_OBJECT_TOKEN    FuncDepListToken;
+
+} CM_ARM_RESOURCE_NODE_INFO;
+
 #pragma pack()

 #endif // ARM_NAMESPACE_OBJECTS_H_
diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/AcpiMpamLibArm.inf b/DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/AcpiMpamLibArm.inf
new file mode 100644
index 0000000000..480130dc21
--- /dev/null
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/AcpiMpamLibArm.inf
@@ -0,0 +1,30 @@
+## @file
+#  MPAM Table Generator Inf file
+#
+#  Copyright (c) 2022, NVIDIA CORPORATION. All rights reserved.
+#  Copyright (c) 2022, ARM Limited. All rights reserved.
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+##
+
+[Defines]
+  INF_VERSION    = 0x0001001B
+  BASE_NAME      = AcpiMpamLibArm
+  FILE_GUID      = 02d0c79f-41cd-45c9-9835-781229c619d1
+  VERSION_STRING = 1.0
+  MODULE_TYPE    = DXE_DRIVER
+  LIBRARY_CLASS  = NULL|DXE_DRIVER
+  CONSTRUCTOR    = AcpiMpamLibConstructor
+  DESTRUCTOR     = AcpiMpamLibDestructor
+
+[Sources]
+  MpamGenerator.c
+  MpamGenerator.h
+
+[Packages]
+  EmbeddedPkg/EmbeddedPkg.dec
+  DynamicTablesPkg/DynamicTablesPkg.dec
+  MdeModulePkg/MdeModulePkg.dec
+  MdePkg/MdePkg.dec
+
+[LibraryClasses]
+  BaseLib
diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/MpamGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/MpamGenerator.c
new file mode 100644
index 0000000000..db3e8e95bc
--- /dev/null
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/MpamGenerator.c
@@ -0,0 +1,649 @@
+/** @file
+  MPAM Table Generator
+
+  Copyright (c) 2022, NVIDIA CORPORATION. All rights reserved.
+  Copyright (c) 2022, ARM Limited. All rights reserved.
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+  @par Reference(s):
+  - ACPI 6.4 Specification, January 2021
+
+  @par Glossary:
+  - Cm or CM   - Configuration Manager
+  - Obj or OBJ - Object
+**/
+
+#include <IndustryStandard/Mpam.h>
+#include <Library/AcpiLib.h>
+#include <Library/BaseLib.h>
+#include <Library/DebugLib.h>
+#include <Library/MemoryAllocationLib.h>
+#include <Protocol/AcpiTable.h>
+
+// Module specific include files.
+#include <AcpiTableGenerator.h>
+#include <ConfigurationManagerObject.h>
+#include <ConfigurationManagerHelper.h>
+#include <Library/TableHelperLib.h>
+#include <Protocol/ConfigurationManagerProtocol.h>
+
+#include "MpamGenerator.h"
+
+/**
+  ARM standard MPAM Generator
+
+  Requirements:
+    The following Configuration Manager Object(s) are used by this Generator:
+    - EArmObjMscNodeInfo (REQUIRED)
+    - EArmObjResNodeInfo
+*/
+
+/**
+  This macro expands to a function that retrieves the MSC Node information
+  from the Configuration Manager.
+*/
+GET_OBJECT_LIST (
+  EObjNameSpaceArm,
+  EArmObjMscNodeInfo,
+  CM_ARM_MSC_NODE_INFO
+  );
+
+/**
+  This macro expands to a function that retrieves the Resource Node
+  information from the Configuration Manager.
+*/
+GET_OBJECT_LIST (
+  EObjNameSpaceArm,
+  EArmObjResNodeInfo,
+  CM_ARM_RESOURCE_NODE_INFO
+  );
+
+/**
+  Returns the size of the MPAM Memory System Controller (MSC) Node
+
+  @param [in]  Node     Pointer to MSC Node Info CM object
+
+  @retval               Size of the MSC Node in bytes.
+**/
+STATIC
+UINT32
+GetMscNodeSize (
+  IN  CONST CM_ARM_MSC_NODE_INFO  *Node
+  )
+{
+  ASSERT (Node != NULL);
+
+  // <size of Memory System Controller Node>
+  return (UINT32)(sizeof (EFI_ACPI_6_4_MPAM_MSC_NODE) +
+                  Node->NumResourceNodes * sizeof (EFI_ACPI_6_4_MPAM_RESOURCE_NODE));
+}
+
+/** Returns the total size required for the MSC and
+    updates the Node Indexer.
+
+    This function calculates the size required for the node group
+    and also populates the Node Indexer array with offsets for the
+    individual nodes.
+
+    @param [in]       NodeStartOffset Offset from the start of the
+                                      MPAM where this node group starts.
+    @param [in]       NodeList        Pointer to MSC Group node list.
+    @param [in]       NodeCount       Count of the MSC Group nodes.
+    @param [in, out]  NodeIndexer     Pointer to the next Node Indexer.
+
+    @retval Total size of the ITS Group Nodes.
+**/
+STATIC
+UINT64
+GetSizeofMscGroupNodes (
+  IN      CONST UINT32                         NodeStartOffset,
+  IN      CONST CM_ARM_MSC_NODE_INFO           *NodeList,
+  IN            UINT32                         NodeCount,
+  IN OUT        MPAM_NODE_INDEXER     **CONST  NodeIndexer
+  )
+{
+  UINT64  Size;
+
+  ASSERT (NodeList != NULL);
+
+  Size = 0;
+  while (NodeCount-- != 0) {
+    (*NodeIndexer)->Token  = NodeList->Token;
+    (*NodeIndexer)->Object = (VOID *)NodeList;
+    (*NodeIndexer)->Offset = (UINT32)(Size + NodeStartOffset);
+    DEBUG ((
+      DEBUG_INFO,
+      "MPAM: MSC Node Indexer = %p, Token = %p, Object = %p, Offset = 0x%x\n",
+      *NodeIndexer,
+      (*NodeIndexer)->Token,
+      (*NodeIndexer)->Object,
+      (*NodeIndexer)->Offset
+      ));
+
+    Size += GetMscNodeSize (NodeList);
+
+    (*NodeIndexer)++;
+    NodeList++;
+  }
+
+  return Size;
+}
+
+/** Update the MSC Group Node Information.
+
+    @param [in]     This             Pointer to the table Generator.
+    @param [in]     CfgMgrProtocol   Pointer to the Configuration Manager
+                                     Protocol Interface.
+    @param [in]     Mpam             Pointer to MPAM table structure.
+    @param [in]     NodesStartOffset Offset for the start of the Msc Group
+                                     Nodes.
+    @param [in]     NodeList         Pointer to an array of Msc Group Node
+                                     Objects.
+    @param [in]     NodeCount        Number of Msc Group Node Objects.
+
+    @retval EFI_SUCCESS           Table generated successfully.
+    @retval EFI_INVALID_PARAMETER A parameter is invalid.
+    @retval EFI_NOT_FOUND         The required object was not found.
+**/
+STATIC
+EFI_STATUS
+AddMscNodes (
+  IN      CONST ACPI_TABLE_GENERATOR                                                       *CONST  This,
+  IN      CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL                                       *CONST  CfgMgrProtocol,
+  IN      CONST EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORING_TABLE_HEADER           *Mpam,
+  IN      CONST UINT32                                                                             NodesStartOffset,
+  IN      CONST CM_ARM_MSC_NODE_INFO                                                               *NodeList,
+  IN            UINT32                                                                             NodeCount
+  )
+{
+  EFI_STATUS                       Status;
+  EFI_ACPI_6_4_MPAM_MSC_NODE       *MscNode;
+  EFI_ACPI_6_4_MPAM_RESOURCE_NODE  *ResourceNodeArray;
+  CM_ARM_RESOURCE_NODE_INFO        *ResourceNodeList;
+  UINT64                           NodeLength;
+  UINT32                           ResourceNodeCount;
+
+  ASSERT (Mpam != NULL);
+
+  MscNode = (EFI_ACPI_6_4_MPAM_MSC_NODE *)((UINT8 *)Mpam + NodesStartOffset);
+
+  // Get the Resource Node info to update the MPAM MSC node
+  Status = GetEArmObjResNodeInfo (
+             CfgMgrProtocol,
+             CM_NULL_TOKEN,
+             &ResourceNodeList,
+             &ResourceNodeCount
+             );
+  if (EFI_ERROR (Status)) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "ERROR: MPAM: Failed to add resource nodes info. Status = %r\n",
+      Status
+      ));
+    return Status;
+  }
+
+  while (NodeCount-- != 0) {
+    NodeLength = GetMscNodeSize (NodeList);
+
+    if (NodeLength > MAX_UINT16) {
+      Status = EFI_INVALID_PARAMETER;
+      DEBUG ((
+        DEBUG_ERROR,
+        "ERROR: MPAM: MSC Node length 0x%lx > MAX_UINT16. Status = %r\n",
+        NodeLength,
+        Status
+        ));
+      return Status;
+    }
+
+    // Populate the node header
+    MscNode->Length                   = (UINT16)NodeLength;
+    MscNode->Reserved                 = EFI_ACPI_RESERVED_WORD;
+    MscNode->Identifier               = EFI_ACPI_RESERVED_DWORD;
+    MscNode->BaseAddress              = NodeList->BaseAddress;
+    MscNode->MmioSize                 = NodeList->MmioSize;
+    MscNode->OverflowInterrupt        = NodeList->OverflowInterrupt;
+    MscNode->OverflowInterruptFlags   = NodeList->OverflowInterruptFlags;
+    MscNode->Reserved1                = EFI_ACPI_RESERVED_DWORD;
+    MscNode->OverflowInterruptAff     = NodeList->OverflowInterruptAff;
+    MscNode->ErrorInterrupt           = NodeList->ErrorInterrupt;
+    MscNode->ErrorInterruptFlags      = NodeList->ErrorInterruptFlags;
+    MscNode->Reserved2                = EFI_ACPI_RESERVED_DWORD;
+    MscNode->ErrorInterruptAff        = NodeList->ErrorInterruptAff;
+    MscNode->MaxNRdyUsec              = NodeList->MaxNRdyUsec;
+    MscNode->LinkedDeviceHwId         = NodeList->LinkedDeviceHwId;
+    MscNode->LinkedDeviceInstanceHwId = NodeList->LinkedDeviceInstanceHwId;
+    MscNode->NumResourceNodes         = NodeList->NumResourceNodes;
+
+    // ResourceNode List for each MSC
+    if (MscNode->NumResourceNodes > 0) {
+      // Resource Node array for this Msc node
+      ResourceNodeArray = (EFI_ACPI_6_4_MPAM_RESOURCE_NODE *)((UINT8 *)MscNode + sizeof (EFI_ACPI_6_4_MPAM_MSC_NODE));
+      // Adding Resource Node content
+      while ( MscNode->NumResourceNodes-- != 0 ) {
+        ResourceNodeArray->Identifier  = ResourceNodeList->Identifier;
+        ResourceNodeArray->RisIndex    = ResourceNodeList->RisIndex;
+        ResourceNodeArray->Reserved1   = EFI_ACPI_RESERVED_WORD;
+        ResourceNodeArray->LocatorType = ResourceNodeList->LocatorType;
+        ResourceNodeArray->Locator     = ResourceNodeList->Locator;
+        ResourceNodeArray->NumFuncDep  = ResourceNodeList->NumFuncDep;
+
+        if (EFI_ERROR (Status)) {
+          DEBUG ((
+            DEBUG_ERROR,
+            "ERROR: MPAM: Failed to add Resource Node List. Status = %r\n",
+            Status
+            ));
+          return Status;
+        }
+
+        ResourceNodeList++;
+        ResourceNodeArray++;
+      }
+    }
+
+    // Next MSC Node
+    MscNode = (EFI_ACPI_6_4_MPAM_MSC_NODE *)((UINT8 *)MscNode + MscNode->Length);
+    NodeList++;
+  } // Msc Node
+
+  return EFI_SUCCESS;
+}
+
+/**
+  Construct the MPAM ACPI table.
+
+  This function invokes the Configuration Manager protocol interface
+  to get the required hardware information for generating the ACPI
+  table.
+
+  If this function allocates any resources then they must be freed
+  in the FreeXXXXTableResources function.
+
+  @param [in]  This                 Pointer to the table generator.
+  @param [in]  AcpiTableInfo        Pointer to the ACPI table generator to be used.
+  @param [in]  CfgMgrProtocol       Pointer to the Configuration Manager
+                                    Protocol Interface.
+  @param [out] Table                Pointer to the constructed ACPI Table.
+
+  @retval EFI_SUCCESS               Table generated successfully.
+  @retval EFI_INVALID_PARAMETER     A parameter is invalid.
+  @retval EFI_NOT_FOUND             The required object was not found.
+  @retval EFI_BAD_BUFFER_SIZE       The size returned by the Configuration
+                                    Manager is less than the Object size for
+                                    the requested object.
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+BuildMpamTable (
+  IN  CONST ACPI_TABLE_GENERATOR                  *CONST  This,
+  IN  CONST CM_STD_OBJ_ACPI_TABLE_INFO            *CONST  AcpiTableInfo,
+  IN  CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  *CONST  CfgMgrProtocol,
+  OUT       EFI_ACPI_DESCRIPTION_HEADER          **CONST  Table
+  )
+{
+  EFI_STATUS  Status;
+  UINT64      TableSize;
+  UINT64      NodeSize;
+  UINT32      MpamNodeCount;
+
+  UINT32  MscNodeCount;
+  UINT32  MscNodeOffset;
+
+  EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORING_TABLE_HEADER  *Mpam;
+  ACPI_MPAM_GENERATOR                                                       *Generator;
+  CM_ARM_MSC_NODE_INFO                                                      *MscNodeList;
+  MPAM_NODE_INDEXER                                                         *NodeIndexer;
+
+  ASSERT (
+    (This != NULL) &&
+    (AcpiTableInfo != NULL) &&
+    (CfgMgrProtocol != NULL) &&
+    (Table != NULL) &&
+    (AcpiTableInfo->TableGeneratorId == This->GeneratorID) &&
+    (AcpiTableInfo->AcpiTableSignature == This->AcpiTableSignature)
+    );
+
+  DEBUG ((
+    DEBUG_ERROR,
+    "DEBUG PRINT: MPAM: Requested table revision = %d\n",
+    AcpiTableInfo->AcpiTableRevision
+    ));
+
+  if ((AcpiTableInfo->AcpiTableRevision < This->MinAcpiTableRevision) ||
+      (AcpiTableInfo->AcpiTableRevision > This->AcpiTableRevision))
+  {
+    DEBUG ((
+      DEBUG_ERROR,
+      "ERROR: MPAM: Requested table revision = %d is not supported. "
+      "Supported table revisions: Minimum = %d. Maximum = %d\n",
+      AcpiTableInfo->AcpiTableRevision,
+      This->MinAcpiTableRevision,
+      This->AcpiTableRevision
+      ));
+    return EFI_INVALID_PARAMETER;
+  }
+
+  Generator = (ACPI_MPAM_GENERATOR *)This;
+  *Table    = NULL;
+
+  // Get the Memory System Controller Node info and update the MPAM
+  // structure count with MSC Node count (Type 0)
+  Status = GetEArmObjMscNodeInfo (
+             CfgMgrProtocol,
+             CM_NULL_TOKEN,
+             &MscNodeList,
+             &MscNodeCount
+             );
+  if (EFI_ERROR (Status)) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "ERROR: MPAM: Failed to get memory system controller node info. Status = %r\n",
+      Status
+      ));
+    goto error_handler;
+  }
+
+  MpamNodeCount           = MscNodeCount;
+  Generator->MscNodeCount = MscNodeCount;
+
+  // Allocate Node Indexer array
+  NodeIndexer = (MPAM_NODE_INDEXER *)AllocateZeroPool (
+                                       sizeof (MPAM_NODE_INDEXER) *
+                                       MpamNodeCount
+                                       );
+  if (NodeIndexer == NULL) {
+    Status = EFI_OUT_OF_RESOURCES;
+    DEBUG ((
+      DEBUG_ERROR,
+      "ERROR: MPAM: Failed to allocate memory for Node Indexer. Status = %r\n ",
+      Status
+      ));
+    goto error_handler;
+  }
+
+  DEBUG ((DEBUG_INFO, "MPAM INFO: NodeIndexer = %p\n", NodeIndexer));
+  Generator->MpamNodeCount = MpamNodeCount;
+  Generator->NodeIndexer   = NodeIndexer;
+
+  // Calculate the size of the MPAM table
+  TableSize = sizeof (EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORING_TABLE_HEADER);
+
+  // Include the size of MSC Nodes and index them
+  if (Generator->MscNodeCount != 0) {
+    MscNodeOffset = TableSize;
+    // Size of MSC nodes.
+    NodeSize = GetSizeofMscGroupNodes (
+                 MscNodeOffset,
+                 MscNodeList,
+                 Generator->MscNodeCount,
+                 &NodeIndexer
+                 );
+    if (NodeSize > MAX_UINT32) {
+      Status = EFI_INVALID_PARAMETER;
+      DEBUG ((
+        DEBUG_ERROR,
+        "ERROR: MPAM: Invalid Size of Group Nodes. Status = %r\n",
+        Status
+        ));
+      goto error_handler;
+    }
+
+    TableSize += NodeSize;
+
+    DEBUG ((
+      DEBUG_INFO,
+      " MscNodeCount = %d\n" \
+      " MscNodeOffset = 0x%x\n",
+      Generator->MscNodeCount,
+      MscNodeOffset
+      ));
+  }
+
+  DEBUG ((
+    DEBUG_INFO,
+    "INFO: MPAM:\n" \
+    " MpamNodeCount = %d\n" \
+    " TableSize = 0x%X\n",
+    MpamNodeCount,
+    TableSize
+    ));
+
+  if (TableSize > MAX_UINT32) {
+    Status = EFI_INVALID_PARAMETER;
+    DEBUG ((
+      DEBUG_ERROR,
+      "ERROR: MPAM: MPAM Table Size 0x%lx > MAX_UINT32," \
+      " Status = %r\n",
+      TableSize,
+      Status
+      ));
+    goto error_handler;
+  }
+
+  // Allocate the Buffer for the MPAM table
+  *Table = (EFI_ACPI_DESCRIPTION_HEADER *)AllocateZeroPool (TableSize);
+  if (*Table == NULL) {
+    Status = EFI_OUT_OF_RESOURCES;
+    DEBUG ((
+      DEBUG_ERROR,
+      "ERROR: MPAM: Failed to allocate memory for MPAM Table. " \
+      "Size = %d. Status = %r\n",
+      TableSize,
+      Status
+      ));
+    goto error_handler;
+  }
+
+  Mpam = (EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORING_TABLE_HEADER *)*Table;
+
+  DEBUG ((
+    DEBUG_INFO,
+    "MPAM: Mpam = 0x%p. TableSize = 0x%x\n",
+    Mpam,
+    TableSize
+    ));
+
+  // Add ACPI header
+  Status = AddAcpiHeader (
+             CfgMgrProtocol,
+             This,
+             &Mpam->Header,
+             AcpiTableInfo,
+             TableSize
+             );
+  if (EFI_ERROR (Status)) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "ERROR: MPAM: Failed to add ACPI header. Status = %r\n",
+      Status
+      ));
+    goto error_handler;
+  }
+
+  // Update MPAM table
+  Mpam->NumNodes   = MscNodeCount;
+  Mpam->NodeOffset = sizeof (EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORING_TABLE_HEADER);
+  Mpam->Reserved   = EFI_ACPI_RESERVED_DWORD;
+
+  // Add MSC Nodes to the generated table
+  if (Mpam->NumNodes != 0) {
+    Status = AddMscNodes (
+               This,
+               CfgMgrProtocol,
+               Mpam,
+               MscNodeOffset,
+               MscNodeList,
+               MscNodeCount
+               );
+    if (EFI_ERROR (Status)) {
+      DEBUG ((
+        DEBUG_ERROR,
+        "ERROR: MPAM: Failed to add MSC Nodes. Status = %r\n",
+        Status
+        ));
+      goto error_handler;
+    }
+  }
+
+  return Status;
+
+error_handler:
+  if (Generator->NodeIndexer != NULL) {
+    FreePool (Generator->NodeIndexer);
+    Generator->NodeIndexer = NULL;
+  }
+
+  if (*Table != NULL) {
+    FreePool (*Table);
+    *Table = NULL;
+  }
+
+  return Status;
+}
+
+/** Free any resources allocated for constructing the MPAM
+
+  @param [in]      This           Pointer to the table generator.
+  @param [in]      AcpiTableInfo  Pointer to the ACPI Table Info.
+  @param [in]      CfgMgrProtocol Pointer to the Configuration Manager
+                                  Protocol Interface.
+  @param [in, out] Table          Pointer to the ACPI Table.
+
+  @retval EFI_SUCCESS           The resources were freed successfully.
+  @retval EFI_INVALID_PARAMETER The table pointer is NULL or invalid.
+**/
+STATIC
+EFI_STATUS
+FreeMpamTableResources (
+  IN      CONST ACPI_TABLE_GENERATOR                  *CONST  This,
+  IN      CONST CM_STD_OBJ_ACPI_TABLE_INFO            *CONST  AcpiTableInfo,
+  IN      CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  *CONST  CfgMgrProtocol,
+  IN OUT        EFI_ACPI_DESCRIPTION_HEADER          **CONST  Table
+  )
+{
+  ACPI_MPAM_GENERATOR  *Generator;
+
+  ASSERT (This != NULL);
+  ASSERT (AcpiTableInfo != NULL);
+  ASSERT (CfgMgrProtocol != NULL);
+  ASSERT (AcpiTableInfo->TableGeneratorId == This->GeneratorID);
+  ASSERT (AcpiTableInfo->AcpiTableSignature == This->AcpiTableSignature);
+
+  Generator = (ACPI_MPAM_GENERATOR *)This;
+
+  // Free any memory allocated by the generator
+  if (Generator->NodeIndexer != NULL) {
+    FreePool (Generator->NodeIndexer);
+    Generator->NodeIndexer = NULL;
+  }
+
+  if ((Table == NULL) || (*Table == NULL)) {
+    DEBUG ((DEBUG_ERROR, "ERROR: MPAM: Invalid Table Pointer\n"));
+    return EFI_INVALID_PARAMETER;
+  }
+
+  FreePool (*Table);
+  *Table = NULL;
+
+  return EFI_SUCCESS;
+}
+
+/** The MPAM Table Generator revision.
+*/
+#define MPAM_GENERATOR_REVISION  CREATE_REVISION (1, 0)
+
+/** The interface for the MPAM Table Generator.
+*/
+STATIC
+ACPI_MPAM_GENERATOR  MpamGenerator = {
+  // ACPI table generator header
+  {
+    // Generator ID
+    CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdMpam),
+    // Generator Description
+    L"ACPI.STD.MPAM.GENERATOR",
+    // ACPI Table Signature
+    EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORING_TABLE_STRUCTURE_SIGNATURE,
+    // ACPI Table Revision supported by this Generator
+    EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORING_TABLE_REVISION,
+    // Minimum supported ACPI Table Revision
+    EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORING_TABLE_REVISION,
+    // Creator ID
+    TABLE_GENERATOR_CREATOR_ID_ARM,
+    // Creator Revision
+    MPAM_GENERATOR_REVISION,
+    // Build Table function
+    BuildMpamTable,
+    // Free Resource function
+    FreeMpamTableResources,
+    // Extended build function not needed
+    NULL,
+    // Extended build function not implemented by the generator.
+    // Hence extended free resource function is not required.
+    NULL
+  },
+
+  // MPAM Generator private data
+
+  // MPAM node count
+  0,
+  // MSC node count
+  0,
+
+  // Pointer to MPAM Node Indexer
+  NULL
+};
+
+/**
+  Register the Generator with the ACPI Table Factory.
+
+  @param [in]  ImageHandle        The handle to the image.
+  @param [in]  SystemTable        Pointer to the System Table.
+
+  @retval EFI_SUCCESS             The Generator is registered.
+  @retval EFI_INVALID_PARAMETER   A parameter is invalid.
+  @retval EFI_ALREADY_STARTED     The Generator for the Table ID
+                                  is already registered.
+**/
+EFI_STATUS
+EFIAPI
+AcpiMpamLibConstructor (
+  IN  EFI_HANDLE        ImageHandle,
+  IN  EFI_SYSTEM_TABLE  *SystemTable
+  )
+{
+  EFI_STATUS  Status;
+
+  Status = RegisterAcpiTableGenerator (&MpamGenerator.Header);
+  DEBUG ((DEBUG_INFO, "MPAM: Register Generator. Status = %r\n", Status));
+  ASSERT_EFI_ERROR (Status);
+  return Status;
+}
+
+/**
+  Deregister the Generator from the ACPI Table Factory.
+
+  @param [in]  ImageHandle        The handle to the image.
+  @param [in]  SystemTable        Pointer to the System Table.
+
+  @retval EFI_SUCCESS             The Generator is deregistered.
+  @retval EFI_INVALID_PARAMETER   A parameter is invalid.
+  @retval EFI_NOT_FOUND           The Generator is not registered.
+**/
+EFI_STATUS
+EFIAPI
+AcpiMpamLibDestructor (
+  IN  EFI_HANDLE        ImageHandle,
+  IN  EFI_SYSTEM_TABLE  *SystemTable
+  )
+{
+  EFI_STATUS  Status;
+
+  Status = DeregisterAcpiTableGenerator (&MpamGenerator.Header);
+  DEBUG ((DEBUG_INFO, "MPAM: Deregister Generator. Status = %r\n", Status));
+  ASSERT_EFI_ERROR (Status);
+  return Status;
+}
diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/MpamGenerator.h b/DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/MpamGenerator.h
new file mode 100644
index 0000000000..1075bd8c6c
--- /dev/null
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiMpamLibArm/MpamGenerator.h
@@ -0,0 +1,47 @@
+/** @file
+  Header file for the dynamic MPAM generator
+
+  Copyright (c) 2022, NVIDIA CORPORATION. All rights reserved.
+  Copyright (c) 2022, ARM Limited. All rights reserved.
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+  @par Reference(s):
+  - ACPI 6.4 Specification, January 2021
+  - ARM Architecture Reference Manual ARMv8
+
+  @par Glossary:
+    - Cm or CM   - Configuration Manager
+    - Obj or OBJ - Object
+**/
+
+#ifndef MPAM_GENERATOR_H_
+#define MPAM_GENERATOR_H_
+
+#pragma pack(1)
+
+/** A structure that describes the Node indexer
+    used for indexing the MPAM MSC nodes.
+*/
+typedef struct MpamNodeIndexer {
+  /// Index token for the Node
+  CM_OBJECT_TOKEN    Token;
+  /// Pointer to the node
+  VOID               *Object;
+  /// Node offset from the start of the MPAM table
+  UINT32             Offset;
+} MPAM_NODE_INDEXER;
+
+typedef struct AcpiMpamGenerator {
+  /// ACPI Table generator header
+  ACPI_TABLE_GENERATOR    Header;
+  /// MPAM structure count
+  UINT32                  MpamNodeCount;
+  /// Count of Msc Nodes
+  UINT32                  MscNodeCount;
+  /// List of indexed CM objects for MPAM generation
+  MPAM_NODE_INDEXER       *NodeIndexer;
+} ACPI_MPAM_GENERATOR;
+
+#pragma pack()
+
+#endif // MPAM_GENERATOR_H_
--
2.17.1


[-- Attachment #2: Type: text/html, Size: 57586 bytes --]

Re: [edk2-devel] [PATCH 1/2] Mde Pkg: Support for MPAM ACPI Table

[Rohit Mathew]

Hi Swatisri,

Thanks for the patch. Please find my comments inline marked [Rohit] -

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Name
> via groups.io
> Sent: 16 August 2022 21:18
> To: devel@edk2.groups.io; Sami Mujawar <Sami.Mujawar@arm.com>;
> Alexei Fedorov <Alexei.Fedorov@arm.com>; michael.d.kinney@intel.com;
> gaoliming@byosoft.com.cn; zhiguang.liu@intel.com
> Cc: Swatisri Kantamsetti <swatisrik@nvidia.com>
> Subject: [edk2-devel] [PATCH 1/2] Mde Pkg: Support for MPAM ACPI Table
> 
> From: Swatisri Kantamsetti <swatisrik@nvidia.com>
> 
> Added MPAM table header, MSC and Resource Node info structures
> 
> Signed-off-by: Swatisri Kantamsetti <swatisrik@nvidia.com>
> ---
>  MdePkg/Include/IndustryStandard/Acpi64.h |  5 ++
>  MdePkg/Include/IndustryStandard/Mpam.h   | 69
> ++++++++++++++++++++++++
>  2 files changed, 74 insertions(+)
>  create mode 100644 MdePkg/Include/IndustryStandard/Mpam.h
> 
> diff --git a/MdePkg/Include/IndustryStandard/Acpi64.h
> b/MdePkg/Include/IndustryStandard/Acpi64.h
> index fe5ebfac2b..e54f631186 100644
> --- a/MdePkg/Include/IndustryStandard/Acpi64.h
> +++ b/MdePkg/Include/IndustryStandard/Acpi64.h
> @@ -2952,6 +2952,11 @@ typedef struct {
>  ///
>  #define
> EFI_ACPI_6_4_PROCESSOR_PROPERTIES_TOPOLOGY_TABLE_STRUCTURE_SI
> GNATURE  SIGNATURE_32('P', 'P', 'T', 'T')
> 
> +///
> +/// "MPAM" Memory System Resource Partitioning And Monitoring Table
> ///
> +#define
> +EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORI
> NG_TABLE_STRUC
> +TURE_SIGNATURE  SIGNATURE_32('M', 'P', 'A', 'M')
> +
>  ///
>  /// "PSDT" Persistent System Description Table  /// diff --git
> a/MdePkg/Include/IndustryStandard/Mpam.h
> b/MdePkg/Include/IndustryStandard/Mpam.h
> new file mode 100644
> index 0000000000..e0f75f0114
> --- /dev/null
> +++ b/MdePkg/Include/IndustryStandard/Mpam.h
> @@ -0,0 +1,69 @@
> +/** @file
> +  ACPI Memory System Resource Partitioning And Monitoring (MPAM)
> +  as specified in ARM spec DEN0065
> +
> +  Copyright (c) 2022, NVIDIA CORPORATION. All rights reserved.
> +  Copyright (c) 2022, ARM Limited. All rights reserved.
> +  SPDX-License-Identifier: BSD-2-Clause-Patent **/
> +
> +#ifndef _MPAM_H_
> +#define _MPAM_H_
> +
> +#pragma pack(1)
> +
> +///
> +/// Memory System Resource Partitioning and Monitoring Table (MPAM)
> ///
> +typedef struct {
> +  EFI_ACPI_DESCRIPTION_HEADER    Header;
> +  UINT32                         NumNodes;
> +  UINT32                         NodeOffset;
> +  UINT32                         Reserved;
> +}
> +EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORI
> NG_TABLE_HEADE
> +R;

[Rohit] Shouldn't the header be followed by MSC node body type as defined in MPAM ACPI 1.0, section 2, table 3 - The MPAM table and section 2.1, table 4 - MSC Node body?

> +
> +///
> +/// MPAM Revision (as defined in ACPI 6.4 spec.) /// #define
> +EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORI
> NG_TABLE_REVIS
> +ION  0x01
> +
> +///
> +/// Memory System Controller Node Structure ///
> +
> +typedef struct {
> +  UINT16    Length;
> +  UINT16    Reserved;
> +  UINT32    Identifier;
> +  UINT64    BaseAddress;
> +  UINT32    MmioSize;
> +  UINT32    OverflowInterrupt;
> +  UINT32    OverflowInterruptFlags;

[Rohit] Would it be better to have a type (possibly bitfield struct) instead of a plain UINT32 for Flags? (MPAM ACPI 1.0, section 2.1.1, table 5 - Interrupt flags)

> +  UINT32    Reserved1;
> +  UINT32    OverflowInterruptAff;
> +  UINT32    ErrorInterrupt;
> +  UINT32    ErrorInterruptFlags;

[Rohit ] Same comment as before above.

> +  UINT32    Reserved2;
> +  UINT32    ErrorInterruptAff;
> +  UINT32    MaxNRdyUsec;
> +  UINT64    LinkedDeviceHwId;
> +  UINT32    LinkedDeviceInstanceHwId;
> +  UINT32    NumResourceNodes;
> +} EFI_ACPI_6_4_MPAM_MSC_NODE;
> +
> +///
> +/// Resource Node Structure
> +///
> +
> +typedef struct {
> +  UINT32    Identifier;
> +  UINT8     RisIndex;
> +  UINT16    Reserved1;
> +  UINT8     LocatorType;
> +  UINT64    Locator;

[Rohit ] Shouldn't " Locator " field be 12 bytes in size, possibly a separate type? (MPAM ACPI 1.0, section 2.2, table 7 - Resource node and section 2.3.2 table 10 - locator descriptor)

> +  UINT32    NumFuncDep;
> +} EFI_ACPI_6_4_MPAM_RESOURCE_NODE;

[Rohit] Since "NumFuncDep" field is part of EFI_ACPI_6_4_MPAM_RESOURCE_NODE type and this could be non-zero, should we also need the type for functional dependency descriptors? (MPAM ACPI 1.0, section 2.2.1, table 8 - Functional dependency descriptor)

[Rohit] Also, could some of the commonly used macros be added to this header, please? (location types, MPAM interrupt mode, interrupt types, affinity type, etc)

> +
> +#pragma pack()
> +
> +#endif
> --
> 2.17.1
> 
> 
> 
> 
> 

Regards,
Rohit

Re: [edk2-devel] [PATCH 1/2] Mde Pkg: Support for MPAM ACPI Table

[Andrew Fish]

[-- Attachment #1: Type: text/plain, Size: 6150 bytes --]



> On Aug 19, 2022, at 1:26 AM, Rohit Mathew <rohit.mathew@arm.com> wrote:
> 
> Hi Swatisri,
> 
> Thanks for the patch. Please find my comments inline marked [Rohit] -
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io <mailto:devel@edk2.groups.io> <devel@edk2.groups.io <mailto:devel@edk2.groups.io>> On Behalf Of Name
>> via groups.io <http://groups.io/>
>> Sent: 16 August 2022 21:18
>> To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Sami Mujawar <Sami.Mujawar@arm.com <mailto:Sami.Mujawar@arm.com>>;
>> Alexei Fedorov <Alexei.Fedorov@arm.com <mailto:Alexei.Fedorov@arm.com>>; michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>;
>> gaoliming@byosoft.com.cn <mailto:gaoliming@byosoft.com.cn>; zhiguang.liu@intel.com <mailto:zhiguang.liu@intel.com>
>> Cc: Swatisri Kantamsetti <swatisrik@nvidia.com <mailto:swatisrik@nvidia.com>>
>> Subject: [edk2-devel] [PATCH 1/2] Mde Pkg: Support for MPAM ACPI Table
>> 
>> From: Swatisri Kantamsetti <swatisrik@nvidia.com>
>> 
>> Added MPAM table header, MSC and Resource Node info structures
>> 
>> Signed-off-by: Swatisri Kantamsetti <swatisrik@nvidia.com>
>> ---
>> MdePkg/Include/IndustryStandard/Acpi64.h |  5 ++
>> MdePkg/Include/IndustryStandard/Mpam.h   | 69
>> ++++++++++++++++++++++++
>> 2 files changed, 74 insertions(+)
>> create mode 100644 MdePkg/Include/IndustryStandard/Mpam.h
>> 
>> diff --git a/MdePkg/Include/IndustryStandard/Acpi64.h
>> b/MdePkg/Include/IndustryStandard/Acpi64.h
>> index fe5ebfac2b..e54f631186 100644
>> --- a/MdePkg/Include/IndustryStandard/Acpi64.h
>> +++ b/MdePkg/Include/IndustryStandard/Acpi64.h
>> @@ -2952,6 +2952,11 @@ typedef struct {
>> ///
>> #define
>> EFI_ACPI_6_4_PROCESSOR_PROPERTIES_TOPOLOGY_TABLE_STRUCTURE_SI
>> GNATURE  SIGNATURE_32('P', 'P', 'T', 'T')
>> 
>> +///
>> +/// "MPAM" Memory System Resource Partitioning And Monitoring Table
>> ///
>> +#define
>> +EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORI
>> NG_TABLE_STRUC
>> +TURE_SIGNATURE  SIGNATURE_32('M', 'P', 'A', 'M')
>> +
>> ///
>> /// "PSDT" Persistent System Description Table  /// diff --git
>> a/MdePkg/Include/IndustryStandard/Mpam.h
>> b/MdePkg/Include/IndustryStandard/Mpam.h
>> new file mode 100644
>> index 0000000000..e0f75f0114
>> --- /dev/null
>> +++ b/MdePkg/Include/IndustryStandard/Mpam.h
>> @@ -0,0 +1,69 @@
>> +/** @file
>> +  ACPI Memory System Resource Partitioning And Monitoring (MPAM)
>> +  as specified in ARM spec DEN0065
>> +
>> +  Copyright (c) 2022, NVIDIA CORPORATION. All rights reserved.
>> +  Copyright (c) 2022, ARM Limited. All rights reserved.
>> +  SPDX-License-Identifier: BSD-2-Clause-Patent **/
>> +
>> +#ifndef _MPAM_H_
>> +#define _MPAM_H_
>> +
>> +#pragma pack(1)
>> +
>> +///
>> +/// Memory System Resource Partitioning and Monitoring Table (MPAM)
>> ///
>> +typedef struct {
>> +  EFI_ACPI_DESCRIPTION_HEADER    Header;
>> +  UINT32                         NumNodes;
>> +  UINT32                         NodeOffset;
>> +  UINT32                         Reserved;
>> +}
>> +EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORI
>> NG_TABLE_HEADE
>> +R;
> 
> [Rohit] Shouldn't the header be followed by MSC node body type as defined in MPAM ACPI 1.0, section 2, table 3 - The MPAM table and section 2.1, table 4 - MSC Node body?
> 
>> +
>> +///
>> +/// MPAM Revision (as defined in ACPI 6.4 spec.) /// #define
>> +EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORI
>> NG_TABLE_REVIS
>> +ION  0x01
>> +
>> +///
>> +/// Memory System Controller Node Structure ///
>> +
>> +typedef struct {
>> +  UINT16    Length;
>> +  UINT16    Reserved;
>> +  UINT32    Identifier;
>> +  UINT64    BaseAddress;
>> +  UINT32    MmioSize;
>> +  UINT32    OverflowInterrupt;
>> +  UINT32    OverflowInterruptFlags;
> 
> [Rohit] Would it be better to have a type (possibly bitfield struct) instead of a plain UINT32 for Flags? (MPAM ACPI 1.0, section 2.1.1, table 5 - Interrupt flags)
> 

Probably better NOT to use bitfields in APIs that are produced and consumed by different worlds. While the the UEFI does speak to the bit order of or a bitfield the rules around packing of bitfields is compiler defined.

I just saw a bug last week with bitfield compatibility that was introduced by clang fixing its -mms-bitfields implementation. The GCC rules for packing bitfields is different than VC++ so that is why the compiler flag -mms-bitfields exists in the 1st place . A clang -mms-bitfields bug  got fixed and it broke the code as the extra padding required by VC++ got added to the bitfield. 

Thanks,

Andrew Fish

>> +  UINT32    Reserved1;
>> +  UINT32    OverflowInterruptAff;
>> +  UINT32    ErrorInterrupt;
>> +  UINT32    ErrorInterruptFlags;
> 
> [Rohit ] Same comment as before above.
> 
>> +  UINT32    Reserved2;
>> +  UINT32    ErrorInterruptAff;
>> +  UINT32    MaxNRdyUsec;
>> +  UINT64    LinkedDeviceHwId;
>> +  UINT32    LinkedDeviceInstanceHwId;
>> +  UINT32    NumResourceNodes;
>> +} EFI_ACPI_6_4_MPAM_MSC_NODE;
>> +
>> +///
>> +/// Resource Node Structure
>> +///
>> +
>> +typedef struct {
>> +  UINT32    Identifier;
>> +  UINT8     RisIndex;
>> +  UINT16    Reserved1;
>> +  UINT8     LocatorType;
>> +  UINT64    Locator;
> 
> [Rohit ] Shouldn't " Locator " field be 12 bytes in size, possibly a separate type? (MPAM ACPI 1.0, section 2.2, table 7 - Resource node and section 2.3.2 table 10 - locator descriptor)
> 
>> +  UINT32    NumFuncDep;
>> +} EFI_ACPI_6_4_MPAM_RESOURCE_NODE;
> 
> [Rohit] Since "NumFuncDep" field is part of EFI_ACPI_6_4_MPAM_RESOURCE_NODE type and this could be non-zero, should we also need the type for functional dependency descriptors? (MPAM ACPI 1.0, section 2.2.1, table 8 - Functional dependency descriptor)
> 
> [Rohit] Also, could some of the commonly used macros be added to this header, please? (location types, MPAM interrupt mode, interrupt types, affinity type, etc)
> 
>> +
>> +#pragma pack()
>> +
>> +#endif
>> --
>> 2.17.1
>> 
>> 
>> 
>> 
>> 
> 
> Regards,
> Rohit
> 
> 
> 
> 


[-- Attachment #2: Type: text/html, Size: 22933 bytes --]

Re: [edk2-devel] [PATCH 1/2] Mde Pkg: Support for MPAM ACPI Table

[Rohit Mathew]

[-- Attachment #1: Type: text/plain, Size: 6457 bytes --]

Hi Andrew,

From: Andrew Fish <afish@apple.com>
Sent: 23 August 2022 21:11
To: devel@edk2.groups.io; Rohit Mathew <Rohit.Mathew@arm.com>
Cc: username@nvidia.com; Sami Mujawar <Sami.Mujawar@arm.com>; Alexei Fedorov <Alexei.Fedorov@arm.com>; Mike Kinney <michael.d.kinney@intel.com>; gaoliming@byosoft.com.cn; zhiguang.liu@intel.com; Swatisri Kantamsetti <swatisrik@nvidia.com>; Thomas Abraham <thomas.abraham@arm.com>; Thanu Rangarajan <Thanu.Rangarajan@arm.com>; nd <nd@arm.com>
Subject: Re: [edk2-devel] [PATCH 1/2] Mde Pkg: Support for MPAM ACPI Table




On Aug 19, 2022, at 1:26 AM, Rohit Mathew <rohit.mathew@arm.com<mailto:rohit.mathew@arm.com>> wrote:

Hi Swatisri,

Thanks for the patch. Please find my comments inline marked [Rohit] -


-----Original Message-----
From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Name
via groups.io<http://groups.io/>
Sent: 16 August 2022 21:18
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Sami Mujawar <Sami.Mujawar@arm.com<mailto:Sami.Mujawar@arm.com>>;
Alexei Fedorov <Alexei.Fedorov@arm.com<mailto:Alexei.Fedorov@arm.com>>; michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>;
gaoliming@byosoft.com.cn<mailto:gaoliming@byosoft.com.cn>; zhiguang.liu@intel.com<mailto:zhiguang.liu@intel.com>
Cc: Swatisri Kantamsetti <swatisrik@nvidia.com<mailto:swatisrik@nvidia.com>>
Subject: [edk2-devel] [PATCH 1/2] Mde Pkg: Support for MPAM ACPI Table

From: Swatisri Kantamsetti <swatisrik@nvidia.com<mailto:swatisrik@nvidia.com>>

Added MPAM table header, MSC and Resource Node info structures

Signed-off-by: Swatisri Kantamsetti <swatisrik@nvidia.com<mailto:swatisrik@nvidia.com>>
---
MdePkg/Include/IndustryStandard/Acpi64.h |  5 ++
MdePkg/Include/IndustryStandard/Mpam.h   | 69
++++++++++++++++++++++++
2 files changed, 74 insertions(+)
create mode 100644 MdePkg/Include/IndustryStandard/Mpam.h

diff --git a/MdePkg/Include/IndustryStandard/Acpi64.h
b/MdePkg/Include/IndustryStandard/Acpi64.h
index fe5ebfac2b..e54f631186 100644
--- a/MdePkg/Include/IndustryStandard/Acpi64.h
+++ b/MdePkg/Include/IndustryStandard/Acpi64.h
@@ -2952,6 +2952,11 @@ typedef struct {
///
#define
EFI_ACPI_6_4_PROCESSOR_PROPERTIES_TOPOLOGY_TABLE_STRUCTURE_SI
GNATURE  SIGNATURE_32('P', 'P', 'T', 'T')

+///
+/// "MPAM" Memory System Resource Partitioning And Monitoring Table
///
+#define
+EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORI
NG_TABLE_STRUC
+TURE_SIGNATURE  SIGNATURE_32('M', 'P', 'A', 'M')
+
///
/// "PSDT" Persistent System Description Table  /// diff --git
a/MdePkg/Include/IndustryStandard/Mpam.h
b/MdePkg/Include/IndustryStandard/Mpam.h
new file mode 100644
index 0000000000..e0f75f0114
--- /dev/null
+++ b/MdePkg/Include/IndustryStandard/Mpam.h
@@ -0,0 +1,69 @@
+/** @file
+  ACPI Memory System Resource Partitioning And Monitoring (MPAM)
+  as specified in ARM spec DEN0065
+
+  Copyright (c) 2022, NVIDIA CORPORATION. All rights reserved.
+  Copyright (c) 2022, ARM Limited. All rights reserved.
+  SPDX-License-Identifier: BSD-2-Clause-Patent **/
+
+#ifndef _MPAM_H_
+#define _MPAM_H_
+
+#pragma pack(1)
+
+///
+/// Memory System Resource Partitioning and Monitoring Table (MPAM)
///
+typedef struct {
+  EFI_ACPI_DESCRIPTION_HEADER    Header;
+  UINT32                         NumNodes;
+  UINT32                         NodeOffset;
+  UINT32                         Reserved;
+}
+EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORI
NG_TABLE_HEADE
+R;

[Rohit] Shouldn't the header be followed by MSC node body type as defined in MPAM ACPI 1.0, section 2, table 3 - The MPAM table and section 2.1, table 4 - MSC Node body?


+
+///
+/// MPAM Revision (as defined in ACPI 6.4 spec.) /// #define
+EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORI
NG_TABLE_REVIS
+ION  0x01
+
+///
+/// Memory System Controller Node Structure ///
+
+typedef struct {
+  UINT16    Length;
+  UINT16    Reserved;
+  UINT32    Identifier;
+  UINT64    BaseAddress;
+  UINT32    MmioSize;
+  UINT32    OverflowInterrupt;
+  UINT32    OverflowInterruptFlags;

[Rohit] Would it be better to have a type (possibly bitfield struct) instead of a plain UINT32 for Flags? (MPAM ACPI 1.0, section 2.1.1, table 5 - Interrupt flags)


Probably better NOT to use bitfields in APIs that are produced and consumed by different worlds. While the the UEFI does speak to the bit order of or a bitfield the rules around packing of bitfields is compiler defined.

I just saw a bug last week with bitfield compatibility that was introduced by clang fixing its -mms-bitfields implementation. The GCC rules for packing bitfields is different than VC++ so that is why the compiler flag -mms-bitfields exists in the 1st place . A clang -mms-bitfields bug  got fixed and it broke the code as the extra padding required by VC++ got added to the bitfield.

[Rohit] Thanks for bringing this point. I think, this type could be left untouched in that case.

Thanks,

Andrew Fish


+  UINT32    Reserved1;
+  UINT32    OverflowInterruptAff;
+  UINT32    ErrorInterrupt;
+  UINT32    ErrorInterruptFlags;

[Rohit ] Same comment as before above.


+  UINT32    Reserved2;
+  UINT32    ErrorInterruptAff;
+  UINT32    MaxNRdyUsec;
+  UINT64    LinkedDeviceHwId;
+  UINT32    LinkedDeviceInstanceHwId;
+  UINT32    NumResourceNodes;
+} EFI_ACPI_6_4_MPAM_MSC_NODE;
+
+///
+/// Resource Node Structure
+///
+
+typedef struct {
+  UINT32    Identifier;
+  UINT8     RisIndex;
+  UINT16    Reserved1;
+  UINT8     LocatorType;
+  UINT64    Locator;

[Rohit ] Shouldn't " Locator " field be 12 bytes in size, possibly a separate type? (MPAM ACPI 1.0, section 2.2, table 7 - Resource node and section 2.3.2 table 10 - locator descriptor)


+  UINT32    NumFuncDep;
+} EFI_ACPI_6_4_MPAM_RESOURCE_NODE;

[Rohit] Since "NumFuncDep" field is part of EFI_ACPI_6_4_MPAM_RESOURCE_NODE type and this could be non-zero, should we also need the type for functional dependency descriptors? (MPAM ACPI 1.0, section 2.2.1, table 8 - Functional dependency descriptor)

[Rohit] Also, could some of the commonly used macros be added to this header, please? (location types, MPAM interrupt mode, interrupt types, affinity type, etc)


+
+#pragma pack()
+
+#endif
--
2.17.1





Regards,
Rohit






[-- Attachment #2: Type: text/html, Size: 16593 bytes --]

Re: [edk2-devel] [PATCH 1/2] Mde Pkg: Support for MPAM ACPI Table

[Andrew Fish]

[-- Attachment #1: Type: text/plain, Size: 7678 bytes --]

Rohit,

FYI I seem to remember when we added the bitfield verbiage to the UEFI Spec it was because there was lots of platform code that was using it. We did not really want to encourage its use it in public interfaces. 

Given there is lots of code 1st kind of things going on I’d figured I’d mention this. 

Thanks,

Andrew Fish

> On Aug 23, 2022, at 2:28 PM, Rohit Mathew <rohit.mathew@arm.com> wrote:
> 
> Hi Andrew,
>  
> From: Andrew Fish <afish@apple.com <mailto:afish@apple.com>> 
> Sent: 23 August 2022 21:11
> To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Rohit Mathew <Rohit.Mathew@arm.com <mailto:Rohit.Mathew@arm.com>>
> Cc: username@nvidia.com <mailto:username@nvidia.com>; Sami Mujawar <Sami.Mujawar@arm.com <mailto:Sami.Mujawar@arm.com>>; Alexei Fedorov <Alexei.Fedorov@arm.com <mailto:Alexei.Fedorov@arm.com>>; Mike Kinney <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>>; gaoliming@byosoft.com.cn <mailto:gaoliming@byosoft.com.cn>; zhiguang.liu@intel.com <mailto:zhiguang.liu@intel.com>; Swatisri Kantamsetti <swatisrik@nvidia.com <mailto:swatisrik@nvidia.com>>; Thomas Abraham <thomas.abraham@arm.com <mailto:thomas.abraham@arm.com>>; Thanu Rangarajan <Thanu.Rangarajan@arm.com <mailto:Thanu.Rangarajan@arm.com>>; nd <nd@arm.com <mailto:nd@arm.com>>
> Subject: Re: [edk2-devel] [PATCH 1/2] Mde Pkg: Support for MPAM ACPI Table
>  
>  
> 
> 
> On Aug 19, 2022, at 1:26 AM, Rohit Mathew <rohit.mathew@arm.com <mailto:rohit.mathew@arm.com>> wrote:
>  
> Hi Swatisri,
> 
> Thanks for the patch. Please find my comments inline marked [Rohit] -
> 
> 
> -----Original Message-----
> From: devel@edk2.groups.io <mailto:devel@edk2.groups.io> <devel@edk2.groups.io <mailto:devel@edk2.groups.io>> On Behalf Of Name
> via groups.io <http://groups.io/>
> Sent: 16 August 2022 21:18
> To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Sami Mujawar <Sami.Mujawar@arm.com <mailto:Sami.Mujawar@arm.com>>;
> Alexei Fedorov <Alexei.Fedorov@arm.com <mailto:Alexei.Fedorov@arm.com>>; michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>;
> gaoliming@byosoft.com.cn <mailto:gaoliming@byosoft.com.cn>; zhiguang.liu@intel.com <mailto:zhiguang.liu@intel.com>
> Cc: Swatisri Kantamsetti <swatisrik@nvidia.com <mailto:swatisrik@nvidia.com>>
> Subject: [edk2-devel] [PATCH 1/2] Mde Pkg: Support for MPAM ACPI Table
> 
> From: Swatisri Kantamsetti <swatisrik@nvidia.com <mailto:swatisrik@nvidia.com>>
> 
> Added MPAM table header, MSC and Resource Node info structures
> 
> Signed-off-by: Swatisri Kantamsetti <swatisrik@nvidia.com <mailto:swatisrik@nvidia.com>>
> ---
> MdePkg/Include/IndustryStandard/Acpi64.h |  5 ++
> MdePkg/Include/IndustryStandard/Mpam.h   | 69
> ++++++++++++++++++++++++
> 2 files changed, 74 insertions(+)
> create mode 100644 MdePkg/Include/IndustryStandard/Mpam.h
> 
> diff --git a/MdePkg/Include/IndustryStandard/Acpi64.h
> b/MdePkg/Include/IndustryStandard/Acpi64.h
> index fe5ebfac2b..e54f631186 100644
> --- a/MdePkg/Include/IndustryStandard/Acpi64.h
> +++ b/MdePkg/Include/IndustryStandard/Acpi64.h
> @@ -2952,6 +2952,11 @@ typedef struct {
> ///
> #define
> EFI_ACPI_6_4_PROCESSOR_PROPERTIES_TOPOLOGY_TABLE_STRUCTURE_SI
> GNATURE  SIGNATURE_32('P', 'P', 'T', 'T')
> 
> +///
> +/// "MPAM" Memory System Resource Partitioning And Monitoring Table
> ///
> +#define
> +EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORI
> NG_TABLE_STRUC
> +TURE_SIGNATURE  SIGNATURE_32('M', 'P', 'A', 'M')
> +
> ///
> /// "PSDT" Persistent System Description Table  /// diff --git
> a/MdePkg/Include/IndustryStandard/Mpam.h
> b/MdePkg/Include/IndustryStandard/Mpam.h
> new file mode 100644
> index 0000000000..e0f75f0114
> --- /dev/null
> +++ b/MdePkg/Include/IndustryStandard/Mpam.h
> @@ -0,0 +1,69 @@
> +/** @file
> +  ACPI Memory System Resource Partitioning And Monitoring (MPAM)
> +  as specified in ARM spec DEN0065
> +
> +  Copyright (c) 2022, NVIDIA CORPORATION. All rights reserved.
> +  Copyright (c) 2022, ARM Limited. All rights reserved.
> +  SPDX-License-Identifier: BSD-2-Clause-Patent **/
> +
> +#ifndef _MPAM_H_
> +#define _MPAM_H_
> +
> +#pragma pack(1)
> +
> +///
> +/// Memory System Resource Partitioning and Monitoring Table (MPAM)
> ///
> +typedef struct {
> +  EFI_ACPI_DESCRIPTION_HEADER    Header;
> +  UINT32                         NumNodes;
> +  UINT32                         NodeOffset;
> +  UINT32                         Reserved;
> +}
> +EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORI
> NG_TABLE_HEADE
> +R;
> 
> [Rohit] Shouldn't the header be followed by MSC node body type as defined in MPAM ACPI 1.0, section 2, table 3 - The MPAM table and section 2.1, table 4 - MSC Node body?
> 
> 
> +
> +///
> +/// MPAM Revision (as defined in ACPI 6.4 spec.) /// #define
> +EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORI
> NG_TABLE_REVIS
> +ION  0x01
> +
> +///
> +/// Memory System Controller Node Structure ///
> +
> +typedef struct {
> +  UINT16    Length;
> +  UINT16    Reserved;
> +  UINT32    Identifier;
> +  UINT64    BaseAddress;
> +  UINT32    MmioSize;
> +  UINT32    OverflowInterrupt;
> +  UINT32    OverflowInterruptFlags;
> 
> [Rohit] Would it be better to have a type (possibly bitfield struct) instead of a plain UINT32 for Flags? (MPAM ACPI 1.0, section 2.1.1, table 5 - Interrupt flags)
> 
>  
> Probably better NOT to use bitfields in APIs that are produced and consumed by different worlds. While the the UEFI does speak to the bit order of or a bitfield the rules around packing of bitfields is compiler defined.
>  
> I just saw a bug last week with bitfield compatibility that was introduced by clang fixing its -mms-bitfields implementation. The GCC rules for packing bitfields is different than VC++ so that is why the compiler flag -mms-bitfields exists in the 1st place . A clang -mms-bitfields bug  got fixed and it broke the code as the extra padding required by VC++ got added to the bitfield. 
>  
> [Rohit] Thanks for bringing this point. I think, this type could be left untouched in that case.
>  
> Thanks,
>  
> Andrew Fish
> 
> 
> +  UINT32    Reserved1;
> +  UINT32    OverflowInterruptAff;
> +  UINT32    ErrorInterrupt;
> +  UINT32    ErrorInterruptFlags;
> 
> [Rohit ] Same comment as before above.
> 
> 
> +  UINT32    Reserved2;
> +  UINT32    ErrorInterruptAff;
> +  UINT32    MaxNRdyUsec;
> +  UINT64    LinkedDeviceHwId;
> +  UINT32    LinkedDeviceInstanceHwId;
> +  UINT32    NumResourceNodes;
> +} EFI_ACPI_6_4_MPAM_MSC_NODE;
> +
> +///
> +/// Resource Node Structure
> +///
> +
> +typedef struct {
> +  UINT32    Identifier;
> +  UINT8     RisIndex;
> +  UINT16    Reserved1;
> +  UINT8     LocatorType;
> +  UINT64    Locator;
> 
> [Rohit ] Shouldn't " Locator " field be 12 bytes in size, possibly a separate type? (MPAM ACPI 1.0, section 2.2, table 7 - Resource node and section 2.3.2 table 10 - locator descriptor)
> 
> 
> +  UINT32    NumFuncDep;
> +} EFI_ACPI_6_4_MPAM_RESOURCE_NODE;
> 
> [Rohit] Since "NumFuncDep" field is part of EFI_ACPI_6_4_MPAM_RESOURCE_NODE type and this could be non-zero, should we also need the type for functional dependency descriptors? (MPAM ACPI 1.0, section 2.2.1, table 8 - Functional dependency descriptor)
> 
> [Rohit] Also, could some of the commonly used macros be added to this header, please? (location types, MPAM interrupt mode, interrupt types, affinity type, etc)
> 
> 
> +
> +#pragma pack()
> +
> +#endif
> --
> 2.17.1
> 
> 
> 
> 
> 
> 
> Regards,
> Rohit
> 
> 
> 
> 
>  


[-- Attachment #2: Type: text/html, Size: 19516 bytes --]

Re: [edk2-devel] [PATCH 1/2] Mde Pkg: Support for MPAM ACPI Table

[Rohit Mathew]

That makes sense, Andrew. Thanks for the info.

Regards,
Rohit

>  Rohit,
>  
>  FYI I seem to remember when we added the bitfield verbiage to the UEFI Spec it was because there was lots of platform code that was using it. We did not really want to encourage its use it in public interfaces. 
>  
>  Given there is lots of code 1st kind of things going on I’d figured I’d mention this. 
>  
>  Thanks,
>  
>  Andrew Fish
>  
>  
>  On Aug 23, 2022, at 2:28 PM, Rohit Mathew <rohit.mathew@arm.com>  wrote:
> >   
> >  Hi Andrew,
> >   
> >  From: Andrew Fish <afish@apple.com>  
> >  Sent: 23 August 2022 21:11
> >  To: devel@edk2.groups.io; Rohit Mathew <Rohit.Mathew@arm.com> 
> >  Cc: username@nvidia.com; Sami Mujawar <Sami.Mujawar@arm.com> ; Alexei Fedorov <Alexei.Fedorov@arm.com> ; Mike Kinney <michael.d.kinney@intel.com> ; gaoliming@byosoft.com.cn; zhiguang.liu@intel.com; Swatisri Kantamsetti <swatisrik@nvidia.com> ; Thomas Abraham <thomas.abraham@arm.com> ; Thanu Rangarajan <Thanu.Rangarajan@arm.com> ; nd <nd@arm.com> 
> >  Subject: Re: [edk2-devel] [PATCH 1/2] Mde Pkg: Support for MPAM ACPI Table
> >   
> >   
> >  
> >  
> >  
> >  On Aug 19, 2022, at 1:26 AM, Rohit Mathew <rohit.mathew@arm.com>  wrote:
> > > >   
> > > >  Hi Swatisri,
> > > >  
> > > >  Thanks for the patch. Please find my comments inline marked [Rohit] -
> > > >  
> > > >  
> > > >  
> > > > >   -----Original Message-----
> > > > >   From: devel@edk2.groups.io <devel@edk2.groups.io>  On Behalf Of Name
> > > > >   via groups.io
> > > > >   Sent: 16 August 2022 21:18
> > > > >   To: devel@edk2.groups.io; Sami Mujawar <Sami.Mujawar@arm.com> ;
> > > > >   Alexei Fedorov <Alexei.Fedorov@arm.com> ; michael.d.kinney@intel.com;
> > > > >   gaoliming@byosoft.com.cn; zhiguang.liu@intel.com
> > > > >   Cc: Swatisri Kantamsetti <swatisrik@nvidia.com> 
> > > > >   Subject: [edk2-devel] [PATCH 1/2] Mde Pkg: Support for MPAM ACPI Table
> > > > >   
> > > > >   From: Swatisri Kantamsetti <swatisrik@nvidia.com> 
> > > > >   
> > > > >   Added MPAM table header, MSC and Resource Node info structures
> > > > >   
> > > > >   Signed-off-by: Swatisri Kantamsetti <swatisrik@nvidia.com> 
> > > > >   ---
> > > > >   MdePkg/Include/IndustryStandard/Acpi64.h |  5 ++
> > > > >   MdePkg/Include/IndustryStandard/Mpam.h   | 69
> > > > >    ++++++++++++++++++++++++
> > > > >   2 files changed, 74 insertions(+)
> > > > >   create mode 100644 MdePkg/Include/IndustryStandard/Mpam.h
> > > > >  
> > > > >   diff --git a/MdePkg/Include/IndustryStandard/Acpi64.h
> > > > >   b/MdePkg/Include/IndustryStandard/Acpi64.h
> > > > >   index fe5ebfac2b..e54f631186 100644
> > > > >   --- a/MdePkg/Include/IndustryStandard/Acpi64.h
> > > > >   +++ b/MdePkg/Include/IndustryStandard/Acpi64.h
> > > > >   @@ -2952,6 +2952,11 @@ typedef struct {
> > > > >   ///
> > > > >  #define
> > > > >  EFI_ACPI_6_4_PROCESSOR_PROPERTIES_TOPOLOGY_TABLE_STRUCTURE_SI
> > > > >  GNATURE  SIGNATURE_32('P', 'P', 'T', 'T')
> > > > >  
> > > > >  +///
> > > > >  +/// "MPAM" Memory System Resource Partitioning And Monitoring Table
> > > > >  ///
> > > > >  +#define
> > > > >  +EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORI
> > > > >  NG_TABLE_STRUC
> > > > >  +TURE_SIGNATURE  SIGNATURE_32('M', 'P', 'A', 'M')
> > > > >  +
> > > > >  ///
> > > > >  /// "PSDT" Persistent System Description Table  /// diff --git
> > > > >  a/MdePkg/Include/IndustryStandard/Mpam.h
> > > > >  b/MdePkg/Include/IndustryStandard/Mpam.h
> > > > >  new file mode 100644
> > > > >  index 0000000000..e0f75f0114
> > > > >  --- /dev/null
> > > > >  +++ b/MdePkg/Include/IndustryStandard/Mpam.h
> > > > >  @@ -0,0 +1,69 @@
> > > > >  +/** @file
> > > > >  +  ACPI Memory System Resource Partitioning And Monitoring (MPAM)
> > > > >  +  as specified in ARM spec DEN0065
> > > > >  +
> > > > >  +  Copyright (c) 2022, NVIDIA CORPORATION. All rights reserved.
> > > > >  +  Copyright (c) 2022, ARM Limited. All rights reserved.
> > > > >  +  SPDX-License-Identifier: BSD-2-Clause-Patent **/
> > > > >  +
> > > > >  +#ifndef _MPAM_H_
> > > > >  +#define _MPAM_H_
> > > > >  +
> > > > >  +#pragma pack(1)
> > > > >  +
> > > > >  +///
> > > > >  +/// Memory System Resource Partitioning and Monitoring Table (MPAM)
> > > > >  ///
> > > > >  +typedef struct {
> > > > >  +  EFI_ACPI_DESCRIPTION_HEADER    Header;
> > > > >  +  UINT32                         NumNodes;
> > > > >  +  UINT32                         NodeOffset;
> > > > >  +  UINT32                         Reserved;
> > > > >  +}
> > > > >  +EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORI
> > > > >  NG_TABLE_HEADE
> > > > >  +R;
> > > >  
> > > >  [Rohit] Shouldn't the header be followed by MSC node body type as defined in MPAM ACPI 1.0, section 2, table 3 - The MPAM table and section 2.1, table 4 - MSC Node body?
> > > >  
> > > >  
> > > >  
> > > > >  +
> > > > >  +///
> > > > >  +/// MPAM Revision (as defined in ACPI 6.4 spec.) /// #define
> > > > >  +EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORI
> > > > >  NG_TABLE_REVIS
> > > > >  +ION  0x01
> > > > >  +
> > > > >  +///
> > > > >  +/// Memory System Controller Node Structure ///
> > > > >  +
> > > > >  +typedef struct {
> > > > >  +  UINT16    Length;
> > > > >  +  UINT16    Reserved;
> > > > >  +  UINT32    Identifier;
> > > > >  +  UINT64    BaseAddress;
> > > > >  +  UINT32    MmioSize;
> > > > >  +  UINT32    OverflowInterrupt;
> > > > >  +  UINT32    OverflowInterruptFlags;
> > > >  
> > > >  [Rohit] Would it be better to have a type (possibly bitfield struct) instead of a plain UINT32 for Flags? (MPAM ACPI 1.0, section 2.1.1, table 5 - Interrupt flags)
> > > >  
> > > >  
> > > >  
> > >  Probably better NOT to use bitfields in APIs that are produced and consumed by different worlds. While the the UEFI does speak to the bit order of or a bitfield the rules around packing of bitfields is compiler defined.
> > >   
> > >  I just saw a bug last week with bitfield compatibility that was introduced by clang fixing its -mms-bitfields implementation. The GCC rules for packing bitfields is different than VC++ so that is why the compiler flag -mms-bitfields exists in the 1st place . A clang -mms-bitfields bug  got fixed and it broke the code as the extra padding required by VC++ got added to the bitfield. 
> >  
> >  [Rohit] Thanks for bringing this point. I think, this type could be left untouched in that case.
> >  
> > >  Thanks,
> > >   
> > >  Andrew Fish
> > >  
> > >  
> > >  
> > > > >  +  UINT32    Reserved1;
> > > > >  +  UINT32    OverflowInterruptAff;
> > > > >  +  UINT32    ErrorInterrupt;
> > > > >  +  UINT32    ErrorInterruptFlags;
> > > >  
> > > >  [Rohit ] Same comment as before above.
> > > >  
> > > >  
> > > >  
> > > > >  +  UINT32    Reserved2;
> > > > >  +  UINT32    ErrorInterruptAff;
> > > > >  +  UINT32    MaxNRdyUsec;
> > > > >  +  UINT64    LinkedDeviceHwId;
> > > > >  +  UINT32    LinkedDeviceInstanceHwId;
> > > > >  +  UINT32    NumResourceNodes;
> > > > >  +} EFI_ACPI_6_4_MPAM_MSC_NODE;
> > > > >  +
> > > > >  +///
> > > > >  +/// Resource Node Structure
> > > > >  +///
> > > > >  +
> > > > >  +typedef struct {
> > > > >  +  UINT32    Identifier;
> > > > >  +  UINT8     RisIndex;
> > > > >  +  UINT16    Reserved1;
> > > > >  +  UINT8     LocatorType;
> > > > >  +  UINT64    Locator;
> > > >  
> > > >  [Rohit ] Shouldn't " Locator " field be 12 bytes in size, possibly a separate type? (MPAM ACPI 1.0, section 2.2, table 7 - Resource node and section 2.3.2 table 10 - locator descriptor)
> > > >  
> > > >  
> > > >  
> > > > >  +  UINT32    NumFuncDep;
> > > > >  +} EFI_ACPI_6_4_MPAM_RESOURCE_NODE;
> > > >  
> > > >  [Rohit] Since "NumFuncDep" field is part of EFI_ACPI_6_4_MPAM_RESOURCE_NODE type and this could be non-zero, should we also need the type for functional dependency descriptors? (MPAM ACPI 1.0, section 2.2.1, table 8 - Functional dependency descriptor)
> > > >  
> > > >  [Rohit] Also, could some of the commonly used macros be added to this header, please? (location types, MPAM interrupt mode, interrupt types, affinity type, etc)
> > > >  
> > > >  
> > > >  
> > > > >   +
> > > > >  +#pragma pack()
> > > > >  +
> > > > >  +#endif
> > > > >  --
> > > > >  2.17.1
> > > >  
> > > >  
> > > >  
> > > >  
> > > >  
> > > >  
> > > >  Regards,
> > > >  Rohit

Re: [edk2-devel] [PATCH 1/2] Mde Pkg: Support for MPAM ACPI Table

[hesham.almatary]

[-- Attachment #1: Type: text/plain, Size: 468 bytes --]

Hello Rohit and Swatisri,

On Tue, Aug 23, 2022 at 09:11 PM, Andrew Fish wrote:

> 
> [Rohit ] Shouldn't " Locator " field be 12 bytes in size, possibly a
> separate type? (MPAM ACPI 1.0, section 2.2, table 7 - Resource node and
> section 2.3.2 table 10 - locator descriptor)

I'd just go for UINT64 locator1 and UINT32 locator2 and not necessarily a separate type. The interpretation of each depends on the the locator type (e.g., MPAM ACPI Tables 11-15).

[-- Attachment #2: Type: text/html, Size: 715 bytes --]

Re: [edk2-devel] [PATCH 1/2] Mde Pkg: Support for MPAM ACPI Table

[Rohit Mathew]

[-- Attachment #1: Type: text/plain, Size: 1709 bytes --]

Hi Hesham,

The idea for keeping it as separate type was to abstract locator field to have a view similar to that of its definition in the spec (MPAM ACPI 1.0, section 2.2, table 7 - Resource node.)

Something of the lines –

typedef struct {
  UINT32                   Identifier;
  UINT8                    RisIndex;
  UINT16                   Reserved1;
  UINT8                    LocatorType;
  MPAM_LOCATOR             Locator;
  UINT32                   NumDependencies;
} MPAM_MSC_RESOURCE;

Locator field could very well be a struct of UINT64 and UINT32 with just descriptor 1 and descriptor 2 as the field names  (Table 10, 2.3.2)– As you said, having hardcoded field names won’t work as the descriptors change with locator type.


///

/// MPAM MSC Locator field

///

typedef struct {

  UINT64                   Descriptor1;

  UINT32                   Descriptor2;

} MPAM_LOCATOR;

Let me know your thoughts on the approach.

Regards,
Rohit

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of hesham.almatary via groups.io
Sent: 24 August 2022 15:39
To: Andrew Fish <afish@apple.com>; devel@edk2.groups.io
Subject: Re: [edk2-devel] [PATCH 1/2] Mde Pkg: Support for MPAM ACPI Table

Hello Rohit and Swatisri,

On Tue, Aug 23, 2022 at 09:11 PM, Andrew Fish wrote:
[Rohit ] Shouldn't " Locator " field be 12 bytes in size, possibly a separate type? (MPAM ACPI 1.0, section 2.2, table 7 - Resource node and section 2.3.2 table 10 - locator descriptor)
I'd just go for UINT64 locator1 and UINT32 locator2 and not necessarily a separate type. The interpretation of each depends on the the locator type (e.g., MPAM ACPI Tables 11-15).


[-- Attachment #2: Type: text/html, Size: 7619 bytes --]

Re: [edk2-devel] [PATCH 1/2] Mde Pkg: Support for MPAM ACPI Table

[hesham.almatary]

Hello Rohit,

On 8/24/2022 5:25 PM, Rohit Mathew wrote:
> Hi Hesham,
>
> The idea for keeping it as separate type was to abstract locator field to have a view similar to that of its definition in the spec (MPAM ACPI 1.0, section 2.2, table 7 - Resource node.)
>
> Something of the lines –
>
> typedef struct {
>    UINT32                   Identifier;
>    UINT8                    RisIndex;
>    UINT16                   Reserved1;
>    UINT8                    LocatorType;
>    MPAM_LOCATOR             Locator;
>    UINT32                   NumDependencies;
> } MPAM_MSC_RESOURCE;
>
> Locator field could very well be a struct of UINT64 and UINT32 with just descriptor 1 and descriptor 2 as the field names  (Table 10, 2.3.2)– As you said, having hardcoded field names won’t work as the descriptors change with locator type.
>
>
> ///
>
> /// MPAM MSC Locator field
>
> ///
>
> typedef struct {
>
>    UINT64                   Descriptor1;
>
>    UINT32                   Descriptor2;
>
> } MPAM_LOCATOR;
>
> Let me know your thoughts on the approach.

Thanks for the explanation. It makes sense to me, I don't have a strong bias 
towards not having it as a struct.


> Regards,
> Rohit
>
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of hesham.almatary via groups.io
> Sent: 24 August 2022 15:39
> To: Andrew Fish <afish@apple.com>; devel@edk2.groups.io
> Subject: Re: [edk2-devel] [PATCH 1/2] Mde Pkg: Support for MPAM ACPI Table
>
> Hello Rohit and Swatisri,
>
> On Tue, Aug 23, 2022 at 09:11 PM, Andrew Fish wrote:
> [Rohit ] Shouldn't " Locator " field be 12 bytes in size, possibly a separate type? (MPAM ACPI 1.0, section 2.2, table 7 - Resource node and section 2.3.2 table 10 - locator descriptor)
> I'd just go for UINT64 locator1 and UINT32 locator2 and not necessarily a separate type. The interpretation of each depends on the the locator type (e.g., MPAM ACPI Tables 11-15).
> 

Re: [PATCH 1/2] Mde Pkg: Support for MPAM ACPI Table

[Swatisri Kantamsetti]

[-- Attachment #1: Type: text/plain, Size: 3718 bytes --]

Just a reminder to provide feedback on this patch.

Thanks,
Swati

swatisrik@nvidia.com
________________________________
From: Name <username@nvidia.com>
Sent: Tuesday, August 16, 2022 2:18 PM
To: devel@edk2.groups.io <devel@edk2.groups.io>; Sami.Mujawar@arm.com <Sami.Mujawar@arm.com>; Alexei.Fedorov@arm.com <Alexei.Fedorov@arm.com>; michael.d.kinney@intel.com <michael.d.kinney@intel.com>; gaoliming@byosoft.com.cn <gaoliming@byosoft.com.cn>; zhiguang.liu@intel.com <zhiguang.liu@intel.com>
Cc: Swatisri Kantamsetti <swatisrik@nvidia.com>
Subject: [PATCH 1/2] Mde Pkg: Support for MPAM ACPI Table

From: Swatisri Kantamsetti <swatisrik@nvidia.com>

Added MPAM table header, MSC and Resource Node
info structures

Signed-off-by: Swatisri Kantamsetti <swatisrik@nvidia.com>
---
 MdePkg/Include/IndustryStandard/Acpi64.h |  5 ++
 MdePkg/Include/IndustryStandard/Mpam.h   | 69 ++++++++++++++++++++++++
 2 files changed, 74 insertions(+)
 create mode 100644 MdePkg/Include/IndustryStandard/Mpam.h

diff --git a/MdePkg/Include/IndustryStandard/Acpi64.h b/MdePkg/Include/IndustryStandard/Acpi64.h
index fe5ebfac2b..e54f631186 100644
--- a/MdePkg/Include/IndustryStandard/Acpi64.h
+++ b/MdePkg/Include/IndustryStandard/Acpi64.h
@@ -2952,6 +2952,11 @@ typedef struct {
 ///
 #define EFI_ACPI_6_4_PROCESSOR_PROPERTIES_TOPOLOGY_TABLE_STRUCTURE_SIGNATURE  SIGNATURE_32('P', 'P', 'T', 'T')

+///
+/// "MPAM" Memory System Resource Partitioning And Monitoring Table
+///
+#define EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORING_TABLE_STRUCTURE_SIGNATURE  SIGNATURE_32('M', 'P', 'A', 'M')
+
 ///
 /// "PSDT" Persistent System Description Table
 ///
diff --git a/MdePkg/Include/IndustryStandard/Mpam.h b/MdePkg/Include/IndustryStandard/Mpam.h
new file mode 100644
index 0000000000..e0f75f0114
--- /dev/null
+++ b/MdePkg/Include/IndustryStandard/Mpam.h
@@ -0,0 +1,69 @@
+/** @file
+  ACPI Memory System Resource Partitioning And Monitoring (MPAM)
+  as specified in ARM spec DEN0065
+
+  Copyright (c) 2022, NVIDIA CORPORATION. All rights reserved.
+  Copyright (c) 2022, ARM Limited. All rights reserved.
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#ifndef _MPAM_H_
+#define _MPAM_H_
+
+#pragma pack(1)
+
+///
+/// Memory System Resource Partitioning and Monitoring Table (MPAM)
+///
+typedef struct {
+  EFI_ACPI_DESCRIPTION_HEADER    Header;
+  UINT32                         NumNodes;
+  UINT32                         NodeOffset;
+  UINT32                         Reserved;
+} EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORING_TABLE_HEADER;
+
+///
+/// MPAM Revision (as defined in ACPI 6.4 spec.)
+///
+#define EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORING_TABLE_REVISION  0x01
+
+///
+/// Memory System Controller Node Structure
+///
+
+typedef struct {
+  UINT16    Length;
+  UINT16    Reserved;
+  UINT32    Identifier;
+  UINT64    BaseAddress;
+  UINT32    MmioSize;
+  UINT32    OverflowInterrupt;
+  UINT32    OverflowInterruptFlags;
+  UINT32    Reserved1;
+  UINT32    OverflowInterruptAff;
+  UINT32    ErrorInterrupt;
+  UINT32    ErrorInterruptFlags;
+  UINT32    Reserved2;
+  UINT32    ErrorInterruptAff;
+  UINT32    MaxNRdyUsec;
+  UINT64    LinkedDeviceHwId;
+  UINT32    LinkedDeviceInstanceHwId;
+  UINT32    NumResourceNodes;
+} EFI_ACPI_6_4_MPAM_MSC_NODE;
+
+///
+/// Resource Node Structure
+///
+
+typedef struct {
+  UINT32    Identifier;
+  UINT8     RisIndex;
+  UINT16    Reserved1;
+  UINT8     LocatorType;
+  UINT64    Locator;
+  UINT32    NumFuncDep;
+} EFI_ACPI_6_4_MPAM_RESOURCE_NODE;
+
+#pragma pack()
+
+#endif
--
2.17.1


[-- Attachment #2: Type: text/html, Size: 6216 bytes --]

Re: [PATCH 1/2] Mde Pkg: Support for MPAM ACPI Table

[Sami Mujawar]

[-- Attachment #1: Type: text/plain, Size: 6039 bytes --]

Hi Swatisri,

Apologies for the delay in feedback.

There is a new version of "ACPI for Memory System Resource Partitioning 
and Monitoring 2.0, Platform Design Document" published on 2022/Sep/30 
at https://developer.arm.com/documentation/den0065/latest.

The version 2.0 of ACPI MPAM table has a few errata fixes and some new 
additions. Therefore, the version 1.0 of the ACPI MPAM table stands 
deprecated and must not be used.

I would therefore suggest that this patch is updated to reflect ACPI 
MPAM table version 2.0. I also have some feedback below that would still 
apply.

Regards,

Sami Mujawar

On 16/08/2022 09:18 pm, Name wrote:
> From: Swatisri Kantamsetti<swatisrik@nvidia.com>
>
> Added MPAM table header, MSC and Resource Node
> info structures
>
> Signed-off-by: Swatisri Kantamsetti<swatisrik@nvidia.com>
> ---
>   MdePkg/Include/IndustryStandard/Acpi64.h |  5 ++
>   MdePkg/Include/IndustryStandard/Mpam.h   | 69 ++++++++++++++++++++++++
>   2 files changed, 74 insertions(+)
>   create mode 100644 MdePkg/Include/IndustryStandard/Mpam.h
>
> diff --git a/MdePkg/Include/IndustryStandard/Acpi64.h b/MdePkg/Include/IndustryStandard/Acpi64.h
> index fe5ebfac2b..e54f631186 100644
> --- a/MdePkg/Include/IndustryStandard/Acpi64.h
> +++ b/MdePkg/Include/IndustryStandard/Acpi64.h
> @@ -2952,6 +2952,11 @@ typedef struct {
>   ///
>   #define EFI_ACPI_6_4_PROCESSOR_PROPERTIES_TOPOLOGY_TABLE_STRUCTURE_SIGNATURE  SIGNATURE_32('P', 'P', 'T', 'T')
>   
> +///
> +/// "MPAM" Memory System Resource Partitioning And Monitoring Table
> +///
> +#define EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORING_TABLE_STRUCTURE_SIGNATURE  SIGNATURE_32('M', 'P', 'A', 'M')
> +
>   ///
>   /// "PSDT" Persistent System Description Table
>   ///
> diff --git a/MdePkg/Include/IndustryStandard/Mpam.h b/MdePkg/Include/IndustryStandard/Mpam.h
> new file mode 100644
> index 0000000000..e0f75f0114
> --- /dev/null
> +++ b/MdePkg/Include/IndustryStandard/Mpam.h
> @@ -0,0 +1,69 @@
> +/** @file
> +  ACPI Memory System Resource Partitioning And Monitoring (MPAM)
> +  as specified in ARM spec DEN0065
> +
> +  Copyright (c) 2022, NVIDIA CORPORATION. All rights reserved.
> +  Copyright (c) 2022, ARM Limited. All rights reserved.
> +  SPDX-License-Identifier: BSD-2-Clause-Patent

[SAMI] Please add a '@par Specification Reference:' section as shown in 
https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/53_include_files#5.3.7-include-files-shall-not-generate-code-or-define-data-variables.

e.g.

@par Specification Reference:

- [1] ACPI for Memory System Resource Partitioning and Monitoring 2.0, 
Platform Design Document

          (https://developer.arm.com/documentation/den0065/latest)

[/SAMI]

> +**/
> +
> +#ifndef _MPAM_H_
> +#define _MPAM_H_
[SAMI] I think the leading underscore should not be present, see 
https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/4_naming_conventions/43_identifiers#4.3.5.4-the-names-of-guard-macros-shall-end-with-an-underscore-character.
> +
> +#pragma pack(1)
> +
> +///
> +/// Memory System Resource Partitioning and Monitoring Table (MPAM)
> +///
> +typedef struct {
> +  EFI_ACPI_DESCRIPTION_HEADER    Header;
> +  UINT32                         NumNodes;
> +  UINT32                         NodeOffset;
> +  UINT32                         Reserved;
[SAMI] I do not see the NumNodes, NodeOffset and the Reserved filed in 
the specification. Can you check, please?
> +} EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORING_TABLE_HEADER;

[SAMI] I think the infix '_6_4_' is not going to work, as the ACPI MPAM 
table specification is not going to be in sync with the ACPI 
specification revision. So, this can be dropped.

Also, it may be better to add a @glossary section in the file header 
describing the MPAM acronym and abbreviate the structure names to 
EFI_ACPI_MPAM_TABLE_HEADER. The macro describing the table revision can 
also follow a similar approach.

[/SAMI]

> +
> +///
> +/// MPAM Revision (as defined in ACPI 6.4 spec.)
[SAMI] I believe this should be (a defined by the 'ACPI for Memory 
System Resource Partitioning and Monitoring, Platform Design Document'). 
Or simply refer it as '(as defined by [1])'.
> +///
> +#define EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORING_TABLE_REVISION  0x01
[SAMI] Please move this definition before the definition of 
EFI_ACPI_6_4_MEMORY_SYSTEM_RESOURCE_PARTITIONING_MONITORING_TABLE_HEADER. 
The ACPI MPAM 1.0 table revision was 0. But for ACPI MPAM table 2.0 the 
revision is 1, so there is no need to change.
> +
> +///
> +/// Memory System Controller Node Structure
> +///
> +
> +typedef struct {
> +  UINT16    Length;
> +  UINT16    Reserved;
[SAMI] ACPI MPAM 2.0 introduces splits the Reserved field to introduce 
'Interface type'.
> +  UINT32    Identifier;
> +  UINT64    BaseAddress;
> +  UINT32    MmioSize;
> +  UINT32    OverflowInterrupt;
> +  UINT32    OverflowInterruptFlags;
> +  UINT32    Reserved1;
> +  UINT32    OverflowInterruptAff;
> +  UINT32    ErrorInterrupt;
> +  UINT32    ErrorInterruptFlags;
> +  UINT32    Reserved2;
> +  UINT32    ErrorInterruptAff;
> +  UINT32    MaxNRdyUsec;
> +  UINT64    LinkedDeviceHwId;
> +  UINT32    LinkedDeviceInstanceHwId;
> +  UINT32    NumResourceNodes;
> +} EFI_ACPI_6_4_MPAM_MSC_NODE;
> +
> +///
> +/// Resource Node Structure
> +///
> +
> +typedef struct {
> +  UINT32    Identifier;
> +  UINT8     RisIndex;
> +  UINT16    Reserved1;
> +  UINT8     LocatorType;
> +  UINT64    Locator;

[SAMI] Would it be possible to define a Locator structure with the first 
field as the LocatorType followed by a union of Locators type specific 
structures?

> +  UINT32    NumFuncDep;
> +} EFI_ACPI_6_4_MPAM_RESOURCE_NODE;
> +

[SAMI] Do you have plans to add definitions for the other structures 
defined by the specifications, or the intention is to add them as required?

Also, if possible, it would be good to add an ACPI MPAM table parser to 
Acpiview.

[/SAMI]

> +#pragma pack()
> +
> +#endif

[-- Attachment #2: Type: text/html, Size: 13740 bytes --]