[PATCH 0/5] arm64: Allow booting with GICv3 in GICv2 mode

[PATCH 0/5] arm64: Allow booting with GICv3 in GICv2 mode

[Marc Zyngier]

Recent evolutions of the ARM Trusted Firmware have outlined issues
when the system is equipped with a GICv3 interrupt controller, but the
firmware has decided to restrict it to GICv2 compatibility mode.

In this mode, system registers cannot be enabled, and the firmware is
expected to pass a GICv2 description (DT or ACPI tables).

This series makes sure that system register access is checked at EL2
setup time and when the feature detection is performed. Additionally,
the GICv2 driver checks that system registers are disabled, and warns
if they are enabled.

The booting requirements are also updated to make the above explicit.

Marc Zyngier (5):
  arm64: el2_setup: Make sure ICC_SRE_EL2.SRE sticks before using GICv3
    sysregs
  irqchip/gic-v3: Make gic_enable_sre an inline function
  arm64: cpufeatures: Check ICC_EL1_SRE.SRE before enabling
    ARM64_HAS_SYSREG_GIC_CPUIF
  irqchip/gic: Warn if GICv3 system registers are enabled
  arm64: Update booting requirements for GICv3 in GICv2 mode

 Documentation/arm64/booting.txt    | 11 ++++++++++-
 arch/arm64/kernel/cpufeature.c     | 19 ++++++++++++++++++-
 arch/arm64/kernel/head.S           |  2 ++
 drivers/irqchip/irq-gic-v3.c       | 32 +++++++++-----------------------
 drivers/irqchip/irq-gic.c          | 15 +++++++++++++++
 include/linux/irqchip/arm-gic-v3.h | 16 ++++++++++++++++
 6 files changed, 70 insertions(+), 25 deletions(-)

-- 
2.1.4

[PATCH 1/5] arm64: el2_setup: Make sure ICC_SRE_EL2.SRE sticks before using GICv3 sysregs

[Marc Zyngier]

Contrary to what was originally expected, EL3 firmware can (for whatever
reason) disable GICv3 system register access. In this case, the kernel
explodes very early.

Work around this by testing if the SRE bit sticks or not. If it doesn't,
abort the GICv3 setup, and pray that the firmware has passed a DT that
doesn't contain a GICv3 node.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/kernel/head.S | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index 90d09ed..351a4de 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -498,6 +498,8 @@ CPU_LE(	bic	x0, x0, #(3 << 24)	)	// Clear the EE and E0E bits for EL1
 	orr	x0, x0, #ICC_SRE_EL2_ENABLE	// Set ICC_SRE_EL2.Enable==1
 	msr_s	ICC_SRE_EL2, x0
 	isb					// Make sure SRE is now set
+	mrs_s	x0, ICC_SRE_EL2			// Read SRE back,
+	tbz	x0, #0, 3f			// and check that it sticks
 	msr_s	ICH_HCR_EL2, xzr		// Reset ICC_HCR_EL2 to defaults
 
 3:
-- 
2.1.4

[PATCH 2/5] irqchip/gic-v3: Make gic_enable_sre an inline function

[Marc Zyngier]

In order for gic_enable_sre to be used by the arm64 core code,
move it to arm-gic-v3.h. As a bonus, we now also check if
system registers have been already enabled, and return early
if they have.

In all cases, the function now returns a boolean indicating if
the enabling has been successful.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/irqchip/irq-gic-v3.c       | 32 +++++++++-----------------------
 include/linux/irqchip/arm-gic-v3.h | 16 ++++++++++++++++
 2 files changed, 25 insertions(+), 23 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 36ecfc8..bfb93c32 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -139,27 +139,6 @@ static void __maybe_unused gic_write_sgi1r(u64 val)
 	asm volatile("msr_s " __stringify(ICC_SGI1R_EL1) ", %0" : : "r" (val));
 }
 
