On Wed, Mar 22 2023, Halil Pasic <[email protected]> wrote:
> On Wed, 22 Mar 2023 10:52:31 +0100
> Cornelia Huck <[email protected]> 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 :)
