[RFC,v3,2/3] virtio: support delay of checks in virtio_load()

Delay checks in virtio_load() to avoid possible address_space_to_flatview() call
during memory region's begin/commit.

Signed-off-by: Chuang Xu <xuchuangxclwt@bytedance.com>
---
 hw/virtio/virtio.c         | 37 +++++++++++++++++++++++++++----------
 include/hw/virtio/virtio.h |  2 ++
 2 files changed, 29 insertions(+), 10 deletions(-)

On Tue, Dec 13, 2022 at 09:35:09PM +0800, Chuang Xu wrote:
> Delay checks in virtio_load() to avoid possible address_space_to_flatview() call
> during memory region's begin/commit.

I didn't notice virtio has the vm change handler already, looks good to
reuse it. :) A few more comments though (before some real virtio developers
chim im).

> 
> Signed-off-by: Chuang Xu <xuchuangxclwt@bytedance.com>
> ---
>  hw/virtio/virtio.c         | 37 +++++++++++++++++++++++++++----------
>  include/hw/virtio/virtio.h |  2 ++
>  2 files changed, 29 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index eb6347ab5d..f556e565c6 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -3642,8 +3642,26 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
>          vdev->start_on_kick = true;
>      }
>  
> +    vdev->delay_check = true;
> +
> +    if (vdc->post_load) {
> +        ret = vdc->post_load(vdev);
> +        if (ret) {
> +            return ret;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +static void virtio_load_check_delay(VirtIODevice *vdev)
> +{
>      RCU_READ_LOCK_GUARD();
> -    for (i = 0; i < num; i++) {
> +    for (int i = 0; i < VIRTIO_QUEUE_MAX; i++) {
> +        if (vdev->vq[i].vring.num == 0) {
> +            break;
> +        }
> +
>          if (vdev->vq[i].vring.desc) {
>              uint16_t nheads;
>  
> @@ -3696,19 +3714,12 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
>                               i, vdev->vq[i].vring.num,
>                               vdev->vq[i].last_avail_idx,
>                               vdev->vq[i].used_idx);
> -                return -1;
> +                abort();

This is when the switchover finished.  I'm not sure how severe this is and
whether there can be something to remedy - abort() is probably the least we
want to do here, since the admin may not want to crash the whole VM due to
one vring failure on one device.

>              }
>          }
>      }
>  
> -    if (vdc->post_load) {
> -        ret = vdc->post_load(vdev);
> -        if (ret) {
> -            return ret;
> -        }
> -    }
> -
> -    return 0;
> +    return;
>  }
>  
>  void virtio_cleanup(VirtIODevice *vdev)
> @@ -3722,6 +3733,11 @@ static void virtio_vmstate_change(void *opaque, bool running, RunState state)
>      BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
>      VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
>      bool backend_run = running && virtio_device_started(vdev, vdev->status);
> +
> +    if (vdev->delay_check) {
> +        virtio_load_check_delay(vdev);
> +        vdev->delay_check = false;
> +    }
>      vdev->vm_running = running;
>  
>      if (backend_run) {
> @@ -3789,6 +3805,7 @@ void virtio_init(VirtIODevice *vdev, uint16_t device_id, size_t config_size)
>              virtio_vmstate_change, vdev);
>      vdev->device_endian = virtio_default_endian();
>      vdev->use_guest_notifier_mask = true;
> +    vdev->delay_check = false;
>  }
>  
>  /*
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index acfd4df125..269e80d04a 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -135,6 +135,8 @@ struct VirtIODevice
>      AddressSpace *dma_as;
>      QLIST_HEAD(, VirtQueue) *vector_queues;
>      QTAILQ_ENTRY(VirtIODevice) next;
> +    /* @delay_check: delay checks in virtio_load */
> +    bool delay_check;

I think it covers more than the check?  It also initializes variables like
used_idx and shadow_avail_idx.  I'm not sure how vital they are, but I'd
just avoid using the word "check" if not sure (e.g. "load_delay", or
"load_finalize"?).

