* [Qemu-devel] [RFC] hw/qxl: inject interrupts in any state @ 2012-10-31 12:53 Alon Levy 2012-11-01 9:19 ` Gerd Hoffmann 0 siblings, 1 reply; 10+ messages in thread From: Alon Levy @ 2012-10-31 12:53 UTC (permalink / raw) To: qemu-devel, kraxel 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [RFC] hw/qxl: inject interrupts in any state 2012-10-31 12:53 [Qemu-devel] [RFC] hw/qxl: inject interrupts in any state Alon Levy @ 2012-11-01 9:19 ` Gerd Hoffmann 2012-11-01 9:45 ` Alon Levy 0 siblings, 1 reply; 10+ messages in thread From: Gerd Hoffmann @ 2012-11-01 9:19 UTC (permalink / raw) To: Alon Levy; +Cc: qemu-devel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [RFC] hw/qxl: inject interrupts in any state 2012-11-01 9:19 ` Gerd Hoffmann @ 2012-11-01 9:45 ` Alon Levy 2012-11-01 9:55 ` Gerd Hoffmann 0 siblings, 1 reply; 10+ messages in thread From: Alon Levy @ 2012-11-01 9:45 UTC (permalink / raw) To: Gerd Hoffmann; +Cc: qemu-devel > 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 > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [RFC] hw/qxl: inject interrupts in any state 2012-11-01 9:45 ` Alon Levy @ 2012-11-01 9:55 ` Gerd Hoffmann 2012-11-01 10:22 ` Alon Levy 0 siblings, 1 reply; 10+ messages in thread From: Gerd Hoffmann @ 2012-11-01 9:55 UTC (permalink / raw) To: Alon Levy; +Cc: qemu-devel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [RFC] hw/qxl: inject interrupts in any state 2012-11-01 9:55 ` Gerd Hoffmann @ 2012-11-01 10:22 ` Alon Levy 2012-11-01 10:33 ` Gerd Hoffmann 0 siblings, 1 reply; 10+ messages in thread From: Alon Levy @ 2012-11-01 10:22 UTC (permalink / raw) To: Gerd Hoffmann; +Cc: qemu-devel > 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 > > > > ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [RFC] hw/qxl: inject interrupts in any state 2012-11-01 10:22 ` Alon Levy @ 2012-11-01 10:33 ` Gerd Hoffmann 2012-11-01 11:48 ` Alon Levy 0 siblings, 1 reply; 10+ messages in thread From: Gerd Hoffmann @ 2012-11-01 10:33 UTC (permalink / raw) To: Alon Levy; +Cc: qemu-devel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [RFC] hw/qxl: inject interrupts in any state 2012-11-01 10:33 ` Gerd Hoffmann @ 2012-11-01 11:48 ` Alon Levy 2012-11-01 12:32 ` Gerd Hoffmann 0 siblings, 1 reply; 10+ messages in thread From: Alon Levy @ 2012-11-01 11:48 UTC (permalink / raw) To: Gerd Hoffmann; +Cc: qemu-devel > 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 > > > ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [RFC] hw/qxl: inject interrupts in any state 2012-11-01 11:48 ` Alon Levy @ 2012-11-01 12:32 ` Gerd Hoffmann 2012-11-01 12:44 ` Alon Levy 0 siblings, 1 reply; 10+ messages in thread From: Gerd Hoffmann @ 2012-11-01 12:32 UTC (permalink / raw) To: Alon Levy; +Cc: qemu-devel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [RFC] hw/qxl: inject interrupts in any state 2012-11-01 12:32 ` Gerd Hoffmann @ 2012-11-01 12:44 ` Alon Levy 2012-11-01 12:47 ` Gerd Hoffmann 0 siblings, 1 reply; 10+ messages in thread From: Alon Levy @ 2012-11-01 12:44 UTC (permalink / raw) To: Gerd Hoffmann; +Cc: qemu-devel > 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" ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [RFC] hw/qxl: inject interrupts in any state 2012-11-01 12:44 ` Alon Levy @ 2012-11-01 12:47 ` Gerd Hoffmann 0 siblings, 0 replies; 10+ messages in thread From: Gerd Hoffmann @ 2012-11-01 12:47 UTC (permalink / raw) To: Alon Levy; +Cc: qemu-devel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2012-11-01 12:48 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-10-31 12:53 [Qemu-devel] [RFC] hw/qxl: inject interrupts in any state Alon Levy 2012-11-01 9:19 ` Gerd Hoffmann 2012-11-01 9:45 ` Alon Levy 2012-11-01 9:55 ` Gerd Hoffmann 2012-11-01 10:22 ` Alon Levy 2012-11-01 10:33 ` Gerd Hoffmann 2012-11-01 11:48 ` Alon Levy 2012-11-01 12:32 ` Gerd Hoffmann 2012-11-01 12:44 ` Alon Levy 2012-11-01 12:47 ` Gerd Hoffmann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).