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


Name <username@...>
 

From: Swatisri Kantamsetti <swatisrik@...>

Added MPAM table header, MSC and Resource Node
info structures

Signed-off-by: Swatisri Kantamsetti <swatisrik@...>
---
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


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@...>;
Alexei Fedorov <Alexei.Fedorov@...>; michael.d.kinney@...;
gaoliming@...; zhiguang.liu@...
Cc: Swatisri Kantamsetti <swatisrik@...>
Subject: [edk2-devel] [PATCH 1/2] Mde Pkg: Support for MPAM ACPI Table

From: Swatisri Kantamsetti <swatisrik@...>

Added MPAM table header, MSC and Resource Node info structures

Signed-off-by: Swatisri Kantamsetti <swatisrik@...>
---
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


Swatisri Kantamsetti <swatisrik@...>
 

Just a reminder to provide feedback on this patch.

Thanks,
Swati

swatisrik@...

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

Added MPAM table header, MSC and Resource Node
info structures

Signed-off-by: Swatisri Kantamsetti <swatisrik@...>
---
 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


Andrew Fish
 



On Aug 19, 2022, at 1:26 AM, Rohit Mathew <rohit.mathew@...> 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@...>;
Alexei Fedorov <Alexei.Fedorov@...>; michael.d.kinney@...;
gaoliming@...; zhiguang.liu@...
Cc: Swatisri Kantamsetti <swatisrik@...>
Subject: [edk2-devel] [PATCH 1/2] Mde Pkg: Support for MPAM ACPI Table

From: Swatisri Kantamsetti <swatisrik@...>

Added MPAM table header, MSC and Resource Node info structures

Signed-off-by: Swatisri Kantamsetti <swatisrik@...>
---
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





Rohit Mathew
 

Hi Andrew,

 

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

From: Swatisri Kantamsetti <
swatisrik@...>

Added MPAM table header, MSC and Resource Node info structures

Signed-off-by: Swatisri Kantamsetti <
swatisrik@...>
---
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



 


Andrew Fish
 

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@...> wrote:

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

From: Swatisri Kantamsetti <
swatisrik@...>

Added MPAM table header, MSC and Resource Node info structures

Signed-off-by: Swatisri Kantamsetti <
swatisrik@...>
---
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



 


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@...> wrote:

Hi Andrew,

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

From: Swatisri Kantamsetti <swatisrik@...>

Added MPAM table header, MSC and Resource Node info structures

Signed-off-by: Swatisri Kantamsetti <swatisrik@...>
---
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


Hesham Almatary
 

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). 


Rohit Mathew
 

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@...>; 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). 


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@...>; 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).


Sami Mujawar
 

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@...>

Added MPAM table header, MSC and Resource Node
info structures

Signed-off-by: Swatisri Kantamsetti <swatisrik@...>
---
 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