[libvirt] [PATCH 3/3] qemu: introduce vram64 attribute for QXL video device
Martin Kletzander
mkletzan at redhat.com
Mon Feb 22 15:16:49 UTC 2016
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?
> 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.
> if (qemuMonitorJSONGetObjectProperty(mon, path, "ram_size", &prop) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("QOM Object '%s' has no property 'ram_size'"),
>diff --git a/tests/qemuxml2argvdata/qemuxml2argv-video-qxl-device-vram64.args b/tests/qemuxml2argvdata/qemuxml2argv-video-qxl-device-vram64.args
>new file mode 100644
>index 0000000..b9e65ea
>--- /dev/null
>+++ b/tests/qemuxml2argvdata/qemuxml2argv-video-qxl-device-vram64.args
>@@ -0,0 +1,25 @@
>+LC_ALL=C \
>+PATH=/bin \
>+HOME=/home/test \
>+USER=test \
>+LOGNAME=test \
>+QEMU_AUDIO_DRV=none \
>+/usr/bin/qemu-system-x86_64 \
>+-name QEMUGuest1 \
>+-S \
>+-M pc \
>+-m 1024 \
>+-smp 1 \
>+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
>+-nographic \
>+-nodefaults \
>+-monitor unix:/tmp/test-monitor,server,nowait \
>+-no-acpi \
>+-boot c \
>+-usb \
>+-drive file=/var/lib/libvirt/images/QEMUGuest1,format=qcow2,if=none,\
>+id=drive-ide0-0-0,cache=none \
>+-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \
>+-device qxl-vga,id=video0,ram_size=67108864,vram_size=67108864,\
>+vram64_size_mb=128,vgamem_mb=16,bus=pci.0,addr=0x2 \
>+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
>diff --git a/tests/qemuxml2argvdata/qemuxml2argv-video-qxl-device-vram64.xml b/tests/qemuxml2argvdata/qemuxml2argv-video-qxl-device-vram64.xml
>new file mode 100644
>index 0000000..1e89d06
>--- /dev/null
>+++ b/tests/qemuxml2argvdata/qemuxml2argv-video-qxl-device-vram64.xml
>@@ -0,0 +1,29 @@
>+<domain type='qemu'>
>+ <name>QEMUGuest1</name>
>+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
>+ <memory unit='KiB'>1048576</memory>
>+ <currentMemory unit='KiB'>1048576</currentMemory>
>+ <vcpu>1</vcpu>
>+ <os>
>+ <type arch='x86_64' machine='pc'>hvm</type>
>+ <boot dev='hd'/>
>+ </os>
>+ <clock offset='utc'/>
>+ <on_poweroff>destroy</on_poweroff>
>+ <on_reboot>restart</on_reboot>
>+ <on_crash>destroy</on_crash>
>+ <devices>
>+ <emulator>/usr/bin/qemu-system-x86_64</emulator>
>+ <disk type='file' device='disk'>
>+ <driver name='qemu' type='qcow2' cache='none'/>
>+ <source file='/var/lib/libvirt/images/QEMUGuest1'/>
>+ <target dev='hda' bus='ide'/>
>+ <address type='drive' controller='0' bus='0' target='0' unit='0'/>
>+ </disk>
>+ <controller type='ide' index='0'/>
>+ <video>
>+ <model type='qxl' vram64='131072' heads='1'/>
>+ </video>
>+ <memballoon model='virtio'/>
>+ </devices>
>+</domain>
>diff --git a/tests/qemuxml2argvdata/qemuxml2argv-video-qxl-sec-device-vram64.args b/tests/qemuxml2argvdata/qemuxml2argv-video-qxl-sec-device-vram64.args
>new file mode 100644
>index 0000000..fadc3ed
>--- /dev/null
>+++ b/tests/qemuxml2argvdata/qemuxml2argv-video-qxl-sec-device-vram64.args
>@@ -0,0 +1,27 @@
>+LC_ALL=C \
>+PATH=/bin \
>+HOME=/home/test \
>+USER=test \
>+LOGNAME=test \
>+QEMU_AUDIO_DRV=none \
>+/usr/bin/qemu \
>+-name QEMUGuest1 \
>+-S \
>+-M pc \
>+-m 1024 \
>+-smp 1 \
>+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
>+-nographic \
>+-nodefaults \
>+-monitor unix:/tmp/test-monitor,server,nowait \
>+-no-acpi \
>+-boot c \
>+-usb \
>+-drive file=/var/lib/libvirt/images/QEMUGuest1,format=qcow2,if=none,\
>+id=drive-ide0-0-0,cache=none \
>+-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \
>+-device qxl-vga,id=video0,ram_size=67108864,vram_size=67108864,vgamem_mb=16,\
>+bus=pci.0,addr=0x2 \
>+-device qxl,id=video1,ram_size=67108864,vram_size=67108864,vram64_size_mb=128,\
>+vgamem_mb=16,bus=pci.0,addr=0x4 \
>+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
>diff --git a/tests/qemuxml2argvdata/qemuxml2argv-video-qxl-sec-device-vram64.xml b/tests/qemuxml2argvdata/qemuxml2argv-video-qxl-sec-device-vram64.xml
>new file mode 100644
>index 0000000..72e8bad
>--- /dev/null
>+++ b/tests/qemuxml2argvdata/qemuxml2argv-video-qxl-sec-device-vram64.xml
>@@ -0,0 +1,32 @@
>+<domain type='qemu'>
>+ <name>QEMUGuest1</name>
>+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
>+ <memory unit='KiB'>1048576</memory>
>+ <currentMemory unit='KiB'>1048576</currentMemory>
>+ <vcpu>1</vcpu>
>+ <os>
>+ <type arch='i686' machine='pc'>hvm</type>
>+ <boot dev='hd'/>
>+ </os>
>+ <clock offset='utc'/>
>+ <on_poweroff>destroy</on_poweroff>
>+ <on_reboot>restart</on_reboot>
>+ <on_crash>destroy</on_crash>
>+ <devices>
>+ <emulator>/usr/bin/qemu</emulator>
>+ <disk type='file' device='disk'>
>+ <driver name='qemu' type='qcow2' cache='none'/>
>+ <source file='/var/lib/libvirt/images/QEMUGuest1'/>
>+ <target dev='hda' bus='ide'/>
>+ <address type='drive' controller='0' bus='0' target='0' unit='0'/>
>+ </disk>
>+ <controller type='ide' index='0'/>
>+ <video>
>+ <model type='qxl' heads='1'/>
>+ </video>
>+ <video>
>+ <model type='qxl' vram64='131072' heads='1'/>
>+ </video>
>+ <memballoon model='virtio'/>
>+ </devices>
>+</domain>
>--
>2.7.1
>
>--
>libvir-list mailing list
>libvir-list at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- 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/20160222/fd6f543b/attachment-0001.sig>
More information about the libvir-list
mailing list