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

[Leif Lindholm]

On Thu, Dec 17, 2020 at 14:38:55 +0100, Laszlo Ersek wrote:
> On 12/15/20 20:11, Leif Lindholm wrote:
> > +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 at 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
> 
> I can follow until here... (and yes, using the concrete register names
> in the function names makes sense)
> 
> > 
> > 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.
> 
> I lost you here, sorry.

I simply meant we probably don't want to call the AArch32 accesssors
ArmReadIdAArch32Pfr0, since ID_PFR0 is the architectural name for the
AArch32 register when executing in AArch32 state.

As Sami pointed out, we could similarily just stick with the
CamelCased architectural names for AArch64 accessors rather than
spelling out AArch64.

/
    Leif

> > The AArch64 prototypes should then only be made available to AARCH64
> > code, and the AArch32 ones only to ARM.
> 
> But this again makes sense to me.
> 
> I guess what confuses me is your interpretation of "implicit" vs.
> "explicit". I'm missing what the "AArch32 aspect" means, probably.
> 
> Thanks
> laszlo
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#69144): https://edk2.groups.io/g/devel/message/69144
Mute This Topic: https://groups.io/mt/78784065/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-