Re: [PATCH 4/4] KVM: x86: Enable MSI for assigned device

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

On Monday 06 October 2008 11:36:08 Yang, Sheng wrote:
> On Sunday 05 October 2008 18:27:20 Avi Kivity wrote:
> > Sheng Yang wrote:
> > > As well as export ioapic_get_delivery_bitmask().
> > >
> > > @@ -132,8 +177,12 @@ static void
> > > kvm_assigned_dev_interrupt_work_handler(struct work_struct *work) *
> > > finer-grained lock, update this
> > >        */
> > >       mutex_lock(&assigned_dev->kvm->lock);
> > > -     kvm_set_irq(assigned_dev->kvm,
> > > -                 assigned_dev->guest_irq, 1);
> > > +     if (assigned_dev->guest_intr_type == KVM_ASSIGNED_DEV_INTR)
> > > +             kvm_set_irq(assigned_dev->kvm, assigned_dev->guest_irq,
> > > 1); +     else if (assigned_dev->guest_intr_type ==
> > > KVM_ASSIGNED_DEV_MSI) { +            
> > > assigned_device_msi_dispatch(assigned_dev);
> > > +             enable_irq(assigned_dev->host_irq);
> > > +     }
> >
> > What happens if the host interrupt is level triggered pci and the guest
> > interrupt is msi?  Or do we not support this combination?
> >
> > If not, how do we prevent it?
>
> I think we don't need to support this combination. Currently, fail to
> enable MSI would fallback to enable IRQ. And MSI disabled guest should not
> expose MSI capability to guest. And also if guest fail to enable MSI, MSI
> enable bit in PCI configuration space should be set 0.
>
> So I would like to sent another return value to tell userspace MSI enable
> failed. And before try to enable, we may also provide a interface to
> userspace to know if MSI can be enabled.

Seems we needn't tell userspace if MSI can be enabled. It's determined mostly 
by the device, and pci_enable_msi() checked that. 

So I think QEmu can expose MSI capability if the device got it. If it's fail 
to be enabled, just fall back to IRQ and return error to QEmu is enough, of 
course, QEmu should set MSI enable bit after kernel space done.

-- 
regards
Yang, Sheng
>
> > > diff --git a/include/linux/kvm.h b/include/linux/kvm.h
> > > index 4269be1..a9b408b 100644
> > > --- a/include/linux/kvm.h
> > > +++ b/include/linux/kvm.h
> > > @@ -493,9 +493,13 @@ struct kvm_assigned_irq {
> > >       __u32 assigned_dev_id;
> > >       __u32 host_irq;
> > >       __u32 guest_irq;
> > > +     __u16 guest_msi_data;
> >
> > Need padding here, just to be safe.
> >
> > > +     __u32 guest_msi_addr;
> >
> > Is u32 enough for the msi address?  Including ia64?
> >
> > >       __u32 flags;
> > >  };
> > >
> > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > > index e24280b..dc6a046 100644
> > > --- a/include/linux/kvm_host.h
> > > +++ b/include/linux/kvm_host.h
> > > @@ -300,8 +300,11 @@ struct kvm_assigned_dev_kernel {
> > >       int host_busnr;
> > >       int host_devfn;
> > >       int host_irq;
> > > +     u16 guest_msi_addr;
> >
> > u32?  or even u64?
>
> Oops...
>
> Well, here is enough for MSI (I mean u16 for msi_data), for PCI spec define
> the size. But I'd better extend msi_data to u32, later I will extend
> msi_addr to u64 or msi_add_lo and msi_addr_hi, for the support of MSI-X.
>
> --
> regards
> Yang, Sheng
>
> > --
> > error compiling committee.c: too many arguments to function
>
> --
> 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