On Wed, Dec 14, 2016 at 08:12:17PM +0100, Halil Pasic wrote: > We have a migration problem, which is in my opinion caused by a > deficiency in how vq->inuse is calculated after the migration (commit > bccdef6b "virtio: recalculate vq->inuse after migration" to > blame). > > > We got a bugreport with this log for a live migration target. > > 2016-12-13T18:59:03.647309Z qemu-system-s390x: VQ 1 size 0x100 < > last_avail_idx 0x2f76 - used_idx 0x762f
Thanks for spotting the signedness bug described below. Regardless of the bug last_avail_idx 0x2f76 - used_idx 0x762f is still an invalid state and should be treated as an error. The virtqueue only has 256 elements so there's no way these descriptor indices can be valid. I wanted to point that out since there must be another problem remaining somewhere. > 2016-12-13T18:59:03.647385Z qemu-system-s390x: error while loading state for > instance 0x0 of device '/fe.0.0001/virtio-net' > 2016-12-13T18:59:03.647540Z qemu-system-s390x: load of migration failed: > Operation not permitted > 2016-12-13 18:59:03.796+0000: shutting down, reason=failed > > They use QEMU version 2.7 but looking at the current git master > I think this did not get fixed in the meanwhile. > > So here goes the argument. The recalculation is done like this: > > + vdev->vq[i].inuse = vdev->vq[i].last_avail_idx - > + vdev->vq[i].used_idx; > > This does not seem correct when last_avail_idx has already > wrapped around but used_idx not yet. We see from the log that > last_avail_idx (0x2f76) less that used_idx (0x762f) thus > inuse (of type int) ends up being negative. Good catch. This works: vdev->vq[i].inuse = (uint16_t)(vdev->vq[i].last_avail_idx - vdev->vq[i].used_idx); > Do we want to try to fix this for 2.8? I already have a small patch prepared. Please send the fix for 2.8.1 (-stable). 2.8.0-rc4 is currently being tagged and build tested, it's too late to change it. Stefan
signature.asc
Description: PGP signature