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@xxxxxxxxxx>
> > ---
> > 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?
> > 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.
> 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()
> 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.
> 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@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list