[PATCHv3 RFC 0/2] kvm: direct msix injection

[PATCHv3 RFC 0/2] kvm: direct msix injection

[Michael S. Tsirkin]

We can deliver certain interrupts, notably MSIX,
from atomic context.
Here's an untested patch to do this (compiled only).

Changes from v2:
Don't inject broadcast interrupts directly
Changes from v1:
Tried to address comments from v1, except unifying
with kvm_set_irq: passing flags to it looks too ugly.
Added a comment.

Jan, you said you can test this?


Michael S. Tsirkin (2):
  kvm: implement kvm_set_msi_inatomic
  kvm: deliver msix interrupts from irq handler

 include/linux/kvm_host.h |  3 ++
 virt/kvm/assigned-dev.c  | 31 ++++++++++++++++++--
 virt/kvm/irq_comm.c      | 75 ++++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 102 insertions(+), 7 deletions(-)

-- 
MST

[PATCHv3 RFC 1/2] kvm: implement kvm_set_msi_inatomic

[Michael S. Tsirkin]

We do not want a potential broadcast to all VCPUs to run in
a host IRQ handler. Implement an API that sends an MSI
interrupt but only if it's safe from interrupt context,
that is if it is a unicast.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/linux/kvm_host.h |  2 ++
 virt/kvm/irq_comm.c      | 37 ++++++++++++++++++++++++++++++++-----
 2 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index c446435..ddb476a 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -618,6 +618,8 @@ void kvm_get_intr_delivery_bitmask(struct kvm_ioapic *ioapic,
 int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level);
 int kvm_set_msi(struct kvm_kernel_irq_routing_entry *irq_entry, struct kvm *kvm,
 		int irq_source_id, int level);
+int kvm_set_msi_inatomic(struct kvm_kernel_irq_routing_entry *irq_entry,
+			 struct kvm *kvm);
 void kvm_notify_acked_irq(struct kvm *kvm, unsigned irqchip, unsigned pin);
 void kvm_register_irq_ack_notifier(struct kvm *kvm,
 				   struct kvm_irq_ack_notifier *kian);
diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
index 5afb431..3395e03 100644
--- a/virt/kvm/irq_comm.c
+++ b/virt/kvm/irq_comm.c
@@ -114,13 +114,20 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
 	return r;
 }
 
-int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
-		struct kvm *kvm, int irq_source_id, int level)
+static bool kvm_msi_is_multicast(unsigned dest, int dest_mode)
 {
-	struct kvm_lapic_irq irq;
+	if (dest_mode == 0)
+		/* Physical mode. */
+		return dest == 0xff;
+	else
+		/* Logical mode. */
+		return dest & (dest - 1);
+}
 
-	if (!level)
-		return -1;
+static int __kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
+			 struct kvm *kvm, bool noblock)
+{
+	struct kvm_lapic_irq irq;
 
 	trace_kvm_msi_set_irq(e->msi.address_lo, e->msi.data);
 
@@ -134,10 +141,30 @@ int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
 	irq.level = 1;
 	irq.shorthand = 0;
 
+	/* Multicast MSI doesn't really block but might take a long time. */
+	if (unlikely(noblock && kvm_msi_is_multicast(irq.dest_id,
+						     irq.delivery_mode)))
+		return -EWOULDBLOCK;
+
 	/* TODO Deal with RH bit of MSI message address */
 	return kvm_irq_delivery_to_apic(kvm, NULL, &irq);
 }
 
