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

[Cornelia Huck]

On Wed, 22 Feb 2017 11:19:45 +0100
Paolo Bonzini <address@hidden> wrote:

> On 22/02/2017 10:16, Cornelia Huck wrote:
> >> +
> >> +            /* 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()?
> 
> virtio_queue_set_addr isn't called at all, is it?
> 
> The alternative would be to call virtio_init_region_cache from a
> postload callback of vmstate_virtqueue, but Stefan's patch is fine too IMHO.

What I meant was:

We currently have

set_addr -> update_rings -> init_region_cache

and for virtio-1

set_rings -> init_region_cache

The code with Stefan's fix now sets up a region cache via update_rings
and replaces it later via the call introduced above - which is fine,
but adds an extra iteration.

If we move to

set_addr -> update_rings
            init_region_cache

we get the same call pattern for both legacy and virtio-1 layouts and
save the extra iteration. But we can do that in an additional cleanup
patch, and I think the patch is fine as-is, so

Reviewed-by: Cornelia Huck <address@hidden>