hw/virtio: Fix getting the correct ring number on loading

[PATCH] hw/virtio: Fix getting the correct ring number on loading

Posted by Wafer 1 day, 10 hours ago

From: Wafer Xie <wafer@jaguarmicro.com>

The virtio-1.2 specification writes:

2.7.6 The Virtqueue Available Ring:
"idx field indicates where the driver would put the next descriptor entry
in the ring (modulo the queue size). This starts at 0, and increases"

The idx will increase from 0 to 0xFFFF and repeat,
So idx may be less than last_avail_idx.

Fixes: 616a6552 (virtio: add endian-ambivalent support to VirtIODevice)

Signed-off-by: Wafer Xie <wafer@jaguarmicro.com>
---
 hw/virtio/virtio.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index a26f18908e..ae7d407113 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -3362,7 +3362,13 @@ virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
                 continue;
             }
 
-            nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx;
+            if (vring_avail_idx(&vdev->vq[i]) >= vdev->vq[i].last_avail_idx) {
+                nheads = vring_avail_idx(&vdev->vq[i]) -
+                         vdev->vq[i].last_avail_idx;
+            } else {
+                nheads = UINT16_MAX - vdev->vq[i].last_avail_idx +
+                         vring_avail_idx(&vdev->vq[i]) + 1;
+            }
             /* Check it isn't doing strange things with descriptor numbers. */
             if (nheads > vdev->vq[i].vring.num) {
                 virtio_error(vdev, "VQ %d size 0x%x Guest index 0x%x "
-- 
2.31.1

Re: [PATCH] hw/virtio: Fix getting the correct ring number on loading

Posted by Philippe Mathieu-Daudé 1 day, 5 hours ago

Hi Wafer,

On 22/11/24 03:00, Wafer wrote:
> From: Wafer Xie <wafer@jaguarmicro.com>
> 
> The virtio-1.2 specification writes:
> 
> 2.7.6 The Virtqueue Available Ring:
> "idx field indicates where the driver would put the next descriptor entry
> in the ring (modulo the queue size). This starts at 0, and increases"

"modulo" ...

> 
> The idx will increase from 0 to 0xFFFF and repeat,
> So idx may be less than last_avail_idx.
> 
> Fixes: 616a6552 (virtio: add endian-ambivalent support to VirtIODevice)

This commit is only about endianness... Do you mean 1abeb5a65d
("virtio: fix up VQ checks") or 258dc7c96b ("virtio: sanity-check
available index")?

> 
> Signed-off-by: Wafer Xie <wafer@jaguarmicro.com>
> ---
>   hw/virtio/virtio.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index a26f18908e..ae7d407113 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -3362,7 +3362,13 @@ virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
>                   continue;
>               }
>   
> -            nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx;
> +            if (vring_avail_idx(&vdev->vq[i]) >= vdev->vq[i].last_avail_idx) {
> +                nheads = vring_avail_idx(&vdev->vq[i]) -
> +                         vdev->vq[i].last_avail_idx;
> +            } else {
> +                nheads = UINT16_MAX - vdev->vq[i].last_avail_idx +
> +                         vring_avail_idx(&vdev->vq[i]) + 1;
> +            }

...           nheads %= UINT16_MAX; ?

>               /* Check it isn't doing strange things with descriptor numbers. */
>               if (nheads > vdev->vq[i].vring.num) {
>                   virtio_error(vdev, "VQ %d size 0x%x Guest index 0x%x "

RE: [PATCH] hw/virtio: Fix getting the correct ring number on loading

Posted by Wafer 3 hours ago


> -----Original Message-----
> From: Philippe Mathieu-Daudé <philmd@linaro.org>
> Sent: 2024年11月22日 15:02
> To: Wafer <wafer@jaguarmicro.com>; mst@redhat.com;
> jasowang@redhat.com; groug@kaod.org
> Cc: eperezma@redhat.com; qemu-devel@nongnu.org; Angus Chen
> <angus.chen@jaguarmicro.com>
> Subject: Re: [PATCH] hw/virtio: Fix getting the correct ring number on loading
> 
> External Mail: This email originated from OUTSIDE of the organization!
> Do not click links, open attachments or provide ANY information unless you
> recognize the sender and know the content is safe.
> 
> 
> Hi Wafer,
> 
> On 22/11/24 03:00, Wafer wrote:
> > From: Wafer Xie <wafer@jaguarmicro.com>
> >
> > The virtio-1.2 specification writes:
> >
> > 2.7.6 The Virtqueue Available Ring:
> > "idx field indicates where the driver would put the next descriptor
> > entry in the ring (modulo the queue size). This starts at 0, and increases"
> 
> "modulo" ...
> 
> >
> > The idx will increase from 0 to 0xFFFF and repeat, So idx may be less
> > than last_avail_idx.
> >
> > Fixes: 616a6552 (virtio: add endian-ambivalent support to
> > VirtIODevice)
> 
> This commit is only about endianness... Do you mean 1abeb5a65d
> ("virtio: fix up VQ checks") or 258dc7c96b ("virtio: sanity-check available
> index")?

Thanks, I mean 258dc7c96b ("virtio: sanity-check available index")
I will make changes in the next version.

> 
> >
> > Signed-off-by: Wafer Xie <wafer@jaguarmicro.com>
> > ---
> >   hw/virtio/virtio.c | 8 +++++++-
> >   1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index
> > a26f18908e..ae7d407113 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -3362,7 +3362,13 @@ virtio_load(VirtIODevice *vdev, QEMUFile *f, int
> version_id)
> >                   continue;
> >               }
> >
> > -            nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx;
> > +            if (vring_avail_idx(&vdev->vq[i]) >= vdev->vq[i].last_avail_idx) {
> > +                nheads = vring_avail_idx(&vdev->vq[i]) -
> > +                         vdev->vq[i].last_avail_idx;
> > +            } else {
> > +                nheads = UINT16_MAX - vdev->vq[i].last_avail_idx +
> > +                         vring_avail_idx(&vdev->vq[i]) + 1;
> > +            }
> 
> ...           nheads %= UINT16_MAX; ?

nheads cannot exceed UINT16_MAX, but is invalid if it exceed vring.num

> >               /* Check it isn't doing strange things with descriptor numbers. */
> >               if (nheads > vdev->vq[i].vring.num) {
> >                   virtio_error(vdev, "VQ %d size 0x%x Guest index 0x%x "

Re: [PATCH] hw/virtio: Fix getting the correct ring number on loading

Posted by Greg Kurz 1 day ago

On Fri, 22 Nov 2024 08:01:34 +0100
Philippe Mathieu-Daudé <philmd@linaro.org> wrote:

> Hi Wafer,
> 
> On 22/11/24 03:00, Wafer wrote:
> > From: Wafer Xie <wafer@jaguarmicro.com>
> > 
> > The virtio-1.2 specification writes:
> > 
> > 2.7.6 The Virtqueue Available Ring:
> > "idx field indicates where the driver would put the next descriptor entry
> > in the ring (modulo the queue size). This starts at 0, and increases"
> 
> "modulo" ...
> 
> > 
> > The idx will increase from 0 to 0xFFFF and repeat,
> > So idx may be less than last_avail_idx.
> > 
> > Fixes: 616a6552 (virtio: add endian-ambivalent support to VirtIODevice)
> 
> This commit is only about endianness... 

I totally agree with you Philippe (bonjour BTW). Commit 616a6552 simply
moves pre-existing VQ check code around. Contributor should `git blame`
deeper ;-)

> Do you mean 1abeb5a65d
> ("virtio: fix up VQ checks") or 258dc7c96b ("virtio: sanity-check
> available index")?
> 

The code this patch is changing was originally introduced by the latter.

> > 
> > Signed-off-by: Wafer Xie <wafer@jaguarmicro.com>
> > ---
> >   hw/virtio/virtio.c | 8 +++++++-
> >   1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index a26f18908e..ae7d407113 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -3362,7 +3362,13 @@ virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
> >                   continue;
> >               }
> >   
> > -            nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx;
> > +            if (vring_avail_idx(&vdev->vq[i]) >= vdev->vq[i].last_avail_idx) {
> > +                nheads = vring_avail_idx(&vdev->vq[i]) -
> > +                         vdev->vq[i].last_avail_idx;
> > +            } else {
> > +                nheads = UINT16_MAX - vdev->vq[i].last_avail_idx +
> > +                         vring_avail_idx(&vdev->vq[i]) + 1;
> > +            }
> 
> ...           nheads %= UINT16_MAX; ?
> 

Exactly and since everything is uint16_t there, we get that for free with
the existing code base. IOW both arms of the if produce exactly the same
result... I don't know the motivation behind this patch but it does not
change anything.

Cheers,

--
Greg

> >               /* Check it isn't doing strange things with descriptor numbers. */
> >               if (nheads > vdev->vq[i].vring.num) {
> >                   virtio_error(vdev, "VQ %d size 0x%x Guest index 0x%x "
>