From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45423) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a1Z76-0005EH-M0 for qemu-devel@nongnu.org; Wed, 25 Nov 2015 07:23:57 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a1Z72-00083p-Nq for qemu-devel@nongnu.org; Wed, 25 Nov 2015 07:23:56 -0500 Received: from mx1.redhat.com ([209.132.183.28]:52941) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a1Z72-00083j-GY for qemu-devel@nongnu.org; Wed, 25 Nov 2015 07:23:52 -0500 Date: Wed, 25 Nov 2015 14:23:47 +0200 From: "Michael S. Tsirkin" Message-ID: <20151125142335-mutt-send-email-mst@redhat.com> References: <1448381436-32106-1-git-send-email-thibaut.collet@6wind.com> <1448381436-32106-2-git-send-email-thibaut.collet@6wind.com> <20151124224712-mutt-send-email-mst@redhat.com> <20151124232037-mutt-send-email-mst@redhat.com> <20151125013215.GK2325@yliu-dev.sh.intel.com> <20151125130452-mutt-send-email-mst@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH for-2.5 1/1] vhost-user: do not send SET_VRING_ENABLE at start List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Thibaut Collet Cc: Jason Wang , Yuanhan Liu , Victor Kaplansky , =?iso-8859-1?Q?Marc-Andr=E9?= Lureau , qemu-devel On Wed, Nov 25, 2015 at 01:17:53PM +0100, Thibaut Collet wrote: > On Wed, Nov 25, 2015 at 12:04 PM, Michael S. Tsirkin wrote: > > On Wed, Nov 25, 2015 at 09:32:15AM +0800, Yuanhan Liu wrote: > >> On Tue, Nov 24, 2015 at 11:23:34PM +0200, Michael S. Tsirkin wrote: > >> > On Tue, Nov 24, 2015 at 10:05:27PM +0100, Thibaut Collet wrote: > >> > > On Tue, Nov 24, 2015 at 9:52 PM, Michael S. Tsirkin wrote: > >> > > > On Tue, Nov 24, 2015 at 05:10:36PM +0100, Thibaut Collet wrote: > >> > > >> This patch reverts partially commit 3a12f32229a. > >> > > >> > >> > > >> In case of live migration several queues can be enabled and not only the first > >> > > >> one. So inform backend that only the first queue is enabled is wrong. > >> > > >> > >> > > >> Since commit 7263a0ad7899 backend is already notified of the state of the vring > >> > > >> through the vring attach operation. This function, called during the startup > >> > > >> sequence, provides the correct state of the vring, even in case of live > >> > > >> migration. > >> > > >> > >> > > >> So nothing has to be added to give the vring state to the backend at the startup. > >> > > >> > >> > > >> Signed-off-by: Thibaut Collet > >> > > >> --- > >> > > >> hw/virtio/vhost.c | 5 ----- > >> > > >> 1 file changed, 5 deletions(-) > >> > > >> > >> > > >> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > >> > > >> index 1794f0d..870cd12 100644 > >> > > >> --- a/hw/virtio/vhost.c > >> > > >> +++ b/hw/virtio/vhost.c > >> > > >> @@ -1226,11 +1226,6 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev) > >> > > >> } > >> > > >> } > >> > > >> > >> > > >> - if (hdev->vhost_ops->vhost_set_vring_enable) { > >> > > >> - /* only enable first vq pair by default */ > >> > > >> - hdev->vhost_ops->vhost_set_vring_enable(hdev, hdev->vq_index == 0); > >> > > >> - } > >> > > >> - > >> > > >> return 0; > >> > > >> fail_log: > >> > > >> vhost_log_put(hdev, false); > >> > > >> -- > >> > > >> 2.1.4 > >> > > > > >> > > > Yes - and I'm beginning to think that maybe we should revert > >> > > > all of 3a12f32229a then, for symmetry. > >> > > > > >> > > > >> > > Keep the disable vring on the stop can be useful. For example if the > >> > > VM is rebooted all the vring will be disabled and backend will avoid > >> > > to send packet to the VM in this case (I am not sure the virtio ring > >> > > address is always valid during a reboot and writingg data in this > >> > > memory can cause unexpected behaviour in this case). > >> > > >> > I think there's still some confusion: > >> > writing memory can still happen even if you disable the ring > >> > since the TX ring is still processed so we write into the used ring. > >> > > >> > We call GET_VRING_BASE on stop and that ensures rings are > >> > stopped. > >> > >> Yes, that's what I suggested first, which also makes the logic quite > >> simple: we use GET_VRING_BASE as the sign of vring stop. Intead of > >> GET_VRING_BASE when protocol not negotiated, and SET_VRING_ENABLE > >> when protocol is negotiated. > >> > >> Michael, should I submit a revert patch, or you could do it directly? > >> > >> --yliu > > > > I can handle it. > > > > The patch must be completly reverted. There are too an issue with > suspend/resume operations. > On the suspend the dev_stop is called that will disable all the vrings. > On the resume the dev_start is called but not the peer_attach (vring > is already attached) and state of the vring is not provided to the > backend. OK, I queued up a revert. Thanks! > >> > > >> > > >> > > > Yunnan, Victor - what do you think? > >> > > > > >> > > > -- > >> > > > MST