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

[Anthony Liguori]

On 07/27/2011 02:07 AM, 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.

The problem is that guest_open is a hack.

You want connection semantics.  You need the following information from 
the backend and the client:

1) backend is associated with a transport.  The transport may disconnect 
at any point in time.  The backend needs to have explicit state 
transitions associated with the transport disconnecting and connecting.

2) the client may disconnect and reconnect at any point in time.  A 
device model reset is a disconnect followed by a reconnect.

This gives you the following matrix of states:

A: backend-connected, client-connected
B: backend-disconnected, client-disconnected
C: backend-connected, client-disconnected
D: backend-disconnected, client-connected

The state transition diagram looks like this:

B: for some devices, immediately goto C.  other devices, on accept() 
goto D.  if in B and client connects, goto D

C: if transport disconnects, goto B. if client connects, goto A

D: if transport connects, goto A.  if client disconects, goto B

A: if transport disconnects, goto B, if client disconnects, goto C

The problem is that guest_open() is a poor approximation of 
'client-connected' and it's not used universally.  We need to introduce 
proper state tracking to the character device layer and we need to have 
a proper connection function that is used by all char device clients.

Semantically, write should only be allowed in states A and D.  Read 
should only be allowed in states A and C.

C and D should have very well defined semantics about what happens to 
the data that is written  Arguably, read/write should not be allowed in 
states C/D.

Device reset should always trigger a client reconnect.  Migration resets 
devices so migration would Just Work if we modelled the state 
transitions appropriately.

Regards,

Anthony Liguori