Re: [libvirt] [PATCH 4/6] qemu: Wire up disk model=virtio-{non-}transitional

On Tue, 2019-01-15 at 16:56 +0000, Daniel P. Berrangé wrote: > On Sun, Jan 13, 2019 at 06:12:06PM -0500, Cole Robinson wrote: > > Add new <disk> model values for virtio transitional devices. When > > combined with bus='virtio': > > > > * "virtio-transitional" maps to qemu "virtio-blk-pci-transitional" > > * "virtio-non-transitional" maps to qemu "virtio-blk-pci-non-transitional" > > > > Signed-off-by: Cole Robinson <crobinso(a)redhat.com&gt; > > --- > > src/qemu/qemu_command.c | 31 ++++++++++++++++++- > > src/qemu/qemu_domain_address.c | 2 ++ > > ...virtio-non-transitional.x86_64-latest.args | 7 +++-- > > .../virtio-transitional.x86_64-latest.args | 4 +-- > > .../virtio-non-transitional.xml | 10 ++++-- > > 5 files changed, 45 insertions(+), 9 deletions(-) > > > > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > > index 822d5f8669..ca6abea227 100644 > > --- a/src/qemu/qemu_command.c > > +++ b/src/qemu/qemu_command.c > > @@ -443,6 +443,33 @@ qemuBuildVirtioDevStr(virBufferPtr buf, > > return 0; > > } > > > > +static int > > +qemuBuildVirtioTransitional(virBufferPtr buf, > > + const char *baseName, > > + virDomainDeviceAddressType type, > > + bool transitional, > > + bool nontransitional) > > +{ > > + if (qemuBuildVirtioDevStr(buf, baseName, type) < 0) > > + return -1; > > + > > + if (type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && > > + (transitional || nontransitional)) { > > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > > + _("virtio transitional models are not supported " > > + "for address type=%s"), > > + virDomainDeviceAddressTypeToString(type)); > > + return -1; > > + } > > + > > + if (transitional) { > > + virBufferAddLit(buf, "-transitional"); > > + } else if (nontransitional) { > > + virBufferAddLit(buf, "-non-transitional"); > > + } > > + return 0; > > So this only works on QEMU >= 4.0.0 - earlier versions will > fail to start. > > We can, however, make it work correctly with old QEMU. > > A transitional device is 100% identical to the existing device > types, so we can simply not add the "-transitional" suffix for > old QEMU. The only difference is the way libvirt does PCI bus > placement of the transitional device - we'd never use PCIe. > > A non-transitional device is identical to the existing device > types, but with disable-legacy=true set. Again, the relationship between existing and new devices is not quite this straighforward because of the reasons I outlined in https://www.redhat.com/archives/libvir-list/2019-January/msg00514.html

But the idea of using disable-{legacy,modern} instead of the new virtio-*-{non,}-transitional devices is one I had already suggested in https://bugzilla.redhat.com/show_bug.cgi?id=1614127 so I'm obviously on board with it :) > QEMU guarantees this compatibility of the different devices, > but only for machine types < pc-i440fx-4.0.0 / pc-q35-4.0.0. > So we should none the less make sure we use the modern device > names for any QEMU which genuinely supports them. As I already mentioned in the bug report linked above, I'm not quite convinced that's the case, and I don't see why we wouldn't just use the options and basically ignore the QEMU-level devices, as the former approach would work on old QEMU releases as well as recent ones with no drawback I can think of.