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 <xuchuangxc...@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
