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

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

Posted by Wafer 1 year, 9 months ago

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.

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

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

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

Posted by Eugenio Perez Martin 1 year, 9 months ago

On Mon, Apr 22, 2024 at 3:41 AM Wafer <wafer@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?

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

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

Posted by Michael S. Tsirkin 1 year, 9 months ago

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

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

Posted by Wafer 1 year, 9 months ago


On Thu, May, 2024 at 2:21 AM Michael S. Tsirkin <mst@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 <wafer@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.

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

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

Posted by Eugenio Perez Martin 1 year, 9 months ago

On Thu, May 9, 2024 at 4:20 AM Wafer <wafer@jaguarmicro.com> wrote:
>
>
>
> On Thu, May, 2024 at 2:21 AM Michael S. Tsirkin <mst@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 <wafer@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 <wafer@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
> > > >
>

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

Posted by Jason Wang 1 year, 9 months ago

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

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
>

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

Posted by Wafer 1 year, 9 months ago


On Wed, May 08, 2024 at 12:01 PM Jason Wang <jasowang@redhat.com> wrote:
> 
> On Mon, Apr 22, 2024 at 9:41 AM Wafer <wafer@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.
> >
> > Fixes: 86044b24e8 ("virtio: basic packed virtqueue support")
> >
> > Signed-off-by: Wafer <wafer@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;
> > +        }
> 
> 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

Posted by Eugenio Perez Martin 1 year, 9 months ago

On Thu, May 9, 2024 at 6:32 AM Wafer <wafer@jaguarmicro.com> wrote:
>
>
>
> On Wed, May 08, 2024 at 12:01 PM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Mon, Apr 22, 2024 at 9:41 AM Wafer <wafer@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.
> > >
> > > Fixes: 86044b24e8 ("virtio: basic packed virtqueue support")
> > >
> > > Signed-off-by: Wafer <wafer@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;
> > > +        }
> >
> > 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 <eperezma@redhat.com>

When you do.

Thanks!


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