diff mbox series

vhost: fix packed ring defines declaration

Message ID 20181122170922.15007-1-maxime.coquelin@redhat.com (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers
Series vhost: fix packed ring defines declaration |

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/intel-Performance-Testing success Performance Testing PASS

Commit Message

Maxime Coquelin Nov. 22, 2018, 5:09 p.m. UTC 
  The packed ring defines were declared only if kernel
header does not declare them.
The problem is that they are not applied in upstream kernel,
and some changes in the names have been required.

This patch declares the defines unconditionally, which
fixes potential build issues.

Fixes: 297b1e7350f6 ("vhost: add virtio packed virtqueue defines")
Cc: stable@dpdk.org

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/librte_vhost/vhost.h | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

Comments

Ferruh Yigit Nov. 22, 2018, 6:23 p.m. UTC | #1 
On 11/22/2018 5:09 PM, Maxime Coquelin wrote:
> The packed ring defines were declared only if kernel
> header does not declare them.
> The problem is that they are not applied in upstream kernel,
> and some changes in the names have been required.
> 
> This patch declares the defines unconditionally, which
> fixes potential build issues.

+1 to address possible build issues.

> 
> Fixes: 297b1e7350f6 ("vhost: add virtio packed virtqueue defines")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  lib/librte_vhost/vhost.h | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
> index 760f42192..5218f1b12 100644
> --- a/lib/librte_vhost/vhost.h
> +++ b/lib/librte_vhost/vhost.h
> @@ -219,13 +219,6 @@ struct vhost_msg {
>  
>  #define VIRTIO_F_RING_PACKED 34
>  
> -#define VRING_DESC_F_NEXT	1
> -#define VRING_DESC_F_WRITE	2
> -#define VRING_DESC_F_INDIRECT	4

Why these are not re-defined below? Not used?

> -
> -#define VRING_DESC_F_AVAIL	(1ULL << 7)
> -#define VRING_DESC_F_USED	(1ULL << 15)
> -
>  struct vring_packed_desc {
>  	uint64_t addr;
>  	uint32_t len;
> @@ -233,16 +226,23 @@ struct vring_packed_desc {
>  	uint16_t flags;
>  };
>  
> -#define VRING_EVENT_F_ENABLE 0x0
> -#define VRING_EVENT_F_DISABLE 0x1
> -#define VRING_EVENT_F_DESC 0x2
> -
>  struct vring_packed_desc_event {
>  	uint16_t off_wrap;
>  	uint16_t flags;
>  };
>  #endif
>  
> +/*
> + * Declare below packed ring defines unconditionally
> + * as Kernel header might use different names.
> + */
> +#define VRING_DESC_F_AVAIL	(1ULL << 7)
> +#define VRING_DESC_F_USED	(1ULL << 15)
> +
> +#define VRING_EVENT_F_ENABLE 0x0
> +#define VRING_EVENT_F_DISABLE 0x1
> +#define VRING_EVENT_F_DESC 0x2

What if Linux changes mind and uses old names again, build will fail again. If
related part in Linux is not released yet, what do you think being on safe side
and adding these defines with "#ifndef" wrap?

> +
>  /*
>   * Available and used descs are in same order
>   */
>
Maxime Coquelin Nov. 22, 2018, 6:51 p.m. UTC | #2 
On 11/22/18 7:23 PM, Ferruh Yigit wrote:
> On 11/22/2018 5:09 PM, Maxime Coquelin wrote:
>> The packed ring defines were declared only if kernel
>> header does not declare them.
>> The problem is that they are not applied in upstream kernel,
>> and some changes in the names have been required.
>>
>> This patch declares the defines unconditionally, which
>> fixes potential build issues.
> 
> +1 to address possible build issues.
> 
>>
>> Fixes: 297b1e7350f6 ("vhost: add virtio packed virtqueue defines")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>>   lib/librte_vhost/vhost.h | 22 +++++++++++-----------
>>   1 file changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
>> index 760f42192..5218f1b12 100644
>> --- a/lib/librte_vhost/vhost.h
>> +++ b/lib/librte_vhost/vhost.h
>> @@ -219,13 +219,6 @@ struct vhost_msg {
>>   
>>   #define VIRTIO_F_RING_PACKED 34
>>   
>> -#define VRING_DESC_F_NEXT	1
>> -#define VRING_DESC_F_WRITE	2
>> -#define VRING_DESC_F_INDIRECT	4
> 
> Why these are not re-defined below? Not used?

These defines are in kernel headers since the beginning of virtio
support, so there are no reasons to keep them (and they aren't packed-
ring specific).

I can let them if you prefer, it does not hurt but shouldn't be here.

>> -
>> -#define VRING_DESC_F_AVAIL	(1ULL << 7)
>> -#define VRING_DESC_F_USED	(1ULL << 15)
>> -
>>   struct vring_packed_desc {
>>   	uint64_t addr;
>>   	uint32_t len;
>> @@ -233,16 +226,23 @@ struct vring_packed_desc {
>>   	uint16_t flags;
>>   };
>>   
>> -#define VRING_EVENT_F_ENABLE 0x0
>> -#define VRING_EVENT_F_DISABLE 0x1
>> -#define VRING_EVENT_F_DESC 0x2
>> -
>>   struct vring_packed_desc_event {
>>   	uint16_t off_wrap;
>>   	uint16_t flags;
>>   };
>>   #endif
>>   
>> +/*
>> + * Declare below packed ring defines unconditionally
>> + * as Kernel header might use different names.
>> + */
>> +#define VRING_DESC_F_AVAIL	(1ULL << 7)
>> +#define VRING_DESC_F_USED	(1ULL << 15)
>> +
>> +#define VRING_EVENT_F_ENABLE 0x0
>> +#define VRING_EVENT_F_DISABLE 0x1
>> +#define VRING_EVENT_F_DESC 0x2
> 
> What if Linux changes mind and uses old names again, build will fail again. If
> related part in Linux is not released yet, what do you think being on safe side
> and adding these defines with "#ifndef" wrap?

There are the "old" ones.
In any case it will work (I tested it).

In a future release, I plan to get rid of this dependency with Kernel
which only causes problems.

There was a benefit at the beginning when all needed declarations where
in the Kernel headers. But now, every time we add a new virtio feature,
we have to check kernel supports it and if not define it.

Thanks,
Maxime
>> +
>>   /*
>>    * Available and used descs are in same order
>>    */
>>
>
Ferruh Yigit Nov. 22, 2018, 7:20 p.m. UTC | #3 
On 11/22/2018 6:51 PM, Maxime Coquelin wrote:
> 
> 
> On 11/22/18 7:23 PM, Ferruh Yigit wrote:
>> On 11/22/2018 5:09 PM, Maxime Coquelin wrote:
>>> The packed ring defines were declared only if kernel
>>> header does not declare them.
>>> The problem is that they are not applied in upstream kernel,
>>> and some changes in the names have been required.
>>>
>>> This patch declares the defines unconditionally, which
>>> fixes potential build issues.
>>
>> +1 to address possible build issues.
>>
>>>
>>> Fixes: 297b1e7350f6 ("vhost: add virtio packed virtqueue defines")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> ---
>>>   lib/librte_vhost/vhost.h | 22 +++++++++++-----------
>>>   1 file changed, 11 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
>>> index 760f42192..5218f1b12 100644
>>> --- a/lib/librte_vhost/vhost.h
>>> +++ b/lib/librte_vhost/vhost.h
>>> @@ -219,13 +219,6 @@ struct vhost_msg {
>>>   
>>>   #define VIRTIO_F_RING_PACKED 34
>>>   
>>> -#define VRING_DESC_F_NEXT	1
>>> -#define VRING_DESC_F_WRITE	2
>>> -#define VRING_DESC_F_INDIRECT	4
>>
>> Why these are not re-defined below? Not used?
> 
> These defines are in kernel headers since the beginning of virtio
> support, so there are no reasons to keep them (and they aren't packed-
> ring specific).
> 
> I can let them if you prefer, it does not hurt but shouldn't be here.

Not required, I just want to confirm nothing missed.

> 
>>> -
>>> -#define VRING_DESC_F_AVAIL	(1ULL << 7)
>>> -#define VRING_DESC_F_USED	(1ULL << 15)
>>> -
>>>   struct vring_packed_desc {
>>>   	uint64_t addr;
>>>   	uint32_t len;
>>> @@ -233,16 +226,23 @@ struct vring_packed_desc {
>>>   	uint16_t flags;
>>>   };
>>>   
>>> -#define VRING_EVENT_F_ENABLE 0x0
>>> -#define VRING_EVENT_F_DISABLE 0x1
>>> -#define VRING_EVENT_F_DESC 0x2
>>> -
>>>   struct vring_packed_desc_event {
>>>   	uint16_t off_wrap;
>>>   	uint16_t flags;
>>>   };
>>>   #endif
>>>   
>>> +/*
>>> + * Declare below packed ring defines unconditionally
>>> + * as Kernel header might use different names.
>>> + */
>>> +#define VRING_DESC_F_AVAIL	(1ULL << 7)
>>> +#define VRING_DESC_F_USED	(1ULL << 15)
>>> +
>>> +#define VRING_EVENT_F_ENABLE 0x0
>>> +#define VRING_EVENT_F_DISABLE 0x1
>>> +#define VRING_EVENT_F_DESC 0x2
>>
>> What if Linux changes mind and uses old names again, build will fail again. If
>> related part in Linux is not released yet, what do you think being on safe side
>> and adding these defines with "#ifndef" wrap?
> 
> There are the "old" ones.
> In any case it will work (I tested it).

I see they are the old ones but is there any chance that kernel uses them again
(for some odd reason) ? Which can break the build. Should we add a protection?

> 
> In a future release, I plan to get rid of this dependency with Kernel
> which only causes problems.
> 
> There was a benefit at the beginning when all needed declarations where
> in the Kernel headers. But now, every time we add a new virtio feature,
> we have to check kernel supports it and if not define it.
> 
> Thanks,
> Maxime
>>> +
>>>   /*
>>>    * Available and used descs are in same order
>>>    */
>>>
>>
Maxime Coquelin Nov. 22, 2018, 7:50 p.m. UTC | #4 
On 11/22/18 8:20 PM, Ferruh Yigit wrote:
> On 11/22/2018 6:51 PM, Maxime Coquelin wrote:
>>
>>
>> On 11/22/18 7:23 PM, Ferruh Yigit wrote:
>>> On 11/22/2018 5:09 PM, Maxime Coquelin wrote:
>>>> The packed ring defines were declared only if kernel
>>>> header does not declare them.
>>>> The problem is that they are not applied in upstream kernel,
>>>> and some changes in the names have been required.
>>>>
>>>> This patch declares the defines unconditionally, which
>>>> fixes potential build issues.
>>>
>>> +1 to address possible build issues.
>>>
>>>>
>>>> Fixes: 297b1e7350f6 ("vhost: add virtio packed virtqueue defines")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>> ---
>>>>    lib/librte_vhost/vhost.h | 22 +++++++++++-----------
>>>>    1 file changed, 11 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
>>>> index 760f42192..5218f1b12 100644
>>>> --- a/lib/librte_vhost/vhost.h
>>>> +++ b/lib/librte_vhost/vhost.h
>>>> @@ -219,13 +219,6 @@ struct vhost_msg {
>>>>    
>>>>    #define VIRTIO_F_RING_PACKED 34
>>>>    
>>>> -#define VRING_DESC_F_NEXT	1
>>>> -#define VRING_DESC_F_WRITE	2
>>>> -#define VRING_DESC_F_INDIRECT	4
>>>
>>> Why these are not re-defined below? Not used?
>>
>> These defines are in kernel headers since the beginning of virtio
>> support, so there are no reasons to keep them (and they aren't packed-
>> ring specific).
>>
>> I can let them if you prefer, it does not hurt but shouldn't be here.
> 
> Not required, I just want to confirm nothing missed.
> 
>>
>>>> -
>>>> -#define VRING_DESC_F_AVAIL	(1ULL << 7)
>>>> -#define VRING_DESC_F_USED	(1ULL << 15)
>>>> -
>>>>    struct vring_packed_desc {
>>>>    	uint64_t addr;
>>>>    	uint32_t len;
>>>> @@ -233,16 +226,23 @@ struct vring_packed_desc {
>>>>    	uint16_t flags;
>>>>    };
>>>>    
>>>> -#define VRING_EVENT_F_ENABLE 0x0
>>>> -#define VRING_EVENT_F_DISABLE 0x1
>>>> -#define VRING_EVENT_F_DESC 0x2
>>>> -
>>>>    struct vring_packed_desc_event {
>>>>    	uint16_t off_wrap;
>>>>    	uint16_t flags;
>>>>    };
>>>>    #endif
>>>>    
>>>> +/*
>>>> + * Declare below packed ring defines unconditionally
>>>> + * as Kernel header might use different names.
>>>> + */
>>>> +#define VRING_DESC_F_AVAIL	(1ULL << 7)
>>>> +#define VRING_DESC_F_USED	(1ULL << 15)
>>>> +
>>>> +#define VRING_EVENT_F_ENABLE 0x0
>>>> +#define VRING_EVENT_F_DISABLE 0x1
>>>> +#define VRING_EVENT_F_DESC 0x2
>>>
>>> What if Linux changes mind and uses old names again, build will fail again. If
>>> related part in Linux is not released yet, what do you think being on safe side
>>> and adding these defines with "#ifndef" wrap?
>>
>> There are the "old" ones.
>> In any case it will work (I tested it).
> 
> I see they are the old ones but is there any chance that kernel uses them again
> (for some odd reason) ? Which can break the build. Should we add a protection?

Yes, there is a chance, but that would not break build, it will just
override it.

> 
>>
>> In a future release, I plan to get rid of this dependency with Kernel
>> which only causes problems.
>>
>> There was a benefit at the beginning when all needed declarations where
>> in the Kernel headers. But now, every time we add a new virtio feature,
>> we have to check kernel supports it and if not define it.
>>
>> Thanks,
>> Maxime
>>>> +
>>>>    /*
>>>>     * Available and used descs are in same order
>>>>     */
>>>>
>>>
>
Ferruh Yigit Nov. 22, 2018, 10:04 p.m. UTC | #5 
On 11/22/2018 7:50 PM, Maxime Coquelin wrote:
> 
> 
> On 11/22/18 8:20 PM, Ferruh Yigit wrote:
>> On 11/22/2018 6:51 PM, Maxime Coquelin wrote:
>>>
>>>
>>> On 11/22/18 7:23 PM, Ferruh Yigit wrote:
>>>> On 11/22/2018 5:09 PM, Maxime Coquelin wrote:
>>>>> The packed ring defines were declared only if kernel
>>>>> header does not declare them.
>>>>> The problem is that they are not applied in upstream kernel,
>>>>> and some changes in the names have been required.
>>>>>
>>>>> This patch declares the defines unconditionally, which
>>>>> fixes potential build issues.
>>>>
>>>> +1 to address possible build issues.
>>>>
>>>>>
>>>>> Fixes: 297b1e7350f6 ("vhost: add virtio packed virtqueue defines")
>>>>> Cc: stable@dpdk.org
>>>>>
>>>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>>> ---
>>>>>    lib/librte_vhost/vhost.h | 22 +++++++++++-----------
>>>>>    1 file changed, 11 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
>>>>> index 760f42192..5218f1b12 100644
>>>>> --- a/lib/librte_vhost/vhost.h
>>>>> +++ b/lib/librte_vhost/vhost.h
>>>>> @@ -219,13 +219,6 @@ struct vhost_msg {
>>>>>    
>>>>>    #define VIRTIO_F_RING_PACKED 34
>>>>>    
>>>>> -#define VRING_DESC_F_NEXT	1
>>>>> -#define VRING_DESC_F_WRITE	2
>>>>> -#define VRING_DESC_F_INDIRECT	4
>>>>
>>>> Why these are not re-defined below? Not used?
>>>
>>> These defines are in kernel headers since the beginning of virtio
>>> support, so there are no reasons to keep them (and they aren't packed-
>>> ring specific).
>>>
>>> I can let them if you prefer, it does not hurt but shouldn't be here.
>>
>> Not required, I just want to confirm nothing missed.
>>
>>>
>>>>> -
>>>>> -#define VRING_DESC_F_AVAIL	(1ULL << 7)
>>>>> -#define VRING_DESC_F_USED	(1ULL << 15)
>>>>> -
>>>>>    struct vring_packed_desc {
>>>>>    	uint64_t addr;
>>>>>    	uint32_t len;
>>>>> @@ -233,16 +226,23 @@ struct vring_packed_desc {
>>>>>    	uint16_t flags;
>>>>>    };
>>>>>    
>>>>> -#define VRING_EVENT_F_ENABLE 0x0
>>>>> -#define VRING_EVENT_F_DISABLE 0x1
>>>>> -#define VRING_EVENT_F_DESC 0x2
>>>>> -
>>>>>    struct vring_packed_desc_event {
>>>>>    	uint16_t off_wrap;
>>>>>    	uint16_t flags;
>>>>>    };
>>>>>    #endif
>>>>>    
>>>>> +/*
>>>>> + * Declare below packed ring defines unconditionally
>>>>> + * as Kernel header might use different names.
>>>>> + */
>>>>> +#define VRING_DESC_F_AVAIL	(1ULL << 7)
>>>>> +#define VRING_DESC_F_USED	(1ULL << 15)
>>>>> +
>>>>> +#define VRING_EVENT_F_ENABLE 0x0
>>>>> +#define VRING_EVENT_F_DISABLE 0x1
>>>>> +#define VRING_EVENT_F_DESC 0x2
>>>>
>>>> What if Linux changes mind and uses old names again, build will fail again. If
>>>> related part in Linux is not released yet, what do you think being on safe side
>>>> and adding these defines with "#ifndef" wrap?
>>>
>>> There are the "old" ones.
>>> In any case it will work (I tested it).
>>
>> I see they are the old ones but is there any chance that kernel uses them again
>> (for some odd reason) ? Which can break the build. Should we add a protection?
> 
> Yes, there is a chance, but that would not break build, it will just
> override it.

It will give redefined warning if values are different but right, won't cause
any issue with same number.

> 
>>
>>>
>>> In a future release, I plan to get rid of this dependency with Kernel
>>> which only causes problems.
>>>
>>> There was a benefit at the beginning when all needed declarations where
>>> in the Kernel headers. But now, every time we add a new virtio feature,
>>> we have to check kernel supports it and if not define it.
>>>
>>> Thanks,
>>> Maxime
>>>>> +
>>>>>    /*
>>>>>     * Available and used descs are in same order
>>>>>     */
>>>>>
>>>>
>>
Ferruh Yigit Nov. 22, 2018, 10:14 p.m. UTC | #6 
On 11/22/2018 5:09 PM, Maxime Coquelin wrote:
> The packed ring defines were declared only if kernel
> header does not declare them.
> The problem is that they are not applied in upstream kernel,
> and some changes in the names have been required.
> 
> This patch declares the defines unconditionally, which
> fixes potential build issues.
> 
> Fixes: 297b1e7350f6 ("vhost: add virtio packed virtqueue defines")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Applied to dpdk-next-net/master, thanks.
diff mbox series

Patch

diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
index 760f42192..5218f1b12 100644
--- a/lib/librte_vhost/vhost.h
+++ b/lib/librte_vhost/vhost.h
@@ -219,13 +219,6 @@  struct vhost_msg {
 
 #define VIRTIO_F_RING_PACKED 34
 
-#define VRING_DESC_F_NEXT	1
-#define VRING_DESC_F_WRITE	2
-#define VRING_DESC_F_INDIRECT	4
-
-#define VRING_DESC_F_AVAIL	(1ULL << 7)
-#define VRING_DESC_F_USED	(1ULL << 15)
-
 struct vring_packed_desc {
 	uint64_t addr;
 	uint32_t len;
@@ -233,16 +226,23 @@  struct vring_packed_desc {
 	uint16_t flags;
 };
 
-#define VRING_EVENT_F_ENABLE 0x0
-#define VRING_EVENT_F_DISABLE 0x1
-#define VRING_EVENT_F_DESC 0x2
-
 struct vring_packed_desc_event {
 	uint16_t off_wrap;
 	uint16_t flags;
 };
 #endif
 
+/*
+ * Declare below packed ring defines unconditionally
+ * as Kernel header might use different names.
+ */
+#define VRING_DESC_F_AVAIL	(1ULL << 7)
+#define VRING_DESC_F_USED	(1ULL << 15)
+
+#define VRING_EVENT_F_ENABLE 0x0
+#define VRING_EVENT_F_DISABLE 0x1
+#define VRING_EVENT_F_DESC 0x2
+
 /*
  * Available and used descs are in same order
  */