qxl_pre_save assertion failure on vm "save"

When I try and save my Windows 10 VM, I see an assertion failure, and the machine is shut down.

I see the following in the log:

main_channel_handle_parsed: agent start
qemu-system-x86_64: /build/qemu-Zwynhi/qemu-2.5+dfsg/hw/display/qxl.c:2101: qxl_pre_save: Assertion `d->last_release_offset < d->vga.vram_size' failed.
2016-10-20 11:52:42.713+0000: shutting down

Please let me know what other information would be relevant!

Hmm docmax on #qemu just complained about a similar error;   although they were on win2016, and using qemu-2.7 and the latest git versions, and that assert has been in there for years.

Can you please add the full qemu command line you're using, and the version of the spice/qxl drivers you're using inside the windows VM.


QXL driver version is 17.54.59.923

Commandline (git compiled today) is:

/usr/sbin/qemu-system-x86_64 -name guest=dc,debug-threads=on -S -object secret,id=masterKey0,format=raw,file=/var/lib/libvirt/qemu/domain-1-dc/master-key.aes -machine pc-i440fx-2.7,accel=kvm,usb=off,vmport=off,dump-guest-core=off -cpu Haswell-noTSX,+vme,+ds,+acpi,+ss,+ht,+tm,+pbe,+dtes64,+monitor,+ds_cpl,+vmx,+est,+tm2,+xtpr,+pdcm,+osxsave,+f16c,+rdrand,+arat,+tsc_adjust,+xsaveopt,+pdpe1gb,+abm -drive file=/usr/share/ovmf/x64/ovmf_code_x64.bin,if=pflash,format=raw,unit=0,readonly=on -drive file=/var/lib/libvirt/qemu/nvram/dc_VARS.fd,if=pflash,format=raw,unit=1 -m 2048 -realtime mlock=off -smp 4,sockets=1,cores=4,threads=1 -uuid 647fd6b2-9a88-4219-af46-052f68a539a5 -no-user-config -nodefaults -chardev socket,id=charmonitor,path=/var/lib/libvirt/qemu/domain-1-dc/monitor.sock,server,nowait -mon chardev=charmonitor,id=monitor,mode=control -rtc base=utc,driftfix=slew -global kvm-pit.lost_tick_policy=discard -no-hpet -no-shutdown -global PIIX4_PM.disable_s3=1 -global PIIX4_PM.disable_s4=1 -boot strict=on -device ich9-usb-ehci1,id=usb,bus=pci.0,addr=0x6.0x7 -device ich9-usb-uhci1,masterbus=usb.0,firstport=0,bus=pci.0,multifunction=on,addr=0x6 -device ich9-usb-uhci2,masterbus=usb.0,firstport=2,bus=pci.0,addr=0x6.0x1 -device ich9-usb-uhci3,masterbus=usb.0,firstport=4,bus=pci.0,addr=0x6.0x2 -device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x5 -drive file=/mnt/shared/server/root/var/lib/libvirt/images/hdd/dc.qcow2,format=qcow2,if=none,id=drive-virtio-disk0 -device virtio-blk-pci,scsi=off,bus=pci.0,addr=0x7,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1 -netdev tap,fd=27,id=hostnet0,vhost=on,vhostfd=29 -device virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:58:ce:4e,bus=pci.0,addr=0x3 -chardev pty,id=charserial0 -device isa-serial,chardev=charserial0,id=serial0 -chardev spicevmc,id=charchannel0,name=vdagent -device virtserialport,bus=virtio-serial0.0,nr=1,chardev=charchannel0,id=channel0,name=com.redhat.spice.0 -spice port=5900,addr=127.0.0.1,disable-ticketing,image-compression=off,seamless-migration=on -device qxl-vga,id=video0,ram_size=67108864,vram_size=67108864,vram64_size_mb=0,vgamem_mb=16,max_outputs=1,bus=pci.0,addr=0x2 -device intel-hda,id=sound0,bus=pci.0,addr=0x4 -device hda-duplex,id=sound0-codec0,bus=sound0.0,cad=0 -chardev spicevmc,id=charredir0,name=usbredir -device usb-redir,chardev=charredir0,id=redir0,bus=usb.0,port=1 -chardev spicevmc,id=charredir1,name=usbredir -device usb-redir,chardev=charredir1,id=redir1,bus=usb.0,port=2 -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x8 -msg timestamp=on

I tried other QXL drivers: 22.33.46.473.
These work (but have a older date: 2015-07-28.

17.54.59.923 have the date 2016-04-21.
I got them from this package:

http://depot.flexvdi.com/guest-tools/flexvdi-guest-tools-2.2.11.iso

Those provide something, which lets my window resize freely.

I'm running into this issue as well:
Arch Linux
Qemu 2.8.0
spice guest tools: 0.132
QXL driver version (as reported by Windows Device Manager): 10.0.0.15000

Everything else works great. It would save me a lot of rebooting if this could get fixed.
If there is anything I can do or try, I'll be glad to help.

Relevant log of VM boot and crash on selecting suspend action in virt-manager:

2017-04-06 16:59:24.681+0000: starting up libvirt version: 3.1.0, qemu version: 2.8.0, hostname: arch-vaio.localdomain
LC_ALL=C PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin QEMU_AUDIO_DRV=spice /usr/bin/qemu-system-x86_64 -name guest=Windows,debug-threads=on -S -object secret,id=masterKey0,format=raw,file=/var/lib/libvirt/qemu/domain-9-Windows/master-key.aes -machine pc-i440fx-2.7,accel=kvm,usb=off,vmport=off,dump-guest-core=off -cpu core2duo,hv_time,hv_relaxed,hv_vapic,hv_spinlocks=0x1fff -m 2048 -realtime mlock=off -smp 2,sockets=1,cores=2,threads=1 -uuid f14684d3-5f81-4743-8512-e516d85ca2c9 -no-user-config -nodefaults -chardev socket,id=charmonitor,path=/var/lib/libvirt/qemu/domain-9-Windows/monitor.sock,server,nowait -mon chardev=charmonitor,id=monitor,mode=control -rtc base=localtime,driftfix=slew -global kvm-pit.lost_tick_policy=delay -no-hpet -no-shutdown -global PIIX4_PM.disable_s3=1 -global PIIX4_PM.disable_s4=1 -boot strict=on -device ich9-usb-ehci1,id=usb,bus=pci.0,addr=0x6.0x7 -device ich9-usb-uhci1,masterbus=usb.0,firstport=0,bus=pci.0,multifunction=on,addr=0x6 -device ich9-usb-uhci2,masterbus=usb.0,firstport=2,bus=pci.0,addr=0x6.0x1 -device ich9-usb-uhci3,masterbus=usb.0,firstport=4,bus=pci.0,addr=0x6.0x2 -device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x5 -drive file=/mnt/media/Qemu/windows10.qcow2,format=qcow2,if=none,id=drive-virtio-disk0 -device virtio-blk-pci,scsi=off,bus=pci.0,addr=0x7,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1 -drive file=/usr/share/spice-guest-tools/spice-guest-tools.iso,format=raw,if=none,id=drive-ide0-0-1,readonly=on -device ide-cd,bus=ide.0,unit=1,drive=drive-ide0-0-1,id=ide0-0-1 -netdev user,id=hostnet0 -device rtl8139,netdev=hostnet0,id=net0,mac=52:54:00:44:08:31,bus=pci.0,addr=0x3 -chardev pty,id=charserial0 -device isa-serial,chardev=charserial0,id=serial0 -chardev spicevmc,id=charchannel0,name=vdagent -device virtserialport,bus=virtio-serial0.0,nr=1,chardev=charchannel0,id=channel0,name=com.redhat.spice.0 -device usb-tablet,id=input2,bus=usb.0,port=3 -spice port=5900,addr=127.0.0.1,disable-ticketing,image-compression=off,seamless-migration=on -device qxl-vga,id=video0,ram_size=67108864,vram_size=67108864,vram64_size_mb=0,vgamem_mb=16,max_outputs=1,bus=pci.0,addr=0x2 -device intel-hda,id=sound0,bus=pci.0,addr=0x4 -device hda-duplex,id=sound0-codec0,bus=sound0.0,cad=0 -chardev spicevmc,id=charredir0,name=usbredir -device usb-redir,chardev=charredir0,id=redir0,bus=usb.0,port=1 -chardev spicevmc,id=charredir1,name=usbredir -device usb-redir,chardev=charredir1,id=redir1,bus=usb.0,port=2 -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x8 -msg timestamp=on
char device redirected to /dev/pts/6 (label charserial0)
main_channel_link: add main channel client
red_dispatcher_set_cursor_peer: 
inputs_connect: inputs channel client create
main_channel_handle_parsed: agent start
ehci warning: guest updated active QH
qemu-system-x86_64: /build/qemu/src/qemu-2.8.0/hw/display/qxl.c:2173: qxl_pre_save: Assertion `d->last_release_offset < d->vga.vram_size' failed.
2017-04-06 17:00:32.819+0000: shutting down, reason=crashed

