From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55826) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a1Ouu-0006mO-Ot for qemu-devel@nongnu.org; Tue, 24 Nov 2015 20:30:41 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a1Our-0007Tu-Hg for qemu-devel@nongnu.org; Tue, 24 Nov 2015 20:30:40 -0500 Received: from mga01.intel.com ([192.55.52.88]:43486) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a1Our-0007Tg-8U for qemu-devel@nongnu.org; Tue, 24 Nov 2015 20:30:37 -0500 Date: Wed, 25 Nov 2015 09:32:15 +0800 From: Yuanhan Liu Message-ID: <20151125013215.GK2325@yliu-dev.sh.intel.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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20151124232037-mutt-send-email-mst@redhat.com> 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: "Michael S. Tsirkin" Cc: Thibaut Collet , Jason Wang , Victor Kaplansky , =?iso-8859-1?Q?Marc-Andr=E9?= Lureau , qemu-devel 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 > > > > > Yunnan, Victor - what do you think? > > > > > > -- > > > MST