Re: [Qemu-devel] [PATCH 3/3] spice-qemu-char: register interface on post load

[Amit Shah]

On (Wed) 28 Nov 2012 [12:59:40], Markus Armbruster wrote:
> Alon Levy <address@hidden> writes:
> 
> >> Alon Levy <address@hidden> writes:
> >> 
> >> > The target has not seen the guest_connected event via
> >> > spice_chr_guest_open or spice_chr_write, and so spice server
> >> > wrongly
> >> > assumes there is no agent active, while the client continues to
> >> > send
> >> > motion events only by the agent channel, which the server ignores.
> >> > The
> >> > net effect is that the mouse is static in the guest.
> >> >
> >> > By registering the interface on post load spice server will pass on
> >> > the
> >> > agent messages fixing the mouse behavior after migration.
> >> >
> >> > RHBZ #725965
> >> >
> >> > Signed-off-by: Alon Levy <address@hidden>
> >> > ---
> >> >  spice-qemu-char.c | 34 ++++++++++++++++++++++++++++++++++
> >> >  1 file changed, 34 insertions(+)
> >> >
> >> > diff --git a/spice-qemu-char.c b/spice-qemu-char.c
> >> > index 09aa22d..08b6ba0 100644
> >> > --- a/spice-qemu-char.c
> >> > +++ b/spice-qemu-char.c
> >> > @@ -1,6 +1,7 @@
> >> >  #include "config-host.h"
> >> >  #include "trace.h"
> >> >  #include "ui/qemu-spice.h"
> >> > +#include "hw/virtio-serial.h"
> >> >  #include <spice.h>
> >> >  #include <spice-experimental.h>
> >> >  
> >> > @@ -25,6 +26,7 @@ typedef struct SpiceCharDriver {
> >> >      uint8_t               *datapos;
> >> >      ssize_t               bufsize, datalen;
> >> >      uint32_t              debug;
> >> > +    QEMUTimer             *post_load_timer;
> >> >  } SpiceCharDriver;
> >> >  
> >> >  static int vmc_write(SpiceCharDeviceInstance *sin, const uint8_t
> >> >  *buf, int len)
> >> > @@ -156,6 +158,7 @@ static void spice_chr_close(struct
> >> > CharDriverState *chr)
> >> >  
> >> >      printf("%s\n", __func__);
> >> >      vmc_unregister_interface(s);
> >> > +    qemu_free_timer(s->post_load_timer);
> >> >      g_free(s);
> >> >  }
> >> >  
> >> > @@ -188,6 +191,33 @@ static void print_allowed_subtypes(void)
> >> >      fprintf(stderr, "\n");
> >> >  }
> >> >  
> >> > +static void spice_chr_post_load_cb(void *opaque)
> >> > +{
> >> > +    SpiceCharDriver *s = opaque;
> >> > +
> >> > +    vmc_register_interface(s);
> >> > +}
> >> > +
> >> > +static int spice_chr_post_load(void *opaque, int version_id)
> >> > +{
> >> > +    SpiceCharDriver *s = opaque;
> >> > +
> >> > +    if (s && s->chr && qemu_chr_be_connected(s->chr)) {
> >> > +        qemu_mod_timer(s->post_load_timer, 1);
> >> > +    }
> >> 
> >> You use the time to delay spice_chr_post_load_cb(), right?  Can you
> >> explain why you have to delay?
> >
> > This is a precaution, it ensures vmc_register_interface is called when
> > the vm is running as opposed to stopped which is the state when
> > spice_chr_post_load is called. In theory vmc_register_interface could
> > lead to an attempt to inject an interrupt into the guest, and we know
> > that fails with kvm irqchip from a previous bug with virtio-serial.
> 
> So your fixed delay of 1ns (I think) on the vm_clock is a roundabout way
> to delay the callback from "post load" until after the run state
> transition to RUN_STATE_RUNNING.  Correct?
> 
> If yes, then a VM change state handler might be cleaner.

Agreed.  Juan and I had discussed this earlier, and there are no hooks
on the target side (i.e. for loadvm) yet.  So this roundabout way is
the best way to solve the problem for now.

(This discussion with Juan was done earlier, when the patch to
virtio-serial-bus pointed to by Alon in the other message was done).

                Amit