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

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(a)redhat.com&gt; >--- > 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?

> 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), ...

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

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

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.