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

[Cornelia Huck]

On Wed, Mar 22 2023, Halil Pasic <pasic@linux.ibm.com> wrote:

> On Wed, 22 Mar 2023 10:52:31 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> [..]
>> >
>> > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
>> > index e33e5207ab..f44de1a8c1 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_init_region_cache(vdev, index);  
>> 
>> Hmm... this is not wrong, but looking at it again, I see that the guest
>> has no way to change num after our last call to
>> virtio_init_region_cache() (while setting up the queue addresses.) IOW,
>> this introduces an extra round trip that is not really needed.
>> 
>
> I don't quite understand. AFAIU the virtio_init_region_cache() would see
> the (new) queue addresses but not the new size (num). Yes virtio-ccw
> already knows the new num but it is yet to call
> to put it into vdev->vq[n].vring.num from where
> virtio_init_region_cache() picks it up.
>
> If we were to first virtio_queue_set_num() and only then the address
> I would understand. But with the code as is, I don't. Am I missing
> something?

Hrm, virtio_queue_set_rings() doesn't pass num, I thought it did... I
wonder whether ordering virtio_queue_set_num() before it would be better
anyway (if the guest gave us an invalid num, we don't need to setup any
addresses and init any caches).

Smth like

if (info) {
   if (desc) {
      if (virtio_queue_get_max_num(...) < num) {
          return -EINVAL;
      }
      virtio_queue_set_num(...);
   }
   virtio_queue_set_rings(...);
} else { /* legacy */
   if (desc && virtio_queue_get_max_num(...) > num) {
       return -EINVAL;
   }
   virtio_queue_set_addr(...);
}
virtio_queue_set_vector(vdev, index, desc ? index : VIRTIO_NO_VECTOR);

might be easier to follow than the current code.

Or we could just go with this patch, which has the advantage of already
existing :)