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; usern...@nvidia.com; Sami Mujawar
> <sami.muja...@arm.com>; Alexei Fedorov <alexei.fedo...@arm.com>;
> michael.d.kin...@intel.com; gaolim...@byosoft.com.cn;
> zhiguang....@intel.com
> Cc: Swatisri Kantamsetti <swatis...@nvidia.com>; Thomas Abraham
> <thomas.abra...@arm.com>; Thanu Rangarajan
> <thanu.rangara...@arm.com>; nd <n...@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.muja...@arm.com>;
> Alexei
> > Fedorov <alexei.fedo...@arm.com>; michael.d.kin...@intel.com;
> > gaolim...@byosoft.com.cn; zhiguang....@intel.com
> > Cc: Swatisri Kantamsetti <swatis...@nvidia.com>
> > Subject: [edk2-devel] [PATCH 2/2] Dynamic Tbl Mgr: MPAM: MPAM
> > Generator and supporting files
> >
> > From: Swatisri Kantamsetti <swatis...@nvidia.com>
> >
> > ACPI header, MSC and Resource Nodes are populated in the MPAM Table
> >
> > Signed-off-by: Swatisri Kantamsetti <swatis...@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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#92750): https://edk2.groups.io/g/devel/message/92750
Mute This Topic: https://groups.io/mt/93069489/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
