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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
