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

["Xie, Huawei" <huawei.xie@intel.com>]

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.

>>>> 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
>>>
>>>
>>>
>>>
>
>