Hi Pierre,

> -----Original Message-----
> From: Pierre Gondois <pierre.gond...@arm.com>
> Sent: Monday, July 24, 2023 3:40 PM
> To: Rohit Mathew <rohit.mat...@arm.com>; devel@edk2.groups.io
> Cc: Michael D Kinney <michael.d.kin...@intel.com>; Liming Gao
> <gaolim...@byosoft.com.cn>; Thomas Abraham
> <thomas.abra...@arm.com>; Sami Mujawar <sami.muja...@arm.com>;
> James Morse <james.mo...@arm.com>; nd <n...@arm.com>
> Subject: Re: [edk2-devel] [PATCH V3 1/3] MdePkg/IndustryStandard: Add
> definitions for MPAM ACPI specification
> 
> 
> 
> On 7/24/23 11:50, Rohit Mathew wrote:
> > Hi Pierre,
> >
> > Thank you for the review. Please find my response inline.
> >
> >>
> >> Hello Rohit,
> >>
> >> On 5/22/23 16:44, Rohit Mathew via groups.io wrote:
> >>> From: Rohit Mathew <rohit.mat...@arm.com>
> >>>
> >>> Add definitions, macros and types for elements associated with MPAM
> >>> ACPI 2.0 specification.
> >>>
> >>> Signed-off-by: Rohit Mathew <rohit.mat...@arm.com>
> >>> ---
> >>>    MdePkg/Include/IndustryStandard/Acpi65.h |   7 +-
> >>>    MdePkg/Include/IndustryStandard/Mpam.h   | 260
> >> ++++++++++++++++++++
> >>>    2 files changed, 266 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/MdePkg/Include/IndustryStandard/Acpi65.h
> >>> b/MdePkg/Include/IndustryStandard/Acpi65.h
> >>> index 1e41ae9a27..8a1d3d125a 100644
> >>> --- a/MdePkg/Include/IndustryStandard/Acpi65.h
> >>> +++ b/MdePkg/Include/IndustryStandard/Acpi65.h
> >>> @@ -2,7 +2,7 @@
> >>>      ACPI 6.5 definitions from the ACPI Specification Revision 6.5 Aug,
> 2022.
> >>>
> >>>      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 Ltd. All rights reserved.<BR>
> >>>      Copyright (c) 2023, Loongson Technology Corporation Limited.
> >>> All rights reserved.<BR>
> >>>
> >>>      SPDX-License-Identifier: BSD-2-Clause-Patent @@ -3251,6
> >>> +3251,11 @@ typedef struct {
> >>>    ///
> >>>    #define EFI_ACPI_6_5_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_MONITORIN
> >> G_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..30b69c869e
> >>> --- /dev/null
> >>> +++ b/MdePkg/Include/IndustryStandard/Mpam.h
> >>> @@ -0,0 +1,260 @@
> >>> +/** @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_MONITORIN
> >> G_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_INTERRUPT_PROCESSOR_AFFINITY            (0x0)
> >>> +#define
> EFI_ACPI_MPAM_INTERRUPT_PROCESSOR_CONTAINER_AFFINITY
> >> (0x1)
> >>> +
> >>> +///
> >>> +/// MPAM MSC affinity valid
> >>> +///
> >>> +#define EFI_ACPI_MPAM_INTERRUPT_AFFINITY_NOT_VALID  (0x0)
> >>> +#define EFI_ACPI_MPAM_INTERRUPT_AFFINITY_VALID      (0x1)
> >>> +
> >>> +///
> >>> +/// MPAM Interrupt flag - bit positions ///
> >>> +#define EFI_ACPI_MPAM_INTERRUPT_MODE_SHIFT            (0)
> >>> +#define EFI_ACPI_MPAM_INTERRUPT_TYPE_SHIFT            (1)
> >>> +#define EFI_ACPI_MPAM_INTERRUPT_AFFINITY_TYPE_SHIFT   (3)
> >>> +#define EFI_ACPI_MPAM_INTERRUPT_AFFINITY_VALID_SHIFT  (4)
> >>> +#define EFI_ACPI_MPAM_INTERRUPT_RESERVED_SHIFT        (5)
> >>> +
> >>> +///
> >>> +/// MPAM Interrupt flag - bit masks ///
> >>> +#define EFI_ACPI_MPAM_INTERRUPT_MODE_MASK            (0x1)
> >>> +#define EFI_ACPI_MPAM_INTERRUPT_TYPE_MASK            (0x3)
> >>> +#define EFI_ACPI_MPAM_INTERRUPT_AFFINITY_TYPE_MASK   (0x8)
> >>> +#define EFI_ACPI_MPAM_INTERRUPT_AFFINITY_VALID_MASK  (0x10)
> >>> +#define EFI_ACPI_MPAM_INTERRUPT_RESERVED_MASK
> (0xFFFFFFE0)
> >>> +
> >>
> >> As there is a structure 'EFI_ACPI_MPAM_MEMORY_CACHE_LOCATOR' to
> >> access the desired fields, and as the reserved/level fields are not
> >> bitfields, I don't think this is necessary to have these definitions:
> >
> > [Rohit] MPAM parser does make use of these masks/shifts to segregate
> reserved and level (as they are treated as part of the same 8 byte object). I
> could move these to the MpamParser.h if we don't have use-case for them
> elsewhere?
> 
> If anyone see any use of the definitions we can keep them, but I m not sure I
> see how they could be used, so I personally think they could be removed,
> 

In that case, I'll move it out of this file. We could decide on keeping it or 
not within MpamParser.h based on how the "ACPI_PARSER for locator" discussion 
goes.

Regards,
Rohit 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107175): https://edk2.groups.io/g/devel/message/107175
Mute This Topic: https://groups.io/mt/99066167/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-


Reply via email to