From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Michael S. Tsirkin" Subject: Re: [virtio-dev] packed ring layout proposal v2 Date: Fri, 10 Feb 2017 17:20:17 +0200 Message-ID: <20170210170548-mutt-send-email-mst@kernel.org> References: <20160915223915.qjlnlvf2w7u37bu3@redhat.com> <240c623b-2d8f-28d9-d349-d01e2c24b93a@redhat.com> <20170208214435-mutt-send-email-mst@kernel.org> <20170209202203-mutt-send-email-mst@kernel.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: virtualization-bounces@lists.linux-foundation.org Errors-To: virtualization-bounces@lists.linux-foundation.org To: Paolo Bonzini Cc: virtio-dev@lists.oasis-open.org, virtualization@lists.linux-foundation.org List-Id: virtualization@lists.linuxfoundation.org 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