[PATCH] vhost: fix checking of device features

[PATCH] vhost: fix checking of device features

[Ivan Dyukov]

To compare enabled features in current device we must use bit
mask instead of bit position.

CC: stable@dpdk.org
Fixes: c843af3aa13e ("vhost: access header only")

Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
---
 lib/librte_vhost/virtio_net.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
index ebfda1c..4fae4c1 100644
--- a/lib/librte_vhost/virtio_net.c
+++ b/lib/librte_vhost/virtio_net.c
@@ -601,9 +601,11 @@ static inline bool
 virtio_net_with_host_offload(struct virtio_net *dev)
 {
 	if (dev->features &
-			(VIRTIO_NET_F_CSUM | VIRTIO_NET_F_HOST_ECN |
-			 VIRTIO_NET_F_HOST_TSO4 | VIRTIO_NET_F_HOST_TSO6 |
-			 VIRTIO_NET_F_HOST_UFO))
+			((1ULL << VIRTIO_NET_F_CSUM) |
+			 (1ULL << VIRTIO_NET_F_HOST_ECN) |
+			 (1ULL << VIRTIO_NET_F_HOST_TSO4) |
+			 (1ULL << VIRTIO_NET_F_HOST_TSO6) |
+			 (1ULL << VIRTIO_NET_F_HOST_UFO)))
 		return true;
 
 	return false;
-- 
2.7.4

Re: [PATCH] vhost: fix checking of device features

[Maxime Coquelin]

On 06/28/2017 02:40 PM, Ivan Dyukov wrote:
> To compare enabled features in current device we must use bit
> mask instead of bit position.
> 
> CC: stable@dpdk.org
> Fixes: c843af3aa13e ("vhost: access header only")
> 
> Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
> ---
>   lib/librte_vhost/virtio_net.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)

Thanks for the fix Ivan, and sorry for introducing this bug.
Out of curiosity, did you noticed it because it broke offloading,
or just by code review?

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

> diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
> index ebfda1c..4fae4c1 100644
> --- a/lib/librte_vhost/virtio_net.c
> +++ b/lib/librte_vhost/virtio_net.c
> @@ -601,9 +601,11 @@ static inline bool
>   virtio_net_with_host_offload(struct virtio_net *dev)
>   {
>   	if (dev->features &
> -			(VIRTIO_NET_F_CSUM | VIRTIO_NET_F_HOST_ECN |
> -			 VIRTIO_NET_F_HOST_TSO4 | VIRTIO_NET_F_HOST_TSO6 |
> -			 VIRTIO_NET_F_HOST_UFO))
> +			((1ULL << VIRTIO_NET_F_CSUM) |
> +			 (1ULL << VIRTIO_NET_F_HOST_ECN) |
> +			 (1ULL << VIRTIO_NET_F_HOST_TSO4) |
> +			 (1ULL << VIRTIO_NET_F_HOST_TSO6) |
> +			 (1ULL << VIRTIO_NET_F_HOST_UFO)))
>   		return true;
>   
>   	return false;
> 

Re: [PATCH] vhost: fix checking of device features

[Ivan Dyukov]

On 06/28/2017 03:54 PM, Maxime Coquelin wrote:
>
>
> On 06/28/2017 02:40 PM, Ivan Dyukov wrote:
>> To compare enabled features in current device we must use bit
>> mask instead of bit position.
>>
>> CC: stable@dpdk.org
>> Fixes: c843af3aa13e ("vhost: access header only")
>>
>> Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
>> ---
>>   lib/librte_vhost/virtio_net.c | 8 +++++---
>>   1 file changed, 5 insertions(+), 3 deletions(-)
>
> Thanks for the fix Ivan, and sorry for introducing this bug.
> Out of curiosity, did you noticed it because it broke offloading,
> or just by code review?
I didn't see any breakages. It's just code review.
>
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>
>> diff --git a/lib/librte_vhost/virtio_net.c 
>> b/lib/librte_vhost/virtio_net.c
>> index ebfda1c..4fae4c1 100644
>> --- a/lib/librte_vhost/virtio_net.c
>> +++ b/lib/librte_vhost/virtio_net.c
>> @@ -601,9 +601,11 @@ static inline bool
>>   virtio_net_with_host_offload(struct virtio_net *dev)
>>   {
>>       if (dev->features &
>> -            (VIRTIO_NET_F_CSUM | VIRTIO_NET_F_HOST_ECN |
>> -             VIRTIO_NET_F_HOST_TSO4 | VIRTIO_NET_F_HOST_TSO6 |
>> -             VIRTIO_NET_F_HOST_UFO))
>> +            ((1ULL << VIRTIO_NET_F_CSUM) |
>> +             (1ULL << VIRTIO_NET_F_HOST_ECN) |
>> +             (1ULL << VIRTIO_NET_F_HOST_TSO4) |
>> +             (1ULL << VIRTIO_NET_F_HOST_TSO6) |
>> +             (1ULL << VIRTIO_NET_F_HOST_UFO)))
>>           return true;
>>         return false;
>>
>
>
>
>

Re: [PATCH] vhost: fix checking of device features

