On 01/18/2019 07:33 AM, Andrea Bolognani wrote:
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.
Indeed, I'll fix that in all the patches
[...]
+++ 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.
Sure sounds good
It'd also be nice if adding the capabilities and wiring up the command line generation bits happened in separate patches.
[...]
OK
+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?
Hmm, seems weird to make call sites that will never support the transitional naming (virtio-keyboard/mouse/tablet/gpu) have to call into this function, passing DEVICE_TYPE etc. I can split the transitional bits into their own function and not have it wrap BuildVirtioDevStr but be a follow on, so for example: if (qemuBuildVirtioDevStr(...) < 0) goto error; if (qemuBuildVirtioTransitionalStr(...) < ) goto error; Does that work?
[...]
+ 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.
I'll respond separately
[...]
@@ -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 :)
This same pattern is duplicated in the rest of the series, seems weird to comment one site, but commenting all of them is overkill. I guess commenting one site is the middle ground unless you have a better idea of where to put the comment Thanks for the review - Cole