Re: [Qemu-devel] commit virtio: recalculate vq->inuse after migration might cause last_avail_idx vs. used_idx failure

[Stefan Hajnoczi]

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