I can see a bunch of similar looking failures in Fedora's automatic backtrace stats system

I put my money on that one:

https://cgit.freedesktop.org/spice/win32/qxl-wddm-dod/commit/?id=f6e099db39e7d0787f294d5fd0dce328b5210faa

commit f6e099db39e7d0787f294d5fd0dce328b5210faa
Author: Sameeh Jubran <email address hidden>
Date:   Sun Sep 11 16:05:24 2016 +0300

    Use the second bar (VRAM) for qxl command buffer.
    
    Based on a patch by Sandy Stutsman <email address hidden>
    
    Signed-off-by: Sameeh Jubran <email address hidden>
    Acked-by: Frediano Ziglio <email address hidden>



Crash reproduced immediately after setting up a win10 VM with qxl driver 10.0.0.15000.

Gerd, are you looking into fixing it? Is it acceptable to crash qemu if the driver is faulty?

damn launchpad, wrong bug and I can't change it back. Please someone move it back to New/Confirmed

Well.  qxl commands are expected to live in bar 0 (same bar where the rings are too).  vram bar was added as surface storage.

Now the windows drivers started to us vram for qxl commands.  Problem is we simply can't live-migrate such a guest.  At least not without changing the vmstate format.  Which isn't something I would attempt just a few days before release.

We can't throw an error in qxl_pre_save either (and fail migration instead of aborting).

