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

[Jason Wang]

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