> On Aug 19, 2022, at 1:26 AM, Rohit Mathew <rohit.mat...@arm.com> wrote:
>
> Hi Swatisri,
>
> Thanks for the patch. Please find my comments inline marked [Rohit] -
>
>> -----Original Message-----
>> From: devel@edk2.groups.io <mailto:devel@edk2.groups.io>
>> <devel@edk2.groups.io <mailto:devel@edk2.groups.io>> On Behalf Of Name
>> via groups.io <http://groups.io/>
>> Sent: 16 August 2022 21:18
>> To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Sami Mujawar
>> <sami.muja...@arm.com <mailto:sami.muja...@arm.com>>;
>> Alexei Fedorov <alexei.fedo...@arm.com <mailto:alexei.fedo...@arm.com>>;
>> michael.d.kin...@intel.com <mailto:michael.d.kin...@intel.com>;
>> gaolim...@byosoft.com.cn <mailto:gaolim...@byosoft.com.cn>;
>> zhiguang....@intel.com <mailto:zhiguang....@intel.com>
>> Cc: Swatisri Kantamsetti <swatis...@nvidia.com <mailto:swatis...@nvidia.com>>
>> Subject: [edk2-devel] [PATCH 1/2] Mde Pkg: Support for MPAM ACPI Table
>>
>> From: Swatisri Kantamsetti <swatis...@nvidia.com>
>>
>> Added MPAM table header, MSC and Resource Node info structures
>>
>> Signed-off-by: Swatisri Kantamsetti <swatis...@nvidia.com>
>> ---
>> 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
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#92728): https://edk2.groups.io/g/devel/message/92728
Mute This Topic: https://groups.io/mt/93069490/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
