Re: [PATCH v3 2/8] vhost-vdpa: use bind_mm/unbind_mm device callbacks

[Stefano Garzarella <sgarzare@xxxxxxxxxx>]

On Thu, Mar 23, 2023 at 11:01:39AM +0800, Jason Wang wrote:
> On Tue, Mar 21, 2023 at 11:42 PM Stefano Garzarella <sgarzare@xxxxxxxxxx> wrote:
> > When the user call VHOST_SET_OWNER ioctl and the vDPA device
> > has `use_va` set to true, let's call the bind_mm callback.
> > In this way we can bind the device to the user address space
> > and directly use the user VA.
> >
> > The unbind_mm callback is called during the release after
> > stopping the device.
> >
> > Signed-off-by: Stefano Garzarella <sgarzare@xxxxxxxxxx>
> > ---
> >
> > Notes:
> >     v3:
> >     - added `case VHOST_SET_OWNER` in vhost_vdpa_unlocked_ioctl() [Jason]
> >     v2:
> >     - call the new unbind_mm callback during the release [Jason]
> >     - avoid to call bind_mm callback after the reset, since the device
> >       is not detaching it now during the reset
> >
> >  drivers/vhost/vdpa.c | 31 +++++++++++++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> >
> > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> > index 7be9d9d8f01c..20250c3418b2 100644
> > --- a/drivers/vhost/vdpa.c
> > +++ b/drivers/vhost/vdpa.c
> > @@ -219,6 +219,28 @@ static int vhost_vdpa_reset(struct vhost_vdpa *v)
> >         return vdpa_reset(vdpa);
> >  }
> >
> > +static long vhost_vdpa_bind_mm(struct vhost_vdpa *v)
> > +{
> > +       struct vdpa_device *vdpa = v->vdpa;
> > +       const struct vdpa_config_ops *ops = vdpa->config;
> > +
> > +       if (!vdpa->use_va || !ops->bind_mm)
> > +               return 0;
> > +
> > +       return ops->bind_mm(vdpa, v->vdev.mm);
> > +}
> > +
> > +static void vhost_vdpa_unbind_mm(struct vhost_vdpa *v)
> > +{
> > +       struct vdpa_device *vdpa = v->vdpa;
> > +       const struct vdpa_config_ops *ops = vdpa->config;
> > +
> > +       if (!vdpa->use_va || !ops->unbind_mm)
> > +               return;
> > +
> > +       ops->unbind_mm(vdpa);
> > +}
> > +
> >  static long vhost_vdpa_get_device_id(struct vhost_vdpa *v, u8 __user *argp)
> >  {
> >         struct vdpa_device *vdpa = v->vdpa;
> > @@ -709,6 +731,14 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
> >         case VHOST_VDPA_RESUME:
> >                 r = vhost_vdpa_resume(v);
> >                 break;
> > +       case VHOST_SET_OWNER:
> > +               r = vhost_dev_set_owner(d);
>
> Nit:
>
> I'd stick to the current way of passing the cmd, argp to
> vhost_dev_ioctl() and introduce a new switch after the
> vhost_dev_ioctl().
>
> In this way, we are immune to any possible changes of dealing with
> VHOST_SET_OWNER in vhost core.

Good point, I'll change in v4.

> Others look good.

Thanks,
Stefano