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

["Yang, Sheng" <sheng.yang@intel.com>]

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.

> > 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