On 4/9/24 1:32 Eugenio Perez Martin <epere...@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 <wa...@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 <epere...@redhat.com>
> > Signed-off-by: Wafer <wa...@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
> >
