Re: [RFC] [PATCH 0/2] Proposal to add EFI_MP_SERVICES_PROTOCOL support for AARCH64

["Leif Lindholm" <leif@nuviainc.com>]

Ard/Sami - any comments?

/
    Leif

On Tue, Sep 28, 2021 at 12:14:35 +0100, Leif Lindholm wrote:
> On Fri, Sep 24, 2021 at 20:17:50 -0600, Rebecca Cran wrote:
> > I'd like to propose adding EFI_MP_SERVICES_PROTOCOL support for
> > AARCH64 systems. I've attached two patches to implement support for it
> > in the DXE phase, based on code in EmulatorPkg and UefiCpuPkg. It's added
> > under ArmPkg for now, but longer term it should probably be moved into
> > UefiCpuPkg.
> >
> > Patch 1/2 is the start of addressing the issue that the Aff0 field of
> > the MPIDR is no longer guaranteed to be the core, and should be referred
> > to in a more generic way: for example it could be the thread, with Aff1
> > being the core and Aff2 the cluster. Clearly much more work is needed
> > to fully remove that assumption.
> 
> Just to add to this:
> Aff0 was never defined by the architecture to be the "core", it was
> just the smallest schedulable entity. The intent being that whether
> you had multiple hardware threads per core or not, you could just use
> the affinity to determine whether
> There is also a bit in the MPIDR to indicate whether the core *had*
> multiple hardware threads.
> 
> In recent processors (without any change to the architecture), ARM
> thought it would be beneficial to keep software developers on their
> toes by starting to use the hyperthreading layout even for processors
> without hyperthreading support. I.e. Aff0 is always 0 even though MT
> is 0:
> https://developer.arm.com/documentation/100798/0301/Register-descriptions/AArch64-system-registers/MPIDR-EL1--Multiprocessor-Affinity-Register--EL1
> The justification being that an SoC might contain both processors
> with and without multiple hardware threads per core.
> 
> Anyway, the point is that from at least Cortex-A76 onwards, Aff0 no
> longer maps to CoreId universally, and Aff1 no longer maps to
> ClusterId, for all non-threaded implementations.
> So we need to start cleaning up this use.
> This will possibly break some out-of-tree platforms, but I figure
> we're far enough from next stable tag for that not to matter too
> much.
> 
> > Patch 2/2 implements the EFI_MP_SERVICES_PROTOCOL for DXE in Library/MpInitLib.
> 
> Worth calling out in the cover letter that this is backed by PSCI.
> 
> /
>     Leif
> 
> > CpuDxe initializes the MP support, and as a result gains a dependency on
> > MpInitLib. ArmVirt.dsc.inc needs updated to add the new library, as will
> > all AARCH64 platforms.
> > 
> > I sent out a patch for MdeModulePkg last week to add a corresponding test
> > application, MpServicesTest (see https://edk2.groups.io/g/devel/message/80878).
> > 
> > Rebecca Cran (2):
> >   ArmPkg: Replace CoreId and ClusterId with Mpidr in ARM_CORE_INFO
> >     struct
> >   ArmPkg: Add Library/MpInitLib to support EFI_MP_SERVICES_PROTOCOL
> > 
> >  ArmPkg/ArmPkg.dec                                              |    4 +
> >  ArmPkg/ArmPkg.dsc                                              |    4 +
> >  ArmPkg/Drivers/CpuDxe/AArch64/Arch.c                           |   22 +
> >  ArmPkg/Drivers/CpuDxe/Arm/Arch.c                               |   22 +
> >  ArmPkg/Drivers/CpuDxe/CpuDxe.c                                 |    2 +
> >  ArmPkg/Drivers/CpuDxe/CpuDxe.h                                 |   10 +
> >  ArmPkg/Drivers/CpuDxe/CpuDxe.inf                               |    6 +
> >  ArmPkg/Drivers/CpuDxe/CpuMpInit.c                              |  604 ++++++++
> >  ArmPkg/Include/Guid/ArmMpCoreInfo.h                            |    3 +-
> >  ArmPkg/Include/Library/ArmLib.h                                |    4 +
> >  ArmPkg/Include/Library/MpInitLib.h                             |  366 +++++
> >  ArmPkg/Library/MpInitLib/AArch64/MpFuncs.S                     |   61 +
> >  ArmPkg/Library/MpInitLib/DxeMpInitLib.inf                      |   53 +
> >  ArmPkg/Library/MpInitLib/DxeMpLib.c                            | 1498 ++++++++++++++++++++
> >  ArmPkg/Library/MpInitLib/InternalMpInitLib.h                   |  358 +++++
> >  ArmPlatformPkg/Library/ArmPlatformLibNull/ArmPlatformLibNull.c |    8 +-
> >  ArmPlatformPkg/PrePeiCore/MainMPCore.c                         |    2 +-
> >  ArmPlatformPkg/PrePi/MainMPCore.c                              |    2 +-
> >  ArmVirtPkg/ArmVirt.dsc.inc                                     |    3 +
> >  19 files changed, 3024 insertions(+), 8 deletions(-)
> >  create mode 100644 ArmPkg/Drivers/CpuDxe/AArch64/Arch.c
> >  create mode 100644 ArmPkg/Drivers/CpuDxe/Arm/Arch.c
> >  create mode 100644 ArmPkg/Drivers/CpuDxe/CpuMpInit.c
> >  create mode 100644 ArmPkg/Include/Library/MpInitLib.h
> >  create mode 100644 ArmPkg/Library/MpInitLib/AArch64/MpFuncs.S
> >  create mode 100644 ArmPkg/Library/MpInitLib/DxeMpInitLib.inf
> >  create mode 100644 ArmPkg/Library/MpInitLib/DxeMpLib.c
> >  create mode 100644 ArmPkg/Library/MpInitLib/InternalMpInitLib.h
> > 
> > -- 
> > 2.31.1
> >