Re: [PATCH 4.4-rc5 v22 3/4] irqchip: gic: Introduce plumbing for IPI FIQ

From: Marc Zyngier
Date: Thu Jan 07 2016 - 12:07:13 EST

Next message: Greg Kroah-Hartman: "Re: [PATCH] tty: plug a use-after-free in TIOCGETD ioctl"
Previous message: Peter Maydell: "Re: [Qemu-devel] arm64 qemu tests failing in linux-next since 'arm64: kernel: enforce pmuserenr_el0 initialization and restore'"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 20/12/15 20:52, Daniel Thompson wrote:
> Currently it is not possible to exploit FIQ for systems with a GIC, even
> on 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. Finally 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 on early
> GICv1 implementations where it is available but tricky to enable) the
> code to change groups does not deploy and all IPIs will be raised via
> IRQ.
>
> 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>
> ---
> drivers/irqchip/irq-gic.c | 183 +++++++++++++++++++++++++++++++++++++---
> include/linux/irqchip/arm-gic.h | 6 ++
> 2 files changed, 175 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index b6c1e96b52a1..8077edd0d38d 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -41,6 +41,7 @@
> #include <linux/irqchip.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>
> @@ -63,6 +64,10 @@ static void gic_check_cpu_features(void)
> #define gic_check_cpu_features() do { } while(0)
> #endif
>
> +#ifndef SMP_IPI_FIQ_MASK
> +#define SMP_IPI_FIQ_MASK 0
> +#endif
> +
> union gic_base {
> void __iomem *common_base;
> void __percpu * __iomem *percpu_base;
> @@ -82,6 +87,7 @@ struct gic_chip_data {
> #endif
> struct irq_domain *domain;
> unsigned int gic_irqs;
> + bool sgi_with_nsatt;
> #ifdef CONFIG_GIC_NON_BANKED
> void __iomem *(*get_base)(union gic_base *);
> #endif
> @@ -350,12 +356,52 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
> }
> #endif
>
> +/*
> + * Fully acknowledge (ack, eoi and deactivate) any outstanding FIQ-based IPI,
> + * otherwise do nothing.
> + */
> +static void __maybe_unused gic_handle_fiq(struct pt_regs *regs)
> +{
> + struct gic_chip_data *gic = &gic_data[0];
> + void __iomem *cpu_base = gic_data_cpu_base(gic);
> + u32 hppstat, hppnr, irqstat, irqnr;
> +
> + do {
> + hppstat = readl_relaxed(cpu_base + GIC_CPU_HIGHPRI);
> + hppnr = hppstat & GICC_IAR_INT_ID_MASK;
> + if (!(hppnr < 16 && BIT(hppnr) & SMP_IPI_FIQ_MASK))
> + break;
> +
> + irqstat = readl_relaxed(cpu_base + GIC_CPU_INTACK);
> + irqnr = irqstat & GICC_IAR_INT_ID_MASK;
> +
> + writel_relaxed(irqstat, cpu_base + GIC_CPU_EOI);
> + if (static_key_true(&supports_deactivate))
> + writel_relaxed(irqstat, cpu_base + GIC_CPU_DEACTIVATE);
> +
> + if (WARN_RATELIMIT(irqnr > 16,

Shouldn't that be irqnr > 15?

> + "Unexpected irqnr %u (bad prioritization?)\n",
> + irqnr))
> + continue;
> +#ifdef CONFIG_SMP
> + handle_IPI(irqnr, regs);
> +#endif
> + } while (1);
> +}
> +
> static void __exception_irq_entry gic_handle_irq(struct pt_regs *regs)
> {
> u32 irqstat, irqnr;
> struct gic_chip_data *gic = &gic_data[0];
> void __iomem *cpu_base = gic_data_cpu_base(gic);
>
> +#ifdef CONFIG_ARM

What is the reason to make this 32bit specific?

> + if (in_nmi()) {
> + gic_handle_fiq(regs);
> + return;
> + }
> +#endif
> +
> do {
> irqstat = readl_relaxed(cpu_base + GIC_CPU_INTACK);
> irqnr = irqstat & GICC_IAR_INT_ID_MASK;
> @@ -439,6 +485,55 @@ static struct irq_chip gic_eoimode1_chip = {
> IRQCHIP_MASK_ON_SUSPEND,
> };
>
> +/*
> + * 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.
> + *
> + * It 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;
> +

nit: spurious space.

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

nit: I tend to prefer expressions to be written the other way around
(readl() & v). But more importantly, you should be able to cache the
grouping state in the gic_chip_data structure (you seem to have similar
code below).

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

The priority register is byte-accessible, so you can save yourself some
effort and just write the priority there.

> +
> + 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);
> + writel_relaxed(pri_val, base + GIC_DIST_PRI + pri_reg);
> +
> + raw_spin_unlock(&irq_controller_lock);
> +}
> +
> +
> void __init gic_cascade_irq(unsigned int gic_nr, unsigned int irq)
> {
> if (gic_nr >= MAX_GIC_NR)
> @@ -469,19 +564,27 @@ static u8 gic_get_cpumask(struct gic_chip_data *gic)
> static void gic_cpu_if_up(struct gic_chip_data *gic)
> {
> void __iomem *cpu_base = gic_data_cpu_base(gic);
> - u32 bypass = 0;
> - u32 mode = 0;
> + void __iomem *dist_base = gic_data_dist_base(gic);
> + u32 ctrl = 0;
>
> - if (static_key_true(&supports_deactivate))
> - mode = GIC_CPU_CTRL_EOImodeNS;
> + /*
> + * Preserve bypass disable bits to be written back later
> + */
> + ctrl = readl(cpu_base + GIC_CPU_CTRL);
> + ctrl &= GICC_DIS_BYPASS_MASK;
>
> /*
> - * Preserve bypass disable bits to be written back later
> - */
> - bypass = readl(cpu_base + GIC_CPU_CTRL);
> - bypass &= GICC_DIS_BYPASS_MASK;
> + * 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;
> +
> + if (static_key_true(&supports_deactivate))
> + ctrl |= GIC_CPU_CTRL_EOImodeNS;
>
> - writel_relaxed(bypass | mode | GICC_ENABLE, cpu_base + GIC_CPU_CTRL);
> + writel_relaxed(ctrl | GICC_ENABLE, cpu_base + GIC_CPU_CTRL);
> }
>
>
> @@ -505,7 +608,31 @@ 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 security mode.
> + */
> + writel_relaxed(GICD_ENABLE_GRP1 | GICD_ENABLE, base + GIC_DIST_CTRL);
> +
> + /*
> + * Some GICv1 devices (even those with security extensions) do not
> + * implement EnableGrp1 meaning some parts of the above write may
> + * be ignored. We will only enable FIQ support if the bit can be set.
> + */
> + if (GICD_ENABLE_GRP1 & readl_relaxed(base + GIC_DIST_CTRL)) {
> + /* Place all SPIs in group 1 (signally with IRQ). */
> + for (i = 32; i < gic_irqs; i += 32)
> + writel_relaxed(0xffffffff,
> + base + GIC_DIST_IGROUP + i * 4 / 32);
> +
> + /*
> + * If the GIC supports the security extension then SGIs
> + * will be filtered based on the value of NSATT. If the
> + * GIC has this support then enable NSATT support.
> + */
> + if (GICD_SECURITY_EXTN & readl_relaxed(base + GIC_DIST_CTR))
> + gic->sgi_with_nsatt = true;
> + }
> }
>
> static void gic_cpu_init(struct gic_chip_data *gic)
> @@ -514,6 +641,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 ipi_fiq_mask, fiq;

Think of the children (arm64), do not make ipi_fiq_mask a long... If you
pass anything but u32 to writel, you're doing something wrong.

>
> /*
> * Setting up the CPU map is only relevant for the primary GIC
> @@ -539,6 +667,23 @@ 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 all SGI and PPI interrupts that are not set in
> + * SMP_IPI_FIQ_MASK to group1 and ensure any remaining group 0
> + * interrupts have the right priority.
> + *
> + * Note that IGROUP[0] is banked, meaning that although we are
> + * writing to a distributor register we are actually performing
> + * part of the per-cpu initialization.
> + */
> + if (GICD_ENABLE_GRP1 & readl_relaxed(dist_base + GIC_DIST_CTRL)) {
> + ipi_fiq_mask = SMP_IPI_FIQ_MASK;
> + writel_relaxed(~ipi_fiq_mask, dist_base + GIC_DIST_IGROUP + 0);

or just turn this into writel_relaxed(~SMP_IPI_FIQ_MASK...) and keep
ipi_fiq_mask as a long.

> + for_each_set_bit(fiq, &ipi_fiq_mask, 16)
> + gic_set_group_irq(gic, fiq, 0);
> + }
> +
> writel_relaxed(GICC_INT_PRI_THRESHOLD, base + GIC_CPU_PRIMASK);
> gic_cpu_if_up(gic);
> }
> @@ -553,7 +698,8 @@ int gic_cpu_if_down(unsigned int gic_nr)
>
> cpu_base = gic_data_cpu_base(&gic_data[gic_nr]);
> val = readl(cpu_base + GIC_CPU_CTRL);
> - val &= ~GICC_ENABLE;
> + val &= ~(GICC_COMMON_BPR | GICC_FIQ_EN | GICC_ACK_CTL |
> + GICC_ENABLE_GRP1 | GICC_ENABLE);
> writel_relaxed(val, cpu_base + GIC_CPU_CTRL);
>
> return 0;
> @@ -648,7 +794,8 @@ static void gic_dist_restore(unsigned int gic_nr)
> dist_base + GIC_DIST_ACTIVE_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)
> @@ -794,6 +941,8 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
> {
> int cpu;
> unsigned long map = 0;
> + unsigned long softint;
> + void __iomem *dist_base;
>
> gic_migration_lock();
>
> @@ -801,14 +950,20 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
> for_each_cpu(cpu, mask)
> map |= gic_cpu_map[cpu];
>
> + /* This always happens on GIC0 */
> + dist_base = gic_data_dist_base(&gic_data[0]);
> +
> /*
> * Ensure that stores to Normal memory are visible to the
> * other CPUs before they observe us issuing the IPI.
> */
> 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;
> +
> + writel_relaxed(softint, dist_base + GIC_DIST_SOFTINT);
> + if (gic_data[0].sgi_with_nsatt)
> + writel_relaxed(softint | 0x8000, dist_base + GIC_DIST_SOFTINT);

Ah! No way. Writing twice to GICD_SGIR is horribly inefficient - just
think of a virtualized environment where you actually trap to HYP to
emulate SGIs (and some actual HW sucks almost as much...). A better
solution would be to keep track of which SGIs are secure and which are
not. A simple u16 would do.

>
> gic_migration_unlock();
> }
> diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h
> index bae69e5d693c..17b9e20d754e 100644
> --- a/include/linux/irqchip/arm-gic.h
> +++ b/include/linux/irqchip/arm-gic.h
> @@ -23,6 +23,10 @@
> #define GIC_CPU_DEACTIVATE 0x1000
>
> #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 GIC_CPU_CTRL_EOImodeNS (1 << 9)
> @@ -48,7 +52,9 @@
> #define GIC_DIST_SGI_PENDING_SET 0xf20
>
> #define GICD_ENABLE 0x1
> +#define GICD_ENABLE_GRP1 0x2
> #define GICD_DISABLE 0x0
> +#define GICD_SECURITY_EXTN 0x400
> #define GICD_INT_ACTLOW_LVLTRIG 0x0
> #define GICD_INT_EN_CLR_X32 0xffffffff
> #define GICD_INT_EN_SET_SGI 0x0000ffff
>

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/

Next message: Greg Kroah-Hartman: "Re: [PATCH] tty: plug a use-after-free in TIOCGETD ioctl"
Previous message: Peter Maydell: "Re: [Qemu-devel] arm64 qemu tests failing in linux-next since 'arm64: kernel: enforce pmuserenr_el0 initialization and restore'"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]