From: "Sami Mujawar" <sami.mujawar@arm.com>
To: Swatisri Kantamsetti <swatisrik@nvidia.com>,
Name <username@nvidia.com>,
devel@edk2.groups.io, Alexei.Fedorov@arm.com,
michael.d.kinney@intel.com, gaoliming@byosoft.com.cn,
zhiguang.liu@intel.com
Cc: "nd@arm.com" <nd@arm.com>, Rohit Mathew <rohit.mathew@arm.com>,
Thanu.Rangarajan@arm.com
Subject: Re: [PATCH 1/2] Mde Pkg: Support for MPAM ACPI Table
Date: Mon, 3 Oct 2022 12:17:28 +0100 [thread overview]
Message-ID: <1c2b4e97-babb-988b-1fff-78d1ba20eb59@arm.com> (raw)
In-Reply-To: <7f8a5c9bbdf1a1f01c6fc822fa298067d280079a.1660667637.git.swatisrik@nvidia.com>
[-- Attachment #1: Type: text/plain, Size: 6039 bytes --]
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@nvidia.com>
>
> Added MPAM table header, MSC and Resource Node
> info structures
>
> Signed-off-by: Swatisri Kantamsetti<swatisrik@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_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
[-- Attachment #2: Type: text/html, Size: 13740 bytes --]
prev parent reply other threads:[~2022-10-03 11:17 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-16 20:18 [PATCH 1/2] Mde Pkg: Support for MPAM ACPI Table Name
2022-08-16 20:18 ` [PATCH 2/2] Dynamic Tbl Mgr: MPAM: MPAM Generator and supporting files Name
2022-08-19 8:26 ` [edk2-devel] " Rohit Mathew
2022-08-24 10:51 ` Rohit Mathew
2022-08-23 17:51 ` Swatisri Kantamsetti
2022-08-19 8:26 ` [edk2-devel] [PATCH 1/2] Mde Pkg: Support for MPAM ACPI Table Rohit Mathew
2022-08-23 20:10 ` Andrew Fish
2022-08-23 21:28 ` Rohit Mathew
2022-08-23 22:59 ` Andrew Fish
2022-08-24 10:42 ` Rohit Mathew
2022-08-24 14:39 ` hesham.almatary
2022-08-24 16:25 ` Rohit Mathew
2022-08-25 9:36 ` hesham.almatary
2022-08-23 17:52 ` Swatisri Kantamsetti
2022-10-03 11:17 ` Sami Mujawar [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1c2b4e97-babb-988b-1fff-78d1ba20eb59@arm.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox