From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web11.49681.1675675651069245538 for ; Mon, 06 Feb 2023 01:27:31 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: pierre.gondois@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 847F3113E; Mon, 6 Feb 2023 01:28:12 -0800 (PST) Received: from [10.34.100.128] (pierre123.nice.arm.com [10.34.100.128]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 179873F71E; Mon, 6 Feb 2023 01:27:28 -0800 (PST) Message-ID: <1cba8837-086f-656b-ba80-a5d2f0f6b18d@arm.com> Date: Mon, 6 Feb 2023 10:27:23 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.4.2 Subject: Re: [PATCH] DynamicTablesPkg: Allow multiple top level physical nodes To: Jeff Brasen , "devel@edk2.groups.io" Cc: "Sami.Mujawar@arm.com" , "Alexei.Fedorov@arm.com" , "quic_llindhol@quicinc.com" , "ardb+tianocore@kernel.org" References: <5a4e5960-ad7d-e8d0-cd87-f51693c51ad8@arm.com> <435fe4ff-aa22-aa09-a1e0-3e3116decdce@arm.com> From: "PierreGondois" In-Reply-To: Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 2/3/23 17:38, Jeff Brasen wrote: > 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 Yes right. However if the UID/Name were to be generated, the topology could be misleading, cf the example below where package/cluster names are incremented even though they are not on the same level. > >> -----Original Message----- >> From: Pierre Gondois >> Sent: Friday, February 3, 2023 9:28 AM >> To: Jeff Brasen ; devel@edk2.groups.io >> Cc: Sami.Mujawar@arm.com; Alexei.Fedorov@arm.com; >> quic_llindhol@quicinc.com; ardb+tianocore@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 >>>> Sent: Friday, February 3, 2023 6:11 AM >>>> To: Jeff Brasen ; devel@edk2.groups.io >>>> Cc: Sami.Mujawar@arm.com; Alexei.Fedorov@arm.com; >>>> quic_llindhol@quicinc.com; ardb+tianocore@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 >>>>>> Sent: Thursday, February 2, 2023 10:49 AM >>>>>> To: Jeff Brasen ; devel@edk2.groups.io >>>>>> Cc: Sami.Mujawar@arm.com; Alexei.Fedorov@arm.com; >>>>>> quic_llindhol@quicinc.com; ardb+tianocore@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 >>>>>>>> Sent: Thursday, February 2, 2023 5:03 AM >>>>>>>> To: Jeff Brasen ; devel@edk2.groups.io >>>>>>>> Cc: Sami.Mujawar@arm.com; Alexei.Fedorov@arm.com; >>>>>>>> quic_llindhol@quicinc.com; ardb+tianocore@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 >>>>>>>>> --- >>>>>>>>> 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 >>>>>>>>> #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/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