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 > --- > 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 > #include > #include > +#include > #include > > #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