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

["Ard Biesheuvel" <ard.biesheuvel@arm.com>]

On 12/17/20 2:38 PM, 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@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.
> 

So did I :-)

But I think that we should raise the level of abstraction here:
something like

if (ArmReadIdPfr0 () & AARCH64_PFR0_GIC) {

should not exist in code that is shared between AArch64 and AArch32, I'd
much rather have a helper

ArmHasGicSre()

that encapsulates whatever is needed on each respective architecture,
and which may or may not end up using the same ID register or mask value.


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