>  };
>  
>  struct VirtioDeviceClass {
> -- 
> 2.20.1
>

On 2022/12/14 上午12:31, Peter Xu wrote:

On Tue, Dec 13, 2022 at 09:35:09PM +0800, Chuang Xu wrote:

Delay checks in virtio_load() to avoid possible address_space_to_flatview() call
during memory region's begin/commit.

I didn't notice virtio has the vm change handler already, looks good to
reuse it. :) A few more comments though (before some real virtio developers
chim im).


Signed-off-by: Chuang Xu <xuchuangxclwt@bytedance.com>
<xuchuangxclwt@bytedance.com>
---
 hw/virtio/virtio.c         | 37 +++++++++++++++++++++++++++----------
 include/hw/virtio/virtio.h |  2 ++
 2 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index eb6347ab5d..f556e565c6 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -3642,8 +3642,26 @@ int virtio_load(VirtIODevice *vdev, QEMUFile
*f, int version_id)
         vdev->start_on_kick = true;
     }

+    vdev->delay_check = true;
+
+    if (vdc->post_load) {
+        ret = vdc->post_load(vdev);
+        if (ret) {
+            return ret;
+        }
+    }
+
+    return 0;
+}
+
+static void virtio_load_check_delay(VirtIODevice *vdev)
+{
     RCU_READ_LOCK_GUARD();
-    for (i = 0; i < num; i++) {
+    for (int i = 0; i < VIRTIO_QUEUE_MAX; i++) {
+        if (vdev->vq[i].vring.num == 0) {
+            break;
+        }
+
         if (vdev->vq[i].vring.desc) {
             uint16_t nheads;

@@ -3696,19 +3714,12 @@ int virtio_load(VirtIODevice *vdev, QEMUFile
*f, int version_id)
                              i, vdev->vq[i].vring.num,
                              vdev->vq[i].last_avail_idx,
                              vdev->vq[i].used_idx);
-                return -1;
+                abort();

This is when the switchover finished.  I'm not sure how severe this is and
whether there can be something to remedy - abort() is probably the least we
want to do here, since the admin may not want to crash the whole VM due to
one vring failure on one device.

At this time, the vcpus are still stopped. If these checks fail in
virtio_load(), - 1 will be returned, and the migration will be rolled
back. But virtio_vmstate_change() returns nothing, if we want to
rollback the migration after the checks fail, I think we need abort().

             }
         }
     }

-    if (vdc->post_load) {
-        ret = vdc->post_load(vdev);
-        if (ret) {
-            return ret;
-        }
-    }
-
-    return 0;
+    return;
 }

 void virtio_cleanup(VirtIODevice *vdev)
@@ -3722,6 +3733,11 @@ static void virtio_vmstate_change(void *opaque,
bool running, RunState state)
     BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
     bool backend_run = running && virtio_device_started(vdev, vdev->status);
+
+    if (vdev->delay_check) {
+        virtio_load_check_delay(vdev);
+        vdev->delay_check = false;
+    }
     vdev->vm_running = running;

     if (backend_run) {
@@ -3789,6 +3805,7 @@ void virtio_init(VirtIODevice *vdev, uint16_t
device_id, size_t config_size)
             virtio_vmstate_change, vdev);
     vdev->device_endian = virtio_default_endian();
     vdev->use_guest_notifier_mask = true;
+    vdev->delay_check = false;
 }

 /*
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index acfd4df125..269e80d04a 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -135,6 +135,8 @@ struct VirtIODevice
     AddressSpace *dma_as;
     QLIST_HEAD(, VirtQueue) *vector_queues;
     QTAILQ_ENTRY(VirtIODevice) next;
+    /* @delay_check: delay checks in virtio_load */
+    bool delay_check;

I think it covers more than the check?  It also initializes variables like
used_idx and shadow_avail_idx.  I'm not sure how vital they are, but I'd
just avoid using the word "check" if not sure (e.g. "load_delay", or
"load_finalize"?).

OK. I prefer to use "load_finalize".

Thanks!

 };

 struct VirtioDeviceClass {