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

["Sami Mujawar" <sami.mujawar@arm.com>]

Hi Leif,

I had a similar observation while reviewing the code. Please see my response inline below marked [SAMI].

Regards,

Sami Mujawar

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Leif Lindholm via groups.io
Sent: 15 December 2020 07:11 PM
To: Rebecca Cran <rebecca@nuviainc.com>
Cc: devel@edk2.groups.io; Ard Biesheuvel <Ard.Biesheuvel@arm.com>; Laszlo Ersek <lersek@redhat.com>
Subject: Re: [edk2-devel] [PATCH v4 04/10] ArmPkg: Add helper to read the Memory Model Features Register 2

+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:
[SAMI] I agree, it will be good to rename the functions.
[/SAMI]

- ArmReadIdAArch64Mmfr0
- ArmReadIdAArch64Pfr0
- ArmReadIdAArch64Pfr1
[SAMI] Or we could name them as ArmReadIdAa64Mmfr2() to closely match ID_AA64MMFR2_EL1. However, I am fine with either.
Since we are renaming some functions, we would need a clear function documentation header specifying which register is being read.
[/SAMI]

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.
[SAMI] I agree, we do not need an AArch32 prefix for registers like ID_MMFR4. These functions can be named as ArmReadIdMmfr4().
[/SAMI]

The AArch64 prototypes should then only be made available to AARCH64
code, and the AArch32 ones only to ARM.
[SAMI] I agree, in AArch64 execution state we do not need functions to read ID_MMFR4_EL1 (as an AArch64 specific register like ID_AA64MMFR2_EL1 would have the equivalent information).
[/SAMI]

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
>