* [PATCH] DynamicTablesPkg: Allow multiple top level physical nodes
@ 2023-02-01 16:42 Jeff Brasen
2023-02-02 12:02 ` PierreGondois
0 siblings, 1 reply; 10+ messages in thread
From: Jeff Brasen @ 2023-02-01 16:42 UTC (permalink / raw)
To: devel
Cc: Sami.Mujawar, Alexei.Fedorov, pierre.gondois, quic_llindhol,
ardb+tianocore, Jeff Brasen
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 <jbrasen@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|BOOLEAN|0x40000009
+ # Force top level container for single socket devices
+ gEdkiiDynamicTablesPkgTokenSpaceGuid.PcdForceTopLevelProcessorContainer|FALSE|BOOLEAN|0x4000000A
+
[Guids]
gEdkiiDynamicTablesPkgTokenSpaceGuid = { 0xab226e66, 0x31d8, 0x4613, { 0x87, 0x9d, 0xd2, 0xfa, 0xb6, 0x10, 0x26, 0x3c } }
diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
index c24da8ec71..58f86ff508 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.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;
+ }
+
// 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
return Status;
}
diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyLibArm.inf b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyLibArm.inf
index 3e2d154749..00adfe986f 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyLibArm.inf
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyLibArm.inf
@@ -31,3 +31,7 @@
AcpiHelperLib
AmlLib
BaseLib
+ PcdLib
+
+[Pcd]
+ gEdkiiDynamicTablesPkgTokenSpaceGuid.PcdForceTopLevelProcessorContainer
--
2.25.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] DynamicTablesPkg: Allow multiple top level physical nodes
2023-02-01 16:42 [PATCH] DynamicTablesPkg: Allow multiple top level physical nodes Jeff Brasen
@ 2023-02-02 12:02 ` PierreGondois
2023-02-02 16:48 ` Jeff Brasen
0 siblings, 1 reply; 10+ messages in thread
From: PierreGondois @ 2023-02-02 12:02 UTC (permalink / raw)
To: Jeff Brasen, devel
Cc: Sami.Mujawar, Alexei.Fedorov, quic_llindhol, ardb+tianocore
[-- Attachment #1: Type: text/plain, Size: 8551 bytes --]
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 <jbrasen@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|BOOLEAN|0x40000009
>
> + # Force top level container for single socket devices
> + gEdkiiDynamicTablesPkgTokenSpaceGuid.PcdForceTopLevelProcessorContainer|FALSE|BOOLEAN|0x4000000A
> +
> [Guids]
> gEdkiiDynamicTablesPkgTokenSpaceGuid = { 0xab226e66, 0x31d8, 0x4613, { 0x87, 0x9d, 0xd2, 0xfa, 0xb6, 0x10, 0x26, 0x3c } }
> diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
> index c24da8ec71..58f86ff508 100644
> --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
> +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.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/SsdtCpuTopologyLibArm.inf b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyLibArm.inf
> index 3e2d154749..00adfe986f 100644
> --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyLibArm.inf
> +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyLibArm.inf
> @@ -31,3 +31,7 @@
> AcpiHelperLib
> AmlLib
> BaseLib
> + PcdLib
> +
> +[Pcd]
> + gEdkiiDynamicTablesPkgTokenSpaceGuid.PcdForceTopLevelProcessorContainer
[-- Attachment #2: 0001-WIP-DynamicTablesPkg-SsdtCpuTopology-Allow-multi-pac.patch --]
[-- Type: text/x-patch, Size: 7467 bytes --]
From 78ef745a20ea9fe231ce5ff19f1c248711989045 Mon Sep 17 00:00:00 2001
From: Pierre Gondois <pierre.gondois@arm.com>
Date: Thu, 2 Feb 2023 12:53:20 +0100
Subject: [PATCH 1/1] WIP: DynamicTablesPkg/SsdtCpuTopology: Allow
multi-packages topologies
Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
---
CryptoPkg/Library/OpensslLib/openssl | 2 +-
.../SsdtCpuTopologyGenerator.c | 127 +++++++++++-------
2 files changed, 80 insertions(+), 49 deletions(-)
diff --git a/CryptoPkg/Library/OpensslLib/openssl b/CryptoPkg/Library/OpensslLib/openssl
index 129058165d19..d82e959e621a 160000
--- a/CryptoPkg/Library/OpensslLib/openssl
+++ b/CryptoPkg/Library/OpensslLib/openssl
@@ -1 +1 @@
-Subproject commit 129058165d195e43a0ad10111b0c2e29bdf65980
+Subproject commit d82e959e621a3d597f1e0d50ff8c2d8b96915fd7
diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
index c24da8ec71ad..8f3dd1defaa8 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
@@ -805,6 +805,67 @@ CreateAmlProcessorContainer (
return Status;
}
+/** Check flags and topology of a a ProcNode.
+
+ @param [in] NodeFlags Flags of the ProcNode to check.
+ @param [in] IsCpu The ProcNode is a CPU.
+ @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 IsCpu,
+ 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 must have the Physical Package bit set.
+ // Clear it once checked.
+ InvalidFlags = HasPhysicalPackageBit ^ IsTopLevelNode;
+ NodeFlags &= ~EFI_ACPI_6_3_PPTT_PACKAGE_PHYSICAL;
+
+ // Don't support topologies with more than 3 levels for now,
+ // i.e. deeper than PhysicalPackage -> Cluster -> CPU
+ InvalidFlags |= !IsTopLevelNode && !IsCpu &&
+ (NodeToken == CM_NULL_TOKEN);
+
+ // Now check CPU/Cluster specific flags.
+ if (IsCpu) {
+ InvalidFlags |= ((NodeFlags & PPTT_PROCESSOR_MASK) !=
+ PPTT_CPU_PROCESSOR_MASK);
+ } else {
+ InvalidFlags |= ((NodeFlags & PPTT_PROCESSOR_MASK) !=
+ PPTT_CLUSTER_PROCESSOR_MASK);
+ }
+
+ // Assume the top level node is a physical package.
+ 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.
@@ -846,7 +907,6 @@ CreateAmlCpuTopologyTree (
ASSERT (Generator->ProcNodeList != NULL);
ASSERT (Generator->ProcNodeCount != 0);
ASSERT (CfgMgrProtocol != NULL);
- ASSERT (NodeToken != CM_NULL_TOKEN);
ASSERT (ParentNode != NULL);
ASSERT (ProcContainerIndex != NULL);
@@ -859,16 +919,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,17 +952,15 @@ 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) {
@@ -974,8 +1031,6 @@ CreateTopologyFromProcHierarchy (
)
{
EFI_STATUS Status;
- UINT32 Index;
- UINT32 TopLevelProcNodeIndex;
UINT32 ProcContainerIndex;
ASSERT (Generator != NULL);
@@ -984,8 +1039,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 +1047,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
);
--
2.25.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] DynamicTablesPkg: Allow multiple top level physical nodes
2023-02-02 12:02 ` PierreGondois
@ 2023-02-02 16:48 ` Jeff Brasen
2023-02-02 17:48 ` PierreGondois
0 siblings, 1 reply; 10+ messages in thread
From: Jeff Brasen @ 2023-02-02 16:48 UTC (permalink / raw)
To: Pierre Gondois, devel@edk2.groups.io
Cc: Sami.Mujawar@arm.com, Alexei.Fedorov@arm.com,
quic_llindhol@quicinc.com, ardb+tianocore@kernel.org
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.gondois@arm.com>
> Sent: Thursday, February 2, 2023 5:03 AM
> To: Jeff Brasen <jbrasen@nvidia.com>; devel@edk2.groups.io
> Cc: Sami.Mujawar@arm.com; Alexei.Fedorov@arm.com;
> quic_llindhol@quicinc.com; ardb+tianocore@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 <jbrasen@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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] DynamicTablesPkg: Allow multiple top level physical nodes
2023-02-02 16:48 ` Jeff Brasen
@ 2023-02-02 17:48 ` PierreGondois
2023-02-02 17:53 ` Jeff Brasen
0 siblings, 1 reply; 10+ messages in thread
From: PierreGondois @ 2023-02-02 17:48 UTC (permalink / raw)
To: Jeff Brasen, devel@edk2.groups.io
Cc: Sami.Mujawar@arm.com, Alexei.Fedorov@arm.com,
quic_llindhol@quicinc.com, ardb+tianocore@kernel.org
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.gondois@arm.com>
>> Sent: Thursday, February 2, 2023 5:03 AM
>> To: Jeff Brasen <jbrasen@nvidia.com>; devel@edk2.groups.io
>> Cc: Sami.Mujawar@arm.com; Alexei.Fedorov@arm.com;
>> quic_llindhol@quicinc.com; ardb+tianocore@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 <jbrasen@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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] DynamicTablesPkg: Allow multiple top level physical nodes
2023-02-02 17:48 ` PierreGondois
@ 2023-02-02 17:53 ` Jeff Brasen
2023-02-03 13:11 ` PierreGondois
0 siblings, 1 reply; 10+ messages in thread
From: Jeff Brasen @ 2023-02-02 17:53 UTC (permalink / raw)
To: Pierre Gondois, devel@edk2.groups.io
Cc: Sami.Mujawar@arm.com, Alexei.Fedorov@arm.com,
quic_llindhol@quicinc.com, ardb+tianocore@kernel.org
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
-Jeff
> -----Original Message-----
> From: Pierre Gondois <pierre.gondois@arm.com>
> Sent: Thursday, February 2, 2023 10:49 AM
> To: Jeff Brasen <jbrasen@nvidia.com>; devel@edk2.groups.io
> Cc: Sami.Mujawar@arm.com; Alexei.Fedorov@arm.com;
> quic_llindhol@quicinc.com; ardb+tianocore@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.gondois@arm.com>
> >> Sent: Thursday, February 2, 2023 5:03 AM
> >> To: Jeff Brasen <jbrasen@nvidia.com>; devel@edk2.groups.io
> >> Cc: Sami.Mujawar@arm.com; Alexei.Fedorov@arm.com;
> >> quic_llindhol@quicinc.com; ardb+tianocore@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 <jbrasen@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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] DynamicTablesPkg: Allow multiple top level physical nodes
2023-02-02 17:53 ` Jeff Brasen
@ 2023-02-03 13:11 ` PierreGondois
2023-02-03 16:00 ` Jeff Brasen
0 siblings, 1 reply; 10+ messages in thread
From: PierreGondois @ 2023-02-03 13:11 UTC (permalink / raw)
To: Jeff Brasen, devel@edk2.groups.io
Cc: Sami.Mujawar@arm.com, Alexei.Fedorov@arm.com,
quic_llindhol@quicinc.com, ardb+tianocore@kernel.org
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.gondois@arm.com>
>> Sent: Thursday, February 2, 2023 10:49 AM
>> To: Jeff Brasen <jbrasen@nvidia.com>; devel@edk2.groups.io
>> Cc: Sami.Mujawar@arm.com; Alexei.Fedorov@arm.com;
>> quic_llindhol@quicinc.com; ardb+tianocore@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.gondois@arm.com>
>>>> Sent: Thursday, February 2, 2023 5:03 AM
>>>> To: Jeff Brasen <jbrasen@nvidia.com>; devel@edk2.groups.io
>>>> Cc: Sami.Mujawar@arm.com; Alexei.Fedorov@arm.com;
>>>> quic_llindhol@quicinc.com; ardb+tianocore@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 <jbrasen@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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] DynamicTablesPkg: Allow multiple top level physical nodes
2023-02-03 13:11 ` PierreGondois
@ 2023-02-03 16:00 ` Jeff Brasen
2023-02-03 16:28 ` PierreGondois
0 siblings, 1 reply; 10+ messages in thread
From: Jeff Brasen @ 2023-02-03 16:00 UTC (permalink / raw)
To: Pierre Gondois, devel@edk2.groups.io
Cc: Sami.Mujawar@arm.com, Alexei.Fedorov@arm.com,
quic_llindhol@quicinc.com, ardb+tianocore@kernel.org
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.
-Jeff
> -----Original Message-----
> From: Pierre Gondois <pierre.gondois@arm.com>
> Sent: Friday, February 3, 2023 6:11 AM
> To: Jeff Brasen <jbrasen@nvidia.com>; devel@edk2.groups.io
> Cc: Sami.Mujawar@arm.com; Alexei.Fedorov@arm.com;
> quic_llindhol@quicinc.com; ardb+tianocore@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.gondois@arm.com>
> >> Sent: Thursday, February 2, 2023 10:49 AM
> >> To: Jeff Brasen <jbrasen@nvidia.com>; devel@edk2.groups.io
> >> Cc: Sami.Mujawar@arm.com; Alexei.Fedorov@arm.com;
> >> quic_llindhol@quicinc.com; ardb+tianocore@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.gondois@arm.com>
> >>>> Sent: Thursday, February 2, 2023 5:03 AM
> >>>> To: Jeff Brasen <jbrasen@nvidia.com>; devel@edk2.groups.io
> >>>> Cc: Sami.Mujawar@arm.com; Alexei.Fedorov@arm.com;
> >>>> quic_llindhol@quicinc.com; ardb+tianocore@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 <jbrasen@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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] DynamicTablesPkg: Allow multiple top level physical nodes
2023-02-03 16:00 ` Jeff Brasen
@ 2023-02-03 16:28 ` PierreGondois
2023-02-03 16:38 ` Jeff Brasen
0 siblings, 1 reply; 10+ messages in thread
From: PierreGondois @ 2023-02-03 16:28 UTC (permalink / raw)
To: Jeff Brasen, devel@edk2.groups.io
Cc: Sami.Mujawar@arm.com, Alexei.Fedorov@arm.com,
quic_llindhol@quicinc.com, ardb+tianocore@kernel.org
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.gondois@arm.com>
>> Sent: Friday, February 3, 2023 6:11 AM
>> To: Jeff Brasen <jbrasen@nvidia.com>; devel@edk2.groups.io
>> Cc: Sami.Mujawar@arm.com; Alexei.Fedorov@arm.com;
>> quic_llindhol@quicinc.com; ardb+tianocore@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.gondois@arm.com>
>>>> Sent: Thursday, February 2, 2023 10:49 AM
>>>> To: Jeff Brasen <jbrasen@nvidia.com>; devel@edk2.groups.io
>>>> Cc: Sami.Mujawar@arm.com; Alexei.Fedorov@arm.com;
>>>> quic_llindhol@quicinc.com; ardb+tianocore@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.gondois@arm.com>
>>>>>> Sent: Thursday, February 2, 2023 5:03 AM
>>>>>> To: Jeff Brasen <jbrasen@nvidia.com>; devel@edk2.groups.io
>>>>>> Cc: Sami.Mujawar@arm.com; Alexei.Fedorov@arm.com;
>>>>>> quic_llindhol@quicinc.com; ardb+tianocore@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 <jbrasen@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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] DynamicTablesPkg: Allow multiple top level physical nodes
2023-02-03 16:28 ` PierreGondois
@ 2023-02-03 16:38 ` Jeff Brasen
2023-02-06 9:27 ` PierreGondois
0 siblings, 1 reply; 10+ messages in thread
From: Jeff Brasen @ 2023-02-03 16:38 UTC (permalink / raw)
To: Pierre Gondois, devel@edk2.groups.io
Cc: Sami.Mujawar@arm.com, Alexei.Fedorov@arm.com,
quic_llindhol@quicinc.com, ardb+tianocore@kernel.org
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
> -----Original Message-----
> From: Pierre Gondois <pierre.gondois@arm.com>
> Sent: Friday, February 3, 2023 9:28 AM
> To: Jeff Brasen <jbrasen@nvidia.com>; devel@edk2.groups.io
> Cc: Sami.Mujawar@arm.com; Alexei.Fedorov@arm.com;
> quic_llindhol@quicinc.com; ardb+tianocore@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.gondois@arm.com>
> >> Sent: Friday, February 3, 2023 6:11 AM
> >> To: Jeff Brasen <jbrasen@nvidia.com>; devel@edk2.groups.io
> >> Cc: Sami.Mujawar@arm.com; Alexei.Fedorov@arm.com;
> >> quic_llindhol@quicinc.com; ardb+tianocore@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.gondois@arm.com>
> >>>> Sent: Thursday, February 2, 2023 10:49 AM
> >>>> To: Jeff Brasen <jbrasen@nvidia.com>; devel@edk2.groups.io
> >>>> Cc: Sami.Mujawar@arm.com; Alexei.Fedorov@arm.com;
> >>>> quic_llindhol@quicinc.com; ardb+tianocore@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.gondois@arm.com>
> >>>>>> Sent: Thursday, February 2, 2023 5:03 AM
> >>>>>> To: Jeff Brasen <jbrasen@nvidia.com>; devel@edk2.groups.io
> >>>>>> Cc: Sami.Mujawar@arm.com; Alexei.Fedorov@arm.com;
> >>>>>> quic_llindhol@quicinc.com; ardb+tianocore@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 <jbrasen@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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] DynamicTablesPkg: Allow multiple top level physical nodes
2023-02-03 16:38 ` Jeff Brasen
@ 2023-02-06 9:27 ` PierreGondois
0 siblings, 0 replies; 10+ messages in thread
From: PierreGondois @ 2023-02-06 9:27 UTC (permalink / raw)
To: Jeff Brasen, devel@edk2.groups.io
Cc: Sami.Mujawar@arm.com, Alexei.Fedorov@arm.com,
quic_llindhol@quicinc.com, ardb+tianocore@kernel.org
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.gondois@arm.com>
>> Sent: Friday, February 3, 2023 9:28 AM
>> To: Jeff Brasen <jbrasen@nvidia.com>; devel@edk2.groups.io
>> Cc: Sami.Mujawar@arm.com; Alexei.Fedorov@arm.com;
>> quic_llindhol@quicinc.com; ardb+tianocore@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.gondois@arm.com>
>>>> Sent: Friday, February 3, 2023 6:11 AM
>>>> To: Jeff Brasen <jbrasen@nvidia.com>; devel@edk2.groups.io
>>>> Cc: Sami.Mujawar@arm.com; Alexei.Fedorov@arm.com;
>>>> quic_llindhol@quicinc.com; ardb+tianocore@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.gondois@arm.com>
>>>>>> Sent: Thursday, February 2, 2023 10:49 AM
>>>>>> To: Jeff Brasen <jbrasen@nvidia.com>; devel@edk2.groups.io
>>>>>> Cc: Sami.Mujawar@arm.com; Alexei.Fedorov@arm.com;
>>>>>> quic_llindhol@quicinc.com; ardb+tianocore@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.gondois@arm.com>
>>>>>>>> Sent: Thursday, February 2, 2023 5:03 AM
>>>>>>>> To: Jeff Brasen <jbrasen@nvidia.com>; devel@edk2.groups.io
>>>>>>>> Cc: Sami.Mujawar@arm.com; Alexei.Fedorov@arm.com;
>>>>>>>> quic_llindhol@quicinc.com; ardb+tianocore@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 <jbrasen@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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-02-06 9:27 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-02-01 16:42 [PATCH] DynamicTablesPkg: Allow multiple top level physical nodes Jeff Brasen
2023-02-02 12:02 ` PierreGondois
2023-02-02 16:48 ` Jeff Brasen
2023-02-02 17:48 ` PierreGondois
2023-02-02 17:53 ` Jeff Brasen
2023-02-03 13:11 ` PierreGondois
2023-02-03 16:00 ` Jeff Brasen
2023-02-03 16:28 ` PierreGondois
2023-02-03 16:38 ` Jeff Brasen
2023-02-06 9:27 ` PierreGondois
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox