[PATCHv4 0/2] kvm: direct msix injection

[PATCHv4 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: add kvm_set_irq_inatomic
  kvm: deliver msi interrupts from irq handler

 include/linux/kvm_host.h |  1 +
 virt/kvm/assigned-dev.c  | 36 +++++++++++++++------
 virt/kvm/irq_comm.c      | 83 +++++++++++++++++++++++++++++++++++++++++-------
 3 files changed, 98 insertions(+), 22 deletions(-)

-- 
MST

[PATCHv4 1/2] kvm: add kvm_set_irq_inatomic

[Michael S. Tsirkin]

Add an API to inject IRQ from atomic context.
Return EWOULDBLOCK if impossible (e.g. for multicast).
Only MSI is supported ATM.

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

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 93bfc9f..e165c09 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -677,6 +677,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);
 void kvm_notify_acked_irq(struct kvm *kvm, unsigned irqchip, unsigned pin);
diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
index 2eb58af..656fa45 100644
--- a/virt/kvm/irq_comm.c
+++ b/virt/kvm/irq_comm.c
@@ -102,6 +102,23 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
 	return r;
 }
 
+static inline void kvm_set_msi_irq(struct kvm_kernel_irq_routing_entry *e,
+				   struct kvm_lapic_irq *irq)
+{
+	trace_kvm_msi_set_irq(e->msi.address_lo, e->msi.data);
+
+	irq->dest_id = (e->msi.address_lo &
+			MSI_ADDR_DEST_ID_MASK) >> MSI_ADDR_DEST_ID_SHIFT;
+	irq->vector = (e->msi.data &
+			MSI_DATA_VECTOR_MASK) >> MSI_DATA_VECTOR_SHIFT;
+	irq->dest_mode = (1 << MSI_ADDR_DEST_MODE_SHIFT) & e->msi.address_lo;
+	irq->trig_mode = (1 << MSI_DATA_TRIGGER_SHIFT) & e->msi.data;
+	irq->delivery_mode = e->msi.data & 0x700;
+	irq->level = 1;
+	irq->shorthand = 0;
+	/* TODO Deal with RH bit of MSI message address */
+}
+
 int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
 		struct kvm *kvm, int irq_source_id, int level)
 {
@@ -110,22 +127,26 @@ int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
 	if (!level)
 		return -1;
 
-	trace_kvm_msi_set_irq(e->msi.address_lo, e->msi.data);
+	kvm_set_msi_irq(e, &irq);
 
-	irq.dest_id = (e->msi.address_lo &
-			MSI_ADDR_DEST_ID_MASK) >> MSI_ADDR_DEST_ID_SHIFT;
-	irq.vector = (e->msi.data &
-			MSI_DATA_VECTOR_MASK) >> MSI_DATA_VECTOR_SHIFT;
-	irq.dest_mode = (1 << MSI_ADDR_DEST_MODE_SHIFT) & e->msi.address_lo;
-	irq.trig_mode = (1 << MSI_DATA_TRIGGER_SHIFT) & e->msi.data;
-	irq.delivery_mode = e->msi.data & 0x700;
-	irq.level = 1;
-	irq.shorthand = 0;
-
-	/* TODO Deal with RH bit of MSI message address */
 	return kvm_irq_delivery_to_apic(kvm, NULL, &irq);
 }
 
+
+static int kvm_set_msi_inatomic(struct kvm_kernel_irq_routing_entry *e,
+			 struct kvm *kvm)
+{
+	struct kvm_lapic_irq irq;
+	int r;
+
+	kvm_set_msi_irq(e, &irq);
+
+	if (kvm_irq_delivery_to_apic_fast(kvm, NULL, &irq, &r))
+		return r;
+	else
+		return -EWOULDBLOCK;
+}
+
 int kvm_send_userspace_msi(struct kvm *kvm, struct kvm_msi *msi)
 {
 	struct kvm_kernel_irq_routing_entry route;
@@ -178,6 +199,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);
+
+	/*
+	 * Injection into either PIC or IOAPIC might need to scan all CPUs,
+	 * which would need to be retried from thread context;  when same GSI
+	 * is connected to both PIC and IOAPIC, we'd have to report a
+	 * partial failure here.
+	 * Since there's no easy way to do this, we only support injecting MSI
+	 * which is limited to 1:1 GSI mapping.
+	 */
+	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

[PATCHv4 2/2] kvm: deliver msi interrupts from irq handler

[Michael S. Tsirkin]

We can deliver certain interrupts, notably MSI,
from atomic context.  Use kvm_set_irq_inatomic,
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>
---
 virt/kvm/assigned-dev.c | 36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)

diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
index 23a41a9..3642239 100644
--- a/virt/kvm/assigned-dev.c
+++ b/virt/kvm/assigned-dev.c
@@ -105,6 +105,15 @@ static irqreturn_t kvm_assigned_dev_thread_intx(int irq, void *dev_id)
 }
 
 #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 irqreturn_t kvm_assigned_dev_thread_msi(int irq, void *dev_id)
 {
 	struct kvm_assigned_dev_kernel *assigned_dev = dev_id;
@@ -117,6 +126,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,11 +360,6 @@ 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)
 {
@@ -363,11 +384,6 @@ 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)
 {
-- 
MST

Re: [PATCHv4 0/2] kvm: direct msix injection

[Michael S. Tsirkin]

On Wed, Oct 17, 2012 at 06:05:53PM +0200, 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?

I have tested this with some networking workloads
and this patchset seems to work fine.
My setup isn't a good fit for benchmarking device
assignment though.
Alex, could you pls verifyu that this solves the
latency issue that you sometimes observe?
With this patchset device assignment latency should be
as fast as vfio.


> 
> Michael S. Tsirkin (2):
>   kvm: add kvm_set_irq_inatomic
>   kvm: deliver msi interrupts from irq handler
> 
>  include/linux/kvm_host.h |  1 +
>  virt/kvm/assigned-dev.c  | 36 +++++++++++++++------
>  virt/kvm/irq_comm.c      | 83 +++++++++++++++++++++++++++++++++++++++++-------
>  3 files changed, 98 insertions(+), 22 deletions(-)
> 
> -- 
> MST

Re: [PATCHv4 0/2] kvm: direct msix injection

[Alex Williamson]

On Wed, 2012-11-21 at 21:26 +0200, Michael S. Tsirkin wrote:
> On Wed, Oct 17, 2012 at 06:05:53PM +0200, 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?
> 
> I have tested this with some networking workloads
> and this patchset seems to work fine.
> My setup isn't a good fit for benchmarking device
> assignment though.
> Alex, could you pls verifyu that this solves the
> latency issue that you sometimes observe?
> With this patchset device assignment latency should be
> as fast as vfio.

Yep, that seems to cover the gap.  My environment is too noisy to
declare an absolute winner, but pci-assign and vfio-pci are now very,
very similar under netperf TCP_RR with these patches.  Thanks,

Alex

Re: [PATCHv4 0/2] kvm: direct msix injection

[Michael S. Tsirkin]

On Tue, Nov 27, 2012 at 09:34:57PM -0700, Alex Williamson wrote:
> On Wed, 2012-11-21 at 21:26 +0200, Michael S. Tsirkin wrote:
> > On Wed, Oct 17, 2012 at 06:05:53PM +0200, 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?
> > 
> > I have tested this with some networking workloads
> > and this patchset seems to work fine.
> > My setup isn't a good fit for benchmarking device
> > assignment though.
> > Alex, could you pls verifyu that this solves the
> > latency issue that you sometimes observe?
> > With this patchset device assignment latency should be
> > as fast as vfio.
> 
> Yep, that seems to cover the gap.  My environment is too noisy to
> declare an absolute winner, but pci-assign and vfio-pci are now very,
> very similar under netperf TCP_RR with these patches.  Thanks,
> 
> Alex

Thanks very much for the report.
Gleb, Marcelo, ACK?
Also please add
Tested-by: Alex Williamson <alex.williamson@redhat.com>

-- 
MST

Re: [PATCHv4 2/2] kvm: deliver msi interrupts from irq handler

[Gleb Natapov]

On Wed, Oct 17, 2012 at 06:06:06PM +0200, Michael S. Tsirkin wrote:
> We can deliver certain interrupts, notably MSI,
> from atomic context.  Use kvm_set_irq_inatomic,
> 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>
> ---
>  virt/kvm/assigned-dev.c | 36 ++++++++++++++++++++++++++----------
>  1 file changed, 26 insertions(+), 10 deletions(-)
> 
> diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
> index 23a41a9..3642239 100644
> --- a/virt/kvm/assigned-dev.c
> +++ b/virt/kvm/assigned-dev.c
> @@ -105,6 +105,15 @@ static irqreturn_t kvm_assigned_dev_thread_intx(int irq, void *dev_id)
>  }
>  
>  #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);
Why not use kvm_set_msi_inatomic() and drop kvm_set_irq_inatomic() from
previous patch? 

> +	return unlikely(ret == -EWOULDBLOCK) ? IRQ_WAKE_THREAD : IRQ_HANDLED;
> +}
> +
>  static irqreturn_t kvm_assigned_dev_thread_msi(int irq, void *dev_id)
>  {
>  	struct kvm_assigned_dev_kernel *assigned_dev = dev_id;
> @@ -117,6 +126,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,11 +360,6 @@ 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)
>  {
> @@ -363,11 +384,6 @@ 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)
>  {
> -- 
> 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: [PATCHv4 2/2] kvm: deliver msi interrupts from irq handler

[Michael S. Tsirkin]

On Wed, Nov 28, 2012 at 01:43:34PM +0200, Gleb Natapov wrote:
> On Wed, Oct 17, 2012 at 06:06:06PM +0200, Michael S. Tsirkin wrote:
> > We can deliver certain interrupts, notably MSI,
> > from atomic context.  Use kvm_set_irq_inatomic,
> > 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>
> > ---
> >  virt/kvm/assigned-dev.c | 36 ++++++++++++++++++++++++++----------
> >  1 file changed, 26 insertions(+), 10 deletions(-)
> > 
> > diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
> > index 23a41a9..3642239 100644
> > --- a/virt/kvm/assigned-dev.c
> > +++ b/virt/kvm/assigned-dev.c
> > @@ -105,6 +105,15 @@ static irqreturn_t kvm_assigned_dev_thread_intx(int irq, void *dev_id)
> >  }
> >  
> >  #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);
> Why not use kvm_set_msi_inatomic() and drop kvm_set_irq_inatomic() from
> previous patch? 

