On Thu, Mar 12, 2026 at 2:28 PM Eugenio Perez Martin <[email protected]> wrote: > > On Thu, Mar 12, 2026 at 5:03 AM Jason Wang <[email protected]> wrote: > > > > On Wed, Mar 11, 2026 at 3:10 AM 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. > > > > As discussed in the pervious version, let's explain why and the plan > > to support resume. > > > > Is it ok to explain it just in the patch message or do you prefer > something like a /* TODO: resume op */ before the suspend callback?
Maybe, but I wonder why lacking resume can make thing like migration work (e.g when migration fails, we should resume the device, or it's workaround by reset + DRIVER_OK?). > > > > > > > Signed-off-by: Eugenio Pérez <[email protected]> > > > --- > > > This patch depends on > > > https://lore.kernel.org/lkml/[email protected] > > > > > > v2: > > > * Take the rwsem only before the actual kick, not in vduse_vdpa_kick_vq. > > > This assures that we're not in a critical section. > > > --- > > > 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 4f642b95a7cb..f56b1e3eb82d 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++; > > > @@ -538,6 +542,10 @@ static void vduse_vq_kick(struct vduse_virtqueue *vq) > > > if (!vq->ready) > > > goto unlock; > > > > > > + guard(rwsem_read)(&vq->dev->rwsem); > > > + if (vq->dev->suspended) > > > + return; > > > > Any reason for this? E.g We don't do this for other transports. > > > > It's just a hardening but I'm happy to remove it. Either should be fine. > > > Everything else looks good. > > > > Thanks > > > Thanks
