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

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