| Message ID | 20170410102746.24889-1-kraxel@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi On Mon, Apr 10, 2017 at 12:27 PM Gerd Hoffmann <kraxel@redhat.com> wrote: > Cc: 1635339@bugs.launchpad.net > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > 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 <kraxel@redhat.com> 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
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(); }
Cc: 1635339@bugs.launchpad.net Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- hw/display/qxl.h | 1 + hw/display/qxl.c | 28 ++++++++++++++++++++++++++++ 2 files changed, 29 insertions(+)