| Message ID | 20240422014041.5706-2-wafer@jaguarmicro.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/virtio: Fix obtain the buffer id from the last descriptor | expand |
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 >
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 >
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 > >
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 > > >
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 > >
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 > > > > >
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 > > > >
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);
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(+)