[v2] Update Arm platforms following addition of EFI_MP_SERVICES_PROTOCOL support in edk2

[edk2-devel] [PATCH v2 14/17] Silicon/Ampere: Update ArmPlatformLib to work with changed ARM_CORE_INFO

Posted by Rebecca Cran 3 years, 8 months ago

The ARM_CORE_INFO struct has been updated so the MPIDR is now a single
field instead of separate cluster/core fields. Update ArmPlatformLib.

Signed-off-by: Rebecca Cran <rebecca@nuviainc.com>
---
 Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c b/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c
index 5b4be0e55516..f2ec923d6f8d 100644
--- a/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c
+++ b/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c
@@ -108,9 +108,8 @@ PrePeiCoreGetMpCoreInfo (
     }
     SocketId = SOCKET_ID (Index);
     ClusterId = CLUSTER_ID (Index);
-    mArmPlatformMpCoreInfoTable[mArmPlatformCoreCount].ClusterId = SocketId;
-    mArmPlatformMpCoreInfoTable[mArmPlatformCoreCount].CoreId =
-      (ClusterId << 8) | (Index % PLATFORM_CPU_NUM_CORES_PER_CPM);
+    mArmPlatformMpCoreInfoTable[mArmPlatformCoreCount].Mpidr = GET_MPID (
+      SocketId, (ClusterId << 8) | (Index % PLATFORM_CPU_NUM_CORES_PER_CPM));
     mArmPlatformMpCoreInfoTable[mArmPlatformCoreCount].MailboxClearAddress = 0;
     mArmPlatformMpCoreInfoTable[mArmPlatformCoreCount].MailboxClearValue = 0;
     mArmPlatformMpCoreInfoTable[mArmPlatformCoreCount].MailboxGetAddress = 0;
-- 
2.31.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#85026): https://edk2.groups.io/g/devel/message/85026
Mute This Topic: https://groups.io/mt/87777839/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 14/17] Silicon/Ampere: Update ArmPlatformLib to work with changed ARM_CORE_INFO

Posted by Nhi Pham via groups.io 3 years, 8 months ago

Hi Rebecca,

Leif is merging the rest of Altra port to the edk2-platforms which has 
SRAT ACPI table consuming the CPU Core Info table. Therefore, we will 
need to fix the SRAT too. I would defer the fix until the Altra port is 
fully merged.

On 17/12/2021 05:07, Rebecca Cran wrote:
> The ARM_CORE_INFO struct has been updated so the MPIDR is now a single
> field instead of separate cluster/core fields. Update ArmPlatformLib.
>
> Signed-off-by: Rebecca Cran <rebecca@nuviainc.com>
> ---
>   Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c b/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c
> index 5b4be0e55516..f2ec923d6f8d 100644
> --- a/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c
> +++ b/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c
> @@ -108,9 +108,8 @@ PrePeiCoreGetMpCoreInfo (
>       }
>       SocketId = SOCKET_ID (Index);
>       ClusterId = CLUSTER_ID (Index);
> -    mArmPlatformMpCoreInfoTable[mArmPlatformCoreCount].ClusterId = SocketId;
> -    mArmPlatformMpCoreInfoTable[mArmPlatformCoreCount].CoreId =
> -      (ClusterId << 8) | (Index % PLATFORM_CPU_NUM_CORES_PER_CPM);
> +    mArmPlatformMpCoreInfoTable[mArmPlatformCoreCount].Mpidr = GET_MPID (
> +      SocketId, (ClusterId << 8) | (Index % PLATFORM_CPU_NUM_CORES_PER_CPM));

For Ampere Altra, the correct MPIDR encoding is SocketId << 32 | 
ClusterId << 16 | (Index % PLATFORM_CPU_NUM_CORES_PER_CPM) << 8

It would be the same what 
Platform/Ampere/JadePkg/Drivers/AcpiPlatformDxe/AcpiMadt.c (not 
available yet - being merged in) is describing.

Best regards,

Nhi

