[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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
> 
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]