On Mon, Mar 13 2023, Carlos López <clo...@suse.de> wrote:
> 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.
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.
