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

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

[Alon Levy]

I cannot find a reason we asserted that injecting interrupts happen only
when the vm is running. This is right now the cause of spice crashing
due to the new interface_client_set_capabilities being called when the
vm is stopped, this happens if a user stops the vm or the vm reboots and
a spice connection is dropped / created meanwhile.

Sending as RFC since I'm not sure what the original reason for the
assert is, git history is no help, it's in the first commit.

RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=870972

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/qxl.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index 7b88a1e..ea92f83 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -1739,7 +1739,6 @@ static void qxl_send_events(PCIQXLDevice *d, uint32_t events)
     uint32_t le_events = cpu_to_le32(events);
 
     trace_qxl_send_events(d->id, events);
-    assert(qemu_spice_display_is_running(&d->ssd));
     old_pending = __sync_fetch_and_or(&d->ram->int_pending, le_events);
     if ((old_pending & le_events) == le_events) {
         return;
-- 
1.7.12.1

Re: [Qemu-devel] [RFC] hw/qxl: inject interrupts in any state

[Gerd Hoffmann]

On 10/31/12 13:53, Alon Levy wrote:
> I cannot find a reason we asserted that injecting interrupts happen only
> when the vm is running. This is right now the cause of spice crashing
> due to the new interface_client_set_capabilities being called when the
> vm is stopped, this happens if a user stops the vm or the vm reboots and
> a spice connection is dropped / created meanwhile.
> 
> Sending as RFC since I'm not sure what the original reason for the
> assert is, git history is no help, it's in the first commit.

The problem is migration.  qxl_send_events modifies guest state, and
there are phases during migration where modifying guest state is a
no-go.  You'll loose events, either because the guest state update on
the source side came to late so it isn't send over or because the update
on the target side came to early so loadvm will overwrite it.

Disallowing events when the vm is stopped catches more cases than
needed, we could change it.  But that wouldn't fix the bug at hand, it
would only make it harder to trigger.

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 ...

cheers,
  Gerd

Re: [Qemu-devel] [RFC] hw/qxl: inject interrupts in any state

[Alon Levy]

> On 10/31/12 13:53, Alon Levy wrote:
> > I cannot find a reason we asserted that injecting interrupts happen
> > only
> > when the vm is running. This is right now the cause of spice
> > crashing
> > due to the new interface_client_set_capabilities being called when
> > the
> > vm is stopped, this happens if a user stops the vm or the vm
> > reboots and
> > a spice connection is dropped / created meanwhile.
> > 
> > Sending as RFC since I'm not sure what the original reason for the
> > assert is, git history is no help, it's in the first commit.
> 
> The problem is migration.  qxl_send_events modifies guest state, and
> there are phases during migration where modifying guest state is a
> no-go.  You'll loose events, either because the guest state update on
> the source side came to late so it isn't send over or because the
> update
> on the target side came to early so loadvm will overwrite it.
> 
> Disallowing events when the vm is stopped catches more cases than
> needed, we could change it.  But that wouldn't fix the bug at hand,
> it
> would only make it harder to trigger.
> 
> 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.

> 
> cheers,
>   Gerd
> 
> 
> 

Re: [Qemu-devel] [RFC] hw/qxl: inject interrupts in any state

[Gerd Hoffmann]

  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 ...

cheers,
  Gerd

Re: [Qemu-devel] [RFC] hw/qxl: inject interrupts in any state

[Alon Levy]

> 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>

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));



> 
> cheers,
>   Gerd
> 
> 
> 
> 

Re: [Qemu-devel] [RFC] hw/qxl: inject interrupts in any state

[Gerd Hoffmann]

  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

Re: [Qemu-devel] [RFC] hw/qxl: inject interrupts in any state

[Alon Levy]

> 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?

To point to the faulty function without resorting to the stack. But I guess we can always get that from systemtap if I add a trace event there.

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

    hw/qxl: qxl_send_events: nop if stopped
    
    Added a trace point for easy logging.
    
    RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=870972
    
    Signed-off-by: Alon Levy <alevy@redhat.com>

diff --git a/hw/qxl.c b/hw/qxl.c
index 7b88a1e..e86e70c 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -1739,7 +1739,11 @@ static void qxl_send_events(PCIQXLDevice *d, uint32_t events)
     uint32_t le_events = cpu_to_le32(events);
 
     trace_qxl_send_events(d->id, events);
