Oh yes, if lguest does a cast we should just do that too. When I initially looked at it I was expecting that the unsigned math would take care of the wrap around, but for some reason or another the result wasn't in the range of a uint16. I'll change it to use the cast and verify that it works.
On Fri, Aug 5, 2016 at 7:35 PM, barret rhoden <[email protected]> wrote: > 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. > -- 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.
