Re: [edk2][PATCH V1 1/1] MdePkg/IndustryStandard: add definitions for MPAM ACPI specification

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

Thanks Sami, Michael, Liming for the review/ack.

I'll post a V2 with the comments addressed. Let me also work on the Acpi parser for the same.

Regards,
Rohit

> -----Original Message-----
> From: Sami Mujawar <Sami.Mujawar@arm.com>
> Sent: Wednesday, March 29, 2023 10:57 AM
> To: Rohit Mathew <Rohit.Mathew@arm.com>; devel@edk2.groups.io
> Cc: Michael D Kinney <michael.d.kinney@intel.com>; Liming Gao
> <gaoliming@byosoft.com.cn>; Zhiguang Liu <ZhiguangLiu@arm.com>; Thomas
> Abraham <thomas.abraham@arm.com>; Swatisri Kantamsetti
> <swatisrik@nvidia.com>; nd <nd@arm.com>
> Subject: Re: [edk2][PATCH V1 1/1] MdePkg/IndustryStandard: add definitions
> for MPAM ACPI specification
> 
> Hi Rohit,
> 
> Thank you for this patch, and apologies for the delay in reply.
> 
> I have a few minor suggestions marked inline as [SAMI].
> 
> Other than that, I would like to see a corresponding patch to add MPAM
> support to Acpiview.
> 
> With that addressed,
> 
> Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
> 
> regards,
> 
> Sami Mujawar
> 
> On 25/01/2023 12:39 am, Rohit Mathew wrote:
> > add definitions, macros and types for elements associated with MPAM
> > ACPI 2.0 specification.
> >
> > Signed-off-by: Rohit Mathew <rohit.mathew@arm.com>
> > ---
> >   MdePkg/Include/IndustryStandard/Acpi64.h |   7 +-
> >   MdePkg/Include/IndustryStandard/Mpam.h   | 229
> ++++++++++++++++++++
> >   2 files changed, 235 insertions(+), 1 deletion(-)
> >
> > github link:
> > https://github.com/rohit-arm/edk2/tree/mpam_acpi
> >
> > diff --git a/MdePkg/Include/IndustryStandard/Acpi64.h
> > b/MdePkg/Include/IndustryStandard/Acpi64.h
> > index 575ca0430c..2be9525979 100644
> > --- a/MdePkg/Include/IndustryStandard/Acpi64.h
> > +++ b/MdePkg/Include/IndustryStandard/Acpi64.h
> > @@ -2,7 +2,7 @@
> >     ACPI 6.4 definitions from the ACPI Specification Revision 6.4 Jan, 2021.
> >
> >     Copyright (c) 2017 - 2022, Intel Corporation. All rights
> > reserved.<BR>
> > -  Copyright (c) 2019 - 2021, ARM Ltd. All rights reserved.<BR>
> > +  Copyright (c) 2019 - 2023, ARM Limited. All rights reserved.<BR>
> >
> >     SPDX-License-Identifier: BSD-2-Clause-Patent
> >   **/
> > @@ -3157,6 +3157,11 @@ typedef struct {
> >   ///
> >   #define EFI_ACPI_6_4_XEN_PROJECT_TABLE_SIGNATURE
> SIGNATURE_32('X',
> > 'E', 'N', 'V')
> >
> > +///
> > +/// "MPAM" Memory System Resource Partitioning and Monitoring Table
> > +/// #define
> >
> +EFI_ACPI_MEMORY_SYSTEM_RESOURCE_PARTITIONING_AND_MONITORING
> _TABLE_SIG
> > +NATURE  SIGNATURE_32('M', 'P', 'A', 'M')
> > +
> >   #pragma pack()
> >
> >   #endif
> > diff --git a/MdePkg/Include/IndustryStandard/Mpam.h
> > b/MdePkg/Include/IndustryStandard/Mpam.h
> > new file mode 100644
> > index 0000000000..8e6e72ea28
> > --- /dev/null
> > +++ b/MdePkg/Include/IndustryStandard/Mpam.h
> > @@ -0,0 +1,229 @@
> > +/** @file
> > +  ACPI for Memory System Resource Partitioning and Monitoring 2.0
> > +(MPAM) as
> > +  specified in ARM spec DEN0065
> > +
> > +  Copyright (c) 2023, Arm Limited. All rights reserved.
> > +
> > +  SPDX-License-Identifier: BSD-2-Clause-Patent
> > +
> > +  @par Specification Reference:
> > +   - [1] ACPI for Memory System Resource Partitioning and Monitoring 2.0
> > +     (https://developer.arm.com/documentation/den0065/latest)
> > +
> > +  @par Glossary:
> > +    - MPAM - Memory System Resource Partitioning And Monitoring
> > +    - MSC  - Memory System Component
> > +    - PCC  - Platform Communication Channel
> > +    - RIS  - Resource Instance Selection
> > +    - SMMU - Arm System Memory Management Unit **/
> > +
> > +#ifndef MPAM_H_
> > +#define MPAM_H_
> > +
> > +#include <IndustryStandard/Acpi.h>
> > +
> > +///
> > +/// MPAM Revision
> > +///
> > +#define
> >
> +EFI_ACPI_MEMORY_SYSTEM_RESOURCE_PARTITIONING_AND_MONITORING
> _TABLE_REV
> > +ISION  (0x01)
> > +
> > +///
> > +/// MPAM Interrupt mode
> > +///
> > +#define EFI_ACPI_MPAM_INTERRUPT_LEVEL_TRIGGERED  (0x0)
> > +#define EFI_ACPI_MPAM_INTERRUPT_EDGE_TRIGGERED   (0x1)
> > +
> > +///
> > +/// MPAM Interrupt type
> > +///
> > +#define EFI_ACPI_MPAM_INTERRUPT_WIRED  (0x0)
> > +
> > +///
> > +/// MPAM Interrupt affinity type
> > +///
> > +#define EFI_ACPI_MPAM_PROCESSOR_AFFINITY            (0x0)
> > +#define EFI_ACPI_MPAM_PROCESSOR_CONTAINER_AFFINITY  (0x1)
> > +
> > +///
> > +/// MPAM MSC affinity valid
> > +///
> > +#define EFI_ACPI_MPAM_AFFINITY_NOT_VALID  (0x0)
> > +#define EFI_ACPI_MPAM_AFFINITY_VALID      (0x1)
> > +
> > +///
> > +/// MPAM Interrupt flag - bit positions /// #define
> > +EFI_ACPI_MPAM_INTERRUPT_MODE_POS  (0x0) #define
> > +EFI_ACPI_MPAM_INTERRUPT_TYPE_POS  (0x1)
> > +#define EFI_ACPI_MPAM_AFFINITY_TYPE_POS   (0x3)
> > +#define EFI_ACPI_MPAM_AFFINITY_VALID_POS  (0x4)
> > +#define EFI_ACPI_MPAM_RESERVED_POS        (0x5)
> [SAMI] I think it would be better to change the postfix _POS to _SHIFT and
> corresponing definitions for masks
> (i.e.EFI_ACPI_MPAM_INTERRUPT_MODE_MASK) be added.
> > +
> > +///
> > +/// MPAM Location types
> > +/// as described in document [1], table 11 /// #define
> > +EFI_ACPI_MPAM_LOCATION_PROCESSOR_CACHE  (0x0)
> > +#define EFI_ACPI_MPAM_LOCATION_MEMORY           (0x1)
> > +#define EFI_ACPI_MPAM_LOCATION_SMMU             (0x2)
> > +#define EFI_ACPI_MPAM_LOCATION_MEMORY_CACHE     (0x3)
> > +#define EFI_ACPI_MPAM_LOCATION_ACPI_DEVICE      (0x4)
> > +#define EFI_ACPI_MPAM_LOCATION_INTERCONNECT     (0x5)
> > +#define EFI_ACPI_MPAM_LOCATION_UNKNOWN          (0xFF)
> > +
> > +///
> > +/// MPAM Interface types
> > +///
> > +#define EFI_ACPI_MPAM_INTERFACE_MMIO  (0x0)
> > +#define EFI_ACPI_MPAM_INTERFACE_PCC   (0x0A)
> > +
> > +#pragma pack(1)
> > +
> > +///
> > +/// MPAM MSC generic locator descriptor /// as described in document
> > +[1], table 12 /// typedef struct {
> > +  UINT64    Descriptor1;
> > +  UINT32    Descriptor2;
> > +} EFI_ACPI_MPAM_GENERIC_LOCATOR;
> > +
> > +///
> > +/// MPAM processor cache locator descriptor /// as described in
> > +document [1], table 13 /// typedef struct {
> > +  UINT64    CacheReference;
> > +  UINT32    Reserved;
> > +} EFI_ACPI_MPAM_CACHE_LOCATOR;
> > +
> > +///
> > +/// MPAM memory locator descriptor
> > +/// as described in document [1], table 14 /// typedef struct {
> > +  UINT64    ProximityDomain;
> > +  UINT32    Reserved;
> > +} EFI_ACPI_MPAM_MEMORY_LOCATOR;
> > +
> > +///
> > +/// MPAM SMMU locator descriptor
> > +/// as described in document [1], table 15 /// typedef struct {
> > +  UINT64    SmmuInterface;
> > +  UINT32    Reserved;
> > +} EFI_ACPI_MPAM_SMMU_LOCATOR;
> > +
> > +///
> > +/// MPAM memory-side cache locator descriptor /// as described in
> > +Document [1], table 16 /// typedef struct {
> > +  UINT8     Reserved[7];
> > +  UINT8     Level;
> > +  UINT32    Reference;
> > +} EFI_ACPI_MPAM_MEMORY_CACHE_LOCATOR;
> > +
> > +///
> > +/// MPAM ACPI device locator descriptor /// as described in document
> > +[1], table 17 /// typedef struct {
> > +  UINT64    AcpiHardwareId;
> > +  UINT32    AcpiUniqueId;
> > +} EFI_ACPI_MPAM_ACPI_LOCATOR;
> > +
> > +///
> > +/// MPAM interconnect locator descriptor /// as described in document
> > +[1], table 18 /// typedef struct {
> > +  UINT64    InterconnectDescTblOff;
> > +  UINT32    Reserved;
> > +} EFI_ACPI_MPAM_INTERCONNECT_LOCATOR;
> > +
> > +///
> > +/// MPAM interconnect descriptor
> > +/// as described in document [1], table 19 /// typedef struct {
> > +  UINT32    SourceID;
> > +  UINT32    DestinationID;
> > +  UINT8     LinkType;
> > +  UINT8     Reserved[3];
> > +} EFI_ACPI_MPAM_INTERCONNECT_DESCRIPTOR;
> > +
> > +///
> > +/// MPAM interconnect descriptor table /// as described in document
> > +[1], table 20 /// typedef struct {
> > +  UINT8     Signature[16];
> > +  UINT32    NumDescriptors;
> > +} EFI_ACPI_MPAM_INTERCONNECT_DESCRIPTOR_TABLE;
> > +
> > +///
> > +/// MPAM resource locator
> > +///
> > +typedef union {
> > +  EFI_ACPI_MPAM_CACHE_LOCATOR           CacheLocator;
> > +  EFI_ACPI_MPAM_MEMORY_LOCATOR          MemoryLocator;
> > +  EFI_ACPI_MPAM_SMMU_LOCATOR            SmmuLocator;
> > +  EFI_ACPI_MPAM_MEMORY_CACHE_LOCATOR    MemCacheLocator;
> > +  EFI_ACPI_MPAM_ACPI_LOCATOR            AcpiLocator;
> > +  EFI_ACPI_MPAM_INTERCONNECT_LOCATOR    InterconnectIfcLocator;
> > +  EFI_ACPI_MPAM_GENERIC_LOCATOR         GenericLocator;
> > +} EFI_ACPI_MPAM_LOCATOR;
> > +
> > +///
> > +/// MPAM MSC node body
> > +/// as described document [1], table 4 /// typedef struct {
> > +  UINT16    Length;
> > +  UINT8     InterfaceType;
> > +  UINT8     Reserved;
> > +  UINT32    Identifier;
> > +  UINT64    BaseAddress;
> > +  UINT32    MmioSize;
> > +  UINT32    OverflowInterrupt;
> > +  UINT32    OverflowInterruptFlags;
> > +  UINT32    Reserved1;
> > +  UINT32    OverflowInterruptAffinity;
> > +  UINT32    ErrorInterrupt;
> > +  UINT32    ErrorInterruptFlags;
> > +  UINT32    Reserved2;
> > +  UINT32    ErrorInterruptAffinity;
> > +  UINT32    MaxNrdyUsec;
> > +  UINT64    HIDLinkedDevice;
> [SAMI] I think the above field name should be HidLinkedDevice and similarly
> the following field name should be InstanceIdLinkedDevice.
> Otherwise I suspect the CI would complain. Can you check, please?
> > +  UINT32    InstanceIDLinkedDevice;
> > +  UINT32    NumResources;
> > +} EFI_ACPI_MPAM_MSC_NODE;
> > +
> > +///
> > +/// MPAM MSC resource
> > +/// as described in document [1], table 9 /// typedef struct {
> > +  UINT32                   Identifier;
> > +  UINT8                    RisIndex;
> > +  UINT16                   Reserved1;
> > +  UINT8                    LocatorType;
> > +  EFI_ACPI_MPAM_LOCATOR    Locator;
> > +  UINT32                   NumDependencies;
> > +} EFI_ACPI_MPAM_MSC_RESOURCE;
> > +
> > +///
> > +/// MPAM Function dependency descriptor /// as described in document
> > +[1], table 10 /// typedef struct {
> > +  UINT32    Producer;
> > +  UINT32    Reserved;
> > +} EFI_ACPI_MPAM_FUNCTIONAL_DEPENDENCY_DESCRIPTOR;
> > +
> > +#pragma pack()
> > +
> > +#endif