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
+ );
+
#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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#68390): https://edk2.groups.io/g/devel/message/68390
Mute This Topic: https://groups.io/mt/78784065/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
+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
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.
The AArch64 prototypes should then only be made available to AARCH64
code, and the AArch32 ones only to ARM.
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
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#68887): https://edk2.groups.io/g/devel/message/68887
Mute This Topic: https://groups.io/mt/78784065/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#68949): https://edk2.groups.io/g/devel/message/68949
Mute This Topic: https://groups.io/mt/78784065/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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@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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#69126): https://edk2.groups.io/g/devel/message/69126
Mute This Topic: https://groups.io/mt/78784065/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On Thu, Dec 17, 2020 at 14:47:48 +0100, Ard Biesheuvel wrote:
> >> 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.
Yeah, agreed. In the end, that's what Rebecca's set does in patch 6/10
- so I'll whip something together to replicate this for the GIC case.
I like the ArmHas* format though - Rebecca, could you use that for
your v5?
>
> >>
> >> 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 (#69145): https://edk2.groups.io/g/devel/message/69145
Mute This Topic: https://groups.io/mt/78784065/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 12/17/20 11:04 AM, Leif Lindholm wrote: > I like the ArmHas* format though - Rebecca, could you use that for > your v5? Yes, I'll do that. -- Rebecca Cran -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#69146): https://edk2.groups.io/g/devel/message/69146 Mute This Topic: https://groups.io/mt/78784065/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.