I don't see an easy way out for 2.9.

Long term options are (a) revert the driver change, and probably add some checks to qxl to make sure guests don't use vram for commands, or (b) extend qxl vmstate so we can handle that case.

An untested hack that might fail cleaner might be:

  error_report("Broken driver ..... migration failed");
  qemu_file_set_error(migrate_get_current()->to_dst_file, -EINVAL);

(untested, probably needs checking it works with savevm).

I must go around and add a return value to pre_save().

We probably also need to make sure some migration testing gets added to the driver dev

Not sure we want a failure mode for pre_save().

If we go for option (a) (from comment 9), I'd add a check when reading the commands from the ring, not at migration time, so we don't run enter a state where pre_save() can fail in the first place.  Because that will break the windows drivers we might add a warning only for 2.9, then for 2.10 raise an error irq.  Something like this:

--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -639,6 +639,24 @@ static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext)
         qxl->guest_primary.commands++;
         qxl_track_command(qxl, ext);
         qxl_log_command(qxl, "cmd", ext);
+        {
+            void *msg = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id);
+            if (msg < (void *)qxl->vga.vram_ptr ||
+                msg > ((void *)qxl->vga.vram_ptr + qxl->vga.vram_size)) {
+#if 1
+                /* temporary, for 2.9 */
+                static int once;
+                if (!once) {
+                    fprintf(stderr, "qxl: guest bug: command not in ram bar, "
+                            "guest not migratable\n");
+                    once = true;
+                }
+#else
+                qxl_set_guest_bug(qxl, "command not in ram bar");
+                return false;
+#endif
+            }
+        }
         trace_qxl_ring_command_get(qxl->id, qxl_mode_to_string(qxl->mode));
         return true;
     default:


Your approach works ok Gerd with a migration blocker. Are you going to send a patch?

I am afraid we would have to make this code permanent though, since there has been several releases of this driver already, and it's much nicer to block migration than to crash a VM. 

I have reached out to wddm driver about the bug.

Cc: <email address hidden>
Signed-off-by: Gerd Hoffmann <email address hidden>
---
 hw/display/qxl.h |  1 +
 hw/display/qxl.c | 22 ++++++++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/hw/display/qxl.h b/hw/display/qxl.h
index d2d49dd..77e5a36 100644
--- a/hw/display/qxl.h
+++ b/hw/display/qxl.h
@@ -40,6 +40,7 @@ typedef struct PCIQXLDevice {
     uint32_t           cmdlog;
 
     uint32_t           guest_bug;
+    Error              *migration_blocker;
 
     enum qxl_mode      mode;
     uint32_t           cmdflags;
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index c31b293..74ebeb9 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -26,6 +26,7 @@
 #include "qemu/queue.h"
 #include "qemu/atomic.h"
 #include "sysemu/sysemu.h"
+#include "migration/migration.h"
 #include "trace.h"
 
 #include "qxl.h"
@@ -639,6 +640,27 @@ static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext)
         qxl->guest_primary.commands++;
         qxl_track_command(qxl, ext);
         qxl_log_command(qxl, "cmd", ext);
