On Thu, May 9, 2024 at 4:20 AM Wafer <wa...@jaguarmicro.com> wrote: > > > > On Thu, May, 2024 at 2:21 AM Michael S. Tsirkin <m...@redhat.com> wrote: > > > > On Wed, May 08, 2024 at 02:56:11PM +0200, Eugenio Perez Martin wrote: > > > On Mon, Apr 22, 2024 at 3:41 AM Wafer <wa...@jaguarmicro.com> wrote: > > > > > > > > The virtio-1.3 specification > > > > <https://docs.oasis-open.org/virtio/virtio/v1.3/virtio-v1.3.html> > > > > writes: > > > > 2.8.6 Next Flag: Descriptor Chaining > > > > Buffer ID is included in the last descriptor in the list. > > > > > > > > If the feature (_F_INDIRECT_DESC) has been negotiated, install only > > > > one descriptor in the virtqueue. > > > > Therefor the buffer id should be obtained from the first descriptor. > > > > > > > > In descriptor chaining scenarios, the buffer id should be obtained > > > > from the last descriptor. > > > > > > > > > > This is actually trickier. While it is true the standard mandates it, > > > both linux virtio_ring driver and QEMU trusts the ID will be the first > > > descriptor of the chain. Does merging this change in QEMU without > > > merging the corresponding one in the linux kernel break things? Or am > > > I missing something? > > > > > The linux virtio_ring driver set the buffer id into all the descriptors of > the chain. >
Ok now after reading the driver code again I see how I missed that. Sorry for the noise! > So Bad things can't happen, with this patch, the Linux VirtIO driver can work > properly. > > I have tested it. > > > > If it breaks I guess this requires more thinking. I didn't check DPDK, > > > neither as driver nor as vhost-user device. > > > > > > Thanks! > > > > I think that if the driver is out of spec we should for starters fix it > > ASAP. > > The linux driver is within spec. > > > > > > > Fixes: 86044b24e8 ("virtio: basic packed virtqueue support") > > > > > > > > Signed-off-by: Wafer <wa...@jaguarmicro.com> > > > > --- > > > > hw/virtio/virtio.c | 5 +++++ > > > > 1 file changed, 5 insertions(+) > > > > > > > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index > > > > 871674f9be..f65d4b4161 100644 > > > > --- a/hw/virtio/virtio.c > > > > +++ b/hw/virtio/virtio.c > > > > @@ -1739,6 +1739,11 @@ static void *virtqueue_packed_pop(VirtQueue > > *vq, size_t sz) > > > > goto err_undo_map; > > > > } > > > > > > > > + if (desc_cache != &indirect_desc_cache) { > > > > + /* Buffer ID is included in the last descriptor in the > > > > list. */ > > > > + id = desc.id; > > > > + } > > > > + > > > > rc = virtqueue_packed_read_next_desc(vq, &desc, desc_cache, > > > > max, > > &i, > > > > desc_cache == > > > > &indirect_desc_cache); > > > > -- > > > > 2.27.0 > > > > >