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

Carlos López posted 1 patch 1 year ago
hw/s390x/virtio-ccw.c      | 1 +
hw/virtio/virtio-mmio.c    | 1 +
hw/virtio/virtio-pci.c     | 1 +
hw/virtio/virtio.c         | 2 +-
include/hw/virtio/virtio.h | 1 +
5 files changed, 5 insertions(+), 1 deletion(-)
[PATCH v2] virtio: refresh vring region cache after updating a virtqueue size
Posted by Carlos López 1 year ago
When a virtqueue size is changed by the guest via
virtio_queue_set_num(), its region cache is not automatically updated.
If the size was increased, this could lead to accessing the cache out
of bounds. For example, in vring_get_used_event():

    static inline uint16_t vring_get_used_event(VirtQueue *vq)
    {
        return vring_avail_ring(vq, vq->vring.num);
    }

    static inline uint16_t vring_avail_ring(VirtQueue *vq, int i)
    {
        VRingMemoryRegionCaches *caches = vring_get_region_caches(vq);
        hwaddr pa = offsetof(VRingAvail, ring[i]);

        if (!caches) {
            return 0;
        }

        return virtio_lduw_phys_cached(vq->vdev, &caches->avail, pa);
    }

vq->vring.num will be greater than caches->avail.len, which will
trigger a failed assertion down the call path of
virtio_lduw_phys_cached().

Fix this by calling virtio_init_region_cache() after
virtio_queue_set_num() if we are not already calling
virtio_queue_set_rings(). In the legacy path this is already done by
virtio_queue_update_rings().

Signed-off-by: Carlos López <clopez@suse.de>
---
v2: use virtio_init_region_cache() instead of
virtio_queue_update_rings() in the path for modern devices.

 hw/s390x/virtio-ccw.c      | 1 +
 hw/virtio/virtio-mmio.c    | 1 +
 hw/virtio/virtio-pci.c     | 1 +
 hw/virtio/virtio.c         | 2 +-
 include/hw/virtio/virtio.h | 1 +
 5 files changed, 5 insertions(+), 1 deletion(-)

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);
         } else if (virtio_queue_get_num(vdev, index) > num) {
             /* Fail if we don't have a big enough queue. */
             return -EINVAL;
diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
index 23ba625eb6..c2c6d85475 100644
--- a/hw/virtio/virtio-mmio.c
+++ b/hw/virtio/virtio-mmio.c
@@ -354,6 +354,7 @@ static void virtio_mmio_write(void *opaque, hwaddr offset, uint64_t value,
         if (proxy->legacy) {
             virtio_queue_update_rings(vdev, vdev->queue_sel);
         } else {
+            virtio_init_region_cache(vdev, vdev->queue_sel);
             proxy->vqs[vdev->queue_sel].num = value;
         }
         break;
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 247325c193..02fb84a8fa 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1554,6 +1554,7 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr,
         proxy->vqs[vdev->queue_sel].num = val;
         virtio_queue_set_num(vdev, vdev->queue_sel,
                              proxy->vqs[vdev->queue_sel].num);
+        virtio_init_region_cache(vdev, vdev->queue_sel);
         break;
     case VIRTIO_PCI_COMMON_Q_MSIX:
         vector = virtio_queue_vector(vdev, vdev->queue_sel);
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 98c4819fcc..272d930721 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -226,7 +226,7 @@ static void virtio_virtqueue_reset_region_cache(struct VirtQueue *vq)
     }
 }
 
