[edk2-devel] [PATCH 1/2] Mde Pkg: Support for MPAM ACPI Table
Andrew Fish via groups.io
afish=apple.com at groups.io
Tue Aug 23 22:59:41 UTC 2022
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 at arm.com> wrote:
>
> Hi Andrew,
>
> From: Andrew Fish <afish at apple.com <mailto:afish at apple.com>>
> Sent: 23 August 2022 21:11
> To: devel at edk2.groups.io <mailto:devel at edk2.groups.io>; Rohit Mathew <Rohit.Mathew at arm.com <mailto:Rohit.Mathew at arm.com>>
> Cc: username at nvidia.com <mailto:username at nvidia.com>; Sami Mujawar <Sami.Mujawar at arm.com <mailto:Sami.Mujawar at arm.com>>; Alexei Fedorov <Alexei.Fedorov at arm.com <mailto:Alexei.Fedorov at arm.com>>; Mike Kinney <michael.d.kinney at intel.com <mailto:michael.d.kinney at intel.com>>; gaoliming at byosoft.com.cn <mailto:gaoliming at byosoft.com.cn>; zhiguang.liu at intel.com <mailto:zhiguang.liu at intel.com>; Swatisri Kantamsetti <swatisrik at nvidia.com <mailto:swatisrik at nvidia.com>>; Thomas Abraham <thomas.abraham at arm.com <mailto:thomas.abraham at arm.com>>; Thanu Rangarajan <Thanu.Rangarajan at arm.com <mailto:Thanu.Rangarajan at arm.com>>; nd <nd at arm.com <mailto:nd at arm.com>>
> 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 at arm.com <mailto:rohit.mathew at arm.com>> wrote:
>
> Hi Swatisri,
>
> Thanks for the patch. Please find my comments inline marked [Rohit] -
>
>
> -----Original Message-----
> From: devel at edk2.groups.io <mailto:devel at edk2.groups.io> <devel at edk2.groups.io <mailto:devel at edk2.groups.io>> On Behalf Of Name
> via groups.io <http://groups.io/>
> Sent: 16 August 2022 21:18
> To: devel at edk2.groups.io <mailto:devel at edk2.groups.io>; Sami Mujawar <Sami.Mujawar at arm.com <mailto:Sami.Mujawar at arm.com>>;
> Alexei Fedorov <Alexei.Fedorov at arm.com <mailto:Alexei.Fedorov at arm.com>>; michael.d.kinney at intel.com <mailto:michael.d.kinney at intel.com>;
> gaoliming at byosoft.com.cn <mailto:gaoliming at byosoft.com.cn>; zhiguang.liu at intel.com <mailto:zhiguang.liu at intel.com>
> Cc: Swatisri Kantamsetti <swatisrik at nvidia.com <mailto:swatisrik at nvidia.com>>
> Subject: [edk2-devel] [PATCH 1/2] Mde Pkg: Support for MPAM ACPI Table
>
> From: Swatisri Kantamsetti <swatisrik at nvidia.com <mailto:swatisrik at nvidia.com>>
>
> Added MPAM table header, MSC and Resource Node info structures
>
> Signed-off-by: Swatisri Kantamsetti <swatisrik at nvidia.com <mailto:swatisrik at 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.
>
> [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
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#92730): https://edk2.groups.io/g/devel/message/92730
Mute This Topic: https://groups.io/mt/93069490/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20220823/567d48aa/attachment-0001.htm>
More information about the edk2-devel-archive
mailing list