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

[Halil Pasic]

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?

[..]
> OTOH, all other transports need this call, as setting the addresses and
> setting num are two distinct operations. So I think we have two options:
> 
> - apply your patch, and accept that we have the extra round trip for ccw
>   (which should not really be an issue, we're processing a channel
>   command anyway)
> 
> - leave it out for ccw and add a comment why it isn't needed
> 
> (I think I'd prefer to just go ahead with your patch.)
> 

Provided we really don't need it: Why do unnecessary work? I would prefer
the "add a comment solution" because doing unnecessary work is
confusing. If we decide to do the unnecessary (and AFAIU completely
useless) work, I believe we should also add a comment why this is done.

OTOH, my current understanding is that we do need it. Or we need to change
the order of virtio_queue_set_rings() and virtio_queue_set_num() which
may or may not be possible.

> Question (mostly for the other ccw folks): Do you think it is a problem
> that ccw sets up queue addresses and size via one command, while pci and
> mmio set addresses and size independently? I guess not; if anything, not
> setting them in one go may lead to issues like the one this patch is
> fixing.
> 
> 

I tend to agree: I don't think it is a problem.

Regards,
Halil