Re: [dpdk-dev] [PATCH] virtio: fix the vq size issue

["Ouyang, Changchun" <changchun.ouyang@intel.com>]

> -----Original Message-----
> From: Xie, Huawei
> Sent: Thursday, July 2, 2015 5:16 PM
> To: Ouyang, Changchun
> Cc: dev@dpdk.org; Thomas Monjalon
> Subject: Re: [dpdk-dev] [PATCH] virtio: fix the vq size issue
> 
> On 7/2/2015 10:16 AM, Ouyang, Changchun wrote:
> >
> >> -----Original Message-----
> >> From: Xie, Huawei
> >> Sent: Thursday, July 2, 2015 10:02 AM
> >> To: Ouyang, Changchun; dev@dpdk.org; Thomas Monjalon
> >> Subject: Re: [dpdk-dev] [PATCH] virtio: fix the vq size issue
> >>
> >> On 7/2/2015 8:29 AM, Ouyang, Changchun wrote:
> >>> Hi huawei,
> >>>
> >>>> -----Original Message-----
> >>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Xie, Huawei
> >>>> Sent: Wednesday, July 1, 2015 11:53 PM
> >>>> To: dev@dpdk.org; Thomas Monjalon
> >>>> Subject: Re: [dpdk-dev] [PATCH] virtio: fix the vq size issue
> >>>>
> >>>> On 7/1/2015 3:49 PM, Ouyang Changchun wrote:
> >>>>> This commit breaks virtio basic packets rx functionality:
> >>>>>   d78deadae4dca240e85054bf2d604a801676becc
> >>>>>
> >>>>> The QEMU use 256 as default vring size, also use this default
> >>>>> value to calculate the virtio avail ring base address and used
> >>>>> ring base address, and vhost in the backend use the ring base
> >>>>> address to do packet
> >>>> IO.
> >>>>> Virtio spec also says the queue size in PCI configuration is
> >>>>> read-only, so virtio front end can't change it. just need use the
> >>>>> read-only value to allocate space for vring and calculate the
> >>>>> avail and used ring base address. Otherwise, the avail and used
> >>>>> ring base
> >>>> address will be different between host and guest, accordingly,
> >>>> packet IO can't work normally.
> >>>> virtio driver could still use the vq_size to initialize avail ring
> >>>> and use ring so that they still have the same base address.
> >>>> The other issue is vhost use  index & (vq->size -1) to index the ring.
> >>> I am not sure what is your clear message here, Vhost has no choice
> >>> but use vq->size -1 to index the ring, It is qemu that always use
> >>> 256 as the vq size, and set the avail and used ring base address, It
> >>> also tells vhost the vq size is 256.
> >> I mean "the same base address issue" could be resolved, but we still
> >> couldn't stop vhost using idx & vq->size -1 to index the ring.
> >>
> > Then this patch will resolve this avail ring base address issue.
> I mean different ring base isn't the root cause. The commit message which
> states that this register is read only is simple and enough.	

The direct root cause is avail ring base address issue,
Virtio front end use: vring->avail = vring->desc + vq_size * SIZE_OF_DESC_ELEMENT,
And fill the vring->avail->avail_idx, and the ring itself.
Qemu use:  vring->avail = vring->desc + 256 * SIZE_OF_DESC_ELEMENT,
And tell vhost this address, Vhost use this address to enqueue packets from phy port  into vring.

Pls note that if vq_size is not 256, e.g. it is changed into 128, then the vring->avail in host and in guest
Is totally different, that is why it fail to rx any packet, because they try to use different address to get
Same content in that space.

This is why I still think it is the root cause and I need add it into the commit.

> 
> >>>> Thomas:
> >>>> This fix works but introduces slight change with original code.
> >>>> Could we just rollback that commit?
> >>> What's your major concern for the slight change here?
> >>> just removing the unnecessary check for nb_desc itself.
> >>> So I think no issue for the slight change.
> >> No major concern. It is better if this patch just rollbacks that
> >> commit without introduce extra change if not necessary.
> >> The original code set nb_desc to vq_size, though it isn't used later.
> >>
> > I prefer to have the slight change to remove unnecessary setting.
> >
> >>> Thanks
> >>> Changchun
> >>>
> >>>
> >>>
> >>>
> >
> >