The changes on your branch seem pretty good to me > -----Original Message----- > From: Pierre Gondois <pierre.gond...@arm.com> > Sent: Monday, February 6, 2023 2:28 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 v2] DynamicTablesPkg: Allow multiple top level physical > nodes > > External email: Use caution opening links or attachments > > > Hello Jeff, > Thanks for the v2. Also cf the first discussion at: > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk > 2.groups.io%2Fg%2Fdevel%2Ftopic%2F96680589%2399612&data=05%7C01% > 7Cjbrasen%40nvidia.com%7Ccee1a4886a754ba2d28508db08246448%7C43083 > d15727340c1b7db39efd9ccc17a%7C0%7C0%7C638112724625353615%7CUnkn > own%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik > 1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=ur6vlVCBpt%2BQdid > 3xJKglx3trDZb4kxajkAWFXsr920%3D&reserved=0 > - I think it would be good to extract a function that does all the checks as > there are many possibilities for the flags/parent combinations. > - I think it would also be nice to reset the index of ProcContainers for each > new level (i.e. not to have the same incrementing index for > clusters/packages) > > I created a branch based on your work at: > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith > ub.com%2Fpierregondois%2Fedk2%2Ftree%2Fpg%2Ftop_level_pnode_Wip > &data=05%7C01%7Cjbrasen%40nvidia.com%7Ccee1a4886a754ba2d28508db0 > 8246448%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C63811272462 > 5353615%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV > 2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=Ydj > RbbboKTyVmi2rr2bvu0ARx9uey5mLYtWikbkP7Ek%3D&reserved=0 > > Regards, > Pierre > > On 2/3/23 19:10, 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 create a > > top level processor container for all systems. > > > > Signed-off-by: Jeff Brasen <jbra...@nvidia.com> > > --- > > .../SsdtCpuTopologyGenerator.c | 43 ++++++------------- > > 1 file changed, 12 insertions(+), 31 deletions(-) > > > > diff --git > > > a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCp > uT > > opologyGenerator.c > > > b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCp > uT > > opologyGenerator.c > > index c24da8ec71..46b757e0b2 100644 > > --- > > > a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCp > uT > > opologyGenerator.c > > +++ > b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/Ssdt > > +++ CpuTopologyGenerator.c > > @@ -814,7 +814,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 +842,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 +894,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; > > + } > > + > > I think that if (NodeToken == CM_NULL_TOKEN) and doesn't have the > Physical Package flag, no error will be triggered even though this is not a > valid > configuration. > > > // 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 (( > > @@ -974,8 +981,6 @@ CreateTopologyFromProcHierarchy ( > > ) > > { > > EFI_STATUS Status; > > - UINT32 Index; > > - UINT32 TopLevelProcNodeIndex; > > UINT32 ProcContainerIndex; > > > > ASSERT (Generator != NULL); > > @@ -984,8 +989,7 @@ CreateTopologyFromProcHierarchy ( > > ASSERT (CfgMgrProtocol != NULL); > > ASSERT (ScopeNode != NULL); > > > > - TopLevelProcNodeIndex = MAX_UINT32; > > - ProcContainerIndex = 0; > > + ProcContainerIndex = 0; > > > > Status = TokenTableInitialize (Generator, Generator->ProcNodeCount); > > if (EFI_ERROR (Status)) { > > @@ -993,33 +997,10 @@ 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; > > - } > > - > > - TopLevelProcNodeIndex = Index; > > - } > > - } // for > > - > > Status = CreateAmlCpuTopologyTree ( > > Generator, > > CfgMgrProtocol, > > - Generator->ProcNodeList[TopLevelProcNodeIndex].Token, > > + CM_NULL_TOKEN, > > ScopeNode, > > &ProcContainerIndex > > );
-=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#100115): https://edk2.groups.io/g/devel/message/100115 Mute This Topic: https://groups.io/mt/96729031/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
