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.web09.2918.1667998695663381438 for ; Wed, 09 Nov 2022 04:58:15 -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 6B2111FB; Wed, 9 Nov 2022 04:58:21 -0800 (PST) Received: from [10.57.3.250] (unknown [10.57.3.250]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id DE3563F73D; Wed, 9 Nov 2022 04:58:13 -0800 (PST) Message-ID: <14750ed9-124f-5441-b378-d34a8e6f09a2@arm.com> Date: Wed, 9 Nov 2022 13:58:07 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.2.2 Subject: Re: [edk2-devel] [PATCH] DynamicTablesPkg: Allow for specified CPU names To: devel@edk2.groups.io, jbrasen@nvidia.com Cc: Sami.Mujawar@arm.com, Alexei.Fedorov@arm.com, quic_llindhol@quicinc.com, ardb+tianocore@kernel.org References: <0fd42dc1fd90f28521a3a1e47934fa7d45bcec6b.1667836523.git.jbrasen@nvidia.com> From: "PierreGondois" In-Reply-To: <0fd42dc1fd90f28521a3a1e47934fa7d45bcec6b.1667836523.git.jbrasen@nvidia.com> Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Hello Jeff, On 11/7/22 16:57, Jeff Brasen via groups.io wrote: > Allow object to specify the name of processor and processor container > > nodes and the UID of processor containers. > > > > This allows these to be more accurately referenced from other tables. > > For example for the _PSL method or the UID in the APMT table. The CM_ARM_PROC_HIERARCHY_INFO struct has a token that can be used for referencing. If there were generators for the _PSL method and the APMT table, this would be possible to avoid handwriting CPUs' Name/Uid and use the ones generated in SsdtCpuTopologyGenerator.c somehow. This would take more time to do. Otherwise the code itself looks good to me, Regards, Pierre > > > > The UID and Name for processor container may be different as if the > > intention is to set names as the corresponding affinity level the UID > > may need to be different if there are multiple levels of containers. > > > > Signed-off-by: Jeff Brasen > > --- > > .../Include/ArmNameSpaceObjects.h | 11 +++++ > > .../SsdtCpuTopologyGenerator.c | 40 ++++++++++++++----- > > .../ConfigurationManagerObjectParser.c | 3 ++ > > 3 files changed, 43 insertions(+), 11 deletions(-) > > > > diff --git a/DynamicTablesPkg/Include/ArmNameSpaceObjects.h b/DynamicTablesPkg/Include/ArmNameSpaceObjects.h > > index 6aafd41a2e..19098609de 100644 > > --- a/DynamicTablesPkg/Include/ArmNameSpaceObjects.h > > +++ b/DynamicTablesPkg/Include/ArmNameSpaceObjects.h > > @@ -768,6 +768,17 @@ typedef struct CmArmProcHierarchyInfo { > > /// Token identifying a CM_ARM_OBJ_REF structure, itself referencing > > /// CM_ARM_LPI_INFO objects. > > CM_OBJECT_TOKEN LpiToken; > > + /// Set to TRUE if UID should override index for name and _UID > > + /// for processor container nodes and name of processors. > > + /// This should be consistently set for containers or processors to avoid > > + /// duplicate values > > + BOOLEAN OverrideNameUidEnabled; > > + /// If OverrideNameUidEnabled is TRUE then this value will be used for name of > > + /// processors and processor containers. > > + UINT16 OverrideName; > > + /// If OverrideNameUidEnabled is TRUE then this value will be used for > > + /// the UID of processor containers. > > + UINT32 OverrideUid; > > } CM_ARM_PROC_HIERARCHY_INFO; > > > > /** A structure that describes the Cache Type Structure (Type 1) in PPTT > > diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c > > index d06c7615fb..92fa904103 100644 > > --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c > > +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c > > @@ -553,7 +553,7 @@ GenerateLpiStates ( > > @param [in] Generator The SSDT Cpu Topology generator. > > @param [in] ParentNode Parent node to attach the Cpu node to. > > @param [in] GicCInfo CM_ARM_GICC_INFO object used to create the node. > > - @param [in] CpuIndex Index used to generate the node name. > > + @param [in] CpuName Value used to generate the node name. > > @param [out] CpuNodePtr If not NULL, return the created Cpu node. > > > > @retval EFI_SUCCESS Success. > > @@ -567,7 +567,7 @@ CreateAmlCpu ( > > IN ACPI_CPU_TOPOLOGY_GENERATOR *Generator, > > IN AML_NODE_HANDLE ParentNode, > > IN CM_ARM_GICC_INFO *GicCInfo, > > - IN UINT32 CpuIndex, > > + IN UINT32 CpuName, > > OUT AML_OBJECT_NODE_HANDLE *CpuNodePtr OPTIONAL > > ) > > { > > @@ -579,7 +579,7 @@ CreateAmlCpu ( > > ASSERT (ParentNode != NULL); > > ASSERT (GicCInfo != NULL); > > > > - Status = WriteAslName ('C', CpuIndex, AslName); > > + Status = WriteAslName ('C', CpuName, AslName); > > if (EFI_ERROR (Status)) { > > ASSERT (0); > > return Status; > > @@ -628,7 +628,7 @@ CreateAmlCpu ( > > @param [in] CfgMgrProtocol Pointer to the Configuration Manager > > Protocol Interface. > > @param [in] ParentNode Parent node to attach the Cpu node to. > > - @param [in] CpuIndex Index used to generate the node name. > > + @param [in] CpuName Value used to generate the node name. > > @param [in] ProcHierarchyNodeInfo CM_ARM_PROC_HIERARCHY_INFO describing > > the Cpu. > > > > @@ -643,7 +643,7 @@ CreateAmlCpuFromProcHierarchy ( > > IN ACPI_CPU_TOPOLOGY_GENERATOR *Generator, > > IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL *CONST CfgMgrProtocol, > > IN AML_NODE_HANDLE ParentNode, > > - IN UINT32 CpuIndex, > > + IN UINT32 CpuName, > > IN CM_ARM_PROC_HIERARCHY_INFO *ProcHierarchyNodeInfo > > ) > > { > > @@ -668,7 +668,7 @@ CreateAmlCpuFromProcHierarchy ( > > return Status; > > } > > > > - Status = CreateAmlCpu (Generator, ParentNode, GicCInfo, CpuIndex, &CpuNode); > > + Status = CreateAmlCpu (Generator, ParentNode, GicCInfo, CpuName, &CpuNode); > > if (EFI_ERROR (Status)) { > > ASSERT (0); > > return Status; > > @@ -735,7 +735,8 @@ CreateAmlProcessorContainer ( > > IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL *CONST CfgMgrProtocol, > > IN AML_NODE_HANDLE ParentNode, > > IN CM_ARM_PROC_HIERARCHY_INFO *ProcHierarchyNodeInfo, > > - IN UINT32 ProcContainerIndex, > > + IN UINT16 ProcContainerName, > > + IN UINT32 ProcContainerUid, > > OUT AML_OBJECT_NODE_HANDLE *ProcContainerNodePtr > > ) > > { > > @@ -749,7 +750,7 @@ CreateAmlProcessorContainer ( > > ASSERT (ProcHierarchyNodeInfo != NULL); > > ASSERT (ProcContainerNodePtr != NULL); > > > > - Status = WriteAslName ('C', ProcContainerIndex, AslNameProcContainer); > > + Status = WriteAslName ('C', ProcContainerName, AslNameProcContainer); > > if (EFI_ERROR (Status)) { > > ASSERT (0); > > return Status; > > @@ -765,7 +766,7 @@ CreateAmlProcessorContainer ( > > // and EFI_ACPI_6_3_PPTT_PROCESSOR_ID_INVALID is set for non-Cpus. > > Status = AmlCodeGenNameInteger ( > > "_UID", > > - ProcContainerIndex, > > + ProcContainerUid, > > ProcContainerNode, > > NULL > > ); > > @@ -838,6 +839,8 @@ CreateAmlCpuTopologyTree ( > > UINT32 Index; > > UINT32 CpuIndex; > > AML_OBJECT_NODE_HANDLE ProcContainerNode; > > + UINT32 Uid; > > + UINT16 Name; > > > > ASSERT (Generator != NULL); > > ASSERT (Generator->ProcNodeList != NULL); > > @@ -868,11 +871,17 @@ CreateAmlCpuTopologyTree ( > > return EFI_INVALID_PARAMETER; > > } > > > > + if (Generator->ProcNodeList[Index].OverrideNameUidEnabled) { > > + Name = Generator->ProcNodeList[Index].OverrideName; > > + } else { > > + Name = CpuIndex; > > + } > > + > > Status = CreateAmlCpuFromProcHierarchy ( > > Generator, > > CfgMgrProtocol, > > ParentNode, > > - CpuIndex, > > + Name, > > &Generator->ProcNodeList[Index] > > ); > > if (EFI_ERROR (Status)) { > > @@ -897,12 +906,21 @@ CreateAmlCpuTopologyTree ( > > return EFI_INVALID_PARAMETER; > > } > > > > + if (Generator->ProcNodeList[Index].OverrideNameUidEnabled) { > > + Name = Generator->ProcNodeList[Index].OverrideName; > > + Uid = Generator->ProcNodeList[Index].OverrideUid; > > + } else { > > + Name = *ProcContainerIndex; > > + Uid = *ProcContainerIndex; > > + } > > + > > Status = CreateAmlProcessorContainer ( > > Generator, > > CfgMgrProtocol, > > ParentNode, > > &Generator->ProcNodeList[Index], > > - *ProcContainerIndex, > > + Name, > > + Uid, > > &ProcContainerNode > > ); > > if (EFI_ERROR (Status)) { > > diff --git a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c > > index 5a01ed0fb8..5e4b88e8cc 100644 > > --- a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c > > +++ b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c > > @@ -305,6 +305,9 @@ STATIC CONST CM_OBJ_PARSER CmArmProcHierarchyInfoParser[] = { > > { "NoOfPrivateResources", 4, "0x%x", NULL }, > > { "PrivateResourcesArrayToken", sizeof (CM_OBJECT_TOKEN), "0x%p", NULL }, > > { "LpiToken", sizeof (CM_OBJECT_TOKEN), "0x%p", NULL }, > > + { "OverrideNameUidEnabled", 1, "%d", NULL }, > > + { "OverrideName", 2, "0x%x", NULL }, > > + { "OverrideUid", 4, "0x%x", NULL } > > }; > > > > /** A parser for EArmObjCacheInfo. >