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