kvm_set_msi_inatomic needs a routing entry, and
we don't have the routing entry at this level.

Further, guest irq might not be an MSI: host MSI
can cause guest intx injection I think, we need to
bounce it to thread as we did earlier.

> > +	return unlikely(ret == -EWOULDBLOCK) ? IRQ_WAKE_THREAD : IRQ_HANDLED;
> > +}
> > +
> >  static irqreturn_t kvm_assigned_dev_thread_msi(int irq, void *dev_id)
> >  {
> >  	struct kvm_assigned_dev_kernel *assigned_dev = dev_id;
> > @@ -117,6 +126,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,11 +360,6 @@ 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)
> >  {
> > @@ -363,11 +384,6 @@ 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)
> >  {
> > -- 
> > 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: [PATCHv4 2/2] kvm: deliver msi interrupts from irq handler

[Gleb Natapov]

On Wed, Nov 28, 2012 at 01:56:16PM +0200, Michael S. Tsirkin wrote:
> On Wed, Nov 28, 2012 at 01:43:34PM +0200, Gleb Natapov wrote:
> > On Wed, Oct 17, 2012 at 06:06:06PM +0200, Michael S. Tsirkin wrote:
> > > We can deliver certain interrupts, notably MSI,
> > > from atomic context.  Use kvm_set_irq_inatomic,
> > > 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>
> > > ---
> > >  virt/kvm/assigned-dev.c | 36 ++++++++++++++++++++++++++----------
> > >  1 file changed, 26 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
> > > index 23a41a9..3642239 100644
> > > --- a/virt/kvm/assigned-dev.c
> > > +++ b/virt/kvm/assigned-dev.c
> > > @@ -105,6 +105,15 @@ static irqreturn_t kvm_assigned_dev_thread_intx(int irq, void *dev_id)
> > >  }
> > >  
> > >  #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);
> > Why not use kvm_set_msi_inatomic() and drop kvm_set_irq_inatomic() from
> > previous patch? 
> 
> kvm_set_msi_inatomic needs a routing entry, and
> we don't have the routing entry at this level.
> 
Yes, right. BTW is this interface will be used only for legacy assigned
device or there will be other users too?

> Further, guest irq might not be an MSI: host MSI
> can cause guest intx injection I think, we need to
> bounce it to thread as we did earlier.
Ah, so msi in kvm_assigned_dev_msi() is about host msi? Can host be intx
but guest msi? You seems to not handle this case. Also injection of intx
via ioapic is the same as injecting MSI. The format and the capability
of irq message are essentially the same.


> 
> > > +	return unlikely(ret == -EWOULDBLOCK) ? IRQ_WAKE_THREAD : IRQ_HANDLED;
> > > +}
> > > +
> > >  static irqreturn_t kvm_assigned_dev_thread_msi(int irq, void *dev_id)
> > >  {
> > >  	struct kvm_assigned_dev_kernel *assigned_dev = dev_id;
> > > @@ -117,6 +126,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,11 +360,6 @@ 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)
> > >  {
> > > @@ -363,11 +384,6 @@ 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)
> > >  {
> > > -- 
> > > 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: [PATCHv4 2/2] kvm: deliver msi interrupts from irq handler

[Michael S. Tsirkin]

On Wed, Nov 28, 2012 at 02:13:01PM +0200, Gleb Natapov wrote:
> On Wed, Nov 28, 2012 at 01:56:16PM +0200, Michael S. Tsirkin wrote:
> > On Wed, Nov 28, 2012 at 01:43:34PM +0200, Gleb Natapov wrote:
> > > On Wed, Oct 17, 2012 at 06:06:06PM +0200, Michael S. Tsirkin wrote:
> > > > We can deliver certain interrupts, notably MSI,
> > > > from atomic context.  Use kvm_set_irq_inatomic,
> > > > 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>
> > > > ---
> > > >  virt/kvm/assigned-dev.c | 36 ++++++++++++++++++++++++++----------
> > > >  1 file changed, 26 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
> > > > index 23a41a9..3642239 100644
> > > > --- a/virt/kvm/assigned-dev.c
> > > > +++ b/virt/kvm/assigned-dev.c
> > > > @@ -105,6 +105,15 @@ static irqreturn_t kvm_assigned_dev_thread_intx(int irq, void *dev_id)
> > > >  }
> > > >  
> > > >  #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);
> > > Why not use kvm_set_msi_inatomic() and drop kvm_set_irq_inatomic() from
> > > previous patch? 
> > 
> > kvm_set_msi_inatomic needs a routing entry, and
> > we don't have the routing entry at this level.
> > 
> Yes, right. BTW is this interface will be used only for legacy assigned
> device or there will be other users too?

I think long term we should convert irqfd to this too.

> > Further, guest irq might not be an MSI: host MSI
> > can cause guest intx injection I think, we need to
> > bounce it to thread as we did earlier.
> Ah, so msi in kvm_assigned_dev_msi() is about host msi?

Yes.

> Can host be intx
> but guest msi?

No.

> You seems to not handle this case. Also injection of intx
> via ioapic is the same as injecting MSI. The format and the capability
> of irq message are essentially the same.

Absolutely. So we will be able to extend this to intx long term.
The difference is in the fact that unlike msi, intx can
(and does) have multiple entries per GSI.
I have not yet figured out how to report and handle failure
in case one of these can be injected in atomic context,
another can't. There's likely an easy way but can
be a follow up patch I think.

