Re: [PATCH] hw/virtio: Fix obtain the buffer id from the last descriptor

[Eugenio Perez Martin]

On Thu, May 9, 2024 at 4:20 AM Wafer  wrote:
>
>
>
> On Thu, May, 2024 at 2:21 AM Michael S. Tsirkin  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  wrote:
> > > >
> > > > The virtio-1.3 specification
> > > >  
> > > > 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 
> > > > ---
> > > >  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
> > > >
>

Re: [PATCH] hw/virtio: Fix obtain the buffer id from the last descriptor

[Eugenio Perez Martin]

On Thu, May 9, 2024 at 6:32 AM Wafer  wrote:
>
>
>
> On Wed, May 08, 2024 at 12:01 PM Jason Wang  wrote:
> >
> > On Mon, Apr 22, 2024 at 9:41 AM Wafer  wrote:
> > >
> > > The virtio-1.3 specification
> > >  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.
> > >
> > > Fixes: 86044b24e8 ("virtio: basic packed virtqueue support")
> > >
> > > Signed-off-by: Wafer 
> > > ---
> > >  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;
> > > +}
> >
> > It looks to me we can move this out of the loop.
> >
> > Others look good.
> >
> > Thanks
> >
>
> Thank you for your suggestion, I'll move out.
>

Please add my

Reviewed-by: Eugenio Pérez 

When you do.

Thanks!


> > > +
> > >  rc = virtqueue_packed_read_next_desc(vq, &desc, desc_cache, max,
> > &i,
> > >   desc_cache ==
> > >   &indirect_desc_cache);
> > > --
> > > 2.27.0
> > >
>

RE: [PATCH] hw/virtio: Fix obtain the buffer id from the last descriptor

[Wafer]

On Wed, May 08, 2024 at 12:01 PM Jason Wang  wrote:
> 
> On Mon, Apr 22, 2024 at 9:41 AM Wafer  wrote:
> >
> > The virtio-1.3 specification
> >  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.
> >
> > Fixes: 86044b24e8 ("virtio: basic packed virtqueue support")
> >
> > Signed-off-by: Wafer 
> > ---
> >  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;
> > +}
> 
> It looks to me we can move this out of the loop.
> 
> Others look good.
> 
> Thanks
> 

Thank you for your suggestion, I'll move out.

> > +
> >  rc = virtqueue_packed_read_next_desc(vq, &desc, desc_cache, max,
> &i,
> >   desc_cache ==
> >   &indirect_desc_cache);
> > --
> > 2.27.0
> >

RE: [PATCH] hw/virtio: Fix obtain the buffer id from the last descriptor

[Wafer]

On Thu, May, 2024 at 2:21 AM Michael S. Tsirkin  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  wrote:
> > >
> > > The virtio-1.3 specification
> > >  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.

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

Re: [PATCH] hw/virtio: Fix obtain the buffer id from the last descriptor

[Michael S. Tsirkin]

On Wed, May 08, 2024 at 02:56:11PM +0200, Eugenio Perez Martin wrote:
> On Mon, Apr 22, 2024 at 3:41 AM Wafer  wrote:
> >
> > The virtio-1.3 specification
> >  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?
> 
> 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.

> > Fixes: 86044b24e8 ("virtio: basic packed virtqueue support")
> >
> > Signed-off-by: Wafer 
> > ---
> >  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
> >

Re: [PATCH] hw/virtio: Fix obtain the buffer id from the last descriptor

[Eugenio Perez Martin]

On Mon, Apr 22, 2024 at 3:41 AM Wafer  wrote:
>
> The virtio-1.3 specification
>  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?

If it breaks I guess this requires more thinking. I didn't check DPDK,
neither as driver nor as vhost-user device.

Thanks!

> Fixes: 86044b24e8 ("virtio: basic packed virtqueue support")
>
> Signed-off-by: Wafer 
> ---
>  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
>

Re: [PATCH] hw/virtio: Fix obtain the buffer id from the last descriptor

[Jason Wang]

On Mon, Apr 22, 2024 at 9:41 AM Wafer  wrote:
>
> The virtio-1.3 specification
>  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.
>
> Fixes: 86044b24e8 ("virtio: basic packed virtqueue support")
>
> Signed-off-by: Wafer 
> ---
>  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;
> +}

It looks to me we can move this out of the loop.

Others look good.

Thanks

> +
>  rc = virtqueue_packed_read_next_desc(vq, &desc, desc_cache, max, &i,
>   desc_cache ==
>   &indirect_desc_cache);
> --
> 2.27.0
>

[PATCH] hw/virtio: Fix obtain the buffer id from the last descriptor

[Wafer]

The virtio-1.3 specification
 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.

Fixes: 86044b24e8 ("virtio: basic packed virtqueue support")

Signed-off-by: Wafer 
---
 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