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

[Gleb Natapov <gleb@redhat.com>]

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.