> 
> > 
> > > > +	return unlikely(ret == -EWOULDBLOCK) ? IRQ_WAKE_THREAD : IRQ_HANDLED;
> > > > +}
> > > > +
> > > >  static irqreturn_t kvm_assigned_dev_thread_msi(int irq, void *dev_id)
> > > >  {
> > > >  	struct kvm_assigned_dev_kernel *assigned_dev = dev_id;
> > > > @@ -117,6 +126,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,11 +360,6 @@ 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)
> > > >  {
> > > > @@ -363,11 +384,6 @@ 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)
> > > >  {
> > > > -- 
> > > > 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: [PATCHv4 2/2] kvm: deliver msi interrupts from irq handler

[Gleb Natapov]

On Wed, Nov 28, 2012 at 02:22:45PM +0200, Michael S. Tsirkin wrote:
> On Wed, Nov 28, 2012 at 02:13:01PM +0200, Gleb Natapov wrote:
> > On Wed, Nov 28, 2012 at 01:56:16PM +0200, Michael S. Tsirkin wrote:
> > > On Wed, Nov 28, 2012 at 01:43:34PM +0200, Gleb Natapov wrote:
> > > > On Wed, Oct 17, 2012 at 06:06:06PM +0200, Michael S. Tsirkin wrote:
> > > > > We can deliver certain interrupts, notably MSI,
> > > > > from atomic context.  Use kvm_set_irq_inatomic,
> > > > > 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>
> > > > > ---
> > > > >  virt/kvm/assigned-dev.c | 36 ++++++++++++++++++++++++++----------
> > > > >  1 file changed, 26 insertions(+), 10 deletions(-)
> > > > > 
> > > > > diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
> > > > > index 23a41a9..3642239 100644
> > > > > --- a/virt/kvm/assigned-dev.c
> > > > > +++ b/virt/kvm/assigned-dev.c
> > > > > @@ -105,6 +105,15 @@ static irqreturn_t kvm_assigned_dev_thread_intx(int irq, void *dev_id)
> > > > >  }
> > > > >  
> > > > >  #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);
> > > > Why not use kvm_set_msi_inatomic() and drop kvm_set_irq_inatomic() from
> > > > previous patch? 
> > > 
> > > kvm_set_msi_inatomic needs a routing entry, and
> > > we don't have the routing entry at this level.
> > > 
> > Yes, right. BTW is this interface will be used only for legacy assigned
> > device or there will be other users too?
> 
> I think long term we should convert irqfd to this too.
> 
VIFO uses irqfd, no? So, why legacy device assignment needs that code
to achieve parity with VFIO? Also why long term? What are the
complications?

> > > Further, guest irq might not be an MSI: host MSI
> > > can cause guest intx injection I think, we need to
> > > bounce it to thread as we did earlier.
> > Ah, so msi in kvm_assigned_dev_msi() is about host msi?
> 
> Yes.
> 
> > Can host be intx
> > but guest msi?
> 
> No.
> 
> > You seems to not handle this case. Also injection of intx
> > via ioapic is the same as injecting MSI. The format and the capability
> > of irq message are essentially the same.
> 
> Absolutely. So we will be able to extend this to intx long term.
> The difference is in the fact that unlike msi, intx can
> (and does) have multiple entries per GSI.
> I have not yet figured out how to report and handle failure
> in case one of these can be injected in atomic context,
> another can't. There's likely an easy way but can
> be a follow up patch I think.
I prefer to figure that out before introducing the interface. Hmm, we
can get rid of vcpu loop in pic (should be very easily done by checking
for kvm_apic_accept_pic_intr() during apic configuration and keeping
global extint vcpu) and then sorting irq routing entries so that ioapic
entry is first since only ioapic injection can fail.

> 
> > 
> > > 
> > > > > +	return unlikely(ret == -EWOULDBLOCK) ? IRQ_WAKE_THREAD : IRQ_HANDLED;
> > > > > +}
> > > > > +
> > > > >  static irqreturn_t kvm_assigned_dev_thread_msi(int irq, void *dev_id)
> > > > >  {
> > > > >  	struct kvm_assigned_dev_kernel *assigned_dev = dev_id;
> > > > > @@ -117,6 +126,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,11 +360,6 @@ 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)
> > > > >  {
> > > > > @@ -363,11 +384,6 @@ 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)
> > > > >  {
> > > > > -- 
> > > > > 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.

--
			Gleb.

Re: [PATCHv4 2/2] kvm: deliver msi interrupts from irq handler

[Michael S. Tsirkin]

On Wed, Nov 28, 2012 at 02:45:09PM +0200, Gleb Natapov wrote:
> On Wed, Nov 28, 2012 at 02:22:45PM +0200, Michael S. Tsirkin wrote:
> > On Wed, Nov 28, 2012 at 02:13:01PM +0200, Gleb Natapov wrote:
> > > On Wed, Nov 28, 2012 at 01:56:16PM +0200, Michael S. Tsirkin wrote:
> > > > On Wed, Nov 28, 2012 at 01:43:34PM +0200, Gleb Natapov wrote:
> > > > > On Wed, Oct 17, 2012 at 06:06:06PM +0200, Michael S. Tsirkin wrote:
> > > > > > We can deliver certain interrupts, notably MSI,
> > > > > > from atomic context.  Use kvm_set_irq_inatomic,
> > > > > > 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>
> > > > > > ---
> > > > > >  virt/kvm/assigned-dev.c | 36 ++++++++++++++++++++++++++----------
> > > > > >  1 file changed, 26 insertions(+), 10 deletions(-)
> > > > > > 
> > > > > > diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
> > > > > > index 23a41a9..3642239 100644
> > > > > > --- a/virt/kvm/assigned-dev.c
> > > > > > +++ b/virt/kvm/assigned-dev.c
> > > > > > @@ -105,6 +105,15 @@ static irqreturn_t kvm_assigned_dev_thread_intx(int irq, void *dev_id)
> > > > > >  }
> > > > > >  
> > > > > >  #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);
> > > > > Why not use kvm_set_msi_inatomic() and drop kvm_set_irq_inatomic() from
> > > > > previous patch? 
> > > > 
> > > > kvm_set_msi_inatomic needs a routing entry, and
> > > > we don't have the routing entry at this level.
> > > > 
> > > Yes, right. BTW is this interface will be used only for legacy assigned
> > > device or there will be other users too?
> > 
> > I think long term we should convert irqfd to this too.
> > 
> VIFO uses irqfd, no? So, why legacy device assignment needs that code
> to achieve parity with VFIO?