-    assert(qemu_spice_display_is_running(&d->ssd));
+    if (!qemu_spice_display_is_running(&d->ssd)) {
+        fprintf(stderr, "%s: guest stopped, ignoring\n", __func__);
+        trace_qxl_send_events_vm_stopped(d->id, events);
+        return;
+    }
     old_pending = __sync_fetch_and_or(&d->ram->int_pending, le_events);
     if ((old_pending & le_events) == le_events) {
         return;
diff --git a/trace-events b/trace-events
index 7ee21e5..d308533 100644
--- a/trace-events
+++ b/trace-events
@@ -994,6 +994,7 @@ qxl_spice_update_area(int qid, uint32_t surface_id, uint32_t left, uint32_t righ
 qxl_spice_update_area_rest(int qid, uint32_t num_dirty_rects, uint32_t clear_dirty_region) "%d #d=%d clear=%d"
 qxl_surfaces_dirty(int qid, int surface, int offset, int size) "%d surface=%d offset=%d size=%d"
 qxl_send_events(int qid, uint32_t events) "%d %d"
+qxl_send_events_vm_stopped(int qid, uint32_t events) "%d %d"
 qxl_set_guest_bug(int qid) "%d"
 qxl_interrupt_client_monitors_config(int qid, int num_heads, void *heads) "%d %d %p"
 qxl_client_monitors_config_unsupported_by_guest(int qid, uint32_t int_mask, void *client_monitors_config) "%d %X %p"


> 
> 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
> 
> 
> 

Re: [Qemu-devel] [RFC] hw/qxl: inject interrupts in any state

[Gerd Hoffmann]

On 11/01/12 12:48, Alon Levy wrote:
> +    if (!qemu_spice_display_is_running(&d->ssd)) {
> +        fprintf(stderr, "%s: guest stopped, ignoring\n", __func__);
> +        trace_qxl_send_events_vm_stopped(d->id, events);
> +        return;
> +    }

Almost perfect.  We should add a note here that this happening is a
spice server bug, at least as source code comment, maybe also in the
fprintf.

cheers,
  Gerd

Re: [Qemu-devel] [RFC] hw/qxl: inject interrupts in any state

[Alon Levy]

> On 11/01/12 12:48, Alon Levy wrote:
> > +    if (!qemu_spice_display_is_running(&d->ssd)) {
> > +        fprintf(stderr, "%s: guest stopped, ignoring\n",
> > __func__);
> > +        trace_qxl_send_events_vm_stopped(d->id, events);
> > +        return;
> > +    }
> 
> Almost perfect.  We should add a note here that this happening is a
> spice server bug, at least as source code comment, maybe also in the
> fprintf.
> 
> cheers,
>   Gerd
> 


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

    hw/qxl: qxl_send_events: nop if stopped
    
    Added a trace point for easy logging.
    
    RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=870972
    
    Signed-off-by: Alon Levy <alevy@redhat.com>

diff --git a/hw/qxl.c b/hw/qxl.c
index 7b88a1e..c33dd03 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -1739,7 +1739,13 @@ static void qxl_send_events(PCIQXLDevice *d, uint32_t events)
     uint32_t le_events = cpu_to_le32(events);
 
     trace_qxl_send_events(d->id, events);
-    assert(qemu_spice_display_is_running(&d->ssd));
+    if (!qemu_spice_display_is_running(&d->ssd)) {
+        /* spice-server tracks guest running state and should not do this */
+        fprintf(stderr, "%s: spice-server bug: guest stopped, ignoring\n",
+                __func__);
+        trace_qxl_send_events_vm_stopped(d->id, events);
+        return;
+    }
     old_pending = __sync_fetch_and_or(&d->ram->int_pending, le_events);
     if ((old_pending & le_events) == le_events) {
         return;
diff --git a/trace-events b/trace-events
index 7ee21e5..d308533 100644
--- a/trace-events
+++ b/trace-events
@@ -994,6 +994,7 @@ qxl_spice_update_area(int qid, uint32_t surface_id, uint32_t left, uint32_t righ
 qxl_spice_update_area_rest(int qid, uint32_t num_dirty_rects, uint32_t clear_dirty_region) "%d #d=%d clear=%d"
 qxl_surfaces_dirty(int qid, int surface, int offset, int size) "%d surface=%d offset=%d size=%d"
 qxl_send_events(int qid, uint32_t events) "%d %d"
+qxl_send_events_vm_stopped(int qid, uint32_t events) "%d %d"
 qxl_set_guest_bug(int qid) "%d"
 qxl_interrupt_client_monitors_config(int qid, int num_heads, void *heads) "%d %d %p"
 qxl_client_monitors_config_unsupported_by_guest(int qid, uint32_t int_mask, void *client_monitors_config) "%d %X %p"

Re: [Qemu-devel] [RFC] hw/qxl: inject interrupts in any state

[Gerd Hoffmann]

On 11/01/12 13:44, Alon Levy wrote:
>> On 11/01/12 12:48, Alon Levy wrote:
>>> +    if (!qemu_spice_display_is_running(&d->ssd)) {
>>> +        fprintf(stderr, "%s: guest stopped, ignoring\n",
>>> __func__);
>>> +        trace_qxl_send_events_vm_stopped(d->id, events);
>>> +        return;
>>> +    }
>>
>> Almost perfect.  We should add a note here that this happening is a
>> spice server bug, at least as source code comment, maybe also in the
>> fprintf.
>>
>> cheers,
>>   Gerd
>>
> 
> 
> commit 49072da137e3d2d86449082e152b0717e5e94672
> Author: Alon Levy <alevy@redhat.com>
> Date:   Tue Oct 30 18:00:33 2012 +0200
> 
>     hw/qxl: qxl_send_events: nop if stopped

Now I need that in 'git send-email' format so I can 'git am' it.  Or
push to git://people.freedesktop.org/~alon/qemu so I can 'git
cherry-pick' from there.

thanks,
  Gerd