public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [edk2-devel] [PATCH] DynamicTablesPkg/SSDT: Remove incorrect root node check
@ 2024-01-08 18:12 Jeshua Smith via groups.io
  2024-01-09  8:22 ` PierreGondois
  0 siblings, 1 reply; 6+ messages in thread
From: Jeshua Smith via groups.io @ 2024-01-08 18:12 UTC (permalink / raw)
  To: devel
  Cc: ardb+tianocore, quic_llindhol, pierre.gondois, Sami.Mujawar,
	sdonthineni, vsethi, Jeshua Smith, Ashish Singhal

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]
-=-=-=-=-=-=-=-=-=-=-=-



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

* Re: [edk2-devel] [PATCH] DynamicTablesPkg/SSDT: Remove incorrect root node check
  2024-01-08 18:12 [edk2-devel] [PATCH] DynamicTablesPkg/SSDT: Remove incorrect root node check Jeshua Smith via groups.io
@ 2024-01-09  8:22 ` PierreGondois
  2024-01-09 15:47   ` Jeshua Smith via groups.io
  0 siblings, 1 reply; 6+ messages in thread
From: PierreGondois @ 2024-01-09  8:22 UTC (permalink / raw)
  To: Jeshua Smith, devel
  Cc: ardb+tianocore, quic_llindhol, Sami.Mujawar, sdonthineni, vsethi,
	Ashish Singhal

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]
-=-=-=-=-=-=-=-=-=-=-=-



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

* Re: [edk2-devel] [PATCH] DynamicTablesPkg/SSDT: Remove incorrect root node check
  2024-01-09  8:22 ` PierreGondois
@ 2024-01-09 15:47   ` Jeshua Smith via groups.io
  2024-01-09 16:15     ` PierreGondois
  0 siblings, 1 reply; 6+ messages in thread
From: Jeshua Smith via groups.io @ 2024-01-09 15:47 UTC (permalink / raw)
  To: Pierre Gondois, devel@edk2.groups.io
  Cc: ardb+tianocore@kernel.org, quic_llindhol@quicinc.com,
	Sami.Mujawar@arm.com, Shanker Donthineni, Vikram Sethi,
	Ashish Singhal

> 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]
-=-=-=-=-=-=-=-=-=-=-=-



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

* Re: [edk2-devel] [PATCH] DynamicTablesPkg/SSDT: Remove incorrect root node check
  2024-01-09 15:47   ` Jeshua Smith via groups.io
@ 2024-01-09 16:15     ` PierreGondois
  2024-01-09 16:55       ` Jeshua Smith via groups.io
  0 siblings, 1 reply; 6+ messages in thread
From: PierreGondois @ 2024-01-09 16:15 UTC (permalink / raw)
  To: Jeshua Smith, devel@edk2.groups.io
  Cc: ardb+tianocore@kernel.org, quic_llindhol@quicinc.com,
	Sami.Mujawar@arm.com, Shanker Donthineni, Vikram Sethi,
	Ashish Singhal



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]
-=-=-=-=-=-=-=-=-=-=-=-



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

* Re: [edk2-devel] [PATCH] DynamicTablesPkg/SSDT: Remove incorrect root node check
  2024-01-09 16:15     ` PierreGondois
@ 2024-01-09 16:55       ` Jeshua Smith via groups.io
  2024-01-15 11:41         ` PierreGondois
  0 siblings, 1 reply; 6+ messages in thread
From: Jeshua Smith via groups.io @ 2024-01-09 16:55 UTC (permalink / raw)
  To: Pierre Gondois, devel@edk2.groups.io
  Cc: ardb+tianocore@kernel.org, quic_llindhol@quicinc.com,
	Sami.Mujawar@arm.com, Shanker Donthineni, Vikram Sethi,
	Ashish Singhal

> > 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]
-=-=-=-=-=-=-=-=-=-=-=-



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

* Re: [edk2-devel] [PATCH] DynamicTablesPkg/SSDT: Remove incorrect root node check
  2024-01-09 16:55       ` Jeshua Smith via groups.io
@ 2024-01-15 11:41         ` PierreGondois
  0 siblings, 0 replies; 6+ messages in thread
From: PierreGondois @ 2024-01-15 11:41 UTC (permalink / raw)
  To: Jeshua Smith, devel@edk2.groups.io
  Cc: ardb+tianocore@kernel.org, quic_llindhol@quicinc.com,
	Sami.Mujawar@arm.com, Shanker Donthineni, Vikram Sethi,
	Ashish Singhal

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]
-=-=-=-=-=-=-=-=-=-=-=-



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

end of thread, other threads:[~2024-01-16  6:27 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-01-08 18:12 [edk2-devel] [PATCH] DynamicTablesPkg/SSDT: Remove incorrect root node check Jeshua Smith via groups.io
2024-01-09  8:22 ` PierreGondois
2024-01-09 15:47   ` Jeshua Smith via groups.io
2024-01-09 16:15     ` PierreGondois
2024-01-09 16:55       ` Jeshua Smith via groups.io
2024-01-15 11:41         ` PierreGondois

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