Re: [PATCH 1/3] KVM: Emulation MSI-X mask bits for assigned devices

[Sheng Yang <sheng@linux.intel.com>]

On Friday 01 October 2010 00:18:10 Marcelo Tosatti wrote:
> On Tue, Sep 28, 2010 at 05:44:10PM +0800, Sheng Yang wrote:
> > This patch enable per-vector mask for assigned devices using MSI-X.
> > 
> > Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> > ---
> > 
> >  arch/x86/kvm/x86.c       |    1 +
> >  include/linux/kvm.h      |    9 ++++++++-
> >  include/linux/kvm_host.h |    1 +
> >  virt/kvm/assigned-dev.c  |   39 
+++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 49 insertions(+), 1 deletions(-)
> >  
> >  
> >  #include <linux/slab.h>
> > 
> > +#include <linux/irqnr.h>
> > +
> > 
> >  #include "irq.h"
> >  
> >  static struct kvm_assigned_dev_kernel *kvm_find_assigned_dev(struct
> >  list_head *head,
> > 
> > @@ -666,6 +668,30 @@ msix_nr_out:
> >  	return r;
> >  
> >  }
> > 
> > +static void update_msix_mask(struct kvm_assigned_dev_kernel
> > *assigned_dev, +			     int index)
> > +{
> > +	int irq;
> > +
> > +	if (!assigned_dev->dev->msix_enabled ||
> > +	    !(assigned_dev->irq_requested_type & KVM_DEV_IRQ_HOST_MSIX))
> > +		return;
> > +
> > +	irq = assigned_dev->host_msix_entries[index].vector;
> > +
> > +	ASSERT(irq != 0);
> > +
> > +	if (assigned_dev->guest_msix_entries[index].flags &
> > +			KVM_ASSIGNED_MSIX_MASK)
> > +		disable_irq(irq);
> > +	else {
> > +		enable_irq(irq);
> > +		if (assigned_dev->guest_msix_entries[index].flags &
> > +				KVM_ASSIGNED_MSIX_PENDING)
> > +			schedule_work(&assigned_dev->interrupt_work);
> > +	}
> > +}
> > +
> 
> Should flush the workqueue after disabling irq. As you say, the
> schedule_work should be unnecessary.
> 
> Also must be careful with races.

Do we need to flush the workqueue? I think the return of writing MSI-X mask bit 
doesn't have implicit meaning that we have handled all pending interrupts. So I 
think leave the work later should also be fine.

--
regards
Yang, Sheng

 
> >  static int kvm_vm_ioctl_set_msix_entry(struct kvm *kvm,
> >  
> >  				       struct kvm_assigned_msix_entry *entry)
> >  
> >  {
> > 
> > @@ -688,6 +714,19 @@ static int kvm_vm_ioctl_set_msix_entry(struct kvm
> > *kvm,
> > 
> >  			adev->guest_msix_entries[i].entry = entry->entry;
> >  			adev->guest_msix_entries[i].vector = entry->gsi;
> >  			adev->host_msix_entries[i].entry = entry->entry;
> > 
> > +			if ((entry->flags & KVM_MSIX_FLAG_MASK) &&
> > +					!(adev->guest_msix_entries[i].flags &
> > +					KVM_ASSIGNED_MSIX_MASK)) {
> > +				adev->guest_msix_entries[i].flags |=
> > +					KVM_ASSIGNED_MSIX_MASK;
> > +				update_msix_mask(adev, i);
> > +			} else if (!(entry->flags & KVM_MSIX_FLAG_MASK) &&
> > +					(adev->guest_msix_entries[i].flags &
> > +					KVM_ASSIGNED_MSIX_MASK)) {
> > +				adev->guest_msix_entries[i].flags &=
> > +					~KVM_ASSIGNED_MSIX_MASK;
> > +				update_msix_mask(adev, i);
> > +			}
> > 
> >  			break;
> >  		
> >  		}
> >  	
> >  	if (i == adev->entries_nr) {