The virtio spec doesn't allow changing virtqueue addresses after
DRIVER_OK. Some devices do support this operation when the device is
suspended. The VHOST_BACKEND_F_CHANGEABLE_VQ_ADDR_IN_SUSPEND flag
advertises this support as a backend features.
Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com>
Suggested-by: Eugenio Pérez <eperezma@redhat.com>
---
include/uapi/linux/vhost_types.h | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/include/uapi/linux/vhost_types.h b/include/uapi/linux/vhost_types.h
index d7656908f730..aacd067afc89 100644
--- a/include/uapi/linux/vhost_types.h
+++ b/include/uapi/linux/vhost_types.h
@@ -192,5 +192,9 @@ struct vhost_vdpa_iova_range {
#define VHOST_BACKEND_F_DESC_ASID 0x7
/* IOTLB don't flush memory mapping across device reset */
#define VHOST_BACKEND_F_IOTLB_PERSIST 0x8
+/* Device supports changing virtqueue addresses when device is suspended
+ * and is in state DRIVER_OK.
+ */
+#define VHOST_BACKEND_F_CHANGEABLE_VQ_ADDR_IN_SUSPEND 0x9
#endif
--
2.43.0
On Tue, Dec 19, 2023 at 7:09 PM Dragos Tatulea <dtatulea@nvidia.com> wrote:
>
> The virtio spec doesn't allow changing virtqueue addresses after
> DRIVER_OK. Some devices do support this operation when the device is
> suspended. The VHOST_BACKEND_F_CHANGEABLE_VQ_ADDR_IN_SUSPEND flag
> advertises this support as a backend features.
>
> Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com>
> Suggested-by: Eugenio Pérez <eperezma@redhat.com>
> ---
> include/uapi/linux/vhost_types.h | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/include/uapi/linux/vhost_types.h b/include/uapi/linux/vhost_types.h
> index d7656908f730..aacd067afc89 100644
> --- a/include/uapi/linux/vhost_types.h
> +++ b/include/uapi/linux/vhost_types.h
> @@ -192,5 +192,9 @@ struct vhost_vdpa_iova_range {
> #define VHOST_BACKEND_F_DESC_ASID 0x7
> /* IOTLB don't flush memory mapping across device reset */
> #define VHOST_BACKEND_F_IOTLB_PERSIST 0x8
> +/* Device supports changing virtqueue addresses when device is suspended
> + * and is in state DRIVER_OK.
> + */
> +#define VHOST_BACKEND_F_CHANGEABLE_VQ_ADDR_IN_SUSPEND 0x9
>
If we go by feature flag,
Acked-by: Eugenio Pérez <eperezma@redhat.com>
> #endif
> --
> 2.43.0
>
On Wed, Dec 20, 2023 at 2:09 AM Dragos Tatulea <dtatulea@nvidia.com> wrote:
>
> The virtio spec doesn't allow changing virtqueue addresses after
> DRIVER_OK. Some devices do support this operation when the device is
> suspended. The VHOST_BACKEND_F_CHANGEABLE_VQ_ADDR_IN_SUSPEND flag
> advertises this support as a backend features.
There's an ongoing effort in virtio spec to introduce the suspend state.
So I wonder if it's better to just allow such behaviour?
Thanks
>
> Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com>
> Suggested-by: Eugenio Pérez <eperezma@redhat.com>
> ---
> include/uapi/linux/vhost_types.h | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/include/uapi/linux/vhost_types.h b/include/uapi/linux/vhost_types.h
> index d7656908f730..aacd067afc89 100644
> --- a/include/uapi/linux/vhost_types.h
> +++ b/include/uapi/linux/vhost_types.h
> @@ -192,5 +192,9 @@ struct vhost_vdpa_iova_range {
> #define VHOST_BACKEND_F_DESC_ASID 0x7
> /* IOTLB don't flush memory mapping across device reset */
> #define VHOST_BACKEND_F_IOTLB_PERSIST 0x8
> +/* Device supports changing virtqueue addresses when device is suspended
> + * and is in state DRIVER_OK.
> + */
> +#define VHOST_BACKEND_F_CHANGEABLE_VQ_ADDR_IN_SUSPEND 0x9
>
> #endif
> --
> 2.43.0
>
On Wed, Dec 20, 2023 at 11:46 AM Jason Wang <jasowang@redhat.com> wrote: > > On Wed, Dec 20, 2023 at 2:09 AM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > The virtio spec doesn't allow changing virtqueue addresses after > > DRIVER_OK. Some devices do support this operation when the device is > > suspended. The VHOST_BACKEND_F_CHANGEABLE_VQ_ADDR_IN_SUSPEND flag > > advertises this support as a backend features. > > There's an ongoing effort in virtio spec to introduce the suspend state. > > So I wonder if it's better to just allow such behaviour? Actually I mean, allow drivers to modify the parameters during suspend without a new feature. Thanks > > Thanks > >
On Wed, Dec 20, 2023 at 5:06 AM Jason Wang <jasowang@redhat.com> wrote: > > On Wed, Dec 20, 2023 at 11:46 AM Jason Wang <jasowang@redhat.com> wrote: > > > > On Wed, Dec 20, 2023 at 2:09 AM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > The virtio spec doesn't allow changing virtqueue addresses after > > > DRIVER_OK. Some devices do support this operation when the device is > > > suspended. The VHOST_BACKEND_F_CHANGEABLE_VQ_ADDR_IN_SUSPEND flag > > > advertises this support as a backend features. > > > > There's an ongoing effort in virtio spec to introduce the suspend state. > > > > So I wonder if it's better to just allow such behaviour? > > Actually I mean, allow drivers to modify the parameters during suspend > without a new feature. > That would be ideal, but how do userland checks if it can suspend + change properties + resume? The only way that comes to my mind is to make sure all parents return error if userland tries to do it, and then fallback in userland. I'm ok with that, but I'm not sure if the current master & previous kernel has a coherent behavior. Do they return error? Or return success without changing address / vq state?
On Wed, Dec 20, 2023 at 9:32 PM Eugenio Perez Martin
<eperezma@redhat.com> wrote:
>
> On Wed, Dec 20, 2023 at 5:06 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Wed, Dec 20, 2023 at 11:46 AM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > On Wed, Dec 20, 2023 at 2:09 AM Dragos Tatulea <dtatulea@nvidia.com> wrote:
> > > >
> > > > The virtio spec doesn't allow changing virtqueue addresses after
> > > > DRIVER_OK. Some devices do support this operation when the device is
> > > > suspended. The VHOST_BACKEND_F_CHANGEABLE_VQ_ADDR_IN_SUSPEND flag
> > > > advertises this support as a backend features.
> > >
> > > There's an ongoing effort in virtio spec to introduce the suspend state.
> > >
> > > So I wonder if it's better to just allow such behaviour?
> >
> > Actually I mean, allow drivers to modify the parameters during suspend
> > without a new feature.
> >
>
> That would be ideal, but how do userland checks if it can suspend +
> change properties + resume?
As discussed, it looks to me the only device that supports suspend is
simulator and it supports change properties.
E.g:
static int vdpasim_set_vq_address(struct vdpa_device *vdpa, u16 idx,
u64 desc_area, u64 driver_area,
u64 device_area)
{
struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
struct vdpasim_virtqueue *vq = &vdpasim->vqs[idx];
vq->desc_addr = desc_area;
vq->driver_addr = driver_area;
vq->device_addr = device_area;
return 0;
}
>
> The only way that comes to my mind is to make sure all parents return
> error if userland tries to do it, and then fallback in userland.
Yes.
> I'm
> ok with that, but I'm not sure if the current master & previous kernel
> has a coherent behavior. Do they return error? Or return success
> without changing address / vq state?
We probably don't need to worry too much here, as e.g set_vq_address
could fail even without suspend (just at uAPI level).
Thanks
>
On Thu, Dec 21, 2023 at 3:03 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Wed, Dec 20, 2023 at 9:32 PM Eugenio Perez Martin
> <eperezma@redhat.com> wrote:
> >
> > On Wed, Dec 20, 2023 at 5:06 AM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > On Wed, Dec 20, 2023 at 11:46 AM Jason Wang <jasowang@redhat.com> wrote:
> > > >
> > > > On Wed, Dec 20, 2023 at 2:09 AM Dragos Tatulea <dtatulea@nvidia.com> wrote:
> > > > >
> > > > > The virtio spec doesn't allow changing virtqueue addresses after
> > > > > DRIVER_OK. Some devices do support this operation when the device is
> > > > > suspended. The VHOST_BACKEND_F_CHANGEABLE_VQ_ADDR_IN_SUSPEND flag
> > > > > advertises this support as a backend features.
> > > >
> > > > There's an ongoing effort in virtio spec to introduce the suspend state.
> > > >
> > > > So I wonder if it's better to just allow such behaviour?
> > >
> > > Actually I mean, allow drivers to modify the parameters during suspend
> > > without a new feature.
> > >
> >
> > That would be ideal, but how do userland checks if it can suspend +
> > change properties + resume?
>
> As discussed, it looks to me the only device that supports suspend is
> simulator and it supports change properties.
>
> E.g:
>
> static int vdpasim_set_vq_address(struct vdpa_device *vdpa, u16 idx,
> u64 desc_area, u64 driver_area,
> u64 device_area)
> {
> struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
> struct vdpasim_virtqueue *vq = &vdpasim->vqs[idx];
>
> vq->desc_addr = desc_area;
> vq->driver_addr = driver_area;
> vq->device_addr = device_area;
>
> return 0;
> }
>
So in the current kernel master it is valid to set a different vq
address while the device is suspended in vdpa_sim. But it is not valid
in mlx5, as the FW will not be updated in resume (Dragos, please
correct me if I'm wrong). Both of them return success.
How can we know in the destination QEMU if it is valid to suspend &
set address? Should we handle this as a bugfix and backport the
change?
> >
> > The only way that comes to my mind is to make sure all parents return
> > error if userland tries to do it, and then fallback in userland.
>
> Yes.
>
> > I'm
> > ok with that, but I'm not sure if the current master & previous kernel
> > has a coherent behavior. Do they return error? Or return success
> > without changing address / vq state?
>
> We probably don't need to worry too much here, as e.g set_vq_address
> could fail even without suspend (just at uAPI level).
>
I don't get this, sorry. I rephrased my point with an example earlier
in the mail.
On Thu, Dec 21, 2023 at 3:47 PM Eugenio Perez Martin
<eperezma@redhat.com> wrote:
>
> On Thu, Dec 21, 2023 at 3:03 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Wed, Dec 20, 2023 at 9:32 PM Eugenio Perez Martin
> > <eperezma@redhat.com> wrote:
> > >
> > > On Wed, Dec 20, 2023 at 5:06 AM Jason Wang <jasowang@redhat.com> wrote:
> > > >
> > > > On Wed, Dec 20, 2023 at 11:46 AM Jason Wang <jasowang@redhat.com> wrote:
> > > > >
> > > > > On Wed, Dec 20, 2023 at 2:09 AM Dragos Tatulea <dtatulea@nvidia.com> wrote:
> > > > > >
> > > > > > The virtio spec doesn't allow changing virtqueue addresses after
> > > > > > DRIVER_OK. Some devices do support this operation when the device is
> > > > > > suspended. The VHOST_BACKEND_F_CHANGEABLE_VQ_ADDR_IN_SUSPEND flag
> > > > > > advertises this support as a backend features.
> > > > >
> > > > > There's an ongoing effort in virtio spec to introduce the suspend state.
> > > > >
> > > > > So I wonder if it's better to just allow such behaviour?
> > > >
> > > > Actually I mean, allow drivers to modify the parameters during suspend
> > > > without a new feature.
> > > >
> > >
> > > That would be ideal, but how do userland checks if it can suspend +
> > > change properties + resume?
> >
> > As discussed, it looks to me the only device that supports suspend is
> > simulator and it supports change properties.
> >
> > E.g:
> >
> > static int vdpasim_set_vq_address(struct vdpa_device *vdpa, u16 idx,
> > u64 desc_area, u64 driver_area,
> > u64 device_area)
> > {
> > struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
> > struct vdpasim_virtqueue *vq = &vdpasim->vqs[idx];
> >
> > vq->desc_addr = desc_area;
> > vq->driver_addr = driver_area;
> > vq->device_addr = device_area;
> >
> > return 0;
> > }
> >
>
> So in the current kernel master it is valid to set a different vq
> address while the device is suspended in vdpa_sim. But it is not valid
> in mlx5, as the FW will not be updated in resume (Dragos, please
> correct me if I'm wrong). Both of them return success.
>
> How can we know in the destination QEMU if it is valid to suspend &
> set address? Should we handle this as a bugfix and backport the
> change?
Good point.
We probably need to do backport, this seems to be the easiest way.
Theoretically, userspace may assume this behavior (though I don't
think there would be a user that depends on the simulator).
>
> > >
> > > The only way that comes to my mind is to make sure all parents return
> > > error if userland tries to do it, and then fallback in userland.
> >
> > Yes.
> >
> > > I'm
> > > ok with that, but I'm not sure if the current master & previous kernel
> > > has a coherent behavior. Do they return error? Or return success
> > > without changing address / vq state?
> >
> > We probably don't need to worry too much here, as e.g set_vq_address
> > could fail even without suspend (just at uAPI level).
> >
>
> I don't get this, sorry. I rephrased my point with an example earlier
> in the mail.
I mean currently, VHOST_SET_VRING_ADDR can fail. So userspace should
not assume it will always succeed.
Thanks
>
© 2016 - 2025 Red Hat, Inc.