Re: [PATCH 1/1] DynamicTablesPkg/SsdtCpuTopology: Allow multi-packages topologies

["Jeff Brasen" <jbrasen@nvidia.com>]

Reviewed-by: Jeff Brasen <jbrasen@nvidia.com>

> -----Original Message-----
> From: pierre.gondois@arm.com <pierre.gondois@arm.com>
> Sent: Thursday, March 9, 2023 8:33 AM
> To: devel@edk2.groups.io
> Cc: Sami Mujawar <sami.mujawar@arm.com>; Alexei Fedorov
> <Alexei.Fedorov@arm.com>; Jeff Brasen <jbrasen@nvidia.com>
> Subject: [PATCH 1/1] DynamicTablesPkg/SsdtCpuTopology: Allow multi-
> packages topologies
> 
> External email: Use caution opening links or attachments
> 
> 
> From: Pierre Gondois <pierre.gondois@arm.com>
> 
> The topology of a platform is represented in ACPI using the PPTT table. It is
> possible to append information to CPUs/processor containers using their
> associated AML nodes in a SSDT table.
> A platform might have multiple 'physical packages' (or top-level
> nodes) in their PPTT topology representation. It can be assumed from [1]
> that a 'physical packages' is always a 'top-level node', and conversely.
> 
> The SSDT topology generator doesn't support having multiple top-level
> nodes. The top-level node is also not generated in the SSDT topology
> representation.
> Add support to generate multiple top-level nodes in the SSDT topology
> generator and generate an AML node for this top-level node. This will allow
> to have matching PPTT and SSDT topology representations. Prior to this
> patch, this top-level AML node was not generated.
> 
> Also factorize the flag checking in CheckProcNode() and add more checks.
> 
> This patch takes inspiration from the discussion at:
> - v1:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk
> 2.groups.io%2Fg%2Fdevel%2Fmessage%2F99410&data=05%7C01%7Cjbrasen
> %40nvidia.com%7C0f68dac1cbf245e37eb608db20b39c00%7C43083d15727340
> c1b7db39efd9ccc17a%7C0%7C0%7C638139728016125186%7CUnknown%7CT
> WFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLC
> JXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=tqQD5RycnMUh%2BuqTG%2F
> %2BJW9fK7gw1KZwA%2BMNoi55IS%2BY%3D&reserved=0
> - v2:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk
> 2.groups.io%2Fg%2Fdevel%2Fmessage%2F99615&data=05%7C01%7Cjbrasen
> %40nvidia.com%7C0f68dac1cbf245e37eb608db20b39c00%7C43083d15727340
> c1b7db39efd9ccc17a%7C0%7C0%7C638139728016125186%7CUnknown%7CT
> WFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLC
> JXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=dG6YaTCQSThzTG47yZPRU7lCZ
> ADzeFFmy6fDFKZInwI%3D&reserved=0
> 
> [1]
> 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.""
> - "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."
> 
> Change-Id: I48452e623906628f44b7e2c69a34ed7b30276e92
> Suggested-by: Jeff Brasen <jbrasen@nvidia.com>
> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
> ---
>  .../SsdtCpuTopologyGenerator.c                | 131 +++++++++++-------
>  .../SsdtCpuTopologyGenerator.h                |   4 +
>  2 files changed, 84 insertions(+), 51 deletions(-)
> 
> diff --git
> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCp
> uTopologyGenerator.c
> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCp
> uTopologyGenerator.c
> index c24da8ec71ad..6fb131b66482 100644
> ---
> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCp
> uTopologyGenerator.c
> +++
> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCp
> +++ uTopologyGenerator.c
> @@ -805,6 +805,57 @@ CreateAmlProcessorContainer (
>    return Status;
>  }
> 
> +/** Check flags and topology of a ProcNode.
> +
> +  @param [in]  NodeFlags        Flags of the ProcNode to check.
> +  @param [in]  IsLeaf           The ProcNode is a leaf.
> +  @param [in]  NodeToken        NodeToken of the ProcNode.
> +  @param [in]  ParentNodeToken  Parent NodeToken of the ProcNode.
> +
> +  @retval EFI_SUCCESS             Success.
> +  @retval EFI_INVALID_PARAMETER   Invalid parameter.
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +CheckProcNode (
> +  UINT32           NodeFlags,
> +  BOOLEAN          IsLeaf,
> +  CM_OBJECT_TOKEN  NodeToken,
> +  CM_OBJECT_TOKEN  ParentNodeToken
> +  )
> +{
> +  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;
> +
> +  // Check Leaf specific flags.
> +  if (IsLeaf) {
> +    InvalidFlags |= ((NodeFlags & PPTT_LEAF_MASK) != PPTT_LEAF_MASK);
> + } else {
> +    InvalidFlags |= ((NodeFlags & PPTT_LEAF_MASK) != 0);  }
> +
> +  if (InvalidFlags) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "ERROR: SSDT-CPU-TOPOLOGY: Invalid flags for ProcNode: 0x%p.\n",
> +      (VOID *)NodeToken
> +      ));
> +    ASSERT (0);
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
>  /** Create an AML representation of the Cpu topology.
> 
>    A processor container is by extension any non-leave device in the cpu
> topology.
> @@ -814,7 +865,6 @@ CreateAmlProcessorContainer (
>                                        Protocol Interface.
>    @param [in] NodeToken               Token of the
> CM_ARM_PROC_HIERARCHY_INFO
>                                        currently handled.
> -                                      Cannot be CM_NULL_TOKEN.
>    @param [in] ParentNode              Parent node to attach the created
>                                        node to.
>    @param [in,out] ProcContainerIndex  Pointer to the current processor
> container @@ -838,6 +888,7 @@ CreateAmlCpuTopologyTree (
>    EFI_STATUS              Status;
>    UINT32                  Index;
>    UINT32                  CpuIndex;
> +  UINT32                  ProcContainerName;
>    AML_OBJECT_NODE_HANDLE  ProcContainerNode;
>    UINT32                  Uid;
>    UINT16                  Name;
> @@ -846,11 +897,11 @@ CreateAmlCpuTopologyTree (
>    ASSERT (Generator->ProcNodeList != NULL);
>    ASSERT (Generator->ProcNodeCount != 0);
>    ASSERT (CfgMgrProtocol != NULL);
> -  ASSERT (NodeToken != CM_NULL_TOKEN);
>    ASSERT (ParentNode != NULL);
>    ASSERT (ProcContainerIndex != NULL);
> 
> -  CpuIndex = 0;
> +  CpuIndex          = 0;
> +  ProcContainerName = 0;
> 
>    for (Index = 0; Index < Generator->ProcNodeCount; Index++) {
>      // Find the children of the CM_ARM_PROC_HIERARCHY_INFO @@ -859,16
> +910,15 @@ CreateAmlCpuTopologyTree (
>        // Only Cpus (leaf nodes in this tree) have a GicCToken.
>        // Create a Cpu node.
>        if (Generator->ProcNodeList[Index].GicCToken != CM_NULL_TOKEN) {
> -        if ((Generator->ProcNodeList[Index].Flags & PPTT_PROCESSOR_MASK)
> !=
> -            PPTT_CPU_PROCESSOR_MASK)
> -        {
> -          DEBUG ((
> -            DEBUG_ERROR,
> -            "ERROR: SSDT-CPU-TOPOLOGY: Invalid flags for cpu: 0x%x.\n",
> -            Generator->ProcNodeList[Index].Flags
> -            ));
> +        Status = CheckProcNode (
> +                   Generator->ProcNodeList[Index].Flags,
> +                   TRUE,
> +                   Generator->ProcNodeList[Index].Token,
> +                   NodeToken
> +                   );
> +        if (EFI_ERROR (Status)) {
>            ASSERT (0);
> -          return EFI_INVALID_PARAMETER;
> +          return Status;
>          }
> 
>          if (Generator->ProcNodeList[Index].OverrideNameUidEnabled) { @@ -
> 893,24 +943,22 @@ CreateAmlCpuTopologyTree (
>        } else {
>          // If this is not a Cpu, then this is a processor container.
> 
> -        // Acpi processor Id for clusters is not handled.
> -        if ((Generator->ProcNodeList[Index].Flags & PPTT_PROCESSOR_MASK)
> !=
> -            PPTT_CLUSTER_PROCESSOR_MASK)
> -        {
> -          DEBUG ((
> -            DEBUG_ERROR,
> -            "ERROR: SSDT-CPU-TOPOLOGY: Invalid flags for cluster: 0x%x.\n",
> -            Generator->ProcNodeList[Index].Flags
> -            ));
> +        Status = CheckProcNode (
> +                   Generator->ProcNodeList[Index].Flags,
> +                   FALSE,
> +                   Generator->ProcNodeList[Index].Token,
> +                   NodeToken
> +                   );
> +        if (EFI_ERROR (Status)) {
>            ASSERT (0);
> -          return EFI_INVALID_PARAMETER;
> +          return Status;
>          }
> 
>          if (Generator->ProcNodeList[Index].OverrideNameUidEnabled) {
>            Name = Generator->ProcNodeList[Index].OverrideName;
>            Uid  = Generator->ProcNodeList[Index].OverrideUid;
>          } else {
> -          Name = *ProcContainerIndex;
> +          Name = ProcContainerName;
>            Uid  = *ProcContainerIndex;
>          }
> 
> @@ -933,6 +981,13 @@ CreateAmlCpuTopologyTree (
>          (*ProcContainerIndex)++;
>          CpuIndex = 0;
> 
> +        // And reset the cluster name whenever there is a package.
> +        if (NodeToken == CM_NULL_TOKEN) {
> +          ProcContainerName = 0;
> +        } else {
> +          ProcContainerName++;
> +        }
> +
>          // Recursively continue creating an AML tree.
>          Status = CreateAmlCpuTopologyTree (
>                     Generator,
> @@ -974,8 +1029,6 @@ CreateTopologyFromProcHierarchy (
>    )
>  {
>    EFI_STATUS  Status;
> -  UINT32      Index;
> -  UINT32      TopLevelProcNodeIndex;
>    UINT32      ProcContainerIndex;
> 
>    ASSERT (Generator != NULL);
> @@ -984,8 +1037,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 +1045,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
>               );
> diff --git
> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCp
> uTopologyGenerator.h
> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCp
> uTopologyGenerator.h
> index f174d9c2e2cb..48e4455490e9 100644
> ---
> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCp
> uTopologyGenerator.h
> +++
> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCp
> +++ uTopologyGenerator.h
> @@ -34,6 +34,10 @@
>            (EFI_ACPI_6_3_PPTT_PROCESSOR_ID_INVALID << 1) |                     \
>            (EFI_ACPI_6_3_PPTT_NODE_IS_NOT_LEAF << 3))
> 
> +// Leaf nodes specific mask.
> +#define PPTT_LEAF_MASK  ((EFI_ACPI_6_3_PPTT_PROCESSOR_ID_VALID
> << 1) |        \
> +                         (EFI_ACPI_6_3_PPTT_NODE_IS_LEAF << 3))
> +
>  /** LPI states are stored in the ASL namespace at '\_SB_.Lxxx',
>      with xxx being the node index of the LPI state.
>  */
> --
> 2.25.1