Re: [PATCH v4 3/4] vhost-vdpa: uAPI to stop the device

[Eugenio Perez Martin]

On Wed, Jun 1, 2022 at 9:13 PM Parav Pandit <parav@xxxxxxxxxx> wrote:
>
>
>
> > From: Eugenio Perez Martin <eperezma@xxxxxxxxxx>
> > Sent: Wednesday, June 1, 2022 7:15 AM
> >
> > On Wed, Jun 1, 2022 at 1:03 PM Michael S. Tsirkin <mst@xxxxxxxxxx> wrote:
> > >
> > > On Thu, May 26, 2022 at 02:43:37PM +0200, Eugenio Pérez wrote:
> > > > The ioctl adds support for stop the device from userspace.
> > > >
> > > > Signed-off-by: Eugenio Pérez <eperezma@xxxxxxxxxx>
> > > > ---
> > > >  drivers/vhost/vdpa.c       | 18 ++++++++++++++++++
> > > >  include/uapi/linux/vhost.h | 14 ++++++++++++++
> > > >  2 files changed, 32 insertions(+)
> > > >
> > > > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c index
> > > > 32713db5831d..d1d19555c4b7 100644
> > > > --- a/drivers/vhost/vdpa.c
> > > > +++ b/drivers/vhost/vdpa.c
> > > > @@ -478,6 +478,21 @@ static long vhost_vdpa_get_vqs_count(struct
> > vhost_vdpa *v, u32 __user *argp)
> > > >       return 0;
> > > >  }
> > > >
> > > > +static long vhost_vdpa_stop(struct vhost_vdpa *v, u32 __user *argp)
> > > > +{
> > > > +     struct vdpa_device *vdpa = v->vdpa;
> > > > +     const struct vdpa_config_ops *ops = vdpa->config;
> > > > +     int stop;
> > > > +
> > > > +     if (!ops->stop)
> > > > +             return -EOPNOTSUPP;
> > > > +
> > > > +     if (copy_from_user(&stop, argp, sizeof(stop)))
> > > > +             return -EFAULT;
> > > > +
> > > > +     return ops->stop(vdpa, stop);
> > > > +}
> > > > +
> > > >  static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int
> > cmd,
> > > >                                  void __user *argp)  { @@ -650,6
> > > > +665,9 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
> > > >       case VHOST_VDPA_GET_VQS_COUNT:
> > > >               r = vhost_vdpa_get_vqs_count(v, argp);
> > > >               break;
> > > > +     case VHOST_VDPA_STOP:
> > > > +             r = vhost_vdpa_stop(v, argp);
> > > > +             break;
> > > >       default:
> > > >               r = vhost_dev_ioctl(&v->vdev, cmd, argp);
> > > >               if (r == -ENOIOCTLCMD) diff --git
> > > > a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h index
> > > > cab645d4a645..c7e47b29bf61 100644
> > > > --- a/include/uapi/linux/vhost.h
> > > > +++ b/include/uapi/linux/vhost.h
> > > > @@ -171,4 +171,18 @@
> > > >  #define VHOST_VDPA_SET_GROUP_ASID    _IOW(VHOST_VIRTIO, 0x7C,
> > \
> > > >                                            struct vhost_vring_state)
> > > >
> > > > +/* Stop or resume a device so it does not process virtqueue
> > > > +requests anymore
> > > > + *
> > > > + * After the return of ioctl with stop != 0, the device must finish
> > > > +any
> > > > + * pending operations like in flight requests. It must also
> > > > +preserve all
> > > > + * the necessary state (the virtqueue vring base plus the possible
> > > > +device
> > > > + * specific states) that is required for restoring in the future.
> > > > +The
> > > > + * device must not change its configuration after that point.
> > > > + *
> > > > + * After the return of ioctl with stop == 0, the device can
> > > > +continue
> > > > + * processing buffers as long as typical conditions are met (vq is
> > > > +enabled,
> > > > + * DRIVER_OK status bit is enabled, etc).
> > > > + */
> > > > +#define VHOST_VDPA_STOP                      _IOW(VHOST_VIRTIO, 0x7D, int)
> > > > +
> A better name is VHOST_VDPA_SET_STATE
> State = stop/suspend
> State = start/resume
>
> Suspend/resume seems more logical, as opposed start/stop, because it more clearly indicates that the resume (start) is from some programmed beginning (and not first boot).
>

It's fine to move to that nomenclature in my opinion.

> > > >  #endif
> > >
> > > I wonder how does this interact with the admin vq idea.
> > > I.e. if we stop all VQs then apparently admin vq can't work either ...
> > > Thoughts?
> > >
> >
> > Copying here the answer to Parav, feel free to answer to any thread or
> > highlight if I missed something :). Using the admin vq proposal terminology of
> > "device group".
> >
> > --
> > This would stop a device of a device
> > group, but not the whole virtqueue group. If the admin VQ is offered by the
> > PF (since it's not exposed to the guest), it will continue accepting requests as
> > normal. If it's exposed in the VF, I think the best bet is to shadow it, since
> > guest and host requests could conflict.
> >
>
> vhost-vdpa device is exposed for a VF through vp-vdpa driver to user land.
> Now vp-vdpa driver will have to choose between using config register vs using AQ to suspend/resume the device.
>

vp_vdpa cannot choose if the virtio device has an admin vq or any
other feature, it just wraps the virtio device. If that virtio device
does not expose AQ, vp_vdpa cannot expose it.

> Why not always begin with more superior interface of AQ that address multiple of these needs for LM case?
>

Because it doesn't address valid use cases like vp_vdpa with no AQ,
devices that are not VF, or nested virtualization.

VHOST_VDPA_STOP / VHOST_VDPA_SET_STATE does not replace AQ commands:
It's just the way vhost-vdpa exposes that capability to qemu. vdpa
backend is free to choose whatever methods it finds better to
implement it.

> For LM case, more you explore, we realize that either VF relying on PF's AQ for query/config/setup/restore makes more sense or have its own dedicated AQ.
>

This ioctl does not mandate that the device cannot implement it
through AQ, or that the device has to be a VF.

Thanks!

> VM's suspend/resume operation can be handled through the shadow Q.
>
> > Since this is offered through vdpa, the device backend driver can route it to
> > whatever method works better for the hardware. For example, to send an
> > admin vq command to the PF. That's why it's important to keep the feature
> > as self-contained and orthogonal to others as possible.
> > --
> >
> > > > --
> > > > 2.31.1
> > >
>