| Message ID | 1548084825-8803-23-git-send-email-julien.thierry@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: provide pseudo NMI with GICv3 | expand |
On 2019/1/21 23:33, Julien Thierry wrote: > Implement NMI callbacks for GICv3 irqchip. Install NMI safe handlers > when setting up interrupt line as NMI. > > Only SPIs and PPIs are allowed to be set up as NMI. > > Signed-off-by: Julien Thierry <julien.thierry@arm.com> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Jason Cooper <jason@lakedaemon.net> > Cc: Marc Zyngier <marc.zyngier@arm.com> > --- > drivers/irqchip/irq-gic-v3.c | 84 ++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 84 insertions(+) > > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c > index 4df1e94..447d8ab 100644 > --- a/drivers/irqchip/irq-gic-v3.c > +++ b/drivers/irqchip/irq-gic-v3.c (snip) > > +static int gic_irq_nmi_setup(struct irq_data *d) > +{ > + struct irq_desc *desc = irq_to_desc(d->irq); > + > + if (!gic_supports_nmi()) > + return -EINVAL; > + > + if (gic_peek_irq(d, GICD_ISENABLER)) { > + pr_err("Cannot set NMI property of enabled IRQ %u\n", d->irq); > + return -EINVAL; > + } > + > + /* > + * A secondary irq_chip should be in charge of LPI request, > + * it should not be possible to get there > + */ > + if (WARN_ON(gic_irq(d) >= 8192)) > + return -EINVAL; > + > + /* desc lock should already be held */ > + if (gic_irq(d) < 32) { > + /* Setting up PPI as NMI, only switch handler for first NMI */ > + if (!refcount_inc_not_zero(&ppi_nmi_refs[gic_irq(d) - 16])) { > + refcount_set(&ppi_nmi_refs[gic_irq(d) - 16], 1); > + desc->handle_irq = handle_percpu_devid_fasteoi_nmi; > + } > + } else { > + desc->handle_irq = handle_fasteoi_nmi; > + } > + > + gic_set_irq_prio(gic_irq(d), gic_dist_base(d), GICD_INT_NMI_PRI); > + > + return 0; > +} > + > +static void gic_irq_nmi_teardown(struct irq_data *d) > +{ > + struct irq_desc *desc = irq_to_desc(d->irq); > + > + if (WARN_ON(!gic_supports_nmi())) > + return; > + > + if (gic_peek_irq(d, GICD_ISENABLER)) { > + pr_err("Cannot set NMI property of enabled IRQ %u\n", d->irq); > + return; > + } > + > + /* > + * A secondary irq_chip should be in charge of LPI request, > + * it should not be possible to get there > + */ > + if (WARN_ON(gic_irq(d) >= 8192)) > + return; > + > + /* desc lock should already be held */ > + if (gic_irq(d) < 32) { > + /* Tearing down NMI, only switch handler for last NMI */ > + if (refcount_dec_and_test(&ppi_nmi_refs[gic_irq(d) - 16])) > + desc->handle_irq = handle_percpu_devid_irq; > + } else { > + desc->handle_irq = handle_fasteoi_irq; > + } > + > + gic_set_irq_prio(gic_irq(d), gic_dist_base(d), GICD_INT_DEF_PRI); > +} > + Hello Julien, I am afraid the setting of priority is not correct here. If the irq is in redistributor(gic_irq(d) < 32), we should set the priority on each cpu, while we just set the priority on the current cpu here. static inline void __iomem *gic_dist_base(struct irq_data *d) { if (gic_irq_in_rdist(d)) /* SGI+PPI -> SGI_base for this CPU */ return gic_data_rdist_sgi_base(); if (d->hwirq <= 1023) /* SPI -> dist_base */ return gic_data.dist_base; return NULL; } I tried to add a smp_call_function here, but the kernel reported a warning as we have disabled irq when calling raw_spin_lock_irqsave in request_nmi or ready_percpu_nmi. [ 2.137262] Call trace: [ 2.137265] smp_call_function_many+0xf8/0x3a0 [ 2.137267] smp_call_function+0x40/0x58 [ 2.137271] gic_irq_nmi_setup+0xe8/0x118 [ 2.137275] ready_percpu_nmi+0x6c/0xf0 [ 2.137279] armpmu_request_irq+0x228/0x250 [ 2.137281] arm_pmu_acpi_init+0x150/0x2f0 [ 2.137284] do_one_initcall+0x54/0x218 [ 2.137289] kernel_init_freeable+0x230/0x354 [ 2.137293] kernel_init+0x18/0x118 [ 2.137295] ret_from_fork+0x10/0x18 I am exploring a better way to solve this issue. Thanks, Wei Li
On Sat, 26 Jan 2019 10:19:52 +0000, "liwei (GF)" <liwei391@huawei.com> wrote: > > > > On 2019/1/21 23:33, Julien Thierry wrote: > > Implement NMI callbacks for GICv3 irqchip. Install NMI safe handlers > > when setting up interrupt line as NMI. > > > > Only SPIs and PPIs are allowed to be set up as NMI. > > > > Signed-off-by: Julien Thierry <julien.thierry@arm.com> > > Cc: Thomas Gleixner <tglx@linutronix.de> > > Cc: Jason Cooper <jason@lakedaemon.net> > > Cc: Marc Zyngier <marc.zyngier@arm.com> > > --- > > drivers/irqchip/irq-gic-v3.c | 84 ++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 84 insertions(+) > > > > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c > > index 4df1e94..447d8ab 100644 > > --- a/drivers/irqchip/irq-gic-v3.c > > +++ b/drivers/irqchip/irq-gic-v3.c > (snip) > > > > +static int gic_irq_nmi_setup(struct irq_data *d) > > +{ > > + struct irq_desc *desc = irq_to_desc(d->irq); > > + > > + if (!gic_supports_nmi()) > > + return -EINVAL; > > + > > + if (gic_peek_irq(d, GICD_ISENABLER)) { > > + pr_err("Cannot set NMI property of enabled IRQ %u\n", d->irq); > > + return -EINVAL; > > + } > > + > > + /* > > + * A secondary irq_chip should be in charge of LPI request, > > + * it should not be possible to get there > > + */ > > + if (WARN_ON(gic_irq(d) >= 8192)) > > + return -EINVAL; > > + > > + /* desc lock should already be held */ > > + if (gic_irq(d) < 32) { > > + /* Setting up PPI as NMI, only switch handler for first NMI */ > > + if (!refcount_inc_not_zero(&ppi_nmi_refs[gic_irq(d) - 16])) { > > + refcount_set(&ppi_nmi_refs[gic_irq(d) - 16], 1); > > + desc->handle_irq = handle_percpu_devid_fasteoi_nmi; > > + } > > + } else { > > + desc->handle_irq = handle_fasteoi_nmi; > > + } > > + > > + gic_set_irq_prio(gic_irq(d), gic_dist_base(d), GICD_INT_NMI_PRI); > > + > > + return 0; > > +} > > + > > +static void gic_irq_nmi_teardown(struct irq_data *d) > > +{ > > + struct irq_desc *desc = irq_to_desc(d->irq); > > + > > + if (WARN_ON(!gic_supports_nmi())) > > + return; > > + > > + if (gic_peek_irq(d, GICD_ISENABLER)) { > > + pr_err("Cannot set NMI property of enabled IRQ %u\n", d->irq); > > + return; > > + } > > + > > + /* > > + * A secondary irq_chip should be in charge of LPI request, > > + * it should not be possible to get there > > + */ > > + if (WARN_ON(gic_irq(d) >= 8192)) > > + return; > > + > > + /* desc lock should already be held */ > > + if (gic_irq(d) < 32) { > > + /* Tearing down NMI, only switch handler for last NMI */ > > + if (refcount_dec_and_test(&ppi_nmi_refs[gic_irq(d) - 16])) > > + desc->handle_irq = handle_percpu_devid_irq; > > + } else { > > + desc->handle_irq = handle_fasteoi_irq; > > + } > > + > > + gic_set_irq_prio(gic_irq(d), gic_dist_base(d), GICD_INT_DEF_PRI); > > +} > > + > > Hello Julien, > I am afraid the setting of priority is not correct here. If the irq > is in redistributor(gic_irq(d) < 32), we should set the priority on > each cpu, while we just set the priority on the current cpu here. I think it really boils down to what semantic we want to present to the drivers. Given that all operations on PPIs are per-CPU (enable/disable), I do not see why the NMI setting should be any different. I'm certainly not keen on having diverging semantics here. > static inline void __iomem *gic_dist_base(struct irq_data *d) > { > if (gic_irq_in_rdist(d)) /* SGI+PPI -> SGI_base for this CPU */ > return gic_data_rdist_sgi_base(); > > if (d->hwirq <= 1023) /* SPI -> dist_base */ > return gic_data.dist_base; > > return NULL; > } > > I tried to add a smp_call_function here, but the kernel reported a warning as we have disabled irq > when calling raw_spin_lock_irqsave in request_nmi or ready_percpu_nmi. > [ 2.137262] Call trace: > [ 2.137265] smp_call_function_many+0xf8/0x3a0 > [ 2.137267] smp_call_function+0x40/0x58 > [ 2.137271] gic_irq_nmi_setup+0xe8/0x118 > [ 2.137275] ready_percpu_nmi+0x6c/0xf0 > [ 2.137279] armpmu_request_irq+0x228/0x250 > [ 2.137281] arm_pmu_acpi_init+0x150/0x2f0 > [ 2.137284] do_one_initcall+0x54/0x218 > [ 2.137289] kernel_init_freeable+0x230/0x354 > [ 2.137293] kernel_init+0x18/0x118 > [ 2.137295] ret_from_fork+0x10/0x18 > > I am exploring a better way to solve this issue. The real question is "why should you care?". As far as the system is concerned, a PPI only makes sense on a single CPU. So a single PPI on two CPUs represent to different interrupts. Yes, they have the same interrupt number, but they really are two independent interrupt lines. What issue do you see with this? Thanks, M.
Hi, On 26/01/2019 10:19, liwei (GF) wrote: > > > On 2019/1/21 23:33, Julien Thierry wrote: >> Implement NMI callbacks for GICv3 irqchip. Install NMI safe handlers >> when setting up interrupt line as NMI. >> >> Only SPIs and PPIs are allowed to be set up as NMI. >> >> Signed-off-by: Julien Thierry <julien.thierry@arm.com> >> Cc: Thomas Gleixner <tglx@linutronix.de> >> Cc: Jason Cooper <jason@lakedaemon.net> >> Cc: Marc Zyngier <marc.zyngier@arm.com> >> --- >> drivers/irqchip/irq-gic-v3.c | 84 ++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 84 insertions(+) >> >> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c >> index 4df1e94..447d8ab 100644 >> --- a/drivers/irqchip/irq-gic-v3.c >> +++ b/drivers/irqchip/irq-gic-v3.c > (snip) >> >> +static int gic_irq_nmi_setup(struct irq_data *d) >> +{ >> + struct irq_desc *desc = irq_to_desc(d->irq); >> + >> + if (!gic_supports_nmi()) >> + return -EINVAL; >> + >> + if (gic_peek_irq(d, GICD_ISENABLER)) { >> + pr_err("Cannot set NMI property of enabled IRQ %u\n", d->irq); >> + return -EINVAL; >> + } >> + >> + /* >> + * A secondary irq_chip should be in charge of LPI request, >> + * it should not be possible to get there >> + */ >> + if (WARN_ON(gic_irq(d) >= 8192)) >> + return -EINVAL; >> + >> + /* desc lock should already be held */ >> + if (gic_irq(d) < 32) { >> + /* Setting up PPI as NMI, only switch handler for first NMI */ >> + if (!refcount_inc_not_zero(&ppi_nmi_refs[gic_irq(d) - 16])) { >> + refcount_set(&ppi_nmi_refs[gic_irq(d) - 16], 1); >> + desc->handle_irq = handle_percpu_devid_fasteoi_nmi; >> + } >> + } else { >> + desc->handle_irq = handle_fasteoi_nmi; >> + } >> + >> + gic_set_irq_prio(gic_irq(d), gic_dist_base(d), GICD_INT_NMI_PRI); >> + >> + return 0; >> +} >> + >> +static void gic_irq_nmi_teardown(struct irq_data *d) >> +{ >> + struct irq_desc *desc = irq_to_desc(d->irq); >> + >> + if (WARN_ON(!gic_supports_nmi())) >> + return; >> + >> + if (gic_peek_irq(d, GICD_ISENABLER)) { >> + pr_err("Cannot set NMI property of enabled IRQ %u\n", d->irq); >> + return; >> + } >> + >> + /* >> + * A secondary irq_chip should be in charge of LPI request, >> + * it should not be possible to get there >> + */ >> + if (WARN_ON(gic_irq(d) >= 8192)) >> + return; >> + >> + /* desc lock should already be held */ >> + if (gic_irq(d) < 32) { >> + /* Tearing down NMI, only switch handler for last NMI */ >> + if (refcount_dec_and_test(&ppi_nmi_refs[gic_irq(d) - 16])) >> + desc->handle_irq = handle_percpu_devid_irq; >> + } else { >> + desc->handle_irq = handle_fasteoi_irq; >> + } >> + >> + gic_set_irq_prio(gic_irq(d), gic_dist_base(d), GICD_INT_DEF_PRI); >> +} >> + > > Hello Julien, > I am afraid the setting of priority is not correct here. If the irq is in redistributor(gic_irq(d) < 32), > we should set the priority on each cpu, while we just set the priority on the current cpu here. As Marc stated, to work with PPIs, the core IRQ API provides a set of *_percpu_irq functions (request, enable, disable...). The current idea is that the driver is in charge of calling ready_percpu_nmi() before enabling on the correct CPU, in a similar manner that the driver is in charge of calling enable_percpu_irq() and disable_percpu_irq() on the correct CPU. > static inline void __iomem *gic_dist_base(struct irq_data *d) > { > if (gic_irq_in_rdist(d)) /* SGI+PPI -> SGI_base for this CPU */ > return gic_data_rdist_sgi_base(); > > if (d->hwirq <= 1023) /* SPI -> dist_base */ > return gic_data.dist_base; > > return NULL; > } > > I tried to add a smp_call_function here, but the kernel reported a warning as we have disabled irq > when calling raw_spin_lock_irqsave in request_nmi or ready_percpu_nmi. > [ 2.137262] Call trace: > [ 2.137265] smp_call_function_many+0xf8/0x3a0 > [ 2.137267] smp_call_function+0x40/0x58 > [ 2.137271] gic_irq_nmi_setup+0xe8/0x118 > [ 2.137275] ready_percpu_nmi+0x6c/0xf0> [ 2.137279] armpmu_request_irq+0x228/0x250 Your issue lies here, if your PMU IRQ is a PPI, you shouldn't be calling ready_percpu_nmi() at the time you request but probably somewhere like arm_perf_starting_cpu(). And you wouldn't need the smp_call to set the priority. Hope this helps. > [ 2.137281] arm_pmu_acpi_init+0x150/0x2f0 > [ 2.137284] do_one_initcall+0x54/0x218 > [ 2.137289] kernel_init_freeable+0x230/0x354 > [ 2.137293] kernel_init+0x18/0x118 > [ 2.137295] ret_from_fork+0x10/0x18 > Thanks,
On Mon, 21 Jan 2019 15:33:41 +0000, Julien Thierry <julien.thierry@arm.com> wrote: > > Implement NMI callbacks for GICv3 irqchip. Install NMI safe handlers > when setting up interrupt line as NMI. > > Only SPIs and PPIs are allowed to be set up as NMI. > > Signed-off-by: Julien Thierry <julien.thierry@arm.com> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Jason Cooper <jason@lakedaemon.net> > Cc: Marc Zyngier <marc.zyngier@arm.com> Reviewed-by: Marc Zyngier <marc.zyngier@arm.com> M.
Hello Julien & Marc, Thanks for your reply, I misunderstood the usage of ready_percpu_nmi() and teardown_percpu_nmi() indeed. I think that adding a note about this in the comment of request_percpu_nmi() will be nice. Regards, Wei Li On 2019/1/28 16:57, Julien Thierry wrote: > Hi, > > On 26/01/2019 10:19, liwei (GF) wrote: >> >> >> On 2019/1/21 23:33, Julien Thierry wrote: >>> Implement NMI callbacks for GICv3 irqchip. Install NMI safe handlers >>> when setting up interrupt line as NMI. >>> >>> Only SPIs and PPIs are allowed to be set up as NMI. >>> >>> Signed-off-by: Julien Thierry <julien.thierry@arm.com> >>> Cc: Thomas Gleixner <tglx@linutronix.de> >>> Cc: Jason Cooper <jason@lakedaemon.net> >>> Cc: Marc Zyngier <marc.zyngier@arm.com> >>> --- >>> drivers/irqchip/irq-gic-v3.c | 84 ++++++++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 84 insertions(+) >>> >>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c >>> index 4df1e94..447d8ab 100644 >>> --- a/drivers/irqchip/irq-gic-v3.c >>> +++ b/drivers/irqchip/irq-gic-v3.c >> (snip) >>> >>> +static int gic_irq_nmi_setup(struct irq_data *d) >>> +{ >>> + struct irq_desc *desc = irq_to_desc(d->irq); >>> + >>> + if (!gic_supports_nmi()) >>> + return -EINVAL; >>> + >>> + if (gic_peek_irq(d, GICD_ISENABLER)) { >>> + pr_err("Cannot set NMI property of enabled IRQ %u\n", d->irq); >>> + return -EINVAL; >>> + } >>> + >>> + /* >>> + * A secondary irq_chip should be in charge of LPI request, >>> + * it should not be possible to get there >>> + */ >>> + if (WARN_ON(gic_irq(d) >= 8192)) >>> + return -EINVAL; >>> + >>> + /* desc lock should already be held */ >>> + if (gic_irq(d) < 32) { >>> + /* Setting up PPI as NMI, only switch handler for first NMI */ >>> + if (!refcount_inc_not_zero(&ppi_nmi_refs[gic_irq(d) - 16])) { >>> + refcount_set(&ppi_nmi_refs[gic_irq(d) - 16], 1); >>> + desc->handle_irq = handle_percpu_devid_fasteoi_nmi; >>> + } >>> + } else { >>> + desc->handle_irq = handle_fasteoi_nmi; >>> + } >>> + >>> + gic_set_irq_prio(gic_irq(d), gic_dist_base(d), GICD_INT_NMI_PRI); >>> + >>> + return 0; >>> +} >>> + >>> +static void gic_irq_nmi_teardown(struct irq_data *d) >>> +{ >>> + struct irq_desc *desc = irq_to_desc(d->irq); >>> + >>> + if (WARN_ON(!gic_supports_nmi())) >>> + return; >>> + >>> + if (gic_peek_irq(d, GICD_ISENABLER)) { >>> + pr_err("Cannot set NMI property of enabled IRQ %u\n", d->irq); >>> + return; >>> + } >>> + >>> + /* >>> + * A secondary irq_chip should be in charge of LPI request, >>> + * it should not be possible to get there >>> + */ >>> + if (WARN_ON(gic_irq(d) >= 8192)) >>> + return; >>> + >>> + /* desc lock should already be held */ >>> + if (gic_irq(d) < 32) { >>> + /* Tearing down NMI, only switch handler for last NMI */ >>> + if (refcount_dec_and_test(&ppi_nmi_refs[gic_irq(d) - 16])) >>> + desc->handle_irq = handle_percpu_devid_irq; >>> + } else { >>> + desc->handle_irq = handle_fasteoi_irq; >>> + } >>> + >>> + gic_set_irq_prio(gic_irq(d), gic_dist_base(d), GICD_INT_DEF_PRI); >>> +} >>> + >> >> Hello Julien, >> I am afraid the setting of priority is not correct here. If the irq is in redistributor(gic_irq(d) < 32), >> we should set the priority on each cpu, while we just set the priority on the current cpu here. > > As Marc stated, to work with PPIs, the core IRQ API provides a set of > *_percpu_irq functions (request, enable, disable...). > > The current idea is that the driver is in charge of calling > ready_percpu_nmi() before enabling on the correct CPU, in a similar > manner that the driver is in charge of calling enable_percpu_irq() and > disable_percpu_irq() on the correct CPU. > > >> static inline void __iomem *gic_dist_base(struct irq_data *d) >> { >> if (gic_irq_in_rdist(d)) /* SGI+PPI -> SGI_base for this CPU */ >> return gic_data_rdist_sgi_base(); >> >> if (d->hwirq <= 1023) /* SPI -> dist_base */ >> return gic_data.dist_base; >> >> return NULL; >> } >> >> I tried to add a smp_call_function here, but the kernel reported a warning as we have disabled irq >> when calling raw_spin_lock_irqsave in request_nmi or ready_percpu_nmi. >> [ 2.137262] Call trace: >> [ 2.137265] smp_call_function_many+0xf8/0x3a0 >> [ 2.137267] smp_call_function+0x40/0x58 >> [ 2.137271] gic_irq_nmi_setup+0xe8/0x118 >> [ 2.137275] ready_percpu_nmi+0x6c/0xf0> [ 2.137279] armpmu_request_irq+0x228/0x250 > > Your issue lies here, if your PMU IRQ is a PPI, you shouldn't be calling > ready_percpu_nmi() at the time you request but probably somewhere like > arm_perf_starting_cpu(). > > And you wouldn't need the smp_call to set the priority. > > Hope this helps. > >> [ 2.137281] arm_pmu_acpi_init+0x150/0x2f0 >> [ 2.137284] do_one_initcall+0x54/0x218 >> [ 2.137289] kernel_init_freeable+0x230/0x354 >> [ 2.137293] kernel_init+0x18/0x118 >> [ 2.137295] ret_from_fork+0x10/0x18 >> > > Thanks, >
On 28/01/2019 13:59, liwei (GF) wrote: > Hello Julien & Marc, > Thanks for your reply, I misunderstood the usage of ready_percpu_nmi() and > teardown_percpu_nmi() indeed. > I think that adding a note about this in the comment of request_percpu_nmi() will be nice. Yes, that's a good suggestion, I'll add that. Also, if this helps, I have been working on some patches that are not fully ready for submission to make the Arm PMU interrupt into an NMI (I was holding onto them until this series would be merged). I pushed them on this branch: http://linux-arm.org/linux-jt.git -b nmi_for_pmu Thanks, Julien > On 2019/1/28 16:57, Julien Thierry wrote: >> Hi, >> >> On 26/01/2019 10:19, liwei (GF) wrote: >>> >>> >>> On 2019/1/21 23:33, Julien Thierry wrote: >>>> Implement NMI callbacks for GICv3 irqchip. Install NMI safe handlers >>>> when setting up interrupt line as NMI. >>>> >>>> Only SPIs and PPIs are allowed to be set up as NMI. >>>> >>>> Signed-off-by: Julien Thierry <julien.thierry@arm.com> >>>> Cc: Thomas Gleixner <tglx@linutronix.de> >>>> Cc: Jason Cooper <jason@lakedaemon.net> >>>> Cc: Marc Zyngier <marc.zyngier@arm.com> >>>> --- >>>> drivers/irqchip/irq-gic-v3.c | 84 ++++++++++++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 84 insertions(+) >>>> >>>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c >>>> index 4df1e94..447d8ab 100644 >>>> --- a/drivers/irqchip/irq-gic-v3.c >>>> +++ b/drivers/irqchip/irq-gic-v3.c >>> (snip) >>>> >>>> +static int gic_irq_nmi_setup(struct irq_data *d) >>>> +{ >>>> + struct irq_desc *desc = irq_to_desc(d->irq); >>>> + >>>> + if (!gic_supports_nmi()) >>>> + return -EINVAL; >>>> + >>>> + if (gic_peek_irq(d, GICD_ISENABLER)) { >>>> + pr_err("Cannot set NMI property of enabled IRQ %u\n", d->irq); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + /* >>>> + * A secondary irq_chip should be in charge of LPI request, >>>> + * it should not be possible to get there >>>> + */ >>>> + if (WARN_ON(gic_irq(d) >= 8192)) >>>> + return -EINVAL; >>>> + >>>> + /* desc lock should already be held */ >>>> + if (gic_irq(d) < 32) { >>>> + /* Setting up PPI as NMI, only switch handler for first NMI */ >>>> + if (!refcount_inc_not_zero(&ppi_nmi_refs[gic_irq(d) - 16])) { >>>> + refcount_set(&ppi_nmi_refs[gic_irq(d) - 16], 1); >>>> + desc->handle_irq = handle_percpu_devid_fasteoi_nmi; >>>> + } >>>> + } else { >>>> + desc->handle_irq = handle_fasteoi_nmi; >>>> + } >>>> + >>>> + gic_set_irq_prio(gic_irq(d), gic_dist_base(d), GICD_INT_NMI_PRI); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static void gic_irq_nmi_teardown(struct irq_data *d) >>>> +{ >>>> + struct irq_desc *desc = irq_to_desc(d->irq); >>>> + >>>> + if (WARN_ON(!gic_supports_nmi())) >>>> + return; >>>> + >>>> + if (gic_peek_irq(d, GICD_ISENABLER)) { >>>> + pr_err("Cannot set NMI property of enabled IRQ %u\n", d->irq); >>>> + return; >>>> + } >>>> + >>>> + /* >>>> + * A secondary irq_chip should be in charge of LPI request, >>>> + * it should not be possible to get there >>>> + */ >>>> + if (WARN_ON(gic_irq(d) >= 8192)) >>>> + return; >>>> + >>>> + /* desc lock should already be held */ >>>> + if (gic_irq(d) < 32) { >>>> + /* Tearing down NMI, only switch handler for last NMI */ >>>> + if (refcount_dec_and_test(&ppi_nmi_refs[gic_irq(d) - 16])) >>>> + desc->handle_irq = handle_percpu_devid_irq; >>>> + } else { >>>> + desc->handle_irq = handle_fasteoi_irq; >>>> + } >>>> + >>>> + gic_set_irq_prio(gic_irq(d), gic_dist_base(d), GICD_INT_DEF_PRI); >>>> +} >>>> + >>> >>> Hello Julien, >>> I am afraid the setting of priority is not correct here. If the irq is in redistributor(gic_irq(d) < 32), >>> we should set the priority on each cpu, while we just set the priority on the current cpu here. >> >> As Marc stated, to work with PPIs, the core IRQ API provides a set of >> *_percpu_irq functions (request, enable, disable...). >> >> The current idea is that the driver is in charge of calling >> ready_percpu_nmi() before enabling on the correct CPU, in a similar >> manner that the driver is in charge of calling enable_percpu_irq() and >> disable_percpu_irq() on the correct CPU. >> >> >>> static inline void __iomem *gic_dist_base(struct irq_data *d) >>> { >>> if (gic_irq_in_rdist(d)) /* SGI+PPI -> SGI_base for this CPU */ >>> return gic_data_rdist_sgi_base(); >>> >>> if (d->hwirq <= 1023) /* SPI -> dist_base */ >>> return gic_data.dist_base; >>> >>> return NULL; >>> } >>> >>> I tried to add a smp_call_function here, but the kernel reported a warning as we have disabled irq >>> when calling raw_spin_lock_irqsave in request_nmi or ready_percpu_nmi. >>> [ 2.137262] Call trace: >>> [ 2.137265] smp_call_function_many+0xf8/0x3a0 >>> [ 2.137267] smp_call_function+0x40/0x58 >>> [ 2.137271] gic_irq_nmi_setup+0xe8/0x118 >>> [ 2.137275] ready_percpu_nmi+0x6c/0xf0> [ 2.137279] armpmu_request_irq+0x228/0x250 >> >> Your issue lies here, if your PMU IRQ is a PPI, you shouldn't be calling >> ready_percpu_nmi() at the time you request but probably somewhere like >> arm_perf_starting_cpu(). >> >> And you wouldn't need the smp_call to set the priority. >> >> Hope this helps. >> >>> [ 2.137281] arm_pmu_acpi_init+0x150/0x2f0 >>> [ 2.137284] do_one_initcall+0x54/0x218 >>> [ 2.137289] kernel_init_freeable+0x230/0x354 >>> [ 2.137293] kernel_init+0x18/0x118 >>> [ 2.137295] ret_from_fork+0x10/0x18 >>> >> >> Thanks, >> >
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c index 4df1e94..447d8ab 100644 --- a/drivers/irqchip/irq-gic-v3.c +++ b/drivers/irqchip/irq-gic-v3.c @@ -27,6 +27,7 @@ #include <linux/of_address.h> #include <linux/of_irq.h> #include <linux/percpu.h> +#include <linux/refcount.h> #include <linux/slab.h> #include <linux/irqchip.h> @@ -93,6 +94,9 @@ struct gic_chip_data { */ static DEFINE_STATIC_KEY_FALSE(supports_pseudo_nmis); +/* ppi_nmi_refs[n] == number of cpus having ppi[n + 16] set as NMI */ +static refcount_t ppi_nmi_refs[16]; + static struct gic_kvm_info gic_v3_kvm_info; static DEFINE_PER_CPU(bool, has_rss); @@ -320,6 +324,72 @@ static int gic_irq_get_irqchip_state(struct irq_data *d, return 0; } +static int gic_irq_nmi_setup(struct irq_data *d) +{ + struct irq_desc *desc = irq_to_desc(d->irq); + + if (!gic_supports_nmi()) + return -EINVAL; + + if (gic_peek_irq(d, GICD_ISENABLER)) { + pr_err("Cannot set NMI property of enabled IRQ %u\n", d->irq); + return -EINVAL; + } + + /* + * A secondary irq_chip should be in charge of LPI request, + * it should not be possible to get there + */ + if (WARN_ON(gic_irq(d) >= 8192)) + return -EINVAL; + + /* desc lock should already be held */ + if (gic_irq(d) < 32) { + /* Setting up PPI as NMI, only switch handler for first NMI */ + if (!refcount_inc_not_zero(&ppi_nmi_refs[gic_irq(d) - 16])) { + refcount_set(&ppi_nmi_refs[gic_irq(d) - 16], 1); + desc->handle_irq = handle_percpu_devid_fasteoi_nmi; + } + } else { + desc->handle_irq = handle_fasteoi_nmi; + } + + gic_set_irq_prio(gic_irq(d), gic_dist_base(d), GICD_INT_NMI_PRI); + + return 0; +} + +static void gic_irq_nmi_teardown(struct irq_data *d) +{ + struct irq_desc *desc = irq_to_desc(d->irq); + + if (WARN_ON(!gic_supports_nmi())) + return; + + if (gic_peek_irq(d, GICD_ISENABLER)) { + pr_err("Cannot set NMI property of enabled IRQ %u\n", d->irq); + return; + } + + /* + * A secondary irq_chip should be in charge of LPI request, + * it should not be possible to get there + */ + if (WARN_ON(gic_irq(d) >= 8192)) + return; + + /* desc lock should already be held */ + if (gic_irq(d) < 32) { + /* Tearing down NMI, only switch handler for last NMI */ + if (refcount_dec_and_test(&ppi_nmi_refs[gic_irq(d) - 16])) + desc->handle_irq = handle_percpu_devid_irq; + } else { + desc->handle_irq = handle_fasteoi_irq; + } + + gic_set_irq_prio(gic_irq(d), gic_dist_base(d), GICD_INT_DEF_PRI); +} + static void gic_eoi_irq(struct irq_data *d) { gic_write_eoir(gic_irq(d)); @@ -958,6 +1028,8 @@ static inline void gic_cpu_pm_init(void) { } .irq_set_affinity = gic_set_affinity, .irq_get_irqchip_state = gic_irq_get_irqchip_state, .irq_set_irqchip_state = gic_irq_set_irqchip_state, + .irq_nmi_setup = gic_irq_nmi_setup, + .irq_nmi_teardown = gic_irq_nmi_teardown, .flags = IRQCHIP_SET_TYPE_MASKED | IRQCHIP_SKIP_SET_WAKE | IRQCHIP_MASK_ON_SUSPEND, @@ -973,6 +1045,8 @@ static inline void gic_cpu_pm_init(void) { } .irq_get_irqchip_state = gic_irq_get_irqchip_state, .irq_set_irqchip_state = gic_irq_set_irqchip_state, .irq_set_vcpu_affinity = gic_irq_set_vcpu_affinity, + .irq_nmi_setup = gic_irq_nmi_setup, + .irq_nmi_teardown = gic_irq_nmi_teardown, .flags = IRQCHIP_SET_TYPE_MASKED | IRQCHIP_SKIP_SET_WAKE | IRQCHIP_MASK_ON_SUSPEND, @@ -1176,7 +1250,17 @@ static bool gic_enable_quirk_msm8996(void *data) static void gic_enable_nmi_support(void) { + int i; + + for (i = 0; i < 16; i++) + refcount_set(&ppi_nmi_refs[i], 0); + static_branch_enable(&supports_pseudo_nmis); + + if (static_branch_likely(&supports_deactivate_key)) + gic_eoimode1_chip.flags |= IRQCHIP_SUPPORTS_NMI; + else + gic_chip.flags |= IRQCHIP_SUPPORTS_NMI; } static int __init gic_init_bases(void __iomem *dist_base,
Implement NMI callbacks for GICv3 irqchip. Install NMI safe handlers when setting up interrupt line as NMI. Only SPIs and PPIs are allowed to be set up as NMI. Signed-off-by: Julien Thierry <julien.thierry@arm.com> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Jason Cooper <jason@lakedaemon.net> Cc: Marc Zyngier <marc.zyngier@arm.com> --- drivers/irqchip/irq-gic-v3.c | 84 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+)