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