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