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

[Michael S. Tsirkin]

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?

-- 
MST