[edk2-devel] [PATCH] DynamicTablesPkg: Allow multiple top level physical nodes
PierreGondois
pierre.gondois at arm.com
Mon Feb 6 09:27:23 UTC 2023
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 at arm.com>
>> Sent: Friday, February 3, 2023 9:28 AM
>> To: Jeff Brasen <jbrasen at nvidia.com>; devel at edk2.groups.io
>> Cc: Sami.Mujawar at arm.com; Alexei.Fedorov at arm.com;
>> quic_llindhol at quicinc.com; ardb+tianocore at 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 at arm.com>
>>>> Sent: Friday, February 3, 2023 6:11 AM
>>>> To: Jeff Brasen <jbrasen at nvidia.com>; devel at edk2.groups.io
>>>> Cc: Sami.Mujawar at arm.com; Alexei.Fedorov at arm.com;
>>>> quic_llindhol at quicinc.com; ardb+tianocore at 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 at arm.com>
>>>>>> Sent: Thursday, February 2, 2023 10:49 AM
>>>>>> To: Jeff Brasen <jbrasen at nvidia.com>; devel at edk2.groups.io
>>>>>> Cc: Sami.Mujawar at arm.com; Alexei.Fedorov at arm.com;
>>>>>> quic_llindhol at quicinc.com; ardb+tianocore at 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 at arm.com>
>>>>>>>> Sent: Thursday, February 2, 2023 5:03 AM
>>>>>>>> To: Jeff Brasen <jbrasen at nvidia.com>; devel at edk2.groups.io
>>>>>>>> Cc: Sami.Mujawar at arm.com; Alexei.Fedorov at arm.com;
>>>>>>>> quic_llindhol at quicinc.com; ardb+tianocore at 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 at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list