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.

Patch 2/2 implements the EFI_MP_SERVICES_PROTOCOL for DXE in Library/MpInitLib.
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



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

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
> 


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

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
> > 


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

On Thu, 7 Oct 2021 at 11:41, Leif Lindholm <leif@nuviainc.com> wrote:
>
> Ard/Sami - any comments?
>

The code changes by itself look fine to me. The only problem I see is
that we cannot run arbitrary code on other cores with the MMU off,
given that in that case, they don't comply with the UEFI AArch64
platform requirements, most notably the one that says that unaligned
accesses must be permitted.

So either we severely constrain the kind of code that we permit to run
on other cores, or we enable the MMU and caches on each core as it
comes out of reset, as well as do any other CPU specific
initialization that we do for the primary core as well.


>
> 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
> > >


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

On Thu, Oct 07, 2021 at 12:03:30 +0200, Ard Biesheuvel wrote:
> On Thu, 7 Oct 2021 at 11:41, Leif Lindholm <leif@nuviainc.com> wrote:
> >
> > Ard/Sami - any comments?
> >
> 
> The code changes by itself look fine to me. The only problem I see is
> that we cannot run arbitrary code on other cores with the MMU off,
> given that in that case,

This is a good point, and something we at the very least should point
out more explicitly - and perhaps ought to provide a helper library to
do - ...

> they don't comply with the UEFI AArch64
> platform requirements, most notably the one that says that unaligned
> accesses must be permitted.

... but I'll note that EFI_MP_SERVICES_PROTOCOL is one of those
incorrectly named protocols defined in PI, not UEFI.

> So either we severely constrain the kind of code that we permit to run
> on other cores, or we enable the MMU and caches on each core as it
> comes out of reset, as well as do any other CPU specific
> initialization that we do for the primary core as well.

The description for StartupAllAPs() has a note:
It is the responsibility of the consumer of the
EFI_MP_SERVICES_PROTOCOL.StartupAllAPs() to make sure that the nature
of the code that is executed on the BSP and the dispatched APs is well
controlled. The MP Services Protocol does not guarantee that the
Procedure function is MP-safe. Hence, the tasks that can be run in
parallel are limited to certain independent tasks and well-controlled
exclusive code. EFI services and protocols may not be called by APs
unless otherwise specified.

