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

["PierreGondois" <pierre.gondois@arm.com>]

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