[Tan, Jianfeng]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ivan Dyukov
> Sent: Wednesday, June 28, 2017 8:41 PM
> To: yliu@fridaylinux.org; maxime.coquelin@redhat.com; dev@dpdk.org
> Cc: i.maximets@samsung.com; heetae82.ahn@samsung.com; Ivan Dyukov;
> stable@dpdk.org
> Subject: [dpdk-dev] [PATCH] vhost: fix checking of device features
> 
> To compare enabled features in current device we must use bit
> mask instead of bit position.
> 
> CC: stable@dpdk.org
> Fixes: c843af3aa13e ("vhost: access header only")
> 
> Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
> ---
>  lib/librte_vhost/virtio_net.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
> index ebfda1c..4fae4c1 100644
> --- a/lib/librte_vhost/virtio_net.c
> +++ b/lib/librte_vhost/virtio_net.c
> @@ -601,9 +601,11 @@ static inline bool
>  virtio_net_with_host_offload(struct virtio_net *dev)
>  {
>  	if (dev->features &
> -			(VIRTIO_NET_F_CSUM | VIRTIO_NET_F_HOST_ECN |
> -			 VIRTIO_NET_F_HOST_TSO4 |
> VIRTIO_NET_F_HOST_TSO6 |
> -			 VIRTIO_NET_F_HOST_UFO))
> +			((1ULL << VIRTIO_NET_F_CSUM) |
> +			 (1ULL << VIRTIO_NET_F_HOST_ECN) |
> +			 (1ULL << VIRTIO_NET_F_HOST_TSO4) |
> +			 (1ULL << VIRTIO_NET_F_HOST_TSO6) |
> +			 (1ULL << VIRTIO_NET_F_HOST_UFO)))

Another problem in this piece of code, we don't support VIRTIO_NET_F_HOST_ECN and VIRTIO_NET_F_HOST_UFO in vhost-user. We might consider to remove those two lines?

Thanks,
Jianfeng


>  		return true;
> 
>  	return false;
> --
> 2.7.4

Re: [PATCH] vhost: fix checking of device features

[Maxime Coquelin]

On 06/29/2017 08:07 AM, Ivan Dyukov wrote:
> On 06/28/2017 03:54 PM, Maxime Coquelin wrote:
>>
>>
>> On 06/28/2017 02:40 PM, Ivan Dyukov wrote:
>>> To compare enabled features in current device we must use bit
>>> mask instead of bit position.
>>>
>>> CC: stable@dpdk.org
>>> Fixes: c843af3aa13e ("vhost: access header only")
>>>
>>> Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
>>> ---
>>>   lib/librte_vhost/virtio_net.c | 8 +++++---
>>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> Thanks for the fix Ivan, and sorry for introducing this bug.
>> Out of curiosity, did you noticed it because it broke offloading,
>> or just by code review?
> I didn't see any breakages. It's just code review.

Ok, thanks.

Maxime

Re: [PATCH] vhost: fix checking of device features

[Maxime Coquelin]

On 06/29/2017 08:13 AM, Tan, Jianfeng wrote:
> 
> 
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ivan Dyukov
>> Sent: Wednesday, June 28, 2017 8:41 PM
>> To: yliu@fridaylinux.org; maxime.coquelin@redhat.com; dev@dpdk.org
>> Cc: i.maximets@samsung.com; heetae82.ahn@samsung.com; Ivan Dyukov;
>> stable@dpdk.org
>> Subject: [dpdk-dev] [PATCH] vhost: fix checking of device features
>>
>> To compare enabled features in current device we must use bit
>> mask instead of bit position.
>>
>> CC: stable@dpdk.org
>> Fixes: c843af3aa13e ("vhost: access header only")
>>
>> Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
>> ---
>>   lib/librte_vhost/virtio_net.c | 8 +++++---
>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
>> index ebfda1c..4fae4c1 100644
>> --- a/lib/librte_vhost/virtio_net.c
>> +++ b/lib/librte_vhost/virtio_net.c
>> @@ -601,9 +601,11 @@ static inline bool
>>   virtio_net_with_host_offload(struct virtio_net *dev)
>>   {
>>   	if (dev->features &
>> -			(VIRTIO_NET_F_CSUM | VIRTIO_NET_F_HOST_ECN |
>> -			 VIRTIO_NET_F_HOST_TSO4 |
>> VIRTIO_NET_F_HOST_TSO6 |
>> -			 VIRTIO_NET_F_HOST_UFO))
>> +			((1ULL << VIRTIO_NET_F_CSUM) |
>> +			 (1ULL << VIRTIO_NET_F_HOST_ECN) |
>> +			 (1ULL << VIRTIO_NET_F_HOST_TSO4) |
>> +			 (1ULL << VIRTIO_NET_F_HOST_TSO6) |
>> +			 (1ULL << VIRTIO_NET_F_HOST_UFO)))
> 
> Another problem in this piece of code, we don't support VIRTIO_NET_F_HOST_ECN and VIRTIO_NET_F_HOST_UFO in vhost-user. We might consider to remove those two lines?