+int kvm_set_msi_inatomic(struct kvm_kernel_irq_routing_entry *e,
+			 struct kvm *kvm)
+{
+	return __kvm_set_msi(e, kvm, true);
+}
+
+int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
+			 struct kvm *kvm, int irq_source_id, int level)
+{
+	if (unlikely(!level))
+		return -1;
+
+	return __kvm_set_msi(e, kvm, false);
+}
+
 int kvm_send_userspace_msi(struct kvm *kvm, struct kvm_msi *msi)
 {
 	struct kvm_kernel_irq_routing_entry route;
-- 
MST

[PATCHv3 RFC 2/2] kvm: deliver msix interrupts from irq handler

[Michael S. Tsirkin]

We can deliver certain interrupts, notably MSIX,
from atomic context.  Add a new API kvm_set_irq_inatomic,
that does exactly that, and use it to implement
an irq handler for msi.

This reduces the pressure on scheduler in case
where host and guest irq share a host cpu.

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

Untested.
Changes from v1:
Tried to address comments from v1.
Not all suggestions from v1 were addressed yet, but run
out of time and weekend is starting.
Posting so Jan can test if he likes.
---
 include/linux/kvm_host.h |  1 +
 virt/kvm/assigned-dev.c  | 31 +++++++++++++++++++++++++++++--
 virt/kvm/irq_comm.c      | 38 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 68 insertions(+), 2 deletions(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index ddb476a..ed6e5aa 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -616,6 +616,7 @@ void kvm_get_intr_delivery_bitmask(struct kvm_ioapic *ioapic,
 				   unsigned long *deliver_bitmask);
 #endif
 int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level);
+int kvm_set_irq_inatomic(struct kvm *kvm, int irq_source_id, u32 irq, int level);
 int kvm_set_msi(struct kvm_kernel_irq_routing_entry *irq_entry, struct kvm *kvm,
 		int irq_source_id, int level);
 int kvm_set_msi_inatomic(struct kvm_kernel_irq_routing_entry *irq_entry,
diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
index 01f572c..c5c332c 100644
--- a/virt/kvm/assigned-dev.c
+++ b/virt/kvm/assigned-dev.c
@@ -117,6 +117,23 @@ static irqreturn_t kvm_assigned_dev_thread_msi(int irq, void *dev_id)
 #endif
 
 #ifdef __KVM_HAVE_MSIX
+static irqreturn_t kvm_assigned_dev_msix(int irq, void *dev_id)
+{
+	struct kvm_assigned_dev_kernel *assigned_dev = dev_id;
+	int index = find_index_from_host_irq(assigned_dev, irq);
+	u32 vector;
+	int ret = 0;
+
+	if (index >= 0) {
+		vector = assigned_dev->guest_msix_entries[index].vector;
+		ret = kvm_set_irq_inatomic(assigned_dev->kvm,
+					   assigned_dev->irq_source_id,
+					   vector, 1);
+	}
+
+	return unlikely(ret == -EWOULDBLOCK) ? IRQ_WAKE_THREAD : IRQ_HANDLED;
+}
+
 static irqreturn_t kvm_assigned_dev_thread_msix(int irq, void *dev_id)
 {
 	struct kvm_assigned_dev_kernel *assigned_dev = dev_id;
@@ -334,6 +351,15 @@ static int assigned_device_enable_host_intx(struct kvm *kvm,
 }
 
 #ifdef __KVM_HAVE_MSI
+static irqreturn_t kvm_assigned_dev_msi(int irq, void *dev_id)
+{
+	struct kvm_assigned_dev_kernel *assigned_dev = dev_id;
+	int ret = kvm_set_irq_inatomic(assigned_dev->kvm,
+				       assigned_dev->irq_source_id,
+				       assigned_dev->guest_irq, 1);
+	return unlikely(ret == -EWOULDBLOCK) ? IRQ_WAKE_THREAD : IRQ_HANDLED;
+}
+
 static int assigned_device_enable_host_msi(struct kvm *kvm,
 					   struct kvm_assigned_dev_kernel *dev)
 {
@@ -346,7 +372,7 @@ static int assigned_device_enable_host_msi(struct kvm *kvm,
 	}
 
 	dev->host_irq = dev->dev->irq;
-	if (request_threaded_irq(dev->host_irq, NULL,
+	if (request_threaded_irq(dev->host_irq, kvm_assigned_dev_msi,
 				 kvm_assigned_dev_thread_msi, 0,
 				 dev->irq_name, dev)) {
 		pci_disable_msi(dev->dev);
@@ -374,7 +400,8 @@ static int assigned_device_enable_host_msix(struct kvm *kvm,
 
 	for (i = 0; i < dev->entries_nr; i++) {
 		r = request_threaded_irq(dev->host_msix_entries[i].vector,
-					 NULL, kvm_assigned_dev_thread_msix,
+					 kvm_assigned_dev_msix,
+					 kvm_assigned_dev_thread_msix,
 					 0, dev->irq_name, dev);
 		if (r)
 			goto err;
diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
index 3395e03..9d11551 100644
--- a/virt/kvm/irq_comm.c
+++ b/virt/kvm/irq_comm.c
@@ -217,6 +217,44 @@ int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level)
 	return ret;
 }
 
+/*
+ * Deliver an IRQ in an atomic context if we can, or return a failure,
+ * user can retry in a process context.
+ * Return value:
+ *  -EWOULDBLOCK - Can't deliver in atomic context: retry in a process context.
+ *  Other values - No need to retry.
+ */
+int kvm_set_irq_inatomic(struct kvm *kvm, int irq_source_id, u32 irq, int level)
+{
+	struct kvm_kernel_irq_routing_entry *e;
+	int ret = -EINVAL;
+	struct kvm_irq_routing_table *irq_rt;
+	struct hlist_node *n;
+
+	trace_kvm_set_irq(irq, level, irq_source_id);
+
+	/*
+	 * We know MSI are safe in interrupt context. They are also
+	 * easy as there's a single routing entry for these GSIs.
+	 * So only handle MSI in an atomic context, for now.
+	 *
+	 * This shares some code with kvm_set_irq: this
+	 * version is optimized for a single entry MSI only case.
+	 */
+	rcu_read_lock();
+	irq_rt = rcu_dereference(kvm->irq_routing);
+	if (irq < irq_rt->nr_rt_entries)
+		hlist_for_each_entry(e, n, &irq_rt->map[irq], link) {
+			if (likely(e->type == KVM_IRQ_ROUTING_MSI))
+				ret = kvm_set_msi_inatomic(e, kvm);
+			else
+				ret = -EWOULDBLOCK;
+			break;
+		}
+	rcu_read_unlock();
+	return ret;
+}
+
 void kvm_notify_acked_irq(struct kvm *kvm, unsigned irqchip, unsigned pin)
 {
 	struct kvm_irq_ack_notifier *kian;
-- 
MST

Re: [PATCHv3 RFC 0/2] kvm: direct msix injection

[Marcelo Tosatti]

On Mon, Jun 11, 2012 at 02:19:17PM +0300, Michael S. Tsirkin wrote:
> We can deliver certain interrupts, notably MSIX,
> from atomic context.
> Here's an untested patch to do this (compiled only).
> 
> Changes from v2:
> Don't inject broadcast interrupts directly
> Changes from v1:
> Tried to address comments from v1, except unifying
> with kvm_set_irq: passing flags to it looks too ugly.
> Added a comment.
> 
> Jan, you said you can test this?

Looks fine to me (except the unlikely). Avi/Gleb?

Re: [PATCHv3 RFC 0/2] kvm: direct msix injection

[Michael S. Tsirkin]

On Tue, Jun 12, 2012 at 08:07:03PM -0300, Marcelo Tosatti wrote:
> On Mon, Jun 11, 2012 at 02:19:17PM +0300, Michael S. Tsirkin wrote:
> > We can deliver certain interrupts, notably MSIX,
> > from atomic context.
> > Here's an untested patch to do this (compiled only).
> > 
> > Changes from v2:
> > Don't inject broadcast interrupts directly
> > Changes from v1:
> > Tried to address comments from v1, except unifying
> > with kvm_set_irq: passing flags to it looks too ugly.
> > Added a comment.
> > 
> > Jan, you said you can test this?
> 
> Looks fine to me (except the unlikely). Avi/Gleb?

Which of the unlikely do you mean?
Most of them cover the case where msix is a broadcast
or where level is 0 for msi. This never triggered
in my testing. So the unlikely seems justified.



-- 
MST

Re: [PATCHv3 RFC 1/2] kvm: implement kvm_set_msi_inatomic

[Gleb Natapov]

On Mon, Jun 11, 2012 at 02:19:22PM +0300, Michael S. Tsirkin wrote:
> We do not want a potential broadcast to all VCPUs to run in
> a host IRQ handler. Implement an API that sends an MSI
> interrupt but only if it's safe from interrupt context,
> that is if it is a unicast.
> 
We will still iterate over all vcpus, we may as well deliver to them. Or
you want to avoid a lot of vcpu kicks?

> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  include/linux/kvm_host.h |  2 ++
>  virt/kvm/irq_comm.c      | 37 ++++++++++++++++++++++++++++++++-----
>  2 files changed, 34 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index c446435..ddb476a 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -618,6 +618,8 @@ void kvm_get_intr_delivery_bitmask(struct kvm_ioapic *ioapic,
>  int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level);
>  int kvm_set_msi(struct kvm_kernel_irq_routing_entry *irq_entry, struct kvm *kvm,
>  		int irq_source_id, int level);
> +int kvm_set_msi_inatomic(struct kvm_kernel_irq_routing_entry *irq_entry,
> +			 struct kvm *kvm);
>  void kvm_notify_acked_irq(struct kvm *kvm, unsigned irqchip, unsigned pin);
>  void kvm_register_irq_ack_notifier(struct kvm *kvm,
>  				   struct kvm_irq_ack_notifier *kian);
> diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
> index 5afb431..3395e03 100644
> --- a/virt/kvm/irq_comm.c
> +++ b/virt/kvm/irq_comm.c
> @@ -114,13 +114,20 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
>  	return r;
>  }
>  
> -int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
> -		struct kvm *kvm, int irq_source_id, int level)
> +static bool kvm_msi_is_multicast(unsigned dest, int dest_mode)
>  {
> -	struct kvm_lapic_irq irq;
> +	if (dest_mode == 0)
> +		/* Physical mode. */
> +		return dest == 0xff;
> +	else
> +		/* Logical mode. */
> +		return dest & (dest - 1);
> +}
>  
> -	if (!level)
> -		return -1;
> +static int __kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
> +			 struct kvm *kvm, bool noblock)
> +{
> +	struct kvm_lapic_irq irq;
>  
>  	trace_kvm_msi_set_irq(e->msi.address_lo, e->msi.data);
>  
> @@ -134,10 +141,30 @@ int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
>  	irq.level = 1;
>  	irq.shorthand = 0;
>  
> +	/* Multicast MSI doesn't really block but might take a long time. */
> +	if (unlikely(noblock && kvm_msi_is_multicast(irq.dest_id,
> +						     irq.delivery_mode)))
delivery_mode? Should be dest_mode. But you probably need to check that
delivery_mode is not ExtINT either.

> +		return -EWOULDBLOCK;
> +
>  	/* TODO Deal with RH bit of MSI message address */
>  	return kvm_irq_delivery_to_apic(kvm, NULL, &irq);
>  }
>  
> +int kvm_set_msi_inatomic(struct kvm_kernel_irq_routing_entry *e,
> +			 struct kvm *kvm)
> +{
> +	return __kvm_set_msi(e, kvm, true);
> +}
> +
> +int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
> +			 struct kvm *kvm, int irq_source_id, int level)
> +{
> +	if (unlikely(!level))
> +		return -1;
> +
> +	return __kvm_set_msi(e, kvm, false);
> +}
> +
>  int kvm_send_userspace_msi(struct kvm *kvm, struct kvm_msi *msi)
>  {
>  	struct kvm_kernel_irq_routing_entry route;
> -- 
> MST
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
			Gleb.

Re: [PATCHv3 RFC 0/2] kvm: direct msix injection

[Avi Kivity]

On 06/13/2012 02:07 AM, Marcelo Tosatti wrote:
> On Mon, Jun 11, 2012 at 02:19:17PM +0300, Michael S. Tsirkin wrote:
>> We can deliver certain interrupts, notably MSIX,
>> from atomic context.
>> Here's an untested patch to do this (compiled only).
>> 
>> Changes from v2:
>> Don't inject broadcast interrupts directly
>> Changes from v1:
>> Tried to address comments from v1, except unifying
>> with kvm_set_irq: passing flags to it looks too ugly.
>> Added a comment.
>> 
>> Jan, you said you can test this?
> 
> Looks fine to me (except the unlikely). Avi/Gleb?

I dislike the vcpu loop in atomic context.  Some crazies keep increasing
the maximum vcpu count.

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

Re: [PATCHv3 RFC 1/2] kvm: implement kvm_set_msi_inatomic

[Michael S. Tsirkin]

On Wed, Jun 13, 2012 at 04:01:24PM +0300, Gleb Natapov wrote:
> On Mon, Jun 11, 2012 at 02:19:22PM +0300, Michael S. Tsirkin wrote:
> > We do not want a potential broadcast to all VCPUs to run in
> > a host IRQ handler. Implement an API that sends an MSI
> > interrupt but only if it's safe from interrupt context,
> > that is if it is a unicast.
> > 
> We will still iterate over all vcpus, we may as well deliver to them. Or
> you want to avoid a lot of vcpu kicks?

This is an intermediate stage (thus RFC) for adding a cache:
we can't cache if it's a broadcast.

> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  include/linux/kvm_host.h |  2 ++
> >  virt/kvm/irq_comm.c      | 37 ++++++++++++++++++++++++++++++++-----
> >  2 files changed, 34 insertions(+), 5 deletions(-)
> > 
> > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > index c446435..ddb476a 100644
> > --- a/include/linux/kvm_host.h
> > +++ b/include/linux/kvm_host.h
> > @@ -618,6 +618,8 @@ void kvm_get_intr_delivery_bitmask(struct kvm_ioapic *ioapic,
> >  int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level);
> >  int kvm_set_msi(struct kvm_kernel_irq_routing_entry *irq_entry, struct kvm *kvm,
> >  		int irq_source_id, int level);
> > +int kvm_set_msi_inatomic(struct kvm_kernel_irq_routing_entry *irq_entry,
> > +			 struct kvm *kvm);
> >  void kvm_notify_acked_irq(struct kvm *kvm, unsigned irqchip, unsigned pin);
> >  void kvm_register_irq_ack_notifier(struct kvm *kvm,
> >  				   struct kvm_irq_ack_notifier *kian);
> > diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
> > index 5afb431..3395e03 100644
> > --- a/virt/kvm/irq_comm.c
> > +++ b/virt/kvm/irq_comm.c
> > @@ -114,13 +114,20 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
> >  	return r;
> >  }
> >  
> > -int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
> > -		struct kvm *kvm, int irq_source_id, int level)
> > +static bool kvm_msi_is_multicast(unsigned dest, int dest_mode)
> >  {
> > -	struct kvm_lapic_irq irq;
> > +	if (dest_mode == 0)
> > +		/* Physical mode. */
> > +		return dest == 0xff;
> > +	else
> > +		/* Logical mode. */
> > +		return dest & (dest - 1);
> > +}
> >  
> > -	if (!level)
> > -		return -1;
> > +static int __kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
> > +			 struct kvm *kvm, bool noblock)
> > +{
> > +	struct kvm_lapic_irq irq;
> >  
> >  	trace_kvm_msi_set_irq(e->msi.address_lo, e->msi.data);
> >  
> > @@ -134,10 +141,30 @@ int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
> >  	irq.level = 1;
> >  	irq.shorthand = 0;
> >  
> > +	/* Multicast MSI doesn't really block but might take a long time. */
> > +	if (unlikely(noblock && kvm_msi_is_multicast(irq.dest_id,
> > +						     irq.delivery_mode)))
> delivery_mode? Should be dest_mode. But you probably need to check that
> delivery_mode is not ExtINT either.
> 
> > +		return -EWOULDBLOCK;
> > +
> >  	/* TODO Deal with RH bit of MSI message address */
> >  	return kvm_irq_delivery_to_apic(kvm, NULL, &irq);
> >  }
> >  
> > +int kvm_set_msi_inatomic(struct kvm_kernel_irq_routing_entry *e,
> > +			 struct kvm *kvm)
> > +{
> > +	return __kvm_set_msi(e, kvm, true);
> > +}
> > +
> > +int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
> > +			 struct kvm *kvm, int irq_source_id, int level)
> > +{
> > +	if (unlikely(!level))
> > +		return -1;
> > +
> > +	return __kvm_set_msi(e, kvm, false);
> > +}
> > +
> >  int kvm_send_userspace_msi(struct kvm *kvm, struct kvm_msi *msi)
> >  {
> >  	struct kvm_kernel_irq_routing_entry route;
> > -- 
> > MST
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> --
> 			Gleb.

Re: [PATCHv3 RFC 1/2] kvm: implement kvm_set_msi_inatomic

[Gleb Natapov]

On Wed, Jun 13, 2012 at 06:59:50PM +0300, Michael S. Tsirkin wrote:
> On Wed, Jun 13, 2012 at 04:01:24PM +0300, Gleb Natapov wrote:
> > On Mon, Jun 11, 2012 at 02:19:22PM +0300, Michael S. Tsirkin wrote:
> > > We do not want a potential broadcast to all VCPUs to run in
> > > a host IRQ handler. Implement an API that sends an MSI
> > > interrupt but only if it's safe from interrupt context,
> > > that is if it is a unicast.
> > > 
> > We will still iterate over all vcpus, we may as well deliver to them. Or
> > you want to avoid a lot of vcpu kicks?
> 
> This is an intermediate stage (thus RFC) for adding a cache:
> we can't cache if it's a broadcast.
> 
You skipped my comment about the code  bellow :)

> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > >  include/linux/kvm_host.h |  2 ++
> > >  virt/kvm/irq_comm.c      | 37 ++++++++++++++++++++++++++++++++-----
> > >  2 files changed, 34 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > > index c446435..ddb476a 100644
> > > --- a/include/linux/kvm_host.h
> > > +++ b/include/linux/kvm_host.h
> > > @@ -618,6 +618,8 @@ void kvm_get_intr_delivery_bitmask(struct kvm_ioapic *ioapic,
> > >  int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level);
> > >  int kvm_set_msi(struct kvm_kernel_irq_routing_entry *irq_entry, struct kvm *kvm,
> > >  		int irq_source_id, int level);
> > > +int kvm_set_msi_inatomic(struct kvm_kernel_irq_routing_entry *irq_entry,
> > > +			 struct kvm *kvm);
> > >  void kvm_notify_acked_irq(struct kvm *kvm, unsigned irqchip, unsigned pin);
> > >  void kvm_register_irq_ack_notifier(struct kvm *kvm,
> > >  				   struct kvm_irq_ack_notifier *kian);
> > > diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
> > > index 5afb431..3395e03 100644
> > > --- a/virt/kvm/irq_comm.c
> > > +++ b/virt/kvm/irq_comm.c
> > > @@ -114,13 +114,20 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
> > >  	return r;
> > >  }
> > >  
> > > -int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
> > > -		struct kvm *kvm, int irq_source_id, int level)
> > > +static bool kvm_msi_is_multicast(unsigned dest, int dest_mode)
> > >  {
> > > -	struct kvm_lapic_irq irq;
> > > +	if (dest_mode == 0)
> > > +		/* Physical mode. */
> > > +		return dest == 0xff;
> > > +	else
> > > +		/* Logical mode. */
> > > +		return dest & (dest - 1);
> > > +}
> > >  
> > > -	if (!level)
> > > -		return -1;
> > > +static int __kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
> > > +			 struct kvm *kvm, bool noblock)
> > > +{
> > > +	struct kvm_lapic_irq irq;
> > >  
> > >  	trace_kvm_msi_set_irq(e->msi.address_lo, e->msi.data);
> > >  
> > > @@ -134,10 +141,30 @@ int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
> > >  	irq.level = 1;
> > >  	irq.shorthand = 0;
> > >  
> > > +	/* Multicast MSI doesn't really block but might take a long time. */
> > > +	if (unlikely(noblock && kvm_msi_is_multicast(irq.dest_id,
> > > +						     irq.delivery_mode)))
> > delivery_mode? Should be dest_mode. But you probably need to check that
> > delivery_mode is not ExtINT either.
> > 
> > > +		return -EWOULDBLOCK;
> > > +
> > >  	/* TODO Deal with RH bit of MSI message address */
> > >  	return kvm_irq_delivery_to_apic(kvm, NULL, &irq);
> > >  }
> > >  
> > > +int kvm_set_msi_inatomic(struct kvm_kernel_irq_routing_entry *e,
> > > +			 struct kvm *kvm)
> > > +{
> > > +	return __kvm_set_msi(e, kvm, true);
> > > +}
> > > +
> > > +int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
> > > +			 struct kvm *kvm, int irq_source_id, int level)
> > > +{
> > > +	if (unlikely(!level))
> > > +		return -1;
> > > +
> > > +	return __kvm_set_msi(e, kvm, false);
> > > +}
> > > +
> > >  int kvm_send_userspace_msi(struct kvm *kvm, struct kvm_msi *msi)
> > >  {
> > >  	struct kvm_kernel_irq_routing_entry route;
> > > -- 
> > > MST
> > > 
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> > --
> > 			Gleb.

--
			Gleb.

Re: [PATCHv3 RFC 1/2] kvm: implement kvm_set_msi_inatomic

[Michael S. Tsirkin]

On Wed, Jun 13, 2012 at 07:01:01PM +0300, Gleb Natapov wrote:
> On Wed, Jun 13, 2012 at 06:59:50PM +0300, Michael S. Tsirkin wrote:
> > On Wed, Jun 13, 2012 at 04:01:24PM +0300, Gleb Natapov wrote:
> > > On Mon, Jun 11, 2012 at 02:19:22PM +0300, Michael S. Tsirkin wrote:
> > > > We do not want a potential broadcast to all VCPUs to run in
> > > > a host IRQ handler. Implement an API that sends an MSI
> > > > interrupt but only if it's safe from interrupt context,
> > > > that is if it is a unicast.
> > > > 
> > > We will still iterate over all vcpus, we may as well deliver to them. Or
> > > you want to avoid a lot of vcpu kicks?
> > 
> > This is an intermediate stage (thus RFC) for adding a cache:
> > we can't cache if it's a broadcast.
> > 
> You skipped my comment about the code  bellow :)

