| Message ID | 20241122020002.564-1-Wafer@jaguarmicro.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | hw/virtio: Fix getting the correct ring number on loading | expand |
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 "
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 " >
> -----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 "
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 "