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

[Alon Levy <alevy@redhat.com>]

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