Re: [edk2-devel] [PATCH v2] DynamicTablesPkg/SSDT: Require Package node in hierarchy

["Sami Mujawar" <sami.mujawar@arm.com>]

Hi Jeshua,

Apologies, I somehow missed getting this merged.
I will get this in before end of this week.

Regards,

Sami Mujawar

On 12/03/2024, 16:12, "Jeshua Smith" <jeshuas@nvidia.com <mailto:jeshuas@nvidia.com>> wrote:


Can we get this reviewed/merged?


> -----Original Message-----
> From: Jeshua Smith <jeshuas@nvidia.com <mailto:jeshuas@nvidia.com>>
> Sent: Monday, February 5, 2024 12:01 PM
> To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>
> Cc: ardb+tianocore@kernel.org <mailto:ardb+tianocore@kernel.org>; quic_llindhol@quicinc.com <mailto:quic_llindhol@quicinc.com>;
> pierre.gondois@arm.com <mailto:pierre.gondois@arm.com>; Sami.Mujawar@arm.com <mailto:Sami.Mujawar@arm.com>; Jeshua Smith
> <jeshuas@nvidia.com <mailto:jeshuas@nvidia.com>>
> Subject: [PATCH v2] DynamicTablesPkg/SSDT: Require Package node in
> hierarchy
> 
> The code was incorrectly assuming that root nodes had to be physical package
> nodes and vice versa. This is not always true, so update the check to simply
> require exactly one package node somewhere in the hierarchy.
> 
> Signed-off-by: Jeshua Smith <jeshuas@nvidia.com <mailto:jeshuas@nvidia.com>>
> Reviewed-by: Pierre Gondois <pierre.gondois@arm.com <mailto:pierre.gondois@arm.com>>
> ---
> Note: This is a complete replacement for [PATCH] DynamicTablesPkg/SSDT:
> Remove incorrect root node check
> 
> Version 2: added documentation for the PackageNodeSeen parameter
> 
> .../SsdtCpuTopologyGenerator.c | 32 +++++++++++++------
> 1 file changed, 22 insertions(+), 10 deletions(-)
> 
> diff --git
> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpu
> TopologyGenerator.c
> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpu
> TopologyGenerator.c
> index 9e3efb49e6..40ed10eae6 100644
> ---
> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpu
> TopologyGenerator.c
> +++
> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCp
> +++ uTopologyGenerator.c
> @@ -1072,6 +1072,7 @@ CreateAmlProcessorContainer (
> @param [in] IsLeaf The ProcNode is a leaf.
> @param [in] NodeToken NodeToken of the ProcNode.
> @param [in] ParentNodeToken Parent NodeToken of the ProcNode.
> + @param [in] PackageNodeSeen A parent of the ProcNode has the physical
> package flag set.
> 
> @retval EFI_SUCCESS Success.
> @retval EFI_INVALID_PARAMETER Invalid parameter.
> @@ -1083,23 +1084,24 @@ CheckProcNode (
> UINT32 NodeFlags,
> BOOLEAN IsLeaf,
> CM_OBJECT_TOKEN NodeToken,
> - CM_OBJECT_TOKEN ParentNodeToken
> + CM_OBJECT_TOKEN ParentNodeToken,
> + BOOLEAN PackageNodeSeen
> )
> {
> BOOLEAN InvalidFlags;
> BOOLEAN HasPhysicalPackageBit;
> - BOOLEAN IsTopLevelNode;
> 
> HasPhysicalPackageBit = (NodeFlags &
> EFI_ACPI_6_3_PPTT_PACKAGE_PHYSICAL) ==
> EFI_ACPI_6_3_PPTT_PACKAGE_PHYSICAL;
> - IsTopLevelNode = (ParentNodeToken == CM_NULL_TOKEN);
> 
> - // A top-level node is a Physical Package and conversely.
> - InvalidFlags = HasPhysicalPackageBit ^ IsTopLevelNode;
> + // Only one Physical Package flag is allowed in the hierarchy
> + InvalidFlags = HasPhysicalPackageBit && PackageNodeSeen;
> 
> // Check Leaf specific flags.
> if (IsLeaf) {
> InvalidFlags |= ((NodeFlags & PPTT_LEAF_MASK) != PPTT_LEAF_MASK);
> + // Must have Physical Package flag somewhere in the hierarchy
> + InvalidFlags |= !(HasPhysicalPackageBit || PackageNodeSeen);
> } else {
> InvalidFlags |= ((NodeFlags & PPTT_LEAF_MASK) != 0);
> }
> @@ -1130,6 +1132,7 @@ CheckProcNode (
> node to.
> @param [in,out] ProcContainerIndex Pointer to the current processor
> container
> index to be used as UID.
> + @param [in] PackageNodeSeen A parent of the ProcNode has the
> physical package flag set.
> 
> @retval EFI_SUCCESS Success.
> @retval EFI_INVALID_PARAMETER Invalid parameter.
> @@ -1143,7 +1146,8 @@ CreateAmlCpuTopologyTree (
> IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL *CONST
> CfgMgrProtocol,
> IN CM_OBJECT_TOKEN NodeToken,
> IN AML_NODE_HANDLE ParentNode,
> - IN OUT UINT32 *ProcContainerIndex
> + IN OUT UINT32 *ProcContainerIndex,
> + IN BOOLEAN PackageNodeSeen
> )
> {
> EFI_STATUS Status;
> @@ -1153,6 +1157,7 @@ CreateAmlCpuTopologyTree (
> AML_OBJECT_NODE_HANDLE ProcContainerNode;
> UINT32 Uid;
> UINT16 Name;
> + BOOLEAN HasPhysicalPackageBit;
> 
> ASSERT (Generator != NULL);
> ASSERT (Generator->ProcNodeList != NULL); @@ -1175,7 +1180,8 @@
> CreateAmlCpuTopologyTree (
> Generator->ProcNodeList[Index].Flags,
> TRUE,
> Generator->ProcNodeList[Index].Token,
> - NodeToken
> + NodeToken,
> + PackageNodeSeen
> );
> if (EFI_ERROR (Status)) {
> ASSERT (0);
> @@ -1208,7 +1214,8 @@ CreateAmlCpuTopologyTree (
> Generator->ProcNodeList[Index].Flags,
> FALSE,
> Generator->ProcNodeList[Index].Token,
> - NodeToken
> + NodeToken,
> + PackageNodeSeen
> );
> if (EFI_ERROR (Status)) {
> ASSERT (0);
> @@ -1249,13 +1256,17 @@ CreateAmlCpuTopologyTree (
> ProcContainerName++;
> }
> 
> + HasPhysicalPackageBit = (Generator->ProcNodeList[Index].Flags &
> EFI_ACPI_6_3_PPTT_PACKAGE_PHYSICAL) ==
> + EFI_ACPI_6_3_PPTT_PACKAGE_PHYSICAL;
> +
> // Recursively continue creating an AML tree.
> Status = CreateAmlCpuTopologyTree (
> Generator,
> CfgMgrProtocol,
> Generator->ProcNodeList[Index].Token,
> ProcContainerNode,
> - ProcContainerIndex
> + ProcContainerIndex,
> + (PackageNodeSeen || HasPhysicalPackageBit)
> );
> if (EFI_ERROR (Status)) {
> ASSERT (0);
> @@ -1311,7 +1322,8 @@ CreateTopologyFromProcHierarchy (
> CfgMgrProtocol,
> CM_NULL_TOKEN,
> ScopeNode,
> - &ProcContainerIndex
> + &ProcContainerIndex,
> + FALSE
> );
> if (EFI_ERROR (Status)) {
> ASSERT (0);
> --
> 2.34.1







-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116690): https://edk2.groups.io/g/devel/message/116690
Mute This Topic: https://groups.io/mt/104183075/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-