On 31 July 2015 at 13:28, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:Well, the function and the caller are so close together that youOn 31 July 2015 at 14:21, Ryan Harkin <ryan.harkin@linaro.org> wrote:
>
>
> On 31 July 2015 at 13:13, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>
>> On 31 July 2015 at 14:06, Ryan Harkin <ryan.harkin@linaro.org> wrote:
>> > Make Arm and Aarch64 both use the same code, conditionally compiled, to
>> > check if the platform has GICv3.
>> >
>> > Both source files for Arm and Aarch64 are now identical and the next
>> > step is to move to a common source file.
>> >
>> > Contributed-under: TianoCore Contribution Agreement 1.0
>> > Signed-off-by: Ryan Harkin <ryan.harkin@linaro.org>
>> > ---
>> > .../Library/ArmGicArchLib/AArch64/ArmGicArchLib.c | 26
>> > +++++++++++++++-------
>> > ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c | 24
>> > ++++++++++++++------
>> > 2 files changed, 35 insertions(+), 15 deletions(-)
>> >
>> > diff --git a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
>> > b/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
>> > index 9853c7b..b092e3a 100644
>> > --- a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
>> > +++ b/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
>> > @@ -15,7 +15,22 @@
>> > #include <Library/ArmLib.h>
>> > #include <Library/ArmGicLib.h>
>> >
>> > -STATIC ARM_GIC_ARCH_REVISION mGicArchRevision;
>> > +STATIC ARM_GIC_ARCH_REVISION mGicArchRevision =
>> > ARM_GIC_ARCH_REVISION_2;
>> > +
>> > +RETURN_STATUS
>> > +EFIAPI
>> > +ArmGicArchLibHasGicv3 (
>> > + VOID
>> > + )
>> > +{
>> > +#if defined (MDE_CPU_ARM)
>> > + return (ArmReadIdPfr1 () & ARM_PFR1_GIC);
>> > +#elif defined(MDE_CPU_AARCH64)
>> > + return (ArmReadIdPfr0 () & AARCH64_PFR0_GIC);
>> > +#else
>> > + #error "Unknown chipset."
>> > +#endif
>> > +}
>> >
>>
>> Officially, the bit indicates whether the GIC system register
>> interface is supported at the current exception level, and from that
>> we infer that the system most likely has a GICv3.
>> So if we don't have the SRE enabled, we try to drive it as a GICv2
>> which only happens to work if this particular GICv3 has the GICv2
>> compatibility mode implemented. (This is also mentioned in the
>> comments)
>>
>> So the function name is a bit misleading here, although I won't insist
>> that you change it.
>
>
> I'm happy to change it if you want to recommend a better name, this seems a
> but unwieldy:
>
> HasGicv3 -> GicSystemRegistersSupported
>
> ... but is perhaps more correct.
>
shouldn't be able to miss the comment.
I do recommend STATIC for the function though.Sure, good point. I'll add it for v2.
>>
>>
>> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
>
> Thanks for the quick review turnaround :-)
Too quick, as it seems. Spotted an inadvertent white space change below :-)Drat, I would have got away with it if it wasn't for you meddling kids :-)
>>
>>
>> > RETURN_STATUS
>> > EFIAPI
>> > @@ -31,7 +46,7 @@ ArmGicArchLibInitialize (
>> > // feature is implemented on the CPU. This is also convenient as our
>> > GICv3
>> > // driver requires SRE. If only Memory mapped access is available we
>> > try to
>> > // drive the GIC as a v2.
>> > - if (ArmReadIdPfr0 () & AARCH64_PFR0_GIC) {
>> > + if (ArmGicArchLibHasGicv3()) {
>> > // Make sure System Register access is enabled (SRE). This depends
>> > on the
>> > // higher privilege level giving us permission, otherwise we will
>> > either
>> > // cause an exception here, or the write doesn't stick in which
>> > case we need
>> > @@ -41,18 +56,13 @@ ArmGicArchLibInitialize (
>> > // It is the OS responsibility to set this bit.
>> > IccSre = ArmGicV3GetControlSystemRegisterEnable ();
>> > if (!(IccSre & ICC_SRE_EL2_SRE)) {
>> > - ArmGicV3SetControlSystemRegisterEnable (IccSre |
>> > ICC_SRE_EL2_SRE);
>> > + ArmGicV3SetControlSystemRegisterEnable (IccSre| ICC_SRE_EL2_SRE);
here ^^^I sort-of did that on purpose to make the two files identical. The space vanished when the original copy/paste/hack operation happened and it crept back in when I made the Arm and Aarch64 files identical.I know it's not important, but if I omit that whitespace change, then the two files won't actually be identical before moving to a single common source.Now, do you *really* want me to drop this hunk for v2??
>> > IccSre = ArmGicV3GetControlSystemRegisterEnable ();
>> > }
>> > if (IccSre & ICC_SRE_EL2_SRE) {
>> > mGicArchRevision = ARM_GIC_ARCH_REVISION_3;
>> > - goto Done;
>> > }
>> > }
>> > -
>> > - mGicArchRevision = ARM_GIC_ARCH_REVISION_2;
>> > -
>> > -Done:
>> > return RETURN_SUCCESS;
>> > }
>> >
>> > diff --git a/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
>> > b/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
>> > index f8822a2..b092e3a 100644
>> > --- a/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
>> > +++ b/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
>> > @@ -15,7 +15,22 @@
>> > #include <Library/ArmLib.h>
>> > #include <Library/ArmGicLib.h>
>> >
>> > -STATIC ARM_GIC_ARCH_REVISION mGicArchRevision;
>> > +STATIC ARM_GIC_ARCH_REVISION mGicArchRevision =
>> > ARM_GIC_ARCH_REVISION_2;
>> > +
>> > +RETURN_STATUS
>> > +EFIAPI
>> > +ArmGicArchLibHasGicv3 (
>> > + VOID
>> > + )
>> > +{
>> > +#if defined (MDE_CPU_ARM)
>> > + return (ArmReadIdPfr1 () & ARM_PFR1_GIC);
>> > +#elif defined(MDE_CPU_AARCH64)
>> > + return (ArmReadIdPfr0 () & AARCH64_PFR0_GIC);
>> > +#else
>> > + #error "Unknown chipset."
>> > +#endif
>> > +}
>> >
>> > RETURN_STATUS
>> > EFIAPI
>> > @@ -31,7 +46,7 @@ ArmGicArchLibInitialize (
>> > // feature is implemented on the CPU. This is also convenient as our
>> > GICv3
>> > // driver requires SRE. If only Memory mapped access is available we
>> > try to
>> > // drive the GIC as a v2.
>> > - if (ArmReadIdPfr1 () & ARM_PFR1_GIC) {
>> > + if (ArmGicArchLibHasGicv3()) {
>> > // Make sure System Register access is enabled (SRE). This depends
>> > on the
>> > // higher privilege level giving us permission, otherwise we will
>> > either
>> > // cause an exception here, or the write doesn't stick in which
>> > case we need
>> > @@ -46,13 +61,8 @@ ArmGicArchLibInitialize (
>> > }
>> > if (IccSre & ICC_SRE_EL2_SRE) {
>> > mGicArchRevision = ARM_GIC_ARCH_REVISION_3;
>> > - goto Done;
>> > }
>> > }
>> > -
>> > - mGicArchRevision = ARM_GIC_ARCH_REVISION_2;
>> > -
>> > -Done:
>> > return RETURN_SUCCESS;
>> > }
>> >
>> > --
>> > 2.1.0
>> >
>
>