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

On Tue, Jan 22, 2019 at 12:45:59PM +0100, Andrea Bolognani wrote: > On Tue, 2019-01-22 at 09:31 +0000, Daniel P. Berrangé wrote: > > 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. > > Wasn't there some old QEMU release where disable-modern didn't > default to off? I seem to remember that. Yes, disable-modern is "on" on 2.6 and older machine-types. > > More generally, I don't see a reason *not* to specify disable-modern > along with disable-legacy. Why be implicit when you can very easily > be explicit instead? Agreed.