Patchwork hw/virtio-serial-bus: replay guest open on destination

login
register
mail settings
Submitter Alon Levy
Date Nov. 25, 2012, 1:39 p.m.
Message ID <1353850754-22704-1-git-send-email-alevy@redhat.com>
Download mbox | patch
Permalink /patch/201544/
State New
Headers show

Comments

Alon Levy - Nov. 25, 2012, 1:39 p.m.
When migrating a host with with a spice agent running the mouse becomes
non operational after the migration due to the agent state being
inconsistent between the guest and the client if the client is using
semi-seamless or switch host migration.

After migration the target client has never received the guest_open
initiated spice message. Virtio-serial holds this state information and
migrates it, so replay that over the chardev post migration. Fix is not
spice specific but spice is the only client that cares about it.

rhbz #725965.
---
 hw/virtio-serial-bus.c | 6 ++++++
 1 file changed, 6 insertions(+)
Amit Shah - Nov. 27, 2012, 12:48 p.m.
On (Sun) 25 Nov 2012 [15:39:14], Alon Levy wrote:
> When migrating a host with with a spice agent running the mouse becomes
> non operational after the migration due to the agent state being
> inconsistent between the guest and the client if the client is using
> semi-seamless or switch host migration.
> 
> After migration the target client has never received the guest_open
> initiated spice message. Virtio-serial holds this state information and
> migrates it, so replay that over the chardev post migration. Fix is not
> spice specific but spice is the only client that cares about it.

Thanks for continuing to pursue this :)

> rhbz #725965.
> ---
>  hw/virtio-serial-bus.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
> index efa8a81..ccce1fa 100644
> --- a/hw/virtio-serial-bus.c
> +++ b/hw/virtio-serial-bus.c
> @@ -641,6 +641,7 @@ static void virtio_serial_post_load_timer_cb(void *opaque)
>      VirtIOSerial *s = opaque;
>      VirtIOSerialPort *port;
>      uint8_t host_connected;
> +    VirtIOSerialPortClass *vsc;
>  
>      for (i = 0 ; i < s->post_load.nr_active_ports; ++i) {
>          port = s->post_load.connected[i].port;
> @@ -653,6 +654,11 @@ static void virtio_serial_post_load_timer_cb(void *opaque)
>              send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN,
>                                 port->host_connected);
>          }
> +        vsc = VIRTIO_SERIAL_PORT_GET_CLASS(port);
> +        if (port->guest_connected && vsc->guest_open) {
> +            /* replay guest open */
> +            vsc->guest_open(port);
> +        }

I think the last time we discussed this, my objection was the guest
isn't really doing an open again, and since spice depends on the
guest's connectedness, spice should have a post-load hook or a similar
bh that would query virtio-serial for guest connectedness status, and,
if connected, do whatever setup is necessary.

Adding Gerd and Markus as I think they were involved in the discussion
last time as well.

		Amit
Markus Armbruster - Nov. 27, 2012, 2:10 p.m.
Amit Shah <amit.shah@redhat.com> writes:

> On (Sun) 25 Nov 2012 [15:39:14], Alon Levy wrote:
>> When migrating a host with with a spice agent running the mouse becomes
>> non operational after the migration due to the agent state being
>> inconsistent between the guest and the client if the client is using
>> semi-seamless or switch host migration.
>> 
>> After migration the target client has never received the guest_open
>> initiated spice message. Virtio-serial holds this state information and
>> migrates it, so replay that over the chardev post migration. Fix is not
>> spice specific but spice is the only client that cares about it.
>
> Thanks for continuing to pursue this :)
>
>> rhbz #725965.
>> ---
>>  hw/virtio-serial-bus.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>> 
>> diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
>> index efa8a81..ccce1fa 100644
>> --- a/hw/virtio-serial-bus.c
>> +++ b/hw/virtio-serial-bus.c
>> @@ -641,6 +641,7 @@ static void virtio_serial_post_load_timer_cb(void *opaque)
>>      VirtIOSerial *s = opaque;
>>      VirtIOSerialPort *port;
>>      uint8_t host_connected;
>> +    VirtIOSerialPortClass *vsc;
>>  
>>      for (i = 0 ; i < s->post_load.nr_active_ports; ++i) {
>>          port = s->post_load.connected[i].port;
>> @@ -653,6 +654,11 @@ static void virtio_serial_post_load_timer_cb(void *opaque)
>>              send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN,
>>                                 port->host_connected);
>>          }
>> +        vsc = VIRTIO_SERIAL_PORT_GET_CLASS(port);
>> +        if (port->guest_connected && vsc->guest_open) {
>> +            /* replay guest open */
>> +            vsc->guest_open(port);
>> +        }
>
> I think the last time we discussed this, my objection was the guest
> isn't really doing an open again, and since spice depends on the
> guest's connectedness, spice should have a post-load hook or a similar
> bh that would query virtio-serial for guest connectedness status, and,
> if connected, do whatever setup is necessary.
>
> Adding Gerd and Markus as I think they were involved in the discussion
> last time as well.

