Re: [PATCH v4] hw/virtio: Fix packed virtqueue flush used_idx

[Wafer]

On 4/9/24 1:32 Eugenio Perez Martin <eperezma@redhat.com> wrote:
> 
> External Mail: This email originated from OUTSIDE of the organization!
> Do not click links, open attachments or provide ANY information unless you
> recognize the sender and know the content is safe.
> 
> 
> On Sun, Apr 7, 2024 at 3:56 AM Wafer <wafer@jaguarmicro.com> wrote:
> >
> 
> Let me suggest a more generic description for the patch:
> 
> In the event of writing many chains of descriptors, the device must write just
> the id of the last buffer in the descriptor chain, skip forward the number of
> descriptors in the chain, and then repeat the operations for the rest of 
> chains.
> 
> Current QEMU code writes all the buffers id consecutively, and then skip all
> the buffers altogether. This is a bug, and can be reproduced with a VirtIONet
> device with _F_MRG_RXBUB and without _F_INDIRECT_DESC...
> ---
> 
> And then your description, particularly for VirtIONet, is totally fine. Feel 
> free
> to make changes to the description or suggest a better wording.
> 
> Thanks!

Thank you for your suggestion.

I will add your description  and Suggested-by to the commit log.

Thanks!

> 
> > If a virtio-net device has the VIRTIO_NET_F_MRG_RXBUF feature but not
> > the VIRTIO_RING_F_INDIRECT_DESC feature, 'VirtIONetQueue->rx_vq' will
> > use the merge feature to store data in multiple 'elems'.
> > The 'num_buffers' in the virtio header indicates how many elements are
> merged.
> > If the value of 'num_buffers' is greater than 1, all the merged
> > elements will be filled into the descriptor ring.
> > The 'idx' of the elements should be the value of 'vq->used_idx' plus
> 'ndescs'.
> >
> > Fixes: 86044b24e8 ("virtio: basic packed virtqueue support")
> > Acked-by: Eugenio Pérez <eperezma@redhat.com>
> > Signed-off-by: Wafer <wafer@jaguarmicro.com>
> >
> > ---
> > Changes in v4:
> >   - Add Acked-by.
> >
> > Changes in v3:
> >   - Add the commit-ID of the introduced problem in commit message.
> >
> > Changes in v2:
> >   - Clarify more in commit message.
> > ---
> >  hw/virtio/virtio.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index
> > fb6b4ccd83..cab5832cac 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -957,12 +957,20 @@ static void virtqueue_packed_flush(VirtQueue
> *vq, unsigned int count)
> >          return;
> >      }
> >
> > +    /*
> > +     * For indirect element's 'ndescs' is 1.
> > +     * For all other elemment's 'ndescs' is the
> > +     * number of descriptors chained by NEXT (as set in
> virtqueue_packed_pop).
> > +     * So When the 'elem' be filled into the descriptor ring,
> > +     * The 'idx' of this 'elem' shall be
> > +     * the value of 'vq->used_idx' plus the 'ndescs'.
> > +     */
> > +    ndescs += vq->used_elems[0].ndescs;
> >      for (i = 1; i < count; i++) {
> > -        virtqueue_packed_fill_desc(vq, &vq->used_elems[i], i, false);
> > +        virtqueue_packed_fill_desc(vq, &vq->used_elems[i], ndescs,
> > + false);
> >          ndescs += vq->used_elems[i].ndescs;
> >      }
> >      virtqueue_packed_fill_desc(vq, &vq->used_elems[0], 0, true);
> > -    ndescs += vq->used_elems[0].ndescs;
> >
> >      vq->inuse -= ndescs;
> >      vq->used_idx += ndescs;
> > --
> > 2.27.0
> >