Re: [PATCH] vhost-vdpa: check vhost_vdpa_set_vring_ready() return value

[Eugenio Perez Martin]

On Mon, Mar 18, 2024 at 5:35 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Fri, Mar 15, 2024 at 4:23 PM Stefano Garzarella <sgarzare@redhat.com> 
> wrote:
> >
> > On Thu, Mar 14, 2024 at 11:17:01AM +0800, Jason Wang wrote:
> > >On Wed, Feb 7, 2024 at 5:27 PM Stefano Garzarella <sgarzare@redhat.com> 
> > >wrote:
> > >>
> > >> vhost_vdpa_set_vring_ready() could already fail, but if Linux's
> > >> patch [1] will be merged, it may fail with more chance if
> > >> userspace does not activate virtqueues before DRIVER_OK when
> > >> VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is not negotiated.
> > >
> > >I wonder what happens if we just leave it as is.
> >
> > Are you referring to this patch or the kernel patch?
>
> This patch.
>
> >
> > Here I'm just checking the return value of vhost_vdpa_set_vring_ready().
> > It can return an error also without that kernel patch, so IMHO is
> > better to check the return value here in QEMU.
> >
> > What issue do you see with this patch applied?
>
> For the parent which can enable after driver_ok but not advertise it.
>
> (To say the truth, I'm not sure if we need to care about this)
>
> >
> > >
> > >VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK: We do know enabling could be
> > >done after driver_ok.
> > >Without VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK: We don't know whether
> > >enabling could be done after driver_ok or not.
> >
> > I see your point, indeed I didn't send a v2 of that patch.
> > Maybe we should document that, because it could be interpreted that if
> > VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is not negotiated the enabling
> > should always be done before driver_ok (which is true for example in
> > VDUSE).
>
> I see, so I think we probably need the fix.
>

A more complete fix is proper checking for
VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK. But even with that, checking
for failure in the ioctl is also worth it, isn't it?

The only point I see to not checking is to closely mimic virtio
behavior, but vDPA can already fail here.

> >
> > BTW I think we should discuss it in the kernel patch.
> >
> > Thanks,
> > Stefano
> >
> > >
> > >Thanks
> > >
> > >>
> > >> So better check its return value anyway.
> > >>
> > >> [1] 
> > >> https://lore.kernel.org/virtualization/20240206145154.118044-1-sgarzare@redhat.com/T/#u
> > >>
> > >> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > >> ---
> > >> Note: This patch conflicts with [2], but the resolution is simple,
> > >> so for now I sent a patch for the current master, but I'll rebase
> > >> this patch if we merge the other one first.
>
> Will go through [2].
>
> Thanks
>
>
> > >>
> > >> [2]
> > >> https://lore.kernel.org/qemu-devel/20240202132521.32714-1-kwolf@redhat.com/
> > >> ---
> > >>  hw/virtio/vdpa-dev.c |  8 +++++++-
> > >>  net/vhost-vdpa.c     | 15 ++++++++++++---
> > >>  2 files changed, 19 insertions(+), 4 deletions(-)
> > >>
> > >> diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
> > >> index eb9ecea83b..d57cd76c18 100644
> > >> --- a/hw/virtio/vdpa-dev.c
> > >> +++ b/hw/virtio/vdpa-dev.c
> > >> @@ -259,7 +259,11 @@ static int vhost_vdpa_device_start(VirtIODevice 
> > >> *vdev, Error **errp)
> > >>          goto err_guest_notifiers;
> > >>      }
> > >>      for (i = 0; i < s->dev.nvqs; ++i) {
> > >> -        vhost_vdpa_set_vring_ready(&s->vdpa, i);
> > >> +        ret = vhost_vdpa_set_vring_ready(&s->vdpa, i);
> > >> +        if (ret < 0) {
> > >> +            error_setg_errno(errp, -ret, "Error starting vring %d", i);
> > >> +            goto err_dev_stop;
> > >> +        }
> > >>      }
> > >>      s->started = true;
> > >>
> > >> @@ -274,6 +278,8 @@ static int vhost_vdpa_device_start(VirtIODevice 
> > >> *vdev, Error **errp)
> > >>
> > >>      return ret;
> > >>
> > >> +err_dev_stop:
> > >> +    vhost_dev_stop(&s->dev, vdev, false);
> > >>  err_guest_notifiers:
> > >>      k->set_guest_notifiers(qbus->parent, s->dev.nvqs, false);
> > >>  err_host_notifiers:
> > >> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > >> index 3726ee5d67..e3d8036479 100644
> > >> --- a/net/vhost-vdpa.c
> > >> +++ b/net/vhost-vdpa.c
> > >> @@ -381,7 +381,10 @@ static int vhost_vdpa_net_data_load(NetClientState 
> > >> *nc)
> > >>      }
> > >>
> > >>      for (int i = 0; i < v->dev->nvqs; ++i) {
> > >> -        vhost_vdpa_set_vring_ready(v, i + v->dev->vq_index);
> > >> +        int ret = vhost_vdpa_set_vring_ready(v, i + v->dev->vq_index);
> > >> +        if (ret < 0) {
> > >> +            return ret;
> > >> +        }
> > >>      }
> > >>      return 0;
> > >>  }
> > >> @@ -1213,7 +1216,10 @@ static int vhost_vdpa_net_cvq_load(NetClientState 
> > >> *nc)
> > >>
> > >>      assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
> > >>
> > >> -    vhost_vdpa_set_vring_ready(v, v->dev->vq_index);
> > >> +    r = vhost_vdpa_set_vring_ready(v, v->dev->vq_index);
> > >> +    if (unlikely(r < 0)) {
> > >> +        return r;
> > >> +    }
> > >>
> > >>      if (v->shadow_vqs_enabled) {
> > >>          n = VIRTIO_NET(v->dev->vdev);
> > >> @@ -1252,7 +1258,10 @@ static int vhost_vdpa_net_cvq_load(NetClientState 
> > >> *nc)
> > >>      }
> > >>
> > >>      for (int i = 0; i < v->dev->vq_index; ++i) {
> > >> -        vhost_vdpa_set_vring_ready(v, i);
> > >> +        r = vhost_vdpa_set_vring_ready(v, i);
> > >> +        if (unlikely(r < 0)) {
> > >> +            return r;
> > >> +        }
> > >>      }
> > >>
> > >>      return 0;
> > >> --
> > >> 2.43.0
> > >>
> > >
> >
>