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

[Michael S. Tsirkin]

On Mon, Mar 27, 2023 at 01:06:19PM +0200, Cornelia Huck wrote:
> 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 :)

Yea ... an ack would be appreciated.

-- 
MST