Re: [Qemu-devel] [PATCH 2/2] virtio: add missing region cache init in virtio_load()

[Cornelia Huck]

On Tue, 21 Feb 2017 18:59:01 +0000
Stefan Hajnoczi <address@hidden> wrote:

> Commit 97cd965c070152bc626c7507df9fb356bbe1cd81 ("virtio: use
> VRingMemoryRegionCaches for avail and used rings") switched to a memory
> region cache to avoid repeated map/unmap operations.
> 
> The virtio_load() process is a little tricky because vring addresses are
> serialized in two separate places.  VIRTIO 1.0 devices serialize desc
> and then a subsection with used and avail.  Legacy devices only
> serialize desc.
> 
> Live migration of VIRTIO 1.0 devices fails on the destination host with:
> 
>   VQ 0 size 0x80 < last_avail_idx 0x12f8 - used_idx 0x0
>   Failed to load virtio-blk:virtio
>   error while loading state for instance 0x0 of device 
> '0000:00:04.0/virtio-blk'
> 
> This happens because the memory region cache is only initialized after
> desc is loaded and not after the used and avail subsection is loaded.
> If the guest chose memory addresses that don't match the legacy ring
> layout then the wrong guest memory location is accessed.
> 
> Clarify comments about VIRTIO 1.0 and force memory region cache
> initialization at the point where all ring addresses are known.

Yes, the XXX comments should have been changed when the subsection for
used/avail had been introduced.

> 
> Cc: Dr. David Alan Gilbert <address@hidden>
> Cc: Paolo Bonzini <address@hidden>
> Signed-off-by: Stefan Hajnoczi <address@hidden>
> ---
>  hw/virtio/virtio.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index da5c6fe..5bbc34b 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1853,7 +1853,10 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
>          if (k->has_variable_vring_alignment) {
>              qemu_put_be32(f, vdev->vq[i].vring.align);
>          }
> -        /* XXX virtio-1 devices */
> +        /*
> +         * Save desc now, the rest of the ring addresses are saved in
> +         * subsections for VIRTIO-1 devices.
> +         */
>          qemu_put_be64(f, vdev->vq[i].vring.desc);
>          qemu_put_be16s(f, &vdev->vq[i].last_avail_idx);
>          if (k->save_queue) {
> @@ -1995,7 +1998,10 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int 
> version_id)
>          vdev->vq[i].notification = true;
> 
>          if (vdev->vq[i].vring.desc) {
> -            /* XXX virtio-1 devices */
> +            /*
> +             * VIRTIO-1 devices may not have final ring addresses here.  The
> +             * used and avail ring addresses are loaded in subsections later.
> +             */
>              virtio_queue_update_rings(vdev, i);
>          } else if (vdev->vq[i].last_avail_idx) {
>              error_report("VQ %d address 0x0 "
> @@ -2062,6 +2068,10 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int 
> version_id)
>      for (i = 0; i < num; i++) {
>          if (vdev->vq[i].vring.desc) {
>              uint16_t nheads;
> +
> +            /* All ring addresses have been loaded now... */
> +            virtio_init_region_cache(vdev, i);

It feels a bit weird that the region cache was already initialized when
update rings was called. Should the call to init_region_cache be moved
from update_rings() to virtio_queue_set_addr()?

> +
>              nheads = vring_avail_idx(&vdev->vq[i]) - 
> vdev->vq[i].last_avail_idx;
>              /* Check it isn't doing strange things with descriptor numbers. 
> */
>              if (nheads > vdev->vq[i].vring.num) {