| Message ID | 1351688023-18991-1-git-send-email-alevy@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
> 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 > > >
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
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;
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(-)