QXL devices have an associated 'revision' which is raised when new features have been introduced which would break migration to older versions. This commit makes it possible to set this revision as QEMU sometimes support newer QXL revisions than what it defaults to. --- docs/formatdomain.html.in | 5 ++++- docs/schemas/domaincommon.rng | 5 +++++ src/conf/domain_conf.c | 17 +++++++++++++++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 8 ++++++++ .../qemuxml2argv-graphics-spice-compression.args | 3 ++- .../qemuxml2argv-graphics-spice-compression.xml | 4 ++-- .../qemuxml2argv-graphics-spice-qxl-vga.args | 3 ++- .../qemuxml2argv-graphics-spice-qxl-vga.xml | 4 ++-- tests/qemuxml2argvdata/qemuxml2argv-graphics-spice.args | 3 ++- tests/qemuxml2argvdata/qemuxml2argv-graphics-spice.xml | 4 ++-- 11 files changed, 47 insertions(+), 10 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 7ad8aea..4b269c8 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3569,7 +3569,10 @@ qemu-kvm -net nic,model=? /dev/null attribute <code>ram</code> (<span class="since">since 1.0.2</span>) is allowed for "qxl" type only and specifies the size of the primary bar, while <code>vram</code> specifies the - secondary bar size. If "ram" is not supplied a default value is used. + secondary bar size. If "ram" or "vram" are not supplied a default + value is used. The optional attribute <code>revision</code> (<span + class="since">since 1.0.3</span>) specifies the revision of + the QXL device, newer revisions provides more functionality. </dd> <dt><code>model</code></dt> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 049f232..fc78e2d 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2280,6 +2280,11 @@ <ref name="unsignedInt"/> </attribute> </optional> + <optional> + <attribute name="revision"> + <ref name="unsignedInt"/> + </attribute> + </optional> </group> </choice> <optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5782abb..83be711 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7391,6 +7391,7 @@ virDomainVideoDefParseXML(const xmlNodePtr node, char *vram = NULL; char *ram = NULL; char *primary = NULL; + char *revision = NULL; if (VIR_ALLOC(def) < 0) { virReportOOMError(); @@ -7406,6 +7407,7 @@ virDomainVideoDefParseXML(const xmlNodePtr node, ram = virXMLPropString(cur, "ram"); vram = virXMLPropString(cur, "vram"); heads = virXMLPropString(cur, "heads"); + revision = virXMLPropString(cur, "revision"); if ((primary = virXMLPropString(cur, "primary")) != NULL) if (STREQ(primary, "yes")) @@ -7456,6 +7458,19 @@ virDomainVideoDefParseXML(const xmlNodePtr node, def->vram = virDomainVideoDefaultRAM(dom, def->type); } + if (revision) { + if (def->type != VIR_DOMAIN_VIDEO_TYPE_QXL) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("revision attribute only supported for type of qxl")); + goto error; + } + if (virStrToLong_ui(revision, NULL, 10, &def->revision) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("cannot parse video revision '%s'"), revision); + goto error; + } + } + if (heads) { if (virStrToLong_ui(heads, NULL, 10, &def->heads) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -13406,6 +13421,8 @@ virDomainVideoDefFormat(virBufferPtr buf, virBufferAsprintf(buf, " heads='%u'", def->heads); if (def->primary) virBufferAddLit(buf, " primary='yes'"); + if (def->revision) + virBufferAsprintf(buf, " revision='%u'", def->revision); if (def->accel) { virBufferAddLit(buf, ">\n"); virDomainVideoAccelDefFormat(buf, def->accel); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 9a9e0b1..81925b1 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1160,6 +1160,7 @@ struct _virDomainVideoDef { unsigned int ram; /* kibibytes (multiples of 1024) */ unsigned int vram; /* kibibytes (multiples of 1024) */ unsigned int heads; + unsigned int revision; bool primary; virDomainVideoAccelDefPtr accel; virDomainDeviceInfo info; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f6273c1..e45c808 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3598,6 +3598,9 @@ qemuBuildDeviceVideoStr(virDomainVideoDefPtr video, /* QEMU accepts bytes for vram_size. */ virBufferAsprintf(&buf, ",vram_size=%u", video->vram * 1024); + + if (video->revision != 0) + virBufferAsprintf(&buf, ",revision=%u", video->revision); } if (qemuBuildDeviceAddressStr(&buf, &video->info, caps) < 0) @@ -6631,6 +6634,11 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArgFormat(cmd, "%s.vram_size=%u", dev, vram * 1024); } + if (def->videos[0]->revision) { + virCommandAddArg(cmd, "-global"); + virCommandAddArgFormat(cmd, "%s.revision=%u", + dev, def->videos[0]->revision); + } } } diff --git a/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-compression.args b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-compression.args index 59f064b..05f5579 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-compression.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-compression.args @@ -7,5 +7,6 @@ image-compression=auto_glz,jpeg-wan-compression=auto,\ zlib-glz-wan-compression=auto,\ playback-compression=on,streaming-video=filter -vga \ qxl -global qxl.ram_size=67108864 -global qxl.vram_size=18874368 \ --device qxl,id=video1,ram_size=67108864,vram_size=33554432,bus=pci.0,addr=0x4 \ +-global qxl.revision=4 \ +-device qxl,id=video1,ram_size=67108864,vram_size=33554432,revision=4,bus=pci.0,addr=0x4 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-compression.xml b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-compression.xml index a8c4ad8..2dc5776 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-compression.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-compression.xml @@ -31,10 +31,10 @@ <streaming mode='filter'/> </graphics> <video> - <model type='qxl' ram='65536' vram='18432' heads='1'/> + <model type='qxl' ram='65536' vram='18432' heads='1' revision='4'/> </video> <video> - <model type='qxl' ram='65536' vram='32768' heads='1'/> + <model type='qxl' ram='65536' vram='32768' heads='1' revision='4'/> </video> <memballoon model='virtio'/> </devices> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-qxl-vga.args b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-qxl-vga.args index ef499e6..0b08038 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-qxl-vga.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-qxl-vga.args @@ -4,5 +4,6 @@ unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb -hda \ /dev/HostVG/QEMUGuest1 -spice port=5903,tls-port=5904,addr=127.0.0.1,\ x509-dir=/etc/pki/libvirt-spice,tls-channel=main,plaintext-channel=inputs -vga \ qxl -global qxl-vga.ram_size=67108864 -global qxl-vga.vram_size=33554432 \ --device qxl,id=video1,ram_size=67108864,vram_size=67108864,bus=pci.0,addr=0x4 \ +-global qxl-vga.revision=4 \ +-device qxl,id=video1,ram_size=67108864,vram_size=67108864,revision=4,bus=pci.0,addr=0x4 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-qxl-vga.xml b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-qxl-vga.xml index 563d371..3cd0c42 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-qxl-vga.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-qxl-vga.xml @@ -28,10 +28,10 @@ <channel name='inputs' mode='insecure'/> </graphics> <video> - <model type='qxl' ram='65536' vram='32768' heads='1'/> + <model type='qxl' ram='65536' vram='32768' heads='1' revision='4'/> </video> <video> - <model type='qxl' ram='65536' vram='65536' heads='1'/> + <model type='qxl' ram='65536' vram='65536' heads='1' revision='4'/> </video> <memballoon model='virtio'/> </devices> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice.args b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice.args index d7cfae0..082eaf7 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice.args @@ -6,5 +6,6 @@ x509-dir=/etc/pki/libvirt-spice,tls-channel=default,tls-channel=main,plaintext-c image-compression=auto_glz,jpeg-wan-compression=auto,zlib-glz-wan-compression=auto,\ playback-compression=on,streaming-video=filter,disable-copy-paste -vga \ qxl -global qxl.ram_size=67108864 -global qxl.vram_size=18874368 \ --device qxl,id=video1,ram_size=67108864,vram_size=33554432,bus=pci.0,addr=0x4 \ +-global qxl.revision=4 \ +-device qxl,id=video1,ram_size=67108864,vram_size=33554432,revision=4,bus=pci.0,addr=0x4 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice.xml b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice.xml index 9a36660..e99dbc8 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice.xml @@ -34,10 +34,10 @@ <clipboard copypaste='no'/> </graphics> <video> - <model type='qxl' ram='65536' vram='18432' heads='1'/> + <model type='qxl' ram='65536' vram='18432' heads='1' revision='4'/> </video> <video> - <model type='qxl' ram='65536' vram='32768' heads='1'/> + <model type='qxl' ram='65536' vram='32768' heads='1' revision='4'/> </video> <memballoon model='virtio'/> </devices> -- 1.8.1 -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On 02/04/2013 04:16 PM, Christophe Fergeau wrote: > QXL devices have an associated 'revision' which is raised when > new features have been introduced which would break migration > to older versions. This commit makes it possible to set this > revision as QEMU sometimes support newer QXL revisions than what > it defaults to. This could help a lot, but few questions, if I may: - This only helps when the revision is specified, otherwise not, right?

- Isn't there a way how to get the current supported (running) revision of QEMU's qxl video? If we don't know the current (and supported) revision, we are exposing a parameter we know completely nothing about and thus can only try to start qemu with it and wait if it initializes (I know that's how we do it with devices, but there's no other option). Transferring current revision during migration (even when unspecified) would help determining such errors before starting QEMU on destination. There's not much info about the revisions if you don't look in QEMU sources, so I'm not sure what our possibilities are. It's a good thing that the 'revision' parameter is supported since the same commit as qxl driver, though.

Few pointers inline... > --- > docs/formatdomain.html.in | 5 ++++- > docs/schemas/domaincommon.rng | 5 +++++ > src/conf/domain_conf.c | 17 > +++++++++++++++++ > src/conf/domain_conf.h | 1 + > src/qemu/qemu_command.c | 8 > ++++++++ > .../qemuxml2argv-graphics-spice-compression.args | 3 ++- > .../qemuxml2argv-graphics-spice-compression.xml | 4 ++-- > .../qemuxml2argv-graphics-spice-qxl-vga.args | 3 ++- > .../qemuxml2argv-graphics-spice-qxl-vga.xml | 4 ++-- > tests/qemuxml2argvdata/qemuxml2argv-graphics-spice.args | 3 ++- > tests/qemuxml2argvdata/qemuxml2argv-graphics-spice.xml | 4 ++-- > 11 files changed, 47 insertions(+), 10 deletions(-) > > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index 7ad8aea..4b269c8 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in > @@ -3569,7 +3569,10 @@ qemu-kvm -net nic,model=? /dev/null > attribute <code>ram</code> (<span class="since">since > 1.0.2</span>) is allowed for "qxl" type only and specifies > the size of the primary bar, while <code>vram</code> > specifies the > - secondary bar size. If "ram" is not supplied a default > value is used. > + secondary bar size. If "ram" or "vram" are not supplied a > default Good fix, but it should in a be separate patch. > + value is used. The optional attribute > <code>revision</code> (<span > + class="since">since 1.0.3</span>) specifies the revision > of > + the QXL device, newer revisions provides more > functionality. s/provides/provide/ if I'm not mistaken > </dd> > > <dt><code>model</code></dt> > diff --git a/docs/schemas/domaincommon.rng > b/docs/schemas/domaincommon.rng > index 049f232..fc78e2d 100644 > --- a/docs/schemas/domaincommon.rng > +++ b/docs/schemas/domaincommon.rng > @@ -2280,6 +2280,11 @@ > <ref name="unsignedInt"/> > </attribute> > </optional> > + <optional> > + <attribute name="revision"> > + <ref name="unsignedInt"/> This should be > 0 according to my experiments with qemu, but I believe we don't deal with such nuances in the RNG scheme. > + </attribute> > + </optional> > </group> > </choice> > <optional> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 5782abb..83be711 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -7391,6 +7391,7 @@ virDomainVideoDefParseXML(const xmlNodePtr > node, > char *vram = NULL; > char *ram = NULL; > char *primary = NULL; > + char *revision = NULL; > > if (VIR_ALLOC(def) < 0) { > virReportOOMError(); > @@ -7406,6 +7407,7 @@ virDomainVideoDefParseXML(const xmlNodePtr > node, > ram = virXMLPropString(cur, "ram"); > vram = virXMLPropString(cur, "vram"); > heads = virXMLPropString(cur, "heads"); > + revision = virXMLPropString(cur, "revision"); You're leaking the revision string in here. > > if ((primary = virXMLPropString(cur, "primary")) > != NULL) > if (STREQ(primary, "yes")) > @@ -7456,6 +7458,19 @@ virDomainVideoDefParseXML(const xmlNodePtr > node, > def->vram = virDomainVideoDefaultRAM(dom, def->type); > } > > + if (revision) { > + if (def->type != VIR_DOMAIN_VIDEO_TYPE_QXL) { > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("revision attribute only supported > for type of qxl")); > + goto error; > + } > + if (virStrToLong_ui(revision, NULL, 10, &def->revision) < > 0) { > + virReportError(VIR_ERR_XML_ERROR, > + _("cannot parse video revision '%s'"), > revision); > + goto error; > + } > + } > + > if (heads) { > if (virStrToLong_ui(heads, NULL, 10, &def->heads) < 0) { > virReportError(VIR_ERR_INTERNAL_ERROR, > @@ -13406,6 +13421,8 @@ virDomainVideoDefFormat(virBufferPtr buf, > virBufferAsprintf(buf, " heads='%u'", def->heads); > if (def->primary) > virBufferAddLit(buf, " primary='yes'"); > + if (def->revision) > + virBufferAsprintf(buf, " revision='%u'", def->revision); > if (def->accel) { > virBufferAddLit(buf, ">\n"); > virDomainVideoAccelDefFormat(buf, def->accel); > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h > index 9a9e0b1..81925b1 100644 > --- a/src/conf/domain_conf.h > +++ b/src/conf/domain_conf.h > @@ -1160,6 +1160,7 @@ struct _virDomainVideoDef { > unsigned int ram; /* kibibytes (multiples of 1024) */ > unsigned int vram; /* kibibytes (multiples of 1024) */ > unsigned int heads; > + unsigned int revision; > bool primary; > virDomainVideoAccelDefPtr accel; > virDomainDeviceInfo info; > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index f6273c1..e45c808 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -3598,6 +3598,9 @@ qemuBuildDeviceVideoStr(virDomainVideoDefPtr > video, > > /* QEMU accepts bytes for vram_size. */ > virBufferAsprintf(&buf, ",vram_size=%u", video->vram * > 1024); > + > + if (video->revision != 0) you can drop the '!= 0' in here, but that's unimportant. > + virBufferAsprintf(&buf, ",revision=%u", > video->revision); > } > > if (qemuBuildDeviceAddressStr(&buf, &video->info, caps) < 0) > @@ -6631,6 +6634,11 @@ qemuBuildCommandLine(virConnectPtr conn, > virCommandAddArgFormat(cmd, > "%s.vram_size=%u", > dev, vram * 1024); > } > + if (def->videos[0]->revision) { > + virCommandAddArg(cmd, "-global"); > + virCommandAddArgFormat(cmd, > "%s.revision=%u", > + dev, > def->videos[0]->revision); > + } > } > } > [...] I haven't checked the tests, but they don't fail, so that's good :) Martin -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On 02/04/2013 04:16 PM, Christophe Fergeau wrote: > --- > docs/formatdomain.html.in | 5 ++++- > docs/schemas/domaincommon.rng | 5 +++++ > src/conf/domain_conf.c | 17 +++++++++++++++++ > src/conf/domain_conf.h | 1 + > src/qemu/qemu_command.c | 8 ++++++++ > .../qemuxml2argv-graphics-spice-compression.args | 3 ++- > .../qemuxml2argv-graphics-spice-compression.xml | 4 ++-- > .../qemuxml2argv-graphics-spice-qxl-vga.args | 3 ++- > .../qemuxml2argv-graphics-spice-qxl-vga.xml | 4 ++-- > tests/qemuxml2argvdata/qemuxml2argv-graphics-spice.args | 3 ++- > tests/qemuxml2argvdata/qemuxml2argv-graphics-spice.xml | 4 ++-- > 11 files changed, 47 insertions(+), 10 deletions(-) > > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index 7ad8aea..4b269c8 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in > @@ -3569,7 +3569,10 @@ qemu-kvm -net nic,model=? /dev/null > attribute <code>ram</code> (<span class="since">since > 1.0.2</span>) is allowed for "qxl" type only and specifies > the size of the primary bar, while <code>vram</code> specifies the > - secondary bar size. If "ram" is not supplied a default value is used. > + secondary bar size. If "ram" or "vram" are not supplied a default Good fix, but it should in a be separate patch.

> + value is used. The optional attribute <code>revision</code> (<span > + class="since">since 1.0.3</span>) specifies the revision of > + the QXL device, newer revisions provides more functionality. s/provides/provide/ if I'm not mistaken

> </dd> > > <dt><code>model</code></dt> > diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng > index 049f232..fc78e2d 100644 > --- a/docs/schemas/domaincommon.rng > +++ b/docs/schemas/domaincommon.rng > @@ -2280,6 +2280,11 @@ > <ref name="unsignedInt"/> > </attribute> > </optional> > + <optional> > + <attribute name="revision"> > + <ref name="unsignedInt"/> This should be > 0 according to my experiments with qemu, but I believe we don't deal with such nuances in the RNG scheme.

> + </attribute> > + </optional> > </group> > </choice> > <optional> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 5782abb..83be711 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -7391,6 +7391,7 @@ virDomainVideoDefParseXML(const xmlNodePtr node, > char *vram = NULL; > char *ram = NULL; > char *primary = NULL; > + char *revision = NULL; > > if (VIR_ALLOC(def) < 0) { > virReportOOMError(); > @@ -7406,6 +7407,7 @@ virDomainVideoDefParseXML(const xmlNodePtr node, > ram = virXMLPropString(cur, "ram"); > vram = virXMLPropString(cur, "vram"); > heads = virXMLPropString(cur, "heads"); > + revision = virXMLPropString(cur, "revision"); You're leaking the revision string in here.

> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h > index 9a9e0b1..81925b1 100644 > --- a/src/conf/domain_conf.h > +++ b/src/conf/domain_conf.h > @@ -1160,6 +1160,7 @@ struct _virDomainVideoDef { > unsigned int ram; /* kibibytes (multiples of 1024) */ > unsigned int vram; /* kibibytes (multiples of 1024) */ > unsigned int heads; > + unsigned int revision; > bool primary; > virDomainVideoAccelDefPtr accel; > virDomainDeviceInfo info; > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index f6273c1..e45c808 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -3598,6 +3598,9 @@ qemuBuildDeviceVideoStr(virDomainVideoDefPtr video, > > /* QEMU accepts bytes for vram_size. */ > virBufferAsprintf(&buf, ",vram_size=%u", video->vram * 1024); > + > + if (video->revision != 0) you can drop the '!= 0' in here, but that's unimportant.