qxl_pre_save assertion failure on vm "save"
Affects | Status | Importance | Assigned to | Milestone | |
---|---|---|---|---|---|
QEMU |
Fix Released
|
Undecided
|
Unassigned |
Bug Description
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_
qemu-system-x86_64: /build/
2016-10-20 11:52:42.713+0000: shutting down
Please let me know what other information would be relevant!
Dr. David Alan Gilbert (dgilbert-h) wrote : | #1 |
DocMAX (docmax) wrote : | #2 |
QXL driver version is 17.54.59.923
Commandline (git compiled today) is:
/usr/sbin/
DocMAX (docmax) wrote : | #3 |
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://
Those provide something, which lets my window resize freely.
rubenvb (vanboxem-ruben-deactivatedaccount) wrote : | #4 |
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.
LC_ALL=C PATH=/usr/
char device redirected to /dev/pts/6 (label charserial0)
main_channel_link: add main channel client
red_dispatcher_
Dr. David Alan Gilbert (dgilbert-h) wrote : | #5 |
I can see a bunch of similar looking failures in Fedora's automatic backtrace stats system
Gerd Hoffmann (kraxel-redhat) wrote : | #6 |
I put my money on that one:
commit f6e099db39e7d07
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>
elmarco (marcandre-lureau) wrote : | #7 |
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?
elmarco (marcandre-lureau) wrote : | #8 |
damn launchpad, wrong bug and I can't change it back. Please someone move it back to New/Confirmed
Changed in qemu: | |
status: | New → Fix Released |
Changed in qemu: | |
status: | Fix Released → Confirmed |
Gerd Hoffmann (kraxel-redhat) wrote : | #9 |
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.
Dr. David Alan Gilbert (dgilbert-h) wrote : | #10 |
An untested hack that might fail cleaner might be:
error_
qemu_
(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
Gerd Hoffmann (kraxel-redhat) wrote : | #11 |
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_
+ {
+ 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.
+#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_
+ return false;
+#endif
+ }
+ }
return true;
default:
elmarco (marcandre-lureau) wrote : | #12 |
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.
Gerd Hoffmann (kraxel-redhat) wrote : [PATCH] qxl: add migration blocker to avoid pre-save assert | #13 |
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/
#include "trace.h"
#include "qxl.h"
@@ -639,6 +640,27 @@ static int interface_
+ {
+ /*
+ * 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:/
+ */
+ 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.
+ if (!qxl->
+ Error *local_err = NULL;
+ error_setg(
+ "qxl: guest bug: command not in ram bar");
+ migrate_
+ }
+ }
+ }
return true;
default:
--
2.9.3
elmarco (marcandre-lureau) wrote : | #14 |
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/
> #include "trace.h"
>
> #include "qxl.h"
> @@ -639,6 +640,27 @@ static int interface_
> struct QXLCommandExt *ext)
> qxl->guest_
> qxl_track_
> qxl_log_
> + {
> + /*
> + * 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:/
> + */
> + 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.
> + if (!qxl->
> + Error *local_err = NULL;
> + error_setg(
> + "qxl: guest bug: command not in ram bar");
> + migrate_
> &local_err);
> + }
>
Should the blocker be removed on reset?
Looks and works ok otherwise
> + }
> + }
> trace_qxl_
> qxl_mode_
> return true;
> default:
> --
> 2.9.3
>
> --
Marc-André Lureau
Gerd Hoffmann (kraxel-redhat) wrote : [PATCH v2] qxl: add migration blocker to avoid pre-save assert | #15 |
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/
#include "trace.h"
#include "qxl.h"
@@ -639,6 +640,27 @@ static int interface_
+ {
+ /*
+ * 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:/
+ */
+ 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.
+ if (!qxl->
+ Error *local_err = NULL;
+ error_setg(
+ "qxl: guest bug: command not in ram bar");
+ migrate_
+ }
+ }
+ }
return true;
default:
@@ -1236,6 +1258,12 @@ static void qxl_hard_
qemu_
qxl_
+ if (d->migration_
+ migrate_
+ error_free(
+ d->migration_
+ }
+
if (startstop) {
}
--
2.9.3
elmarco (marcandre-lureau) wrote : | #16 |
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/
> #include "trace.h"
>
> #include "qxl.h"
> @@ -639,6 +640,27 @@ static int interface_
> struct QXLCommandExt *ext)
> qxl->guest_
> qxl_track_
> qxl_log_
> + {
> + /*
> + * 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:/
> + */
> + 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.
> + if (!qxl->
> + Error *local_err = NULL;
> + error_setg(
> + "qxl: guest bug: command not in ram bar");
> + migrate_
> &local_err);
>
What do you do with the local_err? error_report_err() perhaps ?
> + }
> + }
> + }
> trace_qxl_
> qxl_mode_
> return true;
> default:
> @@ -1236,6 +1258,12 @@ static void qxl_hard_
> loadvm)
> qemu_spice_
> qxl_soft_reset(d);
>
> + if (d->migration_
> + migrate_
> + error_free(
> + d->migration_
> + }
> +
> if (startstop) {
> qemu_spice_
> }
> --
> 2.9.3
>
> --
Marc-André Lureau
Gerd Hoffmann (kraxel-redhat) wrote : Re: [Qemu-devel] [PATCH v2] qxl: add migration blocker to avoid pre-save assert | #17 |
Hi,
> > + if (!qxl->
> > + Error *local_err = NULL;
> > + error_setg(
> > + "qxl: guest bug: command not in ram bar");
> > + migrate_
> > &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
elmarco (marcandre-lureau) wrote : | #18 |
Hi
On Mon, Apr 10, 2017 at 12:51 PM Gerd Hoffmann <email address hidden> wrote:
> Hi,
>
> > > + if (!qxl->
> > > + Error *local_err = NULL;
> > > + error_setg(
> > > + "qxl: guest bug: command not in ram
> bar");
> > > + migrate_
> > > &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
Gerd Hoffmann (kraxel-redhat) wrote : [PATCH v3] qxl: add migration blocker to avoid pre-save assert | #19 |
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/
#include "trace.h"
#include "qxl.h"
@@ -639,6 +640,30 @@ static int interface_
+ {
+ /*
+ * 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:/
+ */
+ 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.
+ if (!qxl->
+ Error *local_err = NULL;
+ error_setg(
+ "qxl: guest bug: command not in ram bar");
+ migrate_
+ if (local_err) {
+ error_report_
+ }
+ }
+ }
+ }
return true;
default:
@@ -1236,6 +1261,12 @@ static void qxl_hard_
qemu_
qxl_
+ if (d->migration_
+ migrate_
+ error_free(
+ d->migration_
+ }
+
if (startstop) {
}
--
2.9.3
Frediano Ziglio (freddy77) wrote : | #20 |
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.
elmarco (marcandre-lureau) wrote : | #21 |
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/
> #include "trace.h"
>
> #include "qxl.h"
> @@ -639,6 +640,30 @@ static int interface_
> struct QXLCommandExt *ext)
> qxl->guest_
> qxl_track_
> qxl_log_
> + {
> + /*
> + * 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:/
> + */
> + 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.
> + if (!qxl->
> + Error *local_err = NULL;
> + error_setg(
> + "qxl: guest bug: command not in ram bar");
> + migrate_
> &local_err);
> + if (local_err) {
> + error_report_
> + }
> + }
> + }
> + }
> trace_qxl_
> qxl_mode_
> return true;
> default:
> @@ -1236,6 +1261,12 @@ static void qxl_hard_
> loadvm)
> qemu_spice_
> qxl_soft_reset(d);
>
> + if (d->migration_
> + migrate_
> + error_free(
> + d->migration_
> + }
> +
> if (startstop) {
> qemu_spice_
> }
> --
> 2.9.3
>
> --
Marc-André Lureau
Gerd Hoffmann (kraxel-redhat) wrote : Re: [Bug 1635339] Re: qxl_pre_save assertion failure on vm "save" | #22 |
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-
What was the reason to move the commands to bar1?
cheers,
Gerd
Gerd Hoffmann (kraxel-redhat) wrote : [PULL 2/2] qxl: add migration blocker to avoid pre-save assert | #23 |
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/
#include "trace.h"
#include "qxl.h"
@@ -639,6 +640,30 @@ static int interface_
+ {
+ /*
+ * 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:/
+ */
+ 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.
+ if (!qxl->
+ Error *local_err = NULL;
+ error_setg(
+ "qxl: guest bug: command not in ram bar");
+ migrate_
+ if (local_err) {
+ error_report_
+ }
+ }
+ }
+ }
return true;
default:
@@ -1236,6 +1261,12 @@ static void qxl_hard_
qemu_
qxl_
+ if (d->migration_
+ migrate_
+ error_free(
+ d->migration_
+ }
+
if (startstop) {
}
--
2.9.3
Marc Pignat (swid) wrote : | #24 |
Did I miss something or this is a bug in the windows qxl driver and should be fixed there?
Gerd Hoffmann (kraxel-redhat) wrote : | #25 |
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.
Frediano Ziglio (freddy77) wrote : | #26 |
Next version of the driver will solve the problem (already fixed in master).
Frediano Ziglio (freddy77) wrote : | #27 |
Similar issue, seems not caused by save/restore/
See https:/
Still working on some updates for the driver.
Frediano Ziglio (freddy77) wrote : | #28 |
wddm dod 0.17 version released which fixes the issue guest side.
Thomas Huth (th-huth) wrote : | #29 |
Patch had been merged here:
https:/
... thus closing this ticket now.
Changed in qemu: | |
status: | Confirmed → Fix Released |
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.