Re: [libvirt] [PATCH 03/18] qemu: Support disk model=virtio-{non-}transitional

On Mon, Jan 21, 2019 at 05:41:47PM -0500, Cole Robinson wrote: > On 01/18/2019 07:33 AM, Andrea Bolognani wrote: > > On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote: > > > + if (has_tmodel) { > > > + if (virQEMUCapsGet(qemuCaps, tmodel_cap)) > > > + virBufferAddLit(buf, "-transitional"); > > > + > > > + /* No error for if -transitional is not supported: our address > > > + * allocation will force the device into plain PCI bus, which > > > + * is functionally identical to standard 'virtio-XXX' behavior > > > + */ > > > + } else if (has_ntmodel) { > > > + if (virQEMUCapsGet(qemuCaps, ntmodel_cap)) { > > > + virBufferAddLit(buf, "-non-transitional"); > > > + } else if (virQEMUCapsGet(qemuCaps, > > > + QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY)) { > > > + virBufferAddLit(buf, ",disable-legacy=on"); > > > + } else { > > > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > > > + _("virtio non-transitional model not supported " > > > + "for this qemu")); > > > + return -1; > > > + } > > > + } > > > > Would it make sense to be more explicit here? Current versions of > > QEMU default to disable-modern=off,disable-legacy=off for virtio-pci > > devices plugged into conventional PCI slots, but unless I'm mistaken > > that was not always the case, so it would perhaps be preferrable to > > not rely on that behavior and always explicitly set both disable-* > > options when the new devices are not available; if the options > > themselves are not available, then we should error out. > > I don't know enough to say, CCing ehabkost and danpb for more eyes The QEMU code for the devices has - Original devs: disable-modern=off disable-legacy=auto - Transitional devs: disable-modern=off disable-legacy=off - Non-transitional devs: disable-modern=off disable-legacy=on IOW, in the case that -transitional is not available, we could set disable-legacy=off. Provided that we always place -transitional devices into PCI slots, never PCI-e slots, whether we set disable-legacy=off or not doesn't have any effect. None the less it would make sense to set it explicitly though as that would cause us to catch bugs if we mistakenly had a -transitional dev in a PCI-e slot. I don't see a need to set disable-modern at all, since it is defaulting to "off" in all cases.