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

[Markus Armbruster]

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.

[...]