Re: [virtio-dev] packed ring layout proposal v2

["Michael S. Tsirkin" <mst@redhat.com>]

On Fri, Feb 10, 2017 at 12:32:49PM +0100, Paolo Bonzini wrote:
> 
> 
> On 09/02/2017 19:24, Michael S. Tsirkin wrote:
> >> I don't know.  Power of 2 ring size is pretty standard, I'd rather avoid
> >> the complication and the gratuitous difference with 1.0.
> >
> > I thought originally there's a reason 1.0 rings had to be powers of two
> > but now I don't see why. OK, we can make it a feature flag later if we
> > want to.
> 
> The reason is that it allows indices to be free running.

Well what I meant is that with qsize not a power of 2 you can still do
this but have to do everything mod N*qsize as opposed to mod 2^16. So
you need a branch there - easiest to do if you do signed math.

int nheads = avail - last_avail;
/*Check and handle index wrap-around */
if (unlikely(nheads < 0)) {
	nheads += N_qsize;
}

if (nheads < 0 || nheads > vdev->vq[i].vring.num) {
	error_report(...);
	return -1;
}

This can only catch bugs if N > 1

>  This is an 
> example of QEMU code that requires that:
> 
>             nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx;
>             /* Check it isn't doing strange things with descriptor numbers. */
>             if (nheads > vdev->vq[i].vring.num) {
>                 error_report("VQ %d size 0x%x Guest index 0x%x "
>                              "inconsistent with Host index 0x%x: delta 0x%x",
>                              i, vdev->vq[i].vring.num,
>                              vring_avail_idx(&vdev->vq[i]),
>                              vdev->vq[i].last_avail_idx, nheads);
>                 return -1;
>             }
> 
> Paolo

Same thing here, this never triggers if vring.num == 2^16

-- 
MST