From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49818) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZKVtK-0001bE-GV for qemu-devel@nongnu.org; Wed, 29 Jul 2015 14:15:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZKVtF-0002hm-Jt for qemu-devel@nongnu.org; Wed, 29 Jul 2015 14:15:46 -0400 Received: from mx1.redhat.com ([209.132.183.28]:37509) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZKVtF-0002gk-8M for qemu-devel@nongnu.org; Wed, 29 Jul 2015 14:15:41 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (Postfix) with ESMTPS id 00DD3A10C6 for ; Wed, 29 Jul 2015 18:15:39 +0000 (UTC) Date: Wed, 29 Jul 2015 21:15:37 +0300 From: "Michael S. Tsirkin" Message-ID: <20150729211429-mutt-send-email-mst@redhat.com> References: <1438189918-2087-1-git-send-email-marcel@redhat.com> <20150729201945-mutt-send-email-mst@redhat.com> <55B910D3.9060103@redhat.com> <20150729210414-mutt-send-email-mst@redhat.com> <55B91746.6070408@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <55B91746.6070408@redhat.com> Subject: Re: [Qemu-devel] [PATCH] vhost-user: sync backend features List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Marcel Apfelbaum Cc: jasowang@redhat.com, famz@redhat.com, qemu-devel@nongnu.org On Wed, Jul 29, 2015 at 09:11:18PM +0300, Marcel Apfelbaum wrote: > On 07/29/2015 09:05 PM, Michael S. Tsirkin wrote: > >On Wed, Jul 29, 2015 at 08:43:47PM +0300, Marcel Apfelbaum wrote: > >>On 07/29/2015 08:20 PM, Michael S. Tsirkin wrote: > >>>On Wed, Jul 29, 2015 at 08:11:58PM +0300, Marcel Apfelbaum wrote: > >>>>Complete vhost-user negotiation by syncing the features > >>>>supported by the backend. > >>>> > >>>>Signed-off-by: Marcel Apfelbaum > >>>>--- > >>>> To be used on top of: > >>>> [PATCH 0/4] vhost-user: protocol updates > >>>> https://lists.gnu.org/archive/html/qemu-devel/2015-07/msg03842.html > >>>> > >>>> Currently the vhost-user supported features are not evaluated. > >>>> The way I see it, and please correct me, the best way to do > >>>> this is to: > >>>> 1. get the backend features on vhost init > >>>> 2. Instead of simply copying them during features ack, > >>>> check that that all backend features are supported by current QEMU > >>>> 3. All other code should remain the same. > >>>> > >>>> Thanks, > >>>> Marcel > >>>> > >>>> hw/net/vhost_net.c | 3 ++- > >>>> hw/virtio/vhost-user.c | 4 ++-- > >>>> 2 files changed, 4 insertions(+), 3 deletions(-) > >>>> > >>>>diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c > >>>>index c864237..1ea5866 100644 > >>>>--- a/hw/net/vhost_net.c > >>>>+++ b/hw/net/vhost_net.c > >>>>@@ -118,7 +118,8 @@ uint64_t vhost_net_get_features(struct vhost_net *net, uint64_t features) > >>>> > >>>> void vhost_net_ack_features(struct vhost_net *net, uint64_t features) > >>>> { > >>>>- net->dev.acked_features = net->dev.backend_features; > >>>>+ vhost_ack_features(&net->dev, vhost_net_get_feature_bits(net), > >>>>+ net->dev.backend_features); > >>>> vhost_ack_features(&net->dev, vhost_net_get_feature_bits(net), features); > >>> > >>>So you ack it twice? > >>Not really, first call to vhost_ack_features acks the net->dev.backend_features, > >>the second one acks the guest features. > >> > >>The first call replaces the previous assignment that assumes that all backend features are > >>supported by QEMU. > >> > >>Thanks, > >>Marcel > > > >I think it's cleaner to whitelist backend features QEMU supports. > >I thought we did this, will look again. > I thought to do it the same as you started in vhost-init: > > if (__virtio_has_feature(msg.u64, VHOST_USER_F_PROTOCOL_FEATURES)) { > dev->backend_features |= 1ULL << VHOST_USER_F_PROTOCOL_FEATURES; > > However, we should check all possible flags, no? We have about 16 now. > And we'll need to manually add to white-list every new feature, seems error prone. But more secure :) > Anyway, I am open to suggestions. > > Thanks, > Marcel Can you describe the problem you are solving? Maybe write a unit test patch that'll make unit test fail? > > >> > >>> > >>>> } > >>>> > >>>>diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c > >>>>index c4428a1..077457b 100644 > >>>>--- a/hw/virtio/vhost-user.c > >>>>+++ b/hw/virtio/vhost-user.c > >>>>@@ -358,9 +358,9 @@ static int vhost_user_init(struct vhost_dev *dev, void *opaque) > >>>> return err; > >>>> } > >>>> > >>>>- if (__virtio_has_feature(msg.u64, VHOST_USER_F_PROTOCOL_FEATURES)) { > >>>>- dev->backend_features |= 1ULL << VHOST_USER_F_PROTOCOL_FEATURES; > >>>>+ dev->backend_features = msg.u64; > >>>> > >>>>+ if (__virtio_has_feature(msg.u64, VHOST_USER_F_PROTOCOL_FEATURES)) { > >>>> msg.request = VHOST_USER_GET_PROTOCOL_FEATURES; > >>>> msg.flags = VHOST_USER_VERSION; > >>>> msg.size = 0; > >>>>-- > >>>>2.1.0