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

["Michael S. Tsirkin" <mst@redhat.com>]

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.