[edk2-platforms][PATCH 1/1] AmpereAltraPkg: Update ArmPlatformLib to work with changed ARM_CORE_INFO

[edk2-platforms][PATCH 1/1] AmpereAltraPkg: Update ArmPlatformLib to work with changed ARM_CORE_INFO

[Nhi Pham]

From: Tinh Nguyen <tinhnguyen@os.amperecomputing.com>

Since the commit 103fa647d159e3d76be2634d2653c2d215dd0d46 instead
("ArmPkg: Replace CoreId and ClusterId with Mpidr in ARM_CORE_INFO
struct") has updated the ARM_CORE_INFO structure so the MPIDR is now a
single field instead of separate cluster/core fields. This patch
updates the ArmPlatformLib to work with the changed ARM_CORE_INFO.

Signed-off-by: Nhi Pham <nhi@os.amperecomputing.com>
---
 .../Ampere/AmpereAltraPkg/Include/Platform/Ac01.h  | 11 ++++++++++-
 .../JadePkg/Drivers/AcpiPlatformDxe/AcpiMadt.c     |  5 ++---
 .../JadePkg/Drivers/AcpiPlatformDxe/AcpiSrat.c     | 14 ++++++++------
 .../Library/ArmPlatformLib/ArmPlatformLib.c        |  8 +++++---
 4 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/Silicon/Ampere/AmpereAltraPkg/Include/Platform/Ac01.h b/Silicon/Ampere/AmpereAltraPkg/Include/Platform/Ac01.h
index d45688f88401..563dd5ef242d 100644
--- a/Silicon/Ampere/AmpereAltraPkg/Include/Platform/Ac01.h
+++ b/Silicon/Ampere/AmpereAltraPkg/Include/Platform/Ac01.h
@@ -1,6 +1,6 @@
 /** @file
 
-  Copyright (c) 2020 - 2021, Ampere Computing LLC. All rights reserved.<BR>
+  Copyright (c) 2020 - 2023, Ampere Computing LLC. All rights reserved.<BR>
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -229,6 +229,15 @@
 //
 #define PLATFORM_CPM_UID_BIT_OFFSET              8
 
+//
+// MPIDR manipulation
+//
+#define AC01_GET_MPIDR(SocketId, ClusterId, CoreId) \
+          (((SocketId) << 32) | ((ClusterId) << 16) | ((CoreId) << 8))
+#define AC01_GET_SOCKET_ID(Mpidr)  (((Mpidr) & ARM_CORE_AFF3) >> 32)
+#define AC01_GET_CLUSTER_ID(Mpidr) (((Mpidr) & ARM_CORE_AFF2) >> 16)
+#define AC01_GET_CORE_ID(Mpidr)    (((Mpidr) & ARM_CORE_AFF1) >> 8)
+
 //
 // Max number for AC01 PCIE Root Complexes
 //
diff --git a/Platform/Ampere/JadePkg/Drivers/AcpiPlatformDxe/AcpiMadt.c b/Platform/Ampere/JadePkg/Drivers/AcpiPlatformDxe/AcpiMadt.c
index 419ce578e452..4db1f9a383a9 100644
--- a/Platform/Ampere/JadePkg/Drivers/AcpiPlatformDxe/AcpiMadt.c
+++ b/Platform/Ampere/JadePkg/Drivers/AcpiPlatformDxe/AcpiMadt.c
@@ -1,6 +1,6 @@
 /** @file
 
-  Copyright (c) 2020 - 2021, Ampere Computing LLC. All rights reserved.<BR>
+  Copyright (c) 2020 - 2023, Ampere Computing LLC. All rights reserved.<BR>
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -160,8 +160,7 @@ AcpiInstallMadtProcessorNode (
     (ClusterId << 8) + (CpuId  % PLATFORM_CPU_NUM_CORES_PER_CPM);
   MadtProcessorEntryPointer->Flags = 1;
   MadtProcessorEntryPointer->MPIDR =
-    (((ClusterId << 8) + (CpuId  % PLATFORM_CPU_NUM_CORES_PER_CPM)) << 8);
-  MadtProcessorEntryPointer->MPIDR += (((UINT64)SocketId) << 32);
+    AC01_GET_MPIDR ((UINT64)SocketId, ClusterId, CpuId  % PLATFORM_CPU_NUM_CORES_PER_CPM);
 
   return Size;
 }
diff --git a/Platform/Ampere/JadePkg/Drivers/AcpiPlatformDxe/AcpiSrat.c b/Platform/Ampere/JadePkg/Drivers/AcpiPlatformDxe/AcpiSrat.c
index f4df60bc2593..3a89014f41f1 100644
--- a/Platform/Ampere/JadePkg/Drivers/AcpiPlatformDxe/AcpiSrat.c
+++ b/Platform/Ampere/JadePkg/Drivers/AcpiPlatformDxe/AcpiSrat.c
@@ -1,6 +1,6 @@
 /** @file
 
-  Copyright (c) 2020 - 2021, Ampere Computing LLC. All rights reserved.<BR>
+  Copyright (c) 2020 - 2023, Ampere Computing LLC. All rights reserved.<BR>
   Copyright (c) 2022, ARM Ltd. All rights reserved.<BR>
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -122,7 +122,6 @@ SratAddGiccAffinity (
   UINTN               Count, NumNode, Idx;
   UINT32              AcpiProcessorUid;
   UINT8               Socket;
-  UINT8               Core;
   UINT8               Cpm;
 
   Hob = GetFirstGuidHob (&gArmMpCoreInfoGuid);
@@ -141,14 +140,17 @@ SratAddGiccAffinity (
   NumNode = 0;
   while (Count != NumberOfEntries) {
     for (Idx = 0; Idx < NumberOfEntries; Idx++ ) {
-      Socket = GET_MPIDR_AFF1 (ArmCoreInfoTable[Idx].Mpidr);
-      Core   = GET_MPIDR_AFF0 (ArmCoreInfoTable[Idx].Mpidr);
-      Cpm = Core >> PLATFORM_CPM_UID_BIT_OFFSET;
+      Socket  = AC01_GET_SOCKET_ID (ArmCoreInfoTable[Idx].Mpidr);
+      Cpm     = AC01_GET_CLUSTER_ID (ArmCoreInfoTable[Idx].Mpidr);
       if (CpuGetSubNumNode (Socket, Cpm) != NumNode) {
         /* We add nodes based on ProximityDomain order */
         continue;
       }
-      AcpiProcessorUid = (Socket << PLATFORM_SOCKET_UID_BIT_OFFSET) + Core;
+
+      AcpiProcessorUid =
+        (AC01_GET_SOCKET_ID (ArmCoreInfoTable[Idx].Mpidr) << PLATFORM_SOCKET_UID_BIT_OFFSET)
+        + (AC01_GET_CLUSTER_ID (ArmCoreInfoTable[Idx].Mpidr) << PLATFORM_CPM_UID_BIT_OFFSET)
+        +  AC01_GET_CORE_ID (ArmCoreInfoTable[Idx].Mpidr);
       ZeroMem ((VOID *)&SratGiccAffinity[Count], sizeof (SratGiccAffinity[Count]));
       SratGiccAffinity[Count].AcpiProcessorUid = AcpiProcessorUid;
       SratGiccAffinity[Count].Flags = 1;
diff --git a/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c b/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c
index f2ec923d6f8d..18023df92880 100644
--- a/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c
+++ b/Silicon/Ampere/AmpereAltraPkg/Library/ArmPlatformLib/ArmPlatformLib.c
@@ -1,6 +1,6 @@
 /** @file
 
-  Copyright (c) 2020 - 2021, Ampere Computing LLC. All rights reserved.<BR>
+  Copyright (c) 2020 - 2023, Ampere Computing LLC. All rights reserved.<BR>
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -108,8 +108,10 @@ PrePeiCoreGetMpCoreInfo (
     }
     SocketId = SOCKET_ID (Index);
     ClusterId = CLUSTER_ID (Index);
-    mArmPlatformMpCoreInfoTable[mArmPlatformCoreCount].Mpidr = GET_MPID (
-      SocketId, (ClusterId << 8) | (Index % PLATFORM_CPU_NUM_CORES_PER_CPM));
+
+    mArmPlatformMpCoreInfoTable[mArmPlatformCoreCount].Mpidr =
+      AC01_GET_MPIDR ((UINT64)SocketId, ClusterId, (Index % PLATFORM_CPU_NUM_CORES_PER_CPM));
+
     mArmPlatformMpCoreInfoTable[mArmPlatformCoreCount].MailboxClearAddress = 0;
     mArmPlatformMpCoreInfoTable[mArmPlatformCoreCount].MailboxClearValue = 0;
     mArmPlatformMpCoreInfoTable[mArmPlatformCoreCount].MailboxGetAddress = 0;
-- 
2.25.1

Re: [edk2-devel] [edk2-platforms][PATCH 1/1] AmpereAltraPkg: Update ArmPlatformLib to work with changed ARM_CORE_INFO

[Rebecca Cran]

On 1/12/23 21:21, Nhi Pham via groups.io wrote:

> +//
> +// MPIDR manipulation
> +//
> +#define AC01_GET_MPIDR(SocketId, ClusterId, CoreId) \
> +          (((SocketId) << 32) | ((ClusterId) << 16) | ((CoreId) << 8))
> +#define AC01_GET_SOCKET_ID(Mpidr)  (((Mpidr) & ARM_CORE_AFF3) >> 32)
> +#define AC01_GET_CLUSTER_ID(Mpidr) (((Mpidr) & ARM_CORE_AFF2) >> 16)
> +#define AC01_GET_CORE_ID(Mpidr)    (((Mpidr) & ARM_CORE_AFF1) >> 8)
> +

Ideally, this should go in ArmPkg/Include/Library/ArmLib.h, but I'm not 
sure how we should handle the older format where the the core was in the 
first 8 bits.

-- 
Rebecca Cran

Re: [edk2-devel] [edk2-platforms][PATCH 1/1] AmpereAltraPkg: Update ArmPlatformLib to work with changed ARM_CORE_INFO

[Nhi Pham]

Hi Rebecca,

That's Ampere Altra Family specific MPIDR encoding. So, we could not 
leverage the definitions in the ArmPkg/Include/Library/ArmLib.h.

-Nhi

On 1/13/2023 9:40 PM, Rebecca Cran wrote:
> On 1/12/23 21:21, Nhi Pham via groups.io wrote:
>
>> +//
>> +// MPIDR manipulation
>> +//
>> +#define AC01_GET_MPIDR(SocketId, ClusterId, CoreId) \
>> +          (((SocketId) << 32) | ((ClusterId) << 16) | ((CoreId) << 8))
>> +#define AC01_GET_SOCKET_ID(Mpidr)  (((Mpidr) & ARM_CORE_AFF3) >> 32)
>> +#define AC01_GET_CLUSTER_ID(Mpidr) (((Mpidr) & ARM_CORE_AFF2) >> 16)
>> +#define AC01_GET_CORE_ID(Mpidr)    (((Mpidr) & ARM_CORE_AFF1) >> 8)
>> +
>
> Ideally, this should go in ArmPkg/Include/Library/ArmLib.h, but I'm 
> not sure how we should handle the older format where the the core was 
> in the first 8 bits.
>

Re: [edk2-devel] [edk2-platforms][PATCH 1/1] AmpereAltraPkg: Update ArmPlatformLib to work with changed ARM_CORE_INFO

[Rebecca Cran]

I was under the impression that this is becoming a more standard format?

For example, the Neoverse N2 has AFF0 always 0 (it's not hyperthreaded), 
it puts the core ID in AFF1, the cluster ID in AFF2, but since it only 
has a single socket AFF3 is always 0. This differs from older cores 
where the core ID was in AFF0 and the cluster ID in AFF1.

https://developer.arm.com/documentation/102099/0000/AArch64-registers/AArch64-identification-registers/MPIDR-EL1--Multiprocessor-Affinity-Register

Am I mistaken?

-- 
Rebecca Cran

On 1/17/23 02:53, Nhi Pham wrote:
> Hi Rebecca,
> 
> That's Ampere Altra Family specific MPIDR encoding. So, we could not 
> leverage the definitions in the ArmPkg/Include/Library/ArmLib.h.
> 
> -Nhi
> 
> On 1/13/2023 9:40 PM, Rebecca Cran wrote:
>> On 1/12/23 21:21, Nhi Pham via groups.io wrote:
>>
>>> +//
>>> +// MPIDR manipulation
>>> +//
>>> +#define AC01_GET_MPIDR(SocketId, ClusterId, CoreId) \
>>> +          (((SocketId) << 32) | ((ClusterId) << 16) | ((CoreId) << 8))
>>> +#define AC01_GET_SOCKET_ID(Mpidr)  (((Mpidr) & ARM_CORE_AFF3) >> 32)
>>> +#define AC01_GET_CLUSTER_ID(Mpidr) (((Mpidr) & ARM_CORE_AFF2) >> 16)
>>> +#define AC01_GET_CORE_ID(Mpidr)    (((Mpidr) & ARM_CORE_AFF1) >> 8)
>>> +
>>
>> Ideally, this should go in ArmPkg/Include/Library/ArmLib.h, but I'm 
>> not sure how we should handle the older format where the the core was 
>> in the first 8 bits.
>>

Re: [edk2-devel] [edk2-platforms][PATCH 1/1] AmpereAltraPkg: Update ArmPlatformLib to work with changed ARM_CORE_INFO

[Ard Biesheuvel]

On Tue, 17 Jan 2023 at 13:55, Rebecca Cran <rebecca@quicinc.com> wrote:
>
> I was under the impression that this is becoming a more standard format?
>

If this is not defined in an ARM spec somewhere, we shouldn't add it
to ArmPkg at this point.

> For example, the Neoverse N2 has AFF0 always 0 (it's not hyperthreaded),
> it puts the core ID in AFF1, the cluster ID in AFF2, but since it only
> has a single socket AFF3 is always 0. This differs from older cores
> where the core ID was in AFF0 and the cluster ID in AFF1.
>
> https://developer.arm.com/documentation/102099/0000/AArch64-registers/AArch64-identification-registers/MPIDR-EL1--Multiprocessor-Affinity-Register
>
> Am I mistaken?
>
> --
> Rebecca Cran
>
> On 1/17/23 02:53, Nhi Pham wrote:
> > Hi Rebecca,
> >
> > That's Ampere Altra Family specific MPIDR encoding. So, we could not
> > leverage the definitions in the ArmPkg/Include/Library/ArmLib.h.
> >
> > -Nhi
> >
> > On 1/13/2023 9:40 PM, Rebecca Cran wrote:
> >> On 1/12/23 21:21, Nhi Pham via groups.io wrote:
> >>
> >>> +//
> >>> +// MPIDR manipulation
> >>> +//
> >>> +#define AC01_GET_MPIDR(SocketId, ClusterId, CoreId) \
> >>> +          (((SocketId) << 32) | ((ClusterId) << 16) | ((CoreId) << 8))
> >>> +#define AC01_GET_SOCKET_ID(Mpidr)  (((Mpidr) & ARM_CORE_AFF3) >> 32)
> >>> +#define AC01_GET_CLUSTER_ID(Mpidr) (((Mpidr) & ARM_CORE_AFF2) >> 16)
> >>> +#define AC01_GET_CORE_ID(Mpidr)    (((Mpidr) & ARM_CORE_AFF1) >> 8)
> >>> +
> >>
> >> Ideally, this should go in ArmPkg/Include/Library/ArmLib.h, but I'm
> >> not sure how we should handle the older format where the the core was
> >> in the first 8 bits.
> >>

Re: [edk2-devel] [edk2-platforms][PATCH 1/1] AmpereAltraPkg: Update ArmPlatformLib to work with changed ARM_CORE_INFO

[Rebecca Cran]

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

On 1/17/23 09:40, Ard Biesheuvel wrote:

> On Tue, 17 Jan 2023 at 13:55, Rebecca Cran<rebecca@quicinc.com>  wrote:
>> I was under the impression that this is becoming a more standard format?
>>
> If this is not defined in an ARM spec somewhere, we shouldn't add it
> to ArmPkg at this point.

 From what I've found, the ARM specs such as the Arm Architecture 
Reference Manual for A-profile architecture don't define the meaning of 
the affinity fields? That appears to be left up to the individual Arm 
core TRMs.

For example, the Cortex-X2 TRM says:

Affinity level 0. This is the affinity level that is most significant 
for determining PE behavior. Higher affinity
levels are increasingly less significant in determining PE behavior. The 
assigned value of the MPIDR.{Aff2,
Aff1, Aff0} or AArch64-MPIDR_EL1.{Aff3, Aff2, Aff1, Aff0} set of fields 
of each PE must be unique within the
system as a whole.
0b00000000
Only one thread.


Affinity level 1. See the description of Aff0 for more information.
Value read from the CPUID configuration pins. Identification number for 
each CPU in an cluster counting from
zero.


Affinity level 2. See the description of Aff0 for more information.
The value will be determined by the CLUSTERIDAFF2 configuration pins.


Affinity level 3. See the description of Aff0 for more information.
The value will be determined by the CLUSTERIDAFF3 configuration pins.


-- 
Rebecca Cran

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

Re: [edk2-devel] [edk2-platforms][PATCH 1/1] AmpereAltraPkg: Update ArmPlatformLib to work with changed ARM_CORE_INFO

[Nhi Pham]

Hi Rebecca,

++ Harb who can give more insights on this. FYI, the original concern is 
https://edk2.groups.io/g/devel/message/98482

On 1/18/2023 1:21 AM, Rebecca Cran wrote:
> On 1/17/23 09:40, Ard Biesheuvel wrote:
>> On Tue, 17 Jan 2023 at 13:55, Rebecca Cran<rebecca@quicinc.com>  wrote:
>>> I was under the impression that this is becoming a more standard format?
>>>
>> If this is not defined in an ARM spec somewhere, we shouldn't add it
>> to ArmPkg at this point.
>
> From what I've found, the ARM specs such as the Arm Architecture 
> Reference Manual for A-profile architecture don't define the meaning 
> of the affinity fields? That appears to be left up to the individual 
> Arm core TRMs.
>
I think so. This might be silicon specific implementation.

Per Arm Armv8-A Architecture Registers 
(https://developer.arm.com/documentation/ddi0595/2021-12/AArch64-Registers/MPIDR-EL1--Multiprocessor-Affinity-Register), 
if I interpret correctly, the AFF0 will give core ID or thread ID based 
on the MT bit in the MPIDR register.

I think we should remove the following definitions particularly for 
getting core id and cluster in ArmPkg/Include/Library/ArmLib.h to avoid 
the confusion to others

#define ARM_CORE_MASK         ARM_CORE_AFF0
#define ARM_CLUSTER_MASK      ARM_CORE_AFF1
#define GET_CORE_ID(MpId)     ((MpId) & ARM_CORE_MASK)
#define GET_CLUSTER_ID(MpId)  (((MpId) & ARM_CLUSTER_MASK) >> 8)
#define GET_MPID(ClusterId, CoreId)   (((ClusterId) << 8) | (CoreId))
#define PRIMARY_CORE_ID       (PcdGet32(PcdArmPrimaryCore) & ARM_CORE_MASK)

And, should support GET_AFFx like

#define ARM_MPIDR_GET_AFF0(Mpid) ((Mpid) & ARM_CORE_AFF0)

For silicon specific usage, it can abstract them to proper IDs like

#define XXX_GET_CORE_ID(Mpid) ARM_MPIDR_GET_AFF0(Mpid)

Thanks,

Nhi

> For example, the Cortex-X2 TRM says:
>
> Affinity level 0. This is the affinity level that is most significant 
> for determining PE behavior. Higher affinity
> levels are increasingly less significant in determining PE behavior. 
> The assigned value of the MPIDR.{Aff2,
> Aff1, Aff0} or AArch64-MPIDR_EL1.{Aff3, Aff2, Aff1, Aff0} set of 
> fields of each PE must be unique within the
> system as a whole.
> 0b00000000
> Only one thread.
>
>
> Affinity level 1. See the description of Aff0 for more information.
> Value read from the CPUID configuration pins. Identification number 
> for each CPU in an cluster counting from
> zero.
>
>
> Affinity level 2. See the description of Aff0 for more information.
> The value will be determined by the CLUSTERIDAFF2 configuration pins.
>
>
> Affinity level 3. See the description of Aff0 for more information.
> The value will be determined by the CLUSTERIDAFF3 configuration pins.
>

Re: [edk2-devel] [edk2-platforms][PATCH 1/1] AmpereAltraPkg: Update ArmPlatformLib to work with changed ARM_CORE_INFO

[Leif Lindholm]

On Tue, Jan 31, 2023 at 13:35:50 +0700, Nhi Pham via groups.io wrote:
> Hi Rebecca,
> 
> ++ Harb who can give more insights on this. FYI, the original concern is
> https://edk2.groups.io/g/devel/message/98482
> 
> On 1/18/2023 1:21 AM, Rebecca Cran wrote:
> > On 1/17/23 09:40, Ard Biesheuvel wrote:
> > > On Tue, 17 Jan 2023 at 13:55, Rebecca Cran<rebecca@quicinc.com>  wrote:
> > > > I was under the impression that this is becoming a more standard format?
> > > > 
> > > If this is not defined in an ARM spec somewhere, we shouldn't add it
> > > to ArmPkg at this point.
> > 
> > From what I've found, the ARM specs such as the Arm Architecture
> > Reference Manual for A-profile architecture don't define the meaning of
> > the affinity fields? That appears to be left up to the individual Arm
> > core TRMs.
> 
> I think so. This might be silicon specific implementation.
> 
> Per Arm Armv8-A Architecture Registers (https://developer.arm.com/documentation/ddi0595/2021-12/AArch64-Registers/MPIDR-EL1--Multiprocessor-Affinity-Register),
> if I interpret correctly, the AFF0 will give core ID or thread ID based on
> the MT bit in the MPIDR register.
> 
> I think we should remove the following definitions particularly for getting
> core id and cluster in ArmPkg/Include/Library/ArmLib.h to avoid the
> confusion to others
> 
> #define ARM_CORE_MASK         ARM_CORE_AFF0
> #define ARM_CLUSTER_MASK      ARM_CORE_AFF1
> #define GET_CORE_ID(MpId)     ((MpId) & ARM_CORE_MASK)
> #define GET_CLUSTER_ID(MpId)  (((MpId) & ARM_CLUSTER_MASK) >> 8)
> #define GET_MPID(ClusterId, CoreId)   (((ClusterId) << 8) | (CoreId))
> #define PRIMARY_CORE_ID       (PcdGet32(PcdArmPrimaryCore) & ARM_CORE_MASK)
> 
> And, should support GET_AFFx like
> 
> #define ARM_MPIDR_GET_AFF0(Mpid) ((Mpid) & ARM_CORE_AFF0)
> 
> For silicon specific usage, it can abstract them to proper IDs like
> 
> #define XXX_GET_CORE_ID(Mpid) ARM_MPIDR_GET_AFF0(Mpid)

And we could hide that selection behind a Pcd.
Then we'd not need anything platform-specific other than overriding
the default Pcd value.

/
    Leif