>       mArmPlatformMpCoreInfoTable[mArmPlatformCoreCount].MailboxClearAddress = 0;
>       mArmPlatformMpCoreInfoTable[mArmPlatformCoreCount].MailboxClearValue = 0;
>       mArmPlatformMpCoreInfoTable[mArmPlatformCoreCount].MailboxGetAddress = 0;


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#85095): https://edk2.groups.io/g/devel/message/85095
Mute This Topic: https://groups.io/mt/87777839/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 14/17] Silicon/Ampere: Update ArmPlatformLib to work with changed ARM_CORE_INFO

Posted by Ard Biesheuvel 3 years, 7 months ago

On Sun, 19 Dec 2021 at 04:36, Nhi Pham <nhi@os.amperecomputing.com> wrote:
>
> Hi Rebecca,
>
> Leif is merging the rest of Altra port to the edk2-platforms which has
> SRAT ACPI table consuming the CPU Core Info table. Therefore, we will
> need to fix the SRAT too. I would defer the fix until the Altra port is
> fully merged.
>

This seems to be stalled so to make progress, I am going to merge this.

> On 17/12/2021 05:07, Rebecca Cran wrote:
> > The ARM_CORE_INFO struct has been updated so the MPIDR is now a single
> > field instead of separate cluster/core fields. Update ArmPlatformLib.
> >
> > Signed-off-by: Rebecca Cran <rebecca@nuviainc.com>
> > ---
> >   Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c | 5 ++---
> >   1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c b/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c
> > index 5b4be0e55516..f2ec923d6f8d 100644
> > --- a/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c
> > +++ b/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c
> > @@ -108,9 +108,8 @@ PrePeiCoreGetMpCoreInfo (
> >       }
> >       SocketId = SOCKET_ID (Index);
> >       ClusterId = CLUSTER_ID (Index);
> > -    mArmPlatformMpCoreInfoTable[mArmPlatformCoreCount].ClusterId = SocketId;
> > -    mArmPlatformMpCoreInfoTable[mArmPlatformCoreCount].CoreId =
> > -      (ClusterId << 8) | (Index % PLATFORM_CPU_NUM_CORES_PER_CPM);
> > +    mArmPlatformMpCoreInfoTable[mArmPlatformCoreCount].Mpidr = GET_MPID (
> > +      SocketId, (ClusterId << 8) | (Index % PLATFORM_CPU_NUM_CORES_PER_CPM));
>
> For Ampere Altra, the correct MPIDR encoding is SocketId << 32 |
> ClusterId << 16 | (Index % PLATFORM_CPU_NUM_CORES_PER_CPM) << 8
>
> It would be the same what
> Platform/Ampere/JadePkg/Drivers/AcpiPlatformDxe/AcpiMadt.c (not
> available yet - being merged in) is describing.
>

Feel free to follow up with a patch that changes this into the correct
representation, but this patch does not make it less correct than it
already is; it just stores the socket ID in the cluster ID field in a
different way.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#86225): https://edk2.groups.io/g/devel/message/86225
Mute This Topic: https://groups.io/mt/87777839/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 14/17] Silicon/Ampere: Update ArmPlatformLib to work with changed ARM_CORE_INFO

Posted by Leif Lindholm 3 years, 7 months ago

On Sun, Jan 30, 2022 at 11:36:51 +0100, Ard Biesheuvel wrote:
> On Sun, 19 Dec 2021 at 04:36, Nhi Pham <nhi@os.amperecomputing.com> wrote:
> >
> > Hi Rebecca,
> >
> > Leif is merging the rest of Altra port to the edk2-platforms which has
> > SRAT ACPI table consuming the CPU Core Info table. Therefore, we will
> > need to fix the SRAT too. I would defer the fix until the Altra port is
> > fully merged.
> >
> 
> This seems to be stalled so to make progress, I am going to merge this.

Thanks, that was the right call.

Apologies for radio silence, catching up on backlog (including
remainder of Altra port) now.

/
    Leif