-static void gic_enable_sre(void)
-{
-	u64 val;
-
-	asm volatile("mrs_s %0, " __stringify(ICC_SRE_EL1) : "=r" (val));
-	val |= ICC_SRE_EL1_SRE;
-	asm volatile("msr_s " __stringify(ICC_SRE_EL1) ", %0" : : "r" (val));
-	isb();
-
-	/*
-	 * Need to check that the SRE bit has actually been set. If
-	 * not, it means that SRE is disabled at EL2. We're going to
-	 * die painfully, and there is nothing we can do about it.
-	 *
-	 * Kindly inform the luser.
-	 */
-	asm volatile("mrs_s %0, " __stringify(ICC_SRE_EL1) : "=r" (val));
-	if (!(val & ICC_SRE_EL1_SRE))
-		pr_err("GIC: unable to set SRE (disabled at EL2), panic ahead\n");
-}
-
 static void gic_enable_redist(bool enable)
 {
 	void __iomem *rbase;
@@ -493,8 +472,15 @@ static int gic_populate_rdist(void)
 
 static void gic_cpu_sys_reg_init(void)
 {
-	/* Enable system registers */
-	gic_enable_sre();
+	/*
+	 * Need to check that the SRE bit has actually been set. If
+	 * not, it means that SRE is disabled at EL2. We're going to
+	 * die painfully, and there is nothing we can do about it.
+	 *
+	 * Kindly inform the luser.
+	 */
+	if (!gic_enable_sre())
+		pr_err("GIC: unable to set SRE (disabled at EL2), panic ahead\n");
 
 	/* Set priority mask register */
 	gic_write_pmr(DEFAULT_PMR_VALUE);
diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
index 9eeeb95..32891b6 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -397,6 +397,22 @@ static inline void gic_write_dir(u64 irq)
 	isb();
 }
 
+static inline bool gic_enable_sre(void)
+{
+	u64 val;
+
+	asm volatile("mrs_s %0, " __stringify(ICC_SRE_EL1) : "=r" (val));
+	if (val & ICC_SRE_EL1_SRE)
+		return true;
+
+	val |= ICC_SRE_EL1_SRE;
+	asm volatile("msr_s " __stringify(ICC_SRE_EL1) ", %0" : : "r" (val));
+	isb();
+	asm volatile("mrs_s %0, " __stringify(ICC_SRE_EL1) : "=r" (val));
+
+	return !!(val & ICC_SRE_EL1_SRE);
+}
+
 struct irq_domain;
 int its_cpu_init(void);
 int its_init(struct device_node *node, struct rdists *rdists,
-- 
2.1.4

[PATCH 2/5] irqchip/gic-v3: Make gic_enable_sre an inline function

[Catalin Marinas]

On Fri, Oct 02, 2015 at 05:37:51PM +0100, Marc Zyngier wrote:
> +static inline bool gic_enable_sre(void)
> +{
> +	u64 val;
> +
> +	asm volatile("mrs_s %0, " __stringify(ICC_SRE_EL1) : "=r" (val));
> +	if (val & ICC_SRE_EL1_SRE)
> +		return true;
> +
> +	val |= ICC_SRE_EL1_SRE;
> +	asm volatile("msr_s " __stringify(ICC_SRE_EL1) ", %0" : : "r" (val));
> +	isb();
> +	asm volatile("mrs_s %0, " __stringify(ICC_SRE_EL1) : "=r" (val));
> +
> +	return !!(val & ICC_SRE_EL1_SRE);
> +}

I don't think !! is needed ;). Apparently, from ISO C99: "When any
scalar value is converted to _Bool, the result is 0 if the value
compares equal to 0; otherwise, the result is 1."

-- 
Catalin

[PATCH 2/5] irqchip/gic-v3: Make gic_enable_sre an inline function

[Marc Zyngier]

On 08/10/15 16:54, Catalin Marinas wrote:
> On Fri, Oct 02, 2015 at 05:37:51PM +0100, Marc Zyngier wrote:
>> +static inline bool gic_enable_sre(void)
>> +{
>> +	u64 val;
>> +
>> +	asm volatile("mrs_s %0, " __stringify(ICC_SRE_EL1) : "=r" (val));
>> +	if (val & ICC_SRE_EL1_SRE)
>> +		return true;
>> +
>> +	val |= ICC_SRE_EL1_SRE;
>> +	asm volatile("msr_s " __stringify(ICC_SRE_EL1) ", %0" : : "r" (val));
>> +	isb();
>> +	asm volatile("mrs_s %0, " __stringify(ICC_SRE_EL1) : "=r" (val));
>> +
>> +	return !!(val & ICC_SRE_EL1_SRE);
>> +}
> 
> I don't think !! is needed ;). Apparently, from ISO C99: "When any
> scalar value is converted to _Bool, the result is 0 if the value
> compares equal to 0; otherwise, the result is 1."

Are you going to break my fingers? ;-)

>From my very own PoV, !! acts as a reminder that I have realised what
the return type is. Yeah, that's how bad my brain is these days :-)

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 3/5] arm64: cpufeatures: Check ICC_EL1_SRE.SRE before enabling ARM64_HAS_SYSREG_GIC_CPUIF

[Marc Zyngier]

