Hi - On 2016-08-05 at 16:22 Kyle Milka wrote: > This check didn't allow the vring to actually wrap around, if the next > available index was less than the last index, it caused the error to be > triggered. > > Change-Id: I90c26cfd894a9b729e44937e1894ce5bfbf1d144 > Signed-off-by: Kyle Milka <[email protected]> > --- > user/vmm/virtio_lguest_helpers.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/user/vmm/virtio_lguest_helpers.c > b/user/vmm/virtio_lguest_helpers.c > index 4d8585f..acbde44 100644 > --- a/user/vmm/virtio_lguest_helpers.c > +++ b/user/vmm/virtio_lguest_helpers.c > @@ -191,10 +191,13 @@ uint32_t virtio_next_avail_vq_desc(struct virtio_vq > *vq, struct iovec iov[], > // we last incremented vq->last_avail, because it would have run out of > // places to put descriptors after incrementing exactly vring.num times > // (prior to our next vq->last_avail++) > - if ((vq->vring.avail->idx - vq->last_avail) > vq->vring.num) > + if (((vq->vring.avail->idx > vq->last_avail) && > + (vq->vring.avail->idx - vq->last_avail) > vq->vring.num) || > + ((vq->vring.avail->idx < vq->last_avail) && > + ((65536 - vq->last_avail + vq->vring.avail->idx) > vq->vring.num))) > VIRTIO_DRI_ERRX(vq->vqdev, > - "The driver advanced vq->vring.avail->idx from %u to > %u, which have a difference greater than the capacity of a queue. The idx is > supposed to increase by 1 for each descriptor chain added to the available > ring; the driver should have run out of room and thus been forced to wait for > us to catch up!", > - vq->last_avail, vq->vring.avail); > + "vq index increased from %u to %u, exceeded > capacity %u\n", > + vq->last_avail, vq->vring.avail->idx, > vq->vring.num);
I'm a little worried here. That line came from: http://lxr.free-electrons.com/source/tools/lguest/lguest.c#L787 So either we did something wrong overall, or there's a bug in lguest. Here's what they had: 787 /* Check it isn't doing very strange things with descriptor numbers. */ 788 if ((u16)(vq->vring.avail->idx - last_avail) > vq->vring.num) 789 bad_driver_vq(vq, "Guest moved used index from %u to %u", 790 last_avail, vq->vring.avail->idx); Also, maybe my unsigned math is off, but even if idx wraps around, isn't the difference between them still within the range? e.g. idx = 65535, avail 65534, u16 diff 1 add 3, then: idx = 2, avail 65534, u16 diff 4. The thing is, it looks like you need to cast it to a uint16_t, as Rusty does in lguest (u16). Here's a dumb example: #include <stdio.h> int main() { unsigned short x1 = 65535; unsigned short a1 = 65534; printf("diff before adding %d\n", x1 - a1); unsigned short x2 = 2; unsigned short a2 = 65534; printf("diff after adding, uncast %d\n", x2 - a2); printf("diff after adding, cast %d\n", (unsigned short)(x2 - a2)); } output: diff before adding 1 diff after adding, uncast -65532 diff after adding, cast 4 So I think you can just do: > @@ -191,10 +191,13 @@ uint32_t virtio_next_avail_vq_desc(struct virtio_vq > *vq, struct iovec iov[], > // we last incremented vq->last_avail, because it would have run out of > // places to put descriptors after incrementing exactly vring.num times > // (prior to our next vq->last_avail++) > - if ((vq->vring.avail->idx - vq->last_avail) > vq->vring.num) > + if ((uint16_t)(vq->vring.avail->idx - vq->last_avail) > vq->vring.num) etc. Basically, do what lguest did. =) FWIW, one of these days we should take a close look at lguest and see where we differ, and why. Perhaps there are other problems lurking about. Barret -- You received this message because you are subscribed to the Google Groups "Akaros" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. For more options, visit https://groups.google.com/d/optout.
