On 1/29/19 5:25 AM, Andrea Bolognani wrote:
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.
Makes for shorter lines which is nice but kind of offsets the benefit of
converting to virDomainDeviceSetData in the first place...
[...]
+ 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?
Good point, i'll change it
[...]
+ } 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?
ACK to all these changes too
Thanks,
Cole
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
