Re: [PATCH v3 2/5] vdpa: add vhost_vdpa_reset_status_fd

[Eugenio Perez Martin <eperezma@redhat.com>]

On Wed, May 17, 2023 at 7:49 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Wed, May 17, 2023 at 1:46 PM Eugenio Perez Martin
> <eperezma@redhat.com> wrote:
> >
> > On Wed, May 17, 2023 at 5:14 AM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > On Tue, May 9, 2023 at 11:44 PM Eugenio Pérez <eperezma@redhat.com> wrote:
> > > >
> > > > This allows to reset a vhost-vdpa device from external subsystems like
> > > > vhost-net, since it does not have any struct vhost_dev by the time we
> > > > need to use it.
> > > >
> > > > It is used in subsequent patches to negotiate features
> > > > and probe for CVQ ASID isolation.
> > > >
> > > > Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
> > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > ---
> > > >  include/hw/virtio/vhost-vdpa.h |  1 +
> > > >  hw/virtio/vhost-vdpa.c         | 58 +++++++++++++++++++++++-----------
> > > >  2 files changed, 41 insertions(+), 18 deletions(-)
> > > >
> > > > diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h
> > > > index c278a2a8de..28de7da91e 100644
> > > > --- a/include/hw/virtio/vhost-vdpa.h
> > > > +++ b/include/hw/virtio/vhost-vdpa.h
> > > > @@ -54,6 +54,7 @@ typedef struct vhost_vdpa {
> > > >      VhostVDPAHostNotifier notifier[VIRTIO_QUEUE_MAX];
> > > >  } VhostVDPA;
> > > >
> > > > +void vhost_vdpa_reset_status_fd(int fd);
> > > >  int vhost_vdpa_get_iova_range(int fd, struct vhost_vdpa_iova_range *iova_range);
> > > >
> > > >  int vhost_vdpa_dma_map(struct vhost_vdpa *v, uint32_t asid, hwaddr iova,
> > > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > > > index bbabea18f3..7a2053b8d9 100644
> > > > --- a/hw/virtio/vhost-vdpa.c
> > > > +++ b/hw/virtio/vhost-vdpa.c
> > > > @@ -335,38 +335,45 @@ static const MemoryListener vhost_vdpa_memory_listener = {
> > > >      .region_del = vhost_vdpa_listener_region_del,
> > > >  };
> > > >
> > > > -static int vhost_vdpa_call(struct vhost_dev *dev, unsigned long int request,
> > > > -                             void *arg)
> > > > +static int vhost_vdpa_dev_fd(const struct vhost_dev *dev)
> > > >  {
> > > >      struct vhost_vdpa *v = dev->opaque;
> > > > -    int fd = v->device_fd;
> > > > -    int ret;
> > > >
> > > >      assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_VDPA);
> > > > +    return v->device_fd;
> > > > +}
> > >
> > > Nit: unless the vhost_dev structure is opaque to the upper layer, I
> > > don't see any advantage for having a dedicated indirect helper to get
> > > device_fd.
> > >
> >
> > The purpose was to not duplicate the assert, but sure it's not mandatory.
>
> Ok, but I think for new codes, we'd better avoid assert as much as possible.
>

I'll remove it, thanks!

