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

Sheng Yang
2010-10-24T12:19:08Z

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