Re: [Qemu-devel] [PATCH] vhost-user: sync backend features

[Marcel Apfelbaum <marcel@redhat.com>]

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 <marcel@redhat.com>
>>>>>> ---
>>>>>>   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