Ouch. I missed it.
Thanks for pointing out and sorry.

> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > ---
> > > >  include/linux/kvm_host.h |  2 ++
> > > >  virt/kvm/irq_comm.c      | 37 ++++++++++++++++++++++++++++++++-----
> > > >  2 files changed, 34 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > > > index c446435..ddb476a 100644
> > > > --- a/include/linux/kvm_host.h
> > > > +++ b/include/linux/kvm_host.h
> > > > @@ -618,6 +618,8 @@ void kvm_get_intr_delivery_bitmask(struct kvm_ioapic *ioapic,
> > > >  int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level);
> > > >  int kvm_set_msi(struct kvm_kernel_irq_routing_entry *irq_entry, struct kvm *kvm,
> > > >  		int irq_source_id, int level);
> > > > +int kvm_set_msi_inatomic(struct kvm_kernel_irq_routing_entry *irq_entry,
> > > > +			 struct kvm *kvm);
> > > >  void kvm_notify_acked_irq(struct kvm *kvm, unsigned irqchip, unsigned pin);
> > > >  void kvm_register_irq_ack_notifier(struct kvm *kvm,
> > > >  				   struct kvm_irq_ack_notifier *kian);
> > > > diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
> > > > index 5afb431..3395e03 100644
> > > > --- a/virt/kvm/irq_comm.c
> > > > +++ b/virt/kvm/irq_comm.c
> > > > @@ -114,13 +114,20 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
> > > >  	return r;
> > > >  }
> > > >  
> > > > -int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
> > > > -		struct kvm *kvm, int irq_source_id, int level)
> > > > +static bool kvm_msi_is_multicast(unsigned dest, int dest_mode)
> > > >  {
> > > > -	struct kvm_lapic_irq irq;
> > > > +	if (dest_mode == 0)
> > > > +		/* Physical mode. */
> > > > +		return dest == 0xff;
> > > > +	else
> > > > +		/* Logical mode. */
> > > > +		return dest & (dest - 1);
> > > > +}
> > > >  
> > > > -	if (!level)
> > > > -		return -1;
> > > > +static int __kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
> > > > +			 struct kvm *kvm, bool noblock)
> > > > +{
> > > > +	struct kvm_lapic_irq irq;
> > > >  
> > > >  	trace_kvm_msi_set_irq(e->msi.address_lo, e->msi.data);
> > > >  
> > > > @@ -134,10 +141,30 @@ int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
> > > >  	irq.level = 1;
> > > >  	irq.shorthand = 0;
> > > >  
> > > > +	/* Multicast MSI doesn't really block but might take a long time. */
> > > > +	if (unlikely(noblock && kvm_msi_is_multicast(irq.dest_id,
> > > > +						     irq.delivery_mode)))
> > > delivery_mode? Should be dest_mode.

Yes. Good catch, thanks.

> But you probably need to check that
> > > delivery_mode is not ExtINT either.

It does not look like anything happens with ExtInt
if you try to trigger it from MSI.

> > > 
> > > > +		return -EWOULDBLOCK;
> > > > +
> > > >  	/* TODO Deal with RH bit of MSI message address */
> > > >  	return kvm_irq_delivery_to_apic(kvm, NULL, &irq);
> > > >  }
> > > >  
> > > > +int kvm_set_msi_inatomic(struct kvm_kernel_irq_routing_entry *e,
> > > > +			 struct kvm *kvm)
> > > > +{
> > > > +	return __kvm_set_msi(e, kvm, true);
> > > > +}
> > > > +
> > > > +int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
> > > > +			 struct kvm *kvm, int irq_source_id, int level)
> > > > +{
> > > > +	if (unlikely(!level))
> > > > +		return -1;
> > > > +
> > > > +	return __kvm_set_msi(e, kvm, false);
> > > > +}
> > > > +
> > > >  int kvm_send_userspace_msi(struct kvm *kvm, struct kvm_msi *msi)
> > > >  {
> > > >  	struct kvm_kernel_irq_routing_entry route;
> > > > -- 
> > > > MST
> > > > 
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > > > the body of a message to majordomo@vger.kernel.org
> > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > 
> > > --
> > > 			Gleb.
> 
> --
> 			Gleb.

Re: [PATCHv3 RFC 1/2] kvm: implement kvm_set_msi_inatomic

[Gleb Natapov]

On Wed, Jun 13, 2012 at 07:12:26PM +0300, Michael S. Tsirkin wrote:
> > > > > @@ -134,10 +141,30 @@ int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
> > > > >  	irq.level = 1;
> > > > >  	irq.shorthand = 0;
> > > > >  
> > > > > +	/* Multicast MSI doesn't really block but might take a long time. */
> > > > > +	if (unlikely(noblock && kvm_msi_is_multicast(irq.dest_id,
> > > > > +						     irq.delivery_mode)))
> > > > delivery_mode? Should be dest_mode.
> 
> Yes. Good catch, thanks.
> 
> > But you probably need to check that
> > > > delivery_mode is not ExtINT either.
> 
> It does not look like anything happens with ExtInt
> if you try to trigger it from MSI.
> 
Currently no, but it should appear as if interrupt comes from PIC.
I wouldn't allow anything but fixed mode here just to be on a safe side.
Lowest prio will have to loop even after introducing irq cache.

--
			Gleb.

Re: [PATCHv3 RFC 0/2] kvm: direct msix injection

[Jan Kiszka]

On 2012-06-11 13:19, Michael S. Tsirkin wrote:
> We can deliver certain interrupts, notably MSIX,
> from atomic context.
> Here's an untested patch to do this (compiled only).
> 
> Changes from v2:
> Don't inject broadcast interrupts directly
> Changes from v1:
> Tried to address comments from v1, except unifying
> with kvm_set_irq: passing flags to it looks too ugly.
> Added a comment.
> 
> Jan, you said you can test this?
> 
> 
> Michael S. Tsirkin (2):
>   kvm: implement kvm_set_msi_inatomic
>   kvm: deliver msix interrupts from irq handler
> 
>  include/linux/kvm_host.h |  3 ++
>  virt/kvm/assigned-dev.c  | 31 ++++++++++++++++++--
>  virt/kvm/irq_comm.c      | 75 ++++++++++++++++++++++++++++++++++++++++++++----
>  3 files changed, 102 insertions(+), 7 deletions(-)
> 

Finally-tested-by: Jan Kiszka <jan.kiszka@siemens.com>

Thanks,
Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCHv3 RFC 0/2] kvm: direct msix injection

