[libvirt] [PATCH] qemu: Support ram bar size for qxl devices
Alon Levy
alevy at redhat.com
Fri Jan 18 18:15:10 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/
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
>
>
More information about the libvir-list
mailing list