| Message ID | 1354093540-22583-4-git-send-email-alevy@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <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) { > >
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 <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 > > [...] >
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
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) {
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(+)