Reviewed-by: Jeff Brasen <jbra...@nvidia.com> > -----Original Message----- > From: pierre.gond...@arm.com <pierre.gond...@arm.com> > Sent: Thursday, March 9, 2023 8:33 AM > To: devel@edk2.groups.io > Cc: Sami Mujawar <sami.muja...@arm.com>; Alexei Fedorov > <alexei.fedo...@arm.com>; Jeff Brasen <jbra...@nvidia.com> > Subject: [PATCH 1/1] DynamicTablesPkg/SsdtCpuTopology: Allow multi- > packages topologies > > External email: Use caution opening links or attachments > > > 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://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 <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/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
-=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#101015): https://edk2.groups.io/g/devel/message/101015 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] -=-=-=-=-=-=-=-=-=-=-=-
