diff mbox

[RFC] hw/qxl: inject interrupts in any state

Message ID 833806772.25295125.1351765330902.JavaMail.root@redhat.com
State New
Headers show

Commit Message

Alon Levy Nov. 1, 2012, 10:22 a.m. UTC 
> Hi,
> 
> >> IMO spice-server must not call interface_client_set_capabilities
> >> when the vm is not running.  After all we notify spice-server
> >> about
> >> the vm stop/start events for a reason ...
> > 
> > OK, I agree that should be fixed, we can queue this until the vm
> > starts running in spice-server. But having an assert on notify in
> > qemu is also wrong - and the only way to fix it like you pointed
> > out
> > without dropping the event is to queue it as well.
> > 
> > So which will it be, queue in spice or in qemu? qemu seems a
> > simpler
> > place to catch everything.
> 
> When queuing in qemu you are facing the migration issue again in a
> different way:  Just this time it isn't guest state, but a qxl
> register.
>  Not guest visible, but still state which must be migrated over ...

OK, good point. So the assert still bothers me - it should be logged but not asserted. I'm talking about interface_set_client_capabilities and anywhere else that qxl_send_events can be called.

i.e.:

commit 6614a4db6b88887dd29bfd5365f38ba0fcc264ed
Author: Alon Levy <alevy@redhat.com>
Date:   Tue Oct 30 18:00:33 2012 +0200

    hw/qxl: warn on qxl_send_events attempt if stopped
    
    This prevents a known abort on set_client_capabilities, that should be
    fixed in upstream, but it should also be checked against in qxl. Checks
    every other location that qxl_send_events is eventually possibly called
    (everywhere is direct except for interface_release_resource which calls
    qxl_push_free_res which may call qxl_send_event).
    
    RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=870972
    
    Signed-off-by: Alon Levy <alevy@redhat.com>




> 
> cheers,
>   Gerd
> 
> 
> 
>

Comments

Gerd Hoffmann Nov. 1, 2012, 10:33 a.m. UTC | #1 
Hi,

>     This prevents a known abort on set_client_capabilities, that should be
>     fixed in upstream, but it should also be checked against in qxl. Checks
>     every other location that qxl_send_events is eventually possibly called

Why check in all callers instead of qxl_send_events directly?

Just print the warning about the spice server bug & possibly lost events
and go on.  With luck the event arrives nevertheless.  And if not we at
least sayed before it can happen ;)

> +static void spice_server_bug(PCIQXLDevice *qxl, const char *msg, ...)
> +{
> +    va_list ap;
> +    va_start(ap, msg);
> +    fprintf(stderr, "qxl-%d: spice-server bug: ", qxl->id);
> +    vfprintf(stderr, msg, ap);
> +    fprintf(stderr, "\n");
> +    va_end(ap);
> +}
> +
> +#define SPICE_SERVER_BUG_ONCE(qxl, msg, ...) {      \
> +    static int called;                              \
> +    if (!called) {                                  \
> +        called = 1;                                 \
> +        spice_server_bug(qxl, msg, __VA_ARGS__);    \
> +    }                                               \
> +}

That feels a bit like overkill too.

cheers,
  Gerd
diff mbox

Patch

diff --git a/hw/qxl.c b/hw/qxl.c
index 7b88a1e..946f5a2 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -136,6 +136,24 @@  static void qxl_reset_memslots(PCIQXLDevice *d);
 static void qxl_reset_surfaces(PCIQXLDevice *d);
 static void qxl_ring_set_dirty(PCIQXLDevice *qxl);
 
+static void spice_server_bug(PCIQXLDevice *qxl, const char *msg, ...)
+{
+    va_list ap;
+    va_start(ap, msg);
+    fprintf(stderr, "qxl-%d: spice-server bug: ", qxl->id);
+    vfprintf(stderr, msg, ap);
+    fprintf(stderr, "\n");
+    va_end(ap);
+}
+
+#define SPICE_SERVER_BUG_ONCE(qxl, msg, ...) {      \
+    static int called;                              \
+    if (!called) {                                  \
+        called = 1;                                 \
+        spice_server_bug(qxl, msg, __VA_ARGS__);    \
+    }                                               \
+}
+
 void qxl_set_guest_bug(PCIQXLDevice *qxl, const char *msg, ...)
 {
     trace_qxl_set_guest_bug(qxl->id);
@@ -600,6 +618,10 @@  static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext)
     int notify, ret;
 
     trace_qxl_ring_command_check(qxl->id, qxl_mode_to_string(qxl->mode));
+    if (!qemu_spice_display_is_running(&qxl->ssd)) {
+        SPICE_SERVER_BUG_ONCE(qxl, "%s: guest stopped", __func__);
+        return false;
+    }
 
     switch (qxl->mode) {
     case QXL_MODE_VGA:
@@ -722,6 +744,11 @@  static void interface_release_resource(QXLInstance *sin,
         return;
     }
 
+    if (!qemu_spice_display_is_running(&qxl->ssd)) {
+        SPICE_SERVER_BUG_ONCE(qxl, "%s: guest stopped", __func__);
+        return;
+    }
+
     /*
      * ext->info points into guest-visible memory
      * pci bar 0, $command.release_info
@@ -761,6 +788,10 @@  static int interface_get_cursor_command(QXLInstance *sin, struct QXLCommandExt *
 
     trace_qxl_ring_cursor_check(qxl->id, qxl_mode_to_string(qxl->mode));
 
+    if (!qemu_spice_display_is_running(&qxl->ssd)) {
+        SPICE_SERVER_BUG_ONCE(qxl, "%s: guest stopped", __func__);
+        return false;
+    }
     switch (qxl->mode) {
     case QXL_MODE_COMPAT:
     case QXL_MODE_NATIVE:
@@ -862,6 +893,11 @@  static void interface_async_complete_io(PCIQXLDevice *qxl, QXLCookie *cookie)
                 "qxl: %s: error: current_async = %d != %"
                 PRId64 " = cookie->io\n", __func__, current_async, cookie->io);
     }
+    if (!qemu_spice_display_is_running(&qxl->ssd)) {
+        SPICE_SERVER_BUG_ONCE(qxl, "%s: guest stopped", __func__);
+        return;
+    }
+
     switch (current_async) {
     case QXL_IO_MEMSLOT_ADD_ASYNC:
     case QXL_IO_DESTROY_PRIMARY_ASYNC:
@@ -963,6 +999,10 @@  static void interface_set_client_capabilities(QXLInstance *sin,
         runstate_check(RUN_STATE_POSTMIGRATE)) {
         return;
     }
+    if (!qemu_spice_display_is_running(&qxl->ssd)) {
+        SPICE_SERVER_BUG_ONCE(qxl, "%s: guest stopped", __func__);
+        return;
+    }
 
     qxl->shadow_rom.client_present = client_present;
     memcpy(qxl->shadow_rom.client_capabilities, caps, sizeof(caps));