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.web10.1055.1675360120464581910 for ; Thu, 02 Feb 2023 09:48:40 -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 D369EC14; Thu, 2 Feb 2023 09:49:21 -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 B16313F64C; Thu, 2 Feb 2023 09:48:38 -0800 (PST) Message-ID: <5a4e5960-ad7d-e8d0-cd87-f51693c51ad8@arm.com> Date: Thu, 2 Feb 2023 18:48:33 +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: From: "PierreGondois" In-Reply-To: Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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.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 >>> #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/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