Re: [Qemu-devel] [PATCH] virtio-serial-bus: replay guest_open on migration

[Alon Levy]

On Wed, Jul 27, 2011 at 03:50:11PM +0530, Amit Shah wrote:
> On (Wed) 27 Jul 2011 [10:07:56], Alon Levy wrote:
> > On Wed, Jul 27, 2011 at 07:45:25AM +0200, Markus Armbruster wrote:
> > > Alon Levy <address@hidden> writes:
> > > 
> > > > Signed-off-by: Alon Levy <address@hidden>
> > > > ---
> > > >  hw/virtio-serial-bus.c |    8 +++++++-
> > > >  1 files changed, 7 insertions(+), 1 deletions(-)
> > > >
> > > > diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
> > > > index c5eb931..7a652ff 100644
> > > > --- a/hw/virtio-serial-bus.c
> > > > +++ b/hw/virtio-serial-bus.c
> > > > @@ -618,14 +618,20 @@ static int virtio_serial_load(QEMUFile *f, void 
> > > > *opaque, int version_id)
> > > >      for (i = 0; i < nr_active_ports; i++) {
> > > >          uint32_t id;
> > > >          bool host_connected;
> > > > +        VirtIOSerialPortInfo *info;
> > > >  
> > > >          id = qemu_get_be32(f);
> > > >          port = find_port_by_id(s, id);
> > > >          if (!port) {
> > > >              return -EINVAL;
> > > >          }
> > > > -
> > > >          port->guest_connected = qemu_get_byte(f);
> > > > +        info = DO_UPCAST(VirtIOSerialPortInfo, qdev, port->dev.info);
> > > > +        if (port->guest_connected && info->guest_open) {
> > > > +            /* replay guest open */
> > > > +            info->guest_open(port);
> > > > +
> > > > +        }
> > > >          host_connected = qemu_get_byte(f);
> > > >          if (host_connected != port->host_connected) {
> > > >              /*
> > > 
> > > The patch makes enough sense to me, but the commit message is
> > > insufficient.  Why do you have to replay?  And what's being fixed?
> > 
> > When migrating a host with with a spice agent running the mouse becomes
> > non operational after the migration. This is rhbz #718463, currently on
> > spice-server but it seems this is a qemu-kvm issue. The problem is that
> > after migration spice doesn't know the guest agent is open. Spice is just
> > a char dev here. And a chardev cannot query it's device, the device has
> > to let the chardev know when it is open. Right now after migration the
> > chardev which is recreated is in it's default state, which assumes the
> > guest is disconnected. Char devices carry no information across migration,
> > but the virtio-serial does already carry the guest_connected state. This
> > patch passes that bit to the chardev.
> 
> It's not guaranteed all ports will be chardevs.
> 

The wording may be off, but the code isn't - it doesn't check for chardev,
it calls the same guest_open callback that is called when guest_connected
is changed from 0 to 1. So the logic is:
1. Start from stratch.
2. guest_connected 0->1
3. info->guest_open(port)

And on migration target:
5. if (guest_connected)
6.  info->guest_open(port)

If that callback was non NULL it would be called in a non migration scenario
as well, so no reason to care if it is specifically a chardev or not, let alone
specifically a spice chardev or not.

> My thinking was this can be handled by qemu-char-spice.c since it can
> add a new migration section and if an image is being restored and the
> guest agent channel is open after migration finishes, it can continue
> its work from there.  What's the benefit of all virtio-serial ports
> receiving a guest_open() event in this case?
> 

Consistency between non migration guest open and migrating when guest connected.

> Also, we'll be lying that a guest opened, since a guest was opened
> much earlier, before migration.  Nothing has changed as far as the
> guest is concerned, this is just some host-side tracking that has to
> be done post-migrate, which belongs in individual devices / ports.

The callback is there on purpose, some qemu side users exist surely. While
I understand the lying part about the time, it is worst to lie completely
by not mentioning the guest has opened the port.

> 
> So I'm not completely sure that this is the right place for such a
> notification.  However, if others feel this is fine, I'll accept the
> patch.
> 
> (Also, when resending, make sure the whitespace changes don't go
> through.)
> 

I removed them. We can continue this on the patch - I didn't cc you since
I thought you were already agreed and just wanted Armbru/Juan to take a look.

> Thanks,
>               Amit
>