Patchwork [3/3] spice-qemu-char: register interface on post load

login
register
mail settings
Submitter Alon Levy
Date Nov. 28, 2012, 9:05 a.m.
Message ID <1354093540-22583-4-git-send-email-alevy@redhat.com>
Download mbox | patch
Permalink /patch/202392/
State New
Headers show

Comments

Alon Levy - Nov. 28, 2012, 9:05 a.m.
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(+)
Markus Armbruster - Nov. 28, 2012, 9:46 a.m.
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?

> +    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) {
Alon Levy - Nov. 28, 2012, 9:51 a.m.
> 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) {
> 
>
Markus Armbruster - Nov. 28, 2012, 11:59 a.m.
Alon Levy <alevy@redhat.com> writes:

>> 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.

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.

[...]
Alon Levy - Nov. 28, 2012, 12:16 p.m.
> Alon Levy <alevy@redhat.com> writes:
> 
> >> 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.
> 
> 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?
Yes.

> 
> If yes, then a VM change state handler might be cleaner.

Not saying that two wrongs make a right, but this is also the way we handled the irq injection in post_load for virtio serial console:

 http://lists.nongnu.org/archive/html/qemu-devel/2012-11/msg01196.html

> 
> [...]
>
Amit Shah - Nov. 29, 2012, 1:08 p.m.
On (Wed) 28 Nov 2012 [12:59:40], Markus Armbruster wrote:
> Alon Levy <alevy@redhat.com> writes:
> 
> >> 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.
> 
> 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

Patch

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);
+    }
+    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) {