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

On Tue, Jan 22, 2019 at 10:44:23AM -0200, Eduardo Habkost wrote: > 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. Ok, I've no objection to an explicit disable-modern=off based on this info.