Re: [PATCH v4 04/10] ArmPkg: Add helper to read the Memory Model Features Register 2

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

+Laszlo

Ard, I could use your input on the below, and Laszlo might also have
an opinion:

On Mon, Dec 07, 2020 at 10:54:21 -0700, Rebecca Cran wrote:
> Add helper function to read the MMFR2 register. We will need this to
> determine CCIDX support.
> 
> Signed-off-by: Rebecca Cran <rebecca@nuviainc.com>
> ---
>  ArmPkg/Library/ArmLib/AArch64/AArch64Lib.h     | 6 ++++++
>  ArmPkg/Library/ArmLib/AArch64/AArch64Support.S | 3 +++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.h b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.h
> index b2c8a8ea0b84..d6bcfc3b82ae 100644
> --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.h
> +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.h
> @@ -35,5 +35,11 @@ ArmCleanInvalidateDataCacheEntryBySetWay (
>    IN  UINTN   SetWayFormat
>    );
>  
> +UINTN
> +EFIAPI
> +ArmReadIdMmfr2 (
> +  VOID
> +  );
> +

First of all, I think this prototype belongs in
Include/Library/ArmLib.h ... but!

So, there are a lot of system registers, many of which share at least
the view of the bottom 32 bits between aarch64/aarch32 versions.

This isn't true for the ID registers - which are always 64-bit for
aarch64 state, and always 32-bit for aarch32, where aarch64 have
access to both.

So this helper function isn't generic - in this particular case, we're
adding this accessor because we want to determine CCIDX support.
For aarch64 this means ID_AA64MMFR2_EL1, but for aarch32 this means
ID_MMFR4 (also accessible from aarch64 as ID_MMFR4_EL1).

We already have ArmReadIdPfr0 and ArmReadIdPfr1 in ArmLib.h, already
being made use of, helping to demonstrate the problem:

ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c:  if (ArmReadIdPfr0 () & AARCH64_PFR0_GIC) {
ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c:  if (ArmReadIdPfr1 () & ARM_PFR1_GIC) {

I would propose that since the high-level abstraction serve only to
confuse things, we change existing (and new) accessors to ID registers
to be explicit:

- ArmReadIdAArch64Mmfr0
- ArmReadIdAArch64Pfr0
- ArmReadIdAArch64Pfr1

The question is whether we should make the AArch32 aspect explicit or
implicit? My instinctive reaction is the latter. This matches the
native naming scheme used in the ARM ARM, and we don't support mixing
instruction set widths in UEFI.

The AArch64 prototypes should then only be made available to AARCH64
code, and the AArch32 ones only to ARM.

Does the above makes sense to everyone?

Best Regards,

Leif

>  #endif // __AARCH64_LIB_H__
>  
> diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
> index 199374ff59e3..874bc2866ac3 100644
> --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
> +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
> @@ -424,6 +424,9 @@ ASM_FUNC(ArmCallWFI)
>    wfi
>    ret
>  
> +ASM_FUNC(ArmReadIdMmfr2)
> +  mrs   x0, ID_AA64MMFR2_EL1           // read EL1 MMFR2
> +  ret
>  
>  ASM_FUNC(ArmReadMpidr)
>    mrs   x0, mpidr_el1           // read EL1 MPIDR
> -- 
> 2.26.2
>