[libvirt] [PATCH] qemu: Support ram bar size for qxl devices
Eric Blake
eblake at redhat.com
Thu Jan 17 21:14:29 UTC 2013
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130117/e8a8c315/attachment-0001.sig>
More information about the libvir-list
mailing list