[v1] DynamicTablesPkg: Allow multiple top level physical nodes

[edk2-devel] [PATCH] DynamicTablesPkg: Allow multiple top level physical nodes

Posted by Jeff Brasen via groups.io 1 year, 2 months ago

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 <jbrasen@nvidia.com>

---

 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|BOOLEAN|0x40000009

 

+  # Force top level container for single socket devices

+  gEdkiiDynamicTablesPkgTokenSpaceGuid.PcdForceTopLevelProcessorContainer|FALSE|BOOLEAN|0x4000000A

+

 [Guids]

   gEdkiiDynamicTablesPkgTokenSpaceGuid = { 0xab226e66, 0x31d8, 0x4613, { 0x87, 0x9d, 0xd2, 0xfa, 0xb6, 0x10, 0x26, 0x3c } }

diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c

index c24da8ec71..58f86ff508 100644

--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c

+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c

@@ -22,6 +22,7 @@

 #include <Library/AcpiHelperLib.h>

 #include <Library/TableHelperLib.h>

 #include <Library/AmlLib/AmlLib.h>

+#include <Library/PcdLib.h>

 #include <Protocol/ConfigurationManagerProtocol.h>

 

 #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;

+        }

+

         // 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

 

   return Status;

 }

diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyLibArm.inf b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyLibArm.inf

index 3e2d154749..00adfe986f 100644

--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyLibArm.inf

+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyLibArm.inf

@@ -31,3 +31,7 @@

   AcpiHelperLib

   AmlLib

   BaseLib

+  PcdLib

+

+[Pcd]

+  gEdkiiDynamicTablesPkgTokenSpaceGuid.PcdForceTopLevelProcessorContainer

-- 

2.25.1





-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99410): https://edk2.groups.io/g/devel/message/99410
Mute This Topic: https://groups.io/mt/96680589/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] DynamicTablesPkg: Allow multiple top level physical nodes

Posted by PierreGondois 1 year, 2 months ago

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 <jbrasen@nvidia.com>
> ---
>   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|BOOLEAN|0x40000009
>   
> +  # Force top level container for single socket devices
> +  gEdkiiDynamicTablesPkgTokenSpaceGuid.PcdForceTopLevelProcessorContainer|FALSE|BOOLEAN|0x4000000A
> +
>   [Guids]
>     gEdkiiDynamicTablesPkgTokenSpaceGuid = { 0xab226e66, 0x31d8, 0x4613, { 0x87, 0x9d, 0xd2, 0xfa, 0xb6, 0x10, 0x26, 0x3c } }
> diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
> index c24da8ec71..58f86ff508 100644
> --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
> +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
> @@ -22,6 +22,7 @@
>   #include <Library/AcpiHelperLib.h>
>   #include <Library/TableHelperLib.h>
>   #include <Library/AmlLib/AmlLib.h>
> +#include <Library/PcdLib.h>
>   #include <Protocol/ConfigurationManagerProtocol.h>
>   
>   #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/SsdtCpuTopologyLibArm.inf b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyLibArm.inf
> index 3e2d154749..00adfe986f 100644
> --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyLibArm.inf
> +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyLibArm.inf
> @@ -31,3 +31,7 @@
>     AcpiHelperLib
>     AmlLib
>     BaseLib
> +  PcdLib
> +
> +[Pcd]
> +  gEdkiiDynamicTablesPkgTokenSpaceGuid.PcdForceTopLevelProcessorContainer


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99471): https://edk2.groups.io/g/devel/message/99471
Mute This Topic: https://groups.io/mt/96680589/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] DynamicTablesPkg: Allow multiple top level physical nodes

Posted by Jeff Brasen via groups.io 1 year, 2 months ago

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 <pierre.gondois@arm.com>
> Sent: Thursday, February 2, 2023 5:03 AM
> To: Jeff Brasen <jbrasen@nvidia.com>; 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 <jbrasen@nvidia.com>
> > ---
> >   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 <Library/AcpiHelperLib.h>
> >   #include <Library/TableHelperLib.h>
> >   #include <Library/AmlLib/AmlLib.h>
> > +#include <Library/PcdLib.h>
> >   #include <Protocol/ConfigurationManagerProtocol.h>
> >
> >   #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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99489): https://edk2.groups.io/g/devel/message/99489
Mute This Topic: https://groups.io/mt/96680589/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] DynamicTablesPkg: Allow multiple top level physical nodes

