[libvirt] [PATCH] qemu: Support ram bar size for qxl devices
Adds a qxl-ram attribute globaly to the video.model element, that changes the resulting qemu command line only if video.type == "qxl". That attribute gets a default value of 64*1024 only if model.type is "qxl". In effect not changing any xml or argv for non qxl devices. For qxl devices a new property is set: -global qxl-vga.ram_size=<ram>*1024 or -global qxl.ram_size=<ram>*1024 For the main and secondary qxl devices respectively. The default for the qxl ram bar is the same as the default for the qxl vram bar, 64*1024. --- I've added a qxl-ram attribute. There is no precedent for adding am attribute prefixed like this, so I'm open for any other suggestion on how to do it. docs/schemas/domaincommon.rng | 9 +++- src/conf/domain_conf.c | 19 ++++++- src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 58 ++++++++++++++++++---- .../qemuxml2argv-graphics-spice-compression.args | 2 +- .../qemuxml2argv-graphics-spice-compression.xml | 4 +- .../qemuxml2argv-graphics-spice-qxl-vga.args | 2 +- .../qemuxml2argv-graphics-spice-qxl-vga.xml | 4 +- .../qemuxml2argv-graphics-spice.args | 2 +- .../qemuxml2argv-graphics-spice.xml | 4 +- .../qemuxml2argv-video-device-pciaddr-default.args | 6 +-- 11 files changed, 86 insertions(+), 25 deletions(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 67ae864..50fc834 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2251,7 +2251,9 @@ </define> <!-- A video adapter description, allowing configuration of device - model, number of virtual heads, and video ram size + model, number of virtual heads, and video ram size. + The qxl-ram property is used for qxl types only to specify the + primary bar size, letting vram specify the secondary bar size. --> <define name="video"> <element name="video"> @@ -2268,6 +2270,11 @@ </choice> </attribute> <optional> + <attribute name="qxl-ram"> + <ref name="unsignedInt"/> + </attribute> + </optional> + <optional> <attribute name="vram"> <ref name="unsignedInt"/> </attribute> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 13c14e9..a16f1b5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7391,6 +7391,7 @@ virDomainVideoDefParseXML(const xmlNodePtr node, char *type = NULL; char *heads = NULL; char *vram = NULL; + char *qxl_ram = NULL; char *primary = NULL; if (VIR_ALLOC(def) < 0) { @@ -7401,9 +7402,10 @@ virDomainVideoDefParseXML(const xmlNodePtr node, cur = node->children; while (cur != NULL) { if (cur->type == XML_ELEMENT_NODE) { - if (!type && !vram && !heads && + if (!type && !vram && !qxl_ram && !heads && xmlStrEqual(cur->name, BAD_CAST "model")) { type = virXMLPropString(cur, "type"); + qxl_ram = virXMLPropString(cur, "qxl-ram"); vram = virXMLPropString(cur, "vram"); heads = virXMLPropString(cur, "heads"); @@ -7431,6 +7433,18 @@ virDomainVideoDefParseXML(const xmlNodePtr node, } } + if (qxl_ram) { + if (virStrToLong_ui(qxl_ram, NULL, 10, &def->qxl_ram) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse video qxl-ram '%s'"), qxl_ram); + goto error; + } + } else { + if (def->type == VIR_DOMAIN_VIDEO_TYPE_QXL) { + def->qxl_ram = virDomainVideoDefaultRAM(dom, def->type); + } + } + if (vram) { if (virStrToLong_ui(vram, NULL, 10, &def->vram) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -7455,6 +7469,7 @@ virDomainVideoDefParseXML(const xmlNodePtr node, goto error; VIR_FREE(type); + VIR_FREE(qxl_ram); VIR_FREE(vram); VIR_FREE(heads); @@ -13383,6 +13398,8 @@ virDomainVideoDefFormat(virBufferPtr buf, virBufferAddLit(buf, " <video>\n"); virBufferAsprintf(buf, " <model type='%s'", model); + if (def->qxl_ram && def->type == VIR_DOMAIN_VIDEO_TYPE_QXL) + virBufferAsprintf(buf, " qxl-ram='%u'", def->qxl_ram); if (def->vram) virBufferAsprintf(buf, " vram='%u'", def->vram); if (def->heads) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ce36003..9333d26 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1157,6 +1157,7 @@ struct _virDomainVideoAccelDef { struct _virDomainVideoDef { int type; + unsigned int qxl_ram; unsigned int vram; unsigned int heads; bool primary; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 981c692..05f656a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3563,6 +3563,15 @@ qemuBuildDeviceVideoStr(virDomainVideoDefPtr video, UINT_MAX / 1024); goto error; } + if (video->qxl_ram > (UINT_MAX / 1024)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("value for 'qxl-ram' must be less than '%u'"), + UINT_MAX / 1024); + goto error; + } + + /* QEMU accepts bytes for ram_size. */ + virBufferAsprintf(&buf, ",ram_size=%u", video->qxl_ram * 1024); /* QEMU accepts bytes for vram_size. */ virBufferAsprintf(&buf, ",vram_size=%u", video->vram * 1024); @@ -6569,23 +6578,48 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArgList(cmd, "-vga", vgastr, NULL); if (def->videos[0]->type == VIR_DOMAIN_VIDEO_TYPE_QXL) { - if (def->videos[0]->vram && + if ((def->videos[0]->vram || def->videos[0]->qxl_ram) && qemuCapsGet(caps, QEMU_CAPS_DEVICE)) { - if (def->videos[0]->vram > (UINT_MAX / 1024)) { + int qxl_ram = def->videos[0]->qxl_ram; + int vram = def->videos[0]->vram; + if (vram > (UINT_MAX / 1024)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("value for 'vram' must be less than '%u'"), + _("value for 'vram' must be less than '%u'"), + UINT_MAX / 1024); + goto error; + } + if (qxl_ram > (UINT_MAX / 1024)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("value for 'qxl-ram' must be less than '%u'"), UINT_MAX / 1024); goto error; } - virCommandAddArg(cmd, "-global"); - - if (qemuCapsGet(caps, QEMU_CAPS_DEVICE_QXL_VGA)) - virCommandAddArgFormat(cmd, "qxl-vga.vram_size=%u", - def->videos[0]->vram * 1024); - else - virCommandAddArgFormat(cmd, "qxl.vram_size=%u", - def->videos[0]->vram * 1024); + if (qemuCapsGet(caps, QEMU_CAPS_DEVICE_QXL_VGA)) { + if (qxl_ram) { + virCommandAddArg(cmd, "-global"); + virCommandAddArgFormat(cmd, + "qxl-vga.ram_size=%u", + qxl_ram * 1024); + } + if (vram) { + virCommandAddArg(cmd, "-global"); + virCommandAddArgFormat(cmd, + "qxl-vga.vram_size=%u", + vram * 1024); + } + } else { + if (qxl_ram) { + virCommandAddArg(cmd, "-global"); + virCommandAddArgFormat(cmd, "qxl.ram_size=%u", + qxl_ram * 1024); + } + if (vram) { + virCommandAddArg(cmd, "-global"); + virCommandAddArgFormat(cmd, "qxl.vram_size=%u", + vram * 1024); + } + } } } } @@ -9247,6 +9281,8 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, else vid->type = video; vid->vram = virDomainVideoDefaultRAM(def, vid->type); + vid->qxl_ram = vid->type == VIR_DOMAIN_VIDEO_TYPE_QXL ? + virDomainVideoDefaultRAM(def, vid->type) : 0; vid->heads = 1; if (VIR_REALLOC_N(def->videos, def->nvideos+1) < 0) { diff --git a/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-compression.args b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-compression.args index 5c5912b..2f537df 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-compression.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-compression.args @@ -5,5 +5,5 @@ unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb -hda \ x509-dir=/etc/pki/libvirt-spice,\ image-compression=auto_glz,jpeg-wan-compression=auto,zlib-glz-wan-compression=auto,\ playback-compression=on,streaming-video=filter -vga \ -qxl -global qxl.vram_size=18874368 -device qxl,id=video1,vram_size=33554432,bus=pci.0,addr=0x4 \ +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 \ -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 52eb5b9..32e4cc8 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' vram='18432' heads='1'/> + <model type='qxl' qxl-ram='65536' vram='18432' heads='1'/> </video> <video> - <model type='qxl' vram='32768' heads='1'/> + <model type='qxl' qxl-ram='65536' vram='32768' heads='1'/> </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 3954c03..007dd3f 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-qxl-vga.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-qxl-vga.args @@ -3,5 +3,5 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=spice \ 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.vram_size=33554432 -device qxl,id=video1,vram_size=67108864,bus=pci.0,addr=0x4 \ +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 \ -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 49cb8cc..ab40c7b 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' vram='32768' heads='1'/> + <model type='qxl' qxl-ram='65536' vram='32768' heads='1'/> </video> <video> - <model type='qxl' vram='65536' heads='1'/> + <model type='qxl' qxl-ram='65536' vram='65536' heads='1'/> </video> <memballoon model='virtio'/> </devices> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice.args b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice.args index 854e723..030e1f2 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice.args @@ -5,5 +5,5 @@ unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb -hda \ x509-dir=/etc/pki/libvirt-spice,tls-channel=default,tls-channel=main,plaintext-channel=inputs,\ 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.vram_size=18874368 -device qxl,id=video1,vram_size=33554432,bus=pci.0,addr=0x4 \ +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 \ -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 d4939a4..b540915 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' vram='18432' heads='1'/> + <model type='qxl' qxl-ram='65536' vram='18432' heads='1'/> </video> <video> - <model type='qxl' vram='32768' heads='1'/> + <model type='qxl' qxl-ram='65536' vram='32768' heads='1'/> </video> <memballoon model='virtio'/> </devices> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-video-device-pciaddr-default.args b/tests/qemuxml2argvdata/qemuxml2argv-video-device-pciaddr-default.args index 9ce852f..4abd7c2 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-video-device-pciaddr-default.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-video-device-pciaddr-default.args @@ -3,7 +3,7 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ -usb \ -hda /var/lib/libvirt/images/QEMUGuest1 -vnc 127.0.0.1:-5900 \ --device qxl-vga,id=video0,vram_size=67108864,bus=pci.0,addr=0x3 \ --device qxl,id=video1,vram_size=67108864,bus=pci.0,addr=0x4 \ --device qxl,id=video2,vram_size=67108864,bus=pci.0,addr=0x5 \ +-device qxl-vga,id=video0,ram_size=67108864,vram_size=67108864,bus=pci.0,addr=0x3 \ +-device qxl,id=video1,ram_size=67108864,vram_size=67108864,bus=pci.0,addr=0x4 \ +-device qxl,id=video2,ram_size=67108864,vram_size=67108864,bus=pci.0,addr=0x5 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x2 -- 1.8.0.1
On 17.01.2013 20:35, Alon Levy wrote:
Adds a qxl-ram attribute globaly to the video.model element, that changes the resulting qemu command line only if video.type == "qxl".
That attribute gets a default value of 64*1024 only if model.type is "qxl". In effect not changing any xml or argv for non qxl devices.
For qxl devices a new property is set: -global qxl-vga.ram_size=<ram>*1024 or -global qxl.ram_size=<ram>*1024
For the main and secondary qxl devices respectively.
The default for the qxl ram bar is the same as the default for the qxl vram bar, 64*1024. --- I've added a qxl-ram attribute. There is no precedent for adding am attribute prefixed like this, so I'm open for any other suggestion on how to do it.
docs/schemas/domaincommon.rng | 9 +++- src/conf/domain_conf.c | 19 ++++++- src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 58 ++++++++++++++++++---- .../qemuxml2argv-graphics-spice-compression.args | 2 +- .../qemuxml2argv-graphics-spice-compression.xml | 4 +- .../qemuxml2argv-graphics-spice-qxl-vga.args | 2 +- .../qemuxml2argv-graphics-spice-qxl-vga.xml | 4 +- .../qemuxml2argv-graphics-spice.args | 2 +- .../qemuxml2argv-graphics-spice.xml | 4 +- .../qemuxml2argv-video-device-pciaddr-default.args | 6 +-- 11 files changed, 86 insertions(+), 25 deletions(-)
Just one question - what is the difference between ram_size and vram_size? If the former is successor of the latter, I think we should be using the ram_size and drop vram_size and not pollute XML at all. However, if there's any difference we should document that. That is, every XML snippet being introduced must go hand in hand with extending docs/format*.html.in. Michal
On 17.01.2013 20:35, Alon Levy wrote:
Adds a qxl-ram attribute globaly to the video.model element, that changes the resulting qemu command line only if video.type == "qxl".
That attribute gets a default value of 64*1024 only if model.type is "qxl". In effect not changing any xml or argv for non qxl devices.
For qxl devices a new property is set: -global qxl-vga.ram_size=<ram>*1024 or -global qxl.ram_size=<ram>*1024
For the main and secondary qxl devices respectively.
The default for the qxl ram bar is the same as the default for the qxl vram bar, 64*1024. --- I've added a qxl-ram attribute. There is no precedent for adding am attribute prefixed like this, so I'm open for any other suggestion on how to do it.
docs/schemas/domaincommon.rng | 9 +++- src/conf/domain_conf.c | 19 ++++++- src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 58 ++++++++++++++++++---- .../qemuxml2argv-graphics-spice-compression.args | 2 +- .../qemuxml2argv-graphics-spice-compression.xml | 4 +- .../qemuxml2argv-graphics-spice-qxl-vga.args | 2 +- .../qemuxml2argv-graphics-spice-qxl-vga.xml | 4 +- .../qemuxml2argv-graphics-spice.args | 2 +- .../qemuxml2argv-graphics-spice.xml | 4 +- .../qemuxml2argv-video-device-pciaddr-default.args | 6 +-- 11 files changed, 86 insertions(+), 25 deletions(-)
Just one question - what is the difference between ram_size and vram_size? If the former is successor of the latter, I think we should be using the ram_size and drop vram_size and not pollute XML at all. However, if there's any difference we should document that. That is, every XML snippet being introduced must go hand in hand with extending docs/format*.html.in.
I'll add the docs. Just to be clear, ram_size is distinct from vram_size, both exist at the same time, so it is not a successor.
Michal
On 01/17/2013 12:35 PM, Alon Levy wrote:
Adds a qxl-ram attribute globaly to the video.model element, that changes
s/globaly/globally/
the resulting qemu command line only if video.type == "qxl".
That attribute gets a default value of 64*1024 only if model.type is "qxl". In effect not changing any xml or argv for non qxl devices.
For qxl devices a new property is set: -global qxl-vga.ram_size=<ram>*1024 or -global qxl.ram_size=<ram>*1024
This part of the commit message shows the qemu interface; but it would also be handy to show the intended libvirt XML interface. As written, this patch is basically adding qxl_ram into: <video> <model type='qxl' vram='65536' qxl-ram='65536' heads='1'/> </video>
For the main and secondary qxl devices respectively.
The default for the qxl ram bar is the same as the default for the qxl vram bar, 64*1024. --- I've added a qxl-ram attribute.
Ah, this information is what I was looking for, but because it came after ---, it would be missing from git history.
There is no precedent for adding am attribute prefixed like this, so I'm open for any other suggestion on how to do it.
Why prefix it at all? What's wrong with just naming it 'ram', which is visually distinct from 'vram'?
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 67ae864..50fc834 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2251,7 +2251,9 @@ </define> <!-- A video adapter description, allowing configuration of device - model, number of virtual heads, and video ram size + model, number of virtual heads, and video ram size. + The qxl-ram property is used for qxl types only to specify the + primary bar size, letting vram specify the secondary bar size.
Michal already pointed out the missing documentation in docs/formatdomain.html.in. Basically, I would move this comment that you added here out of the RNG and into the public html.in documentation. Also, in RNG, it is possible to encode the limitation of using the new attribute only when tied to type='qxl', as follows (and here, with my preferred naming of 'ram' instead of 'qxl-ram'): <element name="model"> <choice> <attribute name="type"> <choice> <value>vga</value> <value>cirrus</value> <value>vmvga</value> <value>xen</value> <value>vbox</value> </choice> </attribute> <group> <attribute name="type"> <value>qxl</value> </attribute> <optional> <attribute name="ram"> <ref name="unsignedInt"/> </attribute> </optional> </group> </choice> <optional> <attribute name="vram"> <ref name="unsignedInt"/> </attribute> </optional>
+++ b/src/conf/domain_conf.c @@ -7391,6 +7391,7 @@ virDomainVideoDefParseXML(const xmlNodePtr node, char *type = NULL; char *heads = NULL; char *vram = NULL; + char *qxl_ram = NULL;
Again, if we go with naming the new attribute just 'ram', this variable name can be shorter.
@@ -7431,6 +7433,18 @@ virDomainVideoDefParseXML(const xmlNodePtr node, } }
+ if (qxl_ram) { + if (virStrToLong_ui(qxl_ram, NULL, 10, &def->qxl_ram) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse video qxl-ram '%s'"), qxl_ram); + goto error; + } + } else { + if (def->type == VIR_DOMAIN_VIDEO_TYPE_QXL) { + def->qxl_ram = virDomainVideoDefaultRAM(dom, def->type);
Did you mean for it to always default to the domain default, even when vram is present but not the domain default? Or did you want it to fall back to the vram value?
+ } + } + if (vram) {
Since you documented that '[qxl-]ram' defaults to the value of 'vram', I think you need to put the new if block after this existing vram block.
@@ -13383,6 +13398,8 @@ virDomainVideoDefFormat(virBufferPtr buf, virBufferAddLit(buf, " <video>\n"); virBufferAsprintf(buf, " <model type='%s'", model); + if (def->qxl_ram && def->type == VIR_DOMAIN_VIDEO_TYPE_QXL) + virBufferAsprintf(buf, " qxl-ram='%u'", def->qxl_ram);
The && def->type == VIR_DOMAIN_VIDEO_TYPE_QXL is not needed here; def->qxl_ram will be 0 for all other video types, based on the restrictions you had in the parser.
+++ b/src/conf/domain_conf.h @@ -1157,6 +1157,7 @@ struct _virDomainVideoAccelDef {
struct _virDomainVideoDef { int type; + unsigned int qxl_ram; unsigned int vram;
Might be worth adding a comment here on units (that is, both qxl_ram and vram use kb).
+++ b/src/qemu/qemu_command.c @@ -3563,6 +3563,15 @@ qemuBuildDeviceVideoStr(virDomainVideoDefPtr video, UINT_MAX / 1024); goto error; } + if (video->qxl_ram > (UINT_MAX / 1024)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
VIR_ERR_OVERFLOW might be better (but this was copy-and-paste from vram, so the same applies there).
@@ -6569,23 +6578,48 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArgList(cmd, "-vga", vgastr, NULL);
if (def->videos[0]->type == VIR_DOMAIN_VIDEO_TYPE_QXL) { - if (def->videos[0]->vram && + if ((def->videos[0]->vram || def->videos[0]->qxl_ram) && qemuCapsGet(caps, QEMU_CAPS_DEVICE)) { - if (def->videos[0]->vram > (UINT_MAX / 1024)) { + int qxl_ram = def->videos[0]->qxl_ram; + int vram = def->videos[0]->vram; + if (vram > (UINT_MAX / 1024)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
More places where the existing code would be better as VIR_ERR_OVERFLOW.
+ if (qemuCapsGet(caps, QEMU_CAPS_DEVICE_QXL_VGA)) { + if (qxl_ram) { + virCommandAddArg(cmd, "-global"); + virCommandAddArgFormat(cmd, + "qxl-vga.ram_size=%u", + qxl_ram * 1024); + } + if (vram) { + virCommandAddArg(cmd, "-global"); + virCommandAddArgFormat(cmd, + "qxl-vga.vram_size=%u", + vram * 1024); + } + } else { + if (qxl_ram) { + virCommandAddArg(cmd, "-global"); + virCommandAddArgFormat(cmd, "qxl.ram_size=%u", + qxl_ram * 1024); + } + if (vram) { + virCommandAddArg(cmd, "-global"); + virCommandAddArgFormat(cmd, "qxl.vram_size=%u", + vram * 1024); + } + }
This feels long. Why not the shorter: const char *dev = (qemuCapsGet(caps, QEMU_CAPS_DEVICE_QXL_VGA) ? "qxl-vga" : "qxl"); if (qxl_ram) virCommandAddArgFormat(cmd, "-global=%s.ram_size=%u", dev, qxl_ram * 1024); if (vram) virCommandAddArgFormat(cmd, "-global=%s.vram_size=%u", dev, vram * 1024);
+++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-qxl-vga.args @@ -3,5 +3,5 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=spice \ 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.vram_size=33554432 -device qxl,id=video1,vram_size=67108864,bus=pci.0,addr=0x4 \ +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 \
This is a really long line; please insert a \-newline pair to break it and keep the file back under 80 columns. Looking forward to v2. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Hi Eric, I'm having trouble with the RNG approach. I've created a test rng and test xml with xmllint that can create the problem I am seeing, specifically: test.rng: <?xml version="1.0"?> <grammar xmlns="http://relaxng.org/ns/structure/1.0" datatypeLibrary="http://www.w3.org/2001/XMLSchema-datatypes"> <start> <ref name="video"/> </start> <define name="video"> <element name="video"> <optional> <element name="model"> <attribute name="type"> <choice> <value>vga</value> </choice> </attribute> <group> <attribute name="type"> <value>qxl</value> </attribute> <optional> <attribute name="ram"> </attribute> </optional> </group> <optional> <attribute name="vram"> </attribute> </optional> </element> </optional> </element> </define> </grammar> With test.xml being: <video/> $ xmllint --relaxng test.rng test.xml test.rng:9: element element: Relax-NG parser error : Attributes conflicts in group Relax-NG schema test.rng failed to compile <?xml version="1.0"?> <video/> So it seems I cannot have two type instances. I thought maybe you can solve it quicker for me then myself :) ----- Original Message -----
On 01/17/2013 12:35 PM, Alon Levy wrote:
Adds a qxl-ram attribute globaly to the video.model element, that changes
s/globaly/globally/
the resulting qemu command line only if video.type == "qxl".
That attribute gets a default value of 64*1024 only if model.type is "qxl". In effect not changing any xml or argv for non qxl devices.
For qxl devices a new property is set: -global qxl-vga.ram_size=<ram>*1024 or -global qxl.ram_size=<ram>*1024
This part of the commit message shows the qemu interface; but it would also be handy to show the intended libvirt XML interface. As written, this patch is basically adding qxl_ram into:
<video> <model type='qxl' vram='65536' qxl-ram='65536' heads='1'/> </video>
For the main and secondary qxl devices respectively.
The default for the qxl ram bar is the same as the default for the qxl vram bar, 64*1024. --- I've added a qxl-ram attribute.
Ah, this information is what I was looking for, but because it came after ---, it would be missing from git history.
There is no precedent for adding am attribute prefixed like this, so I'm open for any other suggestion on how to do it.
Why prefix it at all? What's wrong with just naming it 'ram', which is visually distinct from 'vram'?
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 67ae864..50fc834 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2251,7 +2251,9 @@ </define> <!-- A video adapter description, allowing configuration of device - model, number of virtual heads, and video ram size + model, number of virtual heads, and video ram size. + The qxl-ram property is used for qxl types only to specify the + primary bar size, letting vram specify the secondary bar size.
Michal already pointed out the missing documentation in docs/formatdomain.html.in. Basically, I would move this comment that you added here out of the RNG and into the public html.in documentation.
Also, in RNG, it is possible to encode the limitation of using the new attribute only when tied to type='qxl', as follows (and here, with my preferred naming of 'ram' instead of 'qxl-ram'):
<element name="model"> <choice> <attribute name="type"> <choice> <value>vga</value> <value>cirrus</value> <value>vmvga</value> <value>xen</value> <value>vbox</value> </choice> </attribute> <group> <attribute name="type"> <value>qxl</value> </attribute> <optional> <attribute name="ram"> <ref name="unsignedInt"/> </attribute> </optional> </group> </choice> <optional> <attribute name="vram"> <ref name="unsignedInt"/> </attribute> </optional>
+++ b/src/conf/domain_conf.c @@ -7391,6 +7391,7 @@ virDomainVideoDefParseXML(const xmlNodePtr node, char *type = NULL; char *heads = NULL; char *vram = NULL; + char *qxl_ram = NULL;
Again, if we go with naming the new attribute just 'ram', this variable name can be shorter.
@@ -7431,6 +7433,18 @@ virDomainVideoDefParseXML(const xmlNodePtr node, } }
+ if (qxl_ram) { + if (virStrToLong_ui(qxl_ram, NULL, 10, &def->qxl_ram) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse video qxl-ram '%s'"), qxl_ram); + goto error; + } + } else { + if (def->type == VIR_DOMAIN_VIDEO_TYPE_QXL) { + def->qxl_ram = virDomainVideoDefaultRAM(dom, def->type);
Did you mean for it to always default to the domain default, even when vram is present but not the domain default? Or did you want it to fall back to the vram value?
+ } + } + if (vram) {
Since you documented that '[qxl-]ram' defaults to the value of 'vram', I think you need to put the new if block after this existing vram block.
@@ -13383,6 +13398,8 @@ virDomainVideoDefFormat(virBufferPtr buf, virBufferAddLit(buf, " <video>\n"); virBufferAsprintf(buf, " <model type='%s'", model); + if (def->qxl_ram && def->type == VIR_DOMAIN_VIDEO_TYPE_QXL) + virBufferAsprintf(buf, " qxl-ram='%u'", def->qxl_ram);
The && def->type == VIR_DOMAIN_VIDEO_TYPE_QXL is not needed here; def->qxl_ram will be 0 for all other video types, based on the restrictions you had in the parser.
+++ b/src/conf/domain_conf.h @@ -1157,6 +1157,7 @@ struct _virDomainVideoAccelDef {
struct _virDomainVideoDef { int type; + unsigned int qxl_ram; unsigned int vram;
Might be worth adding a comment here on units (that is, both qxl_ram and vram use kb).
+++ b/src/qemu/qemu_command.c @@ -3563,6 +3563,15 @@ qemuBuildDeviceVideoStr(virDomainVideoDefPtr video, UINT_MAX / 1024); goto error; } + if (video->qxl_ram > (UINT_MAX / 1024)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
VIR_ERR_OVERFLOW might be better (but this was copy-and-paste from vram, so the same applies there).
@@ -6569,23 +6578,48 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArgList(cmd, "-vga", vgastr, NULL);
if (def->videos[0]->type == VIR_DOMAIN_VIDEO_TYPE_QXL) { - if (def->videos[0]->vram && + if ((def->videos[0]->vram || def->videos[0]->qxl_ram) && qemuCapsGet(caps, QEMU_CAPS_DEVICE)) { - if (def->videos[0]->vram > (UINT_MAX / 1024)) { + int qxl_ram = def->videos[0]->qxl_ram; + int vram = def->videos[0]->vram; + if (vram > (UINT_MAX / 1024)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
More places where the existing code would be better as VIR_ERR_OVERFLOW.
+ if (qemuCapsGet(caps, QEMU_CAPS_DEVICE_QXL_VGA)) { + if (qxl_ram) { + virCommandAddArg(cmd, "-global"); + virCommandAddArgFormat(cmd, + "qxl-vga.ram_size=%u", + qxl_ram * 1024); + } + if (vram) { + virCommandAddArg(cmd, "-global"); + virCommandAddArgFormat(cmd, + "qxl-vga.vram_size=%u", + vram * 1024); + } + } else { + if (qxl_ram) { + virCommandAddArg(cmd, "-global"); + virCommandAddArgFormat(cmd, "qxl.ram_size=%u", + qxl_ram * 1024); + } + if (vram) { + virCommandAddArg(cmd, "-global"); + virCommandAddArgFormat(cmd, "qxl.vram_size=%u", + vram * 1024); + } + }
This feels long. Why not the shorter:
const char *dev = (qemuCapsGet(caps, QEMU_CAPS_DEVICE_QXL_VGA) ? "qxl-vga" : "qxl"); if (qxl_ram) virCommandAddArgFormat(cmd, "-global=%s.ram_size=%u", dev, qxl_ram * 1024); if (vram) virCommandAddArgFormat(cmd, "-global=%s.vram_size=%u", dev, vram * 1024);
+++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-qxl-vga.args @@ -3,5 +3,5 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=spice \ 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.vram_size=33554432 -device qxl,id=video1,vram_size=67108864,bus=pci.0,addr=0x4 \ +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 \
This is a really long line; please insert a \-newline pair to break it and keep the file back under 80 columns.
Looking forward to v2.
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Fri, Jan 18, 2013 at 05:22:32 -0500, Alon Levy wrote:
Hi Eric,
I'm having trouble with the RNG approach. I've created a test rng and test xml with xmllint that can create the problem I am seeing, specifically:
test.rng:
<?xml version="1.0"?> <grammar xmlns="http://relaxng.org/ns/structure/1.0" datatypeLibrary="http://www.w3.org/2001/XMLSchema-datatypes"> <start> <ref name="video"/> </start> <define name="video"> <element name="video"> <optional> <element name="model"> <attribute name="type"> <choice> <value>vga</value> </choice> </attribute> <group> <attribute name="type"> <value>qxl</value> </attribute> <optional> <attribute name="ram"> </attribute> </optional> </group>
The critical part you are missing is that the attribute type=vga and the group containing attribute type=qxl need to be within a choice element. That is: <element name="model"> <choice> <!-- this is important --> <attribute name="type"> <choice> <value>vga</value> ... </choice> </attribute> <group> <attribute name="type"> <value>qxl</value> </attribute> <optional> <attribute name="ram"/> </optional> </group> </choice> ...
<optional> <attribute name="vram"> </attribute> </optional> </element> </optional> </element> </define> </grammar>
Jirka
On 01/17/2013 12:35 PM, Alon Levy wrote:
Adds a qxl-ram attribute globaly to the video.model element, that changes
s/globaly/globally/
check.
the resulting qemu command line only if video.type == "qxl".
That attribute gets a default value of 64*1024 only if model.type is "qxl". In effect not changing any xml or argv for non qxl devices.
For qxl devices a new property is set: -global qxl-vga.ram_size=<ram>*1024 or -global qxl.ram_size=<ram>*1024
This part of the commit message shows the qemu interface; but it would also be handy to show the intended libvirt XML interface. As written, this patch is basically adding qxl_ram into:
<video> <model type='qxl' vram='65536' qxl-ram='65536' heads='1'/> </video>
check.
For the main and secondary qxl devices respectively.
The default for the qxl ram bar is the same as the default for the qxl vram bar, 64*1024. --- I've added a qxl-ram attribute.
Ah, this information is what I was looking for, but because it came after ---, it would be missing from git history.
There is no precedent for adding am attribute prefixed like this, so I'm open for any other suggestion on how to do it.
Why prefix it at all? What's wrong with just naming it 'ram', which is visually distinct from 'vram'?
dropped the prefix.
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 67ae864..50fc834 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2251,7 +2251,9 @@ </define> <!-- A video adapter description, allowing configuration of device - model, number of virtual heads, and video ram size + model, number of virtual heads, and video ram size. + The qxl-ram property is used for qxl types only to specify the + primary bar size, letting vram specify the secondary bar size.
Michal already pointed out the missing documentation in docs/formatdomain.html.in. Basically, I would move this comment that you added here out of the RNG and into the public html.in documentation.
fixed.
Also, in RNG, it is possible to encode the limitation of using the new attribute only when tied to type='qxl', as follows (and here, with my preferred naming of 'ram' instead of 'qxl-ram'):
<element name="model"> <choice> <attribute name="type"> <choice> <value>vga</value> <value>cirrus</value> <value>vmvga</value> <value>xen</value> <value>vbox</value> </choice> </attribute> <group> <attribute name="type"> <value>qxl</value> </attribute> <optional> <attribute name="ram"> <ref name="unsignedInt"/> </attribute> </optional> </group> </choice> <optional> <attribute name="vram"> <ref name="unsignedInt"/> </attribute> </optional>
After Jiri's help noticed you had the choice element there and I just missed it (should have just copy pasted..)
+++ b/src/conf/domain_conf.c @@ -7391,6 +7391,7 @@ virDomainVideoDefParseXML(const xmlNodePtr node, char *type = NULL; char *heads = NULL; char *vram = NULL; + char *qxl_ram = NULL;
Again, if we go with naming the new attribute just 'ram', this variable name can be shorter.
@@ -7431,6 +7433,18 @@ virDomainVideoDefParseXML(const xmlNodePtr node, } }
+ if (qxl_ram) { + if (virStrToLong_ui(qxl_ram, NULL, 10, &def->qxl_ram) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse video qxl-ram '%s'"), qxl_ram); + goto error; + } + } else { + if (def->type == VIR_DOMAIN_VIDEO_TYPE_QXL) { + def->qxl_ram = virDomainVideoDefaultRAM(dom, def->type);
Did you mean for it to always default to the domain default, even when vram is present but not the domain default? Or did you want it to fall back to the vram value?
Yes, that's what I meant. I'll make sure the documentation is understood that way.
+ } + } + if (vram) {
Since you documented that '[qxl-]ram' defaults to the value of 'vram', I think you need to put the new if block after this existing vram block.
Oh, now I see. I meant type value, not a copy of the computed vram value. I can make points for both options, i.e. <video type="qxl" vram="1234"/> resulting in vram=1234 and ram=1234 or resulting in vram=1234 and ram=default_vram_value (right now 64*1024) I meant the later.
@@ -13383,6 +13398,8 @@ virDomainVideoDefFormat(virBufferPtr buf, virBufferAddLit(buf, " <video>\n"); virBufferAsprintf(buf, " <model type='%s'", model); + if (def->qxl_ram && def->type == VIR_DOMAIN_VIDEO_TYPE_QXL) + virBufferAsprintf(buf, " qxl-ram='%u'", def->qxl_ram);
The && def->type == VIR_DOMAIN_VIDEO_TYPE_QXL is not needed here; def->qxl_ram will be 0 for all other video types, based on the restrictions you had in the parser.
done.
+++ b/src/conf/domain_conf.h @@ -1157,6 +1157,7 @@ struct _virDomainVideoAccelDef {
struct _virDomainVideoDef { int type; + unsigned int qxl_ram; unsigned int vram;
Might be worth adding a comment here on units (that is, both qxl_ram and vram use kb).
done.
+++ b/src/qemu/qemu_command.c @@ -3563,6 +3563,15 @@ qemuBuildDeviceVideoStr(virDomainVideoDefPtr video, UINT_MAX / 1024); goto error; } + if (video->qxl_ram > (UINT_MAX / 1024)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
VIR_ERR_OVERFLOW might be better (but this was copy-and-paste from vram, so the same applies there).
fixed.
@@ -6569,23 +6578,48 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArgList(cmd, "-vga", vgastr, NULL);
if (def->videos[0]->type == VIR_DOMAIN_VIDEO_TYPE_QXL) { - if (def->videos[0]->vram && + if ((def->videos[0]->vram || def->videos[0]->qxl_ram) && qemuCapsGet(caps, QEMU_CAPS_DEVICE)) { - if (def->videos[0]->vram > (UINT_MAX / 1024)) { + int qxl_ram = def->videos[0]->qxl_ram; + int vram = def->videos[0]->vram; + if (vram > (UINT_MAX / 1024)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
More places where the existing code would be better as VIR_ERR_OVERFLOW.
fixed.
+ if (qemuCapsGet(caps, QEMU_CAPS_DEVICE_QXL_VGA)) { + if (qxl_ram) { + virCommandAddArg(cmd, "-global"); + virCommandAddArgFormat(cmd, + "qxl-vga.ram_size=%u", + qxl_ram * 1024); + } + if (vram) { + virCommandAddArg(cmd, "-global"); + virCommandAddArgFormat(cmd, + "qxl-vga.vram_size=%u", + vram * 1024); + } + } else { + if (qxl_ram) { + virCommandAddArg(cmd, "-global"); + virCommandAddArgFormat(cmd, "qxl.ram_size=%u", + qxl_ram * 1024); + } + if (vram) { + virCommandAddArg(cmd, "-global"); + virCommandAddArgFormat(cmd, "qxl.vram_size=%u", + vram * 1024); + } + }
This feels long. Why not the shorter:
const char *dev = (qemuCapsGet(caps, QEMU_CAPS_DEVICE_QXL_VGA) ? "qxl-vga" : "qxl"); if (qxl_ram) virCommandAddArgFormat(cmd, "-global=%s.ram_size=%u", dev, qxl_ram * 1024); if (vram) virCommandAddArgFormat(cmd, "-global=%s.vram_size=%u", dev, vram * 1024);
done. I wasn't sure this would be worth it, don't know why.
+++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-qxl-vga.args @@ -3,5 +3,5 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=spice \ 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.vram_size=33554432 -device qxl,id=video1,vram_size=67108864,bus=pci.0,addr=0x4 \ +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 \
This is a really long line; please insert a \-newline pair to break it and keep the file back under 80 columns.
fixing.
Looking forward to v2.
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Thu, Jan 17, 2013 at 09:35:22PM +0200, Alon Levy wrote:
Adds a qxl-ram attribute globaly to the video.model element, that changes the resulting qemu command line only if video.type == "qxl".
That attribute gets a default value of 64*1024 only if model.type is "qxl". In effect not changing any xml or argv for non qxl devices.
For qxl devices a new property is set: -global qxl-vga.ram_size=<ram>*1024 or -global qxl.ram_size=<ram>*1024
For the main and secondary qxl devices respectively.
The default for the qxl ram bar is the same as the default for the qxl vram bar, 64*1024. --- I've added a qxl-ram attribute. There is no precedent for adding am attribute prefixed like this, so I'm open for any other suggestion on how to do it.
What is the difference between this new RAM size and what we currently set for ram size. That will influence how we pick a good name, ie one with any qxl prefix, which is not something we can use. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Thu, Jan 17, 2013 at 09:35:22PM +0200, Alon Levy wrote:
Adds a qxl-ram attribute globaly to the video.model element, that changes the resulting qemu command line only if video.type == "qxl".
That attribute gets a default value of 64*1024 only if model.type is "qxl". In effect not changing any xml or argv for non qxl devices.
For qxl devices a new property is set: -global qxl-vga.ram_size=<ram>*1024 or -global qxl.ram_size=<ram>*1024
For the main and secondary qxl devices respectively.
The default for the qxl ram bar is the same as the default for the qxl vram bar, 64*1024. --- I've added a qxl-ram attribute. There is no precedent for adding am attribute prefixed like this, so I'm open for any other suggestion on how to do it.
What is the difference between this new RAM size and what we currently set for ram size. That will influence how we pick a good name, ie one with any qxl prefix, which is not something we can use.
Answering you, Eric and Michal about the difference / name (I'll do a v2 for Eric's comments anyway): vram is the current choice for the across the board attribute, and it is used for qxl devices to determine the size of the *second* bar for a qxl device, the surfaces bar. But there is no attribute to determine the size of the first bar. And it isn't good enough to say "use some deterministic connection between the two, like equality", because you may want to have them with different sizes and we don't yet know what the policy is going to be, for multiple reasons, one of them being that they will effect performance differently, another that seabios doesn't allow setting both to the size we want because of some limitations (512MB seabios PCI allocation hole for pre q35 and for non 64 bit guests). So I need to add a new attribute. Eric's RNG shows how to limit the new attribute to qxl only by RNG instead of code, which I am glad to know and I'll use. The name can be changed, I wanted to make it abundantly clear to any xml user that this is qxl only right now, but I can change it to just "ram" if you don't think it is confusing (I do).
Daniel -- |: http://berrange.com -o- | http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- | http://virt-manager.org :| |: http://autobuild.org -o- | http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- | http://live.gnome.org/gtk-vnc :|
On Thu, Jan 17, 2013 at 09:35:22PM +0200, Alon Levy wrote:
Adds a qxl-ram attribute globaly to the video.model element, that changes the resulting qemu command line only if video.type == "qxl".
That attribute gets a default value of 64*1024 only if model.type is "qxl". In effect not changing any xml or argv for non qxl devices.
For qxl devices a new property is set: -global qxl-vga.ram_size=<ram>*1024 or -global qxl.ram_size=<ram>*1024
For the main and secondary qxl devices respectively.
The default for the qxl ram bar is the same as the default for the qxl vram bar, 64*1024. --- I've added a qxl-ram attribute. There is no precedent for adding am attribute prefixed like this, so I'm open for any other suggestion on how to do it.
What is the difference between this new RAM size and what we currently set for ram size. That will influence how we pick a good name, ie one with any qxl prefix, which is not something we can use.
Just to be clear: ram_size determines the size of the first bar of the qxl device, until now unsettable via libvirt. This bar is used for: surface 0 (and VGA emulation), rendering commands, monitors config, ram header. vram_size determines the size of the second bar of the qxl device. settable via the vram attribute. This bar is used for: surface > 0 allocations (aka off screen surfaces).
Daniel -- |: http://berrange.com -o- | http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- | http://virt-manager.org :| |: http://autobuild.org -o- | http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- | http://live.gnome.org/gtk-vnc :|
Hi,
What is the difference between this new RAM size and what we currently set for ram size. That will influence how we pick a good name, ie one with any qxl prefix, which is not something we can use.
It's all a bit more complicated ... Current upstream has *four* parameters here: qxl-vga.ram_size_mb=uint32 Size of PCI Bar 0. Default: 64. VGA framebuffer lives there, also qxl rendering commands and parameters (image data, clip regions, whatever) for the qxl rendering commands. qxl-vga.vram_size_mb=uint32 Size of PCI Bar 1. Default: 64. Storage for surfaces. Basically image data too, but you can do qxl raster ops on them. X11 uses this for offscreen pixmaps. qxl-vga.vram64_size_mb=uint32 There is an (optional) 64bit version of PCI Bar 1 (Bar 5 IIRC), which can be mapped above 4G. Both bars are backed by the same memory. qxl-vga.vgamem_mb=uint32 VGA framebuffer size. Default: 16. Specifies how much of the PCI bar 0 memory should be available for the VGA framebuffer. ram_size/vramsize is there too. Same as ram_size_mb/vram_size_mb, but they want bytes instead of megabytes. And to complete the picture: The other vga cards (std, cirrus, vmware) got vgamem_mb parameters too, which simply specify the size of the card's memory. For cirrus its pointless as the emulated hardware constrains the memory size and the only reason this is configurable in the first place is qemu <-> qemu-kvm live migration compatibility. For the other cards it makes sense to allow tuning the vgamem_mb parameter via domain xml. So, how expose this to the user? We already have 'vram'. Adding 'ram' makes sense. vram64 not so IMHO. I think it would be better to have a 64bit bool, then do this: if (64bit) qxl-vga.vram64_size_mb=${vram},vram_size_mb=1 else qxl-vga.vram_size_mb=${vram} Dunno what to do best with vgamem_mb for qxl. One more parameter? Or use ram_size/4? The other vga cards are a bit tricky too. If you add a cirrus or stdvga libvirt fills in vram='9216'. Where does *that* come from btw? It isn't the qemu default (used to be 8 MB, now 16 MB). It's also not the qemu-kvm default (was 16 MB all the time). And it isn't valid too (pci bar size must be a power of two). So reusing that for vgamem_mb is probably asking for trouble. So maybe the best and least confusing would be to add a vgamem parameter for all vga cards (including qxl, except cirrus)? cheers, Gerd
participants (6)
-
Alon Levy -
Daniel P. Berrange -
Eric Blake -
Gerd Hoffmann -
Jiri Denemark -
Michal Privoznik