* [PATCH 0/4] Provide core API for NMIs @ 2018-07-24 11:07 Julien Thierry 2018-07-24 11:07 ` [PATCH 1/4] genirq: Provide basic NMI management for interrupt lines Julien Thierry ` (3 more replies) 0 siblings, 4 replies; 17+ messages in thread From: Julien Thierry @ 2018-07-24 11:07 UTC (permalink / raw) To: linux-kernel Cc: tglx, peterz, marc.zyngier, ricardo.neri-calderon, mingo, Julien Thierry Hi, This patch series provides a way for irqchips to define some IRQs as NMIs. For this to be possible, the irqchip must: - be a root irqchip - not require bus locking - have the NMI support flag Once these conditions are met, interrupt lines can be requested as NMIs. These lines must not be shared and the IRQ handling must not be threaded. Callbacks are added to the irqchip in order to set up (clean up) the necessary resources for NMIs before enabling (before releasing) the corresponding interrupt line. For reference, see the examples of the support provided by GICv3 [1] and for the request and management of an NMI in the arm_pmu driver [2]. (These are not on the list yet as I'd like the focus of the discussion to be on the API for now) This work was motivated by a discussion on Ricardo Neri's patch [3] Patches 1 & 2 provide functions necessary to request and manage NMIs. Patches 3 & 4 provide functions for NMI handling. Changes since RFC[4]: - Rebased to v4.18-rc6 - Remove disable_nmi, one should call disable_nmi_nosync instead - Remove code related to affinity balancing from NMI functions - Require PERCPU configuration for NMIs [1] http://www.linux-arm.org/git?p=linux-jt.git;a=commitdiff;h=be1beaf15fe840c8a430b9a8191a5b33877f3c0f [2] http://www.linux-arm.org/git?p=linux-jt.git;a=commitdiff;h=cc35056203445a38b1343fc23d6f3a7b2d707603 [3] https://patchwork.kernel.org/patch/10461381/ [4] https://lkml.org/lkml/2018/7/9/768 Thanks, Julien --> Julien Thierry (4): genirq: Provide basic NMI management for interrupt lines genirq: Provide NMI management for percpu_devid interrupts genirq: Provide NMI handlers irqdesc: Add domain handler for NMIs include/linux/interrupt.h | 18 +++ include/linux/irq.h | 10 ++ include/linux/irqdesc.h | 3 + kernel/irq/chip.c | 54 +++++++ kernel/irq/debugfs.c | 6 +- kernel/irq/internals.h | 1 + kernel/irq/irqdesc.c | 33 ++++ kernel/irq/manage.c | 377 +++++++++++++++++++++++++++++++++++++++++++++- 8 files changed, 499 insertions(+), 3 deletions(-) -- 1.9.1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/4] genirq: Provide basic NMI management for interrupt lines 2018-07-24 11:07 [PATCH 0/4] Provide core API for NMIs Julien Thierry @ 2018-07-24 11:07 ` Julien Thierry 2018-08-01 3:07 ` Ricardo Neri 2018-07-24 11:07 ` [PATCH 2/4] genirq: Provide NMI management for percpu_devid interrupts Julien Thierry ` (2 subsequent siblings) 3 siblings, 1 reply; 17+ messages in thread From: Julien Thierry @ 2018-07-24 11:07 UTC (permalink / raw) To: linux-kernel Cc: tglx, peterz, marc.zyngier, ricardo.neri-calderon, mingo, Julien Thierry Add functionality to allocate interrupt lines that will deliver IRQs as Non-Maskable Interrupts. These allocations are only successful if the irqchip provides the necessary support and allows NMI delivery for the interrupt line. Interrupt lines allocated for NMI delivery must be enabled/disabled through enable_nmi/disable_nmi_nosync to keep their state consistent. To treat a PERCPU IRQ as NMI, the interrupt must not be shared nor threaded, the irqchip directly managing the IRQ must be the root irqchip and the irqchip cannot be behind a slow bus. Signed-off-by: Julien Thierry <julien.thierry@arm.com> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Ingo Molnar <mingo@kernel.org> Cc: Marc Zyngier <marc.zyngier@arm.com> --- include/linux/interrupt.h | 9 ++ include/linux/irq.h | 7 ++ kernel/irq/debugfs.c | 6 +- kernel/irq/internals.h | 1 + kernel/irq/manage.c | 228 +++++++++++++++++++++++++++++++++++++++++++++- 5 files changed, 248 insertions(+), 3 deletions(-) diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h index eeceac3..3b1a320 100644 --- a/include/linux/interrupt.h +++ b/include/linux/interrupt.h @@ -156,6 +156,10 @@ struct irqaction { unsigned long flags, const char *devname, void __percpu *percpu_dev_id); +extern int __must_check +request_nmi(unsigned int irq, irq_handler_t handler, unsigned long flags, + const char *name, void *dev); + static inline int __must_check request_percpu_irq(unsigned int irq, irq_handler_t handler, const char *devname, void __percpu *percpu_dev_id) @@ -167,6 +171,8 @@ struct irqaction { extern const void *free_irq(unsigned int, void *); extern void free_percpu_irq(unsigned int, void __percpu *); +extern const void *free_nmi(unsigned int irq, void *dev_id); + struct device; extern int __must_check @@ -217,6 +223,9 @@ struct irqaction { extern bool irq_percpu_is_enabled(unsigned int irq); extern void irq_wake_thread(unsigned int irq, void *dev_id); +extern void disable_nmi_nosync(unsigned int irq); +extern void enable_nmi(unsigned int irq); + /* The following three functions are for the core kernel use only. */ extern void suspend_device_irqs(void); extern void resume_device_irqs(void); diff --git a/include/linux/irq.h b/include/linux/irq.h index 201de12..0d71f63 100644 --- a/include/linux/irq.h +++ b/include/linux/irq.h @@ -441,6 +441,8 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d) * @irq_set_vcpu_affinity: optional to target a vCPU in a virtual machine * @ipi_send_single: send a single IPI to destination cpus * @ipi_send_mask: send an IPI to destination cpus in cpumask + * @irq_nmi_setup: function called from core code before enabling an NMI + * @irq_nmi_teardown: function called from core code after disabling an NMI * @flags: chip specific flags */ struct irq_chip { @@ -489,6 +491,9 @@ struct irq_chip { void (*ipi_send_single)(struct irq_data *data, unsigned int cpu); void (*ipi_send_mask)(struct irq_data *data, const struct cpumask *dest); + int (*irq_nmi_setup)(struct irq_data *data); + void (*irq_nmi_teardown)(struct irq_data *data); + unsigned long flags; }; @@ -504,6 +509,7 @@ struct irq_chip { * IRQCHIP_ONESHOT_SAFE: One shot does not require mask/unmask * IRQCHIP_EOI_THREADED: Chip requires eoi() on unmask in threaded mode * IRQCHIP_SUPPORTS_LEVEL_MSI Chip can provide two doorbells for Level MSIs + * IRQCHIP_SUPPORTS_NMI: Chip can deliver NMIs, only for root irqchips */ enum { IRQCHIP_SET_TYPE_MASKED = (1 << 0), @@ -514,6 +520,7 @@ enum { IRQCHIP_ONESHOT_SAFE = (1 << 5), IRQCHIP_EOI_THREADED = (1 << 6), IRQCHIP_SUPPORTS_LEVEL_MSI = (1 << 7), + IRQCHIP_SUPPORTS_NMI = (1 << 8), }; #include <linux/irqdesc.h> diff --git a/kernel/irq/debugfs.c b/kernel/irq/debugfs.c index 6f63613..59a04d2 100644 --- a/kernel/irq/debugfs.c +++ b/kernel/irq/debugfs.c @@ -56,6 +56,7 @@ static void irq_debug_show_masks(struct seq_file *m, struct irq_desc *desc) { } BIT_MASK_DESCR(IRQCHIP_ONESHOT_SAFE), BIT_MASK_DESCR(IRQCHIP_EOI_THREADED), BIT_MASK_DESCR(IRQCHIP_SUPPORTS_LEVEL_MSI), + BIT_MASK_DESCR(IRQCHIP_SUPPORTS_NMI), }; static void @@ -140,6 +141,7 @@ static void irq_debug_show_masks(struct seq_file *m, struct irq_desc *desc) { } BIT_MASK_DESCR(IRQS_WAITING), BIT_MASK_DESCR(IRQS_PENDING), BIT_MASK_DESCR(IRQS_SUSPENDED), + BIT_MASK_DESCR(IRQS_NMI), }; @@ -203,8 +205,8 @@ static ssize_t irq_debug_write(struct file *file, const char __user *user_buf, chip_bus_lock(desc); raw_spin_lock_irqsave(&desc->lock, flags); - if (irq_settings_is_level(desc)) { - /* Can't do level, sorry */ + if (irq_settings_is_level(desc) || desc->istate & IRQS_NMI) { + /* Can't do level nor NMIs, sorry */ err = -EINVAL; } else { desc->istate |= IRQS_PENDING; diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h index ca6afa2..1b2c517 100644 --- a/kernel/irq/internals.h +++ b/kernel/irq/internals.h @@ -60,6 +60,7 @@ enum { IRQS_PENDING = 0x00000200, IRQS_SUSPENDED = 0x00000800, IRQS_TIMINGS = 0x00001000, + IRQS_NMI = 0x00002000, }; #include "debug.h" diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c index daeabd7..3b87143 100644 --- a/kernel/irq/manage.c +++ b/kernel/irq/manage.c @@ -341,7 +341,7 @@ static void irq_affinity_notify(struct work_struct *work) /* The release function is promised process context */ might_sleep(); - if (!desc) + if (!desc || desc->istate & IRQS_NMI) return -EINVAL; /* Complete initialisation of *notify */ @@ -550,6 +550,21 @@ bool disable_hardirq(unsigned int irq) } EXPORT_SYMBOL_GPL(disable_hardirq); +/** + * disable_nmi_nosync - disable an nmi without waiting + * @irq: Interrupt to disable + * + * Disable the selected interrupt line. Disables and enables are + * nested. + * The interrupt to disable must have been requested through request_nmi. + * Unlike disable_nmi(), this function does not ensure existing + * instances of the IRQ handler have completed before returning. + */ +void disable_nmi_nosync(unsigned int irq) +{ + disable_irq_nosync(irq); +} + void __enable_irq(struct irq_desc *desc) { switch (desc->depth) { @@ -606,6 +621,20 @@ void enable_irq(unsigned int irq) } EXPORT_SYMBOL(enable_irq); +/** + * enable_nmi - enable handling of an nmi + * @irq: Interrupt to enable + * + * The interrupt to enable must have been requested through request_nmi. + * Undoes the effect of one call to disable_nmi(). If this + * matches the last disable, processing of interrupts on this + * IRQ line is re-enabled. + */ +void enable_nmi(unsigned int irq) +{ + enable_irq(irq); +} + static int set_irq_wake_real(unsigned int irq, unsigned int on) { struct irq_desc *desc = irq_to_desc(irq); @@ -641,6 +670,12 @@ int irq_set_irq_wake(unsigned int irq, unsigned int on) if (!desc) return -EINVAL; + /* Don't use NMIs as wake up interrupts please */ + if (desc->istate & IRQS_NMI) { + ret = -EINVAL; + goto out_unlock; + } + /* wakeup-capable irqs can be shared between drivers that * don't need to have the same sleep mode behaviors. */ @@ -663,6 +698,8 @@ int irq_set_irq_wake(unsigned int irq, unsigned int on) irqd_clear(&desc->irq_data, IRQD_WAKEUP_STATE); } } + +out_unlock: irq_put_desc_busunlock(desc, flags); return ret; } @@ -1110,6 +1147,39 @@ static void irq_release_resources(struct irq_desc *desc) c->irq_release_resources(d); } +static bool irq_supports_nmi(struct irq_desc *desc) +{ + struct irq_data *d = irq_desc_get_irq_data(desc); + +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY + /* Only IRQs directly managed by the root irqchip can be set as NMI */ + if (d->parent_data) + return false; +#endif + /* Don't support NMIs for chips behind a slow bus */ + if (d->chip->irq_bus_lock || d->chip->irq_bus_sync_unlock) + return false; + + return d->chip->flags & IRQCHIP_SUPPORTS_NMI; +} + +static int irq_nmi_setup(struct irq_desc *desc) +{ + struct irq_data *d = irq_desc_get_irq_data(desc); + struct irq_chip *c = d->chip; + + return c->irq_nmi_setup ? c->irq_nmi_setup(d) : -EINVAL; +} + +static void irq_nmi_teardown(struct irq_desc *desc) +{ + struct irq_data *d = irq_desc_get_irq_data(desc); + struct irq_chip *c = d->chip; + + if (c->irq_nmi_teardown) + c->irq_nmi_teardown(d); +} + static int setup_irq_thread(struct irqaction *new, unsigned int irq, bool secondary) { @@ -1282,9 +1352,17 @@ static void irq_release_resources(struct irq_desc *desc) * fields must have IRQF_SHARED set and the bits which * set the trigger type must match. Also all must * agree on ONESHOT. + * Interrupt lines used for NMIs cannot be shared. */ unsigned int oldtype; + if (desc->istate & IRQS_NMI) { + pr_err("Invalid attempt to share NMI for %s (irq %d) on irqchip %s.\n", + new->name, irq, desc->irq_data.chip->name); + ret = -EINVAL; + goto out_unlock; + } + /* * If nobody did set the configuration before, inherit * the one provided by the requester. @@ -1733,6 +1811,59 @@ const void *free_irq(unsigned int irq, void *dev_id) } EXPORT_SYMBOL(free_irq); +/* This function must be called with desc->lock held */ +static const void *__cleanup_nmi(unsigned int irq, struct irq_desc *desc) +{ + const char *devname = NULL; + + desc->istate &= ~IRQS_NMI; + + if (!WARN_ON(desc->action == NULL)) { + irq_pm_remove_action(desc, desc->action); + devname = desc->action->name; + unregister_handler_proc(irq, desc->action); + + kfree(desc->action); + desc->action = NULL; + } + + irq_settings_clr_disable_unlazy(desc); + irq_shutdown(desc); + + irq_release_resources(desc); + + irq_chip_pm_put(&desc->irq_data); + module_put(desc->owner); + + return devname; +} + +const void *free_nmi(unsigned int irq, void *dev_id) +{ + struct irq_desc *desc = irq_to_desc(irq); + unsigned long flags; + const void *devname; + + if (!desc || WARN_ON(!(desc->istate & IRQS_NMI))) + return NULL; + + if (WARN_ON(irq_settings_is_per_cpu_devid(desc))) + return NULL; + + /* NMI still enabled */ + if (WARN_ON(desc->depth == 0)) + disable_nmi_nosync(irq); + + raw_spin_lock_irqsave(&desc->lock, flags); + + irq_nmi_teardown(desc); + devname = __cleanup_nmi(irq, desc); + + raw_spin_unlock_irqrestore(&desc->lock, flags); + + return devname; +} + /** * request_threaded_irq - allocate an interrupt line * @irq: Interrupt line to allocate @@ -1902,6 +2033,101 @@ int request_any_context_irq(unsigned int irq, irq_handler_t handler, } EXPORT_SYMBOL_GPL(request_any_context_irq); +/** + * request_nmi - allocate an interrupt line for NMI delivery + * @irq: Interrupt line to allocate + * @handler: Function to be called when the IRQ occurs. + * Threaded handler for threaded interrupts. + * @flags: Interrupt type flags + * @name: An ascii name for the claiming device + * @dev_id: A cookie passed back to the handler function + * + * This call allocates interrupt resources and enables the + * interrupt line and IRQ handling. It sets up the IRQ line + * to be handled as an NMI. + * + * An interrupt line delivering NMIs cannot be shared and IRQ handling + * cannot be threaded. + * + * Interrupt lines requested for NMI delivering must produce per cpu + * interrupts and have auto enabling setting disabled. + * + * Dev_id must be globally unique. Normally the address of the + * device data structure is used as the cookie. Since the handler + * receives this value it makes sense to use it. + * + * If the interrupt line cannot be used to deliver NMIs, function + * will fail and return a negative value. + */ +int request_nmi(unsigned int irq, irq_handler_t handler, + unsigned long irqflags, const char *name, void *dev_id) +{ + struct irqaction *action; + struct irq_desc *desc; + unsigned long flags; + int retval; + + if (irq == IRQ_NOTCONNECTED) + return -ENOTCONN; + + /* NMI cannot be shared, used for Polling */ + if (irqflags & (IRQF_SHARED | IRQF_COND_SUSPEND | IRQF_IRQPOLL)) + return -EINVAL; + + if (!(irqflags & IRQF_PERCPU)) + return -EINVAL; + + if (!handler) + return -EINVAL; + + desc = irq_to_desc(irq); + + if (!desc || irq_settings_can_autoenable(desc) + || !irq_settings_can_request(desc) + || WARN_ON(irq_settings_is_per_cpu_devid(desc)) + || !irq_supports_nmi(desc)) + return -EINVAL; + + action = kzalloc(sizeof(struct irqaction), GFP_KERNEL); + if (!action) + return -ENOMEM; + + action->handler = handler; + action->flags = irqflags | IRQF_NO_THREAD | IRQF_NOBALANCING; + action->name = name; + action->dev_id = dev_id; + + retval = irq_chip_pm_get(&desc->irq_data); + if (retval < 0) + goto err_out; + + retval = __setup_irq(irq, desc, action); + if (retval) + goto err_irq_setup; + + raw_spin_lock_irqsave(&desc->lock, flags); + + /* Setup NMI state */ + desc->istate |= IRQS_NMI; + retval = irq_nmi_setup(desc); + if (retval) { + __cleanup_nmi(irq, desc); + raw_spin_unlock_irqrestore(&desc->lock, flags); + return -EINVAL; + } + + raw_spin_unlock_irqrestore(&desc->lock, flags); + + return 0; + +err_irq_setup: + irq_chip_pm_put(&desc->irq_data); +err_out: + kfree(action); + + return retval; +} + void enable_percpu_irq(unsigned int irq, unsigned int type) { unsigned int cpu = smp_processor_id(); -- 1.9.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/4] genirq: Provide basic NMI management for interrupt lines 2018-07-24 11:07 ` [PATCH 1/4] genirq: Provide basic NMI management for interrupt lines Julien Thierry @ 2018-08-01 3:07 ` Ricardo Neri 2018-08-01 15:09 ` Julien Thierry 0 siblings, 1 reply; 17+ messages in thread From: Ricardo Neri @ 2018-08-01 3:07 UTC (permalink / raw) To: Julien Thierry; +Cc: linux-kernel, tglx, peterz, marc.zyngier, mingo On Tue, Jul 24, 2018 at 12:07:04PM +0100, Julien Thierry wrote: Hi Julien, Many thanks for your patchset and I apologize for the late reply. I tried to use your patches on my own use case and I have the following comments... > Add functionality to allocate interrupt lines that will deliver IRQs > as Non-Maskable Interrupts. These allocations are only successful if > the irqchip provides the necessary support and allows NMI delivery for the > interrupt line. > > Interrupt lines allocated for NMI delivery must be enabled/disabled through > enable_nmi/disable_nmi_nosync to keep their state consistent. > > To treat a PERCPU IRQ as NMI, the interrupt must not be shared nor threaded, > the irqchip directly managing the IRQ must be the root irqchip and the > irqchip cannot be behind a slow bus. > > Signed-off-by: Julien Thierry <julien.thierry@arm.com> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Ingo Molnar <mingo@kernel.org> > Cc: Marc Zyngier <marc.zyngier@arm.com> > --- > include/linux/interrupt.h | 9 ++ > include/linux/irq.h | 7 ++ > kernel/irq/debugfs.c | 6 +- > kernel/irq/internals.h | 1 + > kernel/irq/manage.c | 228 +++++++++++++++++++++++++++++++++++++++++++++- > 5 files changed, 248 insertions(+), 3 deletions(-) > > diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h > index eeceac3..3b1a320 100644 > --- a/include/linux/interrupt.h > +++ b/include/linux/interrupt.h > @@ -156,6 +156,10 @@ struct irqaction { > unsigned long flags, const char *devname, > void __percpu *percpu_dev_id); > > +extern int __must_check > +request_nmi(unsigned int irq, irq_handler_t handler, unsigned long flags, > + const char *name, void *dev); > + > static inline int __must_check > request_percpu_irq(unsigned int irq, irq_handler_t handler, > const char *devname, void __percpu *percpu_dev_id) > @@ -167,6 +171,8 @@ struct irqaction { > extern const void *free_irq(unsigned int, void *); > extern void free_percpu_irq(unsigned int, void __percpu *); > > +extern const void *free_nmi(unsigned int irq, void *dev_id); > + > struct device; > > extern int __must_check > @@ -217,6 +223,9 @@ struct irqaction { > extern bool irq_percpu_is_enabled(unsigned int irq); > extern void irq_wake_thread(unsigned int irq, void *dev_id); > > +extern void disable_nmi_nosync(unsigned int irq); > +extern void enable_nmi(unsigned int irq); > + > /* The following three functions are for the core kernel use only. */ > extern void suspend_device_irqs(void); > extern void resume_device_irqs(void); > diff --git a/include/linux/irq.h b/include/linux/irq.h > index 201de12..0d71f63 100644 > --- a/include/linux/irq.h > +++ b/include/linux/irq.h > @@ -441,6 +441,8 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d) > * @irq_set_vcpu_affinity: optional to target a vCPU in a virtual machine > * @ipi_send_single: send a single IPI to destination cpus > * @ipi_send_mask: send an IPI to destination cpus in cpumask > + * @irq_nmi_setup: function called from core code before enabling an NMI > + * @irq_nmi_teardown: function called from core code after disabling an NMI > * @flags: chip specific flags > */ > struct irq_chip { > @@ -489,6 +491,9 @@ struct irq_chip { > void (*ipi_send_single)(struct irq_data *data, unsigned int cpu); > void (*ipi_send_mask)(struct irq_data *data, const struct cpumask *dest); > > + int (*irq_nmi_setup)(struct irq_data *data); > + void (*irq_nmi_teardown)(struct irq_data *data); > + > unsigned long flags; > }; > > @@ -504,6 +509,7 @@ struct irq_chip { > * IRQCHIP_ONESHOT_SAFE: One shot does not require mask/unmask > * IRQCHIP_EOI_THREADED: Chip requires eoi() on unmask in threaded mode > * IRQCHIP_SUPPORTS_LEVEL_MSI Chip can provide two doorbells for Level MSIs > + * IRQCHIP_SUPPORTS_NMI: Chip can deliver NMIs, only for root irqchips > */ > enum { > IRQCHIP_SET_TYPE_MASKED = (1 << 0), > @@ -514,6 +520,7 @@ enum { > IRQCHIP_ONESHOT_SAFE = (1 << 5), > IRQCHIP_EOI_THREADED = (1 << 6), > IRQCHIP_SUPPORTS_LEVEL_MSI = (1 << 7), > + IRQCHIP_SUPPORTS_NMI = (1 << 8), > }; > > #include <linux/irqdesc.h> > diff --git a/kernel/irq/debugfs.c b/kernel/irq/debugfs.c > index 6f63613..59a04d2 100644 > --- a/kernel/irq/debugfs.c > +++ b/kernel/irq/debugfs.c > @@ -56,6 +56,7 @@ static void irq_debug_show_masks(struct seq_file *m, struct irq_desc *desc) { } > BIT_MASK_DESCR(IRQCHIP_ONESHOT_SAFE), > BIT_MASK_DESCR(IRQCHIP_EOI_THREADED), > BIT_MASK_DESCR(IRQCHIP_SUPPORTS_LEVEL_MSI), > + BIT_MASK_DESCR(IRQCHIP_SUPPORTS_NMI), > }; > > static void > @@ -140,6 +141,7 @@ static void irq_debug_show_masks(struct seq_file *m, struct irq_desc *desc) { } > BIT_MASK_DESCR(IRQS_WAITING), > BIT_MASK_DESCR(IRQS_PENDING), > BIT_MASK_DESCR(IRQS_SUSPENDED), > + BIT_MASK_DESCR(IRQS_NMI), > }; > > > @@ -203,8 +205,8 @@ static ssize_t irq_debug_write(struct file *file, const char __user *user_buf, > chip_bus_lock(desc); > raw_spin_lock_irqsave(&desc->lock, flags); > > - if (irq_settings_is_level(desc)) { > - /* Can't do level, sorry */ > + if (irq_settings_is_level(desc) || desc->istate & IRQS_NMI) { > + /* Can't do level nor NMIs, sorry */ > err = -EINVAL; > } else { > desc->istate |= IRQS_PENDING; > diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h > index ca6afa2..1b2c517 100644 > --- a/kernel/irq/internals.h > +++ b/kernel/irq/internals.h > @@ -60,6 +60,7 @@ enum { > IRQS_PENDING = 0x00000200, > IRQS_SUSPENDED = 0x00000800, > IRQS_TIMINGS = 0x00001000, > + IRQS_NMI = 0x00002000, It might be good to add a comment on the use of IRQS_NMI to be consistent with the rest of the elements of the enum. > }; > > #include "debug.h" > diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c > index daeabd7..3b87143 100644 > --- a/kernel/irq/manage.c > +++ b/kernel/irq/manage.c > @@ -341,7 +341,7 @@ static void irq_affinity_notify(struct work_struct *work) > /* The release function is promised process context */ > might_sleep(); > > - if (!desc) > + if (!desc || desc->istate & IRQS_NMI) > return -EINVAL; > > /* Complete initialisation of *notify */ > @@ -550,6 +550,21 @@ bool disable_hardirq(unsigned int irq) > } > EXPORT_SYMBOL_GPL(disable_hardirq); > > +/** > + * disable_nmi_nosync - disable an nmi without waiting > + * @irq: Interrupt to disable > + * > + * Disable the selected interrupt line. Disables and enables are > + * nested. > + * The interrupt to disable must have been requested through request_nmi. > + * Unlike disable_nmi(), this function does not ensure existing > + * instances of the IRQ handler have completed before returning. > + */ > +void disable_nmi_nosync(unsigned int irq) > +{ > + disable_irq_nosync(irq); > +} > + > void __enable_irq(struct irq_desc *desc) > { > switch (desc->depth) { > @@ -606,6 +621,20 @@ void enable_irq(unsigned int irq) > } > EXPORT_SYMBOL(enable_irq); > > +/** > + * enable_nmi - enable handling of an nmi > + * @irq: Interrupt to enable > + * > + * The interrupt to enable must have been requested through request_nmi. > + * Undoes the effect of one call to disable_nmi(). If this > + * matches the last disable, processing of interrupts on this > + * IRQ line is re-enabled. > + */ > +void enable_nmi(unsigned int irq) > +{ > + enable_irq(irq); > +} > + > static int set_irq_wake_real(unsigned int irq, unsigned int on) > { > struct irq_desc *desc = irq_to_desc(irq); > @@ -641,6 +670,12 @@ int irq_set_irq_wake(unsigned int irq, unsigned int on) > if (!desc) > return -EINVAL; > > + /* Don't use NMIs as wake up interrupts please */ > + if (desc->istate & IRQS_NMI) { > + ret = -EINVAL; > + goto out_unlock; > + } > + > /* wakeup-capable irqs can be shared between drivers that > * don't need to have the same sleep mode behaviors. > */ > @@ -663,6 +698,8 @@ int irq_set_irq_wake(unsigned int irq, unsigned int on) > irqd_clear(&desc->irq_data, IRQD_WAKEUP_STATE); > } > } > + > +out_unlock: > irq_put_desc_busunlock(desc, flags); > return ret; > } > @@ -1110,6 +1147,39 @@ static void irq_release_resources(struct irq_desc *desc) > c->irq_release_resources(d); > } > > +static bool irq_supports_nmi(struct irq_desc *desc) > +{ > + struct irq_data *d = irq_desc_get_irq_data(desc); > + > +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY > + /* Only IRQs directly managed by the root irqchip can be set as NMI */ > + if (d->parent_data) > + return false; > +#endif I don't think this works in x86 because the local APIC irq_chip is the root of the hierarchy; the rest of the irq_chips are children of it. Furthermore, the delivery mode (i.e., regular interrupt vs NMI) is set in these children controllers, either MSI or interrupt remapping. Instead, could it be possible to determine if NMI mode is supported by checking if any of the parents of the irq_data supports NMI mode? If it has to be the root, perhaps a flag can indicate irq_supports_nmi() about this restriction. > + /* Don't support NMIs for chips behind a slow bus */ > + if (d->chip->irq_bus_lock || d->chip->irq_bus_sync_unlock) > + return false; > + > + return d->chip->flags & IRQCHIP_SUPPORTS_NMI; > +} > + > +static int irq_nmi_setup(struct irq_desc *desc) > +{ > + struct irq_data *d = irq_desc_get_irq_data(desc); > + struct irq_chip *c = d->chip; > + > + return c->irq_nmi_setup ? c->irq_nmi_setup(d) : -EINVAL; Continuing with my comment above, the child can decide whether to configure NMI mode by itself or let it be done by a chip located higher in the hierarchy. This is already done in other situations. For instance, in MSI interrupts with interrupt remapping: the message data is populated by the remapping controller. > +} > + > +static void irq_nmi_teardown(struct irq_desc *desc) > +{ > + struct irq_data *d = irq_desc_get_irq_data(desc); > + struct irq_chip *c = d->chip; > + > + if (c->irq_nmi_teardown) > + c->irq_nmi_teardown(d); > +} > + > static int > setup_irq_thread(struct irqaction *new, unsigned int irq, bool secondary) > { > @@ -1282,9 +1352,17 @@ static void irq_release_resources(struct irq_desc *desc) > * fields must have IRQF_SHARED set and the bits which > * set the trigger type must match. Also all must > * agree on ONESHOT. > + * Interrupt lines used for NMIs cannot be shared. > */ > unsigned int oldtype; > > + if (desc->istate & IRQS_NMI) { > + pr_err("Invalid attempt to share NMI for %s (irq %d) on irqchip %s.\n", > + new->name, irq, desc->irq_data.chip->name); > + ret = -EINVAL; > + goto out_unlock; > + } > + > /* > * If nobody did set the configuration before, inherit > * the one provided by the requester. > @@ -1733,6 +1811,59 @@ const void *free_irq(unsigned int irq, void *dev_id) > } > EXPORT_SYMBOL(free_irq); > > +/* This function must be called with desc->lock held */ > +static const void *__cleanup_nmi(unsigned int irq, struct irq_desc *desc) > +{ > + const char *devname = NULL; > + > + desc->istate &= ~IRQS_NMI; > + > + if (!WARN_ON(desc->action == NULL)) { > + irq_pm_remove_action(desc, desc->action); > + devname = desc->action->name; > + unregister_handler_proc(irq, desc->action); > + > + kfree(desc->action); > + desc->action = NULL; > + } > + > + irq_settings_clr_disable_unlazy(desc); > + irq_shutdown(desc); > + > + irq_release_resources(desc); > + > + irq_chip_pm_put(&desc->irq_data); > + module_put(desc->owner); > + > + return devname; > +} > + > +const void *free_nmi(unsigned int irq, void *dev_id) > +{ > + struct irq_desc *desc = irq_to_desc(irq); > + unsigned long flags; > + const void *devname; > + > + if (!desc || WARN_ON(!(desc->istate & IRQS_NMI))) > + return NULL; > + > + if (WARN_ON(irq_settings_is_per_cpu_devid(desc))) > + return NULL; > + > + /* NMI still enabled */ > + if (WARN_ON(desc->depth == 0)) > + disable_nmi_nosync(irq); > + > + raw_spin_lock_irqsave(&desc->lock, flags); > + > + irq_nmi_teardown(desc); > + devname = __cleanup_nmi(irq, desc); > + > + raw_spin_unlock_irqrestore(&desc->lock, flags); > + > + return devname; > +} > + > /** > * request_threaded_irq - allocate an interrupt line > * @irq: Interrupt line to allocate > @@ -1902,6 +2033,101 @@ int request_any_context_irq(unsigned int irq, irq_handler_t handler, > } > EXPORT_SYMBOL_GPL(request_any_context_irq); > > +/** > + * request_nmi - allocate an interrupt line for NMI delivery > + * @irq: Interrupt line to allocate > + * @handler: Function to be called when the IRQ occurs. > + * Threaded handler for threaded interrupts. > + * @flags: Interrupt type flags > + * @name: An ascii name for the claiming device > + * @dev_id: A cookie passed back to the handler function > + * > + * This call allocates interrupt resources and enables the > + * interrupt line and IRQ handling. It sets up the IRQ line > + * to be handled as an NMI. > + * > + * An interrupt line delivering NMIs cannot be shared and IRQ handling > + * cannot be threaded. > + * > + * Interrupt lines requested for NMI delivering must produce per cpu > + * interrupts and have auto enabling setting disabled. > + * > + * Dev_id must be globally unique. Normally the address of the > + * device data structure is used as the cookie. Since the handler > + * receives this value it makes sense to use it. > + * > + * If the interrupt line cannot be used to deliver NMIs, function > + * will fail and return a negative value. > + */ > +int request_nmi(unsigned int irq, irq_handler_t handler, > + unsigned long irqflags, const char *name, void *dev_id) > +{ > + struct irqaction *action; > + struct irq_desc *desc; > + unsigned long flags; > + int retval; > + > + if (irq == IRQ_NOTCONNECTED) > + return -ENOTCONN; > + > + /* NMI cannot be shared, used for Polling */ > + if (irqflags & (IRQF_SHARED | IRQF_COND_SUSPEND | IRQF_IRQPOLL)) > + return -EINVAL; > + > + if (!(irqflags & IRQF_PERCPU)) > + return -EINVAL; > + > + if (!handler) > + return -EINVAL; It would not be possible to use this handler in x86. All the NMI handlers are put in a list populated by register_nmi_handler(). Could it be possible to have a wrapper function that does not take a handler? > + > + desc = irq_to_desc(irq); > + > + if (!desc || irq_settings_can_autoenable(desc) > + || !irq_settings_can_request(desc) Is this a hard requirement for ARM? I guess it is, because you added enable_nmi() for this very purpose. I could not find any irq in the x86 core code that uses the IRQ_NOAUTOEN flag. > + || WARN_ON(irq_settings_is_per_cpu_devid(desc)) > + || !irq_supports_nmi(desc)) Shouldn't the logical operators in a multi-line statement go at the end of the line? Thanks and BR, Ricardo ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/4] genirq: Provide basic NMI management for interrupt lines 2018-08-01 3:07 ` Ricardo Neri @ 2018-08-01 15:09 ` Julien Thierry 2018-08-01 15:33 ` Joe Perches 2018-08-02 2:03 ` Ricardo Neri 0 siblings, 2 replies; 17+ messages in thread From: Julien Thierry @ 2018-08-01 15:09 UTC (permalink / raw) To: Ricardo Neri; +Cc: linux-kernel, tglx, peterz, marc.zyngier, mingo Hi Ricardo, On 01/08/18 04:07, Ricardo Neri wrote: > On Tue, Jul 24, 2018 at 12:07:04PM +0100, Julien Thierry wrote: > > Hi Julien, > > Many thanks for your patchset and I apologize for the late reply. I tried > to use your patches on my own use case and I have the following > comments... > >> Add functionality to allocate interrupt lines that will deliver IRQs >> as Non-Maskable Interrupts. These allocations are only successful if >> the irqchip provides the necessary support and allows NMI delivery for the >> interrupt line. >> >> Interrupt lines allocated for NMI delivery must be enabled/disabled through >> enable_nmi/disable_nmi_nosync to keep their state consistent. >> >> To treat a PERCPU IRQ as NMI, the interrupt must not be shared nor threaded, >> the irqchip directly managing the IRQ must be the root irqchip and the >> irqchip cannot be behind a slow bus. >> >> Signed-off-by: Julien Thierry <julien.thierry@arm.com> >> Cc: Thomas Gleixner <tglx@linutronix.de> >> Cc: Peter Zijlstra <peterz@infradead.org> >> Cc: Ingo Molnar <mingo@kernel.org> >> Cc: Marc Zyngier <marc.zyngier@arm.com> >> --- >> include/linux/interrupt.h | 9 ++ >> include/linux/irq.h | 7 ++ >> kernel/irq/debugfs.c | 6 +- >> kernel/irq/internals.h | 1 + >> kernel/irq/manage.c | 228 +++++++++++++++++++++++++++++++++++++++++++++- >> 5 files changed, 248 insertions(+), 3 deletions(-) >> >> diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h >> index eeceac3..3b1a320 100644 >> --- a/include/linux/interrupt.h >> +++ b/include/linux/interrupt.h >> @@ -156,6 +156,10 @@ struct irqaction { >> unsigned long flags, const char *devname, >> void __percpu *percpu_dev_id); >> >> +extern int __must_check >> +request_nmi(unsigned int irq, irq_handler_t handler, unsigned long flags, >> + const char *name, void *dev); >> + >> static inline int __must_check >> request_percpu_irq(unsigned int irq, irq_handler_t handler, >> const char *devname, void __percpu *percpu_dev_id) >> @@ -167,6 +171,8 @@ struct irqaction { >> extern const void *free_irq(unsigned int, void *); >> extern void free_percpu_irq(unsigned int, void __percpu *); >> >> +extern const void *free_nmi(unsigned int irq, void *dev_id); >> + >> struct device; >> >> extern int __must_check >> @@ -217,6 +223,9 @@ struct irqaction { >> extern bool irq_percpu_is_enabled(unsigned int irq); >> extern void irq_wake_thread(unsigned int irq, void *dev_id); >> >> +extern void disable_nmi_nosync(unsigned int irq); >> +extern void enable_nmi(unsigned int irq); >> + >> /* The following three functions are for the core kernel use only. */ >> extern void suspend_device_irqs(void); >> extern void resume_device_irqs(void); >> diff --git a/include/linux/irq.h b/include/linux/irq.h >> index 201de12..0d71f63 100644 >> --- a/include/linux/irq.h >> +++ b/include/linux/irq.h >> @@ -441,6 +441,8 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d) >> * @irq_set_vcpu_affinity: optional to target a vCPU in a virtual machine >> * @ipi_send_single: send a single IPI to destination cpus >> * @ipi_send_mask: send an IPI to destination cpus in cpumask >> + * @irq_nmi_setup: function called from core code before enabling an NMI >> + * @irq_nmi_teardown: function called from core code after disabling an NMI >> * @flags: chip specific flags >> */ >> struct irq_chip { >> @@ -489,6 +491,9 @@ struct irq_chip { >> void (*ipi_send_single)(struct irq_data *data, unsigned int cpu); >> void (*ipi_send_mask)(struct irq_data *data, const struct cpumask *dest); >> >> + int (*irq_nmi_setup)(struct irq_data *data); >> + void (*irq_nmi_teardown)(struct irq_data *data); >> + >> unsigned long flags; >> }; >> >> @@ -504,6 +509,7 @@ struct irq_chip { >> * IRQCHIP_ONESHOT_SAFE: One shot does not require mask/unmask >> * IRQCHIP_EOI_THREADED: Chip requires eoi() on unmask in threaded mode >> * IRQCHIP_SUPPORTS_LEVEL_MSI Chip can provide two doorbells for Level MSIs >> + * IRQCHIP_SUPPORTS_NMI: Chip can deliver NMIs, only for root irqchips >> */ >> enum { >> IRQCHIP_SET_TYPE_MASKED = (1 << 0), >> @@ -514,6 +520,7 @@ enum { >> IRQCHIP_ONESHOT_SAFE = (1 << 5), >> IRQCHIP_EOI_THREADED = (1 << 6), >> IRQCHIP_SUPPORTS_LEVEL_MSI = (1 << 7), >> + IRQCHIP_SUPPORTS_NMI = (1 << 8), >> }; >> >> #include <linux/irqdesc.h> >> diff --git a/kernel/irq/debugfs.c b/kernel/irq/debugfs.c >> index 6f63613..59a04d2 100644 >> --- a/kernel/irq/debugfs.c >> +++ b/kernel/irq/debugfs.c >> @@ -56,6 +56,7 @@ static void irq_debug_show_masks(struct seq_file *m, struct irq_desc *desc) { } >> BIT_MASK_DESCR(IRQCHIP_ONESHOT_SAFE), >> BIT_MASK_DESCR(IRQCHIP_EOI_THREADED), >> BIT_MASK_DESCR(IRQCHIP_SUPPORTS_LEVEL_MSI), >> + BIT_MASK_DESCR(IRQCHIP_SUPPORTS_NMI), >> }; >> >> static void >> @@ -140,6 +141,7 @@ static void irq_debug_show_masks(struct seq_file *m, struct irq_desc *desc) { } >> BIT_MASK_DESCR(IRQS_WAITING), >> BIT_MASK_DESCR(IRQS_PENDING), >> BIT_MASK_DESCR(IRQS_SUSPENDED), >> + BIT_MASK_DESCR(IRQS_NMI), >> }; >> >> >> @@ -203,8 +205,8 @@ static ssize_t irq_debug_write(struct file *file, const char __user *user_buf, >> chip_bus_lock(desc); >> raw_spin_lock_irqsave(&desc->lock, flags); >> >> - if (irq_settings_is_level(desc)) { >> - /* Can't do level, sorry */ >> + if (irq_settings_is_level(desc) || desc->istate & IRQS_NMI) { >> + /* Can't do level nor NMIs, sorry */ >> err = -EINVAL; >> } else { >> desc->istate |= IRQS_PENDING; >> diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h >> index ca6afa2..1b2c517 100644 >> --- a/kernel/irq/internals.h >> +++ b/kernel/irq/internals.h >> @@ -60,6 +60,7 @@ enum { >> IRQS_PENDING = 0x00000200, >> IRQS_SUSPENDED = 0x00000800, >> IRQS_TIMINGS = 0x00001000, >> + IRQS_NMI = 0x00002000, > > It might be good to add a comment on the use of IRQS_NMI to be consistent > with the rest of the elements of the enum. > Good point, I'll do that. >> }; >> >> #include "debug.h" >> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c >> index daeabd7..3b87143 100644 >> --- a/kernel/irq/manage.c >> +++ b/kernel/irq/manage.c >> @@ -341,7 +341,7 @@ static void irq_affinity_notify(struct work_struct *work) >> /* The release function is promised process context */ >> might_sleep(); >> >> - if (!desc) >> + if (!desc || desc->istate & IRQS_NMI) >> return -EINVAL; >> >> /* Complete initialisation of *notify */ >> @@ -550,6 +550,21 @@ bool disable_hardirq(unsigned int irq) >> } >> EXPORT_SYMBOL_GPL(disable_hardirq); >> >> +/** >> + * disable_nmi_nosync - disable an nmi without waiting >> + * @irq: Interrupt to disable >> + * >> + * Disable the selected interrupt line. Disables and enables are >> + * nested. >> + * The interrupt to disable must have been requested through request_nmi. >> + * Unlike disable_nmi(), this function does not ensure existing >> + * instances of the IRQ handler have completed before returning. >> + */ >> +void disable_nmi_nosync(unsigned int irq) >> +{ >> + disable_irq_nosync(irq); >> +} >> + >> void __enable_irq(struct irq_desc *desc) >> { >> switch (desc->depth) { >> @@ -606,6 +621,20 @@ void enable_irq(unsigned int irq) >> } >> EXPORT_SYMBOL(enable_irq); >> >> +/** >> + * enable_nmi - enable handling of an nmi >> + * @irq: Interrupt to enable >> + * >> + * The interrupt to enable must have been requested through request_nmi. >> + * Undoes the effect of one call to disable_nmi(). If this >> + * matches the last disable, processing of interrupts on this >> + * IRQ line is re-enabled. >> + */ >> +void enable_nmi(unsigned int irq) >> +{ >> + enable_irq(irq); >> +} >> + >> static int set_irq_wake_real(unsigned int irq, unsigned int on) >> { >> struct irq_desc *desc = irq_to_desc(irq); >> @@ -641,6 +670,12 @@ int irq_set_irq_wake(unsigned int irq, unsigned int on) >> if (!desc) >> return -EINVAL; >> >> + /* Don't use NMIs as wake up interrupts please */ >> + if (desc->istate & IRQS_NMI) { >> + ret = -EINVAL; >> + goto out_unlock; >> + } >> + >> /* wakeup-capable irqs can be shared between drivers that >> * don't need to have the same sleep mode behaviors. >> */ >> @@ -663,6 +698,8 @@ int irq_set_irq_wake(unsigned int irq, unsigned int on) >> irqd_clear(&desc->irq_data, IRQD_WAKEUP_STATE); >> } >> } >> + >> +out_unlock: >> irq_put_desc_busunlock(desc, flags); >> return ret; >> } >> @@ -1110,6 +1147,39 @@ static void irq_release_resources(struct irq_desc *desc) >> c->irq_release_resources(d); >> } >> >> +static bool irq_supports_nmi(struct irq_desc *desc) >> +{ >> + struct irq_data *d = irq_desc_get_irq_data(desc); >> + >> +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY >> + /* Only IRQs directly managed by the root irqchip can be set as NMI */ >> + if (d->parent_data) >> + return false; >> +#endif > > I don't think this works in x86 because the local APIC irq_chip is the > root of the hierarchy; the rest of the irq_chips are children of it. > Furthermore, the delivery mode (i.e., regular interrupt vs NMI) is set in > these children controllers, either MSI or interrupt remapping. > > Instead, could it be possible to determine if NMI mode is supported by > checking if any of the parents of the irq_data supports NMI mode? If it > has to be the root, perhaps a flag can indicate irq_supports_nmi() about > this restriction. > I see, I'm not very sure how to deal with the semantics here. Does it make sense for an irqchip to be able to deliver NMIs if its parent doesn't? If we want to handle NMI in and irqchip hierachy, shouldn't all irqchips between the one delivering the NMI and the root irqchip also be able to deliver NMIs? (Or is it the other way around and an irqchip can deliver an NMI if all its children irqchip are also able to deliver an NMI?) >> + /* Don't support NMIs for chips behind a slow bus */ >> + if (d->chip->irq_bus_lock || d->chip->irq_bus_sync_unlock) >> + return false; >> + >> + return d->chip->flags & IRQCHIP_SUPPORTS_NMI; >> +} >> + >> +static int irq_nmi_setup(struct irq_desc *desc) >> +{ >> + struct irq_data *d = irq_desc_get_irq_data(desc); >> + struct irq_chip *c = d->chip; >> + >> + return c->irq_nmi_setup ? c->irq_nmi_setup(d) : -EINVAL; > > Continuing with my comment above, the child can decide whether to configure > NMI mode by itself or let it be done by a chip located higher in the > hierarchy. This is already done in other situations. For instance, in MSI > interrupts with interrupt remapping: the message data is populated by the > remapping controller. > Yes, if we want to handle the chip hierarchy, this will need modification. >> +} >> + >> +static void irq_nmi_teardown(struct irq_desc *desc) >> +{ >> + struct irq_data *d = irq_desc_get_irq_data(desc); >> + struct irq_chip *c = d->chip; >> + >> + if (c->irq_nmi_teardown) >> + c->irq_nmi_teardown(d); >> +} >> + >> static int >> setup_irq_thread(struct irqaction *new, unsigned int irq, bool secondary) >> { >> @@ -1282,9 +1352,17 @@ static void irq_release_resources(struct irq_desc *desc) >> * fields must have IRQF_SHARED set and the bits which >> * set the trigger type must match. Also all must >> * agree on ONESHOT. >> + * Interrupt lines used for NMIs cannot be shared. >> */ >> unsigned int oldtype; >> >> + if (desc->istate & IRQS_NMI) { >> + pr_err("Invalid attempt to share NMI for %s (irq %d) on irqchip %s.\n", >> + new->name, irq, desc->irq_data.chip->name); >> + ret = -EINVAL; >> + goto out_unlock; >> + } >> + >> /* >> * If nobody did set the configuration before, inherit >> * the one provided by the requester. >> @@ -1733,6 +1811,59 @@ const void *free_irq(unsigned int irq, void *dev_id) >> } >> EXPORT_SYMBOL(free_irq); >> >> +/* This function must be called with desc->lock held */ >> +static const void *__cleanup_nmi(unsigned int irq, struct irq_desc *desc) >> +{ >> + const char *devname = NULL; >> + >> + desc->istate &= ~IRQS_NMI; >> + >> + if (!WARN_ON(desc->action == NULL)) { >> + irq_pm_remove_action(desc, desc->action); >> + devname = desc->action->name; >> + unregister_handler_proc(irq, desc->action); >> + >> + kfree(desc->action); >> + desc->action = NULL; >> + } >> + >> + irq_settings_clr_disable_unlazy(desc); >> + irq_shutdown(desc); >> + >> + irq_release_resources(desc); >> + >> + irq_chip_pm_put(&desc->irq_data); >> + module_put(desc->owner); >> + >> + return devname; >> +} >> + >> +const void *free_nmi(unsigned int irq, void *dev_id) >> +{ >> + struct irq_desc *desc = irq_to_desc(irq); >> + unsigned long flags; >> + const void *devname; >> + >> + if (!desc || WARN_ON(!(desc->istate & IRQS_NMI))) >> + return NULL; >> + >> + if (WARN_ON(irq_settings_is_per_cpu_devid(desc))) >> + return NULL; >> + >> + /* NMI still enabled */ >> + if (WARN_ON(desc->depth == 0)) >> + disable_nmi_nosync(irq); >> + >> + raw_spin_lock_irqsave(&desc->lock, flags); >> + >> + irq_nmi_teardown(desc); >> + devname = __cleanup_nmi(irq, desc); >> + >> + raw_spin_unlock_irqrestore(&desc->lock, flags); >> + >> + return devname; >> +} >> + >> /** >> * request_threaded_irq - allocate an interrupt line >> * @irq: Interrupt line to allocate >> @@ -1902,6 +2033,101 @@ int request_any_context_irq(unsigned int irq, irq_handler_t handler, >> } >> EXPORT_SYMBOL_GPL(request_any_context_irq); >> >> +/** >> + * request_nmi - allocate an interrupt line for NMI delivery >> + * @irq: Interrupt line to allocate >> + * @handler: Function to be called when the IRQ occurs. >> + * Threaded handler for threaded interrupts. >> + * @flags: Interrupt type flags >> + * @name: An ascii name for the claiming device >> + * @dev_id: A cookie passed back to the handler function >> + * >> + * This call allocates interrupt resources and enables the >> + * interrupt line and IRQ handling. It sets up the IRQ line >> + * to be handled as an NMI. >> + * >> + * An interrupt line delivering NMIs cannot be shared and IRQ handling >> + * cannot be threaded. >> + * >> + * Interrupt lines requested for NMI delivering must produce per cpu >> + * interrupts and have auto enabling setting disabled. >> + * >> + * Dev_id must be globally unique. Normally the address of the >> + * device data structure is used as the cookie. Since the handler >> + * receives this value it makes sense to use it. >> + * >> + * If the interrupt line cannot be used to deliver NMIs, function >> + * will fail and return a negative value. >> + */ >> +int request_nmi(unsigned int irq, irq_handler_t handler, >> + unsigned long irqflags, const char *name, void *dev_id) >> +{ >> + struct irqaction *action; >> + struct irq_desc *desc; >> + unsigned long flags; >> + int retval; >> + >> + if (irq == IRQ_NOTCONNECTED) >> + return -ENOTCONN; >> + >> + /* NMI cannot be shared, used for Polling */ >> + if (irqflags & (IRQF_SHARED | IRQF_COND_SUSPEND | IRQF_IRQPOLL)) >> + return -EINVAL; >> + >> + if (!(irqflags & IRQF_PERCPU)) >> + return -EINVAL; >> + >> + if (!handler) >> + return -EINVAL; > > It would not be possible to use this handler in x86. All the NMI handlers are > put in a list populated by register_nmi_handler(). Could it be possible to > have a wrapper function that does not take a handler? > If it is just a matter of having a dummy handler, x86 NMI requesters could provide it (and you maintain your list on the side). I'm not sure there is a necessity for it to be in the core code. I guess the x86 flow handler for NMI simply doesn't make use of the action structure. I can add the dummy handler (and the request_nmi_no_handler) option, I'm just unsure it makes sense to include it in the irq core code. (I can always put it in a separate patch and see if it gets accepted). >> + >> + desc = irq_to_desc(irq); >> + >> + if (!desc || irq_settings_can_autoenable(desc) >> + || !irq_settings_can_request(desc) > > Is this a hard requirement for ARM? I guess it is, because you added > enable_nmi() for this very purpose. I could not find any irq in the x86 > core code that uses the IRQ_NOAUTOEN flag. > On Arm64, we do use IRQ_NOAUTOEN and it is used for one of the interrupts we'd like to handle as NMI. So we do need IRQ_NOAUTOEN to be supported for NMIs. However, the rationale for forcing it for NMIs was: - I wanted to avoid too many things being done automatically for NMIs, this way most of the responsability of managing the NMI is on the NMI requester rather than the core code. - There would be a need to pass down the information that we are requesting an NMI to __setup_irq and do the proper setup when enabling depending on whether we have an NMI or not. I'm not completely against supporting autoenable for NMIs, but I'd like to have the opinion of others on the matter The addition of enable_nmi is unrelated. One could disable an NMI that was auto-enabled and then re-enable it. >> + || WARN_ON(irq_settings_is_per_cpu_devid(desc)) >> + || !irq_supports_nmi(desc)) > > Shouldn't the logical operators in a multi-line statement go at the end of > the line? I couldn't find anything about this in the kernel coding style and checkpatch didn't complain. However the existing multiple line conditions in this file put the operator just before the line break as you say. I'll fix this in the other patches as well. Thanks, -- Julien Thierry ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/4] genirq: Provide basic NMI management for interrupt lines 2018-08-01 15:09 ` Julien Thierry @ 2018-08-01 15:33 ` Joe Perches 2018-08-02 2:03 ` Ricardo Neri 1 sibling, 0 replies; 17+ messages in thread From: Joe Perches @ 2018-08-01 15:33 UTC (permalink / raw) To: Julien Thierry, Ricardo Neri Cc: linux-kernel, tglx, peterz, marc.zyngier, mingo On Wed, 2018-08-01 at 16:09 +0100, Julien Thierry wrote: > On 01/08/18 04:07, Ricardo Neri wrote: > > On Tue, Jul 24, 2018 at 12:07:04PM +0100, Julien Thierry wrote: > > > + || WARN_ON(irq_settings_is_per_cpu_devid(desc)) > > > + || !irq_supports_nmi(desc)) > > > > Shouldn't the logical operators in a multi-line statement go at the end of > > the line? > > I couldn't find anything about this in the kernel coding style and > checkpatch didn't complain. coding-style is a set of suggestions not dicta. Many of the tests in checkpatch are not written out in coding-style. checkpatch does emit a message when used with --strict here: The checkpatch test is: # check for && or || at the start of a line if ($rawline =~ /^\+\s*(&&|\|\|)/) { CHK("LOGICAL_CONTINUATIONS", "Logical continuations should be on the previous line\n" . $hereprev); } This message is not always emitted because code outside of net, drivers/net, and drivers/staging does not use --strict by default. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/4] genirq: Provide basic NMI management for interrupt lines 2018-08-01 15:09 ` Julien Thierry 2018-08-01 15:33 ` Joe Perches @ 2018-08-02 2:03 ` Ricardo Neri 2018-08-02 6:38 ` Marc Zyngier 1 sibling, 1 reply; 17+ messages in thread From: Ricardo Neri @ 2018-08-02 2:03 UTC (permalink / raw) To: Julien Thierry; +Cc: linux-kernel, tglx, peterz, marc.zyngier, mingo On Wed, Aug 01, 2018 at 04:09:59PM +0100, Julien Thierry wrote: > >>+static bool irq_supports_nmi(struct irq_desc *desc) > >>+{ > >>+ struct irq_data *d = irq_desc_get_irq_data(desc); > >>+ > >>+#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY > >>+ /* Only IRQs directly managed by the root irqchip can be set as NMI */ > >>+ if (d->parent_data) > >>+ return false; > >>+#endif > > > >I don't think this works in x86 because the local APIC irq_chip is the > >root of the hierarchy; the rest of the irq_chips are children of it. > >Furthermore, the delivery mode (i.e., regular interrupt vs NMI) is set in > >these children controllers, either MSI or interrupt remapping. > > > >Instead, could it be possible to determine if NMI mode is supported by > >checking if any of the parents of the irq_data supports NMI mode? If it > >has to be the root, perhaps a flag can indicate irq_supports_nmi() about > >this restriction. > > > > I see, I'm not very sure how to deal with the semantics here. Does it make > sense for an irqchip to be able to deliver NMIs if its parent doesn't? I think it does not make sense. > > If we want to handle NMI in and irqchip hierachy, shouldn't all irqchips > between the one delivering the NMI and the root irqchip also be able to > deliver NMIs? (Or is it the other way around and an irqchip can deliver an > NMI if all its children irqchip are also able to deliver an NMI?) At least in APIC, all the irq_chips support NMI delivery. However only one of them will generate the NMI interrupt message, the rest (e.g., the local APIC) will only relay it to the CPU. So yes, I agree that the whole hierarchy should support NMI delivery, but the chip generating the NMI is not necessarily the root chip. One question that I have is that whether the new flag IRQCHIP_SUPPORTS_NMI should be added to irq_chips that only relay NMIs or only to those from which an NMI can be requested. > > >>+ /* Don't support NMIs for chips behind a slow bus */ > >>+ if (d->chip->irq_bus_lock || d->chip->irq_bus_sync_unlock) > >>+ return false; > >>+ > >>+ return d->chip->flags & IRQCHIP_SUPPORTS_NMI; > >>+} > >>+ > >>+static int irq_nmi_setup(struct irq_desc *desc) > >>+{ > >>+ struct irq_data *d = irq_desc_get_irq_data(desc); > >>+ struct irq_chip *c = d->chip; > >>+ > >>+ return c->irq_nmi_setup ? c->irq_nmi_setup(d) : -EINVAL; > > > >Continuing with my comment above, the child can decide whether to configure > >NMI mode by itself or let it be done by a chip located higher in the > >hierarchy. This is already done in other situations. For instance, in MSI > >interrupts with interrupt remapping: the message data is populated by the > >remapping controller. > > > > Yes, if we want to handle the chip hierarchy, this will need modification. > > >>+} > >>+ > >>+static void irq_nmi_teardown(struct irq_desc *desc) > >>+{ > >>+ struct irq_data *d = irq_desc_get_irq_data(desc); > >>+ struct irq_chip *c = d->chip; > >>+ > >>+ if (c->irq_nmi_teardown) > >>+ c->irq_nmi_teardown(d); > >>+} > >>+ > >> static int > >> setup_irq_thread(struct irqaction *new, unsigned int irq, bool secondary) > >> { > >>@@ -1282,9 +1352,17 @@ static void irq_release_resources(struct irq_desc *desc) > >> * fields must have IRQF_SHARED set and the bits which > >> * set the trigger type must match. Also all must > >> * agree on ONESHOT. > >>+ * Interrupt lines used for NMIs cannot be shared. > >> */ > >> unsigned int oldtype; > >>+ if (desc->istate & IRQS_NMI) { > >>+ pr_err("Invalid attempt to share NMI for %s (irq %d) on irqchip %s.\n", > >>+ new->name, irq, desc->irq_data.chip->name); > >>+ ret = -EINVAL; > >>+ goto out_unlock; > >>+ } > >>+ > >> /* > >> * If nobody did set the configuration before, inherit > >> * the one provided by the requester. > >>@@ -1733,6 +1811,59 @@ const void *free_irq(unsigned int irq, void *dev_id) > >> } > >> EXPORT_SYMBOL(free_irq); > >>+/* This function must be called with desc->lock held */ > >>+static const void *__cleanup_nmi(unsigned int irq, struct irq_desc *desc) > >>+{ > >>+ const char *devname = NULL; > >>+ > >>+ desc->istate &= ~IRQS_NMI; > >>+ > >>+ if (!WARN_ON(desc->action == NULL)) { > >>+ irq_pm_remove_action(desc, desc->action); > >>+ devname = desc->action->name; > >>+ unregister_handler_proc(irq, desc->action); > >>+ > >>+ kfree(desc->action); > >>+ desc->action = NULL; > >>+ } > >>+ > >>+ irq_settings_clr_disable_unlazy(desc); > >>+ irq_shutdown(desc); > >>+ > >>+ irq_release_resources(desc); > >>+ > >>+ irq_chip_pm_put(&desc->irq_data); > >>+ module_put(desc->owner); > >>+ > >>+ return devname; > >>+} > >>+ > >>+const void *free_nmi(unsigned int irq, void *dev_id) > >>+{ > >>+ struct irq_desc *desc = irq_to_desc(irq); > >>+ unsigned long flags; > >>+ const void *devname; > >>+ > >>+ if (!desc || WARN_ON(!(desc->istate & IRQS_NMI))) > >>+ return NULL; > >>+ > >>+ if (WARN_ON(irq_settings_is_per_cpu_devid(desc))) > >>+ return NULL; > >>+ > >>+ /* NMI still enabled */ > >>+ if (WARN_ON(desc->depth == 0)) > >>+ disable_nmi_nosync(irq); > >>+ > >>+ raw_spin_lock_irqsave(&desc->lock, flags); > >>+ > >>+ irq_nmi_teardown(desc); > >>+ devname = __cleanup_nmi(irq, desc); > >>+ > >>+ raw_spin_unlock_irqrestore(&desc->lock, flags); > >>+ > >>+ return devname; > >>+} > >>+ > >> /** > >> * request_threaded_irq - allocate an interrupt line > >> * @irq: Interrupt line to allocate > >>@@ -1902,6 +2033,101 @@ int request_any_context_irq(unsigned int irq, irq_handler_t handler, > >> } > >> EXPORT_SYMBOL_GPL(request_any_context_irq); > >>+/** > >>+ * request_nmi - allocate an interrupt line for NMI delivery > >>+ * @irq: Interrupt line to allocate > >>+ * @handler: Function to be called when the IRQ occurs. > >>+ * Threaded handler for threaded interrupts. > >>+ * @flags: Interrupt type flags > >>+ * @name: An ascii name for the claiming device > >>+ * @dev_id: A cookie passed back to the handler function > >>+ * > >>+ * This call allocates interrupt resources and enables the > >>+ * interrupt line and IRQ handling. It sets up the IRQ line > >>+ * to be handled as an NMI. > >>+ * > >>+ * An interrupt line delivering NMIs cannot be shared and IRQ handling > >>+ * cannot be threaded. > >>+ * > >>+ * Interrupt lines requested for NMI delivering must produce per cpu > >>+ * interrupts and have auto enabling setting disabled. > >>+ * > >>+ * Dev_id must be globally unique. Normally the address of the > >>+ * device data structure is used as the cookie. Since the handler > >>+ * receives this value it makes sense to use it. > >>+ * > >>+ * If the interrupt line cannot be used to deliver NMIs, function > >>+ * will fail and return a negative value. > >>+ */ > >>+int request_nmi(unsigned int irq, irq_handler_t handler, > >>+ unsigned long irqflags, const char *name, void *dev_id) > >>+{ > >>+ struct irqaction *action; > >>+ struct irq_desc *desc; > >>+ unsigned long flags; > >>+ int retval; > >>+ > >>+ if (irq == IRQ_NOTCONNECTED) > >>+ return -ENOTCONN; > >>+ > >>+ /* NMI cannot be shared, used for Polling */ > >>+ if (irqflags & (IRQF_SHARED | IRQF_COND_SUSPEND | IRQF_IRQPOLL)) > >>+ return -EINVAL; > >>+ > >>+ if (!(irqflags & IRQF_PERCPU)) > >>+ return -EINVAL; > >>+ > >>+ if (!handler) > >>+ return -EINVAL; > > > >It would not be possible to use this handler in x86. All the NMI handlers are > >put in a list populated by register_nmi_handler(). Could it be possible to > >have a wrapper function that does not take a handler? > > > > If it is just a matter of having a dummy handler, x86 NMI requesters could > provide it (and you maintain your list on the side). I'm not sure there is a > necessity for it to be in the core code. > > I guess the x86 flow handler for NMI simply doesn't make use of the action > structure. > > I can add the dummy handler (and the request_nmi_no_handler) option, I'm > just unsure it makes sense to include it in the irq core code. (I can always > put it in a separate patch and see if it gets accepted). Hmm, either alternatives would look ugly but I don't see a cleaner solution. My suggestion would be to keep the handler as an argument of request_nmi() and have callers to implement their dummy handlers. In this way, the core code can be kept clean. > > >>+ > >>+ desc = irq_to_desc(irq); > >>+ > >>+ if (!desc || irq_settings_can_autoenable(desc) > >>+ || !irq_settings_can_request(desc) > > > >Is this a hard requirement for ARM? I guess it is, because you added > >enable_nmi() for this very purpose. I could not find any irq in the x86 > >core code that uses the IRQ_NOAUTOEN flag. > > > > On Arm64, we do use IRQ_NOAUTOEN and it is used for one of the interrupts > we'd like to handle as NMI. So we do need IRQ_NOAUTOEN to be supported for > NMIs. I am not sure what would be the changes to add IRQ_NOAUTOEN to a specific interrupt in my use case and whether these changes would be regarded as a hack for a special case. I'll investigate and get back to you. > > However, the rationale for forcing it for NMIs was: > - I wanted to avoid too many things being done automatically for NMIs, this > way most of the responsability of managing the NMI is on the NMI requester > rather than the core code. But doesn't the NMI requester already have some control? It can control at least the source of the NMIs (e.g., it can control when to start the timer that would generate the NMI). > > - There would be a need to pass down the information that we are requesting > an NMI to __setup_irq and do the proper setup when enabling depending on > whether we have an NMI or not. > > I'm not completely against supporting autoenable for NMIs, but I'd like to > have the opinion of others on the matter +1 Thanks and BR, Ricardo ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/4] genirq: Provide basic NMI management for interrupt lines 2018-08-02 2:03 ` Ricardo Neri @ 2018-08-02 6:38 ` Marc Zyngier 2018-08-02 6:55 ` Thomas Gleixner 0 siblings, 1 reply; 17+ messages in thread From: Marc Zyngier @ 2018-08-02 6:38 UTC (permalink / raw) To: Ricardo Neri; +Cc: Julien Thierry, linux-kernel, tglx, peterz, mingo On Thu, 02 Aug 2018 03:03:20 +0100, Ricardo Neri <ricardo.neri-calderon@linux.intel.com> wrote: > > On Wed, Aug 01, 2018 at 04:09:59PM +0100, Julien Thierry wrote: > > >>+static bool irq_supports_nmi(struct irq_desc *desc) > > >>+{ > > >>+ struct irq_data *d = irq_desc_get_irq_data(desc); > > >>+ > > >>+#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY > > >>+ /* Only IRQs directly managed by the root irqchip can be set as NMI */ > > >>+ if (d->parent_data) > > >>+ return false; > > >>+#endif > > > > > >I don't think this works in x86 because the local APIC irq_chip is the > > >root of the hierarchy; the rest of the irq_chips are children of it. > > >Furthermore, the delivery mode (i.e., regular interrupt vs NMI) is set in > > >these children controllers, either MSI or interrupt remapping. > > > > > >Instead, could it be possible to determine if NMI mode is supported by > > >checking if any of the parents of the irq_data supports NMI mode? If it > > >has to be the root, perhaps a flag can indicate irq_supports_nmi() about > > >this restriction. > > > > > > > I see, I'm not very sure how to deal with the semantics here. Does it make > > sense for an irqchip to be able to deliver NMIs if its parent doesn't? > > I think it does not make sense. > > > > > If we want to handle NMI in and irqchip hierachy, shouldn't all irqchips > > between the one delivering the NMI and the root irqchip also be able to > > deliver NMIs? (Or is it the other way around and an irqchip can deliver an > > NMI if all its children irqchip are also able to deliver an NMI?) > > At least in APIC, all the irq_chips support NMI delivery. However only one > of them will generate the NMI interrupt message, the rest (e.g., the local > APIC) will only relay it to the CPU. > > So yes, I agree that the whole hierarchy should support NMI delivery, but > the chip generating the NMI is not necessarily the root chip. > > One question that I have is that whether the new flag IRQCHIP_SUPPORTS_NMI > should be added to irq_chips that only relay NMIs or only to those from > which an NMI can be requested. If we need to distinguish between the two, then we need two flags. One that indicates the generation capability, and one that indicates the forwarding capability. > > >>+ > > >>+ desc = irq_to_desc(irq); > > >>+ > > >>+ if (!desc || irq_settings_can_autoenable(desc) > > >>+ || !irq_settings_can_request(desc) > > > > > >Is this a hard requirement for ARM? I guess it is, because you added > > >enable_nmi() for this very purpose. I could not find any irq in the x86 > > >core code that uses the IRQ_NOAUTOEN flag. > > > > > > > On Arm64, we do use IRQ_NOAUTOEN and it is used for one of the interrupts > > we'd like to handle as NMI. So we do need IRQ_NOAUTOEN to be supported for > > NMIs. > > I am not sure what would be the changes to add IRQ_NOAUTOEN to a specific > interrupt in my use case and whether these changes would be regarded as a > hack for a special case. I'll investigate and get back to you. > > > > > However, the rationale for forcing it for NMIs was: > > - I wanted to avoid too many things being done automatically for NMIs, this > > way most of the responsability of managing the NMI is on the NMI requester > > rather than the core code. > > But doesn't the NMI requester already have some control? It can control at > least the source of the NMIs (e.g., it can control when to start the timer > that would generate the NMI). > > > > > - There would be a need to pass down the information that we are requesting > > an NMI to __setup_irq and do the proper setup when enabling depending on > > whether we have an NMI or not. > > > > I'm not completely against supporting autoenable for NMIs, but I'd like to > > have the opinion of others on the matter > > +1 I guess it is a matter of defining precisely where you want the enabling to take place, and using NOAUTOEN does give you flexibility. It also gives a coherent API with the percpu NMIs which do require a separate, percpu enable. Now, it is pretty easy to support both behaviours: we could bring back the auto-enable on request for non-percpu NMIs, and rely on a irq_set_status_flags(irq, IRQ_NOAUTOEN) before the request... Thanks, M. -- Jazz is not dead, it just smell funny. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/4] genirq: Provide basic NMI management for interrupt lines 2018-08-02 6:38 ` Marc Zyngier @ 2018-08-02 6:55 ` Thomas Gleixner 2018-08-02 7:35 ` Marc Zyngier 0 siblings, 1 reply; 17+ messages in thread From: Thomas Gleixner @ 2018-08-02 6:55 UTC (permalink / raw) To: Marc Zyngier Cc: Ricardo Neri, Julien Thierry, LKML, Peter Zijlstra, Ingo Molnar On Thu, 2 Aug 2018, Marc Zyngier wrote: > On Thu, 02 Aug 2018 03:03:20 +0100, > Ricardo Neri <ricardo.neri-calderon@linux.intel.com> wrote: > > On Wed, Aug 01, 2018 at 04:09:59PM +0100, Julien Thierry wrote: > > > > > > > >I don't think this works in x86 because the local APIC irq_chip is the > > > >root of the hierarchy; the rest of the irq_chips are children of it. > > > >Furthermore, the delivery mode (i.e., regular interrupt vs NMI) is set in > > > >these children controllers, either MSI or interrupt remapping. > > > > > > > >Instead, could it be possible to determine if NMI mode is supported by > > > >checking if any of the parents of the irq_data supports NMI mode? If it > > > >has to be the root, perhaps a flag can indicate irq_supports_nmi() about > > > >this restriction. > > > > > > > > > > I see, I'm not very sure how to deal with the semantics here. Does it make > > > sense for an irqchip to be able to deliver NMIs if its parent doesn't? > > > > I think it does not make sense. > > > > > > > > If we want to handle NMI in and irqchip hierachy, shouldn't all irqchips > > > between the one delivering the NMI and the root irqchip also be able to > > > deliver NMIs? (Or is it the other way around and an irqchip can deliver an > > > NMI if all its children irqchip are also able to deliver an NMI?) > > > > At least in APIC, all the irq_chips support NMI delivery. However only one > > of them will generate the NMI interrupt message, the rest (e.g., the local > > APIC) will only relay it to the CPU. > > > > So yes, I agree that the whole hierarchy should support NMI delivery, but > > the chip generating the NMI is not necessarily the root chip. > > > > One question that I have is that whether the new flag IRQCHIP_SUPPORTS_NMI > > should be added to irq_chips that only relay NMIs or only to those from > > which an NMI can be requested. > > If we need to distinguish between the two, then we need two flags. One > that indicates the generation capability, and one that indicates the > forwarding capability. There is absolutely no reason to expose this on x86, really. Why? Because NMI is an utter trainwreck on x86. It's a single entry point without the ability of differentiation from which source the NMI originated. So mapping it to anything generic is just not going to work. It has absolutely nothing to do with the normal way of vector based interrupt operation and I don't see at all how adding this just because would improve anything on x86. In fact it would create more problems than it solves. Thanks, tglx ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/4] genirq: Provide basic NMI management for interrupt lines 2018-08-02 6:55 ` Thomas Gleixner @ 2018-08-02 7:35 ` Marc Zyngier 2018-08-02 9:40 ` Thomas Gleixner 0 siblings, 1 reply; 17+ messages in thread From: Marc Zyngier @ 2018-08-02 7:35 UTC (permalink / raw) To: Thomas Gleixner Cc: Ricardo Neri, Julien Thierry, LKML, Peter Zijlstra, Ingo Molnar On Thu, 02 Aug 2018 07:55:49 +0100, Thomas Gleixner <tglx@linutronix.de> wrote: > > On Thu, 2 Aug 2018, Marc Zyngier wrote: > > > > If we need to distinguish between the two, then we need two flags. One > > that indicates the generation capability, and one that indicates the > > forwarding capability. > > There is absolutely no reason to expose this on x86, really. > > Why? > > Because NMI is an utter trainwreck on x86. It's a single entry point > without the ability of differentiation from which source the NMI > originated. So mapping it to anything generic is just not going to work. > > It has absolutely nothing to do with the normal way of vector based > interrupt operation and I don't see at all how adding this just because > would improve anything on x86. In fact it would create more problems than > it solves. Fair enough. Does it mean Julien can completely ignore the x86 requirements for this and focus on something that fit the need of architectures where (pseudo-)NMIs can be managed similarly to normal interrupts (arm, arm64, sparc...)? Thanks, M. -- Jazz is not dead, it just smell funny. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/4] genirq: Provide basic NMI management for interrupt lines 2018-08-02 7:35 ` Marc Zyngier @ 2018-08-02 9:40 ` Thomas Gleixner 2018-08-03 3:09 ` Ricardo Neri 0 siblings, 1 reply; 17+ messages in thread From: Thomas Gleixner @ 2018-08-02 9:40 UTC (permalink / raw) To: Marc Zyngier Cc: Ricardo Neri, Julien Thierry, LKML, Peter Zijlstra, Ingo Molnar On Thu, 2 Aug 2018, Marc Zyngier wrote: > On Thu, 02 Aug 2018 07:55:49 +0100, > Thomas Gleixner <tglx@linutronix.de> wrote: > > > > On Thu, 2 Aug 2018, Marc Zyngier wrote: > > > > > > If we need to distinguish between the two, then we need two flags. One > > > that indicates the generation capability, and one that indicates the > > > forwarding capability. > > > > There is absolutely no reason to expose this on x86, really. > > > > Why? > > > > Because NMI is an utter trainwreck on x86. It's a single entry point > > without the ability of differentiation from which source the NMI > > originated. So mapping it to anything generic is just not going to work. > > > > It has absolutely nothing to do with the normal way of vector based > > interrupt operation and I don't see at all how adding this just because > > would improve anything on x86. In fact it would create more problems than > > it solves. > > Fair enough. Does it mean Julien can completely ignore the x86 > requirements for this and focus on something that fit the need of > architectures where (pseudo-)NMIs can be managed similarly to normal > interrupts (arm, arm64, sparc...)? Yes, focussing on "sane" architectures (by some definition of sane) where the NMI mode is just changing the delivery restrictions allows to still differentiate from which source the NMI originates. There is no way to make this work on x86 unless they fundamentally change the low level exception mechanics. Not going to happen anytime soon. And if it ever happens then they better implement it in a way which is usable by software sanely. Thanks, tglx ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/4] genirq: Provide basic NMI management for interrupt lines 2018-08-02 9:40 ` Thomas Gleixner @ 2018-08-03 3:09 ` Ricardo Neri 2018-08-03 7:59 ` Thomas Gleixner 0 siblings, 1 reply; 17+ messages in thread From: Ricardo Neri @ 2018-08-03 3:09 UTC (permalink / raw) To: Thomas Gleixner Cc: Marc Zyngier, Julien Thierry, LKML, Peter Zijlstra, Ingo Molnar On Thu, Aug 02, 2018 at 11:40:55AM +0200, Thomas Gleixner wrote: > On Thu, 2 Aug 2018, Marc Zyngier wrote: > > On Thu, 02 Aug 2018 07:55:49 +0100, > > Thomas Gleixner <tglx@linutronix.de> wrote: > > > > > > On Thu, 2 Aug 2018, Marc Zyngier wrote: > > > > > > > > If we need to distinguish between the two, then we need two flags. One > > > > that indicates the generation capability, and one that indicates the > > > > forwarding capability. > > > > > > There is absolutely no reason to expose this on x86, really. > > > > > > Why? > > > > > > Because NMI is an utter trainwreck on x86. It's a single entry point > > > without the ability of differentiation from which source the NMI > > > originated. So mapping it to anything generic is just not going to work. > > > > > > It has absolutely nothing to do with the normal way of vector based > > > interrupt operation and I don't see at all how adding this just because > > > would improve anything on x86. In fact it would create more problems than > > > it solves. > > > > Fair enough. Does it mean Julien can completely ignore the x86 > > requirements for this and focus on something that fit the need of > > architectures where (pseudo-)NMIs can be managed similarly to normal > > interrupts (arm, arm64, sparc...)? > > Yes, focussing on "sane" architectures (by some definition of sane) where > the NMI mode is just changing the delivery restrictions allows to still > differentiate from which source the NMI originates. Let me assume that one can find a way to reliably identify the source of an NMI in x86. If we cannot use the proposed request_nmi() as it does not fit x86, is it acceptable to bypass the existing irq framework and directly program the delivery mode as NMI in the relevant hardware (e.g., a register holding the MSI data)? For instance, in my initial attempt to have the HPET timer to generate NMIs [1]. I could directly write to the FSB Interrupt Route Register. In my view, it makes sense because, as you say, in x86 NMIs are handled separately from the normal vector based interrupts. I guess this would also imply reserving the relevant hardware so that it is not used when calling request_irq(). Thanks and BR, Ricardo [1]. https://lore.kernel.org/patchwork/cover/953394/ ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/4] genirq: Provide basic NMI management for interrupt lines 2018-08-03 3:09 ` Ricardo Neri @ 2018-08-03 7:59 ` Thomas Gleixner 0 siblings, 0 replies; 17+ messages in thread From: Thomas Gleixner @ 2018-08-03 7:59 UTC (permalink / raw) To: Ricardo Neri Cc: Marc Zyngier, Julien Thierry, LKML, Peter Zijlstra, Ingo Molnar On Thu, 2 Aug 2018, Ricardo Neri wrote: > On Thu, Aug 02, 2018 at 11:40:55AM +0200, Thomas Gleixner wrote: > > Yes, focussing on "sane" architectures (by some definition of sane) where > > the NMI mode is just changing the delivery restrictions allows to still > > differentiate from which source the NMI originates. > > Let me assume that one can find a way to reliably identify the source of an > NMI in x86. This assumption is fundamentally wrong. It wont't work unless Intel decides to sanitize the whole exception mechanism. We can discuss that once this happens, but I assume that this will be after my retirement. > If we cannot use the proposed request_nmi() as it does not fit > x86, is it acceptable to bypass the existing irq framework and directly > program the delivery mode as NMI in the relevant hardware (e.g., a register > holding the MSI data)? For instance, in my initial attempt to have the HPET > timer to generate NMIs [1]. I could directly write to the FSB Interrupt > Route Register. In my view, it makes sense because, as you say, in x86 NMIs > are handled separately from the normal vector based interrupts. That HPET thing is a dead horse and won't become more alive by adding magic to the irq core code. > I guess this would also imply reserving the relevant hardware so that it > is not used when calling request_irq(). There is nothing to reserve. Code which needs to deal with NMIs is better written safe and sound and no, we won't expose NMIs to random device driver code either. Thanks, tglx ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/4] genirq: Provide NMI management for percpu_devid interrupts 2018-07-24 11:07 [PATCH 0/4] Provide core API for NMIs Julien Thierry 2018-07-24 11:07 ` [PATCH 1/4] genirq: Provide basic NMI management for interrupt lines Julien Thierry @ 2018-07-24 11:07 ` Julien Thierry 2018-08-01 3:11 ` Ricardo Neri 2018-07-24 11:07 ` [PATCH 3/4] genirq: Provide NMI handlers Julien Thierry 2018-07-24 11:07 ` [PATCH 4/4] irqdesc: Add domain handler for NMIs Julien Thierry 3 siblings, 1 reply; 17+ messages in thread From: Julien Thierry @ 2018-07-24 11:07 UTC (permalink / raw) To: linux-kernel Cc: tglx, peterz, marc.zyngier, ricardo.neri-calderon, mingo, Julien Thierry Add support for percpu_devid interrupts treated as NMIs. Percpu_devid NMIs need to be setup/torn down on each CPU they target. The same restrictions as for global NMIs still apply for percpu_devid NMIs. Signed-off-by: Julien Thierry <julien.thierry@arm.com> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Ingo Molnar <mingo@kernel.org> Cc: Marc Zyngier <marc.zyngier@arm.com> --- include/linux/interrupt.h | 9 +++ kernel/irq/manage.c | 149 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 158 insertions(+) diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h index 3b1a320..59d3877 100644 --- a/include/linux/interrupt.h +++ b/include/linux/interrupt.h @@ -168,10 +168,15 @@ struct irqaction { devname, percpu_dev_id); } +extern int __must_check +request_percpu_nmi(unsigned int irq, irq_handler_t handler, + const char *devname, void __percpu *dev); + extern const void *free_irq(unsigned int, void *); extern void free_percpu_irq(unsigned int, void __percpu *); extern const void *free_nmi(unsigned int irq, void *dev_id); +extern void free_percpu_nmi(unsigned int irq, void __percpu *percpu_dev_id); struct device; @@ -224,7 +229,11 @@ struct irqaction { extern void irq_wake_thread(unsigned int irq, void *dev_id); extern void disable_nmi_nosync(unsigned int irq); +extern void disable_percpu_nmi(unsigned int irq); extern void enable_nmi(unsigned int irq); +extern void enable_percpu_nmi(unsigned int irq, unsigned int type); +extern int ready_percpu_nmi(unsigned int irq); +extern void teardown_percpu_nmi(unsigned int irq); /* The following three functions are for the core kernel use only. */ extern void suspend_device_irqs(void); diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c index 3b87143..ad41c4d 100644 --- a/kernel/irq/manage.c +++ b/kernel/irq/manage.c @@ -2162,6 +2162,11 @@ void enable_percpu_irq(unsigned int irq, unsigned int type) } EXPORT_SYMBOL_GPL(enable_percpu_irq); +void enable_percpu_nmi(unsigned int irq, unsigned int type) +{ + enable_percpu_irq(irq, type); +} + /** * irq_percpu_is_enabled - Check whether the per cpu irq is enabled * @irq: Linux irq number to check for @@ -2201,6 +2206,11 @@ void disable_percpu_irq(unsigned int irq) } EXPORT_SYMBOL_GPL(disable_percpu_irq); +void disable_percpu_nmi(unsigned int irq) +{ + disable_percpu_irq(irq); +} + /* * Internal function to unregister a percpu irqaction. */ @@ -2232,6 +2242,8 @@ static struct irqaction *__free_percpu_irq(unsigned int irq, void __percpu *dev_ /* Found it - now remove it from the list of entries: */ desc->action = NULL; + desc->istate &= ~IRQS_NMI; + raw_spin_unlock_irqrestore(&desc->lock, flags); unregister_handler_proc(irq, action); @@ -2285,6 +2297,19 @@ void free_percpu_irq(unsigned int irq, void __percpu *dev_id) } EXPORT_SYMBOL_GPL(free_percpu_irq); +void free_percpu_nmi(unsigned int irq, void __percpu *dev_id) +{ + struct irq_desc *desc = irq_to_desc(irq); + + if (!desc || !irq_settings_is_per_cpu_devid(desc)) + return; + + if (WARN_ON(!(desc->istate & IRQS_NMI))) + return; + + kfree(__free_percpu_irq(irq, dev_id)); +} + /** * setup_percpu_irq - setup a per-cpu interrupt * @irq: Interrupt line to setup @@ -2375,6 +2400,130 @@ int __request_percpu_irq(unsigned int irq, irq_handler_t handler, EXPORT_SYMBOL_GPL(__request_percpu_irq); /** + * request_percpu_nmi - allocate a percpu interrupt line for NMI delivery + * @irq: Interrupt line to allocate + * @handler: Function to be called when the IRQ occurs. + * @devname: An ascii name for the claiming device + * @dev_id: A percpu cookie passed back to the handler function + * + * This call allocates interrupt resources and enables the + * interrupt on the local CPU. If the interrupt is supposed to be + * enabled on other CPUs, it has to be done on each CPU using + * enable_percpu_nmi(). + * + * Dev_id must be globally unique. It is a per-cpu variable, and + * the handler gets called with the interrupted CPU's instance of + * that variable. + * + * Interrupt lines requested for NMI delivering should have auto enabling + * setting disabled. + * + * If the interrupt line cannot be used to deliver NMIs, function + * will fail returning a negative value. + */ +int request_percpu_nmi(unsigned int irq, irq_handler_t handler, + const char *name, void __percpu *dev_id) +{ + struct irqaction *action; + struct irq_desc *desc; + unsigned long flags; + int retval; + + if (!handler) + return -EINVAL; + + desc = irq_to_desc(irq); + + if (!desc || !irq_settings_can_request(desc) + || !irq_settings_is_per_cpu_devid(desc) + || irq_settings_can_autoenable(desc) + || !irq_supports_nmi(desc)) + return -EINVAL; + + /* The line cannot already be NMI */ + if (desc->istate & IRQS_NMI) + return -EINVAL; + + action = kzalloc(sizeof(struct irqaction), GFP_KERNEL); + if (!action) + return -ENOMEM; + + action->handler = handler; + action->flags = IRQF_PERCPU | IRQF_NO_SUSPEND | IRQF_NO_THREAD + | IRQF_NOBALANCING; + action->name = name; + action->percpu_dev_id = dev_id; + + retval = irq_chip_pm_get(&desc->irq_data); + if (retval < 0) + goto err_out; + + retval = __setup_irq(irq, desc, action); + if (retval) + goto err_irq_setup; + + raw_spin_lock_irqsave(&desc->lock, flags); + desc->istate |= IRQS_NMI; + raw_spin_unlock_irqrestore(&desc->lock, flags); + + return 0; + +err_irq_setup: + irq_chip_pm_put(&desc->irq_data); +err_out: + kfree(action); + + return retval; +} + +int ready_percpu_nmi(unsigned int irq) +{ + unsigned long flags; + struct irq_desc *desc = irq_get_desc_lock(irq, &flags, + IRQ_GET_DESC_CHECK_PERCPU); + int ret = 0; + + if (!desc) { + ret = -EINVAL; + goto out; + } + + if (WARN(!(desc->istate & IRQS_NMI), + KERN_ERR "ready_percpu_nmi called for a non-NMI interrupt: irq %u\n", + irq)) { + ret = -EINVAL; + goto out; + } + + ret = irq_nmi_setup(desc); + if (ret) { + pr_err("Failed to setup NMI delivery: irq %u\n", irq); + goto out; + } + +out: + irq_put_desc_unlock(desc, flags); + return ret; +} + +void teardown_percpu_nmi(unsigned int irq) +{ + unsigned long flags; + struct irq_desc *desc = irq_get_desc_lock(irq, &flags, + IRQ_GET_DESC_CHECK_PERCPU); + + if (!desc) + return; + + if (WARN_ON(!(desc->istate & IRQS_NMI))) + goto out; + + irq_nmi_teardown(desc); +out: + irq_put_desc_unlock(desc, flags); +} + +/** * irq_get_irqchip_state - returns the irqchip state of a interrupt. * @irq: Interrupt line that is forwarded to a VM * @which: One of IRQCHIP_STATE_* the caller wants to know about -- 1.9.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/4] genirq: Provide NMI management for percpu_devid interrupts 2018-07-24 11:07 ` [PATCH 2/4] genirq: Provide NMI management for percpu_devid interrupts Julien Thierry @ 2018-08-01 3:11 ` Ricardo Neri 2018-08-01 15:11 ` Julien Thierry 0 siblings, 1 reply; 17+ messages in thread From: Ricardo Neri @ 2018-08-01 3:11 UTC (permalink / raw) To: Julien Thierry; +Cc: linux-kernel, tglx, peterz, marc.zyngier, mingo On Tue, Jul 24, 2018 at 12:07:05PM +0100, Julien Thierry wrote: > Add support for percpu_devid interrupts treated as NMIs. > > Percpu_devid NMIs need to be setup/torn down on each CPU they target. > > The same restrictions as for global NMIs still apply for percpu_devid NMIs. > > Signed-off-by: Julien Thierry <julien.thierry@arm.com> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Ingo Molnar <mingo@kernel.org> > Cc: Marc Zyngier <marc.zyngier@arm.com> > --- > include/linux/interrupt.h | 9 +++ > kernel/irq/manage.c | 149 ++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 158 insertions(+) > > diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h > index 3b1a320..59d3877 100644 > --- a/include/linux/interrupt.h > +++ b/include/linux/interrupt.h > @@ -168,10 +168,15 @@ struct irqaction { > devname, percpu_dev_id); > } > > +extern int __must_check > +request_percpu_nmi(unsigned int irq, irq_handler_t handler, > + const char *devname, void __percpu *dev); > + > extern const void *free_irq(unsigned int, void *); > extern void free_percpu_irq(unsigned int, void __percpu *); > > extern const void *free_nmi(unsigned int irq, void *dev_id); > +extern void free_percpu_nmi(unsigned int irq, void __percpu *percpu_dev_id); > > struct device; > > @@ -224,7 +229,11 @@ struct irqaction { > extern void irq_wake_thread(unsigned int irq, void *dev_id); > > extern void disable_nmi_nosync(unsigned int irq); > +extern void disable_percpu_nmi(unsigned int irq); Shouldn't this be a disable_percpu_nmi_nosync() as disable_nmi_nosync()? > extern void enable_nmi(unsigned int irq); > +extern void enable_percpu_nmi(unsigned int irq, unsigned int type); > +extern int ready_percpu_nmi(unsigned int irq); > +extern void teardown_percpu_nmi(unsigned int irq); > > /* The following three functions are for the core kernel use only. */ > extern void suspend_device_irqs(void); > diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c > index 3b87143..ad41c4d 100644 > --- a/kernel/irq/manage.c > +++ b/kernel/irq/manage.c > @@ -2162,6 +2162,11 @@ void enable_percpu_irq(unsigned int irq, unsigned int type) > } > EXPORT_SYMBOL_GPL(enable_percpu_irq); > > +void enable_percpu_nmi(unsigned int irq, unsigned int type) > +{ > + enable_percpu_irq(irq, type); > +} > + > /** > * irq_percpu_is_enabled - Check whether the per cpu irq is enabled > * @irq: Linux irq number to check for > @@ -2201,6 +2206,11 @@ void disable_percpu_irq(unsigned int irq) > } > EXPORT_SYMBOL_GPL(disable_percpu_irq); > > +void disable_percpu_nmi(unsigned int irq) > +{ > + disable_percpu_irq(irq); > +} > + > /* > * Internal function to unregister a percpu irqaction. > */ > @@ -2232,6 +2242,8 @@ static struct irqaction *__free_percpu_irq(unsigned int irq, void __percpu *dev_ > /* Found it - now remove it from the list of entries: */ > desc->action = NULL; > > + desc->istate &= ~IRQS_NMI; > + > raw_spin_unlock_irqrestore(&desc->lock, flags); > > unregister_handler_proc(irq, action); > @@ -2285,6 +2297,19 @@ void free_percpu_irq(unsigned int irq, void __percpu *dev_id) > } > EXPORT_SYMBOL_GPL(free_percpu_irq); > > +void free_percpu_nmi(unsigned int irq, void __percpu *dev_id) > +{ > + struct irq_desc *desc = irq_to_desc(irq); > + > + if (!desc || !irq_settings_is_per_cpu_devid(desc)) > + return; > + > + if (WARN_ON(!(desc->istate & IRQS_NMI))) > + return; > + > + kfree(__free_percpu_irq(irq, dev_id)); > +} > + > /** > * setup_percpu_irq - setup a per-cpu interrupt > * @irq: Interrupt line to setup > @@ -2375,6 +2400,130 @@ int __request_percpu_irq(unsigned int irq, irq_handler_t handler, > EXPORT_SYMBOL_GPL(__request_percpu_irq); > > /** > + * request_percpu_nmi - allocate a percpu interrupt line for NMI delivery > + * @irq: Interrupt line to allocate > + * @handler: Function to be called when the IRQ occurs. > + * @devname: An ascii name for the claiming device > + * @dev_id: A percpu cookie passed back to the handler function > + * > + * This call allocates interrupt resources and enables the > + * interrupt on the local CPU. If the interrupt is supposed to be > + * enabled on other CPUs, it has to be done on each CPU using > + * enable_percpu_nmi(). > + * > + * Dev_id must be globally unique. It is a per-cpu variable, and > + * the handler gets called with the interrupted CPU's instance of > + * that variable. > + * > + * Interrupt lines requested for NMI delivering should have auto enabling > + * setting disabled. > + * > + * If the interrupt line cannot be used to deliver NMIs, function > + * will fail returning a negative value. > + */ > +int request_percpu_nmi(unsigned int irq, irq_handler_t handler, > + const char *name, void __percpu *dev_id) > +{ > + struct irqaction *action; > + struct irq_desc *desc; > + unsigned long flags; > + int retval; > + > + if (!handler) > + return -EINVAL; > + > + desc = irq_to_desc(irq); > + > + if (!desc || !irq_settings_can_request(desc) > + || !irq_settings_is_per_cpu_devid(desc) > + || irq_settings_can_autoenable(desc) > + || !irq_supports_nmi(desc)) Shouldn't the logical operators go at the end of the line in a multi-line statement? > + return -EINVAL; > + > + /* The line cannot already be NMI */ > + if (desc->istate & IRQS_NMI) > + return -EINVAL; > + > + action = kzalloc(sizeof(struct irqaction), GFP_KERNEL); > + if (!action) > + return -ENOMEM; > + > + action->handler = handler; > + action->flags = IRQF_PERCPU | IRQF_NO_SUSPEND | IRQF_NO_THREAD > + | IRQF_NOBALANCING; Shouldn't IRQF_NOBALANCING be aligned with IRQF_PERCPU; Thanks and BR, Ricardo ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/4] genirq: Provide NMI management for percpu_devid interrupts 2018-08-01 3:11 ` Ricardo Neri @ 2018-08-01 15:11 ` Julien Thierry 0 siblings, 0 replies; 17+ messages in thread From: Julien Thierry @ 2018-08-01 15:11 UTC (permalink / raw) To: Ricardo Neri; +Cc: linux-kernel, tglx, peterz, marc.zyngier, mingo On 01/08/18 04:11, Ricardo Neri wrote: > On Tue, Jul 24, 2018 at 12:07:05PM +0100, Julien Thierry wrote: >> Add support for percpu_devid interrupts treated as NMIs. >> >> Percpu_devid NMIs need to be setup/torn down on each CPU they target. >> >> The same restrictions as for global NMIs still apply for percpu_devid NMIs. >> >> Signed-off-by: Julien Thierry <julien.thierry@arm.com> >> Cc: Thomas Gleixner <tglx@linutronix.de> >> Cc: Peter Zijlstra <peterz@infradead.org> >> Cc: Ingo Molnar <mingo@kernel.org> >> Cc: Marc Zyngier <marc.zyngier@arm.com> >> --- >> include/linux/interrupt.h | 9 +++ >> kernel/irq/manage.c | 149 ++++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 158 insertions(+) >> >> diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h >> index 3b1a320..59d3877 100644 >> --- a/include/linux/interrupt.h >> +++ b/include/linux/interrupt.h >> @@ -168,10 +168,15 @@ struct irqaction { >> devname, percpu_dev_id); >> } >> >> +extern int __must_check >> +request_percpu_nmi(unsigned int irq, irq_handler_t handler, >> + const char *devname, void __percpu *dev); >> + >> extern const void *free_irq(unsigned int, void *); >> extern void free_percpu_irq(unsigned int, void __percpu *); >> >> extern const void *free_nmi(unsigned int irq, void *dev_id); >> +extern void free_percpu_nmi(unsigned int irq, void __percpu *percpu_dev_id); >> >> struct device; >> >> @@ -224,7 +229,11 @@ struct irqaction { >> extern void irq_wake_thread(unsigned int irq, void *dev_id); >> >> extern void disable_nmi_nosync(unsigned int irq); >> +extern void disable_percpu_nmi(unsigned int irq); > > Shouldn't this be a disable_percpu_nmi_nosync() as disable_nmi_nosync()? > The naming of the disable_percpu_irq didn't include the nosync either (and it doesn't wait for pending percpu_devid_irqs) >> extern void enable_nmi(unsigned int irq); >> +extern void enable_percpu_nmi(unsigned int irq, unsigned int type); >> +extern int ready_percpu_nmi(unsigned int irq); >> +extern void teardown_percpu_nmi(unsigned int irq); >> >> /* The following three functions are for the core kernel use only. */ >> extern void suspend_device_irqs(void); >> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c >> index 3b87143..ad41c4d 100644 >> --- a/kernel/irq/manage.c >> +++ b/kernel/irq/manage.c >> @@ -2162,6 +2162,11 @@ void enable_percpu_irq(unsigned int irq, unsigned int type) >> } >> EXPORT_SYMBOL_GPL(enable_percpu_irq); >> >> +void enable_percpu_nmi(unsigned int irq, unsigned int type) >> +{ >> + enable_percpu_irq(irq, type); >> +} >> + >> /** >> * irq_percpu_is_enabled - Check whether the per cpu irq is enabled >> * @irq: Linux irq number to check for >> @@ -2201,6 +2206,11 @@ void disable_percpu_irq(unsigned int irq) >> } >> EXPORT_SYMBOL_GPL(disable_percpu_irq); >> >> +void disable_percpu_nmi(unsigned int irq) >> +{ >> + disable_percpu_irq(irq); >> +} >> + >> /* >> * Internal function to unregister a percpu irqaction. >> */ >> @@ -2232,6 +2242,8 @@ static struct irqaction *__free_percpu_irq(unsigned int irq, void __percpu *dev_ >> /* Found it - now remove it from the list of entries: */ >> desc->action = NULL; >> >> + desc->istate &= ~IRQS_NMI; >> + >> raw_spin_unlock_irqrestore(&desc->lock, flags); >> >> unregister_handler_proc(irq, action); >> @@ -2285,6 +2297,19 @@ void free_percpu_irq(unsigned int irq, void __percpu *dev_id) >> } >> EXPORT_SYMBOL_GPL(free_percpu_irq); >> >> +void free_percpu_nmi(unsigned int irq, void __percpu *dev_id) >> +{ >> + struct irq_desc *desc = irq_to_desc(irq); >> + >> + if (!desc || !irq_settings_is_per_cpu_devid(desc)) >> + return; >> + >> + if (WARN_ON(!(desc->istate & IRQS_NMI))) >> + return; >> + >> + kfree(__free_percpu_irq(irq, dev_id)); >> +} >> + >> /** >> * setup_percpu_irq - setup a per-cpu interrupt >> * @irq: Interrupt line to setup >> @@ -2375,6 +2400,130 @@ int __request_percpu_irq(unsigned int irq, irq_handler_t handler, >> EXPORT_SYMBOL_GPL(__request_percpu_irq); >> >> /** >> + * request_percpu_nmi - allocate a percpu interrupt line for NMI delivery >> + * @irq: Interrupt line to allocate >> + * @handler: Function to be called when the IRQ occurs. >> + * @devname: An ascii name for the claiming device >> + * @dev_id: A percpu cookie passed back to the handler function >> + * >> + * This call allocates interrupt resources and enables the >> + * interrupt on the local CPU. If the interrupt is supposed to be >> + * enabled on other CPUs, it has to be done on each CPU using >> + * enable_percpu_nmi(). >> + * >> + * Dev_id must be globally unique. It is a per-cpu variable, and >> + * the handler gets called with the interrupted CPU's instance of >> + * that variable. >> + * >> + * Interrupt lines requested for NMI delivering should have auto enabling >> + * setting disabled. >> + * >> + * If the interrupt line cannot be used to deliver NMIs, function >> + * will fail returning a negative value. >> + */ >> +int request_percpu_nmi(unsigned int irq, irq_handler_t handler, >> + const char *name, void __percpu *dev_id) >> +{ >> + struct irqaction *action; >> + struct irq_desc *desc; >> + unsigned long flags; >> + int retval; >> + >> + if (!handler) >> + return -EINVAL; >> + >> + desc = irq_to_desc(irq); >> + >> + if (!desc || !irq_settings_can_request(desc) >> + || !irq_settings_is_per_cpu_devid(desc) >> + || irq_settings_can_autoenable(desc) >> + || !irq_supports_nmi(desc)) > > Shouldn't the logical operators go at the end of the line in a multi-line > statement? Yes, I'll fix that. > >> + return -EINVAL; >> + >> + /* The line cannot already be NMI */ >> + if (desc->istate & IRQS_NMI) >> + return -EINVAL; >> + >> + action = kzalloc(sizeof(struct irqaction), GFP_KERNEL); >> + if (!action) >> + return -ENOMEM; >> + >> + action->handler = handler; >> + action->flags = IRQF_PERCPU | IRQF_NO_SUSPEND | IRQF_NO_THREAD >> + | IRQF_NOBALANCING; > > Shouldn't IRQF_NOBALANCING be aligned with IRQF_PERCPU; > Yes, I'll fix this as well. Thanks, -- Julien Thierry ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 3/4] genirq: Provide NMI handlers 2018-07-24 11:07 [PATCH 0/4] Provide core API for NMIs Julien Thierry 2018-07-24 11:07 ` [PATCH 1/4] genirq: Provide basic NMI management for interrupt lines Julien Thierry 2018-07-24 11:07 ` [PATCH 2/4] genirq: Provide NMI management for percpu_devid interrupts Julien Thierry @ 2018-07-24 11:07 ` Julien Thierry 2018-07-24 11:07 ` [PATCH 4/4] irqdesc: Add domain handler for NMIs Julien Thierry 3 siblings, 0 replies; 17+ messages in thread From: Julien Thierry @ 2018-07-24 11:07 UTC (permalink / raw) To: linux-kernel Cc: tglx, peterz, marc.zyngier, ricardo.neri-calderon, mingo, Julien Thierry Provide flow handlers that are NMI safe for interrupts and percpu_devid interrupts. Signed-off-by: Julien Thierry <julien.thierry@arm.com> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Marc Zyngier <marc.zyngier@arm.com> Cc: Peter Zijlstra <peterz@infradead.org> --- include/linux/irq.h | 3 +++ kernel/irq/chip.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+) diff --git a/include/linux/irq.h b/include/linux/irq.h index 0d71f63..88cbad3 100644 --- a/include/linux/irq.h +++ b/include/linux/irq.h @@ -600,6 +600,9 @@ static inline int irq_set_parent(int irq, int parent_irq) extern void handle_bad_irq(struct irq_desc *desc); extern void handle_nested_irq(unsigned int irq); +extern void handle_fasteoi_nmi(struct irq_desc *desc); +extern void handle_percpu_devid_fasteoi_nmi(struct irq_desc *desc); + extern int irq_chip_compose_msi_msg(struct irq_data *data, struct msi_msg *msg); extern int irq_chip_pm_get(struct irq_data *data); extern int irq_chip_pm_put(struct irq_data *data); diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c index a2b3d9d..c332c17 100644 --- a/kernel/irq/chip.c +++ b/kernel/irq/chip.c @@ -730,6 +730,37 @@ void handle_fasteoi_irq(struct irq_desc *desc) EXPORT_SYMBOL_GPL(handle_fasteoi_irq); /** + * handle_fasteoi_nmi - irq handler for NMI interrupt lines + * @desc: the interrupt description structure for this irq + * + * A simple NMI-safe handler, considering the restrictions + * from request_nmi. + * + * Only a single callback will be issued to the chip: an ->eoi() + * call when the interrupt has been serviced. This enables support + * for modern forms of interrupt handlers, which handle the flow + * details in hardware, transparently. + */ +void handle_fasteoi_nmi(struct irq_desc *desc) +{ + struct irq_chip *chip = irq_desc_get_chip(desc); + struct irqaction *action = desc->action; + unsigned int irq = irq_desc_get_irq(desc); + irqreturn_t res; + + trace_irq_handler_entry(irq, action); + /* + * NMIs cannot be shared, there is only one action. + */ + res = action->handler(irq, action->dev_id); + trace_irq_handler_exit(irq, action, res); + + if (chip->irq_eoi) + chip->irq_eoi(&desc->irq_data); +} +EXPORT_SYMBOL_GPL(handle_fasteoi_nmi); + +/** * handle_edge_irq - edge type IRQ handler * @desc: the interrupt description structure for this irq * @@ -908,6 +939,29 @@ void handle_percpu_devid_irq(struct irq_desc *desc) chip->irq_eoi(&desc->irq_data); } +/** + * handle_percpu_devid_fasteoi_nmi - Per CPU local NMI handler with per cpu + * dev ids + * @desc: the interrupt description structure for this irq + * + * Similar to handle_fasteoi_nmi, but handling the dev_id cookie + * as a percpu pointer. + */ +void handle_percpu_devid_fasteoi_nmi(struct irq_desc *desc) +{ + struct irq_chip *chip = irq_desc_get_chip(desc); + struct irqaction *action = desc->action; + unsigned int irq = irq_desc_get_irq(desc); + irqreturn_t res; + + trace_irq_handler_entry(irq, action); + res = action->handler(irq, raw_cpu_ptr(action->percpu_dev_id)); + trace_irq_handler_exit(irq, action, res); + + if (chip->irq_eoi) + chip->irq_eoi(&desc->irq_data); +} + static void __irq_do_set_handler(struct irq_desc *desc, irq_flow_handler_t handle, int is_chained, const char *name) -- 1.9.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 4/4] irqdesc: Add domain handler for NMIs 2018-07-24 11:07 [PATCH 0/4] Provide core API for NMIs Julien Thierry ` (2 preceding siblings ...) 2018-07-24 11:07 ` [PATCH 3/4] genirq: Provide NMI handlers Julien Thierry @ 2018-07-24 11:07 ` Julien Thierry 3 siblings, 0 replies; 17+ messages in thread From: Julien Thierry @ 2018-07-24 11:07 UTC (permalink / raw) To: linux-kernel Cc: tglx, peterz, marc.zyngier, ricardo.neri-calderon, mingo, Julien Thierry, Will Deacon NMI handling code should be executed between calls to nmi_enter and nmi_exit. Add a separate domain handler to properly setup NMI context when handling an interrupt requested as NMI. Signed-off-by: Julien Thierry <julien.thierry@arm.com> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Marc Zyngier <marc.zyngier@arm.com> Cc: Will Deacon <will.deacon@arm.com> Cc: Peter Zijlstra <peterz@infradead.org> --- include/linux/irqdesc.h | 3 +++ kernel/irq/irqdesc.c | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h index dd1e40d..7c0a243 100644 --- a/include/linux/irqdesc.h +++ b/include/linux/irqdesc.h @@ -171,6 +171,9 @@ static inline int handle_domain_irq(struct irq_domain *domain, { return __handle_domain_irq(domain, hwirq, true, regs); } + +int handle_domain_nmi(struct irq_domain *domain, unsigned int hwirq, + struct pt_regs *regs); #endif /* Test to see if a driver has successfully requested an irq */ diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c index afc7f90..f2b6f9d 100644 --- a/kernel/irq/irqdesc.c +++ b/kernel/irq/irqdesc.c @@ -664,6 +664,39 @@ int __handle_domain_irq(struct irq_domain *domain, unsigned int hwirq, set_irq_regs(old_regs); return ret; } + +/** + * handle_domain_nmi - Invoke the handler for a HW irq belonging to a domain + * @domain: The domain where to perform the lookup + * @hwirq: The HW irq number to convert to a logical one + * @regs: Register file coming from the low-level handling code + * + * Returns: 0 on success, or -EINVAL if conversion has failed + */ +int handle_domain_nmi(struct irq_domain *domain, unsigned int hwirq, + struct pt_regs *regs) +{ + struct pt_regs *old_regs = set_irq_regs(regs); + unsigned int irq; + int ret = 0; + + nmi_enter(); + + irq = irq_find_mapping(domain, hwirq); + + /* + * ack_bad_irq is not NMI-safe, just report + * an invalid interrupt. + */ + if (likely(irq)) + generic_handle_irq(irq); + else + ret = -EINVAL; + + nmi_exit(); + set_irq_regs(old_regs); + return ret; +} #endif /* Dynamic interrupt handling */ -- 1.9.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
end of thread, other threads:[~2018-08-03 7:59 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-07-24 11:07 [PATCH 0/4] Provide core API for NMIs Julien Thierry 2018-07-24 11:07 ` [PATCH 1/4] genirq: Provide basic NMI management for interrupt lines Julien Thierry 2018-08-01 3:07 ` Ricardo Neri 2018-08-01 15:09 ` Julien Thierry 2018-08-01 15:33 ` Joe Perches 2018-08-02 2:03 ` Ricardo Neri 2018-08-02 6:38 ` Marc Zyngier 2018-08-02 6:55 ` Thomas Gleixner 2018-08-02 7:35 ` Marc Zyngier 2018-08-02 9:40 ` Thomas Gleixner 2018-08-03 3:09 ` Ricardo Neri 2018-08-03 7:59 ` Thomas Gleixner 2018-07-24 11:07 ` [PATCH 2/4] genirq: Provide NMI management for percpu_devid interrupts Julien Thierry 2018-08-01 3:11 ` Ricardo Neri 2018-08-01 15:11 ` Julien Thierry 2018-07-24 11:07 ` [PATCH 3/4] genirq: Provide NMI handlers Julien Thierry 2018-07-24 11:07 ` [PATCH 4/4] irqdesc: Add domain handler for NMIs Julien Thierry
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).