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.

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

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