12:26 p.m.
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>
>---
> 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.