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