[PATCH v2 0/4] KVM & genirq: Enable adaptive IRQ sharing for passed-through devices

[PATCH v2 0/4] KVM & genirq: Enable adaptive IRQ sharing for passed-through devices

[Jan Kiszka]

Second try to allow adaptive interrupt handlers according to the line's
current sharing situation. This one converts the explicit notifier
interface into several extension of existing genirq APIs:

 - introduce a interrupt status word that can be read by interrupt
   handlers to obtain the current line sharing state
 - introduce IRQF_ADAPTIVE: handlers registered with this flag will be
   informed about an upcoming line sharing by calling the interrupt
   handler itself (instead of a notifier callback)
 - introduce IRQF_COND_ONESHOT: IRQF_ONESHOT semantics if the line is
   not shared, standard semantics otherwise (removes the need to
   re-register handlers to switch between IRQF_SHARED and IRQF_ONESHOT)

The result may look simpler on first glance than v1, but it comes with
more subtle race scenarios IMO. I thought them through, hopefully
catching all, but I would appreciate any skeptical review.

I also tried to replace line-level disabling from the interrupt handler
with signaling genirq to keep the line masked on handler return.
However, I finally dropped this idea as it turns out to be very hard (if
not impossible) to properly synchronize the generic tail (in genirq)
with the driver that may want to re-enable the line at the same time
(depending on the driver's internal state). But if anyone sees a
magically simple way to achieve this, I'm still all ears.

Finally, the last patch in this series makes use of the new interface
for KVM's PCI pass-through subsystem. KVM has to keep the interrupt
source disabled while calling into the guest to handle the event. This
can be done at device or line level. The former is required to share the
interrupt line, the latter is an order of magnitude faster.

Beside pass-through support of KVM, further users of this new interface
could become VFIO (not yet mainline) and uio_pci_generic which have to
resolve the same conflict.

Note: The KVM patch depends on
http://thread.gmane.org/gmane.comp.emulators.kvm.devel/64515

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/irqdesc.h   |    2 +
 include/linux/kvm.h       |    6 +
 include/linux/kvm_host.h  |   10 ++-
 kernel/irq/irqdesc.c      |    2 +
 kernel/irq/manage.c       |   74 ++++++++++-
 virt/kvm/assigned-dev.c   |  336 ++++++++++++++++++++++++++++++++++++++++-----
 10 files changed, 437 insertions(+), 38 deletions(-)

[PATCH v2 1/4] genirq: Introduce driver-readable IRQ status word

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

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.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 include/linux/interrupt.h |    2 ++
 include/linux/irq.h       |    2 ++
 kernel/irq/manage.c       |   15 +++++++++++++++
 3 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 79d0c4f..16cdbbf 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -126,6 +126,8 @@ struct irqaction {
 
 extern irqreturn_t no_action(int cpl, void *dev_id);
 
+extern unsigned long get_irq_status(unsigned long irq);
+
 #ifdef CONFIG_GENERIC_HARDIRQS
 extern int __must_check
 request_threaded_irq(unsigned int irq, irq_handler_t handler,
diff --git a/include/linux/irq.h b/include/linux/irq.h
index abde252..5554203 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -96,6 +96,7 @@ struct msi_desc;
  *			methods, to allow shared chip implementations
  * @msi_desc:		MSI descriptor
  * @affinity:		IRQ affinity on SMP
+ * @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
@@ -108,6 +109,7 @@ struct irq_data {
 	void			*handler_data;
 	void			*chip_data;
 	struct msi_desc		*msi_desc;
+	unsigned long		status;
 #ifdef CONFIG_SMP
 	cpumask_var_t		affinity;
 #endif
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 5f92acc..df51284 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -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 long irq)
+{
+	struct irq_desc *desc = irq_to_desc(irq);
+
+	return desc ? desc->irq_data.status : 0;
+}
+EXPORT_SYMBOL_GPL(get_irq_status);
-- 
1.7.1

Re: [PATCH v2 1/4] genirq: Introduce driver-readable IRQ status word

[Thomas Gleixner]

On Sun, 12 Dec 2010, Jan Kiszka wrote:

> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> 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.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  include/linux/interrupt.h |    2 ++
>  include/linux/irq.h       |    2 ++
>  kernel/irq/manage.c       |   15 +++++++++++++++
>  3 files changed, 19 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
> index 79d0c4f..16cdbbf 100644
> --- a/include/linux/interrupt.h
> +++ b/include/linux/interrupt.h
> @@ -126,6 +126,8 @@ struct irqaction {
>  
>  extern irqreturn_t no_action(int cpl, void *dev_id);
>  
> +extern unsigned long get_irq_status(unsigned long irq);
> +
>  #ifdef CONFIG_GENERIC_HARDIRQS
>  extern int __must_check
>  request_threaded_irq(unsigned int irq, irq_handler_t handler,
> diff --git a/include/linux/irq.h b/include/linux/irq.h
> index abde252..5554203 100644
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -96,6 +96,7 @@ struct msi_desc;
>   *			methods, to allow shared chip implementations
>   * @msi_desc:		MSI descriptor
>   * @affinity:		IRQ affinity on SMP
> + * @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
> @@ -108,6 +109,7 @@ struct irq_data {
>  	void			*handler_data;
>  	void			*chip_data;
>  	struct msi_desc		*msi_desc;
> +	unsigned long		status;

That breaks the current code which has irq_data embedded and shadowed
in irq_desc for migratory reasons until all users are fixed up and the
GENERIC_HARDIRQS_NO_DEPRECATED sections are cleaned up. I know it's
ugly, but that was the only way not to rewrite the world in one go. :)
Just move it below affinity.

Also we should name it different than status, drv_status perhaps, to
avoid confusion with the irq_desc status.

>  #ifdef CONFIG_SMP
>  	cpumask_var_t		affinity;
>  #endif

Thanks,

	tglx

Re: [PATCH v2 1/4] genirq: Introduce driver-readable IRQ status word

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2331 bytes --]

Am 12.12.2010 18:29, Thomas Gleixner wrote:
> On Sun, 12 Dec 2010, Jan Kiszka wrote:
> 
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> 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.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  include/linux/interrupt.h |    2 ++
>>  include/linux/irq.h       |    2 ++
>>  kernel/irq/manage.c       |   15 +++++++++++++++
>>  3 files changed, 19 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
>> index 79d0c4f..16cdbbf 100644
>> --- a/include/linux/interrupt.h
>> +++ b/include/linux/interrupt.h
>> @@ -126,6 +126,8 @@ struct irqaction {
>>  
>>  extern irqreturn_t no_action(int cpl, void *dev_id);
>>  
>> +extern unsigned long get_irq_status(unsigned long irq);
>> +
>>  #ifdef CONFIG_GENERIC_HARDIRQS
>>  extern int __must_check
>>  request_threaded_irq(unsigned int irq, irq_handler_t handler,
>> diff --git a/include/linux/irq.h b/include/linux/irq.h
>> index abde252..5554203 100644
>> --- a/include/linux/irq.h
>> +++ b/include/linux/irq.h
>> @@ -96,6 +96,7 @@ struct msi_desc;
>>   *			methods, to allow shared chip implementations
>>   * @msi_desc:		MSI descriptor
>>   * @affinity:		IRQ affinity on SMP
>> + * @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
>> @@ -108,6 +109,7 @@ struct irq_data {
>>  	void			*handler_data;
>>  	void			*chip_data;
>>  	struct msi_desc		*msi_desc;
>> +	unsigned long		status;
> 
> That breaks the current code which has irq_data embedded and shadowed
> in irq_desc for migratory reasons until all users are fixed up and the
> GENERIC_HARDIRQS_NO_DEPRECATED sections are cleaned up. I know it's
> ugly, but that was the only way not to rewrite the world in one go. :)
> Just move it below affinity.
> 
> Also we should name it different than status, drv_status perhaps, to
> avoid confusion with the irq_desc status.

OK, will address both in a succeeding round (just waiting for potential
further comments).

Thanks,
Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: [PATCH v2 1/4] genirq: Introduce driver-readable IRQ status word

[Thomas Gleixner]

On Sun, 12 Dec 2010, Jan Kiszka wrote:
> Am 12.12.2010 18:29, Thomas Gleixner wrote:
> > Also we should name it different than status, drv_status perhaps, to
> > avoid confusion with the irq_desc status.
> 
> OK, will address both in a succeeding round (just waiting for potential
> further comments).

No further comments from my side ATM.

Thanks,

	tglx

[PATCH v2 2/4] genirq: Inform handler about line sharing state

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

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.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 include/linux/interrupt.h |   10 ++++++++++
 include/linux/irqdesc.h   |    2 ++
 kernel/irq/irqdesc.c      |    2 ++
 kernel/irq/manage.c       |   44 +++++++++++++++++++++++++++++++++++++++++---
 4 files changed, 55 insertions(+), 3 deletions(-)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 16cdbbf..c6323a2 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -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 long irq);
 
 #ifdef CONFIG_GENERIC_HARDIRQS
diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h
index 979c68c..c490e83 100644
--- a/include/linux/irqdesc.h
+++ b/include/linux/irqdesc.h
@@ -29,6 +29,7 @@ struct timer_rand_state;
  * @wait_for_threads:	wait queue for sync_irq to wait for threaded handlers
  * @dir:		/proc/irq/ procfs entry
  * @name:		flow handler name for /proc/interrupts output
+ * @register_lock:	protects registration & release, for unshared->shared
  */
 struct irq_desc {
 
@@ -80,6 +81,7 @@ struct irq_desc {
 	struct proc_dir_entry	*dir;
 #endif
 	const char		*name;
+	struct mutex		register_lock;
 } ____cacheline_internodealigned_in_smp;
 
 #ifndef CONFIG_SPARSE_IRQ
diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 9988d03..de69845 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -143,6 +143,7 @@ static struct irq_desc *alloc_desc(int irq, int node)
 
 	raw_spin_lock_init(&desc->lock);
 	lockdep_set_class(&desc->lock, &irq_desc_lock_class);
+	mutex_init(&desc->register_lock);
 
 	desc_set_defaults(irq, desc, node);
 
@@ -254,6 +255,7 @@ int __init early_irq_init(void)
 		alloc_masks(desc + i, GFP_KERNEL, node);
 		desc_smp_init(desc + i, node);
 		lockdep_set_class(&desc[i].lock, &irq_desc_lock_class);
+		mutex_init(&desc[i].register_lock);
 	}
 	return arch_early_irq_init();
 }
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index df51284..a4557eb 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -754,6 +754,8 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 			old = *old_ptr;
 		} while (old);
 		shared = 1;
+
+		desc->irq_data.status |= IRQS_SHARED;
 	}
 
 	if (!shared) {
@@ -883,6 +885,7 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)
 {
 	struct irq_desc *desc = irq_to_desc(irq);
 	struct irqaction *action, **action_ptr;
+	bool single_handler = false;
 	unsigned long flags;
 
 	WARN(in_interrupt(), "Trying to free IRQ %d from IRQ context!\n", irq);
@@ -928,7 +931,8 @@ 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)
+		single_handler = true;
 
 #ifdef CONFIG_SMP
 	/* make sure affinity_hint is cleaned up */
@@ -943,6 +947,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.status &= ~IRQS_SHARED;
+
 #ifdef CONFIG_DEBUG_SHIRQ
 	/*
 	 * It's a shared IRQ -- the driver ought to be prepared for an IRQ
@@ -1002,9 +1009,13 @@ void free_irq(unsigned int irq, void *dev_id)
 	if (!desc)
 		return;
 
+	mutex_lock(&desc->register_lock);
+
 	chip_bus_lock(desc);
 	kfree(__free_irq(irq, dev_id));
 	chip_bus_sync_unlock(desc);
+
+	mutex_unlock(&desc->register_lock);
 }
 EXPORT_SYMBOL(free_irq);
 
@@ -1055,7 +1066,7 @@ int request_threaded_irq(unsigned int irq, irq_handler_t handler,
 			 irq_handler_t thread_fn, unsigned long irqflags,
 			 const char *devname, void *dev_id)
 {
-	struct irqaction *action;
+	struct irqaction *action, *old_action;
 	struct irq_desc *desc;
 	int retval;
 
@@ -1091,12 +1102,39 @@ int request_threaded_irq(unsigned int irq, irq_handler_t handler,
 	action->name = devname;
 	action->dev_id = dev_id;
 
+	mutex_lock(&desc->register_lock);
+
+	old_action = desc->action;
+	if (old_action && (old_action->flags & IRQF_ADAPTIVE) &&
+	    !(desc->irq_data.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.status |= IRQS_SHARED | IRQS_MAKE_SHAREABLE;
+		old_action->handler(irq, old_action->dev_id);
+		desc->irq_data.status &= ~IRQS_MAKE_SHAREABLE;
+
+		local_irq_enable();
+		enable_irq(irq);
+		
+	}
+
 	chip_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.status &= ~IRQS_SHARED;
 		kfree(action);
+	}
+
+	mutex_unlock(&desc->register_lock);
 
 #ifdef CONFIG_DEBUG_SHIRQ
 	if (!retval && (irqflags & IRQF_SHARED)) {
-- 
1.7.1

Re: [PATCH v2 2/4] genirq: Inform handler about line sharing state

[Thomas Gleixner]

On Sun, 12 Dec 2010, Jan Kiszka wrote:

> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> 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.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  include/linux/interrupt.h |   10 ++++++++++
>  include/linux/irqdesc.h   |    2 ++
>  kernel/irq/irqdesc.c      |    2 ++
>  kernel/irq/manage.c       |   44 +++++++++++++++++++++++++++++++++++++++++---
>  4 files changed, 55 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
> index 16cdbbf..c6323a2 100644
> --- a/include/linux/interrupt.h
> +++ b/include/linux/interrupt.h
> @@ -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 long irq);
>  
>  #ifdef CONFIG_GENERIC_HARDIRQS
> diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h
> index 979c68c..c490e83 100644
> --- a/include/linux/irqdesc.h
> +++ b/include/linux/irqdesc.h
> @@ -29,6 +29,7 @@ struct timer_rand_state;
>   * @wait_for_threads:	wait queue for sync_irq to wait for threaded handlers
>   * @dir:		/proc/irq/ procfs entry
>   * @name:		flow handler name for /proc/interrupts output
> + * @register_lock:	protects registration & release, for unshared->shared

I think we can make that a global mutex. request/free_irq are not
hotpath operations which require a mutex per irq descriptor.

Thanks,

	tglx

Re: [PATCH v2 2/4] genirq: Inform handler about line sharing state

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 3026 bytes --]

Am 12.12.2010 17:53, Thomas Gleixner wrote:
> On Sun, 12 Dec 2010, Jan Kiszka wrote:
> 
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> 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.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  include/linux/interrupt.h |   10 ++++++++++
>>  include/linux/irqdesc.h   |    2 ++
>>  kernel/irq/irqdesc.c      |    2 ++
>>  kernel/irq/manage.c       |   44 +++++++++++++++++++++++++++++++++++++++++---
>>  4 files changed, 55 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
>> index 16cdbbf..c6323a2 100644
>> --- a/include/linux/interrupt.h
>> +++ b/include/linux/interrupt.h
>> @@ -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 long irq);
>>  
>>  #ifdef CONFIG_GENERIC_HARDIRQS
>> diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h
>> index 979c68c..c490e83 100644
>> --- a/include/linux/irqdesc.h
>> +++ b/include/linux/irqdesc.h
>> @@ -29,6 +29,7 @@ struct timer_rand_state;
>>   * @wait_for_threads:	wait queue for sync_irq to wait for threaded handlers
>>   * @dir:		/proc/irq/ procfs entry
>>   * @name:		flow handler name for /proc/interrupts output
>> + * @register_lock:	protects registration & release, for unshared->shared
> 
> I think we can make that a global mutex. request/free_irq are not
> hotpath operations which require a mutex per irq descriptor.
> 

Agreed, will change this.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

[PATCH v2 3/4] genirq: Add support for IRQF_COND_ONESHOT

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

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.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 include/linux/interrupt.h |    3 +++
 kernel/irq/manage.c       |   19 ++++++++++++++++---
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index c6323a2..f462807 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -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)
 
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index a4557eb..948b7c9 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -580,7 +580,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, &param);
 	current->irqaction = action;
@@ -603,6 +603,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);
@@ -756,6 +757,15 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 		shared = 1;
 
 		desc->irq_data.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) {
@@ -780,7 +790,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)) {
@@ -931,8 +941,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 */
-- 
1.7.1

[PATCH v2 4/4] KVM: Allow host IRQ sharing for passed-through PCI 2.3 devices

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

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.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 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(-)

diff --git a/Documentation/kvm/api.txt b/Documentation/kvm/api.txt
index e1a9297..1c34e25 100644
--- a/Documentation/kvm/api.txt
+++ b/Documentation/kvm/api.txt
@@ -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
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index ed373ba..8775a54 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -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:
diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index ea2dc1a..3cadb42 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -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;
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index ac4e83a..4f95070 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -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];
 };
 
diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
index c6114d3..b64799a 100644
--- a/virt/kvm/assigned-dev.c
+++ b/virt/kvm/assigned-dev.c
@@ -55,22 +55,141 @@ static int find_index_from_host_irq(struct kvm_assigned_dev_kernel
 	return index;
 }
 
-static irqreturn_t kvm_assigned_dev_thread(int irq, void *dev_id)
+static bool
+pci_2_3_set_irq_mask(struct pci_dev *dev, bool mask, bool check_status)
+{
+	u32 cmd_status_dword;
+	u16 origcmd, newcmd;
+	bool mask_updated = true;
+
+	/*
+	 * We do a single dword read to retrieve both command and status.
+	 * Document assumptions that make this possible.
+	 */
+	BUILD_BUG_ON(PCI_COMMAND % 4);
+	BUILD_BUG_ON(PCI_COMMAND + 2 != PCI_STATUS);
+
+	pci_block_user_cfg_access(dev);
+
+	/*
+	 * Read both command and status registers in a single 32-bit operation.
+	 * Note: we could cache the value for command and move the status read
+	 * out of the lock if there was a way to get notified of user changes
+	 * to command register through sysfs. Should be good for shared irqs.
+	 */
+	pci_read_config_dword(dev, PCI_COMMAND, &cmd_status_dword);
+
+	if (check_status) {
+		bool irq_pending =
+			(cmd_status_dword >> 16) & PCI_STATUS_INTERRUPT;
+
+		/*
+		 * Check interrupt status register to see whether our device
+		 * triggered the interrupt (when masking) or the next IRQ is
+		 * already pending (when unmasking).
+		 */
+		if (mask != irq_pending) {
+			mask_updated = false;
+			goto done;
+		}
+	}
+
+	origcmd = cmd_status_dword;
+	newcmd = origcmd & ~PCI_COMMAND_INTX_DISABLE;
+	if (mask)
+		newcmd |= PCI_COMMAND_INTX_DISABLE;
+	if (newcmd != origcmd)
+		pci_write_config_word(dev, PCI_COMMAND, newcmd);
+
+done:
+	pci_unblock_user_cfg_access(dev);
+	return mask_updated;
+}
+
+static void pci_2_3_irq_mask(struct pci_dev *dev)
+{
+	pci_2_3_set_irq_mask(dev, true, false);
+}
+
+static bool pci_2_3_irq_check_and_mask(struct pci_dev *dev)
+{
+	return pci_2_3_set_irq_mask(dev, true, true);
+}
+
+static void pci_2_3_irq_unmask(struct pci_dev *dev)
+{
+	pci_2_3_set_irq_mask(dev, false, false);
+}
+
+static bool pci_2_3_irq_check_and_unmask(struct pci_dev *dev)
+{
+	return pci_2_3_set_irq_mask(dev, false, true);
+}
+
+static irqreturn_t kvm_assigned_dev_intr_intx(int irq, void *dev_id)
+{
+	struct kvm_assigned_dev_kernel *assigned_dev = dev_id;
+	unsigned long irq_status = get_irq_status(irq);
+	int ret;
+
+	assigned_dev->last_irq_status = irq_status;
+
+	if (!(irq_status & (IRQS_SHARED | IRQS_MAKE_SHAREABLE)))
+		return IRQ_WAKE_THREAD;
+
+	spin_lock(&assigned_dev->intx_lock);
+
+	if (irq_status & IRQS_MAKE_SHAREABLE) {
+		if (assigned_dev->intx_state == KVM_INTX_LINE_DISABLED) {
+			pci_2_3_irq_mask(assigned_dev->dev);
+			enable_irq(irq);
+			assigned_dev->intx_state = KVM_INTX_DEVICE_DISABLED;
+		}
+		ret = IRQ_HANDLED;
+	} else if (pci_2_3_irq_check_and_mask(assigned_dev->dev)) {
+		assigned_dev->intx_state = KVM_INTX_DEVICE_DISABLED;
+		ret = IRQ_WAKE_THREAD;
+	} else
+		ret =IRQ_NONE;
+
+	spin_unlock(&assigned_dev->intx_lock);
+
+	return ret;
+}
+
+static irqreturn_t kvm_assigned_dev_thread_intx(int irq, void *dev_id)
 {
 	struct kvm_assigned_dev_kernel *assigned_dev = dev_id;
 
-	if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_HOST_INTX) {
-		spin_lock(&assigned_dev->intx_lock);
-		disable_irq_nosync(irq);
-		assigned_dev->host_irq_disabled = true;
-		spin_unlock(&assigned_dev->intx_lock);
+	if (!(assigned_dev->last_irq_status & IRQS_SHARED)) {
+		spin_lock_irq(&assigned_dev->intx_lock);
+		if (assigned_dev->intx_state == KVM_INTX_ENABLED) {
+			disable_irq_nosync(irq);
+			assigned_dev->intx_state = KVM_INTX_LINE_DISABLED;
+		}
+		spin_unlock_irq(&assigned_dev->intx_lock);
 	}
 
+	spin_lock(&assigned_dev->intx_mask_lock);
+	if (!(assigned_dev->flags & KVM_DEV_ASSIGN_MASK_INTX))
+		kvm_set_irq(assigned_dev->kvm, assigned_dev->irq_source_id,
+			    assigned_dev->guest_irq, 1);
+	spin_unlock(&assigned_dev->intx_mask_lock);
+
+	return IRQ_HANDLED;
+}
+
+#ifdef __KVM_HAVE_MSI
+static irqreturn_t kvm_assigned_dev_thread_msi(int irq, void *dev_id)
+{
+	struct kvm_assigned_dev_kernel *assigned_dev = dev_id;
+
 	kvm_set_irq(assigned_dev->kvm, assigned_dev->irq_source_id,
 		    assigned_dev->guest_irq, 1);
 
 	return IRQ_HANDLED;
 }