-static void virtio_init_region_cache(VirtIODevice *vdev, int n)
+void virtio_init_region_cache(VirtIODevice *vdev, int n)
 {
     VirtQueue *vq = &vdev->vq[n];
     VRingMemoryRegionCaches *old = vq->vring.caches;
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 77c6c55929..fed5fff049 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -309,6 +309,7 @@ int virtio_get_num_queues(VirtIODevice *vdev);
 void virtio_queue_set_rings(VirtIODevice *vdev, int n, hwaddr desc,
                             hwaddr avail, hwaddr used);
 void virtio_queue_update_rings(VirtIODevice *vdev, int n);
+void virtio_init_region_cache(VirtIODevice *vdev, int n);
 void virtio_queue_set_align(VirtIODevice *vdev, int n, int align);
 void virtio_queue_notify(VirtIODevice *vdev, int n);
 uint16_t virtio_queue_vector(VirtIODevice *vdev, int n);
-- 
2.35.3


Re: [PATCH v2] virtio: refresh vring region cache after updating a virtqueue size
Posted by Cornelia Huck 1 year ago
On Fri, Mar 17 2023, Carlos López <clopez@suse.de> wrote:

> When a virtqueue size is changed by the guest via
> virtio_queue_set_num(), its region cache is not automatically updated.
> If the size was increased, this could lead to accessing the cache out
> of bounds. For example, in vring_get_used_event():
>
>     static inline uint16_t vring_get_used_event(VirtQueue *vq)
>     {
>         return vring_avail_ring(vq, vq->vring.num);
>     }
>
>     static inline uint16_t vring_avail_ring(VirtQueue *vq, int i)
>     {
>         VRingMemoryRegionCaches *caches = vring_get_region_caches(vq);
>         hwaddr pa = offsetof(VRingAvail, ring[i]);
>
>         if (!caches) {
>             return 0;
>         }
>
>         return virtio_lduw_phys_cached(vq->vdev, &caches->avail, pa);
>     }
>
> vq->vring.num will be greater than caches->avail.len, which will
> trigger a failed assertion down the call path of
> virtio_lduw_phys_cached().
>
> Fix this by calling virtio_init_region_cache() after
> virtio_queue_set_num() if we are not already calling
> virtio_queue_set_rings(). In the legacy path this is already done by
> virtio_queue_update_rings().
>
> Signed-off-by: Carlos López <clopez@suse.de>
> ---
> v2: use virtio_init_region_cache() instead of
> virtio_queue_update_rings() in the path for modern devices.
>
>  hw/s390x/virtio-ccw.c      | 1 +
>  hw/virtio/virtio-mmio.c    | 1 +
>  hw/virtio/virtio-pci.c     | 1 +
>  hw/virtio/virtio.c         | 2 +-
>  include/hw/virtio/virtio.h | 1 +
>  5 files changed, 5 insertions(+), 1 deletion(-)
>

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

We can always do any ccw reshuffling on top.
Re: [PATCH v2] virtio: refresh vring region cache after updating a virtqueue size
Posted by Cornelia Huck 1 year ago
On Fri, Mar 17 2023, Carlos López <clopez@suse.de> wrote:

> When a virtqueue size is changed by the guest via
> virtio_queue_set_num(), its region cache is not automatically updated.
> If the size was increased, this could lead to accessing the cache out
> of bounds. For example, in vring_get_used_event():
>
>     static inline uint16_t vring_get_used_event(VirtQueue *vq)
>     {
>         return vring_avail_ring(vq, vq->vring.num);
>     }
>
>     static inline uint16_t vring_avail_ring(VirtQueue *vq, int i)
>     {
>         VRingMemoryRegionCaches *caches = vring_get_region_caches(vq);
>         hwaddr pa = offsetof(VRingAvail, ring[i]);
>
>         if (!caches) {
>             return 0;
>         }
>
>         return virtio_lduw_phys_cached(vq->vdev, &caches->avail, pa);
>     }
>
> vq->vring.num will be greater than caches->avail.len, which will
> trigger a failed assertion down the call path of
> virtio_lduw_phys_cached().
>
> Fix this by calling virtio_init_region_cache() after
> virtio_queue_set_num() if we are not already calling
> virtio_queue_set_rings(). In the legacy path this is already done by
> virtio_queue_update_rings().
>
> Signed-off-by: Carlos López <clopez@suse.de>
> ---
> v2: use virtio_init_region_cache() instead of
> virtio_queue_update_rings() in the path for modern devices.
>
>  hw/s390x/virtio-ccw.c      | 1 +
>  hw/virtio/virtio-mmio.c    | 1 +
>  hw/virtio/virtio-pci.c     | 1 +
>  hw/virtio/virtio.c         | 2 +-
>  include/hw/virtio/virtio.h | 1 +
>  5 files changed, 5 insertions(+), 1 deletion(-)
>
> 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.

>          } else if (virtio_queue_get_num(vdev, index) > num) {
>              /* Fail if we don't have a big enough queue. */
>              return -EINVAL;
> diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
> index 23ba625eb6..c2c6d85475 100644
> --- a/hw/virtio/virtio-mmio.c
> +++ b/hw/virtio/virtio-mmio.c
> @@ -354,6 +354,7 @@ static void virtio_mmio_write(void *opaque, hwaddr offset, uint64_t value,
>          if (proxy->legacy) {
>              virtio_queue_update_rings(vdev, vdev->queue_sel);
>          } else {
> +            virtio_init_region_cache(vdev, vdev->queue_sel);
>              proxy->vqs[vdev->queue_sel].num = value;
>          }
>          break;
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 247325c193..02fb84a8fa 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1554,6 +1554,7 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr,
>          proxy->vqs[vdev->queue_sel].num = val;
>          virtio_queue_set_num(vdev, vdev->queue_sel,
>                               proxy->vqs[vdev->queue_sel].num);
> +        virtio_init_region_cache(vdev, vdev->queue_sel);
>          break;
>      case VIRTIO_PCI_COMMON_Q_MSIX:
>          vector = virtio_queue_vector(vdev, vdev->queue_sel);

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

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.
Re: [PATCH v2] virtio: refresh vring region cache after updating a virtqueue size
Posted by Halil Pasic 1 year ago
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
Re: [PATCH v2] virtio: refresh vring region cache after updating a virtqueue size
Posted by Cornelia Huck 1 year ago
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 :)
Re: [PATCH v2] virtio: refresh vring region cache after updating a virtqueue size
Posted by Michael S. Tsirkin 1 year ago
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.

-- 
MST
Re: [PATCH v2] virtio: refresh vring region cache after updating a virtqueue size
Posted by Halil Pasic 1 year ago
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>
Re: [PATCH v2] virtio: refresh vring region cache after updating a virtqueue size
Posted by Halil Pasic 1 year ago
On Wed, 22 Mar 2023 18:24:33 +0100
Halil Pasic <pasic@linux.ibm.com> wrote:

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

Connie: have you had a chance to have yet another look at this? I
would like to understand the reason for seeing this differently.

Regards,
Halil
Re: [PATCH v2] virtio: refresh vring region cache after updating a virtqueue size
Posted by Cornelia Huck 1 year ago
On Fri, Mar 24 2023, Halil Pasic <pasic@linux.ibm.com> wrote:

> On Wed, 22 Mar 2023 18:24:33 +0100
> Halil Pasic <pasic@linux.ibm.com> wrote:
>
>> > > --- 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?
>
> Connie: have you had a chance to have yet another look at this? I
> would like to understand the reason for seeing this differently.

I'm just back from being sick, please give me some time to work through
my backlog.