| Message ID | 8a63a2f5e7d1a30d0cc8389790dfb1a799762aa2.1548278585.git.crobinso@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qemu: virtio-{non-}transitional support | expand |
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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, 2019-02-06 at 12:17 -0500, Cole Robinson wrote: > 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... A bit, yes: if you did this, then the only use you'd get out of the virDomainDeviceDef you just reconstructed would be getting a virDomainDeviceInfo out of it... On the other hand, that also means you wouldn't be basically open-coding virDomainDeviceGetInfo() in yet another place. As I said, pick whichever you like the most :) -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 659274f25a..3cf0f5b13e 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -421,6 +421,8 @@ qemuBuildVirtioDevStr(virBufferPtr buf, const char *implName = NULL; virDomainDeviceDef device = { .type = devtype }; virDomainDeviceInfoPtr info; + int tmodel_cap, ntmodel_cap; + bool has_tmodel, has_ntmodel; virDomainDeviceSetData(&device, devdata); info = virDomainDeviceGetInfo(&device); @@ -462,9 +464,78 @@ qemuBuildVirtioDevStr(virBufferPtr buf, virBufferAsprintf(buf, "%s-%s", baseName, implName); + 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; + + case VIR_DOMAIN_DEVICE_LEASE: + case VIR_DOMAIN_DEVICE_FS: + case VIR_DOMAIN_DEVICE_NET: + case VIR_DOMAIN_DEVICE_INPUT: + case VIR_DOMAIN_DEVICE_SOUND: + case VIR_DOMAIN_DEVICE_VIDEO: + case VIR_DOMAIN_DEVICE_HOSTDEV: + case VIR_DOMAIN_DEVICE_WATCHDOG: + case VIR_DOMAIN_DEVICE_CONTROLLER: + case VIR_DOMAIN_DEVICE_GRAPHICS: + case VIR_DOMAIN_DEVICE_HUB: + case VIR_DOMAIN_DEVICE_REDIRDEV: + case VIR_DOMAIN_DEVICE_NONE: + case VIR_DOMAIN_DEVICE_SMARTCARD: + case VIR_DOMAIN_DEVICE_CHR: + case VIR_DOMAIN_DEVICE_MEMBALLOON: + case VIR_DOMAIN_DEVICE_NVRAM: + case VIR_DOMAIN_DEVICE_SHMEM: + case VIR_DOMAIN_DEVICE_TPM: + case VIR_DOMAIN_DEVICE_PANIC: + case VIR_DOMAIN_DEVICE_RNG: + case VIR_DOMAIN_DEVICE_MEMORY: + case VIR_DOMAIN_DEVICE_IOMMU: + case VIR_DOMAIN_DEVICE_VSOCK: + case VIR_DOMAIN_DEVICE_LAST: + default: + return 0; + } + + if (info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && + (has_tmodel || has_ntmodel)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("virtio (non-)transitional models are not " + "supported for address type=%s"), + virDomainDeviceAddressTypeToString(info->type)); + return -1; + } + + 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,disable-modern=off"); + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("virtio non-transitional model not supported " + "for this qemu")); + return -1; + } + } + return 0; } + static int qemuBuildVirtioOptionsStr(virBufferPtr buf, virDomainVirtioOptionsPtr virtio, diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index f2ae0804a8..808e551b9c 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -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 */ case VIR_DOMAIN_DISK_BUS_IDE: diff --git a/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-3.1.0.args b/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-3.1.0.args index 9e11e900da..70446f16f2 100644 --- a/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-3.1.0.args +++ b/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-3.1.0.args @@ -27,8 +27,8 @@ file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \ addr=0x1 \ -device pcie-root-port,port=0x9,chassis=2,id=pci.2,bus=pcie.0,addr=0x1.0x1 \ -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-virtio-disk0 \ --device virtio-blk-pci,scsi=off,bus=pci.1,addr=0x0,drive=drive-virtio-disk0,\ -id=virtio-disk0,bootindex=1 \ +-device virtio-blk-pci,disable-legacy=on,disable-modern=off,scsi=off,bus=pci.1,\ +addr=0x0,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1 \ -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ resourcecontrol=deny \ -msg timestamp=on diff --git a/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-latest.args b/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-latest.args index 070b4b8334..37078765bc 100644 --- a/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-latest.args +++ b/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-latest.args @@ -27,8 +27,8 @@ file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \ addr=0x1 \ -device pcie-root-port,port=0x9,chassis=2,id=pci.2,bus=pcie.0,addr=0x1.0x1 \ -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-virtio-disk0 \ --device virtio-blk-pci,scsi=off,bus=pci.1,addr=0x0,drive=drive-virtio-disk0,\ -id=virtio-disk0,bootindex=1 \ +-device virtio-blk-pci-non-transitional,scsi=off,bus=pci.1,addr=0x0,\ +drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1 \ -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ resourcecontrol=deny \ -msg timestamp=on diff --git a/tests/qemuxml2argvdata/virtio-transitional.x86_64-3.1.0.args b/tests/qemuxml2argvdata/virtio-transitional.x86_64-3.1.0.args index 9e11e900da..356e8fdf4c 100644 --- a/tests/qemuxml2argvdata/virtio-transitional.x86_64-3.1.0.args +++ b/tests/qemuxml2argvdata/virtio-transitional.x86_64-3.1.0.args @@ -25,9 +25,10 @@ file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \ -boot strict=on \ -device pcie-root-port,port=0x8,chassis=1,id=pci.1,bus=pcie.0,multifunction=on,\ addr=0x1 \ --device pcie-root-port,port=0x9,chassis=2,id=pci.2,bus=pcie.0,addr=0x1.0x1 \ +-device pcie-pci-bridge,id=pci.2,bus=pci.1,addr=0x0 \ +-device pcie-root-port,port=0x9,chassis=3,id=pci.3,bus=pcie.0,addr=0x1.0x1 \ -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-virtio-disk0 \ --device virtio-blk-pci,scsi=off,bus=pci.1,addr=0x0,drive=drive-virtio-disk0,\ +-device virtio-blk-pci,scsi=off,bus=pci.2,addr=0x1,drive=drive-virtio-disk0,\ id=virtio-disk0,bootindex=1 \ -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ resourcecontrol=deny \ diff --git a/tests/qemuxml2argvdata/virtio-transitional.x86_64-latest.args b/tests/qemuxml2argvdata/virtio-transitional.x86_64-latest.args index 070b4b8334..e78223eac8 100644 --- a/tests/qemuxml2argvdata/virtio-transitional.x86_64-latest.args +++ b/tests/qemuxml2argvdata/virtio-transitional.x86_64-latest.args @@ -25,10 +25,11 @@ file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \ -boot strict=on \ -device pcie-root-port,port=0x8,chassis=1,id=pci.1,bus=pcie.0,multifunction=on,\ addr=0x1 \ --device pcie-root-port,port=0x9,chassis=2,id=pci.2,bus=pcie.0,addr=0x1.0x1 \ +-device pcie-pci-bridge,id=pci.2,bus=pci.1,addr=0x0 \ +-device pcie-root-port,port=0x9,chassis=3,id=pci.3,bus=pcie.0,addr=0x1.0x1 \ -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-virtio-disk0 \ --device virtio-blk-pci,scsi=off,bus=pci.1,addr=0x0,drive=drive-virtio-disk0,\ -id=virtio-disk0,bootindex=1 \ +-device virtio-blk-pci-transitional,scsi=off,bus=pci.2,addr=0x1,\ +drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1 \ -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ resourcecontrol=deny \ -msg timestamp=on diff --git a/tests/qemuxml2xmloutdata/virtio-transitional.xml b/tests/qemuxml2xmloutdata/virtio-transitional.xml index 1d28af9abb..c19e133bb3 100644 --- a/tests/qemuxml2xmloutdata/virtio-transitional.xml +++ b/tests/qemuxml2xmloutdata/virtio-transitional.xml @@ -18,7 +18,7 @@ <driver name='qemu' type='raw'/> <source dev='/dev/HostVG/QEMUGuest1'/> <target dev='vda' bus='virtio'/> - <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> + <address type='pci' domain='0x0000' bus='0x02' slot='0x01' function='0x0'/> </disk> <controller type='usb' index='0' model='none'/> <controller type='sata' index='0'> @@ -30,9 +30,13 @@ <target chassis='1' port='0x8'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0' multifunction='on'/> </controller> - <controller type='pci' index='2' model='pcie-root-port'> + <controller type='pci' index='2' model='pcie-to-pci-bridge'> + <model name='pcie-pci-bridge'/> + <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> + </controller> + <controller type='pci' index='3' model='pcie-root-port'> <model name='pcie-root-port'/> - <target chassis='2' port='0x9'/> + <target chassis='3' port='0x9'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> </controller> <input type='mouse' bus='ps2'/>
Add new <disk> model values for virtio transitional devices. When combined with bus='virtio': * "virtio-transitional" maps to qemu "virtio-blk-pci-transitional" * "virtio-non-transitional" maps to qemu "virtio-blk-pci-non-transitional" Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_command.c | 71 +++++++++++++++++++ src/qemu/qemu_domain_address.c | 3 + .../virtio-non-transitional.x86_64-3.1.0.args | 4 +- ...virtio-non-transitional.x86_64-latest.args | 4 +- .../virtio-transitional.x86_64-3.1.0.args | 5 +- .../virtio-transitional.x86_64-latest.args | 7 +- .../virtio-transitional.xml | 10 ++- 7 files changed, 92 insertions(+), 12 deletions(-) -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list