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.muja...@arm.com> Regards, Sami Mujawar On 09/03/2023 03:32 pm, pierre.gond...@arm.com wrote:
From: Pierre Gondois <pierre.gond...@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 <jbra...@nvidia.com> Signed-off-by: Pierre Gondois <pierre.gond...@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. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#103538): https://edk2.groups.io/g/devel/message/103538 Mute This Topic: https://groups.io/mt/97498327/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
