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

[Jason Wang]

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.

For simplicity, I don't implement control path device IOTLB miss. If you 
care about the incomplete length, we can refine vhost_iotlb_miss() to 
make sure it covers all size.

Thanks