Re: [libvirt] [PATCH] qemu: Support ram bar size for qxl devices

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