+        {
+            /*
+             * Windows 8 drivers place qxl commands in the vram
+             * (instead of the ram) bar.  We can't live migrate such a
+             * guest, so add a migration blocker in case we detect
+             * this, to avoid triggering the assert in pre_save().
+             *
+             * https://cgit.freedesktop.org/spice/win32/qxl-wddm-dod/commit/?id=f6e099db39e7d0787f294d5fd0dce328b5210faa
+             */
+            void *msg = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id);
+            if (msg != NULL && (
+                    msg < (void *)qxl->vga.vram_ptr ||
+                    msg > ((void *)qxl->vga.vram_ptr + qxl->vga.vram_size))) {
+                if (!qxl->migration_blocker) {
+                    Error *local_err = NULL;
+                    error_setg(&qxl->migration_blocker,
+                               "qxl: guest bug: command not in ram bar");
+                    migrate_add_blocker(qxl->migration_blocker, &local_err);
+                }
+            }
+        }
         trace_qxl_ring_command_get(qxl->id, qxl_mode_to_string(qxl->mode));
         return true;
     default:
-- 
2.9.3



Hi

On Mon, Apr 10, 2017 at 8:58 AM Gerd Hoffmann <email address hidden> wrote:

> Cc: <email address hidden>
> Signed-off-by: Gerd Hoffmann <email address hidden>
> ---
>  hw/display/qxl.h |  1 +
>  hw/display/qxl.c | 22 ++++++++++++++++++++++
>  2 files changed, 23 insertions(+)
>
> diff --git a/hw/display/qxl.h b/hw/display/qxl.h
> index d2d49dd..77e5a36 100644
> --- a/hw/display/qxl.h
> +++ b/hw/display/qxl.h
> @@ -40,6 +40,7 @@ typedef struct PCIQXLDevice {
>      uint32_t           cmdlog;
>
>      uint32_t           guest_bug;
> +    Error              *migration_blocker;
>
>      enum qxl_mode      mode;
>      uint32_t           cmdflags;
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index c31b293..74ebeb9 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -26,6 +26,7 @@
>  #include "qemu/queue.h"
>  #include "qemu/atomic.h"
>  #include "sysemu/sysemu.h"
> +#include "migration/migration.h"
>  #include "trace.h"
>
>  #include "qxl.h"
> @@ -639,6 +640,27 @@ static int interface_get_command(QXLInstance *sin,
> struct QXLCommandExt *ext)
>          qxl->guest_primary.commands++;
>          qxl_track_command(qxl, ext);
>          qxl_log_command(qxl, "cmd", ext);
> +        {
> +            /*
> +             * Windows 8 drivers place qxl commands in the vram
>
+             * (instead of the ram) bar.  We can't live migrate such a
> +             * guest, so add a migration blocker in case we detect
> +             * this, to avoid triggering the assert in pre_save().
> +             *
> +             *
> https://cgit.freedesktop.org/spice/win32/qxl-wddm-dod/commit/?id=f6e099db39e7d0787f294d5fd0dce328b5210faa
> +             */
> +            void *msg = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id);
> +            if (msg != NULL && (
> +                    msg < (void *)qxl->vga.vram_ptr ||
> +                    msg > ((void *)qxl->vga.vram_ptr +
> qxl->vga.vram_size))) {
> +                if (!qxl->migration_blocker) {
> +                    Error *local_err = NULL;
> +                    error_setg(&qxl->migration_blocker,
> +                               "qxl: guest bug: command not in ram bar");
> +                    migrate_add_blocker(qxl->migration_blocker,
> &local_err);
> +                }
>

Should the blocker be removed on reset?

Looks and works ok otherwise


> +            }
> +        }
>          trace_qxl_ring_command_get(qxl->id,
> qxl_mode_to_string(qxl->mode));
>          return true;
>      default:
> --
> 2.9.3
>
> --
Marc-André Lureau


Cc: <email address hidden>
Signed-off-by: Gerd Hoffmann <email address hidden>
---
 hw/display/qxl.h |  1 +
 hw/display/qxl.c | 28 ++++++++++++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/hw/display/qxl.h b/hw/display/qxl.h
