From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50667) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZKVyI-0003JB-T8 for qemu-devel@nongnu.org; Wed, 29 Jul 2015 14:20:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZKVyE-0004yc-SA for qemu-devel@nongnu.org; Wed, 29 Jul 2015 14:20:54 -0400 Received: from mx1.redhat.com ([209.132.183.28]:39346) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZKVyE-0004yW-KK for qemu-devel@nongnu.org; Wed, 29 Jul 2015 14:20:50 -0400 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (Postfix) with ESMTPS id 559D191EAA for ; Wed, 29 Jul 2015 18:20:50 +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> <55B91746.6070408@redhat.com> <20150729211429-mutt-send-email-mst@redhat.com> From: Marcel Apfelbaum Message-ID: <55B9197F.2040903@redhat.com> Date: Wed, 29 Jul 2015 21:20:47 +0300 MIME-Version: 1.0 In-Reply-To: <20150729211429-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:15 PM, Michael S. Tsirkin wrote: > 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? vhost-user backend features are not enabled if are not negotiated, for example virtio 1.0 . > Maybe write a unit test patch that'll make unit test fail? I'll have a look to the current unit tests to see if we can load a guest virtio driver and check what features are negotiated. If we have something like that, it will not be a problem. 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