Re: [libvirt] [PATCH v2 12/25] qemu: Support disk model=virtio-{non-}transitional
On Wed, 2019-01-23 at 16:32 -0500, Cole Robinson wrote:
[...]
+ switch (devtype) {
+ case VIR_DOMAIN_DEVICE_DISK:
+ has_tmodel = device.data.disk->model ==
VIR_DOMAIN_DISK_MODEL_VIRTIO_TRANSITIONAL;
+ has_ntmodel = device.data.disk->model ==
VIR_DOMAIN_DISK_MODEL_VIRTIO_NON_TRANSITIONAL;
+ tmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_BLK_PCI_TRANSITIONAL;
+ ntmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_BLK_PCI_NON_TRANSITIONAL;
+ break;
I wonder if this would look slightly nicer as
case VIR_DOMAIN_DEVICE_DISK: {
virDomainDiskDefPtr disk = (virDomainDiskDefPtr) devdata;
has_tmodel = disk->model == VIR_DOMAIN_DISK_MODEL_VIRTIO_TRANSITIONAL;
has_ntmodel = disk->model == VIR_DOMAIN_DISK_MODEL_VIRTIO_NON_TRANSITIONAL;
tmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_BLK_PCI_TRANSITIONAL;
ntmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_BLK_PCI_NON_TRANSITIONAL;
break;
}
but up to you, really.
[...]
+ if (has_tmodel) {
+ if (virQEMUCapsGet(qemuCaps, tmodel_cap))
+ virBufferAddLit(buf, "-transitional");
You're definitely using qemuCaps now, so you should remove
ATTRIBUTE_UNUSED from the parameter in the function signature.
+ /* 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
+ */
While this is absolutely correct and we should definitely not error
out if the transitional device is not available, perhaps for the
benefit of someone looking at the generated QEMU command line for
debugging purposes we could do the same as below and also print
disable-legacy=off,disable-modern=off
if the corresponding options are available - we would still not
error out if they aren't, of course. Sounds reasonable?
[...]
+ } 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,disable-modern=off");
Maybe add something like
/* Even if the QEMU binary doesn't support the non-transitional
* device, we can still make it work by manually disabling legacy
* VirtIO and enabling modern VirtIO */
here.
[...]
}
+
static int
qemuBuildVirtioOptionsStr(virBufferPtr buf,
virDomainVirtioOptionsPtr virtio,
Extraneous whitespace change.
[...]
@@ -720,6 +720,9 @@
qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
case VIR_DOMAIN_DEVICE_DISK:
switch ((virDomainDiskBus) dev->data.disk->bus) {
case VIR_DOMAIN_DISK_BUS_VIRTIO:
+ /* Transitional devices only work in conventional PCI slots */
+ if (dev->data.disk->model ==
VIR_DOMAIN_DISK_MODEL_VIRTIO_TRANSITIONAL)
+ return pciFlags;
return virtioFlags; /* only virtio disks use PCI */
Can please you use the same kind of switch statement you later use
for eg. input devices here?
--
Andrea Bolognani / Red Hat / Virtualization