On 2/3/23 17:38, Jeff Brasen wrote:
To solve that problem I had added support for allowing the UID/Name to come
from the node
https://github.com/tianocore/edk2/commit/5fb3f5723a1ea9d9a93e317181e1c11468a9eb45
Yes right. However if the UID/Name were to be generated, the topology could be
misleading, cf the example below where package/cluster names are incremented
even though they are not on the same level.
-----Original Message-----
From: Pierre Gondois <pierre.gond...@arm.com>
Sent: Friday, February 3, 2023 9:28 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
On 2/3/23 17:00, Jeff Brasen wrote:
I'll on an updated patch this morning that only does the new behavior. We
can't reset the procindex as it is used for the _UID as well and we would end up
with the same value in two nodes.
Yes indeed, then maybe the name/uid selection should not be done in
CreateAmlCpuTopologyTree() but in
CreateAmlProcessorContainer()/CreateAmlCpuFromProcHierarchy().
This would allow to have a static counter for the Uid in
CreateAmlProcessorContainer() and always have incrementing names for
packages/cluster. Otherwise the generated name will be:
C000 <- Package
\-C0001 <- Cluster
\-C0000 <- CPU
C002 <- second Package
\-C0003 <- second Cluster
\-C0001 <- second CPU
instead of:
C000 <- Package
\-C0001 <- Cluster
\-C0000 <- CPU
C001 <- second Package
\-C0000 <- second Cluster
\-C0001 <- second CPU
Regards,
Pierre
-Jeff
-----Original Message-----
From: Pierre Gondois <pierre.gond...@arm.com>
Sent: Friday, February 3, 2023 6:11 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
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.PcdForceTopLevelProcessorConta
i
+ 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/SsdtC
p
uT
opologyGenerator.c
b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtC
p
uT
opologyGenerator.c
index c24da8ec71..58f86ff508 100644
---
a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtC
p
uT
opologyGenerator.c
+++
b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/Ssd
t
+++ 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/SsdtC
p
uT
opologyLibArm.inf
b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtC
p
uT
opologyLibArm.inf
index 3e2d154749..00adfe986f 100644
---
a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtC
p
uT
opologyLibArm.inf
+++
b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/Ssd
t
+++ CpuTopologyLibArm.inf
@@ -31,3 +31,7 @@
AcpiHelperLib
AmlLib
BaseLib
+ PcdLib
+
+[Pcd]
+
+gEdkiiDynamicTablesPkgTokenSpaceGuid.PcdForceTopLevelProcessorCont
+a
in
+er
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99654): https://edk2.groups.io/g/devel/message/99654
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]
-=-=-=-=-=-=-=-=-=-=-=-
