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

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

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