Re: [RFC v2 12/13] vhost: Check for device VRING_USED_F_NO_NOTIFY at shadow virtqueue kick

[Eugenio Perez Martin]

On Tue, Mar 16, 2021 at 9:07 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 在 2021/3/16 上午3:48, Eugenio Pérez 写道:
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >   hw/virtio/vhost-shadow-virtqueue.c | 11 ++++++++++-
> >   1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/virtio/vhost-shadow-virtqueue.c 
> > b/hw/virtio/vhost-shadow-virtqueue.c
> > index 68ed0f2740..7df98fc43f 100644
> > --- a/hw/virtio/vhost-shadow-virtqueue.c
> > +++ b/hw/virtio/vhost-shadow-virtqueue.c
> > @@ -145,6 +145,15 @@ static void vhost_shadow_vq_add(VhostShadowVirtqueue 
> > *svq,
> >       svq->ring_id_maps[qemu_head] = elem;
> >   }
> >
> > +static void vhost_shadow_vq_kick(VhostShadowVirtqueue *svq)
> > +{
> > +    /* Make sure we are reading updated device flag */
> > +    smp_rmb();
>
>
> smp_mb() actually? Or it's better to explain this following read needs
> to be orderd with what read before.
>
> Thanks
>

Sorry for the late reply, I moved to vhost-vdpa usage of SVQ and I
missed these comments.

My intentions were just to order the reading of used ring flags. In
other words, to avoid reading an old value in the next conditional.

Descriptors themselves should be already written because
vhost_shadow_vq_add_split just calls smp_wmb almost at the end of the
execution. avail_ring->idx is not protected by it though. Is that what
you meant about turning it to a full barrier?

Maybe it's clearer just to call smp_mb() between the calls to
vhost_shadow_vq_add_split and vhost_shadow_vq_kick, merging both
memory barriers?

Thanks!

>
> > +    if (!(svq->vring.used->flags & VRING_USED_F_NO_NOTIFY)) {
> > +        event_notifier_set(&svq->kick_notifier);
> > +    }
> > +}
> > +
> >   /* Handle guest->device notifications */
> >   static void vhost_handle_guest_kick(EventNotifier *n)
> >   {
> > @@ -174,7 +183,7 @@ static void vhost_handle_guest_kick(EventNotifier *n)
> >               }
> >
> >               vhost_shadow_vq_add(svq, elem);
> > -            event_notifier_set(&svq->kick_notifier);
> > +            vhost_shadow_vq_kick(svq);
> >           }
> >
> >           virtio_queue_set_notification(svq->vq, true);
>