From: "Sami Mujawar" <sami.mujawar@arm.com>
To: pierre.gondois@arm.com, devel@edk2.groups.io
Cc: Alexei Fedorov <Alexei.Fedorov@arm.com>, jbrasen@nvidia.com
Subject: Re: [PATCH 1/1] DynamicTablesPkg/SsdtCpuTopology: Allow multi-packages topologies
Date: Tue, 25 Apr 2023 11:33:43 +0100 [thread overview]
Message-ID: <6fa3cc6f-77fb-3561-5332-0c469331da69@arm.com> (raw)
In-Reply-To: <20230309153249.1105184-1-pierre.gondois@arm.com>
Hi Pierre,
Thank you for this patch.
These changes look good to me, other than the change-id in the commit
message (which I will drop before merging the change).
Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
Regards,
Sami Mujawar
On 09/03/2023 03:32 pm, pierre.gondois@arm.com wrote:
> From: Pierre Gondois <pierre.gondois@arm.com>
>
> The topology of a platform is represented in ACPI using the PPTT
> table. It is possible to append information to CPUs/processor
> containers using their associated AML nodes in a SSDT
> table.
> A platform might have multiple 'physical packages' (or top-level
> nodes) in their PPTT topology representation. It can be assumed
> from [1] that a 'physical packages' is always a 'top-level node',
> and conversely.
>
> The SSDT topology generator doesn't support having multiple top-level
> nodes. The top-level node is also not generated in the SSDT topology
> representation.
> Add support to generate multiple top-level nodes in the SSDT topology
> generator and generate an AML node for this top-level node. This will
> allow to have matching PPTT and SSDT topology representations. Prior
> to this patch, this top-level AML node was not generated.
>
> Also factorize the flag checking in CheckProcNode() and add more
> checks.
>
> This patch takes inspiration from the discussion at:
> - v1: https://edk2.groups.io/g/devel/message/99410
> - v2: https://edk2.groups.io/g/devel/message/99615
>
> [1]
> ACPI 6.5, 5.2.30.1 Processor hierarchy node structure (Type 0):
> - "Multiple trees may be described, covering for example multiple
> packages. For the root of a tree, the parent pointer should be 0.""
> - "Each valid processor must belong to exactly one package. That is,
> the leaf must itself be a physical package or have an ancestor
> marked as a physical package."
>
> Change-Id: I48452e623906628f44b7e2c69a34ed7b30276e92
> Suggested-by: Jeff Brasen <jbrasen@nvidia.com>
> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
> ---
> .../SsdtCpuTopologyGenerator.c | 131 +++++++++++-------
> .../SsdtCpuTopologyGenerator.h | 4 +
> 2 files changed, 84 insertions(+), 51 deletions(-)
>
> diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
> index c24da8ec71ad..6fb131b66482 100644
> --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
> +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
> @@ -805,6 +805,57 @@ CreateAmlProcessorContainer (
> return Status;
> }
>
> +/** Check flags and topology of a ProcNode.
> +
> + @param [in] NodeFlags Flags of the ProcNode to check.
> + @param [in] IsLeaf The ProcNode is a leaf.
> + @param [in] NodeToken NodeToken of the ProcNode.
> + @param [in] ParentNodeToken Parent NodeToken of the ProcNode.
> +
> + @retval EFI_SUCCESS Success.
> + @retval EFI_INVALID_PARAMETER Invalid parameter.
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +CheckProcNode (
> + UINT32 NodeFlags,
> + BOOLEAN IsLeaf,
> + CM_OBJECT_TOKEN NodeToken,
> + CM_OBJECT_TOKEN ParentNodeToken
> + )
> +{
> + BOOLEAN InvalidFlags;
> + BOOLEAN HasPhysicalPackageBit;
> + BOOLEAN IsTopLevelNode;
> +
> + HasPhysicalPackageBit = (NodeFlags & EFI_ACPI_6_3_PPTT_PACKAGE_PHYSICAL) ==
> + EFI_ACPI_6_3_PPTT_PACKAGE_PHYSICAL;
> + IsTopLevelNode = (ParentNodeToken == CM_NULL_TOKEN);
> +
> + // A top-level node is a Physical Package and conversely.
> + InvalidFlags = HasPhysicalPackageBit ^ IsTopLevelNode;
> +
> + // Check Leaf specific flags.
> + if (IsLeaf) {
> + InvalidFlags |= ((NodeFlags & PPTT_LEAF_MASK) != PPTT_LEAF_MASK);
> + } else {
> + InvalidFlags |= ((NodeFlags & PPTT_LEAF_MASK) != 0);
> + }
> +
> + if (InvalidFlags) {
> + DEBUG ((
> + DEBUG_ERROR,
> + "ERROR: SSDT-CPU-TOPOLOGY: Invalid flags for ProcNode: 0x%p.\n",
> + (VOID *)NodeToken
> + ));
> + ASSERT (0);
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + return EFI_SUCCESS;
> +}
> +
> /** Create an AML representation of the Cpu topology.
>
> A processor container is by extension any non-leave device in the cpu topology.
> @@ -814,7 +865,6 @@ CreateAmlProcessorContainer (
> Protocol Interface.
> @param [in] NodeToken Token of the CM_ARM_PROC_HIERARCHY_INFO
> currently handled.
> - Cannot be CM_NULL_TOKEN.
> @param [in] ParentNode Parent node to attach the created
> node to.
> @param [in,out] ProcContainerIndex Pointer to the current processor container
> @@ -838,6 +888,7 @@ CreateAmlCpuTopologyTree (
> EFI_STATUS Status;
> UINT32 Index;
> UINT32 CpuIndex;
> + UINT32 ProcContainerName;
> AML_OBJECT_NODE_HANDLE ProcContainerNode;
> UINT32 Uid;
> UINT16 Name;
> @@ -846,11 +897,11 @@ CreateAmlCpuTopologyTree (
> ASSERT (Generator->ProcNodeList != NULL);
> ASSERT (Generator->ProcNodeCount != 0);
> ASSERT (CfgMgrProtocol != NULL);
> - ASSERT (NodeToken != CM_NULL_TOKEN);
> ASSERT (ParentNode != NULL);
> ASSERT (ProcContainerIndex != NULL);
>
> - CpuIndex = 0;
> + CpuIndex = 0;
> + ProcContainerName = 0;
>
> for (Index = 0; Index < Generator->ProcNodeCount; Index++) {
> // Find the children of the CM_ARM_PROC_HIERARCHY_INFO
> @@ -859,16 +910,15 @@ CreateAmlCpuTopologyTree (
> // Only Cpus (leaf nodes in this tree) have a GicCToken.
> // Create a Cpu node.
> if (Generator->ProcNodeList[Index].GicCToken != CM_NULL_TOKEN) {
> - if ((Generator->ProcNodeList[Index].Flags & PPTT_PROCESSOR_MASK) !=
> - PPTT_CPU_PROCESSOR_MASK)
> - {
> - DEBUG ((
> - DEBUG_ERROR,
> - "ERROR: SSDT-CPU-TOPOLOGY: Invalid flags for cpu: 0x%x.\n",
> - Generator->ProcNodeList[Index].Flags
> - ));
> + Status = CheckProcNode (
> + Generator->ProcNodeList[Index].Flags,
> + TRUE,
> + Generator->ProcNodeList[Index].Token,
> + NodeToken
> + );
> + if (EFI_ERROR (Status)) {
> ASSERT (0);
> - return EFI_INVALID_PARAMETER;
> + return Status;
> }
>
> if (Generator->ProcNodeList[Index].OverrideNameUidEnabled) {
> @@ -893,24 +943,22 @@ CreateAmlCpuTopologyTree (
> } else {
> // If this is not a Cpu, then this is a processor container.
>
> - // Acpi processor Id for clusters is not handled.
> - if ((Generator->ProcNodeList[Index].Flags & PPTT_PROCESSOR_MASK) !=
> - PPTT_CLUSTER_PROCESSOR_MASK)
> - {
> - DEBUG ((
> - DEBUG_ERROR,
> - "ERROR: SSDT-CPU-TOPOLOGY: Invalid flags for cluster: 0x%x.\n",
> - Generator->ProcNodeList[Index].Flags
> - ));
> + Status = CheckProcNode (
> + Generator->ProcNodeList[Index].Flags,
> + FALSE,
> + Generator->ProcNodeList[Index].Token,
> + NodeToken
> + );
> + if (EFI_ERROR (Status)) {
> ASSERT (0);
> - return EFI_INVALID_PARAMETER;
> + return Status;
> }
>
> if (Generator->ProcNodeList[Index].OverrideNameUidEnabled) {
> Name = Generator->ProcNodeList[Index].OverrideName;
> Uid = Generator->ProcNodeList[Index].OverrideUid;
> } else {
> - Name = *ProcContainerIndex;
> + Name = ProcContainerName;
> Uid = *ProcContainerIndex;
> }
>
> @@ -933,6 +981,13 @@ CreateAmlCpuTopologyTree (
> (*ProcContainerIndex)++;
> CpuIndex = 0;
>
> + // And reset the cluster name whenever there is a package.
> + if (NodeToken == CM_NULL_TOKEN) {
> + ProcContainerName = 0;
> + } else {
> + ProcContainerName++;
> + }
> +
> // Recursively continue creating an AML tree.
> Status = CreateAmlCpuTopologyTree (
> Generator,
> @@ -974,8 +1029,6 @@ CreateTopologyFromProcHierarchy (
> )
> {
> EFI_STATUS Status;
> - UINT32 Index;
> - UINT32 TopLevelProcNodeIndex;
> UINT32 ProcContainerIndex;
>
> ASSERT (Generator != NULL);
> @@ -984,8 +1037,7 @@ CreateTopologyFromProcHierarchy (
> ASSERT (CfgMgrProtocol != NULL);
> ASSERT (ScopeNode != NULL);
>
> - TopLevelProcNodeIndex = MAX_UINT32;
> - ProcContainerIndex = 0;
> + ProcContainerIndex = 0;
>
> Status = TokenTableInitialize (Generator, Generator->ProcNodeCount);
> if (EFI_ERROR (Status)) {
> @@ -993,33 +1045,10 @@ CreateTopologyFromProcHierarchy (
> return Status;
> }
>
> - // It is assumed that there is one unique CM_ARM_PROC_HIERARCHY_INFO
> - // structure with no ParentToken and the EFI_ACPI_6_3_PPTT_PACKAGE_PHYSICAL
> - // flag set. All other CM_ARM_PROC_HIERARCHY_INFO are non-physical and
> - // have a ParentToken.
> - for (Index = 0; Index < Generator->ProcNodeCount; Index++) {
> - if ((Generator->ProcNodeList[Index].ParentToken == CM_NULL_TOKEN) &&
> - (Generator->ProcNodeList[Index].Flags &
> - EFI_ACPI_6_3_PPTT_PACKAGE_PHYSICAL))
> - {
> - if (TopLevelProcNodeIndex != MAX_UINT32) {
> - DEBUG ((
> - DEBUG_ERROR,
> - "ERROR: SSDT-CPU-TOPOLOGY: Top level CM_ARM_PROC_HIERARCHY_INFO "
> - "must be unique\n"
> - ));
> - ASSERT (0);
> - goto exit_handler;
> - }
> -
> - TopLevelProcNodeIndex = Index;
> - }
> - } // for
> -
> Status = CreateAmlCpuTopologyTree (
> Generator,
> CfgMgrProtocol,
> - Generator->ProcNodeList[TopLevelProcNodeIndex].Token,
> + CM_NULL_TOKEN,
> ScopeNode,
> &ProcContainerIndex
> );
> diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.h b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.h
> index f174d9c2e2cb..48e4455490e9 100644
> --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.h
> +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.h
> @@ -34,6 +34,10 @@
> (EFI_ACPI_6_3_PPTT_PROCESSOR_ID_INVALID << 1) | \
> (EFI_ACPI_6_3_PPTT_NODE_IS_NOT_LEAF << 3))
>
> +// Leaf nodes specific mask.
> +#define PPTT_LEAF_MASK ((EFI_ACPI_6_3_PPTT_PROCESSOR_ID_VALID << 1) | \
> + (EFI_ACPI_6_3_PPTT_NODE_IS_LEAF << 3))
> +
> /** LPI states are stored in the ASL namespace at '\_SB_.Lxxx',
> with xxx being the node index of the LPI state.
> */
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
prev parent reply other threads:[~2023-04-25 10:34 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-09 15:32 [PATCH 1/1] DynamicTablesPkg/SsdtCpuTopology: Allow multi-packages topologies PierreGondois
2023-03-10 17:22 ` Jeff Brasen
2023-04-25 10:33 ` 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=6fa3cc6f-77fb-3561-5332-0c469331da69@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