+#endif
 
 #ifdef __KVM_HAVE_MSIX
 static irqreturn_t kvm_assigned_dev_thread_msix(int irq, void *dev_id)
@@ -102,15 +221,36 @@ static void kvm_assigned_dev_ack_irq(struct kvm_irq_ack_notifier *kian)
 
 	kvm_set_irq(dev->kvm, dev->irq_source_id, dev->guest_irq, 0);
 
-	/* The guest irq may be shared so this ack may be
-	 * from another device.
-	 */
-	spin_lock(&dev->intx_lock);
-	if (dev->host_irq_disabled) {
-		enable_irq(dev->host_irq);
-		dev->host_irq_disabled = false;
+	if (likely(!(dev->irq_requested_type & KVM_DEV_IRQ_HOST_INTX)))
+		return;
+
+	spin_lock(&dev->intx_mask_lock);
+
+	if (!(dev->flags & KVM_DEV_ASSIGN_MASK_INTX)) {
+		bool reassert = false;
+
+		spin_lock_irq(&dev->intx_lock);
+		/*
+		 * The guest IRQ may be shared so this ack can come from an
+		 * IRQ for another guest device.
+		 */
+		if (dev->intx_state == KVM_INTX_LINE_DISABLED) {
+			enable_irq(dev->host_irq);
+			dev->intx_state = KVM_INTX_ENABLED;
+		} else if (dev->intx_state == KVM_INTX_DEVICE_DISABLED) {
+			if (pci_2_3_irq_check_and_unmask(dev->dev))
+				dev->intx_state = KVM_INTX_ENABLED;
+			else
+				reassert = true;
+		}
+		spin_unlock_irq(&dev->intx_lock);
+
+		if (reassert)
+			kvm_set_irq(dev->kvm, dev->irq_source_id,
+				    dev->guest_irq, 1);
 	}
-	spin_unlock(&dev->intx_lock);
+
+	spin_unlock(&dev->intx_mask_lock);
 }
 
 static void deassign_guest_irq(struct kvm *kvm,
@@ -155,14 +295,21 @@ static void deassign_host_irq(struct kvm *kvm,
 		kfree(assigned_dev->host_msix_entries);
 		kfree(assigned_dev->guest_msix_entries);
 		pci_disable_msix(assigned_dev->dev);
+	} else if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_HOST_MSI) {
+		free_irq(assigned_dev->host_irq, assigned_dev);
+		pci_disable_msi(assigned_dev->dev);
 	} else {
-		/* Deal with MSI and INTx */
-		disable_irq(assigned_dev->host_irq);
+		if (assigned_dev->flags & KVM_DEV_ASSIGN_PCI_2_3) {
+			spin_lock_irq(&assigned_dev->intx_lock);
+			pci_2_3_irq_mask(assigned_dev->dev);
+			/* prevent re-enabling by kvm_assigned_dev_ack_irq */
+			assigned_dev->intx_state = KVM_INTX_ENABLED;
+			spin_unlock_irq(&assigned_dev->intx_lock);
+			synchronize_irq(assigned_dev->host_irq);
+		} else
+			disable_irq(assigned_dev->host_irq);
 
 		free_irq(assigned_dev->host_irq, assigned_dev);
-
-		if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_HOST_MSI)
-			pci_disable_msi(assigned_dev->dev);
 	}
 
 	assigned_dev->irq_requested_type &= ~(KVM_DEV_IRQ_HOST_MASK);
