From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754798AbbDUNpf (ORCPT ); Tue, 21 Apr 2015 09:45:35 -0400 Received: from foss.arm.com ([217.140.101.70]:47004 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751935AbbDUNpc (ORCPT ); Tue, 21 Apr 2015 09:45:32 -0400 Message-ID: <55365477.30404@arm.com> Date: Tue, 21 Apr 2015 14:45:27 +0100 From: Marc Zyngier Organization: ARM Ltd User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.4.0 MIME-Version: 1.0 To: Daniel Thompson , Thomas Gleixner , Jason Cooper CC: Russell King , Will Deacon , Catalin Marinas , Stephen Boyd , John Stultz , Steven Rostedt , "linux-kernel@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "patches@linaro.org" , "linaro-kernel@lists.linaro.org" , Sumit Semwal , Dirk Behme , Daniel Drake , Dmitry Pervushin , Tim Sander Subject: Re: [RESEND PATCH 4.0-rc7 v20 3/6] irqchip: gic: Introduce plumbing for IPI FIQ References: <1427216014-5324-1-git-send-email-daniel.thompson@linaro.org> <1428659511-9590-1-git-send-email-daniel.thompson@linaro.org> <1428659511-9590-4-git-send-email-daniel.thompson@linaro.org> In-Reply-To: <1428659511-9590-4-git-send-email-daniel.thompson@linaro.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > Cc: Thomas Gleixner > Cc: Jason Cooper > Cc: Russell King > Cc: Marc Zyngier > Tested-by: Jon Medhurst > --- > 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 > #include > #include > +#include > > #include > #include > @@ -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 > #include > #include > +#include > > #include > #include > @@ -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...