Re: [PATCH 6/8] KVM: assigned dev: Preparation for mask support in userspace

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

On Friday 22 October 2010 22:53:13 Marcelo Tosatti wrote:
> On Wed, Oct 20, 2010 at 04:26:30PM +0800, Sheng Yang wrote:
> > The feature wouldn't be enabled until later patch set msix_flags_enabled.
> > It would be enabled along with mask support in kernel.
> > 
> > Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> > ---
> > 
> >  arch/x86/include/asm/kvm_host.h |    2 ++
> >  include/linux/kvm.h             |    6 +++++-
> >  include/linux/kvm_host.h        |    1 +
> >  virt/kvm/assigned-dev.c         |   39
> >  +++++++++++++++++++++++++++++++++++++++ 4 files changed, 47
> >  insertions(+), 1 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/kvm_host.h
> > b/arch/x86/include/asm/kvm_host.h index e209078..2bb69ba 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -456,6 +456,8 @@ struct kvm_arch {
> > 
> >  	/* fields used by HYPER-V emulation */
> >  	u64 hv_guest_os_id;
> >  	u64 hv_hypercall;
> > 
> > +
> > +	bool msix_flags_enabled;
> > 
> >  };
> >  
> >  struct kvm_vm_stat {
> > 
> > diff --git a/include/linux/kvm.h b/include/linux/kvm.h
> > index 919ae53..a699ec9 100644
> > --- a/include/linux/kvm.h
> > +++ b/include/linux/kvm.h
> > @@ -787,11 +787,15 @@ struct kvm_assigned_msix_nr {
> > 
> >  };
> >  
> >  #define KVM_MAX_MSIX_PER_DEV		256
> > 
> > +
> > +#define KVM_MSIX_FLAG_MASK	1
> > +
> > 
> >  struct kvm_assigned_msix_entry {
> >  
> >  	__u32 assigned_dev_id;
> >  	__u32 gsi;
> >  	__u16 entry; /* The index of entry in the MSI-X table */
> > 
> > -	__u16 padding[3];
> > +	__u16 flags;
> > +	__u16 padding[2];
> > 
> >  };
> >  
> >  #endif /* __LINUX_KVM_H */
> > 
> > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > index 30f83cd..81a6284 100644
> > --- a/include/linux/kvm_host.h
> > +++ b/include/linux/kvm_host.h
> > @@ -438,6 +438,7 @@ struct kvm_irq_ack_notifier {
> > 
> >  };
> >  
> >  #define KVM_ASSIGNED_MSIX_PENDING		0x1
> > 
> > +#define KVM_ASSIGNED_MSIX_MASK			0x2
> > 
> >  struct kvm_guest_msix_entry {
> >  
> >  	u32 vector;
> >  	u16 entry;
> > 
> > diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
> > index 7c98928..bf96ea7 100644
> > --- a/virt/kvm/assigned-dev.c
> > +++ b/virt/kvm/assigned-dev.c
> > 
> > @@ -666,11 +666,35 @@ msix_nr_out:
> >  	return r;
> >  
> >  }
> > 
> > +static void update_msix_mask(struct kvm_assigned_dev_kernel
> > *assigned_dev, +			     int index)
> > +{
> > +	int irq;
> > +	struct irq_desc *desc;
> > +
> > +	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;
> > +	BUG_ON(irq == 0);
> > +	desc = irq_to_desc(irq);
> > +	BUG_ON(!desc->msi_desc);
> > +
> > +	if (assigned_dev->guest_msix_entries[index].flags &
> > +			KVM_ASSIGNED_MSIX_MASK) {
> > +		desc->chip->mask(irq);
> > +		flush_work(&assigned_dev->interrupt_work);
> > +	} else
> > +		desc->chip->unmask(irq);
> > +}
> 
> Bypassing irq handling code like this is wrong (see all the state
> keeping in kernel/irq/handle.c).

Would check it.
> 
> You need a guarantee that MSIX per-vector mask is used for
> disable_irq/enable_irq, right? I can't see how this provides it.

This one meant to directly operate the mask/unmask bit of the MSI-X table, to 
emulate the mask/unmask behavior that guest want. In the previous patch I used 
enable_irq()/disable_irq(), but they won't directly operate the MSI-X table unless 
it's necessary, and Michael want to read the table in userspace then he prefer 
using mask/unmask directly.

--
regards
Yang, Sheng