On 2/2/23 18:53, Jeff Brasen wrote:
There are some cases (for example the _PSL list in thermal zones) where we need
to have a reference to the node and we have been doing that via an Extern and a
reference to the node path. I am push a patch where the effectively the PCD I
added was fixed true but was unsure if that would have unexpected issues with
other vendors platforms
The current SsdtCpuTopologyGenerator doesn't generate an AML node for the top
level package. Even though this seem compliant to the ACPI spec, this induces a
difference between the ASL topology description and the PPTT topology
description. For instance, for the Juno, the topology generated for the ACPI
tables are:
PPTT:
(PACKAGE)
\-Little Cluster
\-CPU[0,3-5]
\-Big Cluster
\-CPU[1-2]
SSDT:
Little Cluster
\-CPU[0,3-5]
Big Cluster
\-CPU[1-2]
To solve your issue, to have matching topology descriptions, and after
discussing with Sami, it would be better to have:
SSDT:
(PACKAGE)
\-Little Cluster
\-CPU[0,3-5]
\-Big Cluster
\-CPU[1-2]
The Juno is the only platform that publicly uses the SsdtCpuTopologyGenerator,
so I am not sure how other platforms support should be handled.
About the code itself, I think the ProcContainerIndex should also be reset in
CreateAmlCpuTopologyTree() when generating a new level of containers (if it is
decided to go this way).
Regards,
Pierre
-Jeff
-----Original Message-----
From: Pierre Gondois <pierre.gond...@arm.com>
Sent: Thursday, February 2, 2023 10:49 AM
To: Jeff Brasen <jbra...@nvidia.com>; devel@edk2.groups.io
Cc: sami.muja...@arm.com; alexei.fedo...@arm.com;
quic_llind...@quicinc.com; ardb+tianoc...@kernel.org
Subject: Re: [PATCH] DynamicTablesPkg: Allow multiple top level physical
nodes
External email: Use caution opening links or attachments
Hello Jeff,
I was assuming that no other module would rely on the AML path to access
an AML node and that nodes should be retrieved through their
characteristics instead, i.e. internal properties/Name/Uid.
There are currently no public API allowing to do so, but there are internal
APIs that could be relied on to create them.
I'm not sure what Sami is thinking,
Regards,
Pierre
On 2/2/23 17:48, Jeff Brasen wrote:
Just to clarify you are suggesting that all CPU nodes generated via
this with have an outer processor container? I am fine with that but
was concerned with a change in behavior to other platforms in case
they are expecting the CPUs to just be under \SB.C00x instead of
\SB.C000.C00x
-Jeff
-----Original Message-----
From: Pierre Gondois <pierre.gond...@arm.com>
Sent: Thursday, February 2, 2023 5:03 AM
To: Jeff Brasen <jbra...@nvidia.com>; devel@edk2.groups.io
Cc: sami.muja...@arm.com; alexei.fedo...@arm.com;
quic_llind...@quicinc.com; ardb+tianoc...@kernel.org
Subject: Re: [PATCH] DynamicTablesPkg: Allow multiple top level
physical nodes
External email: Use caution opening links or attachments
Hello Jeff,
I think it's ok to make this the generic case and remove the Pcd to enable
it.
Cf 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."
and
"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."
so this original comment is incorrect:
"""
// 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.
"""
On 2/1/23 17:42, Jeff Brasen wrote:
In SSDT CPU topology generator allow for multiple top level physical
nodes as would be seen with a multi-socket system. This will be auto
detected if there are more then one physical device and there is a
new PCD to enable forcing of a top level processor container to
allow for consistency for systems that can be either single or multi
socket.
Signed-off-by: Jeff Brasen <jbra...@nvidia.com>
---
DynamicTablesPkg/DynamicTablesPkg.dec | 3 +
.../SsdtCpuTopologyGenerator.c | 66 ++++++++++---------
.../SsdtCpuTopologyLibArm.inf | 4 ++
3 files changed, 41 insertions(+), 32 deletions(-)
diff --git a/DynamicTablesPkg/DynamicTablesPkg.dec
b/DynamicTablesPkg/DynamicTablesPkg.dec
index adc2e67cbf..a061b70322 100644
--- a/DynamicTablesPkg/DynamicTablesPkg.dec
+++ b/DynamicTablesPkg/DynamicTablesPkg.dec
@@ -63,5 +63,8 @@
# Use PCI segment numbers as UID
gEdkiiDynamicTablesPkgTokenSpaceGuid.PcdPciUseSegmentAsUid|FALSE|B
OOLE
AN|0x40000009
+ # Force top level container for single socket devices
+
gEdkiiDynamicTablesPkgTokenSpaceGuid.PcdForceTopLevelProcessorContai
+ ner|FALSE|BOOLEAN|0x4000000A
+
[Guids]
gEdkiiDynamicTablesPkgTokenSpaceGuid = { 0xab226e66, 0x31d8,
0x4613, { 0x87, 0x9d, 0xd2, 0xfa, 0xb6, 0x10, 0x26, 0x3c } } diff
--git
a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCp
uT
opologyGenerator.c
b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCp
uT
opologyGenerator.c
index c24da8ec71..58f86ff508 100644
---
a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCp
uT
opologyGenerator.c
+++
b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/Ssdt
+++ CpuTopologyGenerator.c
@@ -22,6 +22,7 @@
#include <Library/AcpiHelperLib.h>
#include <Library/TableHelperLib.h>
#include <Library/AmlLib/AmlLib.h>
+#include <Library/PcdLib.h>
#include <Protocol/ConfigurationManagerProtocol.h>
#include "SsdtCpuTopologyGenerator.h"
@@ -814,7 +815,8 @@ CreateAmlProcessorContainer (
Protocol Interface.
@param [in] NodeToken Token of the
CM_ARM_PROC_HIERARCHY_INFO
currently handled.
- Cannot be CM_NULL_TOKEN.
+ CM_NULL_TOKEN if top level container
+ should be created.
@param [in] ParentNode Parent node to attach the created
node to.
@param [in,out] ProcContainerIndex Pointer to the current
processor container @@ -841,12 +843,12 @@
CreateAmlCpuTopologyTree
(
AML_OBJECT_NODE_HANDLE ProcContainerNode;
UINT32 Uid;
UINT16 Name;
+ UINT32 NodeFlags;
ASSERT (Generator != NULL);
ASSERT (Generator->ProcNodeList != NULL);
ASSERT (Generator->ProcNodeCount != 0);
ASSERT (CfgMgrProtocol != NULL);
- ASSERT (NodeToken != CM_NULL_TOKEN);
ASSERT (ParentNode != NULL);
ASSERT (ProcContainerIndex != NULL);
@@ -893,8 +895,14 @@ CreateAmlCpuTopologyTree (
} else {
// If this is not a Cpu, then this is a processor container.
+ NodeFlags = Generator->ProcNodeList[Index].Flags;
+ // Allow physical property for top level nodes
+ if (NodeToken == CM_NULL_TOKEN) {
+ NodeFlags &= ~EFI_ACPI_6_3_PPTT_PACKAGE_PHYSICAL;
+ }
+
Even though it was never encountered so far, it should also be
possible to have a physical package consisting of only one CPU. So I
guess it would be better to create a function to check the flags,
whether the ProcNode is a CPU or a cluster.
I attached a Wip patch base on your work where such function is created.
Feel free to take it/modify it at your will.
// Acpi processor Id for clusters is not handled.
- if ((Generator->ProcNodeList[Index].Flags &
PPTT_PROCESSOR_MASK) !=
+ if ((NodeFlags & PPTT_PROCESSOR_MASK) !=
PPTT_CLUSTER_PROCESSOR_MASK)
{
DEBUG ((
@@ -973,10 +981,10 @@ CreateTopologyFromProcHierarchy (
IN AML_OBJECT_NODE_HANDLE ScopeNode
)
{
- EFI_STATUS Status;
- UINT32 Index;
- UINT32 TopLevelProcNodeIndex;
- UINT32 ProcContainerIndex;
+ EFI_STATUS Status;
+ UINT32 Index;
+ CM_OBJECT_TOKEN TopLevelToken;
+ UINT32 ProcContainerIndex;
ASSERT (Generator != NULL);
ASSERT (Generator->ProcNodeCount != 0); @@ -984,8 +992,8 @@
CreateTopologyFromProcHierarchy (
ASSERT (CfgMgrProtocol != NULL);
ASSERT (ScopeNode != NULL);
- TopLevelProcNodeIndex = MAX_UINT32;
- ProcContainerIndex = 0;
+ TopLevelToken = CM_NULL_TOKEN;
+ ProcContainerIndex = 0;
Status = TokenTableInitialize (Generator, Generator-
ProcNodeCount);
if (EFI_ERROR (Status)) {
@@ -993,33 +1001,27 @@ 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;
- }
+ if (!PcdGetBool (PcdForceTopLevelProcessorContainer)) {
+ 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))
+ {
+ // Multi-socket detected, using top level containers
+ if (TopLevelToken != CM_NULL_TOKEN) {
+ TopLevelToken = CM_NULL_TOKEN;
+ break;
+ }
- TopLevelProcNodeIndex = Index;
- }
- } // for
+ TopLevelToken = Generator->ProcNodeList[Index].Token;
+ }
+ } // for
+ }
Status = CreateAmlCpuTopologyTree (
Generator,
CfgMgrProtocol,
- Generator->ProcNodeList[TopLevelProcNodeIndex].Token,
+ TopLevelToken,
ScopeNode,
&ProcContainerIndex
);
@@ -1106,7 +1108,7 @@ CreateTopologyFromGicC (
break;
}
}
- } // for
+ } // for
Is it possible to remove this change ?
return Status;
}
diff --git
a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCp
uT
opologyLibArm.inf
b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCp
uT
opologyLibArm.inf
index 3e2d154749..00adfe986f 100644
---
a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCp
uT
opologyLibArm.inf
+++
b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/Ssdt
+++ CpuTopologyLibArm.inf
@@ -31,3 +31,7 @@
AcpiHelperLib
AmlLib
BaseLib
+ PcdLib
+
+[Pcd]
+
+gEdkiiDynamicTablesPkgTokenSpaceGuid.PcdForceTopLevelProcessorConta
in
+er
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99582): https://edk2.groups.io/g/devel/message/99582
Mute This Topic: https://groups.io/mt/96680589/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
