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

login
register
mail settings
Submitter Alon Levy
Date Oct. 31, 2012, 12:53 p.m.
Message ID <1351688023-18991-1-git-send-email-alevy@redhat.com>
Download mbox | patch
Permalink /patch/195865/
State New
Headers show

Comments

Alon Levy - Oct. 31, 2012, 12:53 p.m.
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(-)
Gerd Hoffmann - Nov. 1, 2012, 9:19 a.m.
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
Alon Levy - Nov. 1, 2012, 9:45 a.m.
> 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
> 
> 
>
Gerd Hoffmann - Nov. 1, 2012, 9:55 a.m.
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

Patch

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;