index d2d49dd..77e5a36 100644
--- a/hw/display/qxl.h
+++ b/hw/display/qxl.h
@@ -40,6 +40,7 @@ typedef struct PCIQXLDevice {
     uint32_t           cmdlog;
 
     uint32_t           guest_bug;
+    Error              *migration_blocker;
 
     enum qxl_mode      mode;
     uint32_t           cmdflags;
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index c31b293..c1f830c 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -26,6 +26,7 @@
 #include "qemu/queue.h"
 #include "qemu/atomic.h"
 #include "sysemu/sysemu.h"
+#include "migration/migration.h"
 #include "trace.h"
 
 #include "qxl.h"
@@ -639,6 +640,27 @@ static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext)
         qxl->guest_primary.commands++;
         qxl_track_command(qxl, ext);
         qxl_log_command(qxl, "cmd", ext);
+        {
+            /*
+             * Windows 8 drivers place qxl commands in the vram
+             * (instead of the ram) bar.  We can't live migrate such a
+             * guest, so add a migration blocker in case we detect
+             * this, to avoid triggering the assert in pre_save().
+             *
+             * https://cgit.freedesktop.org/spice/win32/qxl-wddm-dod/commit/?id=f6e099db39e7d0787f294d5fd0dce328b5210faa
+             */
+            void *msg = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id);
+            if (msg != NULL && (
+                    msg < (void *)qxl->vga.vram_ptr ||
+                    msg > ((void *)qxl->vga.vram_ptr + qxl->vga.vram_size))) {
+                if (!qxl->migration_blocker) {
+                    Error *local_err = NULL;
+                    error_setg(&qxl->migration_blocker,
+                               "qxl: guest bug: command not in ram bar");
+                    migrate_add_blocker(qxl->migration_blocker, &local_err);
+                }
+            }
+        }
         trace_qxl_ring_command_get(qxl->id, qxl_mode_to_string(qxl->mode));
         return true;
     default:
@@ -1236,6 +1258,12 @@ static void qxl_hard_reset(PCIQXLDevice *d, int loadvm)
     qemu_spice_create_host_memslot(&d->ssd);
     qxl_soft_reset(d);
 
+    if (d->migration_blocker) {
+        migrate_del_blocker(d->migration_blocker);
+        error_free(d->migration_blocker);
+        d->migration_blocker = NULL;
+    }
+
     if (startstop) {
         qemu_spice_display_start();
     }
-- 
2.9.3



Hi

On Mon, Apr 10, 2017 at 12:27 PM Gerd Hoffmann <email address hidden> wrote:

> Cc: <email address hidden>
> Signed-off-by: Gerd Hoffmann <email address hidden>
>
---
>  hw/display/qxl.h |  1 +
>  hw/display/qxl.c | 28 ++++++++++++++++++++++++++++
>  2 files changed, 29 insertions(+)
>
> diff --git a/hw/display/qxl.h b/hw/display/qxl.h
> index d2d49dd..77e5a36 100644
> --- a/hw/display/qxl.h
> +++ b/hw/display/qxl.h
> @@ -40,6 +40,7 @@ typedef struct PCIQXLDevice {
>      uint32_t           cmdlog;
>
>      uint32_t           guest_bug;
> +    Error              *migration_blocker;
>
>      enum qxl_mode      mode;
>      uint32_t           cmdflags;
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index c31b293..c1f830c 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -26,6 +26,7 @@
>  #include "qemu/queue.h"
>  #include "qemu/atomic.h"
>  #include "sysemu/sysemu.h"
> +#include "migration/migration.h"
>  #include "trace.h"
>
>  #include "qxl.h"
> @@ -639,6 +640,27 @@ static int interface_get_command(QXLInstance *sin,
> struct QXLCommandExt *ext)
>          qxl->guest_primary.commands++;
>          qxl_track_command(qxl, ext);
>          qxl_log_command(qxl, "cmd", ext);
> +        {
> +            /*
> +             * Windows 8 drivers place qxl commands in the vram
> +             * (instead of the ram) bar.  We can't live migrate such a
> +             * guest, so add a migration blocker in case we detect
> +             * this, to avoid triggering the assert in pre_save().
> +             *
> +             *
> https://cgit.freedesktop.org/spice/win32/qxl-wddm-dod/commit/?id=f6e099db39e7d0787f294d5fd0dce328b5210faa
> +             */
> +            void *msg = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id);
> +            if (msg != NULL && (
> +                    msg < (void *)qxl->vga.vram_ptr ||
> +                    msg > ((void *)qxl->vga.vram_ptr +
> qxl->vga.vram_size))) {
> +                if (!qxl->migration_blocker) {
> +                    Error *local_err = NULL;
> +                    error_setg(&qxl->migration_blocker,
> +                               "qxl: guest bug: command not in ram bar");
> +                    migrate_add_blocker(qxl->migration_blocker,
> &local_err);
>

What do you do with the local_err? error_report_err() perhaps ?


> +                }
> +            }
> +        }
>          trace_qxl_ring_command_get(qxl->id,
> qxl_mode_to_string(qxl->mode));
>          return true;
>      default:
> @@ -1236,6 +1258,12 @@ static void qxl_hard_reset(PCIQXLDevice *d, int
> loadvm)
>      qemu_spice_create_host_memslot(&d->ssd);
>      qxl_soft_reset(d);
>
> +    if (d->migration_blocker) {
> +        migrate_del_blocker(d->migration_blocker);
> +        error_free(d->migration_blocker);
> +        d->migration_blocker = NULL;
> +    }
> +
>      if (startstop) {
>          qemu_spice_display_start();
>      }
> --
> 2.9.3
>
> --
Marc-André Lureau


  Hi,