> > On 17/12/2021 05:07, Rebecca Cran wrote:
> > > The ARM_CORE_INFO struct has been updated so the MPIDR is now a single
> > > field instead of separate cluster/core fields. Update ArmPlatformLib.
> > >
> > > Signed-off-by: Rebecca Cran <rebecca@nuviainc.com>
> > > ---
> > >   Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c | 5 ++---
> > >   1 file changed, 2 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c b/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c
> > > index 5b4be0e55516..f2ec923d6f8d 100644
> > > --- a/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c
> > > +++ b/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c
> > > @@ -108,9 +108,8 @@ PrePeiCoreGetMpCoreInfo (
> > >       }
> > >       SocketId = SOCKET_ID (Index);
> > >       ClusterId = CLUSTER_ID (Index);
> > > -    mArmPlatformMpCoreInfoTable[mArmPlatformCoreCount].ClusterId = SocketId;
> > > -    mArmPlatformMpCoreInfoTable[mArmPlatformCoreCount].CoreId =
> > > -      (ClusterId << 8) | (Index % PLATFORM_CPU_NUM_CORES_PER_CPM);
> > > +    mArmPlatformMpCoreInfoTable[mArmPlatformCoreCount].Mpidr = GET_MPID (
> > > +      SocketId, (ClusterId << 8) | (Index % PLATFORM_CPU_NUM_CORES_PER_CPM));
> >
> > For Ampere Altra, the correct MPIDR encoding is SocketId << 32 |
> > ClusterId << 16 | (Index % PLATFORM_CPU_NUM_CORES_PER_CPM) << 8
> >
> > It would be the same what
> > Platform/Ampere/JadePkg/Drivers/AcpiPlatformDxe/AcpiMadt.c (not
> > available yet - being merged in) is describing.
> >
> 
> Feel free to follow up with a patch that changes this into the correct
> representation, but this patch does not make it less correct than it
> already is; it just stores the socket ID in the cluster ID field in a
> different way.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#86245): https://edk2.groups.io/g/devel/message/86245
Mute This Topic: https://groups.io/mt/87777839/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 14/17] Silicon/Ampere: Update ArmPlatformLib to work with changed ARM_CORE_INFO

Posted by Nhi Pham via groups.io 3 years, 7 months ago

On 31/01/2022 19:08, Leif Lindholm wrote:
> On Sun, Jan 30, 2022 at 11:36:51 +0100, Ard Biesheuvel wrote:
>> On Sun, 19 Dec 2021 at 04:36, Nhi Pham <nhi@os.amperecomputing.com> wrote:
>>> Hi Rebecca,
>>>
>>> Leif is merging the rest of Altra port to the edk2-platforms which has
>>> SRAT ACPI table consuming the CPU Core Info table. Therefore, we will
>>> need to fix the SRAT too. I would defer the fix until the Altra port is
>>> fully merged.
>>>
>> This seems to be stalled so to make progress, I am going to merge this.
> Thanks, that was the right call.
>
> Apologies for radio silence, catching up on backlog (including
> remainder of Altra port) now.

Thanks much, Leif.

-Nhi

