Re: [PATCH 3/3] qemu: introduce vram64 attribute for QXL video device

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

 



On Mon, Feb 22, 2016 at 04:16:49PM +0100, Martin Kletzander wrote:
> On Mon, Feb 22, 2016 at 02:34:14PM +0100, Pavel Hrdina wrote:
> >This attribute is used to extend secondary PCI bar and expose it to the
> >guest as 64bit memory.  It works like this: attribute vram is there to
> >set size of secondary PCI bar and guest sees it as 32bit memory,
> >attribute vram64 can extend this secondary PCI bar.  If both attributes
> >are used, guest sees two memory bars, both address the same memory, with
> >the difference that the 32bit bar can address only the first part of the
> >whole memory.
> >
> >Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1260749
> >
> >Signed-off-by: Pavel Hrdina <phrdina@xxxxxxxxxx>
> >---
> > docs/formatdomain.html.in                          |  2 ++
> > docs/schemas/domaincommon.rng                      |  5 ++++
> > src/conf/domain_conf.c                             | 34 ++++++++++++++++++----
> > src/conf/domain_conf.h                             |  1 +
> > src/qemu/qemu_command.c                            | 13 +++++++++
> > src/qemu/qemu_monitor_json.c                       |  8 +++++
> > .../qemuxml2argv-video-qxl-device-vram64.args      | 25 ++++++++++++++++
> > .../qemuxml2argv-video-qxl-device-vram64.xml       | 29 ++++++++++++++++++
> > .../qemuxml2argv-video-qxl-sec-device-vram64.args  | 27 +++++++++++++++++
> > .../qemuxml2argv-video-qxl-sec-device-vram64.xml   | 32 ++++++++++++++++++++
> > 10 files changed, 170 insertions(+), 6 deletions(-)
> > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-qxl-device-vram64.args
> > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-qxl-device-vram64.xml
> > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-qxl-sec-device-vram64.args
> > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-qxl-sec-device-vram64.xml
> >
> >diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> >index 3fcd728..318ffd9 100644
> >--- a/docs/formatdomain.html.in
> >+++ b/docs/formatdomain.html.in
> >@@ -5191,6 +5191,8 @@ qemu-kvm -net nic,model=? /dev/null
> >           two as <code>vram</code>. There is also optional attribute
> >           <code>vgamem</code> (<span class="since">since 1.2.11</span>) to set
> >           the size of VGA framebuffer for fallback mode of QXL device.
> >+          Attribute <code>vram64</code> (<span class="since">since 1.3.2</span>)
> >+          extends secondary bar and makes it addressable as 64bit memory.
> >         </p>
> >       </dd>
> >
> >diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> >index 67af93a..fe5eaf0 100644
> >--- a/docs/schemas/domaincommon.rng
> >+++ b/docs/schemas/domaincommon.rng
> >@@ -2938,6 +2938,11 @@
> >                   <ref name="unsignedInt"/>
> >                 </attribute>
> >               </optional>
> >+              <optional>
> >+                <attribute name="vram64">
> >+                  <ref name="unsignedInt"/>
> >+                </attribute>
> >+              </optional>
> >             </group>
> >           </choice>
> >           <optional>
> >diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> >index 56bd1aa..76fc52a 100644
> >--- a/src/conf/domain_conf.c
> >+++ b/src/conf/domain_conf.c
> >@@ -11898,6 +11898,7 @@ virDomainVideoDefParseXML(xmlNodePtr node,
> >     char *type = NULL;
> >     char *heads = NULL;
> >     char *vram = NULL;
> >+    char *vram64 = NULL;
> >     char *ram = NULL;
> >     char *vgamem = NULL;
> >     char *primary = NULL;
> >@@ -11913,6 +11914,7 @@ virDomainVideoDefParseXML(xmlNodePtr node,
> >                 type = virXMLPropString(cur, "type");
> >                 ram = virXMLPropString(cur, "ram");
> >                 vram = virXMLPropString(cur, "vram");
> >+                vram64 = virXMLPropString(cur, "vram64");
> >                 vgamem = virXMLPropString(cur, "vgamem");
> >                 heads = virXMLPropString(cur, "heads");
> >
> >@@ -11967,6 +11969,19 @@ virDomainVideoDefParseXML(xmlNodePtr node,
> >         def->vram = virDomainVideoDefaultRAM(dom, def->type);
> >     }
> >
> >+    if (vram64) {
> >+        if (def->type != VIR_DOMAIN_VIDEO_TYPE_QXL) {
> >+            virReportError(VIR_ERR_XML_ERROR, "%s",
> >+                           _("vram64 attribute only supported for type of qxl"));
> >+            goto error;
> >+        }
> >+        if (virStrToLong_uip(vram64, NULL, 10, &def->vram64) < 0) {
> >+            virReportError(VIR_ERR_XML_ERROR,
> >+                           _("cannot parse video vram64 '%s'"), vram64);
> >+            goto error;
> >+        }
> >+    }
> >+
> >     if (vgamem) {
> >         if (def->type != VIR_DOMAIN_VIDEO_TYPE_QXL) {
> >             virReportError(VIR_ERR_XML_ERROR, "%s",
> >@@ -11993,9 +12008,11 @@ virDomainVideoDefParseXML(xmlNodePtr node,
> >     if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0)
> >         goto error;
> >
> >+ cleanup:
> >     VIR_FREE(type);
> >     VIR_FREE(ram);
> >     VIR_FREE(vram);
> >+    VIR_FREE(vram64);
> >     VIR_FREE(vgamem);
> >     VIR_FREE(heads);
> >
> >@@ -12003,12 +12020,8 @@ virDomainVideoDefParseXML(xmlNodePtr node,
> >
> >  error:
> >     virDomainVideoDefFree(def);
> >-    VIR_FREE(type);
> >-    VIR_FREE(ram);
> >-    VIR_FREE(vram);
> >-    VIR_FREE(vgamem);
> >-    VIR_FREE(heads);
> >-    return NULL;
> >+    def = NULL;
> >+    goto cleanup;
> > }
> >
> > static virDomainHostdevDefPtr
> >@@ -17023,6 +17036,13 @@ virDomainVideoDefCheckABIStability(virDomainVideoDefPtr src,
> >         return false;
> >     }
> >
> >+    if (src->vram64 != dst->vram64) {
> >+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> >+                       _("Target video card vram64 %u does not match source %u"),
> >+                       dst->vram64, src->vram64);
> >+        return false;
> >+    }
> >+
> 
> Does this check make sense if we're updating that value after QEMU
> starts?

Yes, it makes sense, this will ensure, that user doesn't change this value via
qemu hook.

> 
> >     if (src->vgamem != dst->vgamem) {
> >         virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> >                        _("Target video card vgamem %u does not match source %u"),
> >@@ -20708,6 +20728,8 @@ virDomainVideoDefFormat(virBufferPtr buf,
> >         virBufferAsprintf(buf, " ram='%u'", def->ram);
> >     if (def->vram)
> >         virBufferAsprintf(buf, " vram='%u'", def->vram);
> >+    if (def->vram64)
> >+        virBufferAsprintf(buf, " vram64='%u'", def->vram64);
> >     if (def->vgamem)
> >         virBufferAsprintf(buf, " vgamem='%u'", def->vgamem);
> >     if (def->heads)
> >diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> >index 1de3be3..c3a7386 100644
> >--- a/src/conf/domain_conf.h
> >+++ b/src/conf/domain_conf.h
> >@@ -1399,6 +1399,7 @@ struct _virDomainVideoDef {
> >     int type;
> >     unsigned int ram;  /* kibibytes (multiples of 1024) */
> >     unsigned int vram; /* kibibytes (multiples of 1024) */
> >+    unsigned int vram64; /* kibibytes (multiples of 1024) */
> >     unsigned int vgamem; /* kibibytes (multiples of 1024) */
> >     unsigned int heads;
> >     bool primary;
> >diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> >index 78423e7..24ddd8a 100644
> >--- a/src/qemu/qemu_command.c
> >+++ b/src/qemu/qemu_command.c
> >@@ -3347,6 +3347,12 @@ qemuBuildDeviceVideoStr(virDomainDefPtr def,
> >             virBufferAsprintf(&buf, ",vram_size=%u", video->vram * 1024);
> >         }
> >
> >+        if ((primary && virQEMUCapsGet(qemuCaps, QEMU_CAPS_QXL_VGA_VRAM64)) ||
> >+            (!primary && virQEMUCapsGet(qemuCaps, QEMU_CAPS_QXL_VRAM64))) {
> >+            /* QEMU accepts mebibytes for vram64_size_mb. */
> >+            virBufferAsprintf(&buf, ",vram64_size_mb=%u", video->vram64 / 1024);
> >+        }
> >+
> 
> Why do we both set the size here (for both primary and secondary card), ...

This code is a part of for cycle for all video devices.  This means, if current
video device is primary, we need to check different flag than for secondary
video device.

Note: Both secondary and primary qxl video device are internally represented by
the same device model in qemu, so both of them will ether have this feature or
not.

> 
> >         if ((primary && virQEMUCapsGet(qemuCaps, QEMU_CAPS_QXL_VGA_VGAMEM)) ||
> >             (!primary && virQEMUCapsGet(qemuCaps, QEMU_CAPS_QXL_VGAMEM))) {
> >             /* QEMU accepts mebibytes for vgamem_mb. */
> >@@ -8254,6 +8260,7 @@ qemuBuildCommandLine(virConnectPtr conn,
> >                     virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
> >                     unsigned int ram = def->videos[0]->ram;
> >                     unsigned int vram = def->videos[0]->vram;
> >+                    unsigned int vram64 = def->videos[0]->vram64;
> >                     unsigned int vgamem = def->videos[0]->vgamem;
> >
> >                     if (vram > (UINT_MAX / 1024)) {
> >@@ -8279,6 +8286,12 @@ qemuBuildCommandLine(virConnectPtr conn,
> >                         virCommandAddArgFormat(cmd, "%s.vram_size=%u",
> >                                                dev, vram * 1024);
> >                     }
> >+                    if (vram64 &&
> >+                        virQEMUCapsGet(qemuCaps, QEMU_CAPS_QXL_VGA_VRAM64)) {
> >+                        virCommandAddArg(cmd, "-global");
> >+                        virCommandAddArgFormat(cmd, "%s.vram64_size_mb=%u",
> >+                                               dev, vram64 / 1024);
> >+                    }
> 
> ... and also set it here?  And mostly why do we check only for primary
> card capability here?  I remember that this was needed for some
> parameters when QEMU_CAPS_DEVICE was a variable (before we switched to
> supporting only QEMU_CAPS_DEVICE-enabled versions).  Although the code
> does the opposite thing (uses '-global' only for qemu with '-device'
> supported).  I must be clearly on a bad track here.

This is probably a dead code since we've removed support for qemu that doesn't
have -device.  Without -device it was possible only have one video device.

This should be removed, but not as part of this series.

> 
> >                     if (vgamem &&
> >                         virQEMUCapsGet(qemuCaps, QEMU_CAPS_QXL_VGA_VGAMEM)) {
> >                         virCommandAddArg(cmd, "-global");
> >diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> >index d2b641f..34efd4f 100644
> >--- a/src/qemu/qemu_monitor_json.c
> >+++ b/src/qemu/qemu_monitor_json.c
> >@@ -1436,6 +1436,14 @@ qemuMonitorJSONUpdateVideoMemorySize(qemuMonitorPtr mon,
> >             return -1;
> >         }
> >         video->vram = prop.val.ul / 1024;
> >+        if (qemuMonitorJSONGetObjectProperty(mon, path,
> >+                                             "vram64_size_mb", &prop) < 0) {
> >+            virReportError(VIR_ERR_INTERNAL_ERROR,
> >+                           _("QOM Object '%s' has no property 'vram64_size_mb'"),
> >+                           path);
> >+            return -1;
> >+        }
> >+        video->vram64 = prop.val.ul / 1024;
> 
> And a last question: why don't we just skip the vram64_size_mb if it's
> not available and move on to the next one?  What if it's a bit older
> QEMU?

I'm not sure about this, but if a device supports some attribute, it should be
also present in QOM Object.  If this happens, something is really wrong :)

Pavel

> 
> For all the questions a short explanation might be enough as all the
> other properties are coded following the same fashion.  That doesn't
> make them correct, though, so the explanation is still needed.
> 

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]