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

[Chuang Xu]

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>
> > ---
> >  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 {
> > -- 
> > 2.20.1
> >
> >       
>