>
> /
>      Leif
>
>>> On 17/12/2021 05:07, Rebecca Cran wrote:
>>>> The ARM_CORE_INFO struct has been updated so the MPIDR is now a single
>>>> field instead of separate cluster/core fields. Update ArmPlatformLib.
>>>>
>>>> Signed-off-by: Rebecca Cran <rebecca@nuviainc.com>
>>>> ---
>>>>    Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c | 5 ++---
>>>>    1 file changed, 2 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c b/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c
>>>> index 5b4be0e55516..f2ec923d6f8d 100644
>>>> --- a/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c
>>>> +++ b/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c
>>>> @@ -108,9 +108,8 @@ PrePeiCoreGetMpCoreInfo (
>>>>        }
>>>>        SocketId = SOCKET_ID (Index);
>>>>        ClusterId = CLUSTER_ID (Index);
>>>> -    mArmPlatformMpCoreInfoTable[mArmPlatformCoreCount].ClusterId = SocketId;
>>>> -    mArmPlatformMpCoreInfoTable[mArmPlatformCoreCount].CoreId =
>>>> -      (ClusterId << 8) | (Index % PLATFORM_CPU_NUM_CORES_PER_CPM);
>>>> +    mArmPlatformMpCoreInfoTable[mArmPlatformCoreCount].Mpidr = GET_MPID (
>>>> +      SocketId, (ClusterId << 8) | (Index % PLATFORM_CPU_NUM_CORES_PER_CPM));
>>> For Ampere Altra, the correct MPIDR encoding is SocketId << 32 |
>>> ClusterId << 16 | (Index % PLATFORM_CPU_NUM_CORES_PER_CPM) << 8
>>>
>>> It would be the same what
>>> Platform/Ampere/JadePkg/Drivers/AcpiPlatformDxe/AcpiMadt.c (not
>>> available yet - being merged in) is describing.
>>>
>> Feel free to follow up with a patch that changes this into the correct
>> representation, but this patch does not make it less correct than it
>> already is; it just stores the socket ID in the cluster ID field in a
>> different way.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#86439): https://edk2.groups.io/g/devel/message/86439
Mute This Topic: https://groups.io/mt/87777839/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 14/17] Silicon/Ampere: Update ArmPlatformLib to work with changed ARM_CORE_INFO

Posted by Nhi Pham via groups.io 3 years, 7 months ago

On 30/01/2022 17:36, Ard Biesheuvel wrote:
> On Sun, 19 Dec 2021 at 04:36, Nhi Pham <nhi@os.amperecomputing.com> wrote:
>> Hi Rebecca,
>>
>> Leif is merging the rest of Altra port to the edk2-platforms which has
>> SRAT ACPI table consuming the CPU Core Info table. Therefore, we will
>> need to fix the SRAT too. I would defer the fix until the Altra port is
>> fully merged.
>>
> This seems to be stalled so to make progress, I am going to merge this.
>
>> On 17/12/2021 05:07, Rebecca Cran wrote:
>>> The ARM_CORE_INFO struct has been updated so the MPIDR is now a single
>>> field instead of separate cluster/core fields. Update ArmPlatformLib.
>>>
>>> Signed-off-by: Rebecca Cran <rebecca@nuviainc.com>
>>> ---
>>>    Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c | 5 ++---
>>>    1 file changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c b/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c
>>> index 5b4be0e55516..f2ec923d6f8d 100644
>>> --- a/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c
>>> +++ b/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c
>>> @@ -108,9 +108,8 @@ PrePeiCoreGetMpCoreInfo (
>>>        }
>>>        SocketId = SOCKET_ID (Index);
>>>        ClusterId = CLUSTER_ID (Index);
>>> -    mArmPlatformMpCoreInfoTable[mArmPlatformCoreCount].ClusterId = SocketId;
>>> -    mArmPlatformMpCoreInfoTable[mArmPlatformCoreCount].CoreId =
>>> -      (ClusterId << 8) | (Index % PLATFORM_CPU_NUM_CORES_PER_CPM);
>>> +    mArmPlatformMpCoreInfoTable[mArmPlatformCoreCount].Mpidr = GET_MPID (
>>> +      SocketId, (ClusterId << 8) | (Index % PLATFORM_CPU_NUM_CORES_PER_CPM));
>> For Ampere Altra, the correct MPIDR encoding is SocketId << 32 |
>> ClusterId << 16 | (Index % PLATFORM_CPU_NUM_CORES_PER_CPM) << 8
>>
>> It would be the same what
>> Platform/Ampere/JadePkg/Drivers/AcpiPlatformDxe/AcpiMadt.c (not
>> available yet - being merged in) is describing.
>>
> Feel free to follow up with a patch that changes this into the correct
> representation, but this patch does not make it less correct than it
> already is; it just stores the socket ID in the cluster ID field in a
> different way.

Thanks Ard. That's OK.

Best regards,

