On Wed, Mar 4, 2026 at 6:52 AM Jason Wang <[email protected]> wrote:
>
> On Tue, Mar 3, 2026 at 6:51 PM Eugenio Perez Martin <[email protected]>
> wrote:
> >
> > On Tue, Mar 3, 2026 at 8:26 AM Jason Wang <[email protected]> wrote:
> > >
> > > On Wed, Feb 11, 2026 at 8:02 PM Eugenio Pérez <[email protected]> wrote:
> > > >
> > > > Implement suspend operation for vduse devices, so vhost-vdpa will offer
> > > > that backend feature and userspace can effectively suspend the device.
> > > >
> > > > This is a must before get virtqueue indexes (base) for live migration,
> > > > since the device could modify them after userland gets them.
> > > >
> > > > Signed-off-by: Eugenio Pérez <[email protected]>
> > > > ---
> > > > This series depends on
> > > > https://lore.kernel.org/lkml/[email protected]
> > > > ---
> > > > drivers/vdpa/vdpa_user/vduse_dev.c | 86 +++++++++++++++++++++++++++++-
> > > > include/uapi/linux/vduse.h | 4 ++
> > > > 2 files changed, 88 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > index 59d9c4718d86..bdcc114e2710 100644
> > > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > @@ -54,7 +54,8 @@
> > > > #define IRQ_UNBOUND -1
> > > >
> > > > /* Supported VDUSE features */
> > > > -static const uint64_t vduse_features = BIT_U64(VDUSE_F_QUEUE_READY);
> > > > +static const uint64_t vduse_features = BIT_U64(VDUSE_F_QUEUE_READY) |
> > > > + BIT_U64(VDUSE_F_SUSPEND);
> > > >
> > > > /*
> > > > * VDUSE instance have not asked the vduse API version, so assume 0.
> > > > @@ -85,6 +86,7 @@ struct vduse_virtqueue {
> > > > int irq_effective_cpu;
> > > > struct cpumask irq_affinity;
> > > > struct kobject kobj;
> > > > + struct vduse_dev *dev;
> > > > };
> > > >
> > > > struct vduse_dev;
> > > > @@ -134,6 +136,7 @@ struct vduse_dev {
> > > > int minor;
> > > > bool broken;
> > > > bool connected;
> > > > + bool suspended;
> > > > u64 api_version;
> > > > u64 device_features;
> > > > u64 driver_features;
> > > > @@ -480,6 +483,7 @@ static void vduse_dev_reset(struct vduse_dev *dev)
> > > >
> > > > down_write(&dev->rwsem);
> > > >
> > > > + dev->suspended = false;
> > > > dev->status = 0;
> > > > dev->driver_features = 0;
> > > > dev->generation++;
> > > > @@ -559,6 +563,10 @@ static void vduse_vdpa_kick_vq(struct vdpa_device
> > > > *vdpa, u16 idx)
> > > > struct vduse_dev *dev = vdpa_to_vduse(vdpa);
> > > > struct vduse_virtqueue *vq = dev->vqs[idx];
> > > >
> > > > + guard(rwsem_read)(&vq->dev->rwsem);
> > > > + if (vq->dev->suspended)
> > > > + return;
> > > > +
> > > > if (!eventfd_signal_allowed()) {
> > > > schedule_work(&vq->kick);
> > > > return;
> > > > @@ -896,6 +904,27 @@ static int vduse_vdpa_set_map(struct vdpa_device
> > > > *vdpa,
> > > > return 0;
> > > > }
> > > >
> > > > +static int vduse_vdpa_suspend(struct vdpa_device *vdpa)
> > > > +{
> > > > + struct vduse_dev *dev = vdpa_to_vduse(vdpa);
> > > > + struct vduse_dev_msg msg = { 0 };
> > > > + int ret;
> > > > +
> > > > + msg.req.type = VDUSE_SUSPEND;
> > > > +
> > > > + ret = vduse_dev_msg_sync(dev, &msg);
> > > > + if (ret == 0) {
> > > > + scoped_guard(rwsem_write, &dev->rwsem)
> > > > + dev->suspended = true;
> > > > +
> > > > + cancel_work_sync(&dev->inject);
> > > > + for (u32 i = 0; i < dev->vq_num; i++)
> > > > + cancel_work_sync(&dev->vqs[i]->inject);
> > >
> > > Should we cancel those before msy sync?
> > >
> >
> > The only condition is that the driver must not see interrupts from the
> > moment the ioctl returns.
>
> I think you meant "vduse_vdpa_suspend returns" here.
>
> >
> > Also, disabling it before the msg returns would add complexity because
> > it needs to be reenabled & call the driver if it falis. Or am I
> > missing something it adds?
>
> I think not but I have another question:
>
> 1) I don't see resume, any reason for that
It's not strictly neccesary, and QEMU doesn't even support resume at
the moment. Every time we've implemented suspend / resume in the VDPA
core or parent drivers, it has been done in two different series.
> 2) assuming we need to support resume, do we need to record pending
> interrupt otherwise we may lose interrupt after resuming
>
Sure, I'll handle that when / if I add the resume operation.
> >
> > TBH this is more of a hardening than something the VDUSE driver should
> > do. We could even move to the vdpa core.
> >
> > > > + }
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > static void vduse_vdpa_free(struct vdpa_device *vdpa)
> > > > {
> > > > struct vduse_dev *dev = vdpa_to_vduse(vdpa);
> > > > @@ -937,6 +966,41 @@ static const struct vdpa_config_ops
> > > > vduse_vdpa_config_ops = {
> > > > .free = vduse_vdpa_free,
> > > > };
> > > >
> > > > +static const struct vdpa_config_ops vduse_vdpa_config_ops_with_suspend
> > > > = {
> > > > + .set_vq_address = vduse_vdpa_set_vq_address,
> > > > + .kick_vq = vduse_vdpa_kick_vq,
> > > > + .set_vq_cb = vduse_vdpa_set_vq_cb,
> > > > + .set_vq_num = vduse_vdpa_set_vq_num,
> > > > + .get_vq_size = vduse_vdpa_get_vq_size,
> > > > + .get_vq_group = vduse_get_vq_group,
> > > > + .set_vq_ready = vduse_vdpa_set_vq_ready,
> > > > + .get_vq_ready = vduse_vdpa_get_vq_ready,
> > > > + .set_vq_state = vduse_vdpa_set_vq_state,
> > > > + .get_vq_state = vduse_vdpa_get_vq_state,
> > > > + .get_vq_align = vduse_vdpa_get_vq_align,
> > > > + .get_device_features = vduse_vdpa_get_device_features,
> > > > + .set_driver_features = vduse_vdpa_set_driver_features,
> > > > + .get_driver_features = vduse_vdpa_get_driver_features,
> > > > + .set_config_cb = vduse_vdpa_set_config_cb,
> > > > + .get_vq_num_max = vduse_vdpa_get_vq_num_max,
> > > > + .get_device_id = vduse_vdpa_get_device_id,
> > > > + .get_vendor_id = vduse_vdpa_get_vendor_id,
> > > > + .get_status = vduse_vdpa_get_status,
> > > > + .set_status = vduse_vdpa_set_status,
> > > > + .get_config_size = vduse_vdpa_get_config_size,
> > > > + .get_config = vduse_vdpa_get_config,
> > > > + .set_config = vduse_vdpa_set_config,
> > > > + .get_generation = vduse_vdpa_get_generation,
> > > > + .set_vq_affinity = vduse_vdpa_set_vq_affinity,
> > > > + .get_vq_affinity = vduse_vdpa_get_vq_affinity,
> > > > + .reset = vduse_vdpa_reset,
> > > > + .set_map = vduse_vdpa_set_map,
> > > > + .set_group_asid = vduse_set_group_asid,
> > > > + .get_vq_map = vduse_get_vq_map,
> > > > + .suspend = vduse_vdpa_suspend,
> > >
> > > We can have a
> > >
> > > if (suspend_is_supported)
> > > ...
> > >
> > > to avoid having a new set of config ops?
> > >
> >
> > It complicates the code of vhost_vdpa in my opinion.
> >
> > Currently, it is:
> > if (ops->suspend)
>
> Okay, I see the problem which is:
>
> static bool vhost_vdpa_can_suspend(const struct vhost_vdpa *v)
> {
> struct vdpa_device *vdpa = v->vdpa;
> const struct vdpa_config_ops *ops = vdpa->config;
>
> return ops->suspend;
> }
>
> >
> > And that would move to something like:
> > if (ops->suspend) {
> > if (ops->suspend_is_supported && !ops->suspend_is_supported(dev)) {
> > return; // CANNOT SUSPEND
> > }
> >
> > ops->suspend(dev)
> > }
> >
> > Another option I considered is to follow mlx5's approach and have one
> > set of ops per device. But I think duplicating the vdpa_ops in the
> > data section is the cleanest approach. Let me know if you think we
> > should go with suspend_is_supported callback instead.
>
> I think there's no need then.
>
> >
> > > > + .free = vduse_vdpa_free,
> > > > +};
> > > > +
> > > > static void vduse_dev_sync_single_for_device(union virtio_map token,
> > > > dma_addr_t dma_addr,
> > > > size_t size,
> > > > enum dma_data_direction
> > > > dir)
> > > > @@ -1148,6 +1212,10 @@ static void vduse_dev_irq_inject(struct
> > > > work_struct *work)
> > > > {
> > > > struct vduse_dev *dev = container_of(work, struct vduse_dev,
> > > > inject);
> > > >
> > > > + guard(rwsem_read)(&dev->rwsem);
> > > > + if (dev->suspended)
> > > > + return;
> > >
> > > Device can still modify memory, so I wonder how much this can help.
> > >
> >
> > Do you prefer me to remove this guard?
>
> Nope, I misread the code, it should be fine.
>
> Thanks
>
> >
> > > > +
> > > > spin_lock_bh(&dev->irq_lock);
> > > > if (dev->config_cb.callback)
> > > > dev->config_cb.callback(dev->config_cb.private);
> > > > @@ -1159,6 +1227,10 @@ static void vduse_vq_irq_inject(struct
> > > > work_struct *work)
> > > > struct vduse_virtqueue *vq = container_of(work,
> > > > struct vduse_virtqueue, inject);
> > > >
> > > > + guard(rwsem_read)(&vq->dev->rwsem);
> > > > + if (vq->dev->suspended)
> > > > + return;
> > > > +
> > > > spin_lock_bh(&vq->irq_lock);
> > > > if (vq->ready && vq->cb.callback)
> > > > vq->cb.callback(vq->cb.private);
> > > > @@ -1189,6 +1261,9 @@ static int vduse_dev_queue_irq_work(struct
> > > > vduse_dev *dev,
> > > > int ret = -EINVAL;
> > > >
> > > > down_read(&dev->rwsem);
> > > > + if (dev->suspended)
> > > > + return ret;
> > > > +
> > > > if (!(dev->status & VIRTIO_CONFIG_S_DRIVER_OK))
> > > > goto unlock;
> > > >
> > > > @@ -1839,6 +1914,7 @@ static int vduse_dev_init_vqs(struct vduse_dev
> > > > *dev, u32 vq_align, u32 vq_num)
> > > > }
> > > >
> > > > dev->vqs[i]->index = i;
> > > > + dev->vqs[i]->dev = dev;
> > > > dev->vqs[i]->irq_effective_cpu = IRQ_UNBOUND;
> > > > INIT_WORK(&dev->vqs[i]->inject, vduse_vq_irq_inject);
> > > > INIT_WORK(&dev->vqs[i]->kick, vduse_vq_kick_work);
> > > > @@ -2311,12 +2387,18 @@ static struct vduse_mgmt_dev *vduse_mgmt;
> > > > static int vduse_dev_init_vdpa(struct vduse_dev *dev, const char *name)
> > > > {
> > > > struct vduse_vdpa *vdev;
> > > > + const struct vdpa_config_ops *ops;
> > > >
> > > > if (dev->vdev)
> > > > return -EEXIST;
> > > >
> > > > + if (dev->vduse_features & BIT_U64(VDUSE_F_SUSPEND))
> > > > + ops = &vduse_vdpa_config_ops_with_suspend;
> > > > + else
> > > > + ops = &vduse_vdpa_config_ops;
> > > > +
> > > > vdev = vdpa_alloc_device(struct vduse_vdpa, vdpa, dev->dev,
> > > > - &vduse_vdpa_config_ops, &vduse_map_ops,
> > > > + ops, &vduse_map_ops,
> > > > dev->ngroups, dev->nas, name, true);
> > > > if (IS_ERR(vdev))
> > > > return PTR_ERR(vdev);
> > > > diff --git a/include/uapi/linux/vduse.h b/include/uapi/linux/vduse.h
> > > > index d39734cef6d3..95b93bc6bac5 100644
> > > > --- a/include/uapi/linux/vduse.h
> > > > +++ b/include/uapi/linux/vduse.h
> > > > @@ -21,6 +21,9 @@
> > > > /* The VDUSE instance expects a request for vq ready */
> > > > #define VDUSE_F_QUEUE_READY 0
> > > >
> > > > +/* The VDUSE instance expects a request for suspend */
> > > > +#define VDUSE_F_SUSPEND 1
> > > > +
> > > > /*
> > > > * Get the version of VDUSE API that kernel supported
> > > > (VDUSE_API_VERSION).
> > > > * This is used for future extension.
> > > > @@ -338,6 +341,7 @@ enum vduse_req_type {
> > > > VDUSE_UPDATE_IOTLB,
> > > > VDUSE_SET_VQ_GROUP_ASID,
> > > > VDUSE_SET_VQ_READY,
> > > > + VDUSE_SUSPEND,
> > > > };
> > > >
> > > > /**
> > > > --
> > > > 2.53.0
> > > >
> > >
> > > Thanks
> > >
> >
>
