Re: [libvirt] [PATCH 05/11] qemu_process: move video validation out of qemu_command

On 09/30/2016 12:02 PM, Pavel Hrdina wrote: > Runtime validation that depend on qemu capabilities should be moved > into qemuProcessStartValidateXML. > > Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; > --- > src/qemu/qemu_command.c | 33 +------------------- > src/qemu/qemu_process.c | 50 ++++++++++++++++++++++++++++++- > tests/qemuxml2argvtest.c | 78 +++++++++++++++++++++++++++++------------------- > 3 files changed, 98 insertions(+), 63 deletions(-) > Could the qemuxml2argvtest.c to add QEMU_CAPS_DEVICE_CIRRUS_VGA move to earlier? Perhaps there is a relationship, but it's not clear from the commit message...

> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index 761968b..d283c67 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -4312,26 +4312,12 @@ qemuBuildDeviceVideoStr(const virDomainDef *def, > model = "virtio-gpu-pci"; > } > } else { > - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QXL)) { > - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > - "%s", _("only one video card is currently supported")); > - goto error; > - } > - Just checking - does the removal of this end up as part of the (now) singular video loop in qemuProcessStartValidateVideo? Same for the removed check in qemuBuildVideoCommandLine? It wasn't "clear" the first pass through reading...

> model = "qxl"; > } > > virBufferAsprintf(&buf, "%s,id=%s", model, video->info.alias); > > if (video->accel && video->accel->accel3d == VIR_TRISTATE_SWITCH_ON) { > - if (video->type != VIR_DOMAIN_VIDEO_TYPE_VIRTIO || > - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_GPU_VIRGL)) { > - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > - _("%s 3d acceleration is not supported"), > - virDomainVideoTypeToString(video->type)); > - goto error; > - } > - > virBufferAsprintf(&buf, ",virgl=%s", > virTristateSwitchTypeToString(video->accel->accel3d)); > } > @@ -4397,17 +4383,7 @@ qemuBuildVideoCommandLine(virCommandPtr cmd, > > primaryVideoType = def->videos[0]->type; > > - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY) && > - ((primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_VGA && > - virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VGA)) || > - (primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_CIRRUS && > - virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_CIRRUS_VGA)) || > - (primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_VMVGA && > - virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VMWARE_SVGA)) || > - (primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_QXL && > - virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QXL)) || > - (primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_VIRTIO && > - virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_GPU)))) { > + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY)) { > for (i = 0; i < def->nvideos; i++) { > char *str; > virCommandAddArg(cmd, "-device"); > @@ -4422,13 +4398,6 @@ qemuBuildVideoCommandLine(virCommandPtr cmd, > if (primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_XEN) { > /* nothing - vga has no effect on Xen pvfb */ > } else { > - if ((primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_QXL) && > - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QXL)) { > - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > - _("This QEMU does not support QXL graphics adapters")); > - return -1; > - } > - ... Is it safe to assume there can only be one video card here?

> const char *vgastr = qemuVideoTypeToString(primaryVideoType); > if (!vgastr || STREQ(vgastr, "")) { > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > index 6ddcc1f..eb3add9 100644 > --- a/src/qemu/qemu_process.c > +++ b/src/qemu/qemu_process.c > @@ -4448,6 +4448,48 @@ qemuProcessStartValidateGraphics(virDomainObjPtr vm) > > > static int > +qemuProcessStartValidateVideo(virDomainObjPtr vm, > + virQEMUCapsPtr qemuCaps) > +{ > + size_t i; > + virDomainVideoDefPtr video; > + > + for (i = 0; i < vm->def->nvideos; i++) { > + video = vm->def->videos[i]; > + > + if ((video->type == VIR_DOMAIN_VIDEO_TYPE_VGA && > + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VGA)) || > + (video->type == VIR_DOMAIN_VIDEO_TYPE_CIRRUS && > + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_CIRRUS_VGA)) || > + (video->type == VIR_DOMAIN_VIDEO_TYPE_VMVGA && > + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VMWARE_SVGA)) || > + (video->type == VIR_DOMAIN_VIDEO_TYPE_QXL && > + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QXL)) || > + (video->type == VIR_DOMAIN_VIDEO_TYPE_VIRTIO && > + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_GPU))) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("this QEMU does not support '%s' video device"), > + virDomainVideoTypeToString(video->type)); > + return -1; > + } This is a different check to the moved qemuBuildVideoCommandLine code. 1. The video->types were only checked if DEVICE_VIDEO_PRIMARY was set. IOW: This is adding checks and failure scenario that weren't being checked previously in the event that cap isn't set.

