diff mbox

[v11,06/19] irqchip: gic: Provide support for interrupt grouping

Message ID 1409662853-29313-7-git-send-email-daniel.thompson@linaro.org
State New
Headers show

Commit Message

Daniel Thompson Sept. 2, 2014, 1 p.m. UTC 
All GIC hardware except GICv1-without-TrustZone support provides a means
to group exceptions into group 0 (which can optionally be signally using
use FIQ) and group 1. The kernel currently provides no means to exploit
this. This patch alters the initialization of the GIC to place all
interrupts into group 1 which is the foundational requirement to meaningfully
use FIQ.

Note that the hardware functionality is unavailable to the kernel when a
secure monitor is present because access to the grouping registers are
prohibited outside "secure world" (this feature allows grouping to be
used to allow hardware peripherals to send interrupts into the secure
world). The GIC driver will automatically detect this and disable its
attempts to group interrupts.

On systems without TrustZone support the kernel has the power to route
interrupt sources to FIQ, potentially allowing a driver to exploit the
NMI-like properties of FIQ.

Tested on Freescale i.MX6 (quad A9), STiH416 (dual A9) and a self-written
qemu GICv2 model.

Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
Cc: Christoffer Dall <christoffer.dall@linaro.org>
Cc: Sricharan R <r.sricharan@ti.com>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Acked-by: Dirk Behme <dirk.behme@de.bosch.com>
---
 drivers/irqchip/irq-gic.c | 99 ++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 94 insertions(+), 5 deletions(-)

Comments

Russell King - ARM Linux Sept. 2, 2014, 7:33 p.m. UTC | #1 
On Tue, Sep 02, 2014 at 02:00:40PM +0100, Daniel Thompson wrote:
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index 4b959e6..423707c 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -41,6 +41,9 @@
>  #include <linux/irqchip/arm-gic.h>
>  
>  #include <asm/cputype.h>
> +#ifdef CONFIG_FIQ
> +#include <asm/fiq.h>
> +#endif

Is there much advantage to this ifdef over providing a dummy asm/fiq.h
in ARM64?

>  #include <asm/irq.h>
>  #include <asm/exception.h>
>  #include <asm/smp_plat.h>
> @@ -68,6 +71,9 @@ struct gic_chip_data {
>  #ifdef CONFIG_GIC_NON_BANKED
>  	void __iomem *(*get_base)(union gic_base *);
>  #endif
> +#ifdef CONFIG_FIQ
> +	bool fiq_enable;
> +#endif
>  };
>  
>  static DEFINE_RAW_SPINLOCK(irq_controller_lock);
> @@ -131,6 +137,16 @@ static inline void gic_set_base_accessor(struct gic_chip_data *data,
>  #define gic_set_base_accessor(d, f)
>  #endif
>  
> +#ifdef CONFIG_FIQ
> +static inline bool gic_data_fiq_enable(struct gic_chip_data *data)
> +{
> +	return data->fiq_enable;
> +}
> +#else
> +static inline bool gic_data_fiq_enable(
> +		struct gic_chip_data *data) { return false; }

I really hate this style.  Just lay it out as a normal function.

> +	/*
> +	 * If grouping is not available (not implemented or prohibited by
> +	 * security mode) these registers a read-as-zero/write-ignored.
> +	 * However as a precaution we restore the reset default regardless of
> +	 * the result of the test.
> +	 */

Have we found that this additional complexity is actually necessary?
If not, we're over-engineering the code, making it more complex (and
hence more likely to be buggy) for very little reason.

Last night, I booted an unconditional version of this on OMAP3430, and
OMAP4430.  It's also been booted on the range of iMX6 CPUs.  Nothing
here has shown any signs of problems to having these registers written.

> +	/*
> +	 * Set EnableGrp1/EnableGrp0 (bit 1 and 0) or EnableGrp (bit 0 only,
> +	 * bit 1 ignored)
> +	 */
> +	if (gic_data_fiq_enable(gic))
> +		writel_relaxed(3, base + GIC_DIST_CTRL);
> +	else
> +		writel_relaxed(1, base + GIC_DIST_CTRL);

