From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:42114) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qm4uo-0008HB-OD for qemu-devel@nongnu.org; Wed, 27 Jul 2011 10:16:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Qm4uk-0003Hr-HY for qemu-devel@nongnu.org; Wed, 27 Jul 2011 10:16:50 -0400 Received: from mail-yi0-f45.google.com ([209.85.218.45]:53756) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qm4uk-0003Hn-CX for qemu-devel@nongnu.org; Wed, 27 Jul 2011 10:16:46 -0400 Received: by yia25 with SMTP id 25so1280684yia.4 for ; Wed, 27 Jul 2011 07:16:38 -0700 (PDT) Message-ID: <4E301DC1.30901@codemonkey.ws> Date: Wed, 27 Jul 2011 09:16:33 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <1311685434-25282-1-git-send-email-alevy@redhat.com> <20110727070756.GZ2375@bow.redhat.com> In-Reply-To: <20110727070756.GZ2375@bow.redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] virtio-serial-bus: replay guest_open on migration List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster , qemu-devel@nongnu.org, amit.shah@redhat.com, hdegoede@redhat.com, yhalperi@redhat.com, quintela@redhat.com 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 writes: >> >>> Signed-off-by: Alon Levy >>> --- >>> 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