Commit 10c73bf1 fixed a bug that I had introduced back in commit 70249927 - if a vhost-scsi device had no manually assigned PCI address, one wouldn't be assigned automatically. There was a slight problem with the logic of the fix though - in the case of domains with pcie-root (e.g. those with a q35 machinetype), qemuDomainDeviceCalculatePCIConnectFlags() will attempt to determine if the host-side PCI device is Express or legacy by examining sysfs based on the host-side PCI address stored in hostdev->source.subsys.u.pci.addr, but that part of the union is only valid for PCI hostdevs, *not* for SCSI hostdevs. So we end up trying to read sysfs for some probably-non-existent device, which fails, and the function virPCIDeviceIsPCIExpress() returns failure (-1). By coincidence, the return value is being examined as a boolean, and since -1 is true, we still end up assigning the vhost-scsi device to an Express slot, but that is just be chance (and could fail in the case that the gibberish in the "hostside PCI address" was the address of a real device that happened to be legacy PCI). Since (according to Paolo Bonzini at least) vhost-scsi devices appear just like virtio-scsi devices in the guest, they should follow the same rules as virtio devices when deciding whether they should be placed in an Express or a legacy slot. That's accomplished in this patch by returning early with virtioFlags, rather than erroneously using hostdev->source.subsys.u.pci.addr. It also adds a test case for PCIe to assure it doesn't get broken in the future. --- src/qemu/qemu_domain_address.c | 7 ++++ .../hostdev-scsi-vhost-scsi-pcie.args | 25 ++++++++++++++ .../hostdev-scsi-vhost-scsi-pcie.xml | 23 +++++++++++++ tests/qemuxml2argvtest.c | 7 ++++ .../hostdev-scsi-vhost-scsi-pcie.xml | 40 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 7 ++++ 6 files changed, 109 insertions(+) create mode 100644 tests/qemuxml2argvdata/hostdev-scsi-vhost-scsi-pcie.args create mode 100644 tests/qemuxml2argvdata/hostdev-scsi-vhost-scsi-pcie.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-scsi-vhost-scsi-pcie.xml diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 6e7561d39..600de85f8 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -656,6 +656,13 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev, if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV) return pcieFlags; + /* according to pbonzini, from the guest PoV vhost-scsi devices + * are the same as virtio-scsi, so they should use virtioFlags + * (same as virtio-scsi) to determine Express vs. legacy placement + */ + if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST) + return virtioFlags; + if (!(pciDev = virPCIDeviceNew(hostAddr->domain, hostAddr->bus, hostAddr->slot, diff --git a/tests/qemuxml2argvdata/hostdev-scsi-vhost-scsi-pcie.args b/tests/qemuxml2argvdata/hostdev-scsi-vhost-scsi-pcie.args new file mode 100644 index 000000000..296716f58 --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-scsi-vhost-scsi-pcie.args @@ -0,0 +1,25 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-x86_64 \ +-name QEMUGuest2 \ +-S \ +-M pc-q35-2.7 \ +-m 214 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9466-926a-d65c16db1809 \ +-nographic \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest2/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=readline \ +-no-acpi \ +-boot c \ +-device pcie-root-port,port=0x10,chassis=1,id=pci.1,bus=pcie.0,\ +multifunction=on,addr=0x2 \ +-device pcie-root-port,port=0x11,chassis=2,id=pci.2,bus=pcie.0,addr=0x2.0x1 \ +-device vhost-scsi-pci,wwpn=naa.5123456789abcde0,vhostfd=3,id=hostdev0,\ +bus=pci.1,addr=0x0 diff --git a/tests/qemuxml2argvdata/hostdev-scsi-vhost-scsi-pcie.xml b/tests/qemuxml2argvdata/hostdev-scsi-vhost-scsi-pcie.xml new file mode 100644 index 000000000..c4e8c33a9 --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-scsi-vhost-scsi-pcie.xml @@ -0,0 +1,23 @@ +<domain type='kvm'> + <name>QEMUGuest2</name> + <uuid>c7a5fdbd-edaf-9466-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc-q35-2.7'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' index='0' model='none'/> + <hostdev mode='subsystem' type='scsi_host' managed='no'> + <source protocol='vhost' wwpn='naa.5123456789abcde0'/> + </hostdev> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index ca24e0bbb..d5b2881e7 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2465,6 +2465,13 @@ mymain(void) DO_TEST("hostdev-scsi-vhost-scsi-pci", QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_DEVICE_VHOST_SCSI, QEMU_CAPS_DEVICE_SCSI_GENERIC); + DO_TEST("hostdev-scsi-vhost-scsi-pcie", + QEMU_CAPS_KVM, + QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_DEVICE_VHOST_SCSI, + QEMU_CAPS_DEVICE_SCSI_GENERIC, + QEMU_CAPS_PCI_MULTIFUNCTION, + QEMU_CAPS_DEVICE_PCIE_ROOT_PORT, + QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY); DO_TEST("mlock-on", QEMU_CAPS_REALTIME_MLOCK); DO_TEST_FAILURE("mlock-on", NONE); diff --git a/tests/qemuxml2xmloutdata/hostdev-scsi-vhost-scsi-pcie.xml b/tests/qemuxml2xmloutdata/hostdev-scsi-vhost-scsi-pcie.xml new file mode 100644 index 000000000..ef48d8123 --- /dev/null +++ b/tests/qemuxml2xmloutdata/hostdev-scsi-vhost-scsi-pcie.xml @@ -0,0 +1,40 @@ +<domain type='kvm'> + <name>QEMUGuest2</name> + <uuid>c7a5fdbd-edaf-9466-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc-q35-2.7'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' index='0' model='none'/> + <controller type='sata' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='pcie-root-port'> + <model name='pcie-root-port'/> + <target chassis='1' port='0x10'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0' multifunction='on'/> + </controller> + <controller type='pci' index='2' model='pcie-root-port'> + <model name='pcie-root-port'/> + <target chassis='2' port='0x11'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x1'/> + </controller> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <hostdev mode='subsystem' type='scsi_host' managed='no'> + <source protocol='vhost' wwpn='naa.5123456789abcde0'/> + <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> + </hostdev> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 2be8eb2c1..78519f509 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -1109,6 +1109,13 @@ mymain(void) DO_TEST("hostdev-scsi-vhost-scsi-pci", QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_DEVICE_VHOST_SCSI, QEMU_CAPS_DEVICE_SCSI_GENERIC); + DO_TEST("hostdev-scsi-vhost-scsi-pcie", + QEMU_CAPS_KVM, + QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_DEVICE_VHOST_SCSI, + QEMU_CAPS_DEVICE_SCSI_GENERIC, + QEMU_CAPS_PCI_MULTIFUNCTION, + QEMU_CAPS_DEVICE_PCIE_ROOT_PORT, + QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY); DO_TEST("hostdev-scsi-lsi", QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_SCSI_LSI, QEMU_CAPS_DEVICE_SCSI_GENERIC); -- 2.13.6

