On Mon, Mar 27, 2023 at 01:06:19PM +0200, Cornelia Huck wrote:
> On Wed, Mar 22 2023, Halil Pasic <pa...@linux.ibm.com> wrote:
>
> > On Wed, 22 Mar 2023 10:52:31 +0100
> > Cornelia Huck <coh...@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
