Re: [PATCH] virtio: refresh vring region cache after updating a virtqueue size

[Carlos López]

On 9/3/23 11:43, Cornelia Huck wrote:
> On Thu, Mar 02 2023, Carlos López <clopez@suse.de> wrote:
> > Fix this by calling virtio_queue_update_rings() after
> > virtio_queue_set_num() if we are not already calling
> > virtio_queue_set_rings().
>
> Don't we instead need to call virtio_init_region_cache() to update the
> caches? virtio_queue_set_rings() will calculate avail and used from
> desc, which looks wrong for modern devices.

I take it you meant virtio_queue_update_rings() instead of 
virtio_queue_set_rings()? Otherwise I'm not sure what you mean.

If this is the case sure - there is this same kind of logic in 
virtio_load():

            /*
             * VIRTIO-1 devices migrate desc, used, and avail ring 
addresses so
             * only the region cache needs to be set up.  Legacy 
devices need
             * to calculate used and avail ring addresses based on the desc
             * address.
             */
            if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
                virtio_init_region_cache(vdev, i);
            } else {
                virtio_queue_update_rings(vdev, i);
            }

This will require making virtio_init_region_cache() non static of course.

> > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> > index e33e5207ab..89891ac58a 100644
> > --- a/hw/s390x/virtio-ccw.c
> > +++ b/hw/s390x/virtio-ccw.c
> > @@ -237,6 +237,7 @@ static int virtio_ccw_set_vqs(SubchDev *sch, VqInfoBlock 
> > *info,
> >                  return -EINVAL;
> >              }
> >              virtio_queue_set_num(vdev, index, num);
> > +            virtio_queue_update_rings(vdev, index);
>
> Note that this is the non-legacy path.
So if I understand correctly, in virtio_mmio_write() we check via 
proxy->legacy, and in virtio_ccw_set_vqs() we are in the non-legacy 
path. What about virtio_pci_common_write()?

--
Carlos López
Security Engineer
SUSE Software Solutions