[dpdk-dev] [PATCH] virtio: fix used ring address calculation

[dpdk-dev] [PATCH] virtio: fix used ring address calculation

[Huawei Xie]

used event idx is put at the end of available ring. It isn't taken into account
when we calculate the address of used ring. Fortunately, it doesn't introduce
the bug with fixed queue number 256 and 4KB alignment.

Signed-off-by: hxie5 <huawei.xie@intel.com>
---
 drivers/net/virtio/virtio_ring.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/virtio/virtio_ring.h b/drivers/net/virtio/virtio_ring.h
index a16c499..92e430d 100644
--- a/drivers/net/virtio/virtio_ring.h
+++ b/drivers/net/virtio/virtio_ring.h
@@ -145,7 +145,7 @@ vring_init(struct vring *vr, unsigned int num, uint8_t *p,
 	vr->avail = (struct vring_avail *) (p +
 		num * sizeof(struct vring_desc));
 	vr->used = (void *)
-		RTE_ALIGN_CEIL((uintptr_t)(&vr->avail->ring[num]), align);
+		RTE_ALIGN_CEIL((uintptr_t)(&vr->avail->ring[num + 1]), align);
 }
 
 /*
-- 
1.9.3

Re: [dpdk-dev] [PATCH] virtio: fix used ring address calculation

[Tan, Jianfeng]

-----Original Message-----
From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Huawei Xie
Sent: Monday, September 21, 2015 11:39 AM
To: dev@dpdk.org
Subject: [dpdk-dev] [PATCH] virtio: fix used ring address calculation

used event idx is put at the end of available ring. It isn't taken into account when we calculate the address of used ring. Fortunately, it doesn't introduce the bug with fixed queue number 256 and 4KB alignment.

Signed-off-by: hxie5 <huawei.xie@intel.com>
---
 drivers/net/virtio/virtio_ring.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/virtio/virtio_ring.h b/drivers/net/virtio/virtio_ring.h
index a16c499..92e430d 100644
--- a/drivers/net/virtio/virtio_ring.h
+++ b/drivers/net/virtio/virtio_ring.h
@@ -145,7 +145,7 @@ vring_init(struct vring *vr, unsigned int num, uint8_t *p,
 	vr->avail = (struct vring_avail *) (p +
 		num * sizeof(struct vring_desc));
 	vr->used = (void *)
-		RTE_ALIGN_CEIL((uintptr_t)(&vr->avail->ring[num]), align);
+		RTE_ALIGN_CEIL((uintptr_t)(&vr->avail->ring[num + 1]), align);
 }
 
 /*
--

Acked-by: Jianfeng Tan <Jianfeng.tan@intel.com>

1.9.3

Re: [dpdk-dev] [PATCH] virtio: fix used ring address calculation

[Xie, Huawei]

On 9/21/2015 11:39 AM, Xie, Huawei wrote:
vring_size calculation should consider both used_event_idx at the tail
of avail ring and avail_event_idx at the tail of used ring.
Will merge those two fixes and send a new patch.
> used event idx is put at the end of available ring. It isn't taken into account
> when we calculate the address of used ring. Fortunately, it doesn't introduce
> the bug with fixed queue number 256 and 4KB alignment.
>
> Signed-off-by: hxie5 <huawei.xie@intel.com>
> ---
>  drivers/net/virtio/virtio_ring.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/virtio/virtio_ring.h b/drivers/net/virtio/virtio_ring.h
> index a16c499..92e430d 100644
> --- a/drivers/net/virtio/virtio_ring.h
> +++ b/drivers/net/virtio/virtio_ring.h
> @@ -145,7 +145,7 @@ vring_init(struct vring *vr, unsigned int num, uint8_t *p,
>  	vr->avail = (struct vring_avail *) (p +
>  		num * sizeof(struct vring_desc));
>  	vr->used = (void *)
> -		RTE_ALIGN_CEIL((uintptr_t)(&vr->avail->ring[num]), align);
> +		RTE_ALIGN_CEIL((uintptr_t)(&vr->avail->ring[num + 1]), align);
>  }
>  
>  /*

Re: [dpdk-dev] [PATCH] virtio: fix used ring address calculation

[Stephen Hemminger]

On Thu, 24 Sep 2015 07:30:41 +0000
"Xie, Huawei" <huawei.xie@intel.com> wrote:

> On 9/21/2015 11:39 AM, Xie, Huawei wrote:
> vring_size calculation should consider both used_event_idx at the tail
> of avail ring and avail_event_idx at the tail of used ring.
> Will merge those two fixes and send a new patch.
> > used event idx is put at the end of available ring. It isn't taken into account
> > when we calculate the address of used ring. Fortunately, it doesn't introduce
> > the bug with fixed queue number 256 and 4KB alignment.
> >
> > Signed-off-by: hxie5 <huawei.xie@intel.com>
> > ---
> >  drivers/net/virtio/virtio_ring.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/virtio/virtio_ring.h b/drivers/net/virtio/virtio_ring.h
> > index a16c499..92e430d 100644
> > --- a/drivers/net/virtio/virtio_ring.h
> > +++ b/drivers/net/virtio/virtio_ring.h
> > @@ -145,7 +145,7 @@ vring_init(struct vring *vr, unsigned int num, uint8_t *p,
> >  	vr->avail = (struct vring_avail *) (p +
> >  		num * sizeof(struct vring_desc));
> >  	vr->used = (void *)
> > -		RTE_ALIGN_CEIL((uintptr_t)(&vr->avail->ring[num]), align);
> > +		RTE_ALIGN_CEIL((uintptr_t)(&vr->avail->ring[num + 1]), align);
> >  }
> >  
> >  /*
> 

Why aren't we just using the standard Linux includes for this?
See <linux/virtio_ring.h> and the function vring_init()

Keeping parallel copies of headers is prone to failures.

Re: [dpdk-dev] [PATCH] virtio: fix used ring address calculation

[Xie, Huawei]

On 9/25/2015 12:36 AM, Stephen Hemminger wrote:
> On Thu, 24 Sep 2015 07:30:41 +0000
> "Xie, Huawei" <huawei.xie@intel.com> wrote:
>
>> On 9/21/2015 11:39 AM, Xie, Huawei wrote:
>> vring_size calculation should consider both used_event_idx at the tail
>> of avail ring and avail_event_idx at the tail of used ring.
>> Will merge those two fixes and send a new patch.
>>> used event idx is put at the end of available ring. It isn't taken into account
>>> when we calculate the address of used ring. Fortunately, it doesn't introduce
>>> the bug with fixed queue number 256 and 4KB alignment.
>>>
>>> Signed-off-by: hxie5 <huawei.xie@intel.com>
>>> ---
>>>  drivers/net/virtio/virtio_ring.h | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/virtio/virtio_ring.h b/drivers/net/virtio/virtio_ring.h
>>> index a16c499..92e430d 100644
>>> --- a/drivers/net/virtio/virtio_ring.h
>>> +++ b/drivers/net/virtio/virtio_ring.h
>>> @@ -145,7 +145,7 @@ vring_init(struct vring *vr, unsigned int num, uint8_t *p,
>>>  	vr->avail = (struct vring_avail *) (p +
>>>  		num * sizeof(struct vring_desc));
>>>  	vr->used = (void *)
>>> -		RTE_ALIGN_CEIL((uintptr_t)(&vr->avail->ring[num]), align);
>>> +		RTE_ALIGN_CEIL((uintptr_t)(&vr->avail->ring[num + 1]), align);
>>>  }
>>>  
>>>  /*
> Why aren't we just using the standard Linux includes for this?
> See <linux/virtio_ring.h> and the function vring_init()
>
> Keeping parallel copies of headers is prone to failures.
Agree.
Using standard Linux includes then at least we don't need to redefine
the feature and other related MACRO.
This applies to vhost as well.
For vring, vring_init, we could also reuse the linux implementation
unless we have strong reason to define our own structure.
One reason was to support both FreeBSD and Linux. FreeBSD should have
its own header file. To avoid the case they have different vring
structure or VIRTIO_F_xx macro name, they are redefined here.

>

Re: [dpdk-dev] [PATCH] virtio: fix used ring address calculation

[Stephen Hemminger]

On Thu, 24 Sep 2015 18:35:37 +0000
"Xie, Huawei" <huawei.xie@intel.com> wrote:

> On 9/25/2015 12:36 AM, Stephen Hemminger wrote:
> > On Thu, 24 Sep 2015 07:30:41 +0000
> > "Xie, Huawei" <huawei.xie@intel.com> wrote:
> >
> >> On 9/21/2015 11:39 AM, Xie, Huawei wrote:
> >> vring_size calculation should consider both used_event_idx at the tail
> >> of avail ring and avail_event_idx at the tail of used ring.
> >> Will merge those two fixes and send a new patch.
> >>> used event idx is put at the end of available ring. It isn't taken into account
> >>> when we calculate the address of used ring. Fortunately, it doesn't introduce
> >>> the bug with fixed queue number 256 and 4KB alignment.
> >>>
> >>> Signed-off-by: hxie5 <huawei.xie@intel.com>
> >>> ---
> >>>  drivers/net/virtio/virtio_ring.h | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/net/virtio/virtio_ring.h b/drivers/net/virtio/virtio_ring.h
> >>> index a16c499..92e430d 100644
> >>> --- a/drivers/net/virtio/virtio_ring.h
> >>> +++ b/drivers/net/virtio/virtio_ring.h
> >>> @@ -145,7 +145,7 @@ vring_init(struct vring *vr, unsigned int num, uint8_t *p,
> >>>  	vr->avail = (struct vring_avail *) (p +
> >>>  		num * sizeof(struct vring_desc));
> >>>  	vr->used = (void *)
> >>> -		RTE_ALIGN_CEIL((uintptr_t)(&vr->avail->ring[num]), align);
> >>> +		RTE_ALIGN_CEIL((uintptr_t)(&vr->avail->ring[num + 1]), align);
> >>>  }
> >>>  
> >>>  /*
> > Why aren't we just using the standard Linux includes for this?
> > See <linux/virtio_ring.h> and the function vring_init()
> >
> > Keeping parallel copies of headers is prone to failures.
> Agree.
> Using standard Linux includes then at least we don't need to redefine
> the feature and other related MACRO.
> This applies to vhost as well.
> For vring, vring_init, we could also reuse the linux implementation
> unless we have strong reason to define our own structure.
> One reason was to support both FreeBSD and Linux. FreeBSD should have
> its own header file. To avoid the case they have different vring
> structure or VIRTIO_F_xx macro name, they are redefined here.
> 
> >
> 

The Linux headers for virtio are explictly BSD licensed.
You could at least just have a local copy of same code.

Re: [dpdk-dev] [PATCH] virtio: fix used ring address calculation

[Xie, Huawei]

On 9/25/2015 5:01 AM, Stephen Hemminger wrote:
> On Thu, 24 Sep 2015 18:35:37 +0000
> "Xie, Huawei" <huawei.xie@intel.com> wrote:
>
>> On 9/25/2015 12:36 AM, Stephen Hemminger wrote:
>>> On Thu, 24 Sep 2015 07:30:41 +0000
>>> "Xie, Huawei" <huawei.xie@intel.com> wrote:
>>>
>>>> On 9/21/2015 11:39 AM, Xie, Huawei wrote:
>>>> vring_size calculation should consider both used_event_idx at the tail
>>>> of avail ring and avail_event_idx at the tail of used ring.
>>>> Will merge those two fixes and send a new patch.
>>>>> used event idx is put at the end of available ring. It isn't taken into account
>>>>> when we calculate the address of used ring. Fortunately, it doesn't introduce
>>>>> the bug with fixed queue number 256 and 4KB alignment.
>>>>>
>>>>> Signed-off-by: hxie5 <huawei.xie@intel.com>
>>>>> ---
>>>>>  drivers/net/virtio/virtio_ring.h | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/net/virtio/virtio_ring.h b/drivers/net/virtio/virtio_ring.h
>>>>> index a16c499..92e430d 100644
>>>>> --- a/drivers/net/virtio/virtio_ring.h
>>>>> +++ b/drivers/net/virtio/virtio_ring.h
>>>>> @@ -145,7 +145,7 @@ vring_init(struct vring *vr, unsigned int num, uint8_t *p,
>>>>>  	vr->avail = (struct vring_avail *) (p +
>>>>>  		num * sizeof(struct vring_desc));
>>>>>  	vr->used = (void *)
>>>>> -		RTE_ALIGN_CEIL((uintptr_t)(&vr->avail->ring[num]), align);
>>>>> +		RTE_ALIGN_CEIL((uintptr_t)(&vr->avail->ring[num + 1]), align);
>>>>>  }
>>>>>  
>>>>>  /*
>>> Why aren't we just using the standard Linux includes for this?
>>> See <linux/virtio_ring.h> and the function vring_init()
>>>
>>> Keeping parallel copies of headers is prone to failures.
>> Agree.
>> Using standard Linux includes then at least we don't need to redefine
>> the feature and other related MACRO.
>> This applies to vhost as well.
>> For vring, vring_init, we could also reuse the linux implementation
>> unless we have strong reason to define our own structure.
>> One reason was to support both FreeBSD and Linux. FreeBSD should have
>> its own header file. To avoid the case they have different vring
>> structure or VIRTIO_F_xx macro name, they are redefined here.
>>
> The Linux headers for virtio are explictly BSD licensed.
> You could at least just have a local copy of same code.
>
Exactly the same code (if no dependency and no other issue) or copy and
convert it to DPDK style? By DPDK style, i mean like using RTE_ALIGN macro.

Re: [dpdk-dev] [PATCH] virtio: fix used ring address calculation

[Stephen Hemminger]

On Fri, 25 Sep 2015 15:46:34 +0000
"Xie, Huawei" <huawei.xie@intel.com> wrote:

> On 9/25/2015 5:01 AM, Stephen Hemminger wrote:
> > On Thu, 24 Sep 2015 18:35:37 +0000
> > "Xie, Huawei" <huawei.xie@intel.com> wrote:
> >
> >> On 9/25/2015 12:36 AM, Stephen Hemminger wrote:
> >>> On Thu, 24 Sep 2015 07:30:41 +0000
> >>> "Xie, Huawei" <huawei.xie@intel.com> wrote:
> >>>
> >>>> On 9/21/2015 11:39 AM, Xie, Huawei wrote:
> >>>> vring_size calculation should consider both used_event_idx at the tail
> >>>> of avail ring and avail_event_idx at the tail of used ring.
> >>>> Will merge those two fixes and send a new patch.
> >>>>> used event idx is put at the end of available ring. It isn't taken into account
> >>>>> when we calculate the address of used ring. Fortunately, it doesn't introduce
> >>>>> the bug with fixed queue number 256 and 4KB alignment.
> >>>>>
> >>>>> Signed-off-by: hxie5 <huawei.xie@intel.com>
> >>>>> ---
> >>>>>  drivers/net/virtio/virtio_ring.h | 2 +-
> >>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/drivers/net/virtio/virtio_ring.h b/drivers/net/virtio/virtio_ring.h
> >>>>> index a16c499..92e430d 100644
> >>>>> --- a/drivers/net/virtio/virtio_ring.h
> >>>>> +++ b/drivers/net/virtio/virtio_ring.h
> >>>>> @@ -145,7 +145,7 @@ vring_init(struct vring *vr, unsigned int num, uint8_t *p,
> >>>>>  	vr->avail = (struct vring_avail *) (p +
> >>>>>  		num * sizeof(struct vring_desc));
> >>>>>  	vr->used = (void *)
> >>>>> -		RTE_ALIGN_CEIL((uintptr_t)(&vr->avail->ring[num]), align);
> >>>>> +		RTE_ALIGN_CEIL((uintptr_t)(&vr->avail->ring[num + 1]), align);
> >>>>>  }
> >>>>>  
> >>>>>  /*
> >>> Why aren't we just using the standard Linux includes for this?
> >>> See <linux/virtio_ring.h> and the function vring_init()
> >>>
> >>> Keeping parallel copies of headers is prone to failures.
> >> Agree.
> >> Using standard Linux includes then at least we don't need to redefine
> >> the feature and other related MACRO.
> >> This applies to vhost as well.
> >> For vring, vring_init, we could also reuse the linux implementation
> >> unless we have strong reason to define our own structure.
> >> One reason was to support both FreeBSD and Linux. FreeBSD should have
> >> its own header file. To avoid the case they have different vring
> >> structure or VIRTIO_F_xx macro name, they are redefined here.
> >>
> > The Linux headers for virtio are explictly BSD licensed.
> > You could at least just have a local copy of same code.
> >
> Exactly the same code (if no dependency and no other issue) or copy and
> convert it to DPDK style? By DPDK style, i mean like using RTE_ALIGN macro.

No. keep the Linux code as is. Just copy the headers.
Don't introduce DPDK style.