Re: [Qemu-devel] [PATCH 3/6] vhost: Update rings information for IOTLB earlier

[Michael S. Tsirkin]

On Fri, May 12, 2017 at 01:21:18PM +0200, Maxime Coquelin wrote:
> 
> 
> On 05/11/2017 07:33 PM, Michael S. Tsirkin wrote:
> > On Thu, May 11, 2017 at 02:32:43PM +0200, Maxime Coquelin wrote:
> > > Vhost-kernel backend need to receive IOTLB entries for rings
> > > information early, but vhost-user need the same information
> > > earlier, before VHOST_USER_SET_VRING_ADDR is sent.
> > 
> > Weird. What does VHOST_USER_SET_VRING_ADDR have to do with it?
> > 
> > According to
> >     Starting and stopping rings
> > in vhost user spec, vhost user does not access
> > anything until ring is started and enabled.
> > 
> > 
> > > This patch also trigger IOTLB miss for all rings informations
> > > for robustness, even if in practice these adresses are on the
> > > same page.
> 
> Actually, the DPDK vhost-user backend is compliant with the spec,
> but when handling VHOST_USER_SET_VRING_ADDR request, it translates the
> guest addresses into backend VAs, and check they are valid. I will make the
> commit message clearer about this in next revision.
> 
> The check could be done later, for example when the ring are started,
> but it wouldn't change the need to trigger a miss at some point.

I think it should be done later, yes. As long as ring is not
started addresses should not be interpreted.

> Or it could be done in the processing threads, the first time the
> addresses are used, but it would add a check for every burst of packets
> processed.

We don't want that, I agree.

> > > 
> > > Signed-off-by: Maxime Coquelin <address@hidden>
> > > ---
> > >   hw/virtio/vhost.c | 19 +++++++++++--------
> > >   1 file changed, 11 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > > index 748e331..817f6d0 100644
> > > --- a/hw/virtio/vhost.c
> > > +++ b/hw/virtio/vhost.c
> > > @@ -799,7 +799,17 @@ static int vhost_virtqueue_set_addr(struct vhost_dev 
> > > *dev,
> > >           .log_guest_addr = vq->used_phys,
> > >           .flags = enable_log ? (1 << VHOST_VRING_F_LOG) : 0,
> > >       };
> > > -    int r = dev->vhost_ops->vhost_set_vring_addr(dev, &addr);
> > > +    int r;
> > > +
> > > +    /* Update rings information for IOTLB to work correctly,
> > > +     * vhost-kernel & vhost-user backends require for this.*/
> > 
> > Any requirements must be in the spec. Pls add them there.
> > Pls fix comment style as you move code.
> Sure.
> 
> Thanks,
> Maxime