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

From: "Michael S. Tsirkin" <mst@redhat.com>
To: Wafer <wafer@jaguarmicro.com>
Cc: qemu-devel@nongnu.org, angus.chen@jaguarmicro.com,
	Jason Wang <jasowang@redhat.com>
Subject: Re: [PATCH] hw/virtio: Fix packed virtqueue flush used_idx
Date: Wed, 3 Apr 2024 01:58:30 -0400	[thread overview]
Message-ID: <20240403012519-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <20240327061518.13200-1-wafer@jaguarmicro.com>

On Wed, Mar 27, 2024 at 02:15:18PM +0800, Wafer wrote:
> For indirect descriptors the elelm->ndescs was one,
> For direct descriptors the elele->ndesc was the numbe of entries.
> elem->ndescs = (desc_cache == &indirect_desc_cache) ? 1 : elem_entries;
> 
> When flushing multiple elemes,
> the used_idx should be added to all the privious numeric entry value.
> 
> Signed-off-by: Wafer <wafer@jaguarmicro.com>

Thanks for the patch.
It's kind of hard to figure out what you are trying to say
with all the typos and grammar errors in the commit log.
What's up with that?

Please describe the following in the commit log:
- current behaviour is abc
- this is wrong because the virtio spec says def
- as a result we observed guest doing pqr and then stu
- to fix do ghi
- with this fix the guest does xyz as expected
- tested by klm

Also I think you might want to add:

Fixes: 86044b24e8 ("virtio: basic packed virtqueue support")
Cc: "Jason Wang" <jasowang@redhat.com>

> ---
>  hw/virtio/virtio.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index d229755eae..44f1d2fcfc 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -957,12 +957,17 @@ static void virtqueue_packed_flush(VirtQueue *vq, unsigned int count)
>          return;
>      }
>  
> +    /*
> +     * When the descriptor's flag was 'INDIRECT', the value of 'ndescs' is one.
> +     * When the descriptor's flag was 'chain', the value of 'ndescs'
> +     * is the number of entries.
> +     */

There's no such thing as "the flag" - descriptors do have a "flags" field
though. And there's no 'chain' value either.
maybe just "

	For indirect elems, ndescs is 1. For all other elems, ndescs is the
	number of descriptors chained by NEXT (as set in virtqueue_packed_pop).

> +    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;

The patch itself seems correct to me.

> -- 
> 2.27.0