As the firmware (or the hypervisor) may have disabled SRE access,
check that SRE can actually be enabled before declaring that we
do have that capability.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/kernel/cpufeature.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 3c9aed3..305f30dc 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -23,6 +23,8 @@
 #include <asm/cpufeature.h>
 #include <asm/processor.h>
 
+#include <linux/irqchip/arm-gic-v3.h>
+
 static bool
 feature_matches(u64 reg, const struct arm64_cpu_capabilities *entry)
 {
@@ -45,11 +47,26 @@ __ID_FEAT_CHK(id_aa64pfr0);
 __ID_FEAT_CHK(id_aa64mmfr1);
 __ID_FEAT_CHK(id_aa64isar0);
 
+static bool has_useable_gicv3_cpuif(const struct arm64_cpu_capabilities *entry)
+{
+	bool has_sre;
+
+	if (!has_id_aa64pfr0_feature(entry))
+		return false;
+
+	has_sre = gic_enable_sre();
+	if (!has_sre)
+		pr_warn_once("%s present but disabled by higher exception level\n",
+			     entry->desc);
+
+	return has_sre;
+}
+
 static const struct arm64_cpu_capabilities arm64_features[] = {
 	{
 		.desc = "GIC system register CPU interface",
 		.capability = ARM64_HAS_SYSREG_GIC_CPUIF,
-		.matches = has_id_aa64pfr0_feature,
+		.matches = has_useable_gicv3_cpuif,
 		.field_pos = 24,
 		.min_field_value = 1,
 	},
-- 
2.1.4

[PATCH 4/5] irqchip/gic: Warn if GICv3 system registers are enabled

[Marc Zyngier]

When using a GICv3 in compatibility (v2) mode, having GICv3 system
register access enabled is not really compliant with the architecture.

Warn if the firmware (or the hypervisor) has been lazy.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/irqchip/irq-gic.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 982c09c..92da78f 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -51,6 +51,19 @@
 
 #include "irq-gic-common.h"
 
+#ifdef CONFIG_ARM64
+#include <asm/cpufeature.h>
+
+static void gic_check_cpu_features(void)
+{
+	WARN_TAINT_ONCE(cpus_have_cap(ARM64_HAS_SYSREG_GIC_CPUIF),
+			TAINT_CPU_OUT_OF_SPEC,
+			"GICv3 system registers enabled, broken firmware!\n");
+}
+#else
+#define gic_check_cpu_features()	do { } while(0)
+#endif
+
 union gic_base {
 	void __iomem *common_base;
 	void __percpu * __iomem *percpu_base;
@@ -987,6 +1000,8 @@ static void __init __gic_init_bases(unsigned int gic_nr, int irq_start,
 
 	BUG_ON(gic_nr >= MAX_GIC_NR);
 
+	gic_check_cpu_features();
+
 	gic = &gic_data[gic_nr];
 #ifdef CONFIG_GIC_NON_BANKED
 	if (percpu_offset) { /* Frankein-GIC without banked registers... */
-- 
2.1.4

[PATCH 5/5] arm64: Update booting requirements for GICv3 in GICv2 mode

[Marc Zyngier]

The current requirements do not describe the case where a GICv3
system gets booted with system register access disabled, and
expect the kernel to drive GICv3 in GICv2 mode.

Describe the expected settings for that particular case.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 Documentation/arm64/booting.txt | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/Documentation/arm64/booting.txt b/Documentation/arm64/booting.txt
index 7d9d3c2..369a4f4 100644
--- a/Documentation/arm64/booting.txt
+++ b/Documentation/arm64/booting.txt
@@ -173,13 +173,22 @@ Before jumping into the kernel, the following conditions must be met:
   the kernel image will be entered must be initialised by software at a
   higher exception level to prevent execution in an UNKNOWN state.
 
-  For systems with a GICv3 interrupt controller:
+  For systems with a GICv3 interrupt controller to be used in v3 mode:
   - If EL3 is present:
     ICC_SRE_EL3.Enable (bit 3) must be initialiased to 0b1.
     ICC_SRE_EL3.SRE (bit 0) must be initialised to 0b1.
   - If the kernel is entered at EL1:
     ICC.SRE_EL2.Enable (bit 3) must be initialised to 0b1
     ICC_SRE_EL2.SRE (bit 0) must be initialised to 0b1.
+  - The DT or ACPI tables must describe a GICv3 interrupt controller.
+
+  For systems with a GICv3 interrupt controller to be used in
+  compatibility (v2) mode:
+  - If EL3 is present:
+    ICC_SRE_EL3.SRE (bit 0) must be initialised to 0b0.
+  - If the kernel is entered at EL1:
+    ICC_SRE_EL2.SRE (bit 0) must be initialised to 0b0.
+  - The DT or ACPI tables must describe a GICv2 interrupt controller.
 
 The requirements described above for CPU mode, caches, MMUs, architected
 timers, coherency and system registers apply to all CPUs.  All CPUs must
-- 
2.1.4

[PATCH 0/5] arm64: Allow booting with GICv3 in GICv2 mode

[Catalin Marinas]

On Fri, Oct 02, 2015 at 05:37:49PM +0100, Marc Zyngier wrote:
> Recent evolutions of the ARM Trusted Firmware have outlined issues
> when the system is equipped with a GICv3 interrupt controller, but the
> firmware has decided to restrict it to GICv2 compatibility mode.
> 
> In this mode, system registers cannot be enabled, and the firmware is
> expected to pass a GICv2 description (DT or ACPI tables).
> 
> This series makes sure that system register access is checked at EL2
> setup time and when the feature detection is performed. Additionally,
> the GICv2 driver checks that system registers are disabled, and warns
> if they are enabled.
> 
> The booting requirements are also updated to make the above explicit.
> 
> Marc Zyngier (5):
>   arm64: el2_setup: Make sure ICC_SRE_EL2.SRE sticks before using GICv3
>     sysregs
>   irqchip/gic-v3: Make gic_enable_sre an inline function
>   arm64: cpufeatures: Check ICC_EL1_SRE.SRE before enabling
>     ARM64_HAS_SYSREG_GIC_CPUIF
>   irqchip/gic: Warn if GICv3 system registers are enabled
>   arm64: Update booting requirements for GICv3 in GICv2 mode
> 
>  Documentation/arm64/booting.txt    | 11 ++++++++++-
>  arch/arm64/kernel/cpufeature.c     | 19 ++++++++++++++++++-
>  arch/arm64/kernel/head.S           |  2 ++
>  drivers/irqchip/irq-gic-v3.c       | 32 +++++++++-----------------------
>  drivers/irqchip/irq-gic.c          | 15 +++++++++++++++
>  include/linux/irqchip/arm-gic-v3.h | 16 ++++++++++++++++
>  6 files changed, 70 insertions(+), 25 deletions(-)

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

How do you plan to merge these patches? I'm fine for them to go via the
irqchip maintainers since they are all GIC related.

-- 
Catalin

[PATCH 0/5] arm64: Allow booting with GICv3 in GICv2 mode

[Marc Zyngier]

On 08/10/15 16:56, Catalin Marinas wrote:
> On Fri, Oct 02, 2015 at 05:37:49PM +0100, Marc Zyngier wrote:
>> Recent evolutions of the ARM Trusted Firmware have outlined issues
>> when the system is equipped with a GICv3 interrupt controller, but the
>> firmware has decided to restrict it to GICv2 compatibility mode.
>>
>> In this mode, system registers cannot be enabled, and the firmware is
>> expected to pass a GICv2 description (DT or ACPI tables).
>>
>> This series makes sure that system register access is checked at EL2
>> setup time and when the feature detection is performed. Additionally,
>> the GICv2 driver checks that system registers are disabled, and warns
>> if they are enabled.
>>
>> The booting requirements are also updated to make the above explicit.
>>
>> Marc Zyngier (5):
>>   arm64: el2_setup: Make sure ICC_SRE_EL2.SRE sticks before using GICv3
>>     sysregs
>>   irqchip/gic-v3: Make gic_enable_sre an inline function
>>   arm64: cpufeatures: Check ICC_EL1_SRE.SRE before enabling
>>     ARM64_HAS_SYSREG_GIC_CPUIF
>>   irqchip/gic: Warn if GICv3 system registers are enabled
>>   arm64: Update booting requirements for GICv3 in GICv2 mode
>>
>>  Documentation/arm64/booting.txt    | 11 ++++++++++-
>>  arch/arm64/kernel/cpufeature.c     | 19 ++++++++++++++++++-
>>  arch/arm64/kernel/head.S           |  2 ++
>>  drivers/irqchip/irq-gic-v3.c       | 32 +++++++++-----------------------
>>  drivers/irqchip/irq-gic.c          | 15 +++++++++++++++
>>  include/linux/irqchip/arm-gic-v3.h | 16 ++++++++++++++++
>>  6 files changed, 70 insertions(+), 25 deletions(-)
> 
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
> 
> How do you plan to merge these patches? I'm fine for them to go via the
> irqchip maintainers since they are all GIC related.

I guess we can direct this via tip if Thomas is OK with it.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...