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.gond...@arm.com> > Sent: Thursday, February 2, 2023 10:49 AM > To: Jeff Brasen <jbra...@nvidia.com>; devel@edk2.groups.io > Cc: sami.muja...@arm.com; alexei.fedo...@arm.com; > quic_llind...@quicinc.com; ardb+tianoc...@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.gond...@arm.com> > >> Sent: Thursday, February 2, 2023 5:03 AM > >> To: Jeff Brasen <jbra...@nvidia.com>; devel@edk2.groups.io > >> Cc: sami.muja...@arm.com; alexei.fedo...@arm.com; > >> quic_llind...@quicinc.com; ardb+tianoc...@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 <jbra...@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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#99492): https://edk2.groups.io/g/devel/message/99492 Mute This Topic: https://groups.io/mt/96680589/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-