[Alex Williamson]

On Mon, 2012-06-25 at 11:32 +0200, Jan Kiszka wrote:
> On 2012-06-11 13:19, Michael S. Tsirkin wrote:
> > We can deliver certain interrupts, notably MSIX,
> > from atomic context.
> > Here's an untested patch to do this (compiled only).
> > 
> > Changes from v2:
> > Don't inject broadcast interrupts directly
> > Changes from v1:
> > Tried to address comments from v1, except unifying
> > with kvm_set_irq: passing flags to it looks too ugly.
> > Added a comment.
> > 
> > Jan, you said you can test this?
> > 
> > 
> > Michael S. Tsirkin (2):
> >   kvm: implement kvm_set_msi_inatomic
> >   kvm: deliver msix interrupts from irq handler
> > 
> >  include/linux/kvm_host.h |  3 ++
> >  virt/kvm/assigned-dev.c  | 31 ++++++++++++++++++--
> >  virt/kvm/irq_comm.c      | 75 ++++++++++++++++++++++++++++++++++++++++++++----
> >  3 files changed, 102 insertions(+), 7 deletions(-)
> > 
> 
> Finally-tested-by: Jan Kiszka <jan.kiszka@siemens.com>

Michael, we need either this or the simple oneshot patch to get device
assignment working again for 3.5.  Are you planning to push this for
3.5?  Thanks,

