Re: [RESEND PATCH 4.0-rc7 v20 3/6] irqchip: gic: Introduce plumbing for IPI FIQ
From: Marc Zyngier
Date: Tue Apr 21 2015 - 09:45:39 EST
On 10/04/15 10:51, Daniel Thompson wrote:
> Currently it is not possible to exploit FIQ for systems with a GIC, even if
> the systems are otherwise capable of it. This patch makes it possible
> for IPIs to be delivered using FIQ.
>
> To do so it modifies the register state so that normal interrupts are
> placed in group 1 and specific IPIs are placed into group 0. It also
> configures the controller to raise group 0 interrupts using the FIQ
> signal. It provides a means for architecture code to define which IPIs
> shall use FIQ and to acknowledge any IPIs that are raised.
>
> All GIC hardware except GICv1-without-TrustZone support provides a means
> to group exceptions into group 0 and group 1 but 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". However when grouping is not available (or in the case
> of early GICv1 implementations is very hard to configure) the code to
> change groups does not deploy and all IPIs will be raised via IRQ.
>
> It has been tested and shown working on two systems capable of
> supporting grouping (Freescale i.MX6 and STiH416). It has also been
> tested for boot regressions on two systems that do not support grouping
> (vexpress-a9 and Qualcomm Snapdragon 600).
>
> Signed-off-by: Daniel Thompson <daniel.thompson@xxxxxxxxxx>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: Jason Cooper <jason@xxxxxxxxxxxxxx>
> Cc: Russell King <linux@xxxxxxxxxxxxxxxx>
> Cc: Marc Zyngier <marc.zyngier@xxxxxxx>
> Tested-by: Jon Medhurst <tixy@xxxxxxxxxx>
> ---
> arch/arm/kernel/traps.c | 5 +-
> drivers/irqchip/irq-gic.c | 151 +++++++++++++++++++++++++++++++++++++---
> include/linux/irqchip/arm-gic.h | 8 +++
> 3 files changed, 153 insertions(+), 11 deletions(-)
>
> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
> index 788e23fe64d8..b35e220ae1b1 100644
> --- a/arch/arm/kernel/traps.c
> +++ b/arch/arm/kernel/traps.c
> @@ -26,6 +26,7 @@
> #include <linux/init.h>
> #include <linux/sched.h>
> #include <linux/irq.h>
> +#include <linux/irqchip/arm-gic.h>
>
> #include <linux/atomic.h>
> #include <asm/cacheflush.h>
> @@ -479,7 +480,9 @@ asmlinkage void __exception_irq_entry handle_fiq_as_nmi(struct pt_regs *regs)
>
> nmi_enter();
>
> - /* nop. FIQ handlers for special arch/arm features can be added here. */
> +#ifdef CONFIG_ARM_GIC
> + gic_handle_fiq_ipi();
> +#endif
This hunk is what irritates me. It creates a hard dependency between
core ARM code and the GIC, and I don't really see how this works with
multiplatform, where the interrupt controller is not necessarily a GIC.
In that case, you will die a horrible death.
Why can't we just call handle_arch_irq(), and let the normal handler do
its thing? You can have a "if (in_nmi())" in there, and call your FIQ
function. It would at least save us the above problem.
>
> nmi_exit();
>
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index 578ffc5ec087..ffd1c0fe44b2 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -39,6 +39,7 @@
> #include <linux/slab.h>
> #include <linux/irqchip/chained_irq.h>
> #include <linux/irqchip/arm-gic.h>
> +#include <linux/ratelimit.h>
>
> #include <asm/cputype.h>
> #include <asm/irq.h>
> @@ -48,6 +49,10 @@
> #include "irq-gic-common.h"
> #include "irqchip.h"
>
> +#ifndef SMP_IPI_FIQ_MASK
> +#define SMP_IPI_FIQ_MASK 0
> +#endif
> +
> union gic_base {
> void __iomem *common_base;
> void __percpu * __iomem *percpu_base;
> @@ -65,6 +70,7 @@ struct gic_chip_data {
> #endif
> struct irq_domain *domain;
> unsigned int gic_irqs;
> + u32 igroup0_shadow;
> #ifdef CONFIG_GIC_NON_BANKED
> void __iomem *(*get_base)(union gic_base *);
> #endif
> @@ -355,6 +361,83 @@ static struct irq_chip gic_chip = {
> .irq_set_wake = gic_set_wake,
> };
>
> +/*
> + * Shift an interrupt between Group 0 and Group 1.
> + *
> + * In addition to changing the group we also modify the priority to
> + * match what "ARM strongly recommends" for a system where no Group 1
> + * interrupt must ever preempt a Group 0 interrupt.
> + *
> + * If is safe to call this function on systems which do not support
> + * grouping (it will have no effect).
> + */
> +static void gic_set_group_irq(struct gic_chip_data *gic, unsigned int hwirq,
> + int group)
> +{
> + void __iomem *base = gic_data_dist_base(gic);
> + unsigned int grp_reg = hwirq / 32 * 4;
> + u32 grp_mask = BIT(hwirq % 32);
> + u32 grp_val;
> +
> + unsigned int pri_reg = (hwirq / 4) * 4;
> + u32 pri_mask = BIT(7 + ((hwirq % 4) * 8));
> + u32 pri_val;
> +
> + /*
> + * Systems which do not support grouping will have not have
> + * the EnableGrp1 bit set.
> + */
> + if (!(GICD_ENABLE_GRP1 & readl_relaxed(base + GIC_DIST_CTRL)))
I took me a moment to parse this. Can we please have the constants on
the right hand side? It is very much a nitpick, but still...
> + return;
> +
> + raw_spin_lock(&irq_controller_lock);
> +
> + grp_val = readl_relaxed(base + GIC_DIST_IGROUP + grp_reg);
> + pri_val = readl_relaxed(base + GIC_DIST_PRI + pri_reg);
> +
> + if (group) {
> + grp_val |= grp_mask;
> + pri_val |= pri_mask;
> + } else {
> + grp_val &= ~grp_mask;
> + pri_val &= ~pri_mask;
> + }
> +
> + writel_relaxed(grp_val, base + GIC_DIST_IGROUP + grp_reg);
> + if (grp_reg == 0)
> + gic->igroup0_shadow = grp_val;
> +
> + writel_relaxed(pri_val, base + GIC_DIST_PRI + pri_reg);
> +
> + raw_spin_unlock(&irq_controller_lock);
> +}
> +
> +
> +/*
> + * Fully acknowledge (both ack and eoi) any outstanding FIQ-based IPI,
> + * otherwise do nothing.
> + */
> +void gic_handle_fiq_ipi(void)
> +{
> + struct gic_chip_data *gic = &gic_data[0];
> + void __iomem *cpu_base = gic_data_cpu_base(gic);
> + unsigned long irqstat, irqnr;
> +
> + if (WARN_ON(!in_nmi()))
> + return;
> +
> + while ((1u << readl_relaxed(cpu_base + GIC_CPU_HIGHPRI)) &
> + SMP_IPI_FIQ_MASK) {
> + irqstat = readl_relaxed(cpu_base + GIC_CPU_INTACK);
> + writel_relaxed(irqstat, cpu_base + GIC_CPU_EOI);
> +
> + irqnr = irqstat & GICC_IAR_INT_ID_MASK;
> + WARN_RATELIMIT(irqnr > 16,
> + "Unexpected irqnr %lu (bad prioritization?)\n",
> + irqnr);
> + }
> +}
> +
> void __init gic_cascade_irq(unsigned int gic_nr, unsigned int irq)
> {
> if (gic_nr >= MAX_GIC_NR)
> @@ -386,15 +469,24 @@ static u8 gic_get_cpumask(struct gic_chip_data *gic)
> static void gic_cpu_if_up(void)
> {
> void __iomem *cpu_base = gic_data_cpu_base(&gic_data[0]);
> - u32 bypass = 0;
> + void __iomem *dist_base = gic_data_dist_base(&gic_data[0]);
> + u32 ctrl = 0;
>
> /*
> - * Preserve bypass disable bits to be written back later
> - */
> - bypass = readl(cpu_base + GIC_CPU_CTRL);
> - bypass &= GICC_DIS_BYPASS_MASK;
> + * Preserve bypass disable bits to be written back later
> + */
> + ctrl = readl(cpu_base + GIC_CPU_CTRL);
> + ctrl &= GICC_DIS_BYPASS_MASK;
>
> - writel_relaxed(bypass | GICC_ENABLE, cpu_base + GIC_CPU_CTRL);
> + /*
> + * If EnableGrp1 is set in the distributor then enable group 1
> + * support for this CPU (and route group 0 interrupts to FIQ).
> + */
> + if (GICD_ENABLE_GRP1 & readl_relaxed(dist_base + GIC_DIST_CTRL))
> + ctrl |= GICC_COMMON_BPR | GICC_FIQ_EN | GICC_ACK_CTL |
> + GICC_ENABLE_GRP1;
> +
> + writel_relaxed(ctrl | GICC_ENABLE, cpu_base + GIC_CPU_CTRL);
> }
>
>
> @@ -418,7 +510,23 @@ static void __init gic_dist_init(struct gic_chip_data *gic)
>
> gic_dist_config(base, gic_irqs, NULL);
>
> - writel_relaxed(GICD_ENABLE, base + GIC_DIST_CTRL);
> + /*
> + * Set EnableGrp1/EnableGrp0 (bit 1 and 0) or EnableGrp (bit 0 only,
> + * bit 1 ignored) depending on current mode.
> + */
> + writel_relaxed(GICD_ENABLE_GRP1 | GICD_ENABLE, base + GIC_DIST_CTRL);
> +
> + /*
> + * Set all global interrupts to be group 1 if (and only if) it
> + * is possible to enable group 1 interrupts. This register is RAZ/WI
> + * if not accessible or not implemented, however some GICv1 devices
> + * do not implement the EnableGrp1 bit making it unsafe to set
> + * this register unconditionally.
> + */
> + if (GICD_ENABLE_GRP1 & readl_relaxed(base + GIC_DIST_CTRL))
> + for (i = 32; i < gic_irqs; i += 32)
> + writel_relaxed(0xffffffff,
> + base + GIC_DIST_IGROUP + i * 4 / 32);
> }
>
> static void gic_cpu_init(struct gic_chip_data *gic)
> @@ -427,6 +535,7 @@ static void gic_cpu_init(struct gic_chip_data *gic)
> void __iomem *base = gic_data_cpu_base(gic);
> unsigned int cpu_mask, cpu = smp_processor_id();
> int i;
> + unsigned long secure_irqs, secure_irq;
I'd rather see u32 being used for secure_irqs. Remember that this used
on arm64 as well.
>
> /*
> * Get what the GIC says our CPU mask is.
> @@ -445,6 +554,20 @@ static void gic_cpu_init(struct gic_chip_data *gic)
>
> gic_cpu_config(dist_base, NULL);
>
> + /*
> + * If the distributor is configured to support interrupt grouping
> + * then set any PPI and SGI interrupts not set in SMP_IPI_FIQ_MASK
> + * to be group1 and ensure any remaining group 0 interrupts have
> + * the right priority.
> + */
> + if (GICD_ENABLE_GRP1 & readl_relaxed(dist_base + GIC_DIST_CTRL)) {
> + secure_irqs = SMP_IPI_FIQ_MASK;
> + writel_relaxed(~secure_irqs, dist_base + GIC_DIST_IGROUP + 0);
> + gic->igroup0_shadow = ~secure_irqs;
> + for_each_set_bit(secure_irq, &secure_irqs, 16)
> + gic_set_group_irq(gic, secure_irq, 0);
> + }
> +
> writel_relaxed(GICC_INT_PRI_THRESHOLD, base + GIC_CPU_PRIMASK);
> gic_cpu_if_up();
> }
> @@ -534,7 +657,8 @@ 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(GICD_ENABLE, dist_base + GIC_DIST_CTRL);
> + writel_relaxed(GICD_ENABLE_GRP1 | GICD_ENABLE,
> + dist_base + GIC_DIST_CTRL);
> }
>
> static void gic_cpu_save(unsigned int gic_nr)
> @@ -662,6 +786,7 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
> {
> int cpu;
> unsigned long map = 0;
> + unsigned long softint;
u32, please.
>
> gic_migration_lock();
>
> @@ -675,8 +800,14 @@ 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);
> + /* We avoid a readl here by using the shadow copy of IGROUP[0] */
> + softint = map << 16 | irq;
> + if (gic_data[0].igroup0_shadow & BIT(irq))
> + softint |= 0x8000;
> +
> + /* This always happens on GIC0 */
> + writel_relaxed(softint,
> + gic_data_dist_base(&gic_data[0]) + GIC_DIST_SOFTINT);
>
> gic_migration_unlock();
> }
> diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h
> index 71d706d5f169..7690f70049a3 100644
> --- a/include/linux/irqchip/arm-gic.h
> +++ b/include/linux/irqchip/arm-gic.h
> @@ -22,6 +22,10 @@
> #define GIC_CPU_IDENT 0xfc
>
> #define GICC_ENABLE 0x1
> +#define GICC_ENABLE_GRP1 0x2
> +#define GICC_ACK_CTL 0x4
> +#define GICC_FIQ_EN 0x8
> +#define GICC_COMMON_BPR 0x10
> #define GICC_INT_PRI_THRESHOLD 0xf0
> #define GICC_IAR_INT_ID_MASK 0x3ff
> #define GICC_INT_SPURIOUS 1023
> @@ -44,6 +48,7 @@
> #define GIC_DIST_SGI_PENDING_SET 0xf20
>
> #define GICD_ENABLE 0x1
> +#define GICD_ENABLE_GRP1 0x2
> #define GICD_DISABLE 0x0
> #define GICD_INT_ACTLOW_LVLTRIG 0x0
> #define GICD_INT_EN_CLR_X32 0xffffffff
> @@ -121,5 +126,8 @@ static inline void __init register_routable_domain_ops
> {
> gic_routable_irq_domain_ops = ops;
> }
> +
> +void gic_handle_fiq_ipi(void);
> +
> #endif /* __ASSEMBLY */
> #endif
> --
> 2.1.0
>
Thanks,
M.
--
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/