Clarification: there are two issues:

1. legacy assignment has bad worst case latency
	this is because we bounce all ainterrupts through threads
	this patch fixes this
2. irqfd injects all MSIs from an atomic context
	this patch does not fix this, but it can
	be fixed on top of this patch

> Also why long term? What are the complications?

Nothing special. Just need to be careful with all the rcu trickery that
irqfd uses.

> > > > Further, guest irq might not be an MSI: host MSI
> > > > can cause guest intx injection I think, we need to
> > > > bounce it to thread as we did earlier.
> > > Ah, so msi in kvm_assigned_dev_msi() is about host msi?
> > 
> > Yes.
> > 
> > > Can host be intx
> > > but guest msi?
> > 
> > No.
> > 
> > > You seems to not handle this case. Also injection of intx
> > > via ioapic is the same as injecting MSI. The format and the capability
> > > of irq message are essentially the same.
> > 
> > Absolutely. So we will be able to extend this to intx long term.
> > The difference is in the fact that unlike msi, intx can
> > (and does) have multiple entries per GSI.
> > I have not yet figured out how to report and handle failure
> > in case one of these can be injected in atomic context,
> > another can't. There's likely an easy way but can
> > be a follow up patch I think.
>
> I prefer to figure that out before introducing the interface.

Ow come on, it's just an internal interface, not even exported
to modules. Changing it would be trivial and the
implementation is very small too.

> Hmm, we
> can get rid of vcpu loop in pic (should be very easily done by checking
> for kvm_apic_accept_pic_intr() during apic configuration and keeping
> global extint vcpu) and then sorting irq routing entries so that ioapic
> entry is first since only ioapic injection can fail.

Yes, I think it's a good idea to remove as many vcpu loops as possible:
for example, this vcpu loop is currently hit from atomic
context anyway, isn't it?

> > 
> > > 
> > > > 
> > > > > > +	return unlikely(ret == -EWOULDBLOCK) ? IRQ_WAKE_THREAD : IRQ_HANDLED;
> > > > > > +}
> > > > > > +
> > > > > >  static irqreturn_t kvm_assigned_dev_thread_msi(int irq, void *dev_id)
> > > > > >  {
> > > > > >  	struct kvm_assigned_dev_kernel *assigned_dev = dev_id;
> > > > > > @@ -117,6 +126,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,11 +360,6 @@ 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)
> > > > > >  {
> > > > > > @@ -363,11 +384,6 @@ 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)
> > > > > >  {
> > > > > > -- 
> > > > > > 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.
> 
> --
> 			Gleb.

Re: [PATCHv4 2/2] kvm: deliver msi interrupts from irq handler

[Gleb Natapov]

On Wed, Nov 28, 2012 at 03:25:44PM +0200, Michael S. Tsirkin wrote:
> On Wed, Nov 28, 2012 at 02:45:09PM +0200, Gleb Natapov wrote:
> > On Wed, Nov 28, 2012 at 02:22:45PM +0200, Michael S. Tsirkin wrote:
> > > On Wed, Nov 28, 2012 at 02:13:01PM +0200, Gleb Natapov wrote:
> > > > On Wed, Nov 28, 2012 at 01:56:16PM +0200, Michael S. Tsirkin wrote:
> > > > > On Wed, Nov 28, 2012 at 01:43:34PM +0200, Gleb Natapov wrote:
> > > > > > On Wed, Oct 17, 2012 at 06:06:06PM +0200, Michael S. Tsirkin wrote:
> > > > > > > We can deliver certain interrupts, notably MSI,
> > > > > > > from atomic context.  Use kvm_set_irq_inatomic,
> > > > > > > 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>
> > > > > > > ---
> > > > > > >  virt/kvm/assigned-dev.c | 36 ++++++++++++++++++++++++++----------
> > > > > > >  1 file changed, 26 insertions(+), 10 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
> > > > > > > index 23a41a9..3642239 100644
> > > > > > > --- a/virt/kvm/assigned-dev.c
> > > > > > > +++ b/virt/kvm/assigned-dev.c
> > > > > > > @@ -105,6 +105,15 @@ static irqreturn_t kvm_assigned_dev_thread_intx(int irq, void *dev_id)
> > > > > > >  }
> > > > > > >  
> > > > > > >  #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);
> > > > > > Why not use kvm_set_msi_inatomic() and drop kvm_set_irq_inatomic() from
> > > > > > previous patch? 
> > > > > 
> > > > > kvm_set_msi_inatomic needs a routing entry, and
> > > > > we don't have the routing entry at this level.
> > > > > 
> > > > Yes, right. BTW is this interface will be used only for legacy assigned
> > > > device or there will be other users too?
> > > 
> > > I think long term we should convert irqfd to this too.
> > > 
> > VIFO uses irqfd, no? So, why legacy device assignment needs that code
> > to achieve parity with VFIO?
> 
> Clarification: there are two issues:
> 
> 1. legacy assignment has bad worst case latency
> 	this is because we bounce all ainterrupts through threads
> 	this patch fixes this
> 2. irqfd injects all MSIs from an atomic context
> 	this patch does not fix this, but it can
> 	be fixed on top of this patch
> 
Thanks for clarification.

