Re: [Qemu-devel] [PATCH v2 0/6] vhost-user: Specify and implement device IOTLB support

[Jason Wang <jasowang@redhat.com>]

On 2017年06月02日 23:24, Michael S. Tsirkin wrote:
> On Fri, Jun 02, 2017 at 01:53:11PM +0800, Jason Wang wrote:
>>
>> On 2017年06月01日 21:59, Michael S. Tsirkin wrote:
>>> On Wed, May 31, 2017 at 04:33:33PM +0800, Jason Wang wrote:
>>>> On 2017年05月31日 02:20, Michael S. Tsirkin wrote:
>>>>> On Fri, May 26, 2017 at 04:28:52PM +0200, Maxime Coquelin wrote:
>>>>>> This series aims at specifying ans implementing the protocol update
>>>>>> required to support device IOTLB with user backends.
>>>>>>
>>>>>> In this second non-RFC version, main changes are:
>>>>>>     - spec fixes and clarification
>>>>>>     - rings information update has been restored back to ring enablement time
>>>>>>     - Work around GCC 4.4.7 limitation wrt assignment in unnamed union at
>>>>>> declaration time.
>>>>>>
>>>>>> The series can be tested with vhost_iotlb_proto_v2 branch on my gitlab
>>>>>> account[0].
>>>>>>
>>>>>> The slave requests channel part is re-used from Marc-André's series submitted
>>>>>> last year[1], with main changes from original version being request/feature
>>>>>> names renaming and addition of the REPLY_ACK feature support.
>>>>>>
>>>>>> Regarding IOTLB protocol, one noticeable change is the IOTLB miss request
>>>>>> reply made optionnal (i.e. only if slave requests it by setting the
>>>>>> VHOST_USER_NEED_REPLY flag in the message header). This change provides
>>>>>> more flexibility in the backend implementation of the feature.
>>>>>>
>>>>>> The protocol is very close to kernel backends, except that a new
>>>>>> communication channel is introduced to enable the slave to send
>>>>>> requests to the master.
>>>>>>
>>>>>> [0]:https://gitlab.com/mcoquelin/dpdk-next-virtio/commits/vhost_iotlb_proto_v2
>>>>>> [1]:https://lists.gnu.org/archive/html/qemu-devel/2016-04/msg00095.html
>>>>> Overall, this looks good to me. I do think patch 3 isn't a good idea
>>>>> though, if slave wants something let it request it.
>>>>>
>>>>> Need to find out why does vhost in kernel want the used ring iotlb at
>>>>> start time - especially considering we aren't even guaranteed one entry
>>>>> covers the whole ring, and invalidates should affect all addresses at
>>>>> least in theory.
>>>>>
>>>>>
>>>> The reason is probably we want to verify whether or not we could correctly
>>>> access used ring in vhost_vq_init_access(). It was there since vhost_net is
>>>> introduced. We can think to remove this limitation maybe.
>>>>
>>>> Thanks
>>> Well that's only called if iotlb is disabled:
>>>
>>>           if (!vq->iotlb &&
>>>               !access_ok(VERIFY_READ, &vq->used->idx, sizeof vq->used->idx)) {
>>>                   r = -EFAULT;
>>>                   goto err;
>>>           }
>>>
>>> Could you try removing that and see what breaks?
>>>
>> Looks not, the issue is vhost_update_used_flags() which needs device IOTLB
>> translation. If we don't fill IOTLB in advance, it will return -EFAULT.
> Same for vhost_get_used, right?

Yes.

>
>> For simplicity, I don't implement control path device IOTLB miss.
>
> OK so this should be documented in vhost.h.  SET_BACKEND immediately
> writes and reads used ring. User must know this and pre-fault used flags
> and index before setting backend.

Ok.

>> If you
>> care about the incomplete length, we can refine vhost_iotlb_miss() to make
>> sure it covers all size.
>>
>> Thanks
> No need imho, it's only the used flags field that's written, and the
> index that's read right?

Yes.

>    BTW I don't really know why do we do the write
> when event index is setup.  We probably shouldn't, should we?

Yes, looks like we shouldn't.

>
> It's worth considering whether we want this write into used ring at all.
> I put it there originally to help make sure we don't miss the first exit, but
> event index seems to get by fine without this. So why does non event
> index code want it?

Spec said driver must initialize it to zero, so unless we want to 
workaround a buggy driver we can remove this.

>
> I think that if we could get rid of both accesses, it would be
> nice. Would need a feature bit naturally and we'd need to
> support old kernels but at least it will be contained and
> well documented.
>

Technically we can, but we still need a workaround for old kernels. So 
I'm not sure it's worth to do.

Thanks