Posted by PierreGondois 1 year, 2 months ago

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 <pierre.gondois@arm.com>
>> Sent: Thursday, February 2, 2023 5:03 AM
>> To: Jeff Brasen <jbrasen@nvidia.com>; 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 <jbrasen@nvidia.com>
>>> ---
>>>    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 <Library/AcpiHelperLib.h>
>>>    #include <Library/TableHelperLib.h>
>>>    #include <Library/AmlLib/AmlLib.h>
>>> +#include <Library/PcdLib.h>
>>>    #include <Protocol/ConfigurationManagerProtocol.h>
>>>
>>>    #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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99491): https://edk2.groups.io/g/devel/message/99491
Mute This Topic: https://groups.io/mt/96680589/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] DynamicTablesPkg: Allow multiple top level physical nodes

Posted by Jeff Brasen via groups.io 1 year, 2 months ago

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

-Jeff

> -----Original Message-----
> From: Pierre Gondois <pierre.gondois@arm.com>
> Sent: Thursday, February 2, 2023 10:49 AM
> To: Jeff Brasen <jbrasen@nvidia.com>; 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 <pierre.gondois@arm.com>
> >> Sent: Thursday, February 2, 2023 5:03 AM
> >> To: Jeff Brasen <jbrasen@nvidia.com>; 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 <jbrasen@nvidia.com>
> >>> ---
> >>>    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 <Library/AcpiHelperLib.h>
> >>>    #include <Library/TableHelperLib.h>
> >>>    #include <Library/AmlLib/AmlLib.h>
> >>> +#include <Library/PcdLib.h>
> >>>    #include <Protocol/ConfigurationManagerProtocol.h>
> >>>
> >>>    #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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99492): https://edk2.groups.io/g/devel/message/99492
Mute This Topic: https://groups.io/mt/96680589/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] DynamicTablesPkg: Allow multiple top level physical nodes

Posted by PierreGondois 1 year, 2 months ago

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 <pierre.gondois@arm.com>
>> Sent: Thursday, February 2, 2023 10:49 AM
>> To: Jeff Brasen <jbrasen@nvidia.com>; 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 <pierre.gondois@arm.com>
>>>> Sent: Thursday, February 2, 2023 5:03 AM
>>>> To: Jeff Brasen <jbrasen@nvidia.com>; 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 <jbrasen@nvidia.com>
>>>>> ---
>>>>>     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 <Library/AcpiHelperLib.h>
>>>>>     #include <Library/TableHelperLib.h>
>>>>>     #include <Library/AmlLib/AmlLib.h>
>>>>> +#include <Library/PcdLib.h>
>>>>>     #include <Protocol/ConfigurationManagerProtocol.h>
>>>>>
>>>>>     #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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99582): https://edk2.groups.io/g/devel/message/99582
Mute This Topic: https://groups.io/mt/96680589/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] DynamicTablesPkg: Allow multiple top level physical nodes

Posted by Jeff Brasen via groups.io 1 year, 2 months ago

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.

-Jeff


> -----Original Message-----
> From: Pierre Gondois <pierre.gondois@arm.com>
> Sent: Friday, February 3, 2023 6:11 AM
> To: Jeff Brasen <jbrasen@nvidia.com>; 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 <pierre.gondois@arm.com>
> >> Sent: Thursday, February 2, 2023 10:49 AM
> >> To: Jeff Brasen <jbrasen@nvidia.com>; 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 <pierre.gondois@arm.com>
> >>>> Sent: Thursday, February 2, 2023 5:03 AM
> >>>> To: Jeff Brasen <jbrasen@nvidia.com>; 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 <jbrasen@nvidia.com>
> >>>>> ---
> >>>>>     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 <Library/AcpiHelperLib.h>
> >>>>>     #include <Library/TableHelperLib.h>
> >>>>>     #include <Library/AmlLib/AmlLib.h>
> >>>>> +#include <Library/PcdLib.h>
> >>>>>     #include <Protocol/ConfigurationManagerProtocol.h>
> >>>>>
> >>>>>     #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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99609): https://edk2.groups.io/g/devel/message/99609
Mute This Topic: https://groups.io/mt/96680589/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] DynamicTablesPkg: Allow multiple top level physical nodes