> > Also why long term? What are the complications?
> 
> Nothing special. Just need to be careful with all the rcu trickery that
> irqfd uses.
> 
> > > > > Further, guest irq might not be an MSI: host MSI
> > > > > can cause guest intx injection I think, we need to
> > > > > bounce it to thread as we did earlier.
> > > > Ah, so msi in kvm_assigned_dev_msi() is about host msi?
> > > 
> > > Yes.
> > > 
> > > > Can host be intx
> > > > but guest msi?
> > > 
> > > No.
> > > 
> > > > You seems to not handle this case. Also injection of intx
> > > > via ioapic is the same as injecting MSI. The format and the capability
> > > > of irq message are essentially the same.
> > > 
> > > Absolutely. So we will be able to extend this to intx long term.
> > > The difference is in the fact that unlike msi, intx can
> > > (and does) have multiple entries per GSI.
> > > I have not yet figured out how to report and handle failure
> > > in case one of these can be injected in atomic context,
> > > another can't. There's likely an easy way but can
> > > be a follow up patch I think.
> >
> > I prefer to figure that out before introducing the interface.
> 
> Ow come on, it's just an internal interface, not even exported
> to modules. Changing it would be trivial and the
> implementation is very small too.
> 
The question is if it can be done at all or not. If it cannot then it
does not matter that interface is internal, but fortunately looks like
it is possible, so I am fine with proposed implementation for now.

> > Hmm, we
> > can get rid of vcpu loop in pic (should be very easily done by checking
> > for kvm_apic_accept_pic_intr() during apic configuration and keeping
> > global extint vcpu) and then sorting irq routing entries so that ioapic
> > entry is first since only ioapic injection can fail.
> 
> Yes, I think it's a good idea to remove as many vcpu loops as possible:
> for example, this vcpu loop is currently hit from atomic
> context anyway, isn't it?
Actually it is not. The lock is dropped just before the loop, so this
loop shouldn't be the roadblock at all.

