[PATCH 0/2] vdpa: Block vq property change in DRIVER_OK

[PATCH 0/2] vdpa: Block vq property change in DRIVER_OK

[Dragos Tatulea]

This series prevents the change of virtqueue address or state when a
device is in DRIVER_OK and not suspended. The virtio spec doesn't
allow changing virtqueue addresses and state in DRIVER_OK, but some devices
do support this operation when the device is suspended. The series
leaves the door open for these devices.

The series was suggested while discussing the addition of resumable
virtuque support in the mlx5_vdpa driver [0].

[0] https://lore.kernel.org/virtualization/20231219180858.120898-1-dtatulea@nvidia.com/T/#m044ddf540d98a6b025f81bffa058ca647a3d013e

Dragos Tatulea (2):
  vdpa: Track device suspended state
  vdpa: Block vq property changes in DRIVER_OK

 drivers/vhost/vdpa.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

-- 
2.43.0

[PATCH 1/2] vdpa: Track device suspended state

[Dragos Tatulea]

Set vdpa device suspended state on successful suspend. Clear it on
successful resume and reset.

The state will be locked by the vhost_vdpa mutex. The mutex is taken
during suspend, resume and reset in vhost_vdpa_unlocked_ioctl. The
exception is vhost_vdpa_open which does a device reset but that should
be safe because it can only happen before the other ops.

Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com>
Suggested-by: Eugenio Pérez <eperezma@redhat.com>
---
 drivers/vhost/vdpa.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index da7ec77cdaff..4c422be7d1e7 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -59,6 +59,7 @@ struct vhost_vdpa {
 	int in_batch;
 	struct vdpa_iova_range range;
 	u32 batch_asid;
+	bool suspended;
 };
 
 static DEFINE_IDA(vhost_vdpa_ida);
@@ -232,6 +233,8 @@ static int _compat_vdpa_reset(struct vhost_vdpa *v)
 	struct vdpa_device *vdpa = v->vdpa;
 	u32 flags = 0;
 
+	v->suspended = false;
+
 	if (v->vdev.vqs) {
 		flags |= !vhost_backend_has_feature(v->vdev.vqs[0],
 						    VHOST_BACKEND_F_IOTLB_PERSIST) ?
@@ -590,11 +593,16 @@ static long vhost_vdpa_suspend(struct vhost_vdpa *v)
 {
 	struct vdpa_device *vdpa = v->vdpa;
 	const struct vdpa_config_ops *ops = vdpa->config;
+	int ret;
 
 	if (!ops->suspend)
 		return -EOPNOTSUPP;
 
-	return ops->suspend(vdpa);
+	ret = ops->suspend(vdpa);
+	if (!ret)
+		v->suspended = true;
+
+	return ret;
 }
 
 /* After a successful return of this ioctl the device resumes processing
@@ -605,11 +613,16 @@ static long vhost_vdpa_resume(struct vhost_vdpa *v)
 {
 	struct vdpa_device *vdpa = v->vdpa;
 	const struct vdpa_config_ops *ops = vdpa->config;
+	int ret;
 
 	if (!ops->resume)
 		return -EOPNOTSUPP;
 
-	return ops->resume(vdpa);
+	ret = ops->resume(vdpa);
+	if (!ret)
+		v->suspended = false;
+
+	return ret;
 }
 
 static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int cmd,
-- 
2.43.0

[PATCH 2/2] vdpa: Block vq property changes in DRIVER_OK

[Dragos Tatulea]

The virtio standard doesn't allow for virtqueue address and state
changes when the device is in DRIVER_OK. Return an error in such cases
unless the device is suspended.

The suspended device exception is needed because some devices support
virtqueue changes when the device is suspended.

Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com>
Suggested-by: Eugenio Pérez <eperezma@redhat.com>
---
 drivers/vhost/vdpa.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index 4c422be7d1e7..620073383d15 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -703,6 +703,9 @@ static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int cmd,
 
 	switch (cmd) {
 	case VHOST_SET_VRING_ADDR:
+		if ((ops->get_status(vdpa) & VIRTIO_CONFIG_S_DRIVER_OK) && !v->suspended)
+			return -EINVAL;
+
 		if (ops->set_vq_address(vdpa, idx,
 					(u64)(uintptr_t)vq->desc,
 					(u64)(uintptr_t)vq->avail,
@@ -711,6 +714,9 @@ static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int cmd,
 		break;
 
 	case VHOST_SET_VRING_BASE:
+		if ((ops->get_status(vdpa) & VIRTIO_CONFIG_S_DRIVER_OK) && !v->suspended)
+			return -EINVAL;
+
 		if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED)) {
 			vq_state.packed.last_avail_idx = vq->last_avail_idx & 0x7fff;
 			vq_state.packed.last_avail_counter = !!(vq->last_avail_idx & 0x8000);
-- 
2.43.0

Re: [PATCH 0/2] vdpa: Block vq property change in DRIVER_OK

[Michael S. Tsirkin]

On Mon, Dec 25, 2023 at 03:42:08PM +0200, Dragos Tatulea wrote:
> This series prevents the change of virtqueue address or state when a
> device is in DRIVER_OK and not suspended. The virtio spec doesn't
> allow changing virtqueue addresses and state in DRIVER_OK, but some devices
> do support this operation when the device is suspended. The series
> leaves the door open for these devices.
> 
> The series was suggested while discussing the addition of resumable
> virtuque support in the mlx5_vdpa driver [0].


I am confused. Isn't this also included in
 vdpa/mlx5: Add support for resumable vqs

do you now want this merged separately?

> [0] https://lore.kernel.org/virtualization/20231219180858.120898-1-dtatulea@nvidia.com/T/#m044ddf540d98a6b025f81bffa058ca647a3d013e
> 
> Dragos Tatulea (2):
>   vdpa: Track device suspended state
>   vdpa: Block vq property changes in DRIVER_OK
> 
>  drivers/vhost/vdpa.c | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> -- 
> 2.43.0

Re: [PATCH 0/2] vdpa: Block vq property change in DRIVER_OK

[Dragos Tatulea]

On Mon, 2023-12-25 at 09:40 -0500, Michael S. Tsirkin wrote:
> On Mon, Dec 25, 2023 at 03:42:08PM +0200, Dragos Tatulea wrote:
> > This series prevents the change of virtqueue address or state when a
> > device is in DRIVER_OK and not suspended. The virtio spec doesn't
> > allow changing virtqueue addresses and state in DRIVER_OK, but some devices
> > do support this operation when the device is suspended. The series
> > leaves the door open for these devices.
> > 
> > The series was suggested while discussing the addition of resumable
> > virtuque support in the mlx5_vdpa driver [0].
> 
> 
> I am confused. Isn't this also included in
>  vdpa/mlx5: Add support for resumable vqs
> 
> do you now want this merged separately?
The discussion in the linked thread lead to having 2 series that are
independent: this series and the original v2 of of the "vdpa/mlx5: Add support
for resumable vqs" series (for which I will send a v5 now that is same as v2 +
Acked-by tags).

> 
> > [0] https://lore.kernel.org/virtualization/20231219180858.120898-1-dtatulea@nvidia.com/T/#m044ddf540d98a6b025f81bffa058ca647a3d013e
> > 
> > Dragos Tatulea (2):
> >   vdpa: Track device suspended state
> >   vdpa: Block vq property changes in DRIVER_OK
> > 
> >  drivers/vhost/vdpa.c | 23 +++++++++++++++++++++--
> >  1 file changed, 21 insertions(+), 2 deletions(-)
> > 
> > -- 
> > 2.43.0
>