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

Sheng Yang
2009-03-17T03:43:16Z

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

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.

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

Marcelo, how do you think?

--
regards
Yang, Sheng