> 
> > > 
> > > > 
> > > > > 
> > > > > > > +	return unlikely(ret == -EWOULDBLOCK) ? IRQ_WAKE_THREAD : IRQ_HANDLED;
> > > > > > > +}
> > > > > > > +
> > > > > > >  static irqreturn_t kvm_assigned_dev_thread_msi(int irq, void *dev_id)
> > > > > > >  {
> > > > > > >  	struct kvm_assigned_dev_kernel *assigned_dev = dev_id;
> > > > > > > @@ -117,6 +126,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,11 +360,6 @@ 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)
> > > > > > >  {
> > > > > > > @@ -363,11 +384,6 @@ 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)
> > > > > > >  {
> > > > > > > -- 
> > > > > > > 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.
> > 
> > --
> > 			Gleb.

--
			Gleb.

Re: [PATCHv4 2/2] kvm: deliver msi interrupts from irq handler

[Michael S. Tsirkin]

On Wed, Nov 28, 2012 at 03:38:40PM +0200, Gleb Natapov wrote:
> On Wed, Nov 28, 2012 at 03:25:44PM +0200, Michael S. Tsirkin wrote:
> > On Wed, Nov 28, 2012 at 02:45:09PM +0200, Gleb Natapov wrote:
> > > On Wed, Nov 28, 2012 at 02:22:45PM +0200, Michael S. Tsirkin wrote:
> > > > On Wed, Nov 28, 2012 at 02:13:01PM +0200, Gleb Natapov wrote:
> > > > > On Wed, Nov 28, 2012 at 01:56:16PM +0200, Michael S. Tsirkin wrote:
> > > > > > On Wed, Nov 28, 2012 at 01:43:34PM +0200, Gleb Natapov wrote:
> > > > > > > On Wed, Oct 17, 2012 at 06:06:06PM +0200, Michael S. Tsirkin wrote:
> > > > > > > > We can deliver certain interrupts, notably MSI,
> > > > > > > > from atomic context.  Use kvm_set_irq_inatomic,
> > > > > > > > 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>
> > > > > > > > ---
> > > > > > > >  virt/kvm/assigned-dev.c | 36 ++++++++++++++++++++++++++----------
> > > > > > > >  1 file changed, 26 insertions(+), 10 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
> > > > > > > > index 23a41a9..3642239 100644
> > > > > > > > --- a/virt/kvm/assigned-dev.c
> > > > > > > > +++ b/virt/kvm/assigned-dev.c
> > > > > > > > @@ -105,6 +105,15 @@ static irqreturn_t kvm_assigned_dev_thread_intx(int irq, void *dev_id)
> > > > > > > >  }
> > > > > > > >  
> > > > > > > >  #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);
> > > > > > > Why not use kvm_set_msi_inatomic() and drop kvm_set_irq_inatomic() from
> > > > > > > previous patch? 
> > > > > > 
> > > > > > kvm_set_msi_inatomic needs a routing entry, and
> > > > > > we don't have the routing entry at this level.
> > > > > > 
> > > > > Yes, right. BTW is this interface will be used only for legacy assigned
> > > > > device or there will be other users too?
> > > > 
> > > > I think long term we should convert irqfd to this too.
> > > > 
> > > VIFO uses irqfd, no? So, why legacy device assignment needs that code
> > > to achieve parity with VFIO?
> > 
> > Clarification: there are two issues:
> > 
> > 1. legacy assignment has bad worst case latency
> > 	this is because we bounce all ainterrupts through threads
> > 	this patch fixes this
> > 2. irqfd injects all MSIs from an atomic context
> > 	this patch does not fix this, but it can
> > 	be fixed on top of this patch
> > 
> Thanks for clarification.
> 
> > > Also why long term? What are the complications?
> > 
> > Nothing special. Just need to be careful with all the rcu trickery that
> > irqfd uses.
> > 
> > > > > > Further, guest irq might not be an MSI: host MSI
> > > > > > can cause guest intx injection I think, we need to
> > > > > > bounce it to thread as we did earlier.
> > > > > Ah, so msi in kvm_assigned_dev_msi() is about host msi?
> > > > 
> > > > Yes.
> > > > 
> > > > > Can host be intx
> > > > > but guest msi?
> > > > 
> > > > No.
> > > > 
> > > > > You seems to not handle this case. Also injection of intx
> > > > > via ioapic is the same as injecting MSI. The format and the capability
> > > > > of irq message are essentially the same.
> > > > 
> > > > Absolutely. So we will be able to extend this to intx long term.
> > > > The difference is in the fact that unlike msi, intx can
> > > > (and does) have multiple entries per GSI.
> > > > I have not yet figured out how to report and handle failure
> > > > in case one of these can be injected in atomic context,
> > > > another can't. There's likely an easy way but can
> > > > be a follow up patch I think.
> > >
> > > I prefer to figure that out before introducing the interface.
> > 
> > Ow come on, it's just an internal interface, not even exported
> > to modules. Changing it would be trivial and the
> > implementation is very small too.
> > 
> The question is if it can be done at all or not. If it cannot then it
> does not matter that interface is internal, but fortunately looks like
> it is possible, so I am fine with proposed implementation for now.
> 
> > > Hmm, we
> > > can get rid of vcpu loop in pic (should be very easily done by checking
> > > for kvm_apic_accept_pic_intr() during apic configuration and keeping
> > > global extint vcpu) and then sorting irq routing entries so that ioapic
> > > entry is first since only ioapic injection can fail.
> > 
> > Yes, I think it's a good idea to remove as many vcpu loops as possible:
> > for example, this vcpu loop is currently hit from atomic
> > context anyway, isn't it?
> Actually it is not. The lock is dropped just before the loop, so this
> loop shouldn't be the roadblock at all.

Hmm you are saying PIC injections in atomic context always succeeds?

