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

[Alon Levy <alevy@redhat.com>]

> Alon Levy <alevy@redhat.com> 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 <alevy@redhat.com>
> > ---
> >  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.

> 
> > +    return 0;
> > +}
> > +
> > +static VMStateDescription spice_chr_vmstate = {
> > +    .name               = "spice-chr",
> > +    .version_id         = 1,
> > +    .minimum_version_id = 1,
> > +    .post_load          = spice_chr_post_load,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_END_OF_LIST()
> > +    },
> > +};
> > +
> >  CharDriverState *qemu_chr_open_spice(QemuOpts *opts)
> >  {
> >      CharDriverState *chr;
> > @@ -220,12 +250,16 @@ CharDriverState *qemu_chr_open_spice(QemuOpts
> > *opts)
> >      s->debug = debug;
> >      s->active = false;
> >      s->sin.subtype = subtype;
> > +    s->post_load_timer = qemu_new_timer_ns(vm_clock,
> > +                                           spice_chr_post_load_cb,
> > s);
> >      chr->opaque = s;
> >      chr->chr_write = spice_chr_write;
> >      chr->chr_close = spice_chr_close;
> >      chr->chr_guest_open = spice_chr_guest_open;
> >      chr->chr_guest_close = spice_chr_guest_close;
> >  
> > +    vmstate_register(NULL, -1, &spice_chr_vmstate, s);
> > +
> >  #if SPICE_SERVER_VERSION < 0x000901
> >      /* See comment in vmc_state() */
> >      if (strcmp(subtype, "vdagent") == 0) {
> 
>