* [Qemu-devel] [PATCH] vhost-user: sync backend features
@ 2015-07-29 17:11 Marcel Apfelbaum
2015-07-29 17:20 ` Michael S. Tsirkin
0 siblings, 1 reply; 7+ messages in thread
From: Marcel Apfelbaum @ 2015-07-29 17:11 UTC (permalink / raw)
To: qemu-devel; +Cc: jasowang, famz, mst
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);
}
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] vhost-user: sync backend features
2015-07-29 17:11 [Qemu-devel] [PATCH] vhost-user: sync backend features Marcel Apfelbaum
@ 2015-07-29 17:20 ` Michael S. Tsirkin
2015-07-29 17:43 ` Marcel Apfelbaum
0 siblings, 1 reply; 7+ messages in thread
From: Michael S. Tsirkin @ 2015-07-29 17:20 UTC (permalink / raw)
To: Marcel Apfelbaum; +Cc: jasowang, famz, qemu-devel
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?
> }
>
> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] vhost-user: sync backend features
2015-07-29 17:20 ` Michael S. Tsirkin
@ 2015-07-29 17:43 ` Marcel Apfelbaum
2015-07-29 18:05 ` Michael S. Tsirkin
0 siblings, 1 reply; 7+ messages in thread
From: Marcel Apfelbaum @ 2015-07-29 17:43 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: jasowang, famz, qemu-devel
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
>
>> }
>>
>> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] vhost-user: sync backend features
2015-07-29 17:43 ` Marcel Apfelbaum
@ 2015-07-29 18:05 ` Michael S. Tsirkin
2015-07-29 18:11 ` Marcel Apfelbaum
0 siblings, 1 reply; 7+ messages in thread
From: Michael S. Tsirkin @ 2015-07-29 18:05 UTC (permalink / raw)
To: Marcel Apfelbaum; +Cc: jasowang, famz, qemu-devel
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.
>
> >
> >> }
> >>
> >>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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] vhost-user: sync backend features
2015-07-29 18:05 ` Michael S. Tsirkin
@ 2015-07-29 18:11 ` Marcel Apfelbaum
2015-07-29 18:15 ` Michael S. Tsirkin
0 siblings, 1 reply; 7+ messages in thread
From: Marcel Apfelbaum @ 2015-07-29 18:11 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: jasowang, famz, qemu-devel
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.
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] vhost-user: sync backend features
2015-07-29 18:11 ` Marcel Apfelbaum
@ 2015-07-29 18:15 ` Michael S. Tsirkin
2015-07-29 18:20 ` Marcel Apfelbaum
0 siblings, 1 reply; 7+ messages in thread
From: Michael S. Tsirkin @ 2015-07-29 18:15 UTC (permalink / raw)
To: Marcel Apfelbaum; +Cc: jasowang, famz, qemu-devel
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?
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] vhost-user: sync backend features
2015-07-29 18:15 ` Michael S. Tsirkin
@ 2015-07-29 18:20 ` Marcel Apfelbaum
0 siblings, 0 replies; 7+ messages in thread
From: Marcel Apfelbaum @ 2015-07-29 18:20 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: jasowang, famz, qemu-devel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-07-29 18:20 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-07-29 17:11 [Qemu-devel] [PATCH] vhost-user: sync backend features Marcel Apfelbaum
2015-07-29 17:20 ` Michael S. Tsirkin
2015-07-29 17:43 ` Marcel Apfelbaum
2015-07-29 18:05 ` Michael S. Tsirkin
2015-07-29 18:11 ` Marcel Apfelbaum
2015-07-29 18:15 ` Michael S. Tsirkin
2015-07-29 18:20 ` Marcel Apfelbaum
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).