On 10/10/2016 05:44 AM, Pavel Hrdina wrote:
On Sat, Oct 08, 2016 at 09:58:15AM -0400, John Ferlan wrote:
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@redhat.com> --- 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...
The changes to qemuxml2argvtest.c can be moved to separate patch, the only relationship is that the movement of validation code depends on the changes to qemuxml2argvtest.c because this patch adds the validation for secondary video devices too.
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...
That exact error message is replaced by different error message:
"this QEMU does not support '%s' video device"
Currently only QXL can be used as secondary video device so if QXL device is not supported by QEMU it also implicates that only one video card is supported.
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?
Where did you get the assumption from?
Probably written before I found the gem in domain_conf about primary being [0] and I never got back to the line in my followup pass. I was "attempting to" match the removed code with the new code. I guess I didn't find this as clean as it appears to be. In hindsight it seems to be a shortcut. Probably helps to have the history of when the PRIMARY capability was added...
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.
Yes that's true.
And even though we have other checks to ensure QXL is the only one used for non primary, we are adding checks VGA, CIRRUS_VGA, and VMWARE_SVGA for every non primary until we find QXL....
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.
This is also true.
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.
This is already done by qemuDomainDefValidateVideo().
qemuProcessStartValidateXML()->virDomainDefValidate() ->qemuDomainDefValidate()->qemuDomainDefValidateVideo()
I think I was going for - the primary can/should make all those checks and the secondary really only needs to make the QXL check. Again there's this whole PRIMARY cap which plays a role and it really isn't just code motion here.
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
Check the code at src/conf/domain_conf.c:17317
... if (VIR_INSERT_ELEMENT_INPLACE(def->videos, ...
This ensures that there is only one primary video device and that the primary video device is always first.
Right - exactly what I found (eventually). I think it's confusing to see "videos[0]" used in some cases and video->primary used in other cases until you find that INPLACE line and realize it's putting it @ 0. John
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.
At least I tried to hide this change in this patch. I'll create a separate patch for this.
Pavel
+ + 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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list