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

[Peter Xu]

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
> 

-- 
Peter Xu