It is not really a problem as the feature is never negotiated as not
supported, it would just be a clean-up.

I think we should stick with this version as it targets also -stable.

Another patch could be sent on top to remove these unsupported feature
bits.

Thanks,
Maxime

> Thanks,
> Jianfeng
> 
> 
>>   		return true;
>>
>>   	return false;
>> --
>> 2.7.4
> 

Re: [PATCH] vhost: fix checking of device features

[Maxime Coquelin]

On 06/29/2017 09:21 AM, Maxime Coquelin wrote:
> 
> 
> On 06/29/2017 08:07 AM, Ivan Dyukov wrote:
>> On 06/28/2017 03:54 PM, Maxime Coquelin wrote:
>>>
>>>
>>> On 06/28/2017 02:40 PM, Ivan Dyukov wrote:
>>>> To compare enabled features in current device we must use bit
>>>> mask instead of bit position.
>>>>
>>>> CC: stable@dpdk.org
>>>> Fixes: c843af3aa13e ("vhost: access header only")
>>>>
>>>> Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
>>>> ---
>>>>   lib/librte_vhost/virtio_net.c | 8 +++++---
>>>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>> Thanks for the fix Ivan, and sorry for introducing this bug.
>>> Out of curiosity, did you noticed it because it broke offloading,
>>> or just by code review?
>> I didn't see any breakages. It's just code review.
> 
> Ok, thanks.

FYI, I just found another case in vhost.c, sending patch soon.

Cheers,
Maxime

Re: [PATCH] vhost: fix checking of device features

[Tan, Jianfeng]

> -----Original Message-----
> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> Sent: Thursday, June 29, 2017 3:27 PM
> To: Tan, Jianfeng; Ivan Dyukov; yliu@fridaylinux.org; dev@dpdk.org
> Cc: i.maximets@samsung.com; heetae82.ahn@samsung.com;
> stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] vhost: fix checking of device features
> 
> 
> 
> On 06/29/2017 08:13 AM, Tan, Jianfeng wrote:
> >
> >
> >> -----Original Message-----
> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ivan Dyukov
> >> Sent: Wednesday, June 28, 2017 8:41 PM
> >> To: yliu@fridaylinux.org; maxime.coquelin@redhat.com; dev@dpdk.org
> >> Cc: i.maximets@samsung.com; heetae82.ahn@samsung.com; Ivan
> Dyukov;
> >> stable@dpdk.org
> >> Subject: [dpdk-dev] [PATCH] vhost: fix checking of device features
> >>
> >> To compare enabled features in current device we must use bit
> >> mask instead of bit position.
> >>
> >> CC: stable@dpdk.org
> >> Fixes: c843af3aa13e ("vhost: access header only")
> >>
> >> Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
> >> ---
> >>   lib/librte_vhost/virtio_net.c | 8 +++++---
> >>   1 file changed, 5 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
> >> index ebfda1c..4fae4c1 100644
> >> --- a/lib/librte_vhost/virtio_net.c
> >> +++ b/lib/librte_vhost/virtio_net.c
> >> @@ -601,9 +601,11 @@ static inline bool
> >>   virtio_net_with_host_offload(struct virtio_net *dev)
> >>   {
> >>   	if (dev->features &
> >> -			(VIRTIO_NET_F_CSUM | VIRTIO_NET_F_HOST_ECN |
> >> -			 VIRTIO_NET_F_HOST_TSO4 |
> >> VIRTIO_NET_F_HOST_TSO6 |
> >> -			 VIRTIO_NET_F_HOST_UFO))
> >> +			((1ULL << VIRTIO_NET_F_CSUM) |
> >> +			 (1ULL << VIRTIO_NET_F_HOST_ECN) |
> >> +			 (1ULL << VIRTIO_NET_F_HOST_TSO4) |
> >> +			 (1ULL << VIRTIO_NET_F_HOST_TSO6) |
> >> +			 (1ULL << VIRTIO_NET_F_HOST_UFO)))
> >
> > Another problem in this piece of code, we don't support
> VIRTIO_NET_F_HOST_ECN and VIRTIO_NET_F_HOST_UFO in vhost-user. We
> might consider to remove those two lines?
> 
> It is not really a problem as the feature is never negotiated as not
> supported, it would just be a clean-up.

Yes, it's a clean-up to avoid confusion.


> 
> I think we should stick with this version as it targets also -stable.
> 
> Another patch could be sent on top to remove these unsupported feature
> bits.

Agreed.

Thanks,
Jianfeng

> 
> Thanks,
> Maxime
> 
> > Thanks,
> > Jianfeng
> >
> >
> >>   		return true;
> >>
> >>   	return false;
> >> --
> >> 2.7.4
> >

Re: [PATCH] vhost: fix checking of device features

[Yuanhan Liu]

On Wed, Jun 28, 2017 at 03:40:31PM +0300, Ivan Dyukov wrote:
> To compare enabled features in current device we must use bit
> mask instead of bit position.
> 
> CC: stable@dpdk.org
> Fixes: c843af3aa13e ("vhost: access header only")
> 
> Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>

Applied to dpdk-next-virtio.

Thanks.

	--yliu