Posted by PierreGondois 1 year, 2 months ago


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 <pierre.gondois@arm.com>
>> Sent: Friday, February 3, 2023 6:11 AM
>> To: Jeff Brasen <jbrasen@nvidia.com>; 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 <pierre.gondois@arm.com>
>>>> Sent: Thursday, February 2, 2023 10:49 AM
>>>> To: Jeff Brasen <jbrasen@nvidia.com>; 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 <pierre.gondois@arm.com>
>>>>>> Sent: Thursday, February 2, 2023 5:03 AM
>>>>>> To: Jeff Brasen <jbrasen@nvidia.com>; 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 <jbrasen@nvidia.com>
>>>>>>> ---
>>>>>>>      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 <Library/AcpiHelperLib.h>
>>>>>>>      #include <Library/TableHelperLib.h>
>>>>>>>      #include <Library/AmlLib/AmlLib.h>
>>>>>>> +#include <Library/PcdLib.h>
>>>>>>>      #include <Protocol/ConfigurationManagerProtocol.h>
>>>>>>>
>>>>>>>      #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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99610): https://edk2.groups.io/g/devel/message/99610
Mute This Topic: https://groups.io/mt/96680589/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] DynamicTablesPkg: Allow multiple top level physical nodes

Posted by Jeff Brasen via groups.io 1 year, 2 months ago

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

> -----Original Message-----
> From: Pierre Gondois <pierre.gondois@arm.com>
> Sent: Friday, February 3, 2023 9:28 AM
> To: Jeff Brasen <jbrasen@nvidia.com>; 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 <pierre.gondois@arm.com>
> >> Sent: Friday, February 3, 2023 6:11 AM
> >> To: Jeff Brasen <jbrasen@nvidia.com>; 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 <pierre.gondois@arm.com>
> >>>> Sent: Thursday, February 2, 2023 10:49 AM
> >>>> To: Jeff Brasen <jbrasen@nvidia.com>; 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 <pierre.gondois@arm.com>
> >>>>>> Sent: Thursday, February 2, 2023 5:03 AM
> >>>>>> To: Jeff Brasen <jbrasen@nvidia.com>; 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 <jbrasen@nvidia.com>
> >>>>>>> ---
> >>>>>>>      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 <Library/AcpiHelperLib.h>
> >>>>>>>      #include <Library/TableHelperLib.h>
> >>>>>>>      #include <Library/AmlLib/AmlLib.h>
> >>>>>>> +#include <Library/PcdLib.h>
> >>>>>>>      #include <Protocol/ConfigurationManagerProtocol.h>
> >>>>>>>
> >>>>>>>      #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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99612): https://edk2.groups.io/g/devel/message/99612
Mute This Topic: https://groups.io/mt/96680589/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] DynamicTablesPkg: Allow multiple top level physical nodes

Posted by PierreGondois 1 year, 2 months ago


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 <pierre.gondois@arm.com>
>> Sent: Friday, February 3, 2023 9:28 AM
>> To: Jeff Brasen <jbrasen@nvidia.com>; 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 <pierre.gondois@arm.com>
>>>> Sent: Friday, February 3, 2023 6:11 AM
>>>> To: Jeff Brasen <jbrasen@nvidia.com>; 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 <pierre.gondois@arm.com>
>>>>>> Sent: Thursday, February 2, 2023 10:49 AM
>>>>>> To: Jeff Brasen <jbrasen@nvidia.com>; 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 <pierre.gondois@arm.com>
>>>>>>>> Sent: Thursday, February 2, 2023 5:03 AM
>>>>>>>> To: Jeff Brasen <jbrasen@nvidia.com>; 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 <jbrasen@nvidia.com>
>>>>>>>>> ---
>>>>>>>>>       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 <Library/AcpiHelperLib.h>
>>>>>>>>>       #include <Library/TableHelperLib.h>
>>>>>>>>>       #include <Library/AmlLib/AmlLib.h>
>>>>>>>>> +#include <Library/PcdLib.h>
>>>>>>>>>       #include <Protocol/ConfigurationManagerProtocol.h>
>>>>>>>>>
>>>>>>>>>       #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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99654): https://edk2.groups.io/g/devel/message/99654
Mute This Topic: https://groups.io/mt/96680589/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-