> >
> > > > +
> > > > +static int vhost_vdpa_call_fd(int fd, unsigned long int request, void *arg)
> > > > +{
> > > > +    int ret = ioctl(fd, request, arg);
> > > >
> > > > -    ret = ioctl(fd, request, arg);
> > > >      return ret < 0 ? -errno : ret;
> > > >  }
> > > >
> > > > -static int vhost_vdpa_add_status(struct vhost_dev *dev, uint8_t status)
> > > > +static int vhost_vdpa_call(struct vhost_dev *dev, unsigned long int request,
> > > > +                           void *arg)
> > > > +{
> > > > +    return vhost_vdpa_call_fd(vhost_vdpa_dev_fd(dev), request, arg);
> > > > +}
> > > > +
> > > > +static int vhost_vdpa_add_status_fd(int fd, uint8_t status)
> > > >  {
> > > >      uint8_t s;
> > > >      int ret;
> > > >
> > > > -    trace_vhost_vdpa_add_status(dev, status);
> > > > -    ret = vhost_vdpa_call(dev, VHOST_VDPA_GET_STATUS, &s);
> > > > +    ret = vhost_vdpa_call_fd(fd, VHOST_VDPA_GET_STATUS, &s);
> > > >      if (ret < 0) {
> > > >          return ret;
> > > >      }
> > > >
> > > >      s |= status;
> > > >
> > > > -    ret = vhost_vdpa_call(dev, VHOST_VDPA_SET_STATUS, &s);
> > > > +    ret = vhost_vdpa_call_fd(fd, VHOST_VDPA_SET_STATUS, &s);
> > > >      if (ret < 0) {
> > > >          return ret;
> > > >      }
> > > >
> > > > -    ret = vhost_vdpa_call(dev, VHOST_VDPA_GET_STATUS, &s);
> > > > +    ret = vhost_vdpa_call_fd(fd, VHOST_VDPA_GET_STATUS, &s);
> > > >      if (ret < 0) {
> > > >          return ret;
> > > >      }
> > > > @@ -378,6 +385,12 @@ static int vhost_vdpa_add_status(struct vhost_dev *dev, uint8_t status)
> > > >      return 0;
> > > >  }
> > > >
> > > > +static int vhost_vdpa_add_status(struct vhost_dev *dev, uint8_t status)
> > > > +{
> > > > +    trace_vhost_vdpa_add_status(dev, status);
> > > > +    return vhost_vdpa_add_status_fd(vhost_vdpa_dev_fd(dev), status);
> > > > +}
> > > > +
> > > >  int vhost_vdpa_get_iova_range(int fd, struct vhost_vdpa_iova_range *iova_range)
> > > >  {
> > > >      int ret = ioctl(fd, VHOST_VDPA_GET_IOVA_RANGE, iova_range);
> > > > @@ -709,16 +722,20 @@ static int vhost_vdpa_get_device_id(struct vhost_dev *dev,
> > > >      return ret;
> > > >  }
> > > >
> > > > +static int vhost_vdpa_reset_device_fd(int fd)
> > > > +{
> > > > +    uint8_t status = 0;
> > > > +
> > > > +    return vhost_vdpa_call_fd(fd, VHOST_VDPA_SET_STATUS, &status);
> > > > +}
> > > > +
> > > >  static int vhost_vdpa_reset_device(struct vhost_dev *dev)
> > > >  {
> > > >      struct vhost_vdpa *v = dev->opaque;
> > > > -    int ret;
> > > > -    uint8_t status = 0;
> > > >
> > > > -    ret = vhost_vdpa_call(dev, VHOST_VDPA_SET_STATUS, &status);
> > > > -    trace_vhost_vdpa_reset_device(dev);
> > > >      v->suspended = false;
> > > > -    return ret;
> > > > +    trace_vhost_vdpa_reset_device(dev);
> > > > +    return vhost_vdpa_reset_device_fd(vhost_vdpa_dev_fd(dev));
> > > >  }
> > > >
> > > >  static int vhost_vdpa_get_vq_index(struct vhost_dev *dev, int idx)
> > > > @@ -1170,6 +1187,13 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
> > > >      return 0;
> > > >  }
> > > >
> > > > +void vhost_vdpa_reset_status_fd(int fd)
> > > > +{
> > > > +    vhost_vdpa_reset_device_fd(fd);
> > > > +    vhost_vdpa_add_status_fd(fd, VIRTIO_CONFIG_S_ACKNOWLEDGE |
> > > > +                                 VIRTIO_CONFIG_S_DRIVER);
> > >
> > > I would like to rename this function since it does more than just reset.
> > >
> >
> > vhost_vdpa_set_ready?
>
> But it doesn't set DRIVER_OK so it might be somehow misleading.
>

I'm still thinking about this. I don't like the original name either
(vhost_vdpa_reset_status), but that one was already in the code before
this patch. To rename it makes this patch scope bigger.

The main idea of this function is to stop duplicating code between
net/vhost-vdpa and hw/virtio/vdpa. One possibility is to not export
vhost_vdpa_reset_status_fd but export vhost_vdpa_reset_device_fd and
vhost_vdpa_add_status_fd individually. Does that seem better?

The other end is not to offer anything and call raw ioctl as previous
code of net/vhost-vdpa. There is a middle ground, like only exporting
one of them for sure.

What do you think?

Thanks!

> Thanks
>
> >
> > Thanks!
> >
> > > Thanks
> > >
> > > > +}
> > > > +
> > > >  static void vhost_vdpa_reset_status(struct vhost_dev *dev)
> > > >  {
> > > >      struct vhost_vdpa *v = dev->opaque;
> > > > @@ -1178,9 +1202,7 @@ static void vhost_vdpa_reset_status(struct vhost_dev *dev)
> > > >          return;
> > > >      }
> > > >
> > > > -    vhost_vdpa_reset_device(dev);
> > > > -    vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
> > > > -                               VIRTIO_CONFIG_S_DRIVER);
> > > > +    vhost_vdpa_reset_status_fd(vhost_vdpa_dev_fd(dev));
> > > >      memory_listener_unregister(&v->listener);
> > > >  }
> > > >
> > > > --
> > > > 2.31.1
> > > >
> > >
> >
>