Re: [Qemu-devel] [PATCH] virtio: fix vring->inuse recalc after migr

[Stefan Hajnoczi]

On Thu, Dec 15, 2016 at 04:43:30PM +0100, Halil Pasic wrote:
> Correct recalculation of vring->inuse after migration for
> the corner case where the avail_idx has already wrapped
> but used_idx not yet.
> 
> Signed-off-by: Halil Pasic <address@hidden>
> Fixes: bccdef6b ("virtio: recalculate vq->inuse after migration")
> CC: address@hidden
> ---
> 
> I think we could also change the type of inuse to uint16_t.
> Would this be considered a good idea?

VRing.num is unsigned int.  I would use the same type as num since this
is a size, not an index.

> ---
>  hw/virtio/virtio.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 1af2de2..089c6f6 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1855,9 +1855,12 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int 
> version_id)
>              /*
>               * Some devices migrate VirtQueueElements that have been popped
>               * from the avail ring but not yet returned to the used ring.
> +             * Cast to uint16_t is OK because max ring size is 0x8000. Thus
> +             * no the size of largest array indexable by an integral type
> +             * can not be represented by the same type problem.

I think it would be safe up to max ring size UINT16_MAX - 1 (we need
that extra value to distinguish between empty and full avail rings)?

The last sentence is hard to understand due to the double negative.  I
think you are saying "Since max ring size < UINT16_MAX it's safe to use
uint16_t to represent the size of the ring".

>               */
> -            vdev->vq[i].inuse = vdev->vq[i].last_avail_idx -
> -                                vdev->vq[i].used_idx;
> +            vdev->vq[i].inuse = (uint16_t)(vdev->vq[i].last_avail_idx -
> +                                vdev->vq[i].used_idx);
>              if (vdev->vq[i].inuse > vdev->vq[i].vring.num) {
>                  error_report("VQ %d size 0x%x < last_avail_idx 0x%x - "
>                               "used_idx 0x%x",
> -- 
> 2.8.4
> 
>

signature.asc
Description: PGP signature