On Tue, Jul 11, 2023 at 5:52 PM Hanna Czenczek <[email protected]> wrote:
>
> Move the `suspended` field from vhost_vdpa into the global vhost_dev
> struct, so vhost_dev_stop() can check whether the back-end has been
> suspended by `vhost_ops->vhost_dev_start(hdev, false)`. If it has,
> there is no need to reset it; the reset is just a fall-back to stop
> device operations for back-ends that do not support suspend.
>
> Unfortunately, for vDPA specifically, RESUME is not yet implemented, so
> when the device is re-started, we still have to do the reset to have it
> un-suspend.
>
> Signed-off-by: Hanna Czenczek <[email protected]>
> ---
> include/hw/virtio/vhost-vdpa.h | 2 --
> include/hw/virtio/vhost.h | 8 ++++++++
> hw/virtio/vhost-vdpa.c | 11 +++++++----
> hw/virtio/vhost.c | 8 +++++++-
> 4 files changed, 22 insertions(+), 7 deletions(-)
>
> diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h
> index e64bfc7f98..72c3686b7f 100644
> --- a/include/hw/virtio/vhost-vdpa.h
> +++ b/include/hw/virtio/vhost-vdpa.h
> @@ -42,8 +42,6 @@ typedef struct vhost_vdpa {
> bool shadow_vqs_enabled;
> /* Vdpa must send shadow addresses as IOTLB key for data queues, not GPA
> */
> bool shadow_data;
> - /* Device suspended successfully */
> - bool suspended;
> /* IOVA mapping used by the Shadow Virtqueue */
> VhostIOVATree *iova_tree;
> GPtrArray *shadow_vqs;
> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> index 6a173cb9fa..69bf59d630 100644
> --- a/include/hw/virtio/vhost.h
> +++ b/include/hw/virtio/vhost.h
> @@ -120,6 +120,14 @@ struct vhost_dev {
> uint64_t backend_cap;
> /* @started: is the vhost device started? */
> bool started;
> + /**
> + * @suspended: Whether the vhost device is currently suspended. Set
> + * and reset by implementations (vhost-user, vhost-vdpa, ...), which
> + * are supposed to automatically suspend/resume in their
> + * vhost_dev_start handlers as required. Must also be cleared when
> + * the device is reset.
> + */
> + bool suspended;
> bool log_enabled;
> uint64_t log_size;
> Error *migration_blocker;
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index 7b7dee468e..f7fd19a203 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -858,13 +858,12 @@ static int vhost_vdpa_get_device_id(struct vhost_dev
> *dev,
>
> static int vhost_vdpa_reset_device(struct vhost_dev *dev)
> {
> - struct vhost_vdpa *v = dev->opaque;
> int ret;
> uint8_t status = 0;
>
> ret = vhost_vdpa_call(dev, VHOST_VDPA_SET_STATUS, &status);
> trace_vhost_vdpa_reset_device(dev);
> - v->suspended = false;
> + dev->suspended = false;
> return ret;
> }
>
> @@ -1278,7 +1277,7 @@ static void vhost_vdpa_suspend(struct vhost_dev *dev)
> if (unlikely(r)) {
> error_report("Cannot suspend: %s(%d)", g_strerror(errno), errno);
> } else {
> - v->suspended = true;
> + dev->suspended = true;
> return;
> }
> }
> @@ -1313,6 +1312,10 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev,
> bool started)
> return -1;
> }
> vhost_vdpa_set_vring_ready(dev);
> + if (dev->suspended) {
> + /* TODO: When RESUME is available, use it instead of resetting */
> + vhost_vdpa_reset_status(dev);
How is that we reset the status at each vhost_vdpa_dev_start? That
will clean all the vqs configured, features negotiated, etc. in the
vDPA device. Or am I missing something?
I'm totally ok with the move of "suspended" member.
Thanks!
> + }
> } else {
> vhost_vdpa_suspend(dev);
> vhost_vdpa_svqs_stop(dev);
> @@ -1400,7 +1403,7 @@ static int vhost_vdpa_get_vring_base(struct vhost_dev
> *dev,
> return 0;
> }
>
> - if (!v->suspended) {
> + if (!dev->suspended) {
> /*
> * Cannot trust in value returned by device, let vhost recover used
> * idx from guest.
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index abf0d03c8d..2e28e58da7 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -2059,7 +2059,13 @@ void vhost_dev_stop(struct vhost_dev *hdev,
> VirtIODevice *vdev, bool vrings)
> hdev->vqs + i,
> hdev->vq_index + i);
> }
> - if (hdev->vhost_ops->vhost_reset_status) {
> +
> + /*
> + * If we failed to successfully stop the device via SUSPEND (should have
> + * been attempted by `vhost_dev_start(hdev, false)`), reset it to stop
> it.
> + * Stateful devices where this would be problematic must implement
> SUSPEND.
> + */
> + if (!hdev->suspended && hdev->vhost_ops->vhost_reset_status) {
> hdev->vhost_ops->vhost_reset_status(hdev);
> }
>
> --
> 2.41.0
>