@@ -231,16 +378,41 @@ void kvm_free_all_assigned_devices(struct kvm *kvm)
 static int assigned_device_enable_host_intx(struct kvm *kvm,
 					    struct kvm_assigned_dev_kernel *dev)
 {
+	irq_handler_t handler;
+	unsigned long flags;
+	int err;
+
 	dev->host_irq = dev->dev->irq;
-	/* Even though this is PCI, we don't want to use shared
-	 * interrupts. Sharing host devices with guest-assigned devices
-	 * on the same interrupt line is not a happy situation: there
-	 * are going to be long delays in accepting, acking, etc.
+	dev->intx_state = KVM_INTX_ENABLED;
+	dev->last_irq_status = 0;
+
+	/*
+	 * We can only share the IRQ line with other host devices if we are
+	 * able to disable the IRQ source at device-level - independently of
+	 * the guest driver. Otherwise host devices may suffer from unbounded
+	 * IRQ latencies when the guest keeps the line asserted.
+	 * If PCI 2.3 support is available, we can instal a sharing notifier
+	 * and apply the required disabling pattern on demand.
 	 */
-	if (request_threaded_irq(dev->host_irq, NULL, kvm_assigned_dev_thread,
-				 IRQF_ONESHOT, dev->irq_name, dev))
-		return -EIO;
-	return 0;
+	if (dev->flags & KVM_DEV_ASSIGN_PCI_2_3) {
+		handler = kvm_assigned_dev_intr_intx;
+		flags = IRQF_SHARED | IRQF_ADAPTIVE | IRQF_COND_ONESHOT;
+	} else {
+		handler = NULL;
+		flags = IRQF_ONESHOT;
+	}
+
+	err = request_threaded_irq(dev->host_irq, handler,
+				   kvm_assigned_dev_thread_intx, flags,
+				   dev->irq_name, dev);
+
+	if (!err && dev->flags & KVM_DEV_ASSIGN_PCI_2_3) {
+		spin_lock_irq(&dev->intx_lock);
+		pci_2_3_irq_unmask(dev->dev);
+		spin_unlock_irq(&dev->intx_lock);
+	}
+
+	return err;
 }
 
 #ifdef __KVM_HAVE_MSI
