[edk2-devel] [PATCH v4 04/10] ArmPkg: Add helper to read the Memory Model Features Register 2
Laszlo Ersek
lersek at redhat.com
Thu Dec 17 13:38:55 UTC 2020
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.
>
> 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 (#69125): https://edk2.groups.io/g/devel/message/69125
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]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list