To solve that problem I had added support for allowing the UID/Name to come from the node
https://github.com/tianocore/edk2/commit/5fb3f5723a1ea9d9a93e317181e1c11468a9eb45 > -----Original Message----- > From: Pierre Gondois <pierre.gond...@arm.com> > Sent: Friday, February 3, 2023 9:28 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 > > > On 2/3/23 17:00, Jeff Brasen wrote: > > I'll on an updated patch this morning that only does the new behavior. We > can't reset the procindex as it is used for the _UID as well and we would end > up > with the same value in two nodes. > > Yes indeed, then maybe the name/uid selection should not be done in > CreateAmlCpuTopologyTree() but in > CreateAmlProcessorContainer()/CreateAmlCpuFromProcHierarchy(). > This would allow to have a static counter for the Uid in > CreateAmlProcessorContainer() and always have incrementing names for > packages/cluster. Otherwise the generated name will be: > C000 <- Package > \-C0001 <- Cluster > \-C0000 <- CPU > C002 <- second Package > \-C0003 <- second Cluster > \-C0001 <- second CPU > > instead of: > C000 <- Package > \-C0001 <- Cluster > \-C0000 <- CPU > C001 <- second Package > \-C0000 <- second Cluster > \-C0001 <- second CPU > > Regards, > Pierre > > > > > -Jeff > > > > > >> -----Original Message----- > >> From: Pierre Gondois <pierre.gond...@arm.com> > >> Sent: Friday, February 3, 2023 6:11 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 > >> > >> > >> On 2/2/23 18:53, Jeff Brasen wrote: > >>> 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 > >> > >> The current SsdtCpuTopologyGenerator doesn't generate an AML node for > >> the top level package. Even though this seem compliant to the ACPI > >> spec, this induces a difference between the ASL topology description > >> and the PPTT topology description. For instance, for the Juno, the > >> topology generated for the ACPI tables are: > >> PPTT: > >> (PACKAGE) > >> \-Little Cluster > >> \-CPU[0,3-5] > >> \-Big Cluster > >> \-CPU[1-2] > >> > >> SSDT: > >> Little Cluster > >> \-CPU[0,3-5] > >> Big Cluster > >> \-CPU[1-2] > >> > >> To solve your issue, to have matching topology descriptions, and > >> after discussing with Sami, it would be better to have: > >> SSDT: > >> (PACKAGE) > >> \-Little Cluster > >> \-CPU[0,3-5] > >> \-Big Cluster > >> \-CPU[1-2] > >> > >> The Juno is the only platform that publicly uses the > >> SsdtCpuTopologyGenerator, so I am not sure how other platforms support > should be handled. > >> > >> About the code itself, I think the ProcContainerIndex should also be > >> reset in > >> CreateAmlCpuTopologyTree() when generating a new level of containers > >> (if it is decided to go this way). > >> > >> Regards, > >> Pierre > >> > >>> > >>> -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.PcdForceTopLevelProcessorConta > >>>> i > >>>>>>> + 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/SsdtC > >>>> p > >>>>>> uT > >>>>>>> opologyGenerator.c > >>>>>>> > >>>>>> > >>>> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtC > >>>> p > >>>>>> uT > >>>>>>> opologyGenerator.c > >>>>>>> index c24da8ec71..58f86ff508 100644 > >>>>>>> --- > >>>>>>> > >>>>>> > >>>> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtC > >>>> p > >>>>>> uT > >>>>>>> opologyGenerator.c > >>>>>>> +++ > >>>>>> > b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/Ssd > >>>>>> t > >>>>>>> +++ 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/SsdtC > >>>> p > >>>>>> uT > >>>>>>> opologyLibArm.inf > >>>>>>> > >>>>>> > >>>> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtC > >>>> p > >>>>>> uT > >>>>>>> opologyLibArm.inf > >>>>>>> index 3e2d154749..00adfe986f 100644 > >>>>>>> --- > >>>>>>> > >>>>>> > >>>> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtC > >>>> p > >>>>>> uT > >>>>>>> opologyLibArm.inf > >>>>>>> +++ > >>>>>> > b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/Ssd > >>>>>> t > >>>>>>> +++ CpuTopologyLibArm.inf > >>>>>>> @@ -31,3 +31,7 @@ > >>>>>>> AcpiHelperLib > >>>>>>> AmlLib > >>>>>>> BaseLib > >>>>>>> + PcdLib > >>>>>>> + > >>>>>>> +[Pcd] > >>>>>>> + > >>>>>>> > >>>>>> > >>>> > +gEdkiiDynamicTablesPkgTokenSpaceGuid.PcdForceTopLevelProcessorCont > >>>> +a > >>>>>> in > >>>>>>> +er -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#99612): https://edk2.groups.io/g/devel/message/99612 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] -=-=-=-=-=-=-=-=-=-=-=-