If we are going to do this conditionally, and the only thing which
is variable is the value to be written, I much prefer the conditional
bit to be on the value and not the write.  The compiler doesn't always
optimise these things very well.  So:

	writel_relaxed(gic_data_fiq_enable(gic) ? 3 : 1, base + GIC_DIST_CTRL);

works well enough for me.  If you feel better by using a temporary
local, that works for me too.

> +	if (gic_data_fiq_enable(gic))
> +		writel_relaxed(0x1f, base + GIC_CPU_CTRL);
> +	else
> +		writel_relaxed(1, base + GIC_CPU_CTRL);

Same here.

> @@ -485,7 +564,10 @@ static void gic_dist_restore(unsigned int gic_nr)
>  		writel_relaxed(gic_data[gic_nr].saved_spi_enable[i],
>  			dist_base + GIC_DIST_ENABLE_SET + i * 4);
>  
> -	writel_relaxed(1, dist_base + GIC_DIST_CTRL);
> +	if (gic_data_fiq_enable(&gic_data[gic_nr]))
> +		writel_relaxed(3, dist_base + GIC_DIST_CTRL);
> +	else
> +		writel_relaxed(1, dist_base + GIC_DIST_CTRL);

And here.

> @@ -542,7 +624,7 @@ static void gic_cpu_restore(unsigned int gic_nr)
>  		writel_relaxed(0xa0a0a0a0, dist_base + GIC_DIST_PRI + i * 4);
>  
>  	writel_relaxed(0xf0, cpu_base + GIC_CPU_PRIMASK);
> -	writel_relaxed(1, cpu_base + GIC_CPU_CTRL);
> +	writel_relaxed(0x1f, cpu_base + GIC_CPU_CTRL);