Nhi



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#86440): https://edk2.groups.io/g/devel/message/86440
Mute This Topic: https://groups.io/mt/87777839/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 14/17] Silicon/Ampere: Update ArmPlatformLib to work with changed ARM_CORE_INFO

Posted by Leif Lindholm 3 years, 5 months ago

Hi Nhi,

On Sun, Dec 19, 2021 at 10:35:41 +0700, Nhi Pham via groups.io wrote:
> Hi Rebecca,
> 
> Leif is merging the rest of Altra port to the edk2-platforms which has SRAT
> ACPI table consuming the CPU Core Info table. Therefore, we will need to fix
> the SRAT too. I would defer the fix until the Altra port is fully merged.
> 
> On 17/12/2021 05:07, Rebecca Cran wrote:
> > The ARM_CORE_INFO struct has been updated so the MPIDR is now a single
> > field instead of separate cluster/core fields. Update ArmPlatformLib.
> > 
> > Signed-off-by: Rebecca Cran <rebecca@nuviainc.com>
> > ---
> >   Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c | 5 ++---
> >   1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c b/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c
> > index 5b4be0e55516..f2ec923d6f8d 100644
> > --- a/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c
> > +++ b/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c
> > @@ -108,9 +108,8 @@ PrePeiCoreGetMpCoreInfo (
> >       }
> >       SocketId = SOCKET_ID (Index);
> >       ClusterId = CLUSTER_ID (Index);
> > -    mArmPlatformMpCoreInfoTable[mArmPlatformCoreCount].ClusterId = SocketId;
> > -    mArmPlatformMpCoreInfoTable[mArmPlatformCoreCount].CoreId =
> > -      (ClusterId << 8) | (Index % PLATFORM_CPU_NUM_CORES_PER_CPM);
> > +    mArmPlatformMpCoreInfoTable[mArmPlatformCoreCount].Mpidr = GET_MPID (
> > +      SocketId, (ClusterId << 8) | (Index % PLATFORM_CPU_NUM_CORES_PER_CPM));
> 
> For Ampere Altra, the correct MPIDR encoding is SocketId << 32 | ClusterId
> << 16 | (Index % PLATFORM_CPU_NUM_CORES_PER_CPM) << 8
> 
> It would be the same what
> Platform/Ampere/JadePkg/Drivers/AcpiPlatformDxe/AcpiMadt.c (not available
> yet - being merged in) is describing.

This patch already got merged, so if you feel it is wrong, could you
submit a fix please?

The next patch for me to push from your set otherwise also requires
some changes. My naïve attempt would look something like:

diff --git a/Platform/Ampere/JadePkg/Drivers/AcpiPlatformDxe/AcpiSrat.c b/Platform/Ampere/JadePkg/Drivers/AcpiPlatformDxe/AcpiSrat.c
index 906b771a250c..d5bc732b08bb 100644
--- a/Platform/Ampere/JadePkg/Drivers/AcpiPlatformDxe/AcpiSrat.c
+++ b/Platform/Ampere/JadePkg/Drivers/AcpiPlatformDxe/AcpiSrat.c
@@ -7,6 +7,7 @@
 **/
 
 #include <Guid/ArmMpCoreInfo.h>
