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

[Martin Kletzander <mkletzan@xxxxxxxxxx>]

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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list