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

[Halil Pasic]

On Mon, 27 Mar 2023 08:29:09 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

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

I'm in favor of taking the existing one. We can still do the refactoring
afterwards and also get rid of the then redundant update. That way
the git history would also "tell the story".

For the s390x part:
Acked-by: Halil Pasic <pasic@linux.ibm.com>