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/[email protected]/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
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 <[email protected]>
Suggested-by: Eugenio Pérez <[email protected]>
---
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
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 <[email protected]>
Suggested-by: Eugenio Pérez <[email protected]>
---
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
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/[email protected]/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
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/[email protected]/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
>