2. If DEVICE_VIDEO_PRIMARY, then only if the primary (video[0]) was one of the types would the corresponding cap be checked. This change will check each video[N] and each cap.

Since qemuDomainDefValidateVideo would have been called before this and that video[0] == video->primary, then we know video[1]..video[n] can only be VIDEO_TYPE_QXL (so far), so I think this check can be augmented to match that.

For sure the non primary video checks need the QEMU_CAPS_DEVICE_QXL because that is what was removed from qemuBuildDeviceVideoStr. I think this is close, just needs a tweak. Unless you have it as "innate knowledge" that videos[0] == video->primary, it can be trying to look at

code that sometimes keys off of video->primary while other code keys off videos[0] is primary. > + > + if (video->accel) { > + if (video->accel->accel3d == VIR_TRISTATE_SWITCH_ON && > + (video->type != VIR_DOMAIN_VIDEO_TYPE_VIRTIO || > + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_GPU_VIRGL))) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("%s 3d acceleration is not supported"), > + virDomainVideoTypeToString(video->type)); > + return -1; > + } > + } > + } > + > + return 0; > +} > + > + > +static int > qemuProcessStartValidateXML(virQEMUDriverPtr driver, > virDomainObjPtr vm, > virQEMUCapsPtr qemuCaps, > @@ -4517,11 +4559,17 @@ qemuProcessStartValidate(virQEMUDriverPtr driver, > if (qemuProcessStartValidateXML(driver, vm, qemuCaps, caps, flags) < 0) > return -1; > > + if (qemuProcessStartValidateGraphics(vm) < 0) > + return -1; This is a separate "bug fix" due to 'df73f1db8' adding code in that "non-fatal" issues section (whether it's a merge thing or not, I don't recall). But it should be separate.

> + > + if (qemuProcessStartValidateVideo(vm, qemuCaps) < 0) > + return -1; > + > VIR_DEBUG("Checking for any possible (non-fatal) issues"); > > qemuProcessStartWarnShmem(vm); > > - return qemuProcessStartValidateGraphics(vm); > + return 0; > } > As noted earlier, I think the rest of this should be separate. John > > diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c > index 78bfb92..1e910f3 100644 > --- a/tests/qemuxml2argvtest.c > +++ b/tests/qemuxml2argvtest.c > @@ -748,7 +748,8 @@ mymain(void) > QEMU_CAPS_PCI_MULTIFUNCTION, QEMU_CAPS_DRIVE_AIO, > QEMU_CAPS_CCID_PASSTHRU, QEMU_CAPS_CHARDEV, > QEMU_CAPS_CHARDEV_SPICEVMC, QEMU_CAPS_SPICE, > - QEMU_CAPS_HDA_DUPLEX, QEMU_CAPS_USB_HUB); > + QEMU_CAPS_HDA_DUPLEX, QEMU_CAPS_USB_HUB, > + QEMU_CAPS_DEVICE_CIRRUS_VGA); > DO_TEST("eoi-disabled", NONE); > DO_TEST("eoi-enabled", NONE); > DO_TEST("pv-spinlock-disabled", NONE); > @@ -963,32 +964,39 @@ mymain(void) > QEMU_CAPS_KVM, > QEMU_CAPS_DRIVE_SERIAL); > > - DO_TEST("graphics-vnc", QEMU_CAPS_VNC); > - DO_TEST("graphics-vnc-socket", QEMU_CAPS_VNC); > - DO_TEST("graphics-vnc-websocket", QEMU_CAPS_VNC, QEMU_CAPS_VNC_WEBSOCKET); > - DO_TEST("graphics-vnc-policy", QEMU_CAPS_VNC, QEMU_CAPS_VNC_SHARE_POLICY); > - DO_TEST("graphics-vnc-no-listen-attr", QEMU_CAPS_VNC); > - DO_TEST("graphics-vnc-remove-generated-socket", QEMU_CAPS_VNC); > + DO_TEST("graphics-vnc", QEMU_CAPS_VNC, QEMU_CAPS_DEVICE_CIRRUS_VGA); > + DO_TEST("graphics-vnc-socket", QEMU_CAPS_VNC, QEMU_CAPS_DEVICE_CIRRUS_VGA); > + DO_TEST("graphics-vnc-websocket", QEMU_CAPS_VNC, QEMU_CAPS_VNC_WEBSOCKET, > + QEMU_CAPS_DEVICE_CIRRUS_VGA); > + DO_TEST("graphics-vnc-policy", QEMU_CAPS_VNC, QEMU_CAPS_VNC_SHARE_POLICY, > + QEMU_CAPS_DEVICE_CIRRUS_VGA); > + DO_TEST("graphics-vnc-no-listen-attr", QEMU_CAPS_VNC, > + QEMU_CAPS_DEVICE_CIRRUS_VGA); > + DO_TEST("graphics-vnc-remove-generated-socket", QEMU_CAPS_VNC, > + QEMU_CAPS_DEVICE_CIRRUS_VGA); > driver.config->vncAutoUnixSocket = true; > - DO_TEST("graphics-vnc-auto-socket-cfg", QEMU_CAPS_VNC); > + DO_TEST("graphics-vnc-auto-socket-cfg", QEMU_CAPS_VNC, > + QEMU_CAPS_DEVICE_CIRRUS_VGA); > driver.config->vncAutoUnixSocket = false; > - DO_TEST("graphics-vnc-socket", QEMU_CAPS_VNC); > - DO_TEST("graphics-vnc-auto-socket", QEMU_CAPS_VNC); > - DO_TEST("graphics-vnc-none", QEMU_CAPS_VNC); > + DO_TEST("graphics-vnc-socket", QEMU_CAPS_VNC, QEMU_CAPS_DEVICE_CIRRUS_VGA); > + DO_TEST("graphics-vnc-auto-socket", QEMU_CAPS_VNC, > + QEMU_CAPS_DEVICE_CIRRUS_VGA); > + DO_TEST("graphics-vnc-none", QEMU_CAPS_VNC, QEMU_CAPS_DEVICE_CIRRUS_VGA); > > driver.config->vncSASL = 1; > VIR_FREE(driver.config->vncSASLdir); > ignore_value(VIR_STRDUP(driver.config->vncSASLdir, "/root/.sasl2")); > - DO_TEST("graphics-vnc-sasl", QEMU_CAPS_VNC); > + DO_TEST("graphics-vnc-sasl", QEMU_CAPS_VNC, QEMU_CAPS_DEVICE_CIRRUS_VGA); > driver.config->vncTLS = 1; > driver.config->vncTLSx509verify = 1; > - DO_TEST("graphics-vnc-tls", QEMU_CAPS_VNC); > + DO_TEST("graphics-vnc-tls", QEMU_CAPS_VNC, QEMU_CAPS_DEVICE_CIRRUS_VGA); > driver.config->vncSASL = driver.config->vncTLSx509verify = driver.config->vncTLS = 0; > VIR_FREE(driver.config->vncSASLdir); > VIR_FREE(driver.config->vncTLSx509certdir); > > - DO_TEST("graphics-sdl", QEMU_CAPS_SDL); > - DO_TEST("graphics-sdl-fullscreen", QEMU_CAPS_SDL); > + DO_TEST("graphics-sdl", QEMU_CAPS_SDL, QEMU_CAPS_DEVICE_VGA); > + DO_TEST("graphics-sdl-fullscreen", QEMU_CAPS_SDL, > + QEMU_CAPS_DEVICE_CIRRUS_VGA); > DO_TEST("nographics", NONE); > DO_TEST("nographics-display", > QEMU_CAPS_DISPLAY); > @@ -999,7 +1007,7 @@ mymain(void) > QEMU_CAPS_DEVICE_QXL, > QEMU_CAPS_SPICE_FILE_XFER_DISABLE); > DO_TEST("graphics-spice-no-args", > - QEMU_CAPS_SPICE); > + QEMU_CAPS_SPICE, QEMU_CAPS_DEVICE_CIRRUS_VGA); > driver.config->spiceSASL = 1; > ignore_value(VIR_STRDUP(driver.config->spiceSASLdir, "/root/.sasl2")); > DO_TEST("graphics-spice-sasl", > @@ -1011,14 +1019,16 @@ mymain(void) > QEMU_CAPS_DEVICE_QXL, > QEMU_CAPS_SPICE, > QEMU_CAPS_CHARDEV_SPICEVMC, > - QEMU_CAPS_NODEFCONFIG); > + QEMU_CAPS_NODEFCONFIG, > + QEMU_CAPS_DEVICE_CIRRUS_VGA); > DO_TEST("graphics-spice-compression", > QEMU_CAPS_SPICE, > QEMU_CAPS_DEVICE_QXL); > DO_TEST("graphics-spice-timeout", > QEMU_CAPS_KVM, > QEMU_CAPS_SPICE, > - QEMU_CAPS_DEVICE_QXL); > + QEMU_CAPS_DEVICE_QXL, > + QEMU_CAPS_DEVICE_VGA); > DO_TEST("graphics-spice-qxl-vga", > QEMU_CAPS_SPICE, > QEMU_CAPS_DEVICE_QXL); > @@ -1027,21 +1037,24 @@ mymain(void) > QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG, > QEMU_CAPS_PCI_MULTIFUNCTION, QEMU_CAPS_USB_HUB, > QEMU_CAPS_ICH9_USB_EHCI1, QEMU_CAPS_USB_REDIR, > - QEMU_CAPS_CHARDEV_SPICEVMC); > + QEMU_CAPS_CHARDEV_SPICEVMC, QEMU_CAPS_DEVICE_CIRRUS_VGA); > DO_TEST("graphics-spice-agent-file-xfer", > QEMU_CAPS_SPICE, > QEMU_CAPS_DEVICE_QXL, > QEMU_CAPS_SPICE_FILE_XFER_DISABLE); > DO_TEST("graphics-spice-socket", > QEMU_CAPS_SPICE, > - QEMU_CAPS_SPICE_UNIX); > + QEMU_CAPS_SPICE_UNIX, > + QEMU_CAPS_DEVICE_CIRRUS_VGA); > DO_TEST("graphics-spice-auto-socket", > QEMU_CAPS_SPICE, > - QEMU_CAPS_SPICE_UNIX); > + QEMU_CAPS_SPICE_UNIX, > + QEMU_CAPS_DEVICE_CIRRUS_VGA); > driver.config->spiceAutoUnixSocket = true; > DO_TEST("graphics-spice-auto-socket-cfg", > QEMU_CAPS_SPICE, > - QEMU_CAPS_SPICE_UNIX); > + QEMU_CAPS_SPICE_UNIX, > + QEMU_CAPS_DEVICE_CIRRUS_VGA); > driver.config->spiceAutoUnixSocket = false; > > DO_TEST("input-usbmouse", NONE); > @@ -1184,10 +1197,12 @@ mymain(void) > QEMU_CAPS_VIRTIO_S390, QEMU_CAPS_SCLP_S390); > DO_TEST("channel-spicevmc", > QEMU_CAPS_NODEFCONFIG, > - QEMU_CAPS_SPICE, QEMU_CAPS_CHARDEV_SPICEVMC); > + QEMU_CAPS_SPICE, QEMU_CAPS_CHARDEV_SPICEVMC, > + QEMU_CAPS_DEVICE_CIRRUS_VGA); > DO_TEST("channel-spicevmc-old", > QEMU_CAPS_NODEFCONFIG, > - QEMU_CAPS_SPICE, QEMU_CAPS_DEVICE_SPICEVMC); > + QEMU_CAPS_SPICE, QEMU_CAPS_DEVICE_SPICEVMC, > + QEMU_CAPS_DEVICE_CIRRUS_VGA); > DO_TEST("channel-virtio-default", > QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG, > QEMU_CAPS_SPICE, QEMU_CAPS_CHARDEV_SPICEVMC); > @@ -1561,7 +1576,7 @@ mymain(void) > QEMU_CAPS_DEVICE_VIDEO_PRIMARY, > QEMU_CAPS_DEVICE_QXL, > QEMU_CAPS_DEVICE_PCI_BRIDGE); > - DO_TEST("video-vga-nodevice", NONE); > + DO_TEST("video-vga-nodevice", QEMU_CAPS_DEVICE_VGA); > DO_TEST("video-vga-device", QEMU_CAPS_DEVICE_VGA, > QEMU_CAPS_DEVICE_VIDEO_PRIMARY); > DO_TEST("video-vga-device-vgamem", QEMU_CAPS_DEVICE_VGA, > @@ -1652,11 +1667,14 @@ mymain(void) > DO_TEST_PARSE_ERROR("pci-slot-invalid", NONE); > DO_TEST_PARSE_ERROR("pci-function-invalid", NONE); > > - DO_TEST("pci-autoadd-addr", QEMU_CAPS_DEVICE_PCI_BRIDGE); > - DO_TEST("pci-autoadd-idx", QEMU_CAPS_DEVICE_PCI_BRIDGE); > - DO_TEST("pci-autofill-addr", NONE); > + DO_TEST("pci-autoadd-addr", QEMU_CAPS_DEVICE_PCI_BRIDGE, > + QEMU_CAPS_DEVICE_CIRRUS_VGA); > + DO_TEST("pci-autoadd-idx", QEMU_CAPS_DEVICE_PCI_BRIDGE, > + QEMU_CAPS_DEVICE_CIRRUS_VGA); > + DO_TEST("pci-autofill-addr", QEMU_CAPS_DEVICE_CIRRUS_VGA); > DO_TEST("pci-many", > - QEMU_CAPS_DEVICE_PCI_BRIDGE); > + QEMU_CAPS_DEVICE_PCI_BRIDGE, > + QEMU_CAPS_DEVICE_CIRRUS_VGA); > DO_TEST("pci-bridge-many-disks", > QEMU_CAPS_DEVICE_PCI_BRIDGE); > DO_TEST("pcie-root", > @@ -2131,7 +2149,7 @@ mymain(void) > > DO_TEST("name-escape", QEMU_CAPS_NAME_DEBUG_THREADS, > QEMU_CAPS_OBJECT_SECRET, QEMU_CAPS_CHARDEV, QEMU_CAPS_VNC, > - QEMU_CAPS_NAME_GUEST); > + QEMU_CAPS_NAME_GUEST, QEMU_CAPS_DEVICE_CIRRUS_VGA); > DO_TEST("debug-threads", QEMU_CAPS_NAME_DEBUG_THREADS); > > DO_TEST("master-key", QEMU_CAPS_OBJECT_SECRET); > -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list