Re: [PATCH v3 1/4] ArmPkg: Replace CoreId and ClusterId with Mpidr in ARM_CORE_INFO struct

["Ard Biesheuvel" <ardb@kernel.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
>