Re: [libvirt] [PATCH v2 5/5] RFC qemu: add spice opengl support

On 25.11.2015 09:42, Marc-André Lureau wrote: > Add Spice graphics gl attribute. qemu 2.6 should have -spice gl=on > argument to enable opengl rendering context. This is necessary > to actually enable virgl rendering. > > Add a qemuxml2argv test for virtio-gpu + spice with virgl. > > Signed-off-by: Marc-André Lureau <marcandre.lureau(a)gmail.com&gt; > --- > docs/formatdomain.html.in | 6 ++++ > docs/schemas/domaincommon.rng | 5 +++ > src/conf/domain_conf.c | 18 +++++++++++ > src/conf/domain_conf.h | 1 + > src/qemu/qemu_capabilities.c | 2 ++ > src/qemu/qemu_capabilities.h | 1 + > src/qemu/qemu_command.c | 11 +++++++ > tests/qemucapabilitiesdata/caps_2.5.0-1.caps | 1 + > tests/qemucapabilitiesdata/caps_2.5.0-1.replies | 4 +++ > .../qemuxml2argv-video-virtio-gpu-spice-gl.args | 23 ++++++++++++++ > .../qemuxml2argv-video-virtio-gpu-spice-gl.xml | 36 ++++++++++++++++++++++ > tests/qemuxml2argvtest.c | 6 ++++ > tests/qemuxml2xmltest.c | 1 + > 13 files changed, 115 insertions(+) > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-spice-gl.args > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-spice-gl.xml > The idea looks good. ACK to it. However, some changes in the code are required IMO.

> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index 23d8ac9..d39f7d0 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in > @@ -4979,6 +4979,12 @@ qemu-kvm -net nic,model=? /dev/null > 0.8.8</span>); and <code>usbredir</code> > (<span class="since">since 0.9.12</span>). > </p> > + <p> > + Spice may provide accelerated server-side rendering with > + OpenGL. You can enable or disable OpenGL support explicitly with > + the <code>gl</code> attribute. > + (<span class="since">since FIXME</span>). > + </p> > <pre> > &lt;graphics type='spice' port='-1' tlsPort='-1' autoport='yes'&gt; > &lt;channel name='main' mode='secure'/&gt; > diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng > index 228f062..8f4d2ac 100644 > --- a/docs/schemas/domaincommon.rng > +++ b/docs/schemas/domaincommon.rng > @@ -2711,6 +2711,11 @@ > </choice> > </attribute> > </optional> > + <optional> > + <attribute name="gl"> > + <ref name="virYesNo"/> > + </attribute> > + </optional> > <interleave> > <ref name="listenElements"/> > <zeroOrMore> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index e1692ef..d738e71 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -10918,6 +10918,7 @@ virDomainGraphicsDefParseXML(xmlNodePtr node, > char *port = virXMLPropString(node, "port"); > char *tlsPort; > char *autoport; > + char *gl; > char *defaultMode; > int defaultModeVal; > > @@ -10952,6 +10953,19 @@ virDomainGraphicsDefParseXML(xmlNodePtr node, > VIR_FREE(autoport); > } > > + if ((gl = virXMLPropString(node, "gl")) != NULL) { > + int glVal; > + > + if ((glVal = virTristateBoolTypeFromString(gl)) <= 0) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("unknown gl value '%s'"), gl); > + goto error; @gl is leaked here.

> + } > + > + def->data.spice.gl = glVal; > + VIR_FREE(gl); > + } > + > def->data.spice.defaultMode = VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_ANY; > > if ((defaultMode = virXMLPropString(node, "defaultMode")) != NULL) { > @@ -21201,6 +21215,10 @@ virDomainGraphicsDefFormat(virBufferPtr buf, > break; > > case VIR_DOMAIN_GRAPHICS_TYPE_SPICE: > + if (def->data.spice.gl) > + virBufferAsprintf(buf, " gl='%s'", > + virTristateBoolTypeToString(def->data.spice.gl)); > + > if (def->data.spice.port) > virBufferAsprintf(buf, " port='%d'", > def->data.spice.port); > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h > index a47d8ee..ccd9376 100644 > --- a/src/conf/domain_conf.h > +++ b/src/conf/domain_conf.h > @@ -1579,6 +1579,7 @@ struct _virDomainGraphicsDef { > int streaming; > int copypaste; /* enum virTristateBool */ > int filetransfer; /* enum virTristateBool */ > + int gl; /* enum virTristateBool */ > } spice; > } data; > /* nListens, listens, and *port are only useful if type is vnc, > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c > index 4d76c40..64804c8 100644 > --- a/src/qemu/qemu_capabilities.c > +++ b/src/qemu/qemu_capabilities.c > @@ -303,6 +303,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, > "incoming-defer", /* 200 */ > "virtio-gpu", > "virtio-gpu.virgl", > + "spice-gl", > ); > > > @@ -2587,6 +2588,7 @@ static struct virQEMUCapsCommandLineProps virQEMUCapsCommandLine[] = { > { "drive", "throttling.bps-total-max", QEMU_CAPS_DRIVE_IOTUNE_MAX}, > { "machine", "aes-key-wrap", QEMU_CAPS_AES_KEY_WRAP }, > { "machine", "dea-key-wrap", QEMU_CAPS_DEA_KEY_WRAP }, > + { "spice", "gl", QEMU_CAPS_SPICE_GL }, > }; > > static int > diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h > index ab711ff..ecd61d2 100644 > --- a/src/qemu/qemu_capabilities.h > +++ b/src/qemu/qemu_capabilities.h > @@ -329,6 +329,7 @@ typedef enum { > QEMU_CAPS_INCOMING_DEFER, /* -incoming defer and migrate_incoming */ > QEMU_CAPS_DEVICE_VIRTIO_GPU, /* -device virtio-gpu-* & virtio-vga */ > QEMU_CAPS_DEVICE_VIRTIO_GPU_VIRGL, /* -device virtio-gpu-*.virgl */ > + QEMU_CAPS_SPICE_GL, /* -spice gl */ > > QEMU_CAPS_LAST /* this must always be the last item */ > } virQEMUCapsFlags; > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index 2808960..5f6a8fe 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -8447,6 +8447,17 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, > } > } > > + if (graphics->data.spice.gl) { > + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPICE_GL)) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("This QEMU doesn't support spice OpenGL")); > + goto error; > + } else { > + virBufferAsprintf(&opt, ",gl=%s", > + virTristateSwitchTypeToString(graphics->data.spice.gl)); > + } 'else' feels kind of redundant but I don't hesitate to leave it there.