+#include <Library/ArmLib.h>
 #include "AcpiPlatform.h"
 
 EFI_ACPI_6_3_SYSTEM_RESOURCE_AFFINITY_TABLE_HEADER SRATTableHeaderTemplate = {
@@ -119,6 +120,7 @@ SratAddGiccAffinity (
   UINTN               Count, NumNode, Idx;
   UINT32              AcpiProcessorUid;
   UINT8               Socket;
+  UINT8               Core;
   UINT8               Cpm;
 
   for (Idx = 0; Idx < gST->NumberOfTableEntries; Idx++) {
@@ -137,14 +139,14 @@ SratAddGiccAffinity (
   NumNode = 0;
   while (Count != ArmProcessorTable->NumberOfEntries) {
     for (Idx = 0; Idx < ArmProcessorTable->NumberOfEntries; Idx++ ) {
-      Socket = ArmCoreInfoTable[Idx].ClusterId;
-      Cpm = (ArmCoreInfoTable[Idx].CoreId >> PLATFORM_CPM_UID_BIT_OFFSET);
+      Socket = GET_MPIDR_AFF1 (ArmCoreInfoTable[Idx].Mpidr);
+      Core   = GET_MPIDR_AFF0 (ArmCoreInfoTable[Idx].Mpidr);
+      Cpm = Core >> PLATFORM_CPM_UID_BIT_OFFSET;
       if (CpuGetSubNumNode (Socket, Cpm) != NumNode) {
         /* We add nodes based on ProximityDomain order */
         continue;
       }
-      AcpiProcessorUid = (ArmCoreInfoTable[Idx].ClusterId << PLATFORM_SOCKET_UID_BIT_OFFSET) +
-                         ArmCoreInfoTable[Idx].CoreId;
+      AcpiProcessorUid = (Socket << PLATFORM_SOCKET_UID_BIT_OFFSET) + Core;
       ZeroMem ((VOID *)&SratGiccAffinity[Count], sizeof (SratGiccAffinity[Count]));
       SratGiccAffinity[Count].AcpiProcessorUid = AcpiProcessorUid;
       SratGiccAffinity[Count].Flags = 1;

Would you be happy for me to fold that into
"AmpereAltraPkg, JadePkg: Add ACPI support", or would you be able to
submit a v6 of that patch only?

Best Regards,

Leif


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#88029): https://edk2.groups.io/g/devel/message/88029
Mute This Topic: https://groups.io/mt/87777839/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 14/17] Silicon/Ampere: Update ArmPlatformLib to work with changed ARM_CORE_INFO

Posted by Nhi Pham via groups.io 3 years, 5 months ago

Hi Leif,

On 25/03/2022 23:30, Leif Lindholm wrote:
> Hi Nhi,
>
> On Sun, Dec 19, 2021 at 10:35:41 +0700, Nhi Pham via groups.io wrote:
>> Hi Rebecca,
>>
>> Leif is merging the rest of Altra port to the edk2-platforms which has SRAT
>> ACPI table consuming the CPU Core Info table. Therefore, we will need to fix
>> the SRAT too. I would defer the fix until the Altra port is fully merged.
>>
>> On 17/12/2021 05:07, Rebecca Cran wrote:
>>> The ARM_CORE_INFO struct has been updated so the MPIDR is now a single
>>> field instead of separate cluster/core fields. Update ArmPlatformLib.
>>>
>>> Signed-off-by: Rebecca Cran <rebecca@nuviainc.com>
>>> ---
>>>    Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c | 5 ++---
>>>    1 file changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c b/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c
>>> index 5b4be0e55516..f2ec923d6f8d 100644
>>> --- a/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c
>>> +++ b/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c
>>> @@ -108,9 +108,8 @@ PrePeiCoreGetMpCoreInfo (
>>>        }
>>>        SocketId = SOCKET_ID (Index);
>>>        ClusterId = CLUSTER_ID (Index);
>>> -    mArmPlatformMpCoreInfoTable[mArmPlatformCoreCount].ClusterId = SocketId;
>>> -    mArmPlatformMpCoreInfoTable[mArmPlatformCoreCount].CoreId =
>>> -      (ClusterId << 8) | (Index % PLATFORM_CPU_NUM_CORES_PER_CPM);
>>> +    mArmPlatformMpCoreInfoTable[mArmPlatformCoreCount].Mpidr = GET_MPID (
>>> +      SocketId, (ClusterId << 8) | (Index % PLATFORM_CPU_NUM_CORES_PER_CPM));
>> For Ampere Altra, the correct MPIDR encoding is SocketId << 32 | ClusterId
>> << 16 | (Index % PLATFORM_CPU_NUM_CORES_PER_CPM) << 8
>>
>> It would be the same what
>> Platform/Ampere/JadePkg/Drivers/AcpiPlatformDxe/AcpiMadt.c (not available
>> yet - being merged in) is describing.
> This patch already got merged, so if you feel it is wrong, could you
> submit a fix please?
>
> The next patch for me to push from your set otherwise also requires
> some changes. My naïve attempt would look something like:
>
> diff --git a/Platform/Ampere/JadePkg/Drivers/AcpiPlatformDxe/AcpiSrat.c b/Platform/Ampere/JadePkg/Drivers/AcpiPlatformDxe/AcpiSrat.c
> index 906b771a250c..d5bc732b08bb 100644
> --- a/Platform/Ampere/JadePkg/Drivers/AcpiPlatformDxe/AcpiSrat.c
> +++ b/Platform/Ampere/JadePkg/Drivers/AcpiPlatformDxe/AcpiSrat.c
> @@ -7,6 +7,7 @@
>   **/
>   
>   #include <Guid/ArmMpCoreInfo.h>
> +#include <Library/ArmLib.h>
>   #include "AcpiPlatform.h"
>   
>   EFI_ACPI_6_3_SYSTEM_RESOURCE_AFFINITY_TABLE_HEADER SRATTableHeaderTemplate = {
> @@ -119,6 +120,7 @@ SratAddGiccAffinity (
>     UINTN               Count, NumNode, Idx;
>     UINT32              AcpiProcessorUid;
>     UINT8               Socket;
> +  UINT8               Core;
>     UINT8               Cpm;
>   
>     for (Idx = 0; Idx < gST->NumberOfTableEntries; Idx++) {
> @@ -137,14 +139,14 @@ SratAddGiccAffinity (
>     NumNode = 0;
>     while (Count != ArmProcessorTable->NumberOfEntries) {
>       for (Idx = 0; Idx < ArmProcessorTable->NumberOfEntries; Idx++ ) {
> -      Socket = ArmCoreInfoTable[Idx].ClusterId;
> -      Cpm = (ArmCoreInfoTable[Idx].CoreId >> PLATFORM_CPM_UID_BIT_OFFSET);
> +      Socket = GET_MPIDR_AFF1 (ArmCoreInfoTable[Idx].Mpidr);
> +      Core   = GET_MPIDR_AFF0 (ArmCoreInfoTable[Idx].Mpidr);
> +      Cpm = Core >> PLATFORM_CPM_UID_BIT_OFFSET;
>         if (CpuGetSubNumNode (Socket, Cpm) != NumNode) {
>           /* We add nodes based on ProximityDomain order */
>           continue;
>         }
> -      AcpiProcessorUid = (ArmCoreInfoTable[Idx].ClusterId << PLATFORM_SOCKET_UID_BIT_OFFSET) +
> -                         ArmCoreInfoTable[Idx].CoreId;
> +      AcpiProcessorUid = (Socket << PLATFORM_SOCKET_UID_BIT_OFFSET) + Core;
>         ZeroMem ((VOID *)&SratGiccAffinity[Count], sizeof (SratGiccAffinity[Count]));
>         SratGiccAffinity[Count].AcpiProcessorUid = AcpiProcessorUid;
>         SratGiccAffinity[Count].Flags = 1;
>
> Would you be happy for me to fold that into
> "AmpereAltraPkg, JadePkg: Add ACPI support", or would you be able to
> submit a v6 of that patch only?
>
> Best Regards,
>
> Leif

Thanks much for the patch. The MPIDR decoding matches with Rebecca's 
update for the ArmPlatformLib earlier and the ARM_CORE_INFO is just 
consumed in the AcpiSrat.c. So, that is good for now. Please help fold 
that into the ACPI patch when merging the rest of the Mt. Jade support 
patchset.

In the future, I will follow up with a patch that makes the 
representation of MPIDR in the ARM_CORE_INFO the same as in MADT table.

Thanks,

Nhi



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#88063): https://edk2.groups.io/g/devel/message/88063
Mute This Topic: https://groups.io/mt/87777839/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-