Got a pointer to the old thread?
Amit Shah - Nov. 27, 2012, 2:34 p.m.
On (Tue) 27 Nov 2012 [15:10:06], Markus Armbruster wrote:
> Amit Shah <amit.shah@redhat.com> writes:

> > Adding Gerd and Markus as I think they were involved in the discussion
> > last time as well.
> 
> Got a pointer to the old thread?

https://lists.nongnu.org/archive/html/qemu-devel/2011-07/msg02986.html

I didn't go through it entirely, but I'm looking for guidance here --
should spice handle the migration (as I suggest), or should
virtio-serial-bus replay guest_open (as this patch by Alon does)?

		Amit
Anthony Liguori - Nov. 27, 2012, 7:03 p.m.
Amit Shah <amit.shah@redhat.com> writes:

> On (Sun) 25 Nov 2012 [15:39:14], Alon Levy wrote:
>> When migrating a host with with a spice agent running the mouse becomes
>> non operational after the migration due to the agent state being
>> inconsistent between the guest and the client if the client is using
>> semi-seamless or switch host migration.
>> 
>> After migration the target client has never received the guest_open
>> initiated spice message. Virtio-serial holds this state information and
>> migrates it, so replay that over the chardev post migration. Fix is not
>> spice specific but spice is the only client that cares about it.
>
> Thanks for continuing to pursue this :)
>
>> rhbz #725965.
>> ---
>>  hw/virtio-serial-bus.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>> 
>> diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
>> index efa8a81..ccce1fa 100644
>> --- a/hw/virtio-serial-bus.c
>> +++ b/hw/virtio-serial-bus.c
>> @@ -641,6 +641,7 @@ static void virtio_serial_post_load_timer_cb(void *opaque)
>>      VirtIOSerial *s = opaque;
>>      VirtIOSerialPort *port;
>>      uint8_t host_connected;
>> +    VirtIOSerialPortClass *vsc;
>>  
>>      for (i = 0 ; i < s->post_load.nr_active_ports; ++i) {
>>          port = s->post_load.connected[i].port;
>> @@ -653,6 +654,11 @@ static void virtio_serial_post_load_timer_cb(void *opaque)
>>              send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN,
>>                                 port->host_connected);
>>          }
>> +        vsc = VIRTIO_SERIAL_PORT_GET_CLASS(port);
>> +        if (port->guest_connected && vsc->guest_open) {
>> +            /* replay guest open */
>> +            vsc->guest_open(port);
>> +        }
>
> I think the last time we discussed this, my objection was the guest
> isn't really doing an open again, and since spice depends on the
> guest's connectedness, spice should have a post-load hook or a similar
> bh that would query virtio-serial for guest connectedness status, and,
> if connected, do whatever setup is necessary.

Agreed.

Regards,

Anthony Liguori

>
> Adding Gerd and Markus as I think they were involved in the discussion
> last time as well.
>
> 		Amit
Alon Levy - Nov. 28, 2012, 9:05 a.m.
Adds a new char device backend callback to check connectedness, implemented
for virtio console, and used by spice-char-dev in post migration.

Is using NULL for DeviceState the intention for non device vmstates? It works
fine in practice.

Alon Levy (3):
  virtio-serial: add virtio_serial_guest_connected
  qemu-char: add qemu_chr_be_connected
  spice-qemu-char: register interface on post load

 backends/rng-egd.c      |  4 ++--
 gdbstub.c               |  2 +-
 hw/ccid-card-passthru.c |  1 +
 hw/debugcon.c           |  2 +-
 hw/ivshmem.c            |  8 ++++----
 hw/qdev-properties.c    |  2 +-
 hw/serial.c             |  4 ++--
 hw/usb/dev-serial.c     |  4 ++--
 hw/usb/redirect.c       |  2 +-
 hw/virtio-console.c     | 12 ++++++++++--
 hw/virtio-serial-bus.c  |  9 +++++++++
 hw/virtio-serial.h      |  5 +++++
 main-loop.h             |  1 +
 monitor.c               |  4 ++--
 net/slirp.c             |  2 +-
 qemu-char.c             | 11 ++++++++++-
 qemu-char.h             | 13 +++++++++++++
 qtest.c                 |  2 +-
 spice-qemu-char.c       | 34 ++++++++++++++++++++++++++++++++++
 19 files changed, 101 insertions(+), 21 deletions(-)
Amit Shah - Dec. 13, 2012, 10:54 a.m.
On (Wed) 28 Nov 2012 [11:05:37], Alon Levy wrote:
> Adds a new char device backend callback to check connectedness, implemented
> for virtio console, and used by spice-char-dev in post migration.
> 
> Is using NULL for DeviceState the intention for non device vmstates? It works
> fine in practice.

Any more opinions / reviews on this series?  Anthony?

		Amit
Anthony Liguori - Dec. 13, 2012, 2:25 p.m.
Amit Shah <amit.shah@redhat.com> writes:

> On (Wed) 28 Nov 2012 [11:05:37], Alon Levy wrote:
>> Adds a new char device backend callback to check connectedness, implemented
>> for virtio console, and used by spice-char-dev in post migration.
>> 
>> Is using NULL for DeviceState the intention for non device vmstates? It works
>> fine in practice.
>
> Any more opinions / reviews on this series?  Anthony?

Spice should save its state and not rely on the chardev layer to replay
it.

I thought this was already pointed out in this thread?

Regards,

Anthony Liguori

>
> 		Amit
Amit Shah - Dec. 14, 2012, 4:10 a.m.
On (Thu) 13 Dec 2012 [08:25:08], Anthony Liguori wrote:
> Amit Shah <amit.shah@redhat.com> writes:
> 
> > On (Wed) 28 Nov 2012 [11:05:37], Alon Levy wrote:
> >> Adds a new char device backend callback to check connectedness, implemented
> >> for virtio console, and used by spice-char-dev in post migration.
> >> 
> >> Is using NULL for DeviceState the intention for non device vmstates? It works
> >> fine in practice.
> >
> > Any more opinions / reviews on this series?  Anthony?
> 
> Spice should save its state and not rely on the chardev layer to replay
> it.
> 
> I thought this was already pointed out in this thread?

Indeed.  Alon, just adding save/load to spice-qemu-char.c doesn't work
for you?

		Amit

Patch

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index efa8a81..ccce1fa 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -641,6 +641,7 @@  static void virtio_serial_post_load_timer_cb(void *opaque)
     VirtIOSerial *s = opaque;
     VirtIOSerialPort *port;
     uint8_t host_connected;
+    VirtIOSerialPortClass *vsc;
 
     for (i = 0 ; i < s->post_load.nr_active_ports; ++i) {
         port = s->post_load.connected[i].port;
@@ -653,6 +654,11 @@  static void virtio_serial_post_load_timer_cb(void *opaque)
             send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN,
                                port->host_connected);
         }
+        vsc = VIRTIO_SERIAL_PORT_GET_CLASS(port);
+        if (port->guest_connected && vsc->guest_open) {
+            /* replay guest open */
+            vsc->guest_open(port);
+        }
     }
     g_free(s->post_load.connected);
     s->post_load.connected = NULL;