From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48680) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZKVp8-00082V-LK for qemu-devel@nongnu.org; Wed, 29 Jul 2015 14:11:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZKVp3-0000KP-RV for qemu-devel@nongnu.org; Wed, 29 Jul 2015 14:11:26 -0400 Received: from mx1.redhat.com ([209.132.183.28]:36177) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZKVp3-0000K7-K4 for qemu-devel@nongnu.org; Wed, 29 Jul 2015 14:11:21 -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 499D18EA5D for ; Wed, 29 Jul 2015 18:11:21 +0000 (UTC) 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> From: Marcel Apfelbaum Message-ID: <55B91746.6070408@redhat.com> Date: Wed, 29 Jul 2015 21:11:18 +0300 MIME-Version: 1.0 In-Reply-To: <20150729210414-mutt-send-email-mst@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] vhost-user: sync backend features List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: jasowang@redhat.com, famz@redhat.com, qemu-devel@nongnu.org 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. Anyway, I am open to suggestions. Thanks, Marcel >> >>> >>>> } >>>> >>>> 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