Alex

RE: [PATCHv3 RFC 0/2] kvm: direct msix injection

[Hao, Xudong]

Hi, Michael/Alex, do you have progress for device assignment issue fixing? 
https://bugzilla.kernel.org/show_bug.cgi?id=43328

Thanks,
-Xudong

> -----Original Message-----
> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] On
> Behalf Of Alex Williamson
> Sent: Tuesday, July 03, 2012 1:08 AM
> To: Jan Kiszka
> Cc: Michael S. Tsirkin; kvm@vger.kernel.org
> Subject: Re: [PATCHv3 RFC 0/2] kvm: direct msix injection
> 
> On Mon, 2012-06-25 at 11:32 +0200, Jan Kiszka wrote:
> > On 2012-06-11 13:19, Michael S. Tsirkin wrote:
> > > We can deliver certain interrupts, notably MSIX,
> > > from atomic context.
> > > Here's an untested patch to do this (compiled only).
> > >
> > > Changes from v2:
> > > Don't inject broadcast interrupts directly
> > > Changes from v1:
> > > Tried to address comments from v1, except unifying
> > > with kvm_set_irq: passing flags to it looks too ugly.
> > > Added a comment.
> > >
> > > Jan, you said you can test this?
> > >
> > >
> > > Michael S. Tsirkin (2):
> > >   kvm: implement kvm_set_msi_inatomic
> > >   kvm: deliver msix interrupts from irq handler
> > >
> > >  include/linux/kvm_host.h |  3 ++
> > >  virt/kvm/assigned-dev.c  | 31 ++++++++++++++++++--
> > >  virt/kvm/irq_comm.c      | 75
> ++++++++++++++++++++++++++++++++++++++++++++----
> > >  3 files changed, 102 insertions(+), 7 deletions(-)
> > >
> >
> > Finally-tested-by: Jan Kiszka <jan.kiszka@siemens.com>
> 
> Michael, we need either this or the simple oneshot patch to get device
> assignment working again for 3.5.  Are you planning to push this for
> 3.5?  Thanks,
> 
> Alex
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 RFC 0/2] kvm: direct msix injection