qemuDomainDefValidateVideo() is just a loop performing various checks on each video device. Rather than maintaining this outlyer function called from qemuDomainDefValidateVideo(), just fold the validations into qemuDomainDeviceDefValidateVideo(), which is called once for each video device (my guess is that ...DeviceDefValidateVideo() didn't exist yet when ...DomainDefValidateVideo() was added, but I haven't verified this). --- I randomly noticed this when looking up something else in the validation code... src/qemu/qemu_domain.c | 151 +++++++++++++++++++++---------------------------- 1 file changed, 66 insertions(+), 85 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 74b82450b..2ca45fde2 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3216,79 +3216,6 @@ qemuDomainDefPostParse(virDomainDefPtr def, } -static int -qemuDomainDefValidateVideo(const virDomainDef *def) -{ - size_t i; - virDomainVideoDefPtr video; - - for (i = 0; i < def->nvideos; i++) { - video = def->videos[i]; - - switch (video->type) { - case VIR_DOMAIN_VIDEO_TYPE_XEN: - case VIR_DOMAIN_VIDEO_TYPE_VBOX: - case VIR_DOMAIN_VIDEO_TYPE_PARALLELS: - case VIR_DOMAIN_VIDEO_TYPE_DEFAULT: - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("video type '%s' is not supported with QEMU"), - virDomainVideoTypeToString(video->type)); - return -1; - case VIR_DOMAIN_VIDEO_TYPE_VGA: - case VIR_DOMAIN_VIDEO_TYPE_CIRRUS: - case VIR_DOMAIN_VIDEO_TYPE_VMVGA: - case VIR_DOMAIN_VIDEO_TYPE_QXL: - case VIR_DOMAIN_VIDEO_TYPE_VIRTIO: - case VIR_DOMAIN_VIDEO_TYPE_LAST: - break; - } - - if (!video->primary && - video->type != VIR_DOMAIN_VIDEO_TYPE_QXL && - video->type != VIR_DOMAIN_VIDEO_TYPE_VIRTIO) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("video type '%s' is only valid as primary " - "video device"), - virDomainVideoTypeToString(video->type)); - return -1; - } - - if (video->accel && video->accel->accel2d == VIR_TRISTATE_SWITCH_ON) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("qemu does not support the accel2d setting")); - return -1; - } - - if (video->type == VIR_DOMAIN_VIDEO_TYPE_QXL) { - if (video->vram > (UINT_MAX / 1024)) { - virReportError(VIR_ERR_OVERFLOW, - _("value for 'vram' must be less than '%u'"), - UINT_MAX / 1024); - return -1; - } - if (video->ram > (UINT_MAX / 1024)) { - virReportError(VIR_ERR_OVERFLOW, - _("value for 'ram' must be less than '%u'"), - UINT_MAX / 1024); - return -1; - } - } - - if (video->type == VIR_DOMAIN_VIDEO_TYPE_VGA || - video->type == VIR_DOMAIN_VIDEO_TYPE_VMVGA) { - if (video->vram && video->vram < 1024) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - "%s", _("value for 'vram' must be at least " - "1 MiB (1024 KiB)")); - return -1; - } - } - } - - return 0; -} - - #define QEMU_MAX_VCPUS_WITHOUT_EIM 255 @@ -3388,9 +3315,6 @@ qemuDomainDefValidate(const virDomainDef *def, } } - if (qemuDomainDefValidateVideo(def) < 0) - goto cleanup; - ret = 0; cleanup: @@ -3851,18 +3775,75 @@ qemuDomainDeviceDefValidateHostdev(const virDomainHostdevDef *hostdev, static int qemuDomainDeviceDefValidateVideo(const virDomainVideoDef *video) { - if (video->type == VIR_DOMAIN_VIDEO_TYPE_QXL && - video->vgamem) { - if (video->vgamem < 1024) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("value for 'vgamem' must be at least 1 MiB " - "(1024 KiB)")); + switch (video->type) { + case VIR_DOMAIN_VIDEO_TYPE_XEN: + case VIR_DOMAIN_VIDEO_TYPE_VBOX: + case VIR_DOMAIN_VIDEO_TYPE_PARALLELS: + case VIR_DOMAIN_VIDEO_TYPE_DEFAULT: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("video type '%s' is not supported with QEMU"), + virDomainVideoTypeToString(video->type)); + return -1; + case VIR_DOMAIN_VIDEO_TYPE_VGA: + case VIR_DOMAIN_VIDEO_TYPE_CIRRUS: + case VIR_DOMAIN_VIDEO_TYPE_VMVGA: + case VIR_DOMAIN_VIDEO_TYPE_QXL: + case VIR_DOMAIN_VIDEO_TYPE_VIRTIO: + case VIR_DOMAIN_VIDEO_TYPE_LAST: + break; + } + + if (!video->primary && + video->type != VIR_DOMAIN_VIDEO_TYPE_QXL && + video->type != VIR_DOMAIN_VIDEO_TYPE_VIRTIO) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("video type '%s' is only valid as primary " + "video device"), + virDomainVideoTypeToString(video->type)); + return -1; + } + + if (video->accel && video->accel->accel2d == VIR_TRISTATE_SWITCH_ON) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("qemu does not support the accel2d setting")); + return -1; + } + + if (video->type == VIR_DOMAIN_VIDEO_TYPE_QXL) { + if (video->vram > (UINT_MAX / 1024)) { + virReportError(VIR_ERR_OVERFLOW, + _("value for 'vram' must be less than '%u'"), + UINT_MAX / 1024); + return -1; + } + if (video->ram > (UINT_MAX / 1024)) { + virReportError(VIR_ERR_OVERFLOW, + _("value for 'ram' must be less than '%u'"), + UINT_MAX / 1024); return -1; } + if (video->vgamem) { + if (video->vgamem < 1024) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("value for 'vgamem' must be at least 1 MiB " + "(1024 KiB)")); + return -1; + } - if (video->vgamem != VIR_ROUND_UP_POWER_OF_TWO(video->vgamem)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("value for 'vgamem' must be power of two")); + if (video->vgamem != VIR_ROUND_UP_POWER_OF_TWO(video->vgamem)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("value for 'vgamem' must be power of two")); + return -1; + } + } + } + + if (video->type == VIR_DOMAIN_VIDEO_TYPE_VGA || + video->type == VIR_DOMAIN_VIDEO_TYPE_VMVGA) { + if (video->vram && video->vram < 1024) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + "%s", _("value for 'vram' must be at least " + "1 MiB (1024 KiB)")); return -1; } } -- 2.13.6