> > +                if (!qxl->migration_blocker) {
> > +                    Error *local_err = NULL;
> > +                    error_setg(&qxl->migration_blocker,
> > +                               "qxl: guest bug: command not in ram bar");
> > +                    migrate_add_blocker(qxl->migration_blocker,
> > &local_err);
> >
> 
> What do you do with the local_err? error_report_err() perhaps ?

We can't error out at that point, unlike most migration blockers this
isn't added at device initialization time.

So, yes, we could error_report it, but the message would end up in the
logfile unnoticed, so I'm not sure how useful that is ...

cheers,
  Gerd



Hi

On Mon, Apr 10, 2017 at 12:51 PM Gerd Hoffmann <email address hidden> wrote:

>   Hi,
>
> > > +                if (!qxl->migration_blocker) {
> > > +                    Error *local_err = NULL;
> > > +                    error_setg(&qxl->migration_blocker,
> > > +                               "qxl: guest bug: command not in ram
> bar");
> > > +                    migrate_add_blocker(qxl->migration_blocker,
> > > &local_err);
> > >
> >
> > What do you do with the local_err? error_report_err() perhaps ?
>
> We can't error out at that point, unlike most migration blockers this
> isn't added at device initialization time.
>
> So, yes, we could error_report it, but the message would end up in the
> logfile unnoticed, so I'm not sure how useful that is ...
>

Well, it may eventually be read if something breaks. Otherwise, you may
just pass a NULL pointer, no?

thanks
-- 
Marc-André Lureau


Cc: <email address hidden>
Signed-off-by: Gerd Hoffmann <email address hidden>
---
 hw/display/qxl.h |  1 +
 hw/display/qxl.c | 31 +++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/hw/display/qxl.h b/hw/display/qxl.h
index d2d49dd..77e5a36 100644
--- a/hw/display/qxl.h
+++ b/hw/display/qxl.h
@@ -40,6 +40,7 @@ typedef struct PCIQXLDevice {
     uint32_t           cmdlog;
 
     uint32_t           guest_bug;
+    Error              *migration_blocker;
 
     enum qxl_mode      mode;
     uint32_t           cmdflags;
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index c31b293..9feae78 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -26,6 +26,7 @@
 #include "qemu/queue.h"
 #include "qemu/atomic.h"
 #include "sysemu/sysemu.h"
+#include "migration/migration.h"
 #include "trace.h"
 
 #include "qxl.h"
@@ -639,6 +640,30 @@ static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext)
         qxl->guest_primary.commands++;
         qxl_track_command(qxl, ext);
         qxl_log_command(qxl, "cmd", ext);
+        {
+            /*
+             * Windows 8 drivers place qxl commands in the vram
+             * (instead of the ram) bar.  We can't live migrate such a
+             * guest, so add a migration blocker in case we detect
+             * this, to avoid triggering the assert in pre_save().
+             *
+             * https://cgit.freedesktop.org/spice/win32/qxl-wddm-dod/commit/?id=f6e099db39e7d0787f294d5fd0dce328b5210faa
+             */
+            void *msg = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id);
+            if (msg != NULL && (
+                    msg < (void *)qxl->vga.vram_ptr ||
+                    msg > ((void *)qxl->vga.vram_ptr + qxl->vga.vram_size))) {
+                if (!qxl->migration_blocker) {
+                    Error *local_err = NULL;
+                    error_setg(&qxl->migration_blocker,
+                               "qxl: guest bug: command not in ram bar");
+                    migrate_add_blocker(qxl->migration_blocker, &local_err);
+                    if (local_err) {
+                        error_report_err(local_err);
+                    }
+                }
+            }
+        }
         trace_qxl_ring_command_get(qxl->id, qxl_mode_to_string(qxl->mode));
         return true;
     default:
@@ -1236,6 +1261,12 @@ static void qxl_hard_reset(PCIQXLDevice *d, int loadvm)
     qemu_spice_create_host_memslot(&d->ssd);
     qxl_soft_reset(d);
 
+    if (d->migration_blocker) {
+        migrate_del_blocker(d->migration_blocker);
+        error_free(d->migration_blocker);
+        d->migration_blocker = NULL;
+    }
+
     if (startstop) {
         qemu_spice_display_start();
     }
-- 
2.9.3



Is this problem limited to commands or also to data attached to the commands?
To me looks like a limitation Qemu should remove on the long run.

Hi

On Mon, Apr 10, 2017 at 1:31 PM Gerd Hoffmann <email address hidden> wrote:

> Cc: <email address hidden>
> Signed-off-by: Gerd Hoffmann <email address hidden>
>


Reviewed-by: Marc-André Lureau <email address hidden>



> ---
>  hw/display/qxl.h |  1 +
>  hw/display/qxl.c | 31 +++++++++++++++++++++++++++++++
>  2 files changed, 32 insertions(+)
>
> diff --git a/hw/display/qxl.h b/hw/display/qxl.h
> index d2d49dd..77e5a36 100644
> --- a/hw/display/qxl.h
> +++ b/hw/display/qxl.h
> @@ -40,6 +40,7 @@ typedef struct PCIQXLDevice {
>      uint32_t           cmdlog;
>
>      uint32_t           guest_bug;
> +    Error              *migration_blocker;
>
>      enum qxl_mode      mode;
>      uint32_t           cmdflags;
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index c31b293..9feae78 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -26,6 +26,7 @@
>  #include "qemu/queue.h"
>  #include "qemu/atomic.h"
>  #include "sysemu/sysemu.h"
> +#include "migration/migration.h"
>  #include "trace.h"
>
>  #include "qxl.h"
> @@ -639,6 +640,30 @@ static int interface_get_command(QXLInstance *sin,
> struct QXLCommandExt *ext)
>          qxl->guest_primary.commands++;
>          qxl_track_command(qxl, ext);
>          qxl_log_command(qxl, "cmd", ext);
> +        {
> +            /*
> +             * Windows 8 drivers place qxl commands in the vram
> +             * (instead of the ram) bar.  We can't live migrate such a
> +             * guest, so add a migration blocker in case we detect
> +             * this, to avoid triggering the assert in pre_save().
> +             *
> +             *
> https://cgit.freedesktop.org/spice/win32/qxl-wddm-dod/commit/?id=f6e099db39e7d0787f294d5fd0dce328b5210faa
> +             */
> +            void *msg = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id);
> +            if (msg != NULL && (
> +                    msg < (void *)qxl->vga.vram_ptr ||
> +                    msg > ((void *)qxl->vga.vram_ptr +
> qxl->vga.vram_size))) {
> +                if (!qxl->migration_blocker) {
> +                    Error *local_err = NULL;
> +                    error_setg(&qxl->migration_blocker,
> +                               "qxl: guest bug: command not in ram bar");
> +                    migrate_add_blocker(qxl->migration_blocker,
> &local_err);
> +                    if (local_err) {
> +                        error_report_err(local_err);
> +                    }
> +                }
> +            }
> +        }
>          trace_qxl_ring_command_get(qxl->id,
> qxl_mode_to_string(qxl->mode));
>          return true;
>      default:
> @@ -1236,6 +1261,12 @@ static void qxl_hard_reset(PCIQXLDevice *d, int
> loadvm)
>      qemu_spice_create_host_memslot(&d->ssd);
>      qxl_soft_reset(d);
>
> +    if (d->migration_blocker) {
> +        migrate_del_blocker(d->migration_blocker);
> +        error_free(d->migration_blocker);
> +        d->migration_blocker = NULL;
> +    }
> +
>      if (startstop) {
>          qemu_spice_display_start();
>      }
> --
> 2.9.3
>
> --
Marc-André Lureau


On Mo, 2017-04-10 at 11:56 +0000, Frediano Ziglio wrote:
> Is this problem limited to commands or also to data attached to the commands?

Everything which contains QXLReleaseInfo and is released via release
ring.

> To me looks like a limitation Qemu should remove on the long run.

That is an option, but it is tricky backward-compatibility wise.
What was the reason to move the commands to bar1?

cheers,
  Gerd



Cc: <email address hidden>
Signed-off-by: Gerd Hoffmann <email address hidden>
Reviewed-by: Marc-André Lureau <email address hidden>
Message-id: <email address hidden>
---
 hw/display/qxl.h |  1 +
 hw/display/qxl.c | 31 +++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/hw/display/qxl.h b/hw/display/qxl.h
index d2d49dd..77e5a36 100644
--- a/hw/display/qxl.h
+++ b/hw/display/qxl.h
@@ -40,6 +40,7 @@ typedef struct PCIQXLDevice {
     uint32_t           cmdlog;
 
     uint32_t           guest_bug;
+    Error              *migration_blocker;
 
     enum qxl_mode      mode;
     uint32_t           cmdflags;
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index c31b293..9feae78 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -26,6 +26,7 @@
 #include "qemu/queue.h"
 #include "qemu/atomic.h"
 #include "sysemu/sysemu.h"
+#include "migration/migration.h"
 #include "trace.h"
 
 #include "qxl.h"
@@ -639,6 +640,30 @@ static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext)
         qxl->guest_primary.commands++;
         qxl_track_command(qxl, ext);
         qxl_log_command(qxl, "cmd", ext);
+        {
+            /*
+             * Windows 8 drivers place qxl commands in the vram
+             * (instead of the ram) bar.  We can't live migrate such a
+             * guest, so add a migration blocker in case we detect
+             * this, to avoid triggering the assert in pre_save().
+             *
+             * https://cgit.freedesktop.org/spice/win32/qxl-wddm-dod/commit/?id=f6e099db39e7d0787f294d5fd0dce328b5210faa
+             */
+            void *msg = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id);
+            if (msg != NULL && (
+                    msg < (void *)qxl->vga.vram_ptr ||
+                    msg > ((void *)qxl->vga.vram_ptr + qxl->vga.vram_size))) {
+                if (!qxl->migration_blocker) {
+                    Error *local_err = NULL;
+                    error_setg(&qxl->migration_blocker,
+                               "qxl: guest bug: command not in ram bar");
+                    migrate_add_blocker(qxl->migration_blocker, &local_err);
+                    if (local_err) {
+                        error_report_err(local_err);
+                    }
+                }
+            }
+        }
         trace_qxl_ring_command_get(qxl->id, qxl_mode_to_string(qxl->mode));
         return true;
     default:
@@ -1236,6 +1261,12 @@ static void qxl_hard_reset(PCIQXLDevice *d, int loadvm)
     qemu_spice_create_host_memslot(&d->ssd);
     qxl_soft_reset(d);
 
+    if (d->migration_blocker) {
+        migrate_del_blocker(d->migration_blocker);
+        error_free(d->migration_blocker);
+        d->migration_blocker = NULL;
+    }
+
     if (startstop) {
         qemu_spice_display_start();
     }
-- 
2.9.3



Did I miss something or this is a bug in the windows qxl driver and should be fixed there?

Will be fixed in the windows driver, yes.

But throwing core dumps isn't exactly nice, even in case the guest is buggy, thats why the qemu workaround, so we simply refuse to live-migrate instead of crashing.

Next version of the driver will solve the problem (already fixed in master).

Similar issue, seems not caused by save/restore/migration but is still detecting offset problems with resource deallocation.
See https://lists.freedesktop.org/archives/spice-devel/2017-April/037248.html.

Still working on some updates for the driver.

wddm dod 0.17 version released which fixes the issue guest side.

Patch had been merged here:
https://git.qemu.org/?p=qemu.git;a=commitdiff;h=86dbcdd9c7590d06db89ca
... thus closing this ticket now.