[Michael S. Tsirkin]

On Mon, Jul 02, 2012 at 11:08:20AM -0600, Alex Williamson wrote:
> On Mon, 2012-06-25 at 11:32 +0200, Jan Kiszka wrote:
> > On 2012-06-11 13:19, Michael S. Tsirkin wrote:
> > > We can deliver certain interrupts, notably MSIX,
> > > from atomic context.
> > > Here's an untested patch to do this (compiled only).
> > > 
> > > Changes from v2:
> > > Don't inject broadcast interrupts directly
> > > Changes from v1:
> > > Tried to address comments from v1, except unifying
> > > with kvm_set_irq: passing flags to it looks too ugly.
> > > Added a comment.
> > > 
> > > Jan, you said you can test this?
> > > 
> > > 
> > > Michael S. Tsirkin (2):
> > >   kvm: implement kvm_set_msi_inatomic
> > >   kvm: deliver msix interrupts from irq handler
> > > 
> > >  include/linux/kvm_host.h |  3 ++
> > >  virt/kvm/assigned-dev.c  | 31 ++++++++++++++++++--
> > >  virt/kvm/irq_comm.c      | 75 ++++++++++++++++++++++++++++++++++++++++++++----
> > >  3 files changed, 102 insertions(+), 7 deletions(-)
> > > 
> > 
> > Finally-tested-by: Jan Kiszka <jan.kiszka@siemens.com>
> 
> Michael, we need either this or the simple oneshot patch to get device
> assignment working again for 3.5.  Are you planning to push this for
> 3.5?  Thanks,
> 
> Alex
> 

