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?