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

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

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.