KVM & genirq: Enable adaptive IRQ sharing for passed-through devices
Subsystems (5)
v3 cover KVM & genirq: Enable adaptive IRQ sharing for passed-through devices
This addresses the review comments of the previous round:
- renamed irq_data::status to drv_status
- moved drv_status around to unbreak GENERIC_HARDIRQS_NO_DEPRECATED
- fixed signature of get_irq_status (irq is now unsigned int)
- converted register_lock into a global one
- fixed critical white space breakage (that I just left in to check if anyone is actually reading the code, of course...)
Note: The KVM patch still depends on
http://thread.gmane.org/gmane.comp.emulators.kvm.devel/64515
Thanks for all comments!
Final but critical question: Who will pick up which bits?
Jan Kiszka (4):
genirq: Introduce driver-readable IRQ status word
genirq: Inform handler about line sharing state
genirq: Add support for IRQF_COND_ONESHOT
KVM: Allow host IRQ sharing for passed-through PCI 2.3 devices
Documentation/kvm/api.txt | 27 ++++ arch/x86/kvm/x86.c | 1 + include/linux/interrupt.h | 15 ++ include/linux/irq.h | 2 + include/linux/kvm.h | 6 + include/linux/kvm_host.h | 10 ++- kernel/irq/manage.c | 77 ++++++++++- virt/kvm/assigned-dev.c | 336 ++++++++++++++++++++++++++++++++++++++++----- 8 files changed, 436 insertions(+), 38 deletions(-)
PATCH v3 1/4 genirq: Introduce driver-readable IRQ status word
From: Jan Kiszka <[email protected]>
This associates a status word with every IRQ descriptor. Drivers can obtain its content via get_irq_status(irq). First use case will be propagating the interrupt sharing state.
--- include/linux/interrupt.h | 2 ++ include/linux/irq.h | 2 ++ kernel/irq/manage.c | 15 +++++++++++++++ 3 files changed, 19 insertions(+), 0 deletions(-)
include/linux/interrupt.h +2 -0
@@ -126,6 +126,8 @@ struct irqaction {extern irqreturn_t no_action (int cpl,void *dev_id); +extern unsigned long get_irq_status (unsigned int irq); +#ifdef CONFIG_GENERIC_HARDIRQS extern int __must_check request_threaded_irq(unsigned int irq, irq_handler_t handler,
include/linux/irq.h +2 -0
@@ -96,6 +96,7 @@ struct msi_desc;* methods, to allow shared chip implementations * @msi_desc: MSI descriptor * @affinity: IRQ affinity on SMP +* @drv_status: driver-readable status flags (IRQS_*) * * The fields here need to overlay the ones in irq_desc until we * cleaned up the direct references and switched everything over to
@@ -111,6 +112,7 @@ struct irq_data {#ifdef CONFIG_SMP cpumask_var_t affinity;#endif +unsigned long drv_status; };/**
kernel/irq/manage.c +15 -0
@@ -1157,3 +1157,18 @@ int request_any_context_irq(unsigned int irq, irq_handler_t handler,return !ret ?IRQC_IS_HARDIRQ : ret; }EXPORT_SYMBOL_GPL (request_any_context_irq); + +/** +* get_irq_status - read interrupt line status word +* @irq: Interrupt line of the status word +* +* This returns the current content of the status word associated with +* the given interrupt line. See IRQS_* flags for details. +*/ +unsigned long get_irq_status (unsigned int irq) +{ +struct irq_desc *desc =irq_to_desc (irq); + +return desc ? desc->irq_data.drv_status :0 ; +} +EXPORT_SYMBOL_GPL (get_irq_status);
Signature · 1 line
1.7.1
Re: [PATCH v3 1/4] genirq: Introduce driver-readable IRQ status word
Quoted 4 lines · Jan Kiszka
kernel/irq/manage.c +4
-0+/** +* get_irq_status - read interrupt line status word +* @irq: Interrupt line of the status word +*
kernel/irq/manage.c int request_any_context_irq(unsigned int irq, irq_handler_t handler, · line 1174
Quoted 10 lines
kernel/irq/manage.c +10
-0+* This returns the current content of the status word associated with +* the given interrupt line. See IRQS_* flags for details. +*/ +unsigned long get_irq_status (unsigned int irq) +{ +struct irq_desc *desc =irq_to_desc (irq); + +return desc ? desc->irq_data.drv_status :0 ; +} +EXPORT_SYMBOL_GPL (get_irq_status);
We should document that this is a snapshot and in no way serialized against modifications of drv_status. I'll fix up the kernel doc.
Thanks,
tglx
Re: [PATCH v3 1/4] genirq: Introduce driver-readable IRQ status word
Quoted 10 lines · Thomas Gleixner
Quoted 9 lines · Jan Kiszka
kernel/irq/manage.c +9
-0+/** +* get_irq_status - read interrupt line status word +* @irq: Interrupt line of the status word +* +* This returns the current content of the status word associated with +* the given interrupt line. See IRQS_* flags for details. +*/ +unsigned long get_irq_status (unsigned int irq) +{
Quoted 8 lines
kernel/irq/manage.c int request_any_context_irq(unsigned int irq, irq_handler_t handler, · line 1174
Quoted 5 lines
kernel/irq/manage.c +5
-0+struct irq_desc *desc =irq_to_desc (irq); + +return desc ? desc->irq_data.drv_status :0 ; +} +EXPORT_SYMBOL_GPL (get_irq_status);We should document that this is a snapshot and in no way serialized against modifications of
drv_status. I'll fix up the kernel doc.
Yeah, I think I had some hint on this in the previous version but apparently dropped it for this round.
Thanks,
Jan
PATCH v3 2/4 genirq: Inform handler about line sharing state
In reply to Jan KiszkaFrom: Jan Kiszka <[email protected]>
This enabled interrupt handlers to retrieve the current line sharing state via the new interrupt status word so that they can adapt to it.
The switch from shared to exclusive is generally uncritical and can thus be performed on demand. However, preparing a line for shared mode may require preparational steps of the currently registered handler. It can therefore request an ahead-of-time notification via IRQF_ADAPTIVE. The notification consists of an exceptional handler invocation with IRQS_MAKE_SHAREABLE set in the status word.
--- include/linux/interrupt.h | 10 +++++++++ kernel/irq/manage.c | 47 ++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 54 insertions(+), 3 deletions(-)
include/linux/interrupt.h +10 -0
@@ -55,6 +55,7 @@* Used by threaded interrupts which need to keep the * irq line disabled until the threaded handler has been run. * IRQF_NO_SUSPEND - Do not disable this IRQ during suspend +* IRQF_ADAPTIVE - Request notification about upcoming interrupt line sharing * */ #define IRQF_DISABLED 0x00000020
@@ -67,6 +68,7 @@#define IRQF_IRQPOLL 0x00001000#define IRQF_ONESHOT 0x00002000#define IRQF_NO_SUSPEND 0x00004000 +#define IRQF_ADAPTIVE 0x00008000#define IRQF_TIMER (__IRQF_TIMER | IRQF_NO_SUSPEND)
@@ -126,6 +128,14 @@ struct irqaction {extern irqreturn_t no_action (int cpl,void *dev_id); +/* +* Driver-readable IRQ line status flags: +* IRQS_SHARED - line is shared between multiple handlers +* IRQS_MAKE_SHAREABLE - in the process of making an exclusive line shareable +*/ +#define IRQS_SHARED 0x00000001 +#define IRQS_MAKE_SHAREABLE 0x00000002 +extern unsigned long get_irq_status (unsigned int irq);#ifdef CONFIG_GENERIC_HARDIRQS
kernel/irq/manage.c +44 -3
106 lines not on this page · raw message
Signature · 1 line
1.7.1
Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state
kernel/irq/manage.c static struct irqaction *__free_irq(unsigned int irq, void *dev_id) · line 955
Quoted 7 lines · Jan Kiszka
kernel/irq/manage.c +3
-0@@ -943,6 +950,9 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)/* Make sure it's not being used on another CPU: */ synchronize_irq (irq); +if (single_handler) + desc->irq_data.drv_status &= ~IRQS_SHARED ; +
What's the reason to clear this flag outside of the desc->lock held region. I need this status for other purposes as well, where I definitely need serialization.
Quoted 4 lines
kernel/irq/manage.c +4
-0+mutex_lock (®ister_lock); + + old_action = desc->action; + if (old_action && (old_action->flags & IRQF_ADAPTIVE) &&
kernel/irq/manage.c int request_threaded_irq(unsigned int irq, irq_handler_t handler, · line 1121
Quoted 10 lines
kernel/irq/manage.c +10
-0+ !(desc->irq_data.drv_status & IRQS_SHARED)) { +/* +* Signal the old handler that is has to switch to shareable +* handling mode. Disable the line to avoid any conflict with +* a real IRQ. +*/ +disable_irq (irq); +local_irq_disable (); + + desc->irq_data.drv_status |=IRQS_SHARED |IRQS_MAKE_SHAREABLE ;
Unserialized access as well. Will think about it.
kernel/irq/manage.c int request_threaded_irq(unsigned int irq, irq_handler_t handler, · line 1123
Quoted 2 lines
kernel/irq/manage.c +2
-0+ old_action->handler (irq, old_action->dev_id); + desc->irq_data.drv_status &= ~IRQS_MAKE_SHAREABLE ;
Thanks,
tglx
Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state
Quoted 11 lines · Thomas Gleixner
kernel/irq/manage.c static struct irqaction *__free_irq(unsigned int irq, void *dev_id) · line 955
Quoted 7 lines · Jan Kiszka
kernel/irq/manage.c +3
-0@@ -943,6 +950,9 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)/* Make sure it's not being used on another CPU: */ synchronize_irq (irq); +if (single_handler) + desc->irq_data.drv_status &= ~IRQS_SHARED ; +What's the reason to clear this flag outside of the
desc->lockheld region.
We need to synchronize the irq first before clearing the flag.
The problematic scenario behind this: An IRQ started in shared mode, this the line was unmasked after the hardirq. Now we clear IRQS_SHARED before calling into the threaded handler. And that handler may now think that the line is still masked as IRQS_SHARED is set.
Quoted 2 lines
I need this status for other purposes as well, where I definitely need serialization.
Well, two options: wrap all bit manipulations with desc->lock acquisition/release or turn drv_status into an atomic. I don't know what your plans with drv_status are, so...
Quoted 18 lines
Quoted 5 lines
kernel/irq/manage.c +4
-0+mutex_lock (®ister_lock); + + old_action = desc->action; + if (old_action && (old_action->flags & IRQF_ADAPTIVE) &&kernel/irq/manage.c int request_threaded_irq(unsigned int irq, irq_handler_t handler, · line 1121
Quoted 10 lines
kernel/irq/manage.c +10
-0+ !(desc->irq_data.drv_status & IRQS_SHARED)) { +/* +* Signal the old handler that is has to switch to shareable +* handling mode. Disable the line to avoid any conflict with +* a real IRQ. +*/ +disable_irq (irq); +local_irq_disable (); + + desc->irq_data.drv_status |=IRQS_SHARED |IRQS_MAKE_SHAREABLE ;Unserialized access as well. Will think about it.
Quoted 6 lines
kernel/irq/manage.c int request_threaded_irq(unsigned int irq, irq_handler_t handler, · line 1123
Quoted 2 lines
kernel/irq/manage.c +2
-0+ old_action->handler (irq, old_action->dev_id); + desc->irq_data.drv_status &= ~IRQS_MAKE_SHAREABLE ;Thanks,
tglx
Jan
Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state
Quoted 13 lines · Jan Kiszka
Quoted 11 lines · Thomas Gleixner
kernel/irq/manage.c static struct irqaction *__free_irq(unsigned int irq, void *dev_id) · line 955
Quoted 7 lines · Jan Kiszka
kernel/irq/manage.c +3
-0@@ -943,6 +950,9 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)/* Make sure it's not being used on another CPU: */ synchronize_irq (irq); +if (single_handler) + desc->irq_data.drv_status &= ~IRQS_SHARED ; +What's the reason to clear this flag outside of the
desc->lockheld region.
Quoted 6 lines
We need to synchronize the irq first before clearing the flag.
The problematic scenario behind this: An IRQ started in shared mode, this the line was unmasked after the hardirq. Now we clear
IRQS_SHAREDbefore calling into the threaded handler. And that handler may now think that the line is still masked asIRQS_SHAREDis set.
That should read "not set" I guess. Hmm, needs more thoughts :(
Quoted 6 lines
Quoted 2 lines
I need this status for other purposes as well, where I definitely need serialization.
Well, two options: wrap all bit manipulations with
desc->lockacquisition/release or turndrv_statusinto an atomic. I don't know what your plans withdrv_statusare, so...
Some bits for irq migration and other stuff, which allows us to avoid fiddling with irqdesc in the drivers.
Thanks,
tglx
Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state
Quoted 14 lines · Thomas Gleixner
Quoted 13 lines · Jan Kiszka
Quoted 11 lines · Thomas Gleixner
kernel/irq/manage.c static struct irqaction *__free_irq(unsigned int irq, void *dev_id) · line 955
Quoted 7 lines · Jan Kiszka
kernel/irq/manage.c +3
-0@@ -943,6 +950,9 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)/* Make sure it's not being used on another CPU: */ synchronize_irq (irq); +if (single_handler) + desc->irq_data.drv_status &= ~IRQS_SHARED ; +What's the reason to clear this flag outside of the
desc->lockheld region.
Quoted 8 lines
Quoted 6 lines
We need to synchronize the irq first before clearing the flag.
The problematic scenario behind this: An IRQ started in shared mode, this the line was unmasked after the hardirq. Now we clear
IRQS_SHAREDbefore calling into the threaded handler. And that handler may now think that the line is still masked asIRQS_SHAREDis set.That should read "not set" I guess.
Can't remember who wrote this, but that guy might have been too tired for clear sentences: Yes, of course, we could run into troubles, if IRQS_SHARED was _not_ set while the IRQ line is unmasked between hard and threaded handler.
Quoted 1 line
Hmm, needs more thoughts :(
Be warned, might be painful.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state
Quoted 22 lines · Jan Kiszka
Quoted 21 lines · Thomas Gleixner
Quoted 19 lines · Jan Kiszka
Quoted 11 lines · Thomas Gleixner
kernel/irq/manage.c static struct irqaction *__free_irq(unsigned int irq, void *dev_id) · line 955
Quoted 7 lines · Jan Kiszka
kernel/irq/manage.c +3
-0@@ -943,6 +950,9 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)/* Make sure it's not being used on another CPU: */ synchronize_irq (irq); +if (single_handler) + desc->irq_data.drv_status &= ~IRQS_SHARED ; +What's the reason to clear this flag outside of the
desc->lockheld region.We need to synchronize the irq first before clearing the flag.
The problematic scenario behind this: An IRQ started in shared mode, this the line was unmasked after the hardirq. Now we clear
IRQS_SHAREDbefore calling into the threaded handler. And that handler may now think that the line is still masked asIRQS_SHAREDis set.
Quoted 6 lines
Quoted 1 line
That should read "not set" I guess.
Can't remember who wrote this, but that guy might have been too tired for clear sentences: Yes, of course, we could run into troubles, if
IRQS_SHAREDwas _not_ set while the IRQ line is unmasked between hard and threaded handler.
Right.
As a side note, the current implementation requires that you lookup irq_data.drv_status for every invocation of the handler or have a reference to irq_data.drv_status somewhere locally, which I don't like either.
I have an evil and nasy idea how to avoid that, need to look how ugly it gets. Worst case we need to go back to that notification thing which I wanted really avoid in the first place.
Though I like the register_mutex idea which came out of this a lot as it allows us to reduce desc->lock held and interrupt disabled regions quite nicely.
/me goes back to stare at the code
Quoted 3 lines
Quoted 1 line
Hmm, needs more thoughts :(
Be warned, might be painful.
Bah, my brain became pain resistant when I started hacking that code.
Thanks,
tglx
Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state
In reply to Jan KiszkaQuoted 24 lines · Jan Kiszka
Quoted 14 lines · Thomas Gleixner
Quoted 13 lines · Jan Kiszka
Quoted 11 lines · Thomas Gleixner
kernel/irq/manage.c static struct irqaction *__free_irq(unsigned int irq, void *dev_id) · line 955
Quoted 7 lines · Jan Kiszka
kernel/irq/manage.c +3
-0@@ -943,6 +950,9 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)/* Make sure it's not being used on another CPU: */ synchronize_irq (irq); +if (single_handler) + desc->irq_data.drv_status &= ~IRQS_SHARED ; +What's the reason to clear this flag outside of the
desc->lockheld region.Quoted 8 lines
Quoted 6 lines
We need to synchronize the irq first before clearing the flag.
The problematic scenario behind this: An IRQ started in shared mode, this the line was unmasked after the hardirq. Now we clear
IRQS_SHAREDbefore calling into the threaded handler. And that handler may now think that the line is still masked asIRQS_SHAREDis set.That should read "not set" I guess.
Quoted 8 lines
Can't remember who wrote this, but that guy might have been too tired for clear sentences: Yes, of course, we could run into troubles, if
IRQS_SHAREDwas _not_ set while the IRQ line is unmasked between hard and threaded handler.Quoted 1 line
Hmm, needs more thoughts :(
Be warned, might be painful.
Talking about headache. Your solution above does not prevent that scenario.
CPU 0 CPU 1
synchronize_irq();
hard irq comes in sees shared and unmasks
clear IRQS_SHARED
thread handler runs and sees !SHARED
Same scenario, just moved by a few lines :)
Thanks,
tglx
Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state
Quoted 35 lines · Thomas Gleixner
Quoted 24 lines · Jan Kiszka
Quoted 14 lines · Thomas Gleixner
Quoted 13 lines · Jan Kiszka
Quoted 11 lines · Thomas Gleixner
kernel/irq/manage.c static struct irqaction *__free_irq(unsigned int irq, void *dev_id) · line 955
Quoted 7 lines · Jan Kiszka
kernel/irq/manage.c +3
-0@@ -943,6 +950,9 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)/* Make sure it's not being used on another CPU: */ synchronize_irq (irq); +if (single_handler) + desc->irq_data.drv_status &= ~IRQS_SHARED ; +What's the reason to clear this flag outside of the
desc->lockheld region.Quoted 8 lines
Quoted 6 lines
We need to synchronize the irq first before clearing the flag.
The problematic scenario behind this: An IRQ started in shared mode, this the line was unmasked after the hardirq. Now we clear
IRQS_SHAREDbefore calling into the threaded handler. And that handler may now think that the line is still masked asIRQS_SHAREDis set.That should read "not set" I guess.
Quoted 8 lines
Can't remember who wrote this, but that guy might have been too tired for clear sentences: Yes, of course, we could run into troubles, if
IRQS_SHAREDwas _not_ set while the IRQ line is unmasked between hard and threaded handler.Quoted 1 line
Hmm, needs more thoughts :(
Be warned, might be painful.
Quoted 7 lines
Talking about headache. Your solution above does not prevent that scenario.
CPU 0 CPU 1
synchronize_irq();
hard irq comes in sees shared and unmasks
Nope, IRQ_ONESHOT is already cleared at that point.
Quoted 4 lines
clear
IRQS_SHARED
thread handler runs and sees !SHAREDSame scenario, just moved by a few lines :)
The same, just the other way around - and mostly harmless, I hope. :)
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state
Quoted 10 lines · Jan Kiszka
Quoted 7 lines · Thomas Gleixner
Talking about headache. Your solution above does not prevent that scenario.
CPU 0 CPU 1
synchronize_irq();
hard irq comes in sees shared and unmasksNope,
IRQ_ONESHOTis already cleared at that point.
Errm ? It's set. Could you please stop to increase my confusion ? :)
Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state
In reply to Jan Kiszkakernel/irq/manage.c int request_threaded_irq(unsigned int irq, irq_handler_t handler, · line 1136
Quoted 9 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
kernel/irq/manage.c +3
-1chip_bus_lock (desc); retval =__setup_irq (irq, desc, action);chip_bus_sync_unlock (desc); -if (retval) +if (retval) { +if (desc->action && !desc->action->next) + desc->irq_data.drv_status &= ~IRQS_SHARED ;
This is redundant. IRQS_SHARED gets set in a code path where all checks are done already.
To make that more obvious we can set it right before
raw_spin_unlock_irqrestore(&desc->lock, flags);
conditionally on (shared).
That way we can also move the kfree out of the mutex locked section.
Thanks,
tglx
Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state
Quoted 6 lines · Thomas Gleixner
Quoted 5 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
kernel/irq/manage.c +0
-0chip_bus_lock (desc); retval =__setup_irq (irq, desc, action);chip_bus_sync_unlock (desc);
Quoted 7 lines
kernel/irq/manage.c int request_threaded_irq(unsigned int irq, irq_handler_t handler, · line 1136
Quoted 4 lines
kernel/irq/manage.c +3
-1-if (retval) +if (retval) { +if (desc->action && !desc->action->next) + desc->irq_data.drv_status &= ~IRQS_SHARED ;This is redundant.
IRQS_SHAREDgets set in a code path where all checks are done already.
Nope, it's also set before entry of __setup_irq in case we call an IRQF_ADAPTIVE handler.
We need to set it that early as we may race with IRQ events for the already registered handler happening between the sharing notification and the actual registration of the second handler.
Jan
Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state
Quoted 15 lines · Jan Kiszka
Quoted 6 lines · Thomas Gleixner
Quoted 5 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
kernel/irq/manage.c +0
-0chip_bus_lock (desc); retval =__setup_irq (irq, desc, action);chip_bus_sync_unlock (desc);Quoted 7 lines
kernel/irq/manage.c int request_threaded_irq(unsigned int irq, irq_handler_t handler, · line 1136
Quoted 4 lines
kernel/irq/manage.c +3
-1-if (retval) +if (retval) { +if (desc->action && !desc->action->next) + desc->irq_data.drv_status &= ~IRQS_SHARED ;This is redundant.
IRQS_SHAREDgets set in a code path where all checks are done already.
Quoted 6 lines
Nope, it's also set before entry of
__setup_irqin case we call anIRQF_ADAPTIVEhandler.We need to set it that early as we may race with IRQ events for the already registered handler happening between the sharing notification and the actual registration of the second handler.
Hmm, ok. Though the MAKE_SHAREABLE flag should be sufficient to do the notification.
Thanks,
tglx
Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state
Quoted 20 lines · Thomas Gleixner
Quoted 18 lines · Jan Kiszka
Quoted 6 lines · Thomas Gleixner
Quoted 5 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
kernel/irq/manage.c +0
-0chip_bus_lock (desc); retval =__setup_irq (irq, desc, action);chip_bus_sync_unlock (desc);Quoted 7 lines
kernel/irq/manage.c int request_threaded_irq(unsigned int irq, irq_handler_t handler, · line 1136
Quoted 4 lines
kernel/irq/manage.c +3
-1-if (retval) +if (retval) { +if (desc->action && !desc->action->next) + desc->irq_data.drv_status &= ~IRQS_SHARED ;This is redundant.
IRQS_SHAREDgets set in a code path where all checks are done already.Nope, it's also set before entry of
__setup_irqin case we call anIRQF_ADAPTIVEhandler.
Quoted 6 lines
Quoted 3 lines
We need to set it that early as we may race with IRQ events for the already registered handler happening between the sharing notification and the actual registration of the second handler.
Hmm, ok. Though the
MAKE_SHAREABLEflag should be sufficient to do the notification.
For notification, yes. But we need SHARED once we reenable the line after the notification.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state
Quoted 25 lines · Jan Kiszka
Quoted 24 lines · Thomas Gleixner
Quoted 21 lines · Jan Kiszka
Quoted 6 lines · Thomas Gleixner
Quoted 5 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
kernel/irq/manage.c +0
-0chip_bus_lock (desc); retval =__setup_irq (irq, desc, action);chip_bus_sync_unlock (desc);Quoted 7 lines
kernel/irq/manage.c int request_threaded_irq(unsigned int irq, irq_handler_t handler, · line 1136
Quoted 4 lines
kernel/irq/manage.c +3
-1-if (retval) +if (retval) { +if (desc->action && !desc->action->next) + desc->irq_data.drv_status &= ~IRQS_SHARED ;This is redundant.
IRQS_SHAREDgets set in a code path where all checks are done already.Nope, it's also set before entry of
__setup_irqin case we call anIRQF_ADAPTIVEhandler.We need to set it that early as we may race with IRQ events for the already registered handler happening between the sharing notification and the actual registration of the second handler.
Quoted 5 lines
Quoted 2 lines
Hmm, ok. Though the
MAKE_SHAREABLEflag should be sufficient to do the notification.For notification, yes. But we need SHARED once we reenable the line after the notification.
Darn. Will think more about it.
Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state
In reply to Jan Kiszkakernel/irq/manage.c int request_threaded_irq(unsigned int irq, irq_handler_t handler, · line 1118
Quoted 8 lines · Jan Kiszka
kernel/irq/manage.c +8
-0+if (old_action && (old_action->flags &IRQF_ADAPTIVE ) && + !(desc->irq_data.drv_status &IRQS_SHARED )) { +/* +* Signal the old handler that is has to switch to shareable +* handling mode. Disable the line to avoid any conflict with +* a real IRQ. +*/ +disable_irq (irq);
This is weird, really. I thought you wanted to avoid waiting for the threaded handler to finish if it's on the fly. So this should be disable_irq_nosync() or did you change your mind ?
Thanks,
tglx
Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state
Quoted 13 lines · Thomas Gleixner
kernel/irq/manage.c int request_threaded_irq(unsigned int irq, irq_handler_t handler, · line 1118
Quoted 8 lines · Jan Kiszka
kernel/irq/manage.c +8
-0+if (old_action && (old_action->flags &IRQF_ADAPTIVE ) && + !(desc->irq_data.drv_status &IRQS_SHARED )) { +/* +* Signal the old handler that is has to switch to shareable +* handling mode. Disable the line to avoid any conflict with +* a real IRQ. +*/ +disable_irq (irq);This is weird, really. I thought you wanted to avoid waiting for the threaded handler to finish if it's on the fly. So this should be
disable_irq_nosync()or did you change your mind ?
No, I did not. I wanted to avoid that we set MAKE_SHAREABLE while there might be another IRQ in flight. The handler that is called due to a real IRQ might misinterpret MAKE_SHAREABLE as "there is no real event" and perform the wrong steps (at least the current implementation for KVM would).
However, I will rebase my patch over your series now and try to re-think this. The question is what could go wrong if we do not guarantee that MAKE_SHAREABLE and ordinary IRQ will always be distinguishable. If there is really nothing, specifically for the KVM scenario, we could even drop the disable/enable_irq. That would be also be nicer when thinking about potential delays of the already registered handler during this transitional phase.
Jan
change of email address: [email protected] -> [email protected]
eom
Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state
In reply to Jan KiszkaQuoted 15 lines · Jan Kiszka
Quoted 13 lines · Thomas Gleixner
kernel/irq/manage.c int request_threaded_irq(unsigned int irq, irq_handler_t handler, · line 1118
Quoted 8 lines · Jan Kiszka
kernel/irq/manage.c +8
-0+if (old_action && (old_action->flags &IRQF_ADAPTIVE ) && + !(desc->irq_data.drv_status &IRQS_SHARED )) { +/* +* Signal the old handler that is has to switch to shareable +* handling mode. Disable the line to avoid any conflict with +* a real IRQ. +*/ +disable_irq (irq);This is weird, really. I thought you wanted to avoid waiting for the threaded handler to finish if it's on the fly. So this should be
disable_irq_nosync()or did you change your mind ?
Quoted 4 lines
No, I did not. I wanted to avoid that we set
MAKE_SHAREABLEwhile there might be another IRQ in flight. The handler that is called due to a real IRQ might misinterpretMAKE_SHAREABLEas "there is no real event" and perform the wrong steps (at least the current implementation for KVM would).
Actually, the requirement we have to fulfill here is to avoid that the hardirq handler sees !SHARED while the threaded one reads "SHARED". To achieve this without disabling the line, I'm still searching for a way to couple the sharing state of associated hard and threaded handler runs
- but I think there is no reliable association, is there?
Jan
Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state
Quoted 21 lines · Jan Kiszka
Quoted 15 lines · Jan Kiszka
Quoted 13 lines · Thomas Gleixner
kernel/irq/manage.c int request_threaded_irq(unsigned int irq, irq_handler_t handler, · line 1118
Quoted 8 lines · Jan Kiszka
kernel/irq/manage.c +8
-0+if (old_action && (old_action->flags &IRQF_ADAPTIVE ) && + !(desc->irq_data.drv_status &IRQS_SHARED )) { +/* +* Signal the old handler that is has to switch to shareable +* handling mode. Disable the line to avoid any conflict with +* a real IRQ. +*/ +disable_irq (irq);This is weird, really. I thought you wanted to avoid waiting for the threaded handler to finish if it's on the fly. So this should be
disable_irq_nosync()or did you change your mind ?Quoted 4 lines
No, I did not. I wanted to avoid that we set
MAKE_SHAREABLEwhile there might be another IRQ in flight. The handler that is called due to a real IRQ might misinterpretMAKE_SHAREABLEas "there is no real event" and perform the wrong steps (at least the current implementation for KVM would).
Quoted 5 lines
Actually, the requirement we have to fulfill here is to avoid that the hardirq handler sees !SHARED while the threaded one reads "SHARED". To achieve this without disabling the line, I'm still searching for a way to couple the sharing state of associated hard and threaded handler runs
- but I think there is no reliable association, is there?
Unfortunately not. So the only way to solve that is disabling the interrupt which makes sure that all handlers have completed.
OTOH, if we have to disable anyway, then we could simply keep it disabled across the installation of a new handler. That would make the notification business go away, wouldn't it ?
Thanks,
tglx
Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state
Quoted 22 lines · Thomas Gleixner
Quoted 21 lines · Jan Kiszka
Quoted 19 lines · Jan Kiszka
Quoted 13 lines · Thomas Gleixner
kernel/irq/manage.c int request_threaded_irq(unsigned int irq, irq_handler_t handler, · line 1118
Quoted 8 lines · Jan Kiszka
kernel/irq/manage.c +8
-0+if (old_action && (old_action->flags &IRQF_ADAPTIVE ) && + !(desc->irq_data.drv_status &IRQS_SHARED )) { +/* +* Signal the old handler that is has to switch to shareable +* handling mode. Disable the line to avoid any conflict with +* a real IRQ. +*/ +disable_irq (irq);This is weird, really. I thought you wanted to avoid waiting for the threaded handler to finish if it's on the fly. So this should be
disable_irq_nosync()or did you change your mind ?No, I did not. I wanted to avoid that we set
MAKE_SHAREABLEwhile there might be another IRQ in flight. The handler that is called due to a real IRQ might misinterpretMAKE_SHAREABLEas "there is no real event" and perform the wrong steps (at least the current implementation for KVM would).
Quoted 8 lines
Quoted 5 lines
Actually, the requirement we have to fulfill here is to avoid that the hardirq handler sees !SHARED while the threaded one reads "SHARED". To achieve this without disabling the line, I'm still searching for a way to couple the sharing state of associated hard and threaded handler runs
- but I think there is no reliable association, is there?Unfortunately not. So the only way to solve that is disabling the interrupt which makes sure that all handlers have completed.
Hmm, what a pity.
Quoted 4 lines
OTOH, if we have to disable anyway, then we could simply keep it disabled across the installation of a new handler. That would make the notification business go away, wouldn't it ?
No, the notification is still necessary in case the registered handler keeps the line off after returning from both hard and threaded handler.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state
Quoted 7 lines · Jan Kiszka
Quoted 3 lines · Thomas Gleixner
OTOH, if we have to disable anyway, then we could simply keep it disabled across the installation of a new handler. That would make the notification business go away, wouldn't it ?
No, the notification is still necessary in case the registered handler keeps the line off after returning from both hard and threaded handler.
And how should that happen? If it is in oneshot mode then the line is reenabled when the thread handler returns.
Thanks,
tglx
Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state
Quoted 11 lines · Thomas Gleixner
Quoted 7 lines · Jan Kiszka
Quoted 3 lines · Thomas Gleixner
OTOH, if we have to disable anyway, then we could simply keep it disabled across the installation of a new handler. That would make the notification business go away, wouldn't it ?
No, the notification is still necessary in case the registered handler keeps the line off after returning from both hard and threaded handler.
And how should that happen? If it is in oneshot mode then the line is reenabled when the thread handler returns.
disable_irq_nosync is called by the handler before returning. And it's the handler's job to revert this, properly synchronizing it internally.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state
Quoted 7 lines · Jan Kiszka
Quoted 6 lines · Thomas Gleixner
Quoted 5 lines · Jan Kiszka
Quoted 3 lines · Thomas Gleixner
OTOH, if we have to disable anyway, then we could simply keep it disabled across the installation of a new handler. That would make the notification business go away, wouldn't it ?
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
No, the notification is still necessary in case the registered handler keeps the line off after returning from both hard and threaded handler.
And how should that happen? If it is in oneshot mode then the line is reenabled when the thread handler returns.
disable_irq_nosyncis called by the handler before returning. And it's the handler's job to revert this, properly synchronizing it internally.
disable_irq_nosync() is really the worst thing to do. That's simply not going to work without a lot of fuglyness.
What about the following:
primary_handler(....)
{
if (!shared)
return IRQ_WAKE_THREAD;
spin_lock(dev->irq_lock);
if (from_my_device() || dev->irq_thread_waiting) {
mask_dev();
dev->masked = true;
ret = IRQ_WAKE_THREAD;
} else
ret = IRQ_NONE;
spin_unlock(dev->irq_lock);
return ret;
}check_timeout()
{
if (dev->irq_active && wait_longer())
return IRQ_WAKE_THREAD;
return IRQ_HANDLED;
}unmask_dev_if_necessary()
{
if (dev->masked && dev->irq_active)
umask_dev();
}threaded_handler(....)
{
if (!dev->irq_thread_waiting) {
spin_lock_irq(dev->irq_lock);
wake_user = do_magic_stuff_with_the_dev();
dev->irq_thread_waiting = wake_user;
spin_unlock(dev->irq_lock);
if (wake_user)
wake_up(user);
}
if (!dev->irq_thread_waiting) {
spin_lock_irq(dev->irq_lock);
unmask_dev_if_necessary();
spin_unlock(dev->irq_lock);
return IRQ_HANDLED;
}
/*
* Wait for user space to complete. Timeout is to
* avoid starvation of the irq line when
* something goes wrong
*/
wait_for_completion_timeout(dev->compl, SENSIBLE_TIMEOUT);
spin_lock_irq(dev->irq_lock);
if (timedout) {
mask_dev();
dev->masked = true;
/*
* Leave dev->irq_thread_waiting untouched and let
* the core code reschedule us when check_timeout
* decides it's worth to wait. In any case we leave
* the device masked at the device level, so we don't
* cause an interrupt storm.
*/
ret = check_timeout();
} else {
unmask_dev_if_necessary();
dev->irq_thread_waiting = false;
ret = IRQ_HANDLED;
}
spin_unlock(dev->irq_lock);
return ret;
}userspace_complete()
{
complete(dev->irq_compl);
}Your aproach with disable_irq_nosync() is completely flawed, simply because you try to pretend that your interrupt handler is done, while it is not done at all. The threaded interrupt handler is done when user space completes. Everything else is just hacking around the problem and creates all that nasty transitional problems.
The above code does not have them at all. The threaded handler does not care at all about the dev_id shared state encoding and the state transitions. It merily cares about the device internal status dev->masked. Everything else is handled by the genirq code and the litte status check in the primary handler.
Neither does the user space completion care about it. It just completes and is completely oblivious of the irq line state/mode. And really, the user space part should not care at all. It can set some status before calling complete(), but that's driver specific stuff and has nothing to do with the irq handling magic.
It requires a few not too intrusive modifications to the genirq code:
- Rescheduling the thread handler on IRQ_WAKE_THREAD
- Changing the oneshot finalizing a bit
- Adding the status manipulations for request/free_irq
Now you might argue that the timeout is ugly, but I don't think it's ugly at all. You need it anyway in case user space failed completely. And coming back after 100ms to let the genirq code handle a disable_irq() or synchronize_irq() is a reasonable request, it's the error/corner case at all. If there is code which installs/removes an interrupt handler on the same line every 5ms, then this code becomes rightfully blocked out for 100ms or such.
Thanks,
tglx
Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state
Quoted 105 lines · Thomas Gleixner
Quoted 7 lines · Jan Kiszka
Quoted 6 lines · Thomas Gleixner
Quoted 5 lines · Jan Kiszka
Quoted 3 lines · Thomas Gleixner
OTOH, if we have to disable anyway, then we could simply keep it disabled across the installation of a new handler. That would make the notification business go away, wouldn't it ?
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
No, the notification is still necessary in case the registered handler keeps the line off after returning from both hard and threaded handler.
And how should that happen? If it is in oneshot mode then the line is reenabled when the thread handler returns.
disable_irq_nosyncis called by the handler before returning. And it's the handler's job to revert this, properly synchronizing it internally.
disable_irq_nosync()is really the worst thing to do. That's simply not going to work without a lot of fuglyness.What about the following:
primary_handler(....){ if (!shared) return IRQ_WAKE_THREAD; spin_lock(dev->irq_lock); if (from_my_device() || dev->irq_thread_waiting) { mask_dev(); dev->masked = true; ret = IRQ_WAKE_THREAD; } else ret = IRQ_NONE; spin_unlock(dev->irq_lock); return ret; }
check_timeout(){ if (dev->irq_active && wait_longer()) return IRQ_WAKE_THREAD; return IRQ_HANDLED; }
unmask_dev_if_necessary(){ if (dev->masked && dev->irq_active) umask_dev(); }
threaded_handler(....){ if (!dev->irq_thread_waiting) { spin_lock_irq(dev->irq_lock); wake_user = do_magic_stuff_with_the_dev(); dev->irq_thread_waiting = wake_user; spin_unlock(dev->irq_lock); if (wake_user) wake_up(user); } if (!dev->irq_thread_waiting) { spin_lock_irq(dev->irq_lock); unmask_dev_if_necessary(); spin_unlock(dev->irq_lock); return IRQ_HANDLED; } /* * Wait for user space to complete. Timeout is to * avoid starvation of the irq line when * something goes wrong */ wait_for_completion_timeout(dev->compl, SENSIBLE_TIMEOUT); spin_lock_irq(dev->irq_lock); if (timedout) { mask_dev(); dev->masked = true; /* * Leave dev->irq_thread_waiting untouched and let * the core code reschedule us when check_timeout * decides it's worth to wait. In any case we leave * the device masked at the device level, so we don't * cause an interrupt storm. */ ret = check_timeout(); } else { unmask_dev_if_necessary(); dev->irq_thread_waiting = false; ret = IRQ_HANDLED; } spin_unlock(dev->irq_lock); return ret; }
userspace_complete(){ complete(dev->irq_compl); }
Quoted 5 lines
Your aproach with
disable_irq_nosync()is completely flawed, simply because you try to pretend that your interrupt handler is done, while it is not done at all. The threaded interrupt handler is done when user space completes. Everything else is just hacking around the problem and creates all that nasty transitional problems.
disable_irq_nosync is the pattern currently used in KVM, it's nothing new in fact.
The approach looks interesting but requires separate code for non-PCI-2.3 devices, i.e. when we have no means to mask at device level. Further drawbacks - unless I missed something on first glance:
- prevents any future optimizations that would work without IRQ thread ping-pong (ie. once we allow guest IRQ injection from hardirq context for selected but typical setups)
- two additional, though light-weight, context switches on each interrupt completion
- continuous polling if user space decides to leave the interrupt unhandled (e.g. because the virtual IRQ line is masked)
Maybe the latter can be solved in a nicer way, but I don't think we can avoid the first two. I'm not saying yet that they are killing this approach, we just need to asses their relevance.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state
Quoted 9 lines · Jan Kiszka
Quoted 5 lines · Thomas Gleixner
Your aproach with
disable_irq_nosync()is completely flawed, simply because you try to pretend that your interrupt handler is done, while it is not done at all. The threaded interrupt handler is done when user space completes. Everything else is just hacking around the problem and creates all that nasty transitional problems.
disable_irq_nosyncis the pattern currently used in KVM, it's nothing new in fact.
That does not make it less flawed :)
Quoted 2 lines
The approach looks interesting but requires separate code for non-PCI-2.3 devices, i.e. when we have no means to mask at device level.
Why? You can have the same code, you just can't request COND_ONESHOT handlers for it, so it needs unshared ONESHOT or it won't work at all, no matter what approach you chose. No device level mask, no sharing, it's that simple.
Quoted 7 lines
Further drawbacks - unless I missed something on first glance:
- prevents any future optimizations that would work without IRQ thread ping-pong (ie. once we allow guest IRQ injection from hardirq context for selected but typical setups)
- two additional, though light-weight, context switches on each interrupt completion
The drawback of these two points is way less than the horror which you need to introduce to do the whole async handler disable, userspace enable dance. Robust and simple solutions really a preferred over complex and fragile horror which has a questionable runtime benefit.
Quoted 2 lines
- continuous polling if user space decides to leave the interrupt unhandled (e.g. because the virtual IRQ line is masked)
That should be a solvable problem.
Thanks,
tglx
Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state
Quoted 29 lines · Thomas Gleixner
Quoted 9 lines · Jan Kiszka
Quoted 5 lines · Thomas Gleixner
Your aproach with
disable_irq_nosync()is completely flawed, simply because you try to pretend that your interrupt handler is done, while it is not done at all. The threaded interrupt handler is done when user space completes. Everything else is just hacking around the problem and creates all that nasty transitional problems.
disable_irq_nosyncis the pattern currently used in KVM, it's nothing new in fact.That does not make it less flawed :)
Quoted 2 lines
The approach looks interesting but requires separate code for non-PCI-2.3 devices, i.e. when we have no means to mask at device level.
Why? You can have the same code, you just can't request
COND_ONESHOThandlers for it, so it needs unshared ONESHOT or it won't work at all, no matter what approach you chose. No device level mask, no sharing, it's that simple.Quoted 7 lines
Further drawbacks - unless I missed something on first glance:
- prevents any future optimizations that would work without IRQ thread ping-pong (ie. once we allow guest IRQ injection from hardirq context for selected but typical setups)
- two additional, though light-weight, context switches on each interrupt completion
Quoted 4 lines
The drawback of these two points is way less than the horror which you need to introduce to do the whole async handler disable, userspace enable dance. Robust and simple solutions really a preferred over complex and fragile horror which has a questionable runtime benefit.
I'd like to note that the overhead of involving the scheduler in interrupt injection for an assigned device should be easily measurable:
just make the MSI handlers threaded and see what the result is.
In the case of emulated devices, when we had an extra thread involved in MSI handling, the vcpu thread and the interrupt injection thread were competing for cpu with strange fluctuations in performance as the result
(i.e. sometimes we would get good speed as threading would introduce a kind of interrupt coalescing, sometimes we would get huge latency).
Quoted 8 lines
Quoted 2 lines
- continuous polling if user space decides to leave the interrupt unhandled (e.g. because the virtual IRQ line is masked)
That should be a solvable problem.
Thanks,
tglx
PATCH v3 3/4 genirq: Add support for IRQF_COND_ONESHOT
In reply to Jan KiszkaFrom: Jan Kiszka <[email protected]>
Provide an adaptive version of IRQF_ONESHOT: If the line is exclusively used, IRQF_COND_ONESHOT provides the same semantics as IRQF_ONESHOT. If it is shared, the line will be unmasked directly after the hardirq handler, just as if IRQF_COND_ONESHOT was not provided.
--- include/linux/interrupt.h | 3 +++ kernel/irq/manage.c | 19 ++++++++++++++++--- 2 files changed, 19 insertions(+), 3 deletions(-)
include/linux/interrupt.h +3 -0
@@ -56,6 +56,8 @@* irq line disabled until the threaded handler has been run. * IRQF_NO_SUSPEND - Do not disable this IRQ during suspend * IRQF_ADAPTIVE - Request notification about upcoming interrupt line sharing +* IRQF_COND_ONESHOT - If line is not shared, keep interrupt disabled after +* hardirq handler finshed. * */ #define IRQF_DISABLED 0x00000020
@@ -69,6 +71,7 @@#define IRQF_ONESHOT 0x00002000#define IRQF_NO_SUSPEND 0x00004000#define IRQF_ADAPTIVE 0x00008000 +#define IRQF_COND_ONESHOT 0x00010000#define IRQF_TIMER (__IRQF_TIMER | IRQF_NO_SUSPEND)
kernel/irq/manage.c +16 -3
@@ -583,7 +583,7 @@ static int irq_thread(void *data)struct sched_param param = { .sched_priority =MAX_USER_RT_PRIO /2 , };struct irqaction *action = data;struct irq_desc *desc =irq_to_desc (action->irq); -int wake, oneshot = desc->status &IRQ_ONESHOT ; +int wake, oneshot;sched_setscheduler (current,SCHED_FIFO , ¶m); current->irqaction = action;
@@ -606,6 +606,7 @@ static int irq_thread(void *data) desc->status |=IRQ_PENDING ;raw_spin_unlock_irq (&desc->lock); }else { + oneshot = desc->status &IRQ_ONESHOT ;raw_spin_unlock_irq (&desc->lock); action->thread_fn (action->irq, action->dev_id);
@@ -759,6 +760,15 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new) shared =1 ; desc->irq_data.drv_status |=IRQS_SHARED ; + desc->status &= ~IRQ_ONESHOT ; + +/* Unmask if the interrupt was masked due to oneshot mode. */ +if ((desc->status & + (IRQ_INPROGRESS |IRQ_DISABLED |IRQ_MASKED )) == +IRQ_MASKED ) { + desc->irq_data.chip->irq_unmask (&desc->irq_data); + desc->status &= ~IRQ_MASKED ; + } }if (!shared) {
@@ -783,7 +793,7 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new) desc->status &= ~(IRQ_AUTODETECT |IRQ_WAITING |IRQ_ONESHOT |IRQ_INPROGRESS |IRQ_SPURIOUS_DISABLED ); -if (new->flags &IRQF_ONESHOT ) +if (new->flags & (IRQF_ONESHOT |IRQF_COND_ONESHOT )) desc->status |=IRQ_ONESHOT ;if (!(desc->status &IRQ_NOAUTOEN )) {
@@ -934,8 +944,11 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id) desc->irq_data.chip->irq_shutdown (&desc->irq_data); else desc->irq_data.chip->irq_disable(&desc->irq_data); - }else if (!desc->action->next) + }else if (!desc->action->next) { single_handler = true; +if (desc->action->flags &IRQF_COND_ONESHOT ) + desc->status |=IRQ_ONESHOT ; + }#ifdef CONFIG_SMP /* make sure affinity_hint is cleaned up */
Signature · 1 line
1.7.1
PATCH v3 4/4 KVM: Allow host IRQ sharing for passed-through PCI 2.3 devices
In reply to Jan KiszkaFrom: Jan Kiszka <[email protected]>
PCI 2.3 allows to generically disable IRQ sources at device level. This enables us to share IRQs of such devices on the host side when passing them to a guest.
However, IRQ disabling via the PCI config space is more costly than masking the line via disable_irq. Therefore we register the IRQ in adaptive mode and switch between line and device level disabling on demand.
This feature is optional, user space has to request it explicitly as it also has to inform us about its view of PCI_COMMAND_INTX_DISABLE. That way, we can avoid unmasking the interrupt and signaling it if the guest masked it via the PCI config space.
--- Documentation/kvm/api.txt | 27 ++++ arch/x86/kvm/x86.c | 1 + include/linux/kvm.h | 6 + include/linux/kvm_host.h | 10 ++- virt/kvm/assigned-dev.c | 336 ++++++++++++++++++++++++++++++++++++++++----- 5 files changed, 346 insertions(+), 34 deletions(-)
Documentation/kvm/api.txt +27 -0
@@ -1112,6 +1112,14 @@ following flags are specified: /* Depends on KVM_CAP_IOMMU */ #define KVM_DEV_ASSIGN_ENABLE_IOMMU (1 << 0) +/* The following two depend on KVM_CAP_PCI_2_3 */ +#define KVM_DEV_ASSIGN_PCI_2_3 (1 << 1) +#define KVM_DEV_ASSIGN_MASK_INTX (1 << 2) + +If KVM_DEV_ASSIGN_PCI_2_3 is set, the kernel will manage legacy INTx interrupts +via the PCI-2.3-compliant device-level mask, but only if IRQ sharing with other +assigned or host devices requires it. KVM_DEV_ASSIGN_MASK_INTX specifies the +guest's view on the INTx mask, see KVM_ASSIGN_SET_INTX_MASK for details. 4.48 KVM_DEASSIGN_PCI_DEVICE
@@ -1263,6 +1271,25 @@ struct kvm_assigned_msix_entry { __u16 padding[3]; }; +4.54 KVM_ASSIGN_SET_INTX_MASK + +Capability: KVM_CAP_PCI_2_3 +Architectures: x86 +Type: vm ioctl +Parameters: struct kvm_assigned_pci_dev (in) +Returns: 0 on success, -1 on error + +Informs the kernel about the guest's view on the INTx mask. As long as the +guest masks the legacy INTx, the kernel will refrain from unmasking it at +hardware level and will not assert the guest's IRQ line. User space is still +responsible for applying this state to the assigned device's real config space. +To avoid that the kernel overwrites the state user space wants to set, +KVM_ASSIGN_SET_INTX_MASK has to be called prior to updating the config space. + +See KVM_ASSIGN_DEV_IRQ for the data structure. The target device is specified +by assigned_dev_id. In the flags field, only KVM_DEV_ASSIGN_MASK_INTX is +evaluated. + 5. The kvm_run structure Application code obtains a pointer to the kvm_run structure by
arch/x86/kvm/x86.c +1 -0
@@ -1965,6 +1965,7 @@ int kvm_dev_ioctl_check_extension(long ext)case KVM_CAP_X86_ROBUST_SINGLESTEP :case KVM_CAP_XSAVE :case KVM_CAP_ASYNC_PF : +case KVM_CAP_PCI_2_3 : r =1 ;break ;case KVM_CAP_COALESCED_MMIO :
include/linux/kvm.h +6 -0
@@ -541,6 +541,7 @@ struct kvm_ppc_pvinfo {#define KVM_CAP_PPC_GET_PVINFO 57#define KVM_CAP_PPC_IRQ_LEVEL 58#define KVM_CAP_ASYNC_PF 59 +#define KVM_CAP_PCI_2_3 60#ifdef KVM_CAP_IRQ_ROUTING
@@ -677,6 +678,9 @@ struct kvm_clock_data {#define KVM_SET_PIT2 _IOW(KVMIO, 0xa0, struct kvm_pit_state2)/* Available with KVM_CAP_PPC_GET_PVINFO */ #define KVM_PPC_GET_PVINFO _IOW(KVMIO, 0xa1, struct kvm_ppc_pvinfo) +/* Available with KVM_CAP_PCI_2_3 */ +#define KVM_ASSIGN_SET_INTX_MASK _IOW(KVMIO, 0xa2, \ + struct kvm_assigned_pci_dev)/* * ioctls for vcpu fds
@@ -742,6 +746,8 @@ struct kvm_clock_data {#define KVM_SET_XCRS _IOW(KVMIO, 0xa7, struct kvm_xcrs)#define KVM_DEV_ASSIGN_ENABLE_IOMMU (1 << 0) +#define KVM_DEV_ASSIGN_PCI_2_3 (1 << 1) +#define KVM_DEV_ASSIGN_MASK_INTX (1 << 2)struct kvm_assigned_pci_dev {__u32 assigned_dev_id;
include/linux/kvm_host.h +9 -1
@@ -477,6 +477,12 @@ struct kvm_irq_ack_notifier {void (*irq_acked)(struct kvm_irq_ack_notifier *kian); }; +enum kvm_intx_state { +KVM_INTX_ENABLED , +KVM_INTX_LINE_DISABLED , +KVM_INTX_DEVICE_DISABLED , +}; +struct kvm_assigned_dev_kernel {struct kvm_irq_ack_notifier ack_notifier;struct list_head list;
@@ -486,7 +492,7 @@ struct kvm_assigned_dev_kernel {int host_devfn;unsigned int entries_nr;int host_irq; -bool host_irq_disabled; +unsigned long last_irq_status;struct msix_entry *host_msix_entries;int guest_irq;struct msix_entry *guest_msix_entries;
@@ -496,6 +502,8 @@ struct kvm_assigned_dev_kernel {struct pci_dev *dev;struct kvm *kvm;spinlock_t intx_lock; +spinlock_t intx_mask_lock; +enum kvm_intx_state intx_state;char irq_name[32 ]; };
virt/kvm/assigned-dev.c +303 -33
441 lines not on this page · raw message
Signature · 1 line
1.7.1
Quoted 2 lines · Jan Kiszka
Final but critical question: Who will pick up which bits?
The procedure which has served us well in the past is that tip picks up the irq stuff and sticks them in a fast-forward-only branch; kvm merges the branch and applies the kvm bits on top.
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 7 lines · Jan Kiszka
This addresses the review comments of the previous round:
- renamedirq_data::statustodrv_status
- moveddrv_statusaround to unbreakGENERIC_HARDIRQS_NO_DEPRECATED
- fixed signature ofget_irq_status(irq is now unsigned int)
- convertedregister_lockinto a global one
- fixed critical white space breakage (that I just left in to check if anyone is actually reading the code, of course...)
Just for the record, you either missed or introduced some new white space noise :)