From: "Jeff Brasen" <jbrasen@nvidia.com>
To: Pierre Gondois <pierre.gondois@arm.com>,
"devel@edk2.groups.io" <devel@edk2.groups.io>
Cc: "Sami.Mujawar@arm.com" <Sami.Mujawar@arm.com>,
"Alexei.Fedorov@arm.com" <Alexei.Fedorov@arm.com>,
"quic_llindhol@quicinc.com" <quic_llindhol@quicinc.com>,
"ardb+tianocore@kernel.org" <ardb+tianocore@kernel.org>
Subject: Re: [PATCH] DynamicTablesPkg: Allow multiple top level physical nodes
Date: Fri, 3 Feb 2023 16:38:04 +0000 [thread overview]
Message-ID: <DS7PR12MB57895AA6296E563687E36CD3CBD79@DS7PR12MB5789.namprd12.prod.outlook.com> (raw)
In-Reply-To: <435fe4ff-aa22-aa09-a1e0-3e3116decdce@arm.com>
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
next prev parent reply other threads:[~2023-02-03 16:38 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-01 16:42 [PATCH] DynamicTablesPkg: Allow multiple top level physical nodes Jeff Brasen
2023-02-02 12:02 ` PierreGondois
2023-02-02 16:48 ` Jeff Brasen
2023-02-02 17:48 ` PierreGondois
2023-02-02 17:53 ` Jeff Brasen
2023-02-03 13:11 ` PierreGondois
2023-02-03 16:00 ` Jeff Brasen
2023-02-03 16:28 ` PierreGondois
2023-02-03 16:38 ` Jeff Brasen [this message]
2023-02-06 9:27 ` PierreGondois
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=DS7PR12MB57895AA6296E563687E36CD3CBD79@DS7PR12MB5789.namprd12.prod.outlook.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox