[libvirt] [PATCH 3/3] qemu: introduce vram64 attribute for QXL video device
Martin Kletzander
mkletzan at redhat.com
Tue Feb 23 07:55:29 UTC 2016
On Mon, Feb 22, 2016 at 07:26:42PM +0100, Pavel Hrdina wrote:
>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 at redhat.com>
>> >---
>> > 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.
>
Yes, I know that, it was just an introduction to ...
>>
>> > 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.
>
... this question. Good that we're on the same page, I just wanted to
make sure I understand it the same way you do.
>>
>> > 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 :)
>
Yes, but we are not checking that it supports that attribute anywhere.
Or are you saying that whenever QEMU supports qxl, it has this property?
>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.
>>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160223/d9ebdd25/attachment-0001.sig>
More information about the libvir-list
mailing list