> > 
> > > > 
> > > > > 
> > > > > > 
> > > > > > > > +	return unlikely(ret == -EWOULDBLOCK) ? IRQ_WAKE_THREAD : IRQ_HANDLED;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > >  static irqreturn_t kvm_assigned_dev_thread_msi(int irq, void *dev_id)
> > > > > > > >  {
> > > > > > > >  	struct kvm_assigned_dev_kernel *assigned_dev = dev_id;
> > > > > > > > @@ -117,6 +126,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,11 +360,6 @@ 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)
> > > > > > > >  {
> > > > > > > > @@ -363,11 +384,6 @@ 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)
> > > > > > > >  {
> > > > > > > > -- 
> > > > > > > > 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.
> > > 
> > > --
> > > 			Gleb.
> 
> --
> 			Gleb.

Re: [PATCHv4 2/2] kvm: deliver msi interrupts from irq handler

[Gleb Natapov]

On Wed, Nov 28, 2012 at 05:25:17PM +0200, Michael S. Tsirkin wrote:
> On Wed, Nov 28, 2012 at 03:38:40PM +0200, Gleb Natapov wrote:
> > On Wed, Nov 28, 2012 at 03:25:44PM +0200, Michael S. Tsirkin wrote:
> > > On Wed, Nov 28, 2012 at 02:45:09PM +0200, Gleb Natapov wrote:
> > > > On Wed, Nov 28, 2012 at 02:22:45PM +0200, Michael S. Tsirkin wrote:
> > > > > On Wed, Nov 28, 2012 at 02:13:01PM +0200, Gleb Natapov wrote:
> > > > > > On Wed, Nov 28, 2012 at 01:56:16PM +0200, Michael S. Tsirkin wrote:
> > > > > > > On Wed, Nov 28, 2012 at 01:43:34PM +0200, Gleb Natapov wrote:
> > > > > > > > On Wed, Oct 17, 2012 at 06:06:06PM +0200, Michael S. Tsirkin wrote:
> > > > > > > > > We can deliver certain interrupts, notably MSI,
> > > > > > > > > from atomic context.  Use kvm_set_irq_inatomic,
> > > > > > > > > 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>
> > > > > > > > > ---
> > > > > > > > >  virt/kvm/assigned-dev.c | 36 ++++++++++++++++++++++++++----------
> > > > > > > > >  1 file changed, 26 insertions(+), 10 deletions(-)
> > > > > > > > > 
> > > > > > > > > diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
> > > > > > > > > index 23a41a9..3642239 100644
> > > > > > > > > --- a/virt/kvm/assigned-dev.c
> > > > > > > > > +++ b/virt/kvm/assigned-dev.c
> > > > > > > > > @@ -105,6 +105,15 @@ static irqreturn_t kvm_assigned_dev_thread_intx(int irq, void *dev_id)
> > > > > > > > >  }
> > > > > > > > >  
> > > > > > > > >  #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);
> > > > > > > > Why not use kvm_set_msi_inatomic() and drop kvm_set_irq_inatomic() from
> > > > > > > > previous patch? 
> > > > > > > 
> > > > > > > kvm_set_msi_inatomic needs a routing entry, and
> > > > > > > we don't have the routing entry at this level.
> > > > > > > 
> > > > > > Yes, right. BTW is this interface will be used only for legacy assigned
> > > > > > device or there will be other users too?
> > > > > 
> > > > > I think long term we should convert irqfd to this too.
> > > > > 
> > > > VIFO uses irqfd, no? So, why legacy device assignment needs that code
> > > > to achieve parity with VFIO?
> > > 
> > > Clarification: there are two issues:
> > > 
> > > 1. legacy assignment has bad worst case latency
> > > 	this is because we bounce all ainterrupts through threads
> > > 	this patch fixes this
> > > 2. irqfd injects all MSIs from an atomic context
> > > 	this patch does not fix this, but it can
> > > 	be fixed on top of this patch
> > > 
> > Thanks for clarification.
> > 
> > > > Also why long term? What are the complications?
> > > 
> > > Nothing special. Just need to be careful with all the rcu trickery that
> > > irqfd uses.
> > > 
> > > > > > > Further, guest irq might not be an MSI: host MSI
> > > > > > > can cause guest intx injection I think, we need to
> > > > > > > bounce it to thread as we did earlier.
> > > > > > Ah, so msi in kvm_assigned_dev_msi() is about host msi?
> > > > > 
> > > > > Yes.
> > > > > 
> > > > > > Can host be intx
> > > > > > but guest msi?
> > > > > 
> > > > > No.
> > > > > 
> > > > > > You seems to not handle this case. Also injection of intx
> > > > > > via ioapic is the same as injecting MSI. The format and the capability
> > > > > > of irq message are essentially the same.
> > > > > 
> > > > > Absolutely. So we will be able to extend this to intx long term.
> > > > > The difference is in the fact that unlike msi, intx can
> > > > > (and does) have multiple entries per GSI.
> > > > > I have not yet figured out how to report and handle failure
> > > > > in case one of these can be injected in atomic context,
> > > > > another can't. There's likely an easy way but can
> > > > > be a follow up patch I think.
> > > >
> > > > I prefer to figure that out before introducing the interface.
> > > 
> > > Ow come on, it's just an internal interface, not even exported
> > > to modules. Changing it would be trivial and the
> > > implementation is very small too.
> > > 
> > The question is if it can be done at all or not. If it cannot then it
> > does not matter that interface is internal, but fortunately looks like
> > it is possible, so I am fine with proposed implementation for now.
> > 
> > > > Hmm, we
> > > > can get rid of vcpu loop in pic (should be very easily done by checking
> > > > for kvm_apic_accept_pic_intr() during apic configuration and keeping
> > > > global extint vcpu) and then sorting irq routing entries so that ioapic
> > > > entry is first since only ioapic injection can fail.
> > > 
> > > Yes, I think it's a good idea to remove as many vcpu loops as possible:
> > > for example, this vcpu loop is currently hit from atomic
> > > context anyway, isn't it?
> > Actually it is not. The lock is dropped just before the loop, so this
> > loop shouldn't be the roadblock at all.
> 
> Hmm you are saying PIC injections in atomic context always succeeds?
> 
No, I am saying vcpu loop is not hit from atomic context currently.

--
			Gleb.

Re: [PATCHv4 0/2] kvm: direct msix injection

[Gleb Natapov]

On Wed, Oct 17, 2012 at 06:05:55PM +0200, 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?
> 
> 
Applied to queue. Thanks.

> Michael S. Tsirkin (2):
>   kvm: add kvm_set_irq_inatomic
>   kvm: deliver msi interrupts from irq handler
> 
>  include/linux/kvm_host.h |  1 +
>  virt/kvm/assigned-dev.c  | 36 +++++++++++++++------
>  virt/kvm/irq_comm.c      | 83 +++++++++++++++++++++++++++++++++++++++++-------
>  3 files changed, 98 insertions(+), 22 deletions(-)
> 
> -- 
> 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.