* [PATCH] DynamicTablesPkg: Correct cluster index
@ 2022-08-16 20:45 Jeff Brasen
2022-08-17 7:01 ` Ard Biesheuvel
0 siblings, 1 reply; 5+ messages in thread
From: Jeff Brasen @ 2022-08-16 20:45 UTC (permalink / raw)
To: devel
Cc: quic_llindhol, ardb+tianocore, Sami.Mujawar, Alexei.Fedorov,
Jeff Brasen
Current code will generate duplicate UID if there are nested clusters
in the topology.
Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
---
.../SsdtCpuTopologyGenerator.c | 22 ++++++++++++-------
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
index 3266d8dd98..9295117f1f 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
@@ -706,6 +706,8 @@ CreateAmlCluster (
Cannot be CM_NULL_TOKEN.
@param [in] ParentNode Parent node to attach the created
node to.
+ @param [in,out] ClusterIndex Pointer to the current cluster index
+ to be used as UID.
@retval EFI_SUCCESS Success.
@retval EFI_INVALID_PARAMETER Invalid parameter.
@@ -718,13 +720,13 @@ CreateAmlCpuTopologyTree (
IN ACPI_CPU_TOPOLOGY_GENERATOR *Generator,
IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL *CONST CfgMgrProtocol,
IN CM_OBJECT_TOKEN NodeToken,
- IN AML_NODE_HANDLE ParentNode
+ IN AML_NODE_HANDLE ParentNode,
+ IN OUT UINT32 *ClusterIndex
)
{
EFI_STATUS Status;
UINT32 Index;
UINT32 CpuIndex;
- UINT32 ClusterIndex;
AML_OBJECT_NODE_HANDLE ClusterNode;
ASSERT (Generator != NULL);
@@ -733,9 +735,9 @@ CreateAmlCpuTopologyTree (
ASSERT (CfgMgrProtocol != NULL);
ASSERT (NodeToken != CM_NULL_TOKEN);
ASSERT (ParentNode != NULL);
+ ASSERT (ClusterIndex != NULL);
- CpuIndex = 0;
- ClusterIndex = 0;
+ CpuIndex = 0;
for (Index = 0; Index < Generator->ProcNodeCount; Index++) {
// Find the children of the CM_ARM_PROC_HIERARCHY_INFO
@@ -790,7 +792,7 @@ CreateAmlCpuTopologyTree (
CfgMgrProtocol,
ParentNode,
&Generator->ProcNodeList[Index],
- ClusterIndex,
+ *ClusterIndex,
&ClusterNode
);
if (EFI_ERROR (Status)) {
@@ -800,7 +802,7 @@ CreateAmlCpuTopologyTree (
// Nodes must have a unique name in the ASL namespace.
// Reset the Cpu index whenever we create a new Cluster.
- ClusterIndex++;
+ (*ClusterIndex)++;
CpuIndex = 0;
// Recursively continue creating an AML tree.
@@ -808,7 +810,8 @@ CreateAmlCpuTopologyTree (
Generator,
CfgMgrProtocol,
Generator->ProcNodeList[Index].Token,
- ClusterNode
+ ClusterNode,
+ ClusterIndex
);
if (EFI_ERROR (Status)) {
ASSERT (0);
@@ -845,6 +848,7 @@ CreateTopologyFromProcHierarchy (
EFI_STATUS Status;
UINT32 Index;
UINT32 TopLevelProcNodeIndex;
+ UINT32 ClusterIndex;
ASSERT (Generator != NULL);
ASSERT (Generator->ProcNodeCount != 0);
@@ -853,6 +857,7 @@ CreateTopologyFromProcHierarchy (
ASSERT (ScopeNode != NULL);
TopLevelProcNodeIndex = MAX_UINT32;
+ ClusterIndex = 0;
Status = TokenTableInitialize (Generator, Generator->ProcNodeCount);
if (EFI_ERROR (Status)) {
@@ -887,7 +892,8 @@ CreateTopologyFromProcHierarchy (
Generator,
CfgMgrProtocol,
Generator->ProcNodeList[TopLevelProcNodeIndex].Token,
- ScopeNode
+ ScopeNode,
+ &ClusterIndex
);
if (EFI_ERROR (Status)) {
ASSERT (0);
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] DynamicTablesPkg: Correct cluster index
2022-08-16 20:45 [PATCH] DynamicTablesPkg: Correct cluster index Jeff Brasen
@ 2022-08-17 7:01 ` Ard Biesheuvel
2022-08-17 16:14 ` Jeff Brasen
0 siblings, 1 reply; 5+ messages in thread
From: Ard Biesheuvel @ 2022-08-17 7:01 UTC (permalink / raw)
To: Jeff Brasen
Cc: devel, quic_llindhol, ardb+tianocore, Sami.Mujawar,
Alexei.Fedorov
On Tue, 16 Aug 2022 at 22:46, Jeff Brasen <jbrasen@nvidia.com> wrote:
>
> Current code will generate duplicate UID if there are nested clusters
> in the topology.
>
What is a nested cluster?
> Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
> ---
> .../SsdtCpuTopologyGenerator.c | 22 ++++++++++++-------
> 1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
> index 3266d8dd98..9295117f1f 100644
> --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
> +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
> @@ -706,6 +706,8 @@ CreateAmlCluster (
> Cannot be CM_NULL_TOKEN.
> @param [in] ParentNode Parent node to attach the created
> node to.
> + @param [in,out] ClusterIndex Pointer to the current cluster index
> + to be used as UID.
>
> @retval EFI_SUCCESS Success.
> @retval EFI_INVALID_PARAMETER Invalid parameter.
> @@ -718,13 +720,13 @@ CreateAmlCpuTopologyTree (
> IN ACPI_CPU_TOPOLOGY_GENERATOR *Generator,
> IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL *CONST CfgMgrProtocol,
> IN CM_OBJECT_TOKEN NodeToken,
> - IN AML_NODE_HANDLE ParentNode
> + IN AML_NODE_HANDLE ParentNode,
> + IN OUT UINT32 *ClusterIndex
> )
> {
> EFI_STATUS Status;
> UINT32 Index;
> UINT32 CpuIndex;
> - UINT32 ClusterIndex;
> AML_OBJECT_NODE_HANDLE ClusterNode;
>
> ASSERT (Generator != NULL);
> @@ -733,9 +735,9 @@ CreateAmlCpuTopologyTree (
> ASSERT (CfgMgrProtocol != NULL);
> ASSERT (NodeToken != CM_NULL_TOKEN);
> ASSERT (ParentNode != NULL);
> + ASSERT (ClusterIndex != NULL);
>
> - CpuIndex = 0;
> - ClusterIndex = 0;
> + CpuIndex = 0;
>
> for (Index = 0; Index < Generator->ProcNodeCount; Index++) {
> // Find the children of the CM_ARM_PROC_HIERARCHY_INFO
> @@ -790,7 +792,7 @@ CreateAmlCpuTopologyTree (
> CfgMgrProtocol,
> ParentNode,
> &Generator->ProcNodeList[Index],
> - ClusterIndex,
> + *ClusterIndex,
> &ClusterNode
> );
> if (EFI_ERROR (Status)) {
> @@ -800,7 +802,7 @@ CreateAmlCpuTopologyTree (
>
> // Nodes must have a unique name in the ASL namespace.
> // Reset the Cpu index whenever we create a new Cluster.
> - ClusterIndex++;
> + (*ClusterIndex)++;
> CpuIndex = 0;
>
> // Recursively continue creating an AML tree.
> @@ -808,7 +810,8 @@ CreateAmlCpuTopologyTree (
> Generator,
> CfgMgrProtocol,
> Generator->ProcNodeList[Index].Token,
> - ClusterNode
> + ClusterNode,
> + ClusterIndex
> );
> if (EFI_ERROR (Status)) {
> ASSERT (0);
> @@ -845,6 +848,7 @@ CreateTopologyFromProcHierarchy (
> EFI_STATUS Status;
> UINT32 Index;
> UINT32 TopLevelProcNodeIndex;
> + UINT32 ClusterIndex;
>
> ASSERT (Generator != NULL);
> ASSERT (Generator->ProcNodeCount != 0);
> @@ -853,6 +857,7 @@ CreateTopologyFromProcHierarchy (
> ASSERT (ScopeNode != NULL);
>
> TopLevelProcNodeIndex = MAX_UINT32;
> + ClusterIndex = 0;
>
> Status = TokenTableInitialize (Generator, Generator->ProcNodeCount);
> if (EFI_ERROR (Status)) {
> @@ -887,7 +892,8 @@ CreateTopologyFromProcHierarchy (
> Generator,
> CfgMgrProtocol,
> Generator->ProcNodeList[TopLevelProcNodeIndex].Token,
> - ScopeNode
> + ScopeNode,
> + &ClusterIndex
> );
> if (EFI_ERROR (Status)) {
> ASSERT (0);
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] DynamicTablesPkg: Correct cluster index
2022-08-17 7:01 ` Ard Biesheuvel
@ 2022-08-17 16:14 ` Jeff Brasen
2022-08-18 10:22 ` Ard Biesheuvel
0 siblings, 1 reply; 5+ messages in thread
From: Jeff Brasen @ 2022-08-17 16:14 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: devel@edk2.groups.io, quic_llindhol@quicinc.com,
ardb+tianocore@kernel.org, Sami.Mujawar@arm.com,
Alexei.Fedorov@arm.com
> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> Sent: Wednesday, August 17, 2022 1:01 AM
> To: Jeff Brasen <jbrasen@nvidia.com>
> Cc: devel@edk2.groups.io; quic_llindhol@quicinc.com;
> ardb+tianocore@kernel.org; Sami.Mujawar@arm.com;
> Alexei.Fedorov@arm.com
> Subject: Re: [PATCH] DynamicTablesPkg: Correct cluster index
>
> External email: Use caution opening links or attachments
>
>
> On Tue, 16 Aug 2022 at 22:46, Jeff Brasen <jbrasen@nvidia.com> wrote:
> >
> > Current code will generate duplicate UID if there are nested clusters
> > in the topology.
> >
>
> What is a nested cluster?
>
Better terminology would be nested processor containers, in our case we have socket/cluster/cpu. I'll update the commit message. Should I also change CreateAmlCluster to CreateAmlProcessorContainer?
> > Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
> > ---
> > .../SsdtCpuTopologyGenerator.c | 22 ++++++++++++-------
> > 1 file changed, 14 insertions(+), 8 deletions(-)
> >
> > diff --git
> >
> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCp
> uT
> > opologyGenerator.c
> >
> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCp
> uT
> > opologyGenerator.c
> > index 3266d8dd98..9295117f1f 100644
> > ---
> >
> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCp
> uT
> > opologyGenerator.c
> > +++
> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/Ssdt
> > +++ CpuTopologyGenerator.c
> > @@ -706,6 +706,8 @@ CreateAmlCluster (
> > Cannot be CM_NULL_TOKEN.
> > @param [in] ParentNode Parent node to attach the created
> > node to.
> > + @param [in,out] ClusterIndex Pointer to the current cluster index
> > + to be used as UID.
> >
> > @retval EFI_SUCCESS Success.
> > @retval EFI_INVALID_PARAMETER Invalid parameter.
> > @@ -718,13 +720,13 @@ CreateAmlCpuTopologyTree (
> > IN ACPI_CPU_TOPOLOGY_GENERATOR *Generator,
> > IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL *CONST
> CfgMgrProtocol,
> > IN CM_OBJECT_TOKEN NodeToken,
> > - IN AML_NODE_HANDLE ParentNode
> > + IN AML_NODE_HANDLE ParentNode,
> > + IN OUT UINT32 *ClusterIndex
> > )
> > {
> > EFI_STATUS Status;
> > UINT32 Index;
> > UINT32 CpuIndex;
> > - UINT32 ClusterIndex;
> > AML_OBJECT_NODE_HANDLE ClusterNode;
> >
> > ASSERT (Generator != NULL);
> > @@ -733,9 +735,9 @@ CreateAmlCpuTopologyTree (
> > ASSERT (CfgMgrProtocol != NULL);
> > ASSERT (NodeToken != CM_NULL_TOKEN);
> > ASSERT (ParentNode != NULL);
> > + ASSERT (ClusterIndex != NULL);
> >
> > - CpuIndex = 0;
> > - ClusterIndex = 0;
> > + CpuIndex = 0;
> >
> > for (Index = 0; Index < Generator->ProcNodeCount; Index++) {
> > // Find the children of the CM_ARM_PROC_HIERARCHY_INFO @@ -
> 790,7
> > +792,7 @@ CreateAmlCpuTopologyTree (
> > CfgMgrProtocol,
> > ParentNode,
> > &Generator->ProcNodeList[Index],
> > - ClusterIndex,
> > + *ClusterIndex,
> > &ClusterNode
> > );
> > if (EFI_ERROR (Status)) {
> > @@ -800,7 +802,7 @@ CreateAmlCpuTopologyTree (
> >
> > // Nodes must have a unique name in the ASL namespace.
> > // Reset the Cpu index whenever we create a new Cluster.
> > - ClusterIndex++;
> > + (*ClusterIndex)++;
> > CpuIndex = 0;
> >
> > // Recursively continue creating an AML tree.
> > @@ -808,7 +810,8 @@ CreateAmlCpuTopologyTree (
> > Generator,
> > CfgMgrProtocol,
> > Generator->ProcNodeList[Index].Token,
> > - ClusterNode
> > + ClusterNode,
> > + ClusterIndex
> > );
> > if (EFI_ERROR (Status)) {
> > ASSERT (0);
> > @@ -845,6 +848,7 @@ CreateTopologyFromProcHierarchy (
> > EFI_STATUS Status;
> > UINT32 Index;
> > UINT32 TopLevelProcNodeIndex;
> > + UINT32 ClusterIndex;
> >
> > ASSERT (Generator != NULL);
> > ASSERT (Generator->ProcNodeCount != 0); @@ -853,6 +857,7 @@
> > CreateTopologyFromProcHierarchy (
> > ASSERT (ScopeNode != NULL);
> >
> > TopLevelProcNodeIndex = MAX_UINT32;
> > + ClusterIndex = 0;
> >
> > Status = TokenTableInitialize (Generator, Generator->ProcNodeCount);
> > if (EFI_ERROR (Status)) {
> > @@ -887,7 +892,8 @@ CreateTopologyFromProcHierarchy (
> > Generator,
> > CfgMgrProtocol,
> > Generator->ProcNodeList[TopLevelProcNodeIndex].Token,
> > - ScopeNode
> > + ScopeNode,
> > + &ClusterIndex
> > );
> > if (EFI_ERROR (Status)) {
> > ASSERT (0);
> > --
> > 2.25.1
> >
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] DynamicTablesPkg: Correct cluster index
2022-08-17 16:14 ` Jeff Brasen
@ 2022-08-18 10:22 ` Ard Biesheuvel
2022-08-18 15:28 ` Sami Mujawar
0 siblings, 1 reply; 5+ messages in thread
From: Ard Biesheuvel @ 2022-08-18 10:22 UTC (permalink / raw)
To: Jeff Brasen
Cc: devel@edk2.groups.io, quic_llindhol@quicinc.com,
Sami.Mujawar@arm.com, Alexei.Fedorov@arm.com
On Wed, 17 Aug 2022 at 18:14, Jeff Brasen <jbrasen@nvidia.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Ard Biesheuvel <ardb@kernel.org>
> > Sent: Wednesday, August 17, 2022 1:01 AM
> > To: Jeff Brasen <jbrasen@nvidia.com>
> > Cc: devel@edk2.groups.io; quic_llindhol@quicinc.com;
> > ardb+tianocore@kernel.org; Sami.Mujawar@arm.com;
> > Alexei.Fedorov@arm.com
> > Subject: Re: [PATCH] DynamicTablesPkg: Correct cluster index
> >
> > External email: Use caution opening links or attachments
> >
> >
> > On Tue, 16 Aug 2022 at 22:46, Jeff Brasen <jbrasen@nvidia.com> wrote:
> > >
> > > Current code will generate duplicate UID if there are nested clusters
> > > in the topology.
> > >
> >
> > What is a nested cluster?
> >
>
> Better terminology would be nested processor containers, in our case we have socket/cluster/cpu. I'll update the commit message. Should I also change CreateAmlCluster to CreateAmlProcessorContainer?
>
Works for me, but Sami has the final call as he maintains this package.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] DynamicTablesPkg: Correct cluster index
2022-08-18 10:22 ` Ard Biesheuvel
@ 2022-08-18 15:28 ` Sami Mujawar
0 siblings, 0 replies; 5+ messages in thread
From: Sami Mujawar @ 2022-08-18 15:28 UTC (permalink / raw)
To: Ard Biesheuvel, Jeff Brasen
Cc: devel@edk2.groups.io, quic_llindhol@quicinc.com,
Alexei.Fedorov@arm.com, nd@arm.com
Hi Jeff,
Thank you for this patch.
On 18/08/2022 11:22 am, Ard Biesheuvel wrote:
> On Wed, 17 Aug 2022 at 18:14, Jeff Brasen <jbrasen@nvidia.com> wrote:
>>
>>
>>> -----Original Message-----
>>> From: Ard Biesheuvel <ardb@kernel.org>
>>> Sent: Wednesday, August 17, 2022 1:01 AM
>>> To: Jeff Brasen <jbrasen@nvidia.com>
>>> Cc: devel@edk2.groups.io; quic_llindhol@quicinc.com;
>>> ardb+tianocore@kernel.org; Sami.Mujawar@arm.com;
>>> Alexei.Fedorov@arm.com
>>> Subject: Re: [PATCH] DynamicTablesPkg: Correct cluster index
>>>
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On Tue, 16 Aug 2022 at 22:46, Jeff Brasen <jbrasen@nvidia.com> wrote:
>>>> Current code will generate duplicate UID if there are nested clusters
>>>> in the topology.
>>>>
>>> What is a nested cluster?
>>>
>> Better terminology would be nested processor containers, in our case we have socket/cluster/cpu. I'll update the commit message. Should I also change CreateAmlCluster to CreateAmlProcessorContainer?
>>
> Works for me, but Sami has the final call as he maintains this package.
[SAMI] CreateAmlProcessorContainer is more appropriate. Please change
CreateAmlCluster to CreateAmlProcessorContainer.
Regards,
Sami Mujawar
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-08-18 15:29 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-08-16 20:45 [PATCH] DynamicTablesPkg: Correct cluster index Jeff Brasen
2022-08-17 7:01 ` Ard Biesheuvel
2022-08-17 16:14 ` Jeff Brasen
2022-08-18 10:22 ` Ard Biesheuvel
2022-08-18 15:28 ` Sami Mujawar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox