On Wed, Jul 27, 2011 at 09:16:33AM -0500, Anthony Liguori wrote:
> 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<al...@redhat.com> writes:
> >>
> >>>Signed-off-by: Alon Levy<al...@redhat.com>
> >>>---
> >>> 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.
Are you saying currently on migration the guest (client above) always
receives an event from virtio? The guest_open callback happens when
a guest operation happens, not when the device gets reset, unless the later
triggers the former, but I don't understand how that would happen since a
reset can happen while the guest isn't ready to handle anything (guest is
booting).
I do see a virtio_pci_reset does a virtio_reset which sends the
VIRTIO_NO_VECTOR interrupt, but I don't understand what happens after that.
Besides, I understand the need to fix the connection semantics of chardevs,
but the situation is broken right now and even if someone were to write this
I don't believe you would just take it to 0.15.0, would you?
Also, the conversation is still ongoing but Armbru mentioned some
''relevant-cases''
in http://lists.nongnu.org/archive/html/qemu-devel/2011-07/msg03221.html
backing the fix-the-hack approach (at least for 0.15.0).
>
> Regards,
>
> Anthony Liguori
>