So I think this is actually fine, implementation-wise. *Except* for
the SwitchBSP function (where we're currently bailing out anyway).

Regards,

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
> > > >


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

On Thu, 7 Oct 2021 at 13:02, Leif Lindholm <leif@nuviainc.com> wrote:
>
> On Thu, Oct 07, 2021 at 12:03:30 +0200, Ard Biesheuvel wrote:
> > On Thu, 7 Oct 2021 at 11:41, Leif Lindholm <leif@nuviainc.com> wrote:
> > >
> > > Ard/Sami - any comments?
> > >
> >
> > The code changes by itself look fine to me. The only problem I see is
> > that we cannot run arbitrary code on other cores with the MMU off,
> > given that in that case,
>
> This is a good point, and something we at the very least should point
> out more explicitly - and perhaps ought to provide a helper library to
> do - ...
>
> > they don't comply with the UEFI AArch64
> > platform requirements, most notably the one that says that unaligned
> > accesses must be permitted.
>
> ... but I'll note that EFI_MP_SERVICES_PROTOCOL is one of those
> incorrectly named protocols defined in PI, not UEFI.
>
> > So either we severely constrain the kind of code that we permit to run
> > on other cores, or we enable the MMU and caches on each core as it
> > comes out of reset, as well as do any other CPU specific
> > initialization that we do for the primary core as well.
>
> The description for StartupAllAPs() has a note:
> It is the responsibility of the consumer of the
> EFI_MP_SERVICES_PROTOCOL.StartupAllAPs() to make sure that the nature
> of the code that is executed on the BSP and the dispatched APs is well
> controlled. The MP Services Protocol does not guarantee that the
> Procedure function is MP-safe. Hence, the tasks that can be run in
> parallel are limited to certain independent tasks and well-controlled
> exclusive code. EFI services and protocols may not be called by APs
> unless otherwise specified.
>
> So I think this is actually fine, implementation-wise. *Except* for
> the SwitchBSP function (where we're currently bailing out anyway).
>

Ok, so that doesn't look as bad as I thought. But we'll have to be
more strict than other arches: even EFI services and protocols that
are marked as safe for execution under this MP protocol are likely to
explode if they rely on CopyMem() or SetMem() for in/outputs that are
not a multiple of 8 bytes in case the platform uses the
BaseMemoryLibOptDxe flavour of this library, since it relies heavily
on deliberately misaligned loads and stores.


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

+Samer

On Fri, Oct 8, 2021 at 3:51 PM Ard Biesheuvel <ardb@kernel.org> wrote:

> > > So either we severely constrain the kind of code that we permit to run
> > > on other cores, or we enable the MMU and caches on each core as it
> > > comes out of reset, as well as do any other CPU specific
> > > initialization that we do for the primary core as well.
> >
> > The description for StartupAllAPs() has a note:
> > It is the responsibility of the consumer of the
> > EFI_MP_SERVICES_PROTOCOL.StartupAllAPs() to make sure that the nature
> > of the code that is executed on the BSP and the dispatched APs is well
> > controlled. The MP Services Protocol does not guarantee that the
> > Procedure function is MP-safe. Hence, the tasks that can be run in
> > parallel are limited to certain independent tasks and well-controlled
> > exclusive code. EFI services and protocols may not be called by APs
> > unless otherwise specified.
> >
> > So I think this is actually fine, implementation-wise. *Except* for
> > the SwitchBSP function (where we're currently bailing out anyway).
>
> Ok, so that doesn't look as bad as I thought. But we'll have to be
> more strict than other arches: even EFI services and protocols that
> are marked as safe for execution under this MP protocol are likely to
> explode if they rely on CopyMem() or SetMem() for in/outputs that are
> not a multiple of 8 bytes in case the platform uses the
> BaseMemoryLibOptDxe flavour of this library, since it relies heavily
> on deliberately misaligned loads and stores.
>

I think there is no way a protocol defined in the UEFI specification could
be
safe to use by non-BSP. In PI, the only references I find to the protocol
are
in MM and SAL protocols.
And we're not even looking at EFI_MP_SERVICES_PPI at this point.

But it might be good to hear something from ARM whether the use of this
protocol which "must be produced on any system with more than one logical
processor"
*should* be able to rely on anything being set up for it, or whether we
need an aforementioned helper library.

/
    Leif


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

> In PI, the only references I find to the protocol are in MM and SAL protocols.
> And we're not even looking at EFI_MP_SERVICES_PPI at this point.

The PI 1.7 spec defined the EFI_MP_SERVICES_PROTOCOL in page 2-180, with the PPI and MM versions in 1-193 and 4-57 respectively.


> But it might be good to hear something from ARM whether the use of this
> protocol which "must be produced on any system with more than one logical processor"
> *should* be able to rely on anything being set up for it, or whether we
> need an aforementioned helper library.

This statement (from the PI spec) is overly ambitious. I bet that it does not hold true today on most DXE-based UEFI implementations on other architectures, not just AARCH64. If we agree, I will file an ECR to remove this statement from the PI spec.

From AARCH64 SBBR systems point of view:

  *   The requirements from Arm SBBR point of view are around using PSCI to online/offline Secondary cores, and leaving them offlined before ReadyToBoot is signaled.
  *   PI-based UEFI implementations are not required. And even when they are implemented, the EFI_MP_SERVICES_PROTOCOL is not required
  *   I agree with the analysis in this thread. EFI_MP implementations on AARCh64 need to be severely limited in the general case. Platforms (upstream or downstream) can still innovate and write their own code to run in these services as they wish.


Thanks,
--Samer



From: Leif Lindholm <leif@nuviainc.com>
Sent: Monday, October 11, 2021 8:28 AM
To: Ard Biesheuvel <ardb@kernel.org>; Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Sami Mujawar <Sami.Mujawar@arm.com>; edk2-devel-groups-io <devel@edk2.groups.io>; Rebecca Cran <rebecca@nuviainc.com>; Gerd Hoffmann <kraxel@redhat.com>; edk2 RFC list <rfc@edk2.groups.io>
Subject: Re: [RFC] [PATCH 0/2] Proposal to add EFI_MP_SERVICES_PROTOCOL support for AARCH64

+Samer

On Fri, Oct 8, 2021 at 3:51 PM Ard Biesheuvel <ardb@kernel.org<mailto:ardb@kernel.org>> wrote:
> > So either we severely constrain the kind of code that we permit to run
> > on other cores, or we enable the MMU and caches on each core as it
> > comes out of reset, as well as do any other CPU specific
> > initialization that we do for the primary core as well.
>
> The description for StartupAllAPs() has a note:
> It is the responsibility of the consumer of the
> EFI_MP_SERVICES_PROTOCOL.StartupAllAPs() to make sure that the nature
> of the code that is executed on the BSP and the dispatched APs is well
> controlled. The MP Services Protocol does not guarantee that the
> Procedure function is MP-safe. Hence, the tasks that can be run in
> parallel are limited to certain independent tasks and well-controlled
> exclusive code. EFI services and protocols may not be called by APs
> unless otherwise specified.
>
> So I think this is actually fine, implementation-wise. *Except* for
> the SwitchBSP function (where we're currently bailing out anyway).

Ok, so that doesn't look as bad as I thought. But we'll have to be
more strict than other arches: even EFI services and protocols that
are marked as safe for execution under this MP protocol are likely to
explode if they rely on CopyMem() or SetMem() for in/outputs that are
not a multiple of 8 bytes in case the platform uses the
BaseMemoryLibOptDxe flavour of this library, since it relies heavily
on deliberately misaligned loads and stores.

I think there is no way a protocol defined in the UEFI specification could be
safe to use by non-BSP. In PI, the only references I find to the protocol are
in MM and SAL protocols.
And we're not even looking at EFI_MP_SERVICES_PPI at this point.

But it might be good to hear something from ARM whether the use of this
protocol which "must be produced on any system with more than one logical processor"
*should* be able to rely on anything being set up for it, or whether we
need an aforementioned helper library.

/
    Leif

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.


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

Hi Samer,

On Mon, Oct 11, 2021 at 14:20:17 +0000, Samer El-Haj-Mahmoud wrote:
> > In PI, the only references I find to the protocol are in MM and SAL protocols.
> > And we're not even looking at EFI_MP_SERVICES_PPI at this point.
> 
> The PI 1.7 spec defined the EFI_MP_SERVICES_PROTOCOL in page 2-180,
> with the PPI and MM versions in 1-193 and 4-57 respectively.

Yes, I was referring to references, as in any protocols explicitly
stating compatibility with being called from an AP.

> > But it might be good to hear something from ARM whether the use of this
> > protocol which "must be produced on any system with more than one logical processor"
> > *should* be able to rely on anything being set up for it, or whether we
> > need an aforementioned helper library.
> 
> This statement (from the PI spec) is overly ambitious. I bet that it
> does not hold true today on most DXE-based UEFI implementations on
> other architectures, not just AARCH64. If we agree, I will file an
> ECR to remove this statement from the PI spec.

That feels like a weird response to the submission of a patch set
adding the functionality for AArch64.

> 
> From AARCH64 SBBR systems point of view:
> 
>   *   The requirements from Arm SBBR point of view are around using
>       PSCI to online/offline Secondary cores, and leaving them
>       offlined before ReadyToBoot is signaled.

SBBR is not relevant here. PI covers firmware internals, not OS
boot compatibility.

>   *   PI-based UEFI implementations are not required. And even when
>       they are implemented, the EFI_MP_SERVICES_PROTOCOL is not
>       required

So ARM's strategy is to encourage people not to us it *in* PI
implementations, even when it is portably implemented on top of PSCI?

Regards,

Leif

>   *   I agree with the analysis in this thread. EFI_MP
>       implementations on AARCh64 need to be severely limited in the
>       general case. Platforms (upstream or downstream) can still
>       innovate and write their own code to run in these services as
>       they wish.

> 
> 
> Thanks,
> --Samer
> 
> 
> 
> From: Leif Lindholm <leif@nuviainc.com>
> Sent: Monday, October 11, 2021 8:28 AM
> To: Ard Biesheuvel <ardb@kernel.org>; Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Sami Mujawar <Sami.Mujawar@arm.com>; edk2-devel-groups-io <devel@edk2.groups.io>; Rebecca Cran <rebecca@nuviainc.com>; Gerd Hoffmann <kraxel@redhat.com>; edk2 RFC list <rfc@edk2.groups.io>
> Subject: Re: [RFC] [PATCH 0/2] Proposal to add EFI_MP_SERVICES_PROTOCOL support for AARCH64
> 
> +Samer
> 
> On Fri, Oct 8, 2021 at 3:51 PM Ard Biesheuvel <ardb@kernel.org<mailto:ardb@kernel.org>> wrote:
> > > So either we severely constrain the kind of code that we permit to run
> > > on other cores, or we enable the MMU and caches on each core as it
> > > comes out of reset, as well as do any other CPU specific
> > > initialization that we do for the primary core as well.
> >
> > The description for StartupAllAPs() has a note:
> > It is the responsibility of the consumer of the
> > EFI_MP_SERVICES_PROTOCOL.StartupAllAPs() to make sure that the nature
> > of the code that is executed on the BSP and the dispatched APs is well
> > controlled. The MP Services Protocol does not guarantee that the
> > Procedure function is MP-safe. Hence, the tasks that can be run in
> > parallel are limited to certain independent tasks and well-controlled
> > exclusive code. EFI services and protocols may not be called by APs
> > unless otherwise specified.
> >
> > So I think this is actually fine, implementation-wise. *Except* for
> > the SwitchBSP function (where we're currently bailing out anyway).
> 
> Ok, so that doesn't look as bad as I thought. But we'll have to be
> more strict than other arches: even EFI services and protocols that
> are marked as safe for execution under this MP protocol are likely to
> explode if they rely on CopyMem() or SetMem() for in/outputs that are
> not a multiple of 8 bytes in case the platform uses the
> BaseMemoryLibOptDxe flavour of this library, since it relies heavily
> on deliberately misaligned loads and stores.
> 
> I think there is no way a protocol defined in the UEFI specification could be
> safe to use by non-BSP. In PI, the only references I find to the protocol are
> in MM and SAL protocols.
> And we're not even looking at EFI_MP_SERVICES_PPI at this point.
> 
> But it might be good to hear something from ARM whether the use of this
> protocol which "must be produced on any system with more than one logical processor"
> *should* be able to rely on anything being set up for it, or whether we
> need an aforementioned helper library.
> 
> /
>     Leif
> 
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
> 
> 
> 
> 
> 


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

Hello Leif,

>
> > > But it might be good to hear something from ARM whether the use of
> this
> > > protocol which "must be produced on any system with more than one
> logical processor"
> > > *should* be able to rely on anything being set up for it, or whether we
> > > need an aforementioned helper library.
> >
> > This statement (from the PI spec) is overly ambitious. I bet that it
> > does not hold true today on most DXE-based UEFI implementations on
> > other architectures, not just AARCH64. If we agree, I will file an
> > ECR to remove this statement from the PI spec.
>
> That feels like a weird response to the submission of a patch set
> adding the functionality for AArch64.
>
Agree this comment is not directly related to the RFC, and should be directed to the UEFI Forum instead, so please ignore my comment.


> >
> > From AARCH64 SBBR systems point of view:
> >
> >   *   The requirements from Arm SBBR point of view are around using
> >       PSCI to online/offline Secondary cores, and leaving them
> >       offlined before ReadyToBoot is signaled.
>
> SBBR is not relevant here. PI covers firmware internals, not OS
> boot compatibility.
>
> >   *   PI-based UEFI implementations are not required. And even when
> >       they are implemented, the EFI_MP_SERVICES_PROTOCOL is not
> >       required
>
> So ARM's strategy is to encourage people not to us it *in* PI
> implementations, even when it is portably implemented on top of PSCI?
>
Arm does not have any PI specific requirements on platform firmware, beyond what is in the PI spec itself.  Arm's official requirements are only on OS boot interoperability (SBBR).

For the RFC itself, I personally do not have any objection, and welcome the addition of this protocol to AARCH64, as long as it utilizes the PSCI services to achieve the OS boot requirements.

It may be worth getting feedback from Sami since he is the EDK2 maintainer for Arm platforms.



> Regards,
>
> Leif
>
> >   *   I agree with the analysis in this thread. EFI_MP
> >       implementations on AARCh64 need to be severely limited in the
> >       general case. Platforms (upstream or downstream) can still
> >       innovate and write their own code to run in these services as
> >       they wish.
>
> >
> >
> > Thanks,
> > --Samer
> >
> >
> >
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.


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

On Mon, Oct 11, 2021 at 21:52:13 +0000, Samer El-Haj-Mahmoud wrote:
> For the RFC itself, I personally do not have any objection, and
> welcome the addition of this protocol to AARCH64, as long as it
> utilizes the PSCI services to achieve the OS boot requirements.
> 
> It may be worth getting feedback from Sami since he is the EDK2 maintainer for Arm platforms.

Sami, any comments on this set?

In fact ... we could use more help on ArmPkg in general. Would you be
up for becoming an additional Maintainer or Reviewer?

/
    Leif


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

Hi Leif,

Please find my response inline marked [SAMI].

Regards,

Sami Mujawar
On 14/10/2021 02:14 PM, Leif Lindholm wrote:
> On Mon, Oct 11, 2021 at 21:52:13 +0000, Samer El-Haj-Mahmoud wrote:
>> For the RFC itself, I personally do not have any objection, and
>> welcome the addition of this protocol to AARCH64, as long as it
>> utilizes the PSCI services to achieve the OS boot requirements.
>>
>> It may be worth getting feedback from Sami since he is the EDK2 maintainer for Arm platforms.
> Sami, any comments on this set?
[SAMI] I will start reviewing this patch set.
>
> In fact ... we could use more help on ArmPkg in general. Would you be
> up for becoming an additional Maintainer or Reviewer?
[SAMI] I can start helping with the review of the patches to start with, 
and we can revisit the maintainer role in the next 6-8 months.
Please let me know if this approach is fine with you, and I will submit 
a patch to add myself as a reviewer.
[/SAMI]
> /
>      Leif



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

On Mon, Oct 18, 2021 at 10:21:59 +0100, Sami Mujawar wrote:
> > In fact ... we could use more help on ArmPkg in general. Would you be
> > up for becoming an additional Maintainer or Reviewer?
> [SAMI] I can start helping with the review of the patches to start with, and
> we can revisit the maintainer role in the next 6-8 months.
> Please let me know if this approach is fine with you, and I will submit a
> patch to add myself as a reviewer.
> [/SAMI]

Sure, that sounds good.

/
    Leif


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

Hi Leif,

Apologies, I had forgotten to send a patch to add myself as the reviewer for ArmPkg.
I have now sent a patch to update the maintainer.txt at https://edk2.groups.io/g/devel/message/85812

Regards,

Sami Mujawar


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

On 9/28/21 5:14 AM, 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.

This patch will also break out-of-tree platforms because it causes 
ArmPkg/Drivers/CpuDxe to gain a dependency on MpInitLib.


-- 
Rebecca Cran



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