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

[Maxime Coquelin]

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.

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.

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