> + } > + > if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEAMLESS_MIGRATION)) { > /* If qemu supports seamless migration turn it > * unconditionally on. If migration destination > diff --git a/tests/qemucapabilitiesdata/caps_2.5.0-1.caps b/tests/qemucapabilitiesdata/caps_2.5.0-1.caps > index 975ed0c..1d6b5aa 100644 > --- a/tests/qemucapabilitiesdata/caps_2.5.0-1.caps > +++ b/tests/qemucapabilitiesdata/caps_2.5.0-1.caps > @@ -164,4 +164,5 @@ > <flag name='incoming-defer'/> > <flag name='virtio-gpu'/> > <flag name='virtio-gpu.virgl'/> > + <flag name='spice-gl'/> > </qemuCaps> > diff --git a/tests/qemucapabilitiesdata/caps_2.5.0-1.replies b/tests/qemucapabilitiesdata/caps_2.5.0-1.replies > index d90a74b..5452c0f 100644 > --- a/tests/qemucapabilitiesdata/caps_2.5.0-1.replies > +++ b/tests/qemucapabilitiesdata/caps_2.5.0-1.replies > @@ -3120,6 +3120,10 @@ > { > "parameters": [ > { > + "name": "gl", > + "type": "boolean" > + }, > + { > "name": "seamless-migration", > "type": "boolean" > }, > diff --git a/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-spice-gl.args b/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-spice-gl.args > new file mode 100644 > index 0000000..9751ee3 > --- /dev/null > +++ b/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-spice-gl.args > @@ -0,0 +1,23 @@ > +LC_ALL=C \ > +PATH=/bin \ > +HOME=/home/test \ > +USER=test \ > +LOGNAME=test \ > +QEMU_AUDIO_DRV=spice \ > +/usr/bin/qemu \ > +-name QEMUGuest1 \ > +-S \ > +-M pc \ > +-m 1024 \ > +-smp 1 \ > +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ > +-nodefaults \ > +-monitor unix:/tmp/test-monitor,server,nowait \ > +-no-acpi \ > +-boot c \ > +-usb \ > +-drive file=/var/lib/libvirt/images/QEMUGuest1,if=none,id=drive-ide0-0-0,format=qcow2,cache=none \ Long line.

> +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ > +-spice port=0,gl=on \ > +-device virtio-vga,id=video0,virgl=on,bus=pci.0,addr=0x2 \ > +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 > diff --git a/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-spice-gl.xml b/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-spice-gl.xml > new file mode 100644 > index 0000000..07afad2 > --- /dev/null > +++ b/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-spice-gl.xml > @@ -0,0 +1,36 @@ > +<domain type='qemu'> > + <name>QEMUGuest1</name> > + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> > + <memory unit='KiB'>1048576</memory> > + <currentMemory unit='KiB'>1048576</currentMemory> > + <vcpu placement='static'>1</vcpu> > + <os> > + <type arch='i686' machine='pc'>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</emulator> > + <disk type='file' device='disk'> > + <driver name='qemu' type='qcow2' cache='none'/> > + <source file='/var/lib/libvirt/images/QEMUGuest1'/> > + <target dev='hda' bus='ide'/> > + <address type='drive' controller='0' bus='0' target='0' unit='0'/> > + </disk> > + <controller type='ide' index='0'/> > + <controller type='usb' index='0'/> > + <controller type='pci' index='0' model='pci-root'/> > + <input type='mouse' bus='ps2'/> > + <input type='keyboard' bus='ps2'/> > + <graphics type='spice' gl='yes' autoport='no'/> > + <video> > + <model type='virtio' heads='1'> > + <acceleration accel3d='yes'/> > + </model> > + </video> > + <memballoon model='virtio'/> > + </devices> > +</domain> > diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c > index 4baadb7..060c646 100644 > --- a/tests/qemuxml2argvtest.c > +++ b/tests/qemuxml2argvtest.c > @@ -1755,6 +1755,12 @@ mymain(void) > QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_VIRTIO_GPU, > QEMU_CAPS_DEVICE_VIRTIO_GPU_VIRGL, > QEMU_CAPS_DEVICE_VIDEO_PRIMARY); > + DO_TEST("video-virtio-gpu-spice-gl", QEMU_CAPS_DEVICE, > + QEMU_CAPS_DEVICE_VIRTIO_GPU, > + QEMU_CAPS_DEVICE_VIRTIO_GPU_VIRGL, > + QEMU_CAPS_SPICE, > + QEMU_CAPS_SPICE_GL, > + QEMU_CAPS_DEVICE_VIDEO_PRIMARY); Move this few lines up.

> > qemuTestDriverFree(&driver); > > diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c > index a4fefef..23cc549 100644 > --- a/tests/qemuxml2xmltest.c > +++ b/tests/qemuxml2xmltest.c > @@ -628,6 +628,7 @@ mymain(void) > > DO_TEST("video-virtio-gpu-device"); > DO_TEST("video-virtio-gpu-virgl"); > + DO_TEST("video-virtio-gpu-spice-gl"); > > qemuTestDriverFree(&driver); > > Michal