Interestingly, here you write 0x1f unconditionally.
>  }
>  
>  static int gic_notifier(struct notifier_block *self, unsigned long cmd,	void *v)
> @@ -604,6 +686,7 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
>  {
>  	int cpu;
>  	unsigned long flags, map = 0;
> +	unsigned long softint;
>  
>  	raw_spin_lock_irqsave(&irq_controller_lock, flags);
>  
> @@ -618,7 +701,11 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
>  	dmb(ishst);
>  
>  	/* this always happens on GIC0 */
> -	writel_relaxed(map << 16 | irq, gic_data_dist_base(&gic_data[0]) + GIC_DIST_SOFTINT);
> +	softint = map << 16 | irq;
> +	if (gic_data_fiq_enable(&gic_data[0]))
> +		softint |= 0x8000;

I guess that this always has to be done conditionally.  I'd prefer this
test to be done slightly differently (and we might as well wrap in a bit
of patch 9 here):

	if (sgi_is_nonsecure(irq, &gic_data[0]))
		softint |= 0x8000;

which follows the true purpose of that bit.  This bit only has effect if
we are running in secure mode, where it must match the status of the
target interrupt (as programmed into GIC_DIST_IGROUP).

We probably should do this based on a bitmask of SGIs in the
gic_chip_data, which is initialised according to how we've been able
to setup the GIC_DIST_IGROUP register(s).
Catalin Marinas Sept. 2, 2014, 9:36 p.m. UTC | #2 
On 2 Sep 2014, at 20:33, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> On Tue, Sep 02, 2014 at 02:00:40PM +0100, Daniel Thompson wrote:
>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
>> index 4b959e6..423707c 100644
>> --- a/drivers/irqchip/irq-gic.c
>> +++ b/drivers/irqchip/irq-gic.c
>> @@ -41,6 +41,9 @@
>> #include <linux/irqchip/arm-gic.h>
>> 
>> #include <asm/cputype.h>
>> +#ifdef CONFIG_FIQ
>> +#include <asm/fiq.h>
>> +#endif
> 
> Is there much advantage to this ifdef over providing a dummy asm/fiq.h
> in ARM64?

While it’s unlikely we’ll use FIQs on arm64 (they are generally
reserved for the secure world/firmware), I don’t mind an empty
asm/fiq.h file.

Catalin
Daniel Thompson Sept. 3, 2014, 9:28 a.m. UTC | #3 
On 02/09/14 20:33, Russell King - ARM Linux wrote:
> On Tue, Sep 02, 2014 at 02:00:40PM +0100, Daniel Thompson wrote:
>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
>> index 4b959e6..423707c 100644
>> --- a/drivers/irqchip/irq-gic.c
>> +++ b/drivers/irqchip/irq-gic.c
>> @@ -41,6 +41,9 @@
>>  #include <linux/irqchip/arm-gic.h>
>>  
>>  #include <asm/cputype.h>
>> +#ifdef CONFIG_FIQ
>> +#include <asm/fiq.h>
>> +#endif
> 
> Is there much advantage to this ifdef over providing a dummy asm/fiq.h
> in ARM64?
> 
>>  #include <asm/irq.h>
>>  #include <asm/exception.h>
>>  #include <asm/smp_plat.h>
>> @@ -68,6 +71,9 @@ struct gic_chip_data {
>>  #ifdef CONFIG_GIC_NON_BANKED
>>  	void __iomem *(*get_base)(union gic_base *);
>>  #endif
>> +#ifdef CONFIG_FIQ
>> +	bool fiq_enable;
>> +#endif
>>  };
>>  
>>  static DEFINE_RAW_SPINLOCK(irq_controller_lock);
>> @@ -131,6 +137,16 @@ static inline void gic_set_base_accessor(struct gic_chip_data *data,
>>  #define gic_set_base_accessor(d, f)
>>  #endif
>>  
>> +#ifdef CONFIG_FIQ
>> +static inline bool gic_data_fiq_enable(struct gic_chip_data *data)
>> +{
>> +	return data->fiq_enable;
>> +}
>> +#else
>> +static inline bool gic_data_fiq_enable(
>> +		struct gic_chip_data *data) { return false; }
> 
> I really hate this style.  Just lay it out as a normal function.

Will do.


>> +	/*
>> +	 * If grouping is not available (not implemented or prohibited by
>> +	 * security mode) these registers a read-as-zero/write-ignored.
>> +	 * However as a precaution we restore the reset default regardless of
>> +	 * the result of the test.
>> +	 */
> 
> Have we found that this additional complexity is actually necessary?
> If not, we're over-engineering the code, making it more complex (and
> hence more likely to be buggy) for very little reason.
>
> Last night, I booted an unconditional version of this on OMAP3430, and
> OMAP4430.  It's also been booted on the range of iMX6 CPUs.  Nothing
> here has shown any signs of problems to having these registers written.

No, I haven't proven that most of the conditional code based on
gic_data_fiq_enable() is required.

I should certainly be safe to remove the conditional code from registers
specified was RAZ/WI.

I suspect we could also remove it from registers with bits that are
reserved (although spec. doesn't not state this explicitly).

I could aggressively remove the conditionals and keep the current code
on a branch to make it quicker to support any reported regressions.


>> +	/*
>> +	 * Set EnableGrp1/EnableGrp0 (bit 1 and 0) or EnableGrp (bit 0 only,
>> +	 * bit 1 ignored)
>> +	 */
>> +	if (gic_data_fiq_enable(gic))
>> +		writel_relaxed(3, base + GIC_DIST_CTRL);
>> +	else
>> +		writel_relaxed(1, base + GIC_DIST_CTRL);
> 
> If we are going to do this conditionally, and the only thing which
> is variable is the value to be written, I much prefer the conditional
> bit to be on the value and not the write.  The compiler doesn't always
> optimise these things very well.  So:
> 
> 	writel_relaxed(gic_data_fiq_enable(gic) ? 3 : 1, base + GIC_DIST_CTRL);
> 
> works well enough for me.  If you feel better by using a temporary
> local, that works for me too.

Ternary is fine by me although I'm inclined to be more aggressive with
removal of conditional code.

> 
>> +	if (gic_data_fiq_enable(gic))
>> +		writel_relaxed(0x1f, base + GIC_CPU_CTRL);
>> +	else
>> +		writel_relaxed(1, base + GIC_CPU_CTRL);
> 
> Same here.

Ok.

>> @@ -485,7 +564,10 @@ static void gic_dist_restore(unsigned int gic_nr)
>>  		writel_relaxed(gic_data[gic_nr].saved_spi_enable[i],
>>  			dist_base + GIC_DIST_ENABLE_SET + i * 4);
>>  
>> -	writel_relaxed(1, dist_base + GIC_DIST_CTRL);
>> +	if (gic_data_fiq_enable(&gic_data[gic_nr]))
>> +		writel_relaxed(3, dist_base + GIC_DIST_CTRL);
>> +	else
>> +		writel_relaxed(1, dist_base + GIC_DIST_CTRL);
> 
> And here.

Ok.

>> @@ -542,7 +624,7 @@ static void gic_cpu_restore(unsigned int gic_nr)
>>  		writel_relaxed(0xa0a0a0a0, dist_base + GIC_DIST_PRI + i * 4);
>>  
>>  	writel_relaxed(0xf0, cpu_base + GIC_CPU_PRIMASK);
>> -	writel_relaxed(1, cpu_base + GIC_CPU_CTRL);
>> +	writel_relaxed(0x1f, cpu_base + GIC_CPU_CTRL);
> 
> Interestingly, here you write 0x1f unconditionally.

Oops. Can I pretend this was was a premonition?


>>  }
>>  
>>  static int gic_notifier(struct notifier_block *self, unsigned long cmd,	void *v)
>> @@ -604,6 +686,7 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
>>  {
>>  	int cpu;
>>  	unsigned long flags, map = 0;
>> +	unsigned long softint;
>>  
>>  	raw_spin_lock_irqsave(&irq_controller_lock, flags);
>>  
>> @@ -618,7 +701,11 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
>>  	dmb(ishst);
>>  
>>  	/* this always happens on GIC0 */
>> -	writel_relaxed(map << 16 | irq, gic_data_dist_base(&gic_data[0]) + GIC_DIST_SOFTINT);
>> +	softint = map << 16 | irq;
>> +	if (gic_data_fiq_enable(&gic_data[0]))
>> +		softint |= 0x8000;
> 
> I guess that this always has to be done conditionally.  I'd prefer this
> test to be done slightly differently (and we might as well wrap in a bit
> of patch 9 here):
> 
> 	if (sgi_is_nonsecure(irq, &gic_data[0]))
> 		softint |= 0x8000;
> 
> which follows the true purpose of that bit.  This bit only has effect if
> we are running in secure mode, where it must match the status of the
> target interrupt (as programmed into GIC_DIST_IGROUP).
> 
> We probably should do this based on a bitmask of SGIs in the
> gic_chip_data, which is initialised according to how we've been able
> to setup the GIC_DIST_IGROUP register(s).

Will do.
Daniel Thompson Sept. 3, 2014, 9:44 a.m. UTC | #4 
On 02/09/14 22:36, Catalin Marinas wrote:
> On 2 Sep 2014, at 20:33, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
>> On Tue, Sep 02, 2014 at 02:00:40PM +0100, Daniel Thompson wrote:
>>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
>>> index 4b959e6..423707c 100644
>>> --- a/drivers/irqchip/irq-gic.c
>>> +++ b/drivers/irqchip/irq-gic.c
>>> @@ -41,6 +41,9 @@
>>> #include <linux/irqchip/arm-gic.h>
>>>
>>> #include <asm/cputype.h>
>>> +#ifdef CONFIG_FIQ
>>> +#include <asm/fiq.h>
>>> +#endif
>>
>> Is there much advantage to this ifdef over providing a dummy asm/fiq.h
>> in ARM64?
> 
> While it’s unlikely we’ll use FIQs on arm64 (they are generally
> reserved for the secure world/firmware), I don’t mind an empty
> asm/fiq.h file.

Thanks Catalin, Thanks Russell.

I will do this.
diff mbox

Patch

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 4b959e6..423707c 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -41,6 +41,9 @@ 
 #include <linux/irqchip/arm-gic.h>
 
 #include <asm/cputype.h>
+#ifdef CONFIG_FIQ
+#include <asm/fiq.h>
+#endif
 #include <asm/irq.h>
 #include <asm/exception.h>
 #include <asm/smp_plat.h>
@@ -68,6 +71,9 @@  struct gic_chip_data {
 #ifdef CONFIG_GIC_NON_BANKED
 	void __iomem *(*get_base)(union gic_base *);
 #endif
+#ifdef CONFIG_FIQ
+	bool fiq_enable;
+#endif
 };
 
 static DEFINE_RAW_SPINLOCK(irq_controller_lock);
@@ -131,6 +137,16 @@  static inline void gic_set_base_accessor(struct gic_chip_data *data,
 #define gic_set_base_accessor(d, f)
 #endif
 
+#ifdef CONFIG_FIQ
+static inline bool gic_data_fiq_enable(struct gic_chip_data *data)
+{
+	return data->fiq_enable;
+}
+#else
+static inline bool gic_data_fiq_enable(
+		struct gic_chip_data *data) { return false; }
+#endif
+
 static inline void __iomem *gic_dist_base(struct irq_data *d)
 {
 	struct gic_chip_data *gic_data = irq_data_get_irq_chip_data(d);
@@ -325,6 +341,42 @@  static struct irq_chip gic_chip = {
 	.irq_set_wake		= gic_set_wake,
 };
 
+#ifdef CONFIG_FIQ
+static void __init gic_init_fiq(struct gic_chip_data *gic,
+				irq_hw_number_t first_irq,
+				unsigned int num_irqs)
+{
+	void __iomem *dist_base = gic_data_dist_base(gic_data);
+	unsigned int i;
+
+	/*
+	 * FIQ can only be supported on platforms without an extended irq_eoi
+	 * method (otherwise we take a lock during eoi handling).
+	 */
+	if (gic_arch_extn.irq_eoi)
+		return;
+
+	/*
+	 * If grouping is not available (not implemented or prohibited by
+	 * security mode) these registers a read-as-zero/write-ignored.
+	 * However as a precaution we restore the reset default regardless of
+	 * the result of the test.
+	 */
+	writel_relaxed(1, dist_base + GIC_DIST_IGROUP + 0);
+	gic->fiq_enable = readl_relaxed(dist_base + GIC_DIST_IGROUP + 0);
+	writel_relaxed(0, dist_base + GIC_DIST_IGROUP + 0);
+	pr_debug("gic: FIQ support %s\n",
+		 gic->fiq_enable ? "enabled" : "disabled");
+
+	if (!gic->fiq_enable)
+		return;
+}
+#else /* CONFIG_FIQ */
+static inline void gic_init_fiq(struct gic_chip_data *gic,
+				irq_hw_number_t first_irq,
+				unsigned int num_irqs) {}
+#endif /* CONFIG_FIQ */
+
 void __init gic_cascade_irq(unsigned int gic_nr, unsigned int irq)
 {
 	if (gic_nr >= MAX_GIC_NR)
@@ -373,7 +425,22 @@  static void __init gic_dist_init(struct gic_chip_data *gic)
 
 	gic_dist_config(base, gic_irqs, NULL);
 
-	writel_relaxed(1, base + GIC_DIST_CTRL);
+	/*
+	 * Optionally set all global interrupts to be group 1.
+	 */
+	if (gic_data_fiq_enable(gic))
+		for (i = 32; i < gic_irqs; i += 32)
+			writel_relaxed(0xffffffff,
+				       base + GIC_DIST_IGROUP + i * 4 / 32);
+
+	/*
+	 * Set EnableGrp1/EnableGrp0 (bit 1 and 0) or EnableGrp (bit 0 only,
+	 * bit 1 ignored)
+	 */
+	if (gic_data_fiq_enable(gic))
+		writel_relaxed(3, base + GIC_DIST_CTRL);
+	else
+		writel_relaxed(1, base + GIC_DIST_CTRL);
 }
 
 static void gic_cpu_init(struct gic_chip_data *gic)
@@ -400,8 +467,20 @@  static void gic_cpu_init(struct gic_chip_data *gic)
 
 	gic_cpu_config(dist_base, NULL);
 
+	/*
+	 * Set all PPI and SGI interrupts to be group 1.
+	 *
+	 * If grouping is not available (not implemented or prohibited by
+	 * security mode) these registers are read-as-zero/write-ignored.
+	 */
+	if (gic_data_fiq_enable(gic))
+		writel_relaxed(0xffffffff, dist_base + GIC_DIST_IGROUP + 0);
+
 	writel_relaxed(0xf0, base + GIC_CPU_PRIMASK);
-	writel_relaxed(1, base + GIC_CPU_CTRL);
+	if (gic_data_fiq_enable(gic))
+		writel_relaxed(0x1f, base + GIC_CPU_CTRL);
+	else
+		writel_relaxed(1, base + GIC_CPU_CTRL);
 }
 
 void gic_cpu_if_down(void)
@@ -485,7 +564,10 @@  static void gic_dist_restore(unsigned int gic_nr)
 		writel_relaxed(gic_data[gic_nr].saved_spi_enable[i],
 			dist_base + GIC_DIST_ENABLE_SET + i * 4);
 
-	writel_relaxed(1, dist_base + GIC_DIST_CTRL);
+	if (gic_data_fiq_enable(&gic_data[gic_nr]))
+		writel_relaxed(3, dist_base + GIC_DIST_CTRL);
+	else
+		writel_relaxed(1, dist_base + GIC_DIST_CTRL);
 }
 
 static void gic_cpu_save(unsigned int gic_nr)
@@ -542,7 +624,7 @@  static void gic_cpu_restore(unsigned int gic_nr)
 		writel_relaxed(0xa0a0a0a0, dist_base + GIC_DIST_PRI + i * 4);
 
 	writel_relaxed(0xf0, cpu_base + GIC_CPU_PRIMASK);
-	writel_relaxed(1, cpu_base + GIC_CPU_CTRL);
+	writel_relaxed(0x1f, cpu_base + GIC_CPU_CTRL);
 }
 
 static int gic_notifier(struct notifier_block *self, unsigned long cmd,	void *v)
@@ -604,6 +686,7 @@  static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
 {
 	int cpu;
 	unsigned long flags, map = 0;
+	unsigned long softint;
 
 	raw_spin_lock_irqsave(&irq_controller_lock, flags);
 
@@ -618,7 +701,11 @@  static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
 	dmb(ishst);
 
 	/* this always happens on GIC0 */
-	writel_relaxed(map << 16 | irq, gic_data_dist_base(&gic_data[0]) + GIC_DIST_SOFTINT);
+	softint = map << 16 | irq;
+	if (gic_data_fiq_enable(&gic_data[0]))
+		softint |= 0x8000;
+	writel_relaxed(softint,
+		       gic_data_dist_base(&gic_data[0]) + GIC_DIST_SOFTINT);
 
 	raw_spin_unlock_irqrestore(&irq_controller_lock, flags);
 }
@@ -964,6 +1051,8 @@  void __init gic_init_bases(unsigned int gic_nr, int irq_start,
 
 		gic->domain = irq_domain_add_legacy(node, gic_irqs, irq_base,
 					hwirq_base, &gic_irq_domain_ops, gic);
+
+		gic_init_fiq(gic, irq_base, gic_irqs);
 	} else {
 		gic->domain = irq_domain_add_linear(node, nr_routable_irqs,
 						    &gic_irq_domain_ops,