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

[Cornelia Huck]

On Mon, Mar 13 2023, Carlos López <clopez@suse.de> wrote:

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

I think I had been looking at the code for too long :(

>
> 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);
>              }

Yes, I think we need to follow the same logic.

> 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()?

IIUC, only modern drivers will write to the modern bar.