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? [..] > 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
