public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH] DynamicTablesPkg: Allow for specified CPU names
@ 2022-11-07 15:57 Jeff Brasen
  2022-11-07 16:06 ` Jeff Brasen
  2022-11-09 12:58 ` PierreGondois
  0 siblings, 2 replies; 5+ messages in thread
From: Jeff Brasen @ 2022-11-07 15:57 UTC (permalink / raw)
  To: devel
  Cc: Sami.Mujawar, Alexei.Fedorov, pierre.gondois, quic_llindhol,
	ardb+tianocore, Jeff Brasen

Allow object to specify the name of processor and processor container
nodes and the UID of processor containers.

This allows these to be more accurately referenced from other tables.
For example for the _PSL method or the UID in the APMT table.

The UID and Name for processor container may be different as if the
intention is to set names as the corresponding affinity level the UID
may need to be different if there are multiple levels of containers.

Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
---
 .../Include/ArmNameSpaceObjects.h             | 11 +++++
 .../SsdtCpuTopologyGenerator.c                | 40 ++++++++++++++-----
 .../ConfigurationManagerObjectParser.c        |  3 ++
 3 files changed, 43 insertions(+), 11 deletions(-)

diff --git a/DynamicTablesPkg/Include/ArmNameSpaceObjects.h b/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
index 6aafd41a2e..19098609de 100644
--- a/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
+++ b/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
@@ -768,6 +768,17 @@ typedef struct CmArmProcHierarchyInfo {
   /// Token identifying a CM_ARM_OBJ_REF structure, itself referencing
   /// CM_ARM_LPI_INFO objects.
   CM_OBJECT_TOKEN    LpiToken;
+  /// Set to TRUE if UID should override index for name and _UID
+  /// for processor container nodes and name of processors.
+  /// This should be consistently set for containers or processors to avoid
+  /// duplicate values
+  BOOLEAN            OverrideNameUidEnabled;
+  /// If OverrideNameUidEnabled is TRUE then this value will be used for name of
+  /// processors and processor containers.
+  UINT16             OverrideName;
+  /// If OverrideNameUidEnabled is TRUE then this value will be used for
+  /// the UID of processor containers.
+  UINT32             OverrideUid;
 } CM_ARM_PROC_HIERARCHY_INFO;
 
 /** A structure that describes the Cache Type Structure (Type 1) in PPTT
diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
index d06c7615fb..92fa904103 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
@@ -553,7 +553,7 @@ GenerateLpiStates (
   @param [in]  Generator    The SSDT Cpu Topology generator.
   @param [in]  ParentNode   Parent node to attach the Cpu node to.
   @param [in]  GicCInfo     CM_ARM_GICC_INFO object used to create the node.
-  @param [in]  CpuIndex     Index used to generate the node name.
+  @param [in]  CpuName      Value used to generate the node name.
   @param [out] CpuNodePtr   If not NULL, return the created Cpu node.
 
   @retval EFI_SUCCESS             Success.
@@ -567,7 +567,7 @@ CreateAmlCpu (
   IN   ACPI_CPU_TOPOLOGY_GENERATOR  *Generator,
   IN   AML_NODE_HANDLE              ParentNode,
   IN   CM_ARM_GICC_INFO             *GicCInfo,
-  IN   UINT32                       CpuIndex,
+  IN   UINT32                       CpuName,
   OUT  AML_OBJECT_NODE_HANDLE       *CpuNodePtr OPTIONAL
   )
 {
@@ -579,7 +579,7 @@ CreateAmlCpu (
   ASSERT (ParentNode != NULL);
   ASSERT (GicCInfo != NULL);
 
-  Status = WriteAslName ('C', CpuIndex, AslName);
+  Status = WriteAslName ('C', CpuName, AslName);
   if (EFI_ERROR (Status)) {
     ASSERT (0);
     return Status;
@@ -628,7 +628,7 @@ CreateAmlCpu (
   @param [in]  CfgMgrProtocol         Pointer to the Configuration Manager
                                       Protocol Interface.
   @param [in]  ParentNode             Parent node to attach the Cpu node to.
-  @param [in]  CpuIndex               Index used to generate the node name.
+  @param [in]  CpuName                Value used to generate the node name.
   @param [in]  ProcHierarchyNodeInfo  CM_ARM_PROC_HIERARCHY_INFO describing
                                       the Cpu.
 
@@ -643,7 +643,7 @@ CreateAmlCpuFromProcHierarchy (
   IN        ACPI_CPU_TOPOLOGY_GENERATOR                   *Generator,
   IN  CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  *CONST  CfgMgrProtocol,
   IN        AML_NODE_HANDLE                               ParentNode,
-  IN        UINT32                                        CpuIndex,
+  IN        UINT32                                        CpuName,
   IN        CM_ARM_PROC_HIERARCHY_INFO                    *ProcHierarchyNodeInfo
   )
 {
@@ -668,7 +668,7 @@ CreateAmlCpuFromProcHierarchy (
     return Status;
   }
 
-  Status = CreateAmlCpu (Generator, ParentNode, GicCInfo, CpuIndex, &CpuNode);
+  Status = CreateAmlCpu (Generator, ParentNode, GicCInfo, CpuName, &CpuNode);
   if (EFI_ERROR (Status)) {
     ASSERT (0);
     return Status;
@@ -735,7 +735,8 @@ CreateAmlProcessorContainer (
   IN  CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  *CONST  CfgMgrProtocol,
   IN        AML_NODE_HANDLE                               ParentNode,
   IN        CM_ARM_PROC_HIERARCHY_INFO                    *ProcHierarchyNodeInfo,
-  IN        UINT32                                        ProcContainerIndex,
+  IN        UINT16                                        ProcContainerName,
+  IN        UINT32                                        ProcContainerUid,
   OUT       AML_OBJECT_NODE_HANDLE                        *ProcContainerNodePtr
   )
 {
@@ -749,7 +750,7 @@ CreateAmlProcessorContainer (
   ASSERT (ProcHierarchyNodeInfo != NULL);
   ASSERT (ProcContainerNodePtr != NULL);
 
-  Status = WriteAslName ('C', ProcContainerIndex, AslNameProcContainer);
+  Status = WriteAslName ('C', ProcContainerName, AslNameProcContainer);
   if (EFI_ERROR (Status)) {
     ASSERT (0);
     return Status;
@@ -765,7 +766,7 @@ CreateAmlProcessorContainer (
   // and EFI_ACPI_6_3_PPTT_PROCESSOR_ID_INVALID is set for non-Cpus.
   Status = AmlCodeGenNameInteger (
              "_UID",
-             ProcContainerIndex,
+             ProcContainerUid,
              ProcContainerNode,
              NULL
              );
@@ -838,6 +839,8 @@ CreateAmlCpuTopologyTree (
   UINT32                  Index;
   UINT32                  CpuIndex;
   AML_OBJECT_NODE_HANDLE  ProcContainerNode;
+  UINT32                  Uid;
+  UINT16                  Name;
 
   ASSERT (Generator != NULL);
   ASSERT (Generator->ProcNodeList != NULL);
@@ -868,11 +871,17 @@ CreateAmlCpuTopologyTree (
           return EFI_INVALID_PARAMETER;
         }
 
+        if (Generator->ProcNodeList[Index].OverrideNameUidEnabled) {
+          Name = Generator->ProcNodeList[Index].OverrideName;
+        } else {
+          Name = CpuIndex;
+        }
+
         Status = CreateAmlCpuFromProcHierarchy (
                    Generator,
                    CfgMgrProtocol,
                    ParentNode,
-                   CpuIndex,
+                   Name,
                    &Generator->ProcNodeList[Index]
                    );
         if (EFI_ERROR (Status)) {
@@ -897,12 +906,21 @@ CreateAmlCpuTopologyTree (
           return EFI_INVALID_PARAMETER;
         }
 
+        if (Generator->ProcNodeList[Index].OverrideNameUidEnabled) {
+          Name = Generator->ProcNodeList[Index].OverrideName;
+          Uid  = Generator->ProcNodeList[Index].OverrideUid;
+        } else {
+          Name = *ProcContainerIndex;
+          Uid  = *ProcContainerIndex;
+        }
+
         Status = CreateAmlProcessorContainer (
                    Generator,
                    CfgMgrProtocol,
                    ParentNode,
                    &Generator->ProcNodeList[Index],
-                   *ProcContainerIndex,
+                   Name,
+                   Uid,
                    &ProcContainerNode
                    );
         if (EFI_ERROR (Status)) {
diff --git a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
index 5a01ed0fb8..5e4b88e8cc 100644
--- a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
+++ b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
@@ -305,6 +305,9 @@ STATIC CONST CM_OBJ_PARSER  CmArmProcHierarchyInfoParser[] = {
   { "NoOfPrivateResources",       4,                        "0x%x", NULL },
   { "PrivateResourcesArrayToken", sizeof (CM_OBJECT_TOKEN), "0x%p", NULL },
   { "LpiToken",                   sizeof (CM_OBJECT_TOKEN), "0x%p", NULL },
+  { "OverrideNameUidEnabled",     1,                        "%d",   NULL },
+  { "OverrideName",               2,                        "0x%x", NULL },
+  { "OverrideUid",                4,                        "0x%x", NULL }
 };
 
 /** A parser for EArmObjCacheInfo.
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] DynamicTablesPkg: Allow for specified CPU names
  2022-11-07 15:57 [PATCH] DynamicTablesPkg: Allow for specified CPU names Jeff Brasen
@ 2022-11-07 16:06 ` Jeff Brasen
  2022-12-19 11:24   ` [edk2-devel] " Sami Mujawar
  2022-11-09 12:58 ` PierreGondois
  1 sibling, 1 reply; 5+ messages in thread
From: Jeff Brasen @ 2022-11-07 16:06 UTC (permalink / raw)
  To: devel@edk2.groups.io
  Cc: Sami.Mujawar@arm.com, Alexei.Fedorov@arm.com,
	pierre.gondois@arm.com, quic_llindhol@quicinc.com,
	ardb+tianocore@kernel.org

This was the simplest approach that I had to solve this issue, but not sure if it would be better to do something smarter. A couple other ideas I had where:
- Specify the affinity level that CPUs use and use the levels above that for the containers.
- Have a new object that has the container levels and affinity mapping. This wouldn't support unbalanced layouts.
- I can't think of a good way to auto detect the cpu level affinity level but that would be nice. 

-Jeff


> -----Original Message-----
> From: Jeff Brasen <jbrasen@nvidia.com>
> Sent: Monday, November 7, 2022 8:57 AM
> To: devel@edk2.groups.io
> Cc: Sami.Mujawar@arm.com; Alexei.Fedorov@arm.com;
> pierre.gondois@arm.com; quic_llindhol@quicinc.com;
> ardb+tianocore@kernel.org; Jeff Brasen <jbrasen@nvidia.com>
> Subject: [PATCH] DynamicTablesPkg: Allow for specified CPU names
> 
> Allow object to specify the name of processor and processor container
> 
> nodes and the UID of processor containers.
> 
> 
> 
> This allows these to be more accurately referenced from other tables.
> 
> For example for the _PSL method or the UID in the APMT table.
> 
> 
> 
> The UID and Name for processor container may be different as if the
> 
> intention is to set names as the corresponding affinity level the UID
> 
> may need to be different if there are multiple levels of containers.
> 
> 
> 
> Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
> 
> ---
> 
>  .../Include/ArmNameSpaceObjects.h             | 11 +++++
> 
>  .../SsdtCpuTopologyGenerator.c                | 40 ++++++++++++++-----
> 
>  .../ConfigurationManagerObjectParser.c        |  3 ++
> 
>  3 files changed, 43 insertions(+), 11 deletions(-)
> 
> 
> 
> diff --git a/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
> b/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
> 
> index 6aafd41a2e..19098609de 100644
> 
> --- a/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
> 
> +++ b/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
> 
> @@ -768,6 +768,17 @@ typedef struct CmArmProcHierarchyInfo {
> 
>    /// Token identifying a CM_ARM_OBJ_REF structure, itself referencing
> 
>    /// CM_ARM_LPI_INFO objects.
> 
>    CM_OBJECT_TOKEN    LpiToken;
> 
> +  /// Set to TRUE if UID should override index for name and _UID
> 
> +  /// for processor container nodes and name of processors.
> 
> +  /// This should be consistently set for containers or processors to avoid
> 
> +  /// duplicate values
> 
> +  BOOLEAN            OverrideNameUidEnabled;
> 
> +  /// If OverrideNameUidEnabled is TRUE then this value will be used for
> name of
> 
> +  /// processors and processor containers.
> 
> +  UINT16             OverrideName;
> 
> +  /// If OverrideNameUidEnabled is TRUE then this value will be used for
> 
> +  /// the UID of processor containers.
> 
> +  UINT32             OverrideUid;
> 
>  } CM_ARM_PROC_HIERARCHY_INFO;
> 
> 
> 
>  /** A structure that describes the Cache Type Structure (Type 1) in PPTT
> 
> diff --git
> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCp
> uTopologyGenerator.c
> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCp
> uTopologyGenerator.c
> 
> index d06c7615fb..92fa904103 100644
> 
> ---
> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCp
> uTopologyGenerator.c
> 
> +++
> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCp
> uTopologyGenerator.c
> 
> @@ -553,7 +553,7 @@ GenerateLpiStates (
> 
>    @param [in]  Generator    The SSDT Cpu Topology generator.
> 
>    @param [in]  ParentNode   Parent node to attach the Cpu node to.
> 
>    @param [in]  GicCInfo     CM_ARM_GICC_INFO object used to create the
> node.
> 
> -  @param [in]  CpuIndex     Index used to generate the node name.
> 
> +  @param [in]  CpuName      Value used to generate the node name.
> 
>    @param [out] CpuNodePtr   If not NULL, return the created Cpu node.
> 
> 
> 
>    @retval EFI_SUCCESS             Success.
> 
> @@ -567,7 +567,7 @@ CreateAmlCpu (
> 
>    IN   ACPI_CPU_TOPOLOGY_GENERATOR  *Generator,
> 
>    IN   AML_NODE_HANDLE              ParentNode,
> 
>    IN   CM_ARM_GICC_INFO             *GicCInfo,
> 
> -  IN   UINT32                       CpuIndex,
> 
> +  IN   UINT32                       CpuName,
> 
>    OUT  AML_OBJECT_NODE_HANDLE       *CpuNodePtr OPTIONAL
> 
>    )
> 
>  {
> 
> @@ -579,7 +579,7 @@ CreateAmlCpu (
> 
>    ASSERT (ParentNode != NULL);
> 
>    ASSERT (GicCInfo != NULL);
> 
> 
> 
> -  Status = WriteAslName ('C', CpuIndex, AslName);
> 
> +  Status = WriteAslName ('C', CpuName, AslName);
> 
>    if (EFI_ERROR (Status)) {
> 
>      ASSERT (0);
> 
>      return Status;
> 
> @@ -628,7 +628,7 @@ CreateAmlCpu (
> 
>    @param [in]  CfgMgrProtocol         Pointer to the Configuration Manager
> 
>                                        Protocol Interface.
> 
>    @param [in]  ParentNode             Parent node to attach the Cpu node to.
> 
> -  @param [in]  CpuIndex               Index used to generate the node name.
> 
> +  @param [in]  CpuName                Value used to generate the node name.
> 
>    @param [in]  ProcHierarchyNodeInfo  CM_ARM_PROC_HIERARCHY_INFO
> describing
> 
>                                        the Cpu.
> 
> 
> 
> @@ -643,7 +643,7 @@ CreateAmlCpuFromProcHierarchy (
> 
>    IN        ACPI_CPU_TOPOLOGY_GENERATOR                   *Generator,
> 
>    IN  CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  *CONST
> CfgMgrProtocol,
> 
>    IN        AML_NODE_HANDLE                               ParentNode,
> 
> -  IN        UINT32                                        CpuIndex,
> 
> +  IN        UINT32                                        CpuName,
> 
>    IN        CM_ARM_PROC_HIERARCHY_INFO
> *ProcHierarchyNodeInfo
> 
>    )
> 
>  {
> 
> @@ -668,7 +668,7 @@ CreateAmlCpuFromProcHierarchy (
> 
>      return Status;
> 
>    }
> 
> 
> 
> -  Status = CreateAmlCpu (Generator, ParentNode, GicCInfo, CpuIndex,
> &CpuNode);
> 
> +  Status = CreateAmlCpu (Generator, ParentNode, GicCInfo, CpuName,
> &CpuNode);
> 
>    if (EFI_ERROR (Status)) {
> 
>      ASSERT (0);
> 
>      return Status;
> 
> @@ -735,7 +735,8 @@ CreateAmlProcessorContainer (
> 
>    IN  CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  *CONST
> CfgMgrProtocol,
> 
>    IN        AML_NODE_HANDLE                               ParentNode,
> 
>    IN        CM_ARM_PROC_HIERARCHY_INFO
> *ProcHierarchyNodeInfo,
> 
> -  IN        UINT32                                        ProcContainerIndex,
> 
> +  IN        UINT16                                        ProcContainerName,
> 
> +  IN        UINT32                                        ProcContainerUid,
> 
>    OUT       AML_OBJECT_NODE_HANDLE                        *ProcContainerNodePtr
> 
>    )
> 
>  {
> 
> @@ -749,7 +750,7 @@ CreateAmlProcessorContainer (
> 
>    ASSERT (ProcHierarchyNodeInfo != NULL);
> 
>    ASSERT (ProcContainerNodePtr != NULL);
> 
> 
> 
> -  Status = WriteAslName ('C', ProcContainerIndex, AslNameProcContainer);
> 
> +  Status = WriteAslName ('C', ProcContainerName, AslNameProcContainer);
> 
>    if (EFI_ERROR (Status)) {
> 
>      ASSERT (0);
> 
>      return Status;
> 
> @@ -765,7 +766,7 @@ CreateAmlProcessorContainer (
> 
>    // and EFI_ACPI_6_3_PPTT_PROCESSOR_ID_INVALID is set for non-Cpus.
> 
>    Status = AmlCodeGenNameInteger (
> 
>               "_UID",
> 
> -             ProcContainerIndex,
> 
> +             ProcContainerUid,
> 
>               ProcContainerNode,
> 
>               NULL
> 
>               );
> 
> @@ -838,6 +839,8 @@ CreateAmlCpuTopologyTree (
> 
>    UINT32                  Index;
> 
>    UINT32                  CpuIndex;
> 
>    AML_OBJECT_NODE_HANDLE  ProcContainerNode;
> 
> +  UINT32                  Uid;
> 
> +  UINT16                  Name;
> 
> 
> 
>    ASSERT (Generator != NULL);
> 
>    ASSERT (Generator->ProcNodeList != NULL);
> 
> @@ -868,11 +871,17 @@ CreateAmlCpuTopologyTree (
> 
>            return EFI_INVALID_PARAMETER;
> 
>          }
> 
> 
> 
> +        if (Generator->ProcNodeList[Index].OverrideNameUidEnabled) {
> 
> +          Name = Generator->ProcNodeList[Index].OverrideName;
> 
> +        } else {
> 
> +          Name = CpuIndex;
> 
> +        }
> 
> +
> 
>          Status = CreateAmlCpuFromProcHierarchy (
> 
>                     Generator,
> 
>                     CfgMgrProtocol,
> 
>                     ParentNode,
> 
> -                   CpuIndex,
> 
> +                   Name,
> 
>                     &Generator->ProcNodeList[Index]
> 
>                     );
> 
>          if (EFI_ERROR (Status)) {
> 
> @@ -897,12 +906,21 @@ CreateAmlCpuTopologyTree (
> 
>            return EFI_INVALID_PARAMETER;
> 
>          }
> 
> 
> 
> +        if (Generator->ProcNodeList[Index].OverrideNameUidEnabled) {
> 
> +          Name = Generator->ProcNodeList[Index].OverrideName;
> 
> +          Uid  = Generator->ProcNodeList[Index].OverrideUid;
> 
> +        } else {
> 
> +          Name = *ProcContainerIndex;
> 
> +          Uid  = *ProcContainerIndex;
> 
> +        }
> 
> +
> 
>          Status = CreateAmlProcessorContainer (
> 
>                     Generator,
> 
>                     CfgMgrProtocol,
> 
>                     ParentNode,
> 
>                     &Generator->ProcNodeList[Index],
> 
> -                   *ProcContainerIndex,
> 
> +                   Name,
> 
> +                   Uid,
> 
>                     &ProcContainerNode
> 
>                     );
> 
>          if (EFI_ERROR (Status)) {
> 
> diff --git
> a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManag
> erObjectParser.c
> b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManag
> erObjectParser.c
> 
> index 5a01ed0fb8..5e4b88e8cc 100644
> 
> ---
> a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManag
> erObjectParser.c
> 
> +++
> b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManag
> erObjectParser.c
> 
> @@ -305,6 +305,9 @@ STATIC CONST CM_OBJ_PARSER
> CmArmProcHierarchyInfoParser[] = {
> 
>    { "NoOfPrivateResources",       4,                        "0x%x", NULL },
> 
>    { "PrivateResourcesArrayToken", sizeof (CM_OBJECT_TOKEN), "0x%p",
> NULL },
> 
>    { "LpiToken",                   sizeof (CM_OBJECT_TOKEN), "0x%p", NULL },
> 
> +  { "OverrideNameUidEnabled",     1,                        "%d",   NULL },
> 
> +  { "OverrideName",               2,                        "0x%x", NULL },
> 
> +  { "OverrideUid",                4,                        "0x%x", NULL }
> 
>  };
> 
> 
> 
>  /** A parser for EArmObjCacheInfo.
> 
> --
> 
> 2.25.1
> 
> 


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [edk2-devel] [PATCH] DynamicTablesPkg: Allow for specified CPU names
  2022-11-07 15:57 [PATCH] DynamicTablesPkg: Allow for specified CPU names Jeff Brasen
  2022-11-07 16:06 ` Jeff Brasen
@ 2022-11-09 12:58 ` PierreGondois
  1 sibling, 0 replies; 5+ messages in thread
From: PierreGondois @ 2022-11-09 12:58 UTC (permalink / raw)
  To: devel, jbrasen
  Cc: Sami.Mujawar, Alexei.Fedorov, quic_llindhol, ardb+tianocore

Hello Jeff,

On 11/7/22 16:57, Jeff Brasen via groups.io wrote:
> Allow object to specify the name of processor and processor container
> 
> nodes and the UID of processor containers.
> 
> 
> 
> This allows these to be more accurately referenced from other tables.
> 
> For example for the _PSL method or the UID in the APMT table.

The CM_ARM_PROC_HIERARCHY_INFO struct has a token that can be used
for referencing. If there were generators for the _PSL method and the
APMT table, this would be possible to avoid handwriting CPUs' Name/Uid
and use the ones generated in SsdtCpuTopologyGenerator.c somehow.

This would take more time to do. Otherwise the code itself looks good
to me,

Regards,
Pierre

> 
> 
> 
> The UID and Name for processor container may be different as if the
> 
> intention is to set names as the corresponding affinity level the UID
> 
> may need to be different if there are multiple levels of containers.
> 
> 
> 
> Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
> 
> ---
> 
>   .../Include/ArmNameSpaceObjects.h             | 11 +++++
> 
>   .../SsdtCpuTopologyGenerator.c                | 40 ++++++++++++++-----
> 
>   .../ConfigurationManagerObjectParser.c        |  3 ++
> 
>   3 files changed, 43 insertions(+), 11 deletions(-)
> 
> 
> 
> diff --git a/DynamicTablesPkg/Include/ArmNameSpaceObjects.h b/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
> 
> index 6aafd41a2e..19098609de 100644
> 
> --- a/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
> 
> +++ b/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
> 
> @@ -768,6 +768,17 @@ typedef struct CmArmProcHierarchyInfo {
> 
>     /// Token identifying a CM_ARM_OBJ_REF structure, itself referencing
> 
>     /// CM_ARM_LPI_INFO objects.
> 
>     CM_OBJECT_TOKEN    LpiToken;
> 
> +  /// Set to TRUE if UID should override index for name and _UID
> 
> +  /// for processor container nodes and name of processors.
> 
> +  /// This should be consistently set for containers or processors to avoid
> 
> +  /// duplicate values
> 
> +  BOOLEAN            OverrideNameUidEnabled;
> 
> +  /// If OverrideNameUidEnabled is TRUE then this value will be used for name of
> 
> +  /// processors and processor containers.
> 
> +  UINT16             OverrideName;
> 
> +  /// If OverrideNameUidEnabled is TRUE then this value will be used for
> 
> +  /// the UID of processor containers.
> 
> +  UINT32             OverrideUid;
> 
>   } CM_ARM_PROC_HIERARCHY_INFO;
> 
>   
> 
>   /** A structure that describes the Cache Type Structure (Type 1) in PPTT
> 
> diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
> 
> index d06c7615fb..92fa904103 100644
> 
> --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
> 
> +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
> 
> @@ -553,7 +553,7 @@ GenerateLpiStates (
> 
>     @param [in]  Generator    The SSDT Cpu Topology generator.
> 
>     @param [in]  ParentNode   Parent node to attach the Cpu node to.
> 
>     @param [in]  GicCInfo     CM_ARM_GICC_INFO object used to create the node.
> 
> -  @param [in]  CpuIndex     Index used to generate the node name.
> 
> +  @param [in]  CpuName      Value used to generate the node name.
> 
>     @param [out] CpuNodePtr   If not NULL, return the created Cpu node.
> 
>   
> 
>     @retval EFI_SUCCESS             Success.
> 
> @@ -567,7 +567,7 @@ CreateAmlCpu (
> 
>     IN   ACPI_CPU_TOPOLOGY_GENERATOR  *Generator,
> 
>     IN   AML_NODE_HANDLE              ParentNode,
> 
>     IN   CM_ARM_GICC_INFO             *GicCInfo,
> 
> -  IN   UINT32                       CpuIndex,
> 
> +  IN   UINT32                       CpuName,
> 
>     OUT  AML_OBJECT_NODE_HANDLE       *CpuNodePtr OPTIONAL
> 
>     )
> 
>   {
> 
> @@ -579,7 +579,7 @@ CreateAmlCpu (
> 
>     ASSERT (ParentNode != NULL);
> 
>     ASSERT (GicCInfo != NULL);
> 
>   
> 
> -  Status = WriteAslName ('C', CpuIndex, AslName);
> 
> +  Status = WriteAslName ('C', CpuName, AslName);
> 
>     if (EFI_ERROR (Status)) {
> 
>       ASSERT (0);
> 
>       return Status;
> 
> @@ -628,7 +628,7 @@ CreateAmlCpu (
> 
>     @param [in]  CfgMgrProtocol         Pointer to the Configuration Manager
> 
>                                         Protocol Interface.
> 
>     @param [in]  ParentNode             Parent node to attach the Cpu node to.
> 
> -  @param [in]  CpuIndex               Index used to generate the node name.
> 
> +  @param [in]  CpuName                Value used to generate the node name.
> 
>     @param [in]  ProcHierarchyNodeInfo  CM_ARM_PROC_HIERARCHY_INFO describing
> 
>                                         the Cpu.
> 
>   
> 
> @@ -643,7 +643,7 @@ CreateAmlCpuFromProcHierarchy (
> 
>     IN        ACPI_CPU_TOPOLOGY_GENERATOR                   *Generator,
> 
>     IN  CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  *CONST  CfgMgrProtocol,
> 
>     IN        AML_NODE_HANDLE                               ParentNode,
> 
> -  IN        UINT32                                        CpuIndex,
> 
> +  IN        UINT32                                        CpuName,
> 
>     IN        CM_ARM_PROC_HIERARCHY_INFO                    *ProcHierarchyNodeInfo
> 
>     )
> 
>   {
> 
> @@ -668,7 +668,7 @@ CreateAmlCpuFromProcHierarchy (
> 
>       return Status;
> 
>     }
> 
>   
> 
> -  Status = CreateAmlCpu (Generator, ParentNode, GicCInfo, CpuIndex, &CpuNode);
> 
> +  Status = CreateAmlCpu (Generator, ParentNode, GicCInfo, CpuName, &CpuNode);
> 
>     if (EFI_ERROR (Status)) {
> 
>       ASSERT (0);
> 
>       return Status;
> 
> @@ -735,7 +735,8 @@ CreateAmlProcessorContainer (
> 
>     IN  CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  *CONST  CfgMgrProtocol,
> 
>     IN        AML_NODE_HANDLE                               ParentNode,
> 
>     IN        CM_ARM_PROC_HIERARCHY_INFO                    *ProcHierarchyNodeInfo,
> 
> -  IN        UINT32                                        ProcContainerIndex,
> 
> +  IN        UINT16                                        ProcContainerName,
> 
> +  IN        UINT32                                        ProcContainerUid,
> 
>     OUT       AML_OBJECT_NODE_HANDLE                        *ProcContainerNodePtr
> 
>     )
> 
>   {
> 
> @@ -749,7 +750,7 @@ CreateAmlProcessorContainer (
> 
>     ASSERT (ProcHierarchyNodeInfo != NULL);
> 
>     ASSERT (ProcContainerNodePtr != NULL);
> 
>   
> 
> -  Status = WriteAslName ('C', ProcContainerIndex, AslNameProcContainer);
> 
> +  Status = WriteAslName ('C', ProcContainerName, AslNameProcContainer);
> 
>     if (EFI_ERROR (Status)) {
> 
>       ASSERT (0);
> 
>       return Status;
> 
> @@ -765,7 +766,7 @@ CreateAmlProcessorContainer (
> 
>     // and EFI_ACPI_6_3_PPTT_PROCESSOR_ID_INVALID is set for non-Cpus.
> 
>     Status = AmlCodeGenNameInteger (
> 
>                "_UID",
> 
> -             ProcContainerIndex,
> 
> +             ProcContainerUid,
> 
>                ProcContainerNode,
> 
>                NULL
> 
>                );
> 
> @@ -838,6 +839,8 @@ CreateAmlCpuTopologyTree (
> 
>     UINT32                  Index;
> 
>     UINT32                  CpuIndex;
> 
>     AML_OBJECT_NODE_HANDLE  ProcContainerNode;
> 
> +  UINT32                  Uid;
> 
> +  UINT16                  Name;
> 
>   
> 
>     ASSERT (Generator != NULL);
> 
>     ASSERT (Generator->ProcNodeList != NULL);
> 
> @@ -868,11 +871,17 @@ CreateAmlCpuTopologyTree (
> 
>             return EFI_INVALID_PARAMETER;
> 
>           }
> 
>   
> 
> +        if (Generator->ProcNodeList[Index].OverrideNameUidEnabled) {
> 
> +          Name = Generator->ProcNodeList[Index].OverrideName;
> 
> +        } else {
> 
> +          Name = CpuIndex;
> 
> +        }
> 
> +
> 
>           Status = CreateAmlCpuFromProcHierarchy (
> 
>                      Generator,
> 
>                      CfgMgrProtocol,
> 
>                      ParentNode,
> 
> -                   CpuIndex,
> 
> +                   Name,
> 
>                      &Generator->ProcNodeList[Index]
> 
>                      );
> 
>           if (EFI_ERROR (Status)) {
> 
> @@ -897,12 +906,21 @@ CreateAmlCpuTopologyTree (
> 
>             return EFI_INVALID_PARAMETER;
> 
>           }
> 
>   
> 
> +        if (Generator->ProcNodeList[Index].OverrideNameUidEnabled) {
> 
> +          Name = Generator->ProcNodeList[Index].OverrideName;
> 
> +          Uid  = Generator->ProcNodeList[Index].OverrideUid;
> 
> +        } else {
> 
> +          Name = *ProcContainerIndex;
> 
> +          Uid  = *ProcContainerIndex;
> 
> +        }
> 
> +
> 
>           Status = CreateAmlProcessorContainer (
> 
>                      Generator,
> 
>                      CfgMgrProtocol,
> 
>                      ParentNode,
> 
>                      &Generator->ProcNodeList[Index],
> 
> -                   *ProcContainerIndex,
> 
> +                   Name,
> 
> +                   Uid,
> 
>                      &ProcContainerNode
> 
>                      );
> 
>           if (EFI_ERROR (Status)) {
> 
> diff --git a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
> 
> index 5a01ed0fb8..5e4b88e8cc 100644
> 
> --- a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
> 
> +++ b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
> 
> @@ -305,6 +305,9 @@ STATIC CONST CM_OBJ_PARSER  CmArmProcHierarchyInfoParser[] = {
> 
>     { "NoOfPrivateResources",       4,                        "0x%x", NULL },
> 
>     { "PrivateResourcesArrayToken", sizeof (CM_OBJECT_TOKEN), "0x%p", NULL },
> 
>     { "LpiToken",                   sizeof (CM_OBJECT_TOKEN), "0x%p", NULL },
> 
> +  { "OverrideNameUidEnabled",     1,                        "%d",   NULL },
> 
> +  { "OverrideName",               2,                        "0x%x", NULL },
> 
> +  { "OverrideUid",                4,                        "0x%x", NULL }
> 
>   };
> 
>   
> 
>   /** A parser for EArmObjCacheInfo.
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [edk2-devel] [PATCH] DynamicTablesPkg: Allow for specified CPU names
  2022-11-07 16:06 ` Jeff Brasen
@ 2022-12-19 11:24   ` Sami Mujawar
  2022-12-19 12:56     ` Sami Mujawar
  0 siblings, 1 reply; 5+ messages in thread
From: Sami Mujawar @ 2022-12-19 11:24 UTC (permalink / raw)
  To: Jeff Brasen, devel

[-- Attachment #1: Type: text/plain, Size: 245 bytes --]

Hi Jeff,

I think we can go ahead with this patch for now. We can revisit this with any modifications should we have any other requirement that needs addressing.

Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>

Regards,

Sami Mujawar

[-- Attachment #2: Type: text/html, Size: 283 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [edk2-devel] [PATCH] DynamicTablesPkg: Allow for specified CPU names
  2022-12-19 11:24   ` [edk2-devel] " Sami Mujawar
@ 2022-12-19 12:56     ` Sami Mujawar
  0 siblings, 0 replies; 5+ messages in thread
From: Sami Mujawar @ 2022-12-19 12:56 UTC (permalink / raw)
  To: Sami Mujawar, devel

[-- Attachment #1: Type: text/plain, Size: 77 bytes --]

Merged as 05da2d24b08b..5fb3f5723a1e

Thanks.

Regards,

Sami Mujawar

[-- Attachment #2: Type: text/html, Size: 101 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2022-12-19 12:56 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-11-07 15:57 [PATCH] DynamicTablesPkg: Allow for specified CPU names Jeff Brasen
2022-11-07 16:06 ` Jeff Brasen
2022-12-19 11:24   ` [edk2-devel] " Sami Mujawar
2022-12-19 12:56     ` Sami Mujawar
2022-11-09 12:58 ` PierreGondois

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox