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.gond...@arm.com>
> Sent: Friday, February 3, 2023 6:11 AM
> To: Jeff Brasen <jbra...@nvidia.com>; devel@edk2.groups.io
> Cc: sami.muja...@arm.com; alexei.fedo...@arm.com;
> quic_llind...@quicinc.com; ardb+tianoc...@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.gond...@arm.com>
> >> Sent: Thursday, February 2, 2023 10:49 AM
> >> To: Jeff Brasen <jbra...@nvidia.com>; devel@edk2.groups.io
> >> Cc: sami.muja...@arm.com; alexei.fedo...@arm.com;
> >> quic_llind...@quicinc.com; ardb+tianoc...@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.gond...@arm.com>
> >>>> Sent: Thursday, February 2, 2023 5:03 AM
> >>>> To: Jeff Brasen <jbra...@nvidia.com>; devel@edk2.groups.io
> >>>> Cc: sami.muja...@arm.com; alexei.fedo...@arm.com;
> >>>> quic_llind...@quicinc.com; ardb+tianoc...@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 <jbra...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
