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.49682.1675675659830769491 for ; Mon, 06 Feb 2023 01:27:39 -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 C6C4D113E; Mon, 6 Feb 2023 01:28: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 722843F71E; Mon, 6 Feb 2023 01:27:38 -0800 (PST) Message-ID: Date: Mon, 6 Feb 2023 10:27:36 +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 v2] 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: <2fbd84095cc52b908f0a59d98358f36a396c319b.1675447806.git.jbrasen@nvidia.com> From: "PierreGondois" In-Reply-To: <2fbd84095cc52b908f0a59d98358f36a396c319b.1675447806.git.jbrasen@nvidia.com> Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Hello Jeff, Thanks for the v2. Also cf the first discussion at: https://edk2.groups.io/g/devel/topic/96680589#99612 - I think it would be good to extract a function that does all the checks as there are many possibilities for the flags/parent combinations. - I think it would also be nice to reset the index of ProcContainers for each new level (i.e. not to have the same incrementing index for clusters/packages) I created a branch based on your work at: https://github.com/pierregondois/edk2/tree/pg/top_level_pnode_Wip Regards, Pierre On 2/3/23 19:10, 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 create a > top level processor container for all systems. > > Signed-off-by: Jeff Brasen > --- > .../SsdtCpuTopologyGenerator.c | 43 ++++++------------- > 1 file changed, 12 insertions(+), 31 deletions(-) > > diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c > index c24da8ec71..46b757e0b2 100644 > --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c > +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c > @@ -814,7 +814,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 +842,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 +894,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; > + } > + I think that if (NodeToken == CM_NULL_TOKEN) and doesn't have the Physical Package flag, no error will be triggered even though this is not a valid configuration. > // 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 (( > @@ -974,8 +981,6 @@ CreateTopologyFromProcHierarchy ( > ) > { > EFI_STATUS Status; > - UINT32 Index; > - UINT32 TopLevelProcNodeIndex; > UINT32 ProcContainerIndex; > > ASSERT (Generator != NULL); > @@ -984,8 +989,7 @@ CreateTopologyFromProcHierarchy ( > ASSERT (CfgMgrProtocol != NULL); > ASSERT (ScopeNode != NULL); > > - TopLevelProcNodeIndex = MAX_UINT32; > - ProcContainerIndex = 0; > + ProcContainerIndex = 0; > > Status = TokenTableInitialize (Generator, Generator->ProcNodeCount); > if (EFI_ERROR (Status)) { > @@ -993,33 +997,10 @@ 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; > - } > - > - TopLevelProcNodeIndex = Index; > - } > - } // for > - > Status = CreateAmlCpuTopologyTree ( > Generator, > CfgMgrProtocol, > - Generator->ProcNodeList[TopLevelProcNodeIndex].Token, > + CM_NULL_TOKEN, > ScopeNode, > &ProcContainerIndex > );