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

["Laszlo Ersek" <lersek@redhat.com>]

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

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