@@ -256,8 +428,9 @@ static int assigned_device_enable_host_msi(struct kvm *kvm,
 	}
 
 	dev->host_irq = dev->dev->irq;
-	if (request_threaded_irq(dev->host_irq, NULL, kvm_assigned_dev_thread,
-				 0, dev->irq_name, dev)) {
+	if (request_threaded_irq(dev->host_irq, NULL,
+				 kvm_assigned_dev_thread_msi, 0,
+				 dev->irq_name, dev)) {
 		pci_disable_msi(dev->dev);
 		return -EIO;
 	}
@@ -296,7 +469,6 @@ err:
 	pci_disable_msix(dev->dev);
 	return r;
 }
-
 #endif
 
 static int assigned_device_enable_guest_intx(struct kvm *kvm,
@@ -315,7 +487,6 @@ static int assigned_device_enable_guest_msi(struct kvm *kvm,
 {
 	dev->guest_irq = irq->guest_irq;
 	dev->ack_notifier.gsi = -1;
-	dev->host_irq_disabled = false;
 	return 0;
 }
 #endif
@@ -327,7 +498,6 @@ static int assigned_device_enable_guest_msix(struct kvm *kvm,
 {
 	dev->guest_irq = irq->guest_irq;
 	dev->ack_notifier.gsi = -1;
-	dev->host_irq_disabled = false;
 	return 0;
 }
 #endif
@@ -461,6 +631,7 @@ static int kvm_vm_ioctl_deassign_dev_irq(struct kvm *kvm,
 {
 	int r = -ENODEV;
 	struct kvm_assigned_dev_kernel *match;
+	unsigned long irq_type;
 
 	mutex_lock(&kvm->lock);
 
@@ -469,12 +640,51 @@ static int kvm_vm_ioctl_deassign_dev_irq(struct kvm *kvm,
 	if (!match)
 		goto out;
 
-	r = kvm_deassign_irq(kvm, match, assigned_irq->flags);
+	irq_type = assigned_irq->flags & (KVM_DEV_IRQ_HOST_MASK |
+					  KVM_DEV_IRQ_GUEST_MASK);
+	r = kvm_deassign_irq(kvm, match, irq_type);
 out:
 	mutex_unlock(&kvm->lock);
 	return r;
 }
 
+/*
+ * Verify that the device supports Interrupt Disable bit in command register,
+ * per PCI 2.3, by flipping this bit and reading it back: this bit was readonly
+ * in PCI 2.2.
+ */
+static bool pci_2_3_supported(struct pci_dev *pdev)
+{
+	u16 orig, new;
+
+	pci_block_user_cfg_access(pdev);
+
+	pci_read_config_word(pdev, PCI_COMMAND, &orig);
+	pci_write_config_word(pdev, PCI_COMMAND,
+			      orig ^ PCI_COMMAND_INTX_DISABLE);
+	pci_read_config_word(pdev, PCI_COMMAND, &new);
+	pci_write_config_word(pdev, PCI_COMMAND, orig);
+
+	pci_unblock_user_cfg_access(pdev);
+
+	/*
+	 * There's no way to protect against
+	 * hardware bugs or detect them reliably, but as long as we know
+	 * what the value should be, let's go ahead and check it.
+	 */
+	if ((new ^ orig) & ~PCI_COMMAND_INTX_DISABLE) {
+		dev_err(&pdev->dev, "Command changed from 0x%x to 0x%x: "
+			"driver or HW bug?\n", orig, new);
+		return false;
+	}
+	if (!((new ^ orig) & PCI_COMMAND_INTX_DISABLE)) {
+		dev_warn(&pdev->dev, "Device does not support disabling "
+			 "interrupts, IRQ sharing impossible.\n");
+		return false;
+	}
+	return true;
+}
+
 static int kvm_vm_ioctl_assign_device(struct kvm *kvm,
 				      struct kvm_assigned_pci_dev *assigned_dev)
 {
@@ -523,6 +733,9 @@ static int kvm_vm_ioctl_assign_device(struct kvm *kvm,
 	pci_reset_function(dev);
 	pci_save_state(dev);
 
+	if (!pci_2_3_supported(dev))
+		assigned_dev->flags &= ~KVM_DEV_ASSIGN_PCI_2_3;
+
 	match->assigned_dev_id = assigned_dev->assigned_dev_id;
 	match->host_segnr = assigned_dev->segnr;
 	match->host_busnr = assigned_dev->busnr;
@@ -530,6 +743,7 @@ static int kvm_vm_ioctl_assign_device(struct kvm *kvm,
 	match->flags = assigned_dev->flags;
 	match->dev = dev;
 	spin_lock_init(&match->intx_lock);
+	spin_lock_init(&match->intx_mask_lock);
 	match->irq_source_id = -1;
 	match->kvm = kvm;
 	match->ack_notifier.irq_acked = kvm_assigned_dev_ack_irq;
@@ -676,6 +890,53 @@ msix_entry_out:
 }
 #endif
 
+static int kvm_vm_ioctl_set_pci_irq_mask(struct kvm *kvm,
+		struct kvm_assigned_pci_dev *assigned_dev)
+{
+	int r = 0;
+	struct kvm_assigned_dev_kernel *match;
+
+	mutex_lock(&kvm->lock);
+
+	match = kvm_find_assigned_dev(&kvm->arch.assigned_dev_head,
+				      assigned_dev->assigned_dev_id);
+	if (!match) {
+		r = -ENODEV;
+		goto out;
+	}
+
+	spin_lock(&match->intx_mask_lock);
+
+	match->flags &= ~KVM_DEV_ASSIGN_MASK_INTX;
+	match->flags |= assigned_dev->flags & KVM_DEV_ASSIGN_MASK_INTX;
+
+	if (assigned_dev->flags & KVM_DEV_ASSIGN_MASK_INTX) {
+		kvm_set_irq(match->kvm, match->irq_source_id,
+			    match->guest_irq, 0);
+		/*
+		 * Masking at hardware-level is performed on demand, i.e. when
+		 * an IRQ actually arrives at the host.
+		 */
+	} else {
+		/*
+		 * Unmask the IRQ line. It may have been masked meanwhile if
+		 * we aren't using PCI 2.3 INTx masking on the host side.
+		 */
+		spin_lock_irq(&match->intx_lock);
+		if (match->intx_state == KVM_INTX_LINE_DISABLED) {
+			enable_irq(match->host_irq);
+			match->intx_state = KVM_INTX_ENABLED;
+		}
+		spin_unlock_irq(&match->intx_lock);
+	}
+
+	spin_unlock(&match->intx_mask_lock);
+
+out:
+	mutex_unlock(&kvm->lock);
+	return r;
+}
+
 long kvm_vm_ioctl_assigned_device(struct kvm *kvm, unsigned ioctl,
 				  unsigned long arg)
 {
@@ -783,6 +1044,15 @@ long kvm_vm_ioctl_assigned_device(struct kvm *kvm, unsigned ioctl,
 		break;
 	}
 #endif
+	case KVM_ASSIGN_SET_INTX_MASK: {
+		struct kvm_assigned_pci_dev assigned_dev;
+
+		r = -EFAULT;
+		if (copy_from_user(&assigned_dev, argp, sizeof assigned_dev))
+			goto out;
+		r = kvm_vm_ioctl_set_pci_irq_mask(kvm, &assigned_dev);
+		break;
+	}
 	default:
 		r = -ENOTTY;
 		break;
-- 
1.7.1

Re: [PATCH v2 4/4] KVM: Allow host IRQ sharing for passed-through PCI 2.3 devices

[Avi Kivity]

On 12/12/2010 01:22 PM, Jan Kiszka wrote:
> From: Jan Kiszka<jan.kiszka@siemens.com>
>
> 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.
>

Looks fine.

> +		ret =IRQ_NONE;
> +

Danger, whitespace error detected.  Initiating self-destruct sequence.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH v2 0/4] KVM & genirq: Enable adaptive IRQ sharing for passed-through devices

[Michael S. Tsirkin]

On Sun, Dec 12, 2010 at 12:22:40PM +0100, Jan Kiszka wrote:
> The result may look simpler on first glance than v1, but it comes with
> more subtle race scenarios IMO. I thought them through, hopefully
> catching all, but I would appreciate any skeptical review.

Thought about the races till my head hurt, and yes, they
all seem to be handled correctly. FWIW

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

Re: [PATCH v2 0/4] KVM & genirq: Enable adaptive IRQ sharing for passed-through devices

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 559 bytes --]

Am 13.12.2010 11:10, Michael S. Tsirkin wrote:
> On Sun, Dec 12, 2010 at 12:22:40PM +0100, Jan Kiszka wrote:
>> The result may look simpler on first glance than v1, but it comes with
>> more subtle race scenarios IMO. I thought them through, hopefully
>> catching all, but I would appreciate any skeptical review.
> 
> Thought about the races till my head hurt, and yes, they
> all seem to be handled correctly. FWIW

Ouch, I'm endlessly sorry for causing this pain.

> 
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> 

Thanks!

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]