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

[Michael S. Tsirkin]

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?

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

> 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?  BTW I don't really know why do we do the write
when event index is setup.  We probably shouldn't, should we?

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?

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.

-- 
MST