Re: [PATCH 0/16 v5] Device assignment improvement in userspace

[Marcelo Tosatti <mtosatti@redhat.com>]

On Tue, Mar 17, 2009 at 11:43:10AM +0800, Sheng Yang wrote:
> On Tuesday 17 March 2009 02:12:11 Marcelo Tosatti wrote:
> > On Mon, Mar 16, 2009 at 11:10:47AM +0200, Avi Kivity wrote:
> > > Sheng Yang wrote:
> > >> Patch 1 and 2 are new ones, all the others had been sent before.
> > >>
> > >> This (huge) patchset, contained:
> > >>
> > >> Patch 1..2 are new interface after reworked device assignment kernel
> > >> part.
> > >>
> > >> Patch 3..6 are generic capability support mechanism. These may can be
> > >> adopted by QEmu upstream as well.
> > >>
> > >> Patch 7..10 enable MSI with device assignment on KVM. Also due to
> > >> reworked device assignment kernel part discard MSI convert to INTx
> > >> mechanism, patch 10 enable it again in userspace.
> > >>
> > >> Patch 11..13 enable MSI-X with device assignment on KVM.
> > >>
> > >> And Patch 14..16 enable SR-IOV with KVM.
> > >>
> > >> Update from latest series:
> > >>
> > >> 1. Convert to the new ioctl interface.
> > >> 2. Merge capability configuration space with PCIDevice one.
> > >> 3. Support of deassign IRQ(unload driver) with MSI/MSI-X better.
> > >> 4. Not assume IRQ0 means no INTx any longer, but check interrupt pin
> > >> field in configuration space for the judgment.
> > >>
> > >> Please help to review! Thanks!
> > >
> > > This looks ready to apply.  I'd like Marcelo to look it over, though,
> > > before.
> >
> > Looks good to me, ready to be applied.
> >
> > There is one pending detail in the ioctl interface. Its a minor issue,
> > but might become troublesome later (and can be fixed after the patchset
> > has been applied).
> >
> > The unassign ioctl takes "struct kvm_assigned_irq" and parses its flags
> > to decide what to do, in this way:
> >
> > - If any bit is set in the guest mask (GUEST_INTX, GUEST_MSI,
> >   GUEST_MSIX), we disable guest-side interrupt.
> > - Likewise for host, disabling host-side interrupt.
> >
> >         host_irq_type = irq_requested_type & KVM_DEV_IRQ_HOST_MASK;
> >         guest_irq_type = irq_requested_type & KVM_DEV_IRQ_GUEST_MASK;
> >
> >         if (host_irq_type)
> >                 deassign_host_irq(kvm, assigned_dev);
> >         if (guest_irq_type)
> >                 deassign_guest_irq(kvm, assigned_dev);
> >
> > This is a little confusing. If we simply want to disable
> > _whatever is assigned_ in either guest or host side, we want a
> > UNASSIGN_GUEST/UNASSIGN_HOST pair of flags (this is how the ioctl
> > behaves, but we pass more flags and don't use them effectively).
> >
> > Or, if the unassign ioctl continues to receive guest/host flags with
> > interrupt type detail, it should error out if userspace passed a type
> > that does not match what is currently assigned.
> >
> > The current behaviour is simpler for userspace, but then we'd need not
> > to pass "struct kvm_assigned_irq".
> >
> > Sheng, what do you say?
> 
> Yeah, it's a ambiguous point.
> 
> I think we have three questions here:
> 
> 1. Do we need to verify guest's "qualification" before deassign the IRQ? 
> 
> I think it's unnecessary, because even if we got a "malicious" userspace, it 
> can try different combination and finally got it...

By requiring the type to be deassigned we force userspace to keep
correct accounting, which is not bad.

> 2. Do we need to keep the flexibility? (e.g. "struct kvm_assigned_irq" and the 
> split of guest and host IRQ deassign)
> 
> I am not sure. I think we can. And for it have been there(upstream) already, I 
> think just keep it there is OK. It shouldn't affect much, and maybe we can use 
> it in the future.

Right.

> 3. How to clarify the ambiguous of flags of kvm_assigned_irq?
> 
> I've updated the patchset, add one irq_requested_type for assigned_dev in 
> userspace. At least, it's more precise in semantic. This can also be used to 
> implement "deassign guest irq" only in the future(if it's necessary. But I am 
> still worry about what if device have interrupt between deassign guest irq and 
> assign new guest irq).

Yes, it needs to be done carefully.

> Marcelo, how do you think? 

I think that enforcing the correct type on deassign is alright.

Avi, I'm good with this patchset, we can force the correct type on
deassign ioctl as a separate kernel patch.