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

Well, the function and the caller are so close together that you
shouldn't be able to miss the comment.

I do recommend STATIC for the function though.

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

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

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