[edk2-devel] [PATCH] DynamicTablesPkg/SSDT: Remove incorrect root node check

[edk2-devel] [PATCH] DynamicTablesPkg/SSDT: Remove incorrect root node check

[Jeshua Smith via groups.io]

The code was incorrectly assuming that root nodes had to be physical
package nodes and vice versa. This is not always true, so the check
is being removed.

Signed-off-by: Jeshua Smith <jeshuas@nvidia.com>
Tested-by: Ashish Singhal <ashishsingha@nvidia.com>
Reviewed-by: Ashish Singhal <ashishsingha@nvidia.com>
---
 .../SsdtCpuTopologyGenerator.c                | 23 ++++---------------
 1 file changed, 5 insertions(+), 18 deletions(-)

diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
index 724f33c660..4ad9508f57 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
@@ -983,7 +983,6 @@ CreateAmlProcessorContainer (
   @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.
@@ -994,26 +993,16 @@ EFIAPI
 CheckProcNode (
   UINT32           NodeFlags,
   BOOLEAN          IsLeaf,
-  CM_OBJECT_TOKEN  NodeToken,
-  CM_OBJECT_TOKEN  ParentNodeToken
+  CM_OBJECT_TOKEN  NodeToken
   )
 {
   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);
+    InvalidFlags = ((NodeFlags & PPTT_LEAF_MASK) != PPTT_LEAF_MASK);
   } else {
-    InvalidFlags |= ((NodeFlags & PPTT_LEAF_MASK) != 0);
+    InvalidFlags = ((NodeFlags & PPTT_LEAF_MASK) != 0);
   }
 
   if (InvalidFlags) {
@@ -1086,8 +1075,7 @@ CreateAmlCpuTopologyTree (
         Status = CheckProcNode (
                    Generator->ProcNodeList[Index].Flags,
                    TRUE,
-                   Generator->ProcNodeList[Index].Token,
-                   NodeToken
+                   Generator->ProcNodeList[Index].Token
                    );
         if (EFI_ERROR (Status)) {
           ASSERT (0);
@@ -1119,8 +1107,7 @@ CreateAmlCpuTopologyTree (
         Status = CheckProcNode (
                    Generator->ProcNodeList[Index].Flags,
                    FALSE,
-                   Generator->ProcNodeList[Index].Token,
-                   NodeToken
+                   Generator->ProcNodeList[Index].Token
                    );
         if (EFI_ERROR (Status)) {
           ASSERT (0);
-- 
2.34.1



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

Re: [edk2-devel] [PATCH] DynamicTablesPkg/SSDT: Remove incorrect root node check

[PierreGondois]

Hello Jeshua,

On 1/8/24 19:12, Jeshua Smith wrote:
> The code was incorrectly assuming that root nodes had to be physical
> package nodes and vice versa. This is not always true, so the check
> is being removed.

Does it mean that you have a topology where the top-level node is not
a physical package ? If yes, does it also mean that multiple physical
packages share a resource (which belong to the top-level node) ?

It is correct that the check is a bit stronger than what the specification
states, but it was handling all topologies so far, so would it be possible
to describe the topology that you have ?

Regards,
Pierre

> 
> Signed-off-by: Jeshua Smith <jeshuas@nvidia.com>
> Tested-by: Ashish Singhal <ashishsingha@nvidia.com>
> Reviewed-by: Ashish Singhal <ashishsingha@nvidia.com>
> ---
>   .../SsdtCpuTopologyGenerator.c                | 23 ++++---------------
>   1 file changed, 5 insertions(+), 18 deletions(-)
> 
> diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
> index 724f33c660..4ad9508f57 100644
> --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
> +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
> @@ -983,7 +983,6 @@ CreateAmlProcessorContainer (
>     @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.
> @@ -994,26 +993,16 @@ EFIAPI
>   CheckProcNode (
>     UINT32           NodeFlags,
>     BOOLEAN          IsLeaf,
> -  CM_OBJECT_TOKEN  NodeToken,
> -  CM_OBJECT_TOKEN  ParentNodeToken
> +  CM_OBJECT_TOKEN  NodeToken
>     )
>   {
>     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);
> +    InvalidFlags = ((NodeFlags & PPTT_LEAF_MASK) != PPTT_LEAF_MASK);
>     } else {
> -    InvalidFlags |= ((NodeFlags & PPTT_LEAF_MASK) != 0);
> +    InvalidFlags = ((NodeFlags & PPTT_LEAF_MASK) != 0);
>     }
>   
>     if (InvalidFlags) {
> @@ -1086,8 +1075,7 @@ CreateAmlCpuTopologyTree (
>           Status = CheckProcNode (
>                      Generator->ProcNodeList[Index].Flags,
>                      TRUE,
> -                   Generator->ProcNodeList[Index].Token,
> -                   NodeToken
> +                   Generator->ProcNodeList[Index].Token
>                      );
>           if (EFI_ERROR (Status)) {
>             ASSERT (0);
> @@ -1119,8 +1107,7 @@ CreateAmlCpuTopologyTree (
>           Status = CheckProcNode (
>                      Generator->ProcNodeList[Index].Flags,
>                      FALSE,
> -                   Generator->ProcNodeList[Index].Token,
> -                   NodeToken
> +                   Generator->ProcNodeList[Index].Token
>                      );
>           if (EFI_ERROR (Status)) {
>             ASSERT (0);


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

Re: [edk2-devel] [PATCH] DynamicTablesPkg/SSDT: Remove incorrect root node check

[Jeshua Smith via groups.io]

> From: Pierre Gondois <pierre.gondois@arm.com>
> Sent: Tuesday, January 9, 2024 1:22 AM

> On 1/8/24 19:12, Jeshua Smith wrote:
> > The code was incorrectly assuming that root nodes had to be physical
> > package nodes and vice versa. This is not always true, so the check is
> > being removed.
> 
> Does it mean that you have a topology where the top-level node is not a
> physical package ? If yes, does it also mean that multiple physical packages
> share a resource (which belong to the top-level node) ?

Yes, this change is due to the check incorrectly flagging our topology as invalid. Simply removing the check fixed the problem for us.
 
> It is correct that the check is a bit stronger than what the specification states,
> but it was handling all topologies so far, so would it be possible to describe the
> topology that you have ?

Two physical packages are on a multi-chip module and share resources on the module. The module then plugs into the baseboard/motherboard. 

Note: While investigating this we noticed that another vendor also has a similar PPTT topology to what is being flagged as invalid, so either that vendor isn't using EDK2 or they have done something to avoid this check without submitting a patch to EDK2.


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

Re: [edk2-devel] [PATCH] DynamicTablesPkg/SSDT: Remove incorrect root node check

[PierreGondois]

On 1/9/24 16:47, Jeshua Smith wrote:
>> From: Pierre Gondois <pierre.gondois@arm.com>
>> Sent: Tuesday, January 9, 2024 1:22 AM
> 
>> On 1/8/24 19:12, Jeshua Smith wrote:
>>> The code was incorrectly assuming that root nodes had to be physical
>>> package nodes and vice versa. This is not always true, so the check is
>>> being removed.
>>
>> Does it mean that you have a topology where the top-level node is not a
>> physical package ? If yes, does it also mean that multiple physical packages
>> share a resource (which belong to the top-level node) ?
> 
> Yes, this change is due to the check incorrectly flagging our topology as invalid. Simply removing the check fixed the problem for us.
>   
>> It is correct that the check is a bit stronger than what the specification states,
>> but it was handling all topologies so far, so would it be possible to describe the
>> topology that you have ?
> 
> Two physical packages are on a multi-chip module and share resources on the module. The module then plugs into the baseboard/motherboard.

Is it possible to elaborate on the resource being shared ?
Does it fall into the subject of this thread ? Some resources might be aswell described in other ACPI tables.
https://edk2.groups.io/g/devel/message/89121

> 
> Note: While investigating this we noticed that another vendor also has a similar PPTT topology to what is being flagged as invalid, so either that vendor isn't using EDK2 or they have done something to avoid this check without submitting a patch to EDK2.

This check is only present in the DynamicTablesPkg, so it shouldn't be too restrictive.
If the platform is known to use it, is it possible to share which platform it is ?

Regards,
Pierre


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

Re: [edk2-devel] [PATCH] DynamicTablesPkg/SSDT: Remove incorrect root node check

[Jeshua Smith via groups.io]

> > Two physical packages are on a multi-chip module and share resources on
> the module. The module then plugs into the baseboard/motherboard.
> 
> Is it possible to elaborate on the resource being shared ?

In our specific case the problem is related to the PPTT's "Identical Implementation" flag. We need a top level node above the physical package nodes to be able to set the "Identical Implementation" flag to indicate that all of the procs in all of the child packages are the same identical implementation. Without this (ie. forcing each physical package to be its own root node) Linux will fail to load the SPE driver when there are multiple identical packages because it detects that some of the procs have a different root node than other procs, implying that the packages don't have identical implementations.

> Does it fall into the subject of this thread ? Some resources might be aswell
> described in other ACPI tables.

The thread you linked looks like it is about non-processor resources, and therefore wasn't in scope for PPTT. I don't think this is related.

> > Note: While investigating this we noticed that another vendor also has a
> similar PPTT topology to what is being flagged as invalid, so either that vendor
> isn't using EDK2 or they have done something to avoid this check without
> submitting a patch to EDK2.
> 
> This check is only present in the DynamicTablesPkg, so it shouldn't be too
> restrictive.

Ah, correct. If they're not using DynamicTablesPkg to generate the PPTT/SSDT, then they wouldn't hit this.

> If the platform is known to use it, is it possible to share which platform it is ?

We see the topology in question (ie. a root node is a parent node to physical package nodes) in Ampere's two-socket Altra PPTT.


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

Re: [edk2-devel] [PATCH] DynamicTablesPkg/SSDT: Remove incorrect root node check

[PierreGondois]

Hello Jeshua,

On 1/9/24 17:55, Jeshua Smith wrote:
>>> Two physical packages are on a multi-chip module and share resources on
>> the module. The module then plugs into the baseboard/motherboard.
>>
>> Is it possible to elaborate on the resource being shared ?
> 
> In our specific case the problem is related to the PPTT's "Identical Implementation" flag. We need a top level node above the physical package nodes to be able to set the "Identical Implementation" flag to indicate that all of the procs in all of the child packages are the same identical implementation. Without this (ie. forcing each physical package to be its own root node) Linux will fail to load the SPE driver when there are multiple identical packages because it detects that some of the procs have a different root node than other procs, implying that the packages don't have identical implementations.
> 

Ok understood.

Would it then be possible to re-format the check instead of removing it ?
It should be possible to add a parameter to CreateAmlCpuTopologyTree()
to recursively check there was a node with the
EFI_ACPI_6_3_PPTT_PACKAGE_PHYSICAL bit set in the parents of
all the processor nodes,

Regards,
Pierre



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