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

["Jeshua Smith via groups.io" <jeshuas=nvidia.com@groups.io>]

Can we get this reviewed/merged?

> -----Original Message-----
> From: Jeshua Smith <jeshuas@nvidia.com>
> Sent: Monday, February 5, 2024 12:01 PM
> To: devel@edk2.groups.io
> Cc: ardb+tianocore@kernel.org; quic_llindhol@quicinc.com;
> pierre.gondois@arm.com; Sami.Mujawar@arm.com; Jeshua Smith
> <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>
> Reviewed-by: Pierre Gondois <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 (#116689): https://edk2.groups.io/g/devel/message/116689
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]
-=-=-=-=-=-=-=-=-=-=-=-