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

["Rohit Mathew" <rohit.mathew@arm.com>]

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