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 > > > >