Avi wants this reworked using an injection cache. I agree with Jan
though: oneshot looks too ugly. Just so you can make progress, can't we
add a stub handler returning IRQ_WAKE_THREAD unconditionally for now?
I.e. like the below (warning: completely untested).

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

--

diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
index b1e091a..18cc36e 100644
--- a/virt/kvm/assigned-dev.c
+++ b/virt/kvm/assigned-dev.c
@@ -334,6 +334,11 @@ static int assigned_device_enable_host_intx(struct kvm *kvm,
 }
 
 #ifdef __KVM_HAVE_MSI
+static irqreturn_t kvm_assigned_dev_msi(int irq, void *dev_id)
+{
+       return IRQ_WAKE_THREAD;
+}
+
 static int assigned_device_enable_host_msi(struct kvm *kvm,
 					   struct kvm_assigned_dev_kernel *dev)
 {
@@ -346,7 +351,7 @@ static int assigned_device_enable_host_msi(struct kvm *kvm,
 	}
 
 	dev->host_irq = dev->dev->irq;
-	if (request_threaded_irq(dev->host_irq, NULL,
+	if (request_threaded_irq(dev->host_irq, kvm_assigned_dev_msi,
 				 kvm_assigned_dev_thread_msi, 0,
 				 dev->irq_name, dev)) {
 		pci_disable_msi(dev->dev);
@@ -358,6 +363,11 @@ static int assigned_device_enable_host_msi(struct kvm *kvm,
 #endif
 
 #ifdef __KVM_HAVE_MSIX
+static irqreturn_t kvm_assigned_dev_msix(int irq, void *dev_id)
+{
+       return IRQ_WAKE_THREAD;
+}
+
 static int assigned_device_enable_host_msix(struct kvm *kvm,
 					    struct kvm_assigned_dev_kernel *dev)
 {
@@ -374,7 +384,8 @@ static int assigned_device_enable_host_msix(struct kvm *kvm,
 
 	for (i = 0; i < dev->entries_nr; i++) {
 		r = request_threaded_irq(dev->host_msix_entries[i].vector,
-					 NULL, kvm_assigned_dev_thread_msix,
+					 kvm_assigned_dev_msix,
+					 kvm_assigned_dev_thread_msix,
 					 0, dev->irq_name, dev);
 		if (r)
 			goto err;
-- 
MST

Re: [PATCHv3 RFC 0/2] kvm: direct msix injection

[Jan Kiszka]

On 2012-07-09 00:55, Michael S. Tsirkin wrote:
> On Mon, Jul 02, 2012 at 11:08:20AM -0600, Alex Williamson wrote:
>> On Mon, 2012-06-25 at 11:32 +0200, Jan Kiszka wrote:
>>> On 2012-06-11 13:19, Michael S. Tsirkin wrote:
>>>> We can deliver certain interrupts, notably MSIX,
>>>> from atomic context.
>>>> Here's an untested patch to do this (compiled only).
>>>>
>>>> Changes from v2:
>>>> Don't inject broadcast interrupts directly
>>>> Changes from v1:
>>>> Tried to address comments from v1, except unifying
>>>> with kvm_set_irq: passing flags to it looks too ugly.
>>>> Added a comment.
>>>>
>>>> Jan, you said you can test this?
>>>>
>>>>
>>>> Michael S. Tsirkin (2):
>>>>   kvm: implement kvm_set_msi_inatomic
>>>>   kvm: deliver msix interrupts from irq handler
>>>>
>>>>  include/linux/kvm_host.h |  3 ++
>>>>  virt/kvm/assigned-dev.c  | 31 ++++++++++++++++++--
>>>>  virt/kvm/irq_comm.c      | 75 ++++++++++++++++++++++++++++++++++++++++++++----
>>>>  3 files changed, 102 insertions(+), 7 deletions(-)
>>>>
>>>
>>> Finally-tested-by: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> Michael, we need either this or the simple oneshot patch to get device
>> assignment working again for 3.5.  Are you planning to push this for
>> 3.5?  Thanks,
>>
>> Alex
>>
> 
> Avi wants this reworked using an injection cache. I agree with Jan
> though: oneshot looks too ugly. Just so you can make progress, can't we
> add a stub handler returning IRQ_WAKE_THREAD unconditionally for now?
> I.e. like the below (warning: completely untested).
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> --
> 
> diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
> index b1e091a..18cc36e 100644
> --- a/virt/kvm/assigned-dev.c
> +++ b/virt/kvm/assigned-dev.c
> @@ -334,6 +334,11 @@ static int assigned_device_enable_host_intx(struct kvm *kvm,
>  }
>  
>  #ifdef __KVM_HAVE_MSI
> +static irqreturn_t kvm_assigned_dev_msi(int irq, void *dev_id)
> +{
> +       return IRQ_WAKE_THREAD;
> +}
> +
>  static int assigned_device_enable_host_msi(struct kvm *kvm,
>  					   struct kvm_assigned_dev_kernel *dev)
>  {
> @@ -346,7 +351,7 @@ static int assigned_device_enable_host_msi(struct kvm *kvm,
>  	}
>  
>  	dev->host_irq = dev->dev->irq;
> -	if (request_threaded_irq(dev->host_irq, NULL,
> +	if (request_threaded_irq(dev->host_irq, kvm_assigned_dev_msi,
>  				 kvm_assigned_dev_thread_msi, 0,
>  				 dev->irq_name, dev)) {
>  		pci_disable_msi(dev->dev);
> @@ -358,6 +363,11 @@ static int assigned_device_enable_host_msi(struct kvm *kvm,
>  #endif
>  
>  #ifdef __KVM_HAVE_MSIX
> +static irqreturn_t kvm_assigned_dev_msix(int irq, void *dev_id)
> +{
> +       return IRQ_WAKE_THREAD;
> +}
> +
>  static int assigned_device_enable_host_msix(struct kvm *kvm,
>  					    struct kvm_assigned_dev_kernel *dev)
>  {
> @@ -374,7 +384,8 @@ static int assigned_device_enable_host_msix(struct kvm *kvm,
>  
>  	for (i = 0; i < dev->entries_nr; i++) {
>  		r = request_threaded_irq(dev->host_msix_entries[i].vector,
> -					 NULL, kvm_assigned_dev_thread_msix,
> +					 kvm_assigned_dev_msix,
> +					 kvm_assigned_dev_thread_msix,
>  					 0, dev->irq_name, dev);
>  		if (r)
>  			goto err;
> 

Should restore the status quo before 3.5's IRQ layer changes. Looks ok
to me.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [PATCHv3 RFC 0/2] kvm: direct msix injection

[Alex Williamson]

On Mon, 2012-07-09 at 17:32 +0200, Jan Kiszka wrote:
> On 2012-07-09 00:55, Michael S. Tsirkin wrote:
> > On Mon, Jul 02, 2012 at 11:08:20AM -0600, Alex Williamson wrote:
> >> On Mon, 2012-06-25 at 11:32 +0200, Jan Kiszka wrote:
> >>> On 2012-06-11 13:19, Michael S. Tsirkin wrote:
> >>>> We can deliver certain interrupts, notably MSIX,
> >>>> from atomic context.
> >>>> Here's an untested patch to do this (compiled only).
> >>>>
> >>>> Changes from v2:
> >>>> Don't inject broadcast interrupts directly
> >>>> Changes from v1:
> >>>> Tried to address comments from v1, except unifying
> >>>> with kvm_set_irq: passing flags to it looks too ugly.
> >>>> Added a comment.
> >>>>
> >>>> Jan, you said you can test this?
> >>>>
> >>>>
> >>>> Michael S. Tsirkin (2):
> >>>>   kvm: implement kvm_set_msi_inatomic
> >>>>   kvm: deliver msix interrupts from irq handler
> >>>>
> >>>>  include/linux/kvm_host.h |  3 ++
> >>>>  virt/kvm/assigned-dev.c  | 31 ++++++++++++++++++--
> >>>>  virt/kvm/irq_comm.c      | 75 ++++++++++++++++++++++++++++++++++++++++++++----
> >>>>  3 files changed, 102 insertions(+), 7 deletions(-)
> >>>>
> >>>
> >>> Finally-tested-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>
> >> Michael, we need either this or the simple oneshot patch to get device
> >> assignment working again for 3.5.  Are you planning to push this for
> >> 3.5?  Thanks,
> >>
> >> Alex
> >>
> > 
> > Avi wants this reworked using an injection cache. I agree with Jan
> > though: oneshot looks too ugly. Just so you can make progress, can't we
> > add a stub handler returning IRQ_WAKE_THREAD unconditionally for now?
> > I.e. like the below (warning: completely untested).
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > 
> > --
> > 
> > diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
> > index b1e091a..18cc36e 100644
> > --- a/virt/kvm/assigned-dev.c
> > +++ b/virt/kvm/assigned-dev.c
> > @@ -334,6 +334,11 @@ static int assigned_device_enable_host_intx(struct kvm *kvm,
> >  }
> >  
> >  #ifdef __KVM_HAVE_MSI
> > +static irqreturn_t kvm_assigned_dev_msi(int irq, void *dev_id)
> > +{
> > +       return IRQ_WAKE_THREAD;
> > +}
> > +
> >  static int assigned_device_enable_host_msi(struct kvm *kvm,
> >  					   struct kvm_assigned_dev_kernel *dev)
> >  {
> > @@ -346,7 +351,7 @@ static int assigned_device_enable_host_msi(struct kvm *kvm,
> >  	}
> >  
> >  	dev->host_irq = dev->dev->irq;
> > -	if (request_threaded_irq(dev->host_irq, NULL,
> > +	if (request_threaded_irq(dev->host_irq, kvm_assigned_dev_msi,
> >  				 kvm_assigned_dev_thread_msi, 0,
> >  				 dev->irq_name, dev)) {
> >  		pci_disable_msi(dev->dev);
> > @@ -358,6 +363,11 @@ static int assigned_device_enable_host_msi(struct kvm *kvm,
> >  #endif
> >  
> >  #ifdef __KVM_HAVE_MSIX
> > +static irqreturn_t kvm_assigned_dev_msix(int irq, void *dev_id)
> > +{
> > +       return IRQ_WAKE_THREAD;
> > +}
> > +
> >  static int assigned_device_enable_host_msix(struct kvm *kvm,
> >  					    struct kvm_assigned_dev_kernel *dev)
> >  {
> > @@ -374,7 +384,8 @@ static int assigned_device_enable_host_msix(struct kvm *kvm,
> >  
> >  	for (i = 0; i < dev->entries_nr; i++) {
> >  		r = request_threaded_irq(dev->host_msix_entries[i].vector,
> > -					 NULL, kvm_assigned_dev_thread_msix,
> > +					 kvm_assigned_dev_msix,
> > +					 kvm_assigned_dev_thread_msix,
> >  					 0, dev->irq_name, dev);
> >  		if (r)
> >  			goto err;
> > 
> 
> Should restore the status quo before 3.5's IRQ layer changes. Looks ok
> to me.

I'll test and post this, Michael is out this week.  Thanks,

Alex