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

On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote: [...] > @@ -1108,6 +1110,8 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { > { "vfio-ap", QEMU_CAPS_DEVICE_VFIO_AP }, > { "zpci", QEMU_CAPS_DEVICE_ZPCI }, > { "memory-backend-memfd", QEMU_CAPS_OBJECT_MEMORY_MEMFD }, > + {"virtio-blk-pci-transitional", QEMU_CAPS_DEVICE_VIRTIO_BLK_TRANSITIONAL}, > + {"virtio-blk-pci-non-transitional", QEMU_CAPS_DEVICE_VIRTIO_BLK_NON_TRANSITIONAL}, There should be whitespace before and after curly braces, for consistency with existing entries.

[...] > +++ b/src/qemu/qemu_capabilities.h > @@ -504,6 +504,8 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ > /* 325 */ > QEMU_CAPS_OBJECT_MEMORY_FILE_PMEM, /* -object memory-backend-file,pmem= */ > QEMU_CAPS_DEVICE_NVDIMM_UNARMED, /* -device nvdimm,unarmed= */ > + QEMU_CAPS_DEVICE_VIRTIO_BLK_TRANSITIONAL, /* -device virtio-blk-pci-transitional */ > + QEMU_CAPS_DEVICE_VIRTIO_BLK_NON_TRANSITIONAL, /* -device virtio-blk-pci-non-transitional */ A bit more verbose, but I think we should go for QEMU_CAPS_DEVICE_VIRTIO_BLK_PCI_{,NON_}TRANSITIONAL since there are several non-PCI variants of VirtIO devices.

It'd also be nice if adding the capabilities and wiring up the command line generation bits happened in separate patches. [...]

> +static int > +qemuBuildVirtioTransitional(virBufferPtr buf, > + const char *baseName, > + virQEMUCapsPtr qemuCaps, > + virDomainDeviceAddressType type, > + int model, > + virDomainDeviceType devtype) > +{ > + int tmodel_cap, ntmodel_cap; > + bool has_tmodel, has_ntmodel; > + > + if (qemuBuildVirtioDevStr(buf, baseName, type) < 0) > + return -1; > + > + switch (devtype) { > + case VIR_DOMAIN_DEVICE_DISK: > + has_tmodel = model == VIR_DOMAIN_DISK_MODEL_VIRTIO_TRANSITIONAL; > + has_ntmodel = model == VIR_DOMAIN_DISK_MODEL_VIRTIO_NON_TRANSITIONAL; > + tmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_BLK_TRANSITIONAL; > + ntmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_BLK_NON_TRANSITIONAL; > + break; I like this approach much better than the one used in the RFC, eg. passing two booleans to the function. Nice! What I don't like is that you're building this fairly thin wrapper around qemuBuildVirtioDevStr() when IMHO you should rather be agumenting the original function - mostly because the new name is not nearly as good :) Do you think you could make that happen?

[...] > + if (type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && > + (has_tmodel || has_ntmodel)) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("virtio transitional models are not supported " > + "for address type=%s"), s/transitional/(non-)transitional/ [...] > + 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.

[...] > @@ -723,6 +723,8 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev, > case VIR_DOMAIN_DEVICE_DISK: > switch ((virDomainDiskBus) dev->data.disk->bus) { > case VIR_DOMAIN_DISK_BUS_VIRTIO: > + if (dev->data.disk->model == VIR_DOMAIN_DISK_MODEL_VIRTIO_TRANSITIONAL) > + return pciFlags; Perhaps a short comment about how transitional VirtIO devices can only be plugged into conventional PCI slots would be appropriate here, for the benefit of those looking at the code years from now :)