Remove the ClusterId and CoreId fields in the ARM_CORE_INFO structure in
favor of a new Mpidr field. Update code in
ArmPlatformPkg/PrePeiCore/MainMPCore and ArmPlatformPkg/PrePi/MainMPCore.c
to use the new field and call new macros GET_MPIDR_AFF0 and GET_MPIDR_AFF1
instead.
Signed-off-by: Rebecca Cran <rebecca@nuviainc.com>
---
ArmPkg/Include/Guid/ArmMpCoreInfo.h | 3 +--
ArmPkg/Include/Library/ArmLib.h | 4 ++++
ArmPlatformPkg/Library/ArmPlatformLibNull/ArmPlatformLibNull.c | 8 ++++----
ArmPlatformPkg/PrePeiCore/MainMPCore.c | 4 +++-
ArmPlatformPkg/PrePi/MainMPCore.c | 4 +++-
5 files changed, 15 insertions(+), 8 deletions(-)
diff --git a/ArmPkg/Include/Guid/ArmMpCoreInfo.h b/ArmPkg/Include/Guid/ArmMpCoreInfo.h
index 06f9326ca021..43f0848e78b8 100644
--- a/ArmPkg/Include/Guid/ArmMpCoreInfo.h
+++ b/ArmPkg/Include/Guid/ArmMpCoreInfo.h
@@ -14,8 +14,7 @@
#define MPIDR_U_BIT_MASK 0x40000000
typedef struct {
- UINT32 ClusterId;
- UINT32 CoreId;
+ UINT64 Mpidr;
// MP Core Mailbox
EFI_PHYSICAL_ADDRESS MailboxSetAddress;
diff --git a/ArmPkg/Include/Library/ArmLib.h b/ArmPkg/Include/Library/ArmLib.h
index e4d0476090c7..6566deebdde2 100644
--- a/ArmPkg/Include/Library/ArmLib.h
+++ b/ArmPkg/Include/Library/ArmLib.h
@@ -111,6 +111,10 @@ typedef enum {
#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 GET_MPIDR_AFF0(MpId) ((MpId) & ARM_CORE_AFF0)
+#define GET_MPIDR_AFF1(MpId) (((MpId) & ARM_CORE_AFF1) >> 8)
+#define GET_MPIDR_AFF2(MpId) (((MpId) & ARM_CORE_AFF2) >> 16)
+#define GET_MPIDR_AFF3(MpId) (((MpId) & ARM_CORE_AFF3) >> 32)
#define PRIMARY_CORE_ID (PcdGet32(PcdArmPrimaryCore) & ARM_CORE_MASK)
/** Reads the CCSIDR register for the specified cache.
diff --git a/ArmPlatformPkg/Library/ArmPlatformLibNull/ArmPlatformLibNull.c b/ArmPlatformPkg/Library/ArmPlatformLibNull/ArmPlatformLibNull.c
index eeab58805e67..852275f44fc3 100644
--- a/ArmPlatformPkg/Library/ArmPlatformLibNull/ArmPlatformLibNull.c
+++ b/ArmPlatformPkg/Library/ArmPlatformLibNull/ArmPlatformLibNull.c
@@ -14,7 +14,7 @@
ARM_CORE_INFO mArmPlatformNullMpCoreInfoTable[] = {
{
// Cluster 0, Core 0
- 0x0, 0x0,
+ 0x0,
// MP Core MailBox Set/Get/Clear Addresses and Clear Value
(EFI_PHYSICAL_ADDRESS)0,
@@ -24,7 +24,7 @@ ARM_CORE_INFO mArmPlatformNullMpCoreInfoTable[] = {
},
{
// Cluster 0, Core 1
- 0x0, 0x1,
+ 0x1,
// MP Core MailBox Set/Get/Clear Addresses and Clear Value
(EFI_PHYSICAL_ADDRESS)0,
@@ -34,7 +34,7 @@ ARM_CORE_INFO mArmPlatformNullMpCoreInfoTable[] = {
},
{
// Cluster 0, Core 2
- 0x0, 0x2,
+ 0x2,
// MP Core MailBox Set/Get/Clear Addresses and Clear Value
(EFI_PHYSICAL_ADDRESS)0,
@@ -44,7 +44,7 @@ ARM_CORE_INFO mArmPlatformNullMpCoreInfoTable[] = {
},
{
// Cluster 0, Core 3
- 0x0, 0x3,
+ 0x3,
// MP Core MailBox Set/Get/Clear Addresses and Clear Value
(EFI_PHYSICAL_ADDRESS)0,
diff --git a/ArmPlatformPkg/PrePeiCore/MainMPCore.c b/ArmPlatformPkg/PrePeiCore/MainMPCore.c
index 0b8e5dfb3f30..b5d0d3a6442f 100644
--- a/ArmPlatformPkg/PrePeiCore/MainMPCore.c
+++ b/ArmPlatformPkg/PrePeiCore/MainMPCore.c
@@ -68,7 +68,9 @@ SecondaryMain (
// Find the core in the ArmCoreTable
for (Index = 0; Index < ArmCoreCount; Index++) {
- if ((ArmCoreInfoTable[Index].ClusterId == ClusterId) && (ArmCoreInfoTable[Index].CoreId == CoreId)) {
+ if ((GET_MPIDR_AFF1 (ArmCoreInfoTable[Index].Mpidr) == ClusterId) &&
+ (GET_MPIDR_AFF0 (ArmCoreInfoTable[Index].Mpidr) == CoreId))
+ {
break;
}
}
diff --git a/ArmPlatformPkg/PrePi/MainMPCore.c b/ArmPlatformPkg/PrePi/MainMPCore.c
index ce53cea6367c..68a7c13298d0 100644
--- a/ArmPlatformPkg/PrePi/MainMPCore.c
+++ b/ArmPlatformPkg/PrePi/MainMPCore.c
@@ -67,7 +67,9 @@ SecondaryMain (
// Find the core in the ArmCoreTable
for (Index = 0; Index < ArmCoreCount; Index++) {
- if ((ArmCoreInfoTable[Index].ClusterId == ClusterId) && (ArmCoreInfoTable[Index].CoreId == CoreId)) {
+ if ((GET_MPIDR_AFF1 (ArmCoreInfoTable[Index].Mpidr) == ClusterId) &&
+ (GET_MPIDR_AFF0 (ArmCoreInfoTable[Index].Mpidr) == CoreId))
+ {
break;
}
}
--
2.31.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#84950): https://edk2.groups.io/g/devel/message/84950
Mute This Topic: https://groups.io/mt/87760630/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Hello Rebecca,
On Thu, 16 Dec 2021 at 04:46, Rebecca Cran <rebecca@nuviainc.com> wrote:
>
> Remove the ClusterId and CoreId fields in the ARM_CORE_INFO structure in
> favor of a new Mpidr field. Update code in
> ArmPlatformPkg/PrePeiCore/MainMPCore and ArmPlatformPkg/PrePi/MainMPCore.c
> to use the new field and call new macros GET_MPIDR_AFF0 and GET_MPIDR_AFF1
> instead.
>
> Signed-off-by: Rebecca Cran <rebecca@nuviainc.com>
I am going to merge this patch into EDK2 in isolation, along with the
edk2-platforms changes to keep things working.
For the remainder of the series, there are two issues that I think
should be resolved. Apologies for not mentioning this before.
1) Can we make the MP protocol a separate driver? That would be
cleaner in terms of breakage of other platforms re MpInitLib, and it
would also help with the next point.
2) I don't see any management of coherency between the BSP and the APs
(unless I am missing something). I think it would be best to treat APs
as non-coherent masters (given that they boot with the MMU disabled),
which means that every variable in memory that is used to pass
information between BSP and AP needs to be DMA mapped and unmapped
appropriately, and follow the ownership rules of DMA mappings.
On platforms where none of this is needed, the DmaLib dependency can
be satisfied by CoherentDmaLib, whereas other platforms can use the
non-coherent version instead, which does all the tedious cache
maintenance.
Given that library dependencies can be specified per-driver in .DSC
file, using a separate driver permits us to pick the right DmaLib
without forcing it upon other parts of the code. It should also
prevent circular dependency issues for DmaLib implementations that
DEPEX on the CPU arch protocol produced by CpuDxe.
Thanks,
Ard.
> ---
> ArmPkg/Include/Guid/ArmMpCoreInfo.h | 3 +--
> ArmPkg/Include/Library/ArmLib.h | 4 ++++
> ArmPlatformPkg/Library/ArmPlatformLibNull/ArmPlatformLibNull.c | 8 ++++----
> ArmPlatformPkg/PrePeiCore/MainMPCore.c | 4 +++-
> ArmPlatformPkg/PrePi/MainMPCore.c | 4 +++-
> 5 files changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/ArmPkg/Include/Guid/ArmMpCoreInfo.h b/ArmPkg/Include/Guid/ArmMpCoreInfo.h
> index 06f9326ca021..43f0848e78b8 100644
> --- a/ArmPkg/Include/Guid/ArmMpCoreInfo.h
> +++ b/ArmPkg/Include/Guid/ArmMpCoreInfo.h
> @@ -14,8 +14,7 @@
> #define MPIDR_U_BIT_MASK 0x40000000
>
> typedef struct {
> - UINT32 ClusterId;
> - UINT32 CoreId;
> + UINT64 Mpidr;
>
> // MP Core Mailbox
> EFI_PHYSICAL_ADDRESS MailboxSetAddress;
> diff --git a/ArmPkg/Include/Library/ArmLib.h b/ArmPkg/Include/Library/ArmLib.h
> index e4d0476090c7..6566deebdde2 100644
> --- a/ArmPkg/Include/Library/ArmLib.h
> +++ b/ArmPkg/Include/Library/ArmLib.h
> @@ -111,6 +111,10 @@ typedef enum {
> #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 GET_MPIDR_AFF0(MpId) ((MpId) & ARM_CORE_AFF0)
> +#define GET_MPIDR_AFF1(MpId) (((MpId) & ARM_CORE_AFF1) >> 8)
> +#define GET_MPIDR_AFF2(MpId) (((MpId) & ARM_CORE_AFF2) >> 16)
> +#define GET_MPIDR_AFF3(MpId) (((MpId) & ARM_CORE_AFF3) >> 32)
> #define PRIMARY_CORE_ID (PcdGet32(PcdArmPrimaryCore) & ARM_CORE_MASK)
>
> /** Reads the CCSIDR register for the specified cache.
> diff --git a/ArmPlatformPkg/Library/ArmPlatformLibNull/ArmPlatformLibNull.c b/ArmPlatformPkg/Library/ArmPlatformLibNull/ArmPlatformLibNull.c
> index eeab58805e67..852275f44fc3 100644
> --- a/ArmPlatformPkg/Library/ArmPlatformLibNull/ArmPlatformLibNull.c
> +++ b/ArmPlatformPkg/Library/ArmPlatformLibNull/ArmPlatformLibNull.c
> @@ -14,7 +14,7 @@
> ARM_CORE_INFO mArmPlatformNullMpCoreInfoTable[] = {
> {
> // Cluster 0, Core 0
> - 0x0, 0x0,
> + 0x0,
>
> // MP Core MailBox Set/Get/Clear Addresses and Clear Value
> (EFI_PHYSICAL_ADDRESS)0,
> @@ -24,7 +24,7 @@ ARM_CORE_INFO mArmPlatformNullMpCoreInfoTable[] = {
> },
> {
> // Cluster 0, Core 1
> - 0x0, 0x1,
> + 0x1,
>
> // MP Core MailBox Set/Get/Clear Addresses and Clear Value
> (EFI_PHYSICAL_ADDRESS)0,
> @@ -34,7 +34,7 @@ ARM_CORE_INFO mArmPlatformNullMpCoreInfoTable[] = {
> },
> {
> // Cluster 0, Core 2
> - 0x0, 0x2,
> + 0x2,
>
> // MP Core MailBox Set/Get/Clear Addresses and Clear Value
> (EFI_PHYSICAL_ADDRESS)0,
> @@ -44,7 +44,7 @@ ARM_CORE_INFO mArmPlatformNullMpCoreInfoTable[] = {
> },
> {
> // Cluster 0, Core 3
> - 0x0, 0x3,
> + 0x3,
>
> // MP Core MailBox Set/Get/Clear Addresses and Clear Value
> (EFI_PHYSICAL_ADDRESS)0,
> diff --git a/ArmPlatformPkg/PrePeiCore/MainMPCore.c b/ArmPlatformPkg/PrePeiCore/MainMPCore.c
> index 0b8e5dfb3f30..b5d0d3a6442f 100644
> --- a/ArmPlatformPkg/PrePeiCore/MainMPCore.c
> +++ b/ArmPlatformPkg/PrePeiCore/MainMPCore.c
> @@ -68,7 +68,9 @@ SecondaryMain (
>
> // Find the core in the ArmCoreTable
> for (Index = 0; Index < ArmCoreCount; Index++) {
> - if ((ArmCoreInfoTable[Index].ClusterId == ClusterId) && (ArmCoreInfoTable[Index].CoreId == CoreId)) {
> + if ((GET_MPIDR_AFF1 (ArmCoreInfoTable[Index].Mpidr) == ClusterId) &&
> + (GET_MPIDR_AFF0 (ArmCoreInfoTable[Index].Mpidr) == CoreId))
> + {
> break;
> }
> }
> diff --git a/ArmPlatformPkg/PrePi/MainMPCore.c b/ArmPlatformPkg/PrePi/MainMPCore.c
> index ce53cea6367c..68a7c13298d0 100644
> --- a/ArmPlatformPkg/PrePi/MainMPCore.c
> +++ b/ArmPlatformPkg/PrePi/MainMPCore.c
> @@ -67,7 +67,9 @@ SecondaryMain (
>
> // Find the core in the ArmCoreTable
> for (Index = 0; Index < ArmCoreCount; Index++) {
> - if ((ArmCoreInfoTable[Index].ClusterId == ClusterId) && (ArmCoreInfoTable[Index].CoreId == CoreId)) {
> + if ((GET_MPIDR_AFF1 (ArmCoreInfoTable[Index].Mpidr) == ClusterId) &&
> + (GET_MPIDR_AFF0 (ArmCoreInfoTable[Index].Mpidr) == CoreId))
> + {
> break;
> }
> }
> --
> 2.31.1
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#86226): https://edk2.groups.io/g/devel/message/86226
Mute This Topic: https://groups.io/mt/87760630/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On Sun, 30 Jan 2022 at 11:44, Ard Biesheuvel <ardb@kernel.org> wrote: > > Hello Rebecca, > > On Thu, 16 Dec 2021 at 04:46, Rebecca Cran <rebecca@nuviainc.com> wrote: > > > > Remove the ClusterId and CoreId fields in the ARM_CORE_INFO structure in > > favor of a new Mpidr field. Update code in > > ArmPlatformPkg/PrePeiCore/MainMPCore and ArmPlatformPkg/PrePi/MainMPCore.c > > to use the new field and call new macros GET_MPIDR_AFF0 and GET_MPIDR_AFF1 > > instead. > > > > Signed-off-by: Rebecca Cran <rebecca@nuviainc.com> > > I am going to merge this patch into EDK2 in isolation, along with the > edk2-platforms changes to keep things working. > > For the remainder of the series, there are two issues that I think > should be resolved. Apologies for not mentioning this before. > > 1) Can we make the MP protocol a separate driver? That would be > cleaner in terms of breakage of other platforms re MpInitLib, and it > would also help with the next point. > > 2) I don't see any management of coherency between the BSP and the APs > (unless I am missing something). I think it would be best to treat APs > as non-coherent masters (given that they boot with the MMU disabled), > which means that every variable in memory that is used to pass > information between BSP and AP needs to be DMA mapped and unmapped > appropriately, and follow the ownership rules of DMA mappings. > > On platforms where none of this is needed, the DmaLib dependency can > be satisfied by CoherentDmaLib, whereas other platforms can use the > non-coherent version instead, which does all the tedious cache > maintenance. > > Given that library dependencies can be specified per-driver in .DSC > file, using a separate driver permits us to pick the right DmaLib > without forcing it upon other parts of the code. It should also > prevent circular dependency issues for DmaLib implementations that > DEPEX on the CPU arch protocol produced by CpuDxe. > I've had a stab at refactoring this code. Branch can be found here: https://github.com/ardbiesheuvel/edk2/tree/armpkg-mpservicesdxe-refactor This still needs the coherency management changes, I'll have a go at that later this week unless anyone else beats me to it. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#86231): https://edk2.groups.io/g/devel/message/86231 Mute This Topic: https://groups.io/mt/87760630/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On Mon, 31 Jan 2022 at 00:22, Ard Biesheuvel <ardb@kernel.org> wrote: > > On Sun, 30 Jan 2022 at 11:44, Ard Biesheuvel <ardb@kernel.org> wrote: > > > > Hello Rebecca, > > > > On Thu, 16 Dec 2021 at 04:46, Rebecca Cran <rebecca@nuviainc.com> wrote: > > > > > > Remove the ClusterId and CoreId fields in the ARM_CORE_INFO structure in > > > favor of a new Mpidr field. Update code in > > > ArmPlatformPkg/PrePeiCore/MainMPCore and ArmPlatformPkg/PrePi/MainMPCore.c > > > to use the new field and call new macros GET_MPIDR_AFF0 and GET_MPIDR_AFF1 > > > instead. > > > > > > Signed-off-by: Rebecca Cran <rebecca@nuviainc.com> > > > > I am going to merge this patch into EDK2 in isolation, along with the > > edk2-platforms changes to keep things working. > > > > For the remainder of the series, there are two issues that I think > > should be resolved. Apologies for not mentioning this before. > > > > 1) Can we make the MP protocol a separate driver? That would be > > cleaner in terms of breakage of other platforms re MpInitLib, and it > > would also help with the next point. > > > > 2) I don't see any management of coherency between the BSP and the APs > > (unless I am missing something). I think it would be best to treat APs > > as non-coherent masters (given that they boot with the MMU disabled), > > which means that every variable in memory that is used to pass > > information between BSP and AP needs to be DMA mapped and unmapped > > appropriately, and follow the ownership rules of DMA mappings. > > > > On platforms where none of this is needed, the DmaLib dependency can > > be satisfied by CoherentDmaLib, whereas other platforms can use the > > non-coherent version instead, which does all the tedious cache > > maintenance. > > > > Given that library dependencies can be specified per-driver in .DSC > > file, using a separate driver permits us to pick the right DmaLib > > without forcing it upon other parts of the code. It should also > > prevent circular dependency issues for DmaLib implementations that > > DEPEX on the CPU arch protocol produced by CpuDxe. > > > > I've had a stab at refactoring this code. Branch can be found here: > https://github.com/ardbiesheuvel/edk2/tree/armpkg-mpservicesdxe-refactor > OK, I've did some more work on this, and ended up with a branch that builds and runs correctly on Raspberry Pi 4. Note that it requires cache maintenance in the test app as well, or the ApFunction() routine may be sitting in the cache on the BSP, and the AP will branch to who knows where. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#86239): https://edk2.groups.io/g/devel/message/86239 Mute This Topic: https://groups.io/mt/87760630/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 1/31/22 04:42, Ard Biesheuvel wrote: > On Mon, 31 Jan 2022 at 00:22, Ard Biesheuvel<ardb@kernel.org> wrote: >> I've had a stab at refactoring this code. Branch can be found here: >> https://github.com/ardbiesheuvel/edk2/tree/armpkg-mpservicesdxe-refactor >> > OK, I've did some more work on this, and ended up with a branch that > builds and runs correctly on Raspberry Pi 4. Note that it requires > cache maintenance in the test app as well, or the ApFunction() routine > may be sitting in the cache on the BSP, and the AP will branch to who > knows where. Thanks. I'll do some testing on my end and review the changes. I did notice thatDmaLib is missing from ArmPkg/ArmPkg.dsc. -- Rebecca Cran -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#86398): https://edk2.groups.io/g/devel/message/86398 Mute This Topic: https://groups.io/mt/87760630/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.