From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57835) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1botvR-0003rz-Q0 for qemu-devel@nongnu.org; Tue, 27 Sep 2016 11:04:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1botvM-0001hj-P9 for qemu-devel@nongnu.org; Tue, 27 Sep 2016 11:04:04 -0400 Received: from mx1.redhat.com ([209.132.183.28]:48060) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1botvM-0001gZ-G8 for qemu-devel@nongnu.org; Tue, 27 Sep 2016 11:04:00 -0400 Date: Tue, 27 Sep 2016 16:03:55 +0100 From: Stefan Hajnoczi Message-ID: <20160927150355.GA2835@stefanha-x1.localdomain> References: <1474291685-24226-1-git-send-email-stefanha@redhat.com> <1474291685-24226-2-git-send-email-stefanha@redhat.com> <20160927100832.GC563@stefanha-x1.localdomain> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="7JfCtLOvnd9MIVvH" Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH 1/3] virtio: add virtio_detach_element() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Ladi Prosek Cc: Stefan Hajnoczi , qemu-devel , "Michael S. Tsirkin" --7JfCtLOvnd9MIVvH Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Sep 27, 2016 at 02:12:09PM +0200, Ladi Prosek wrote: > On Tue, Sep 27, 2016 at 12:08 PM, Stefan Hajnoczi wr= ote: > > On Tue, Sep 27, 2016 at 09:32:40AM +0200, Ladi Prosek wrote: > >> On Mon, Sep 19, 2016 at 3:28 PM, Stefan Hajnoczi = wrote: > >> > During device res> > +/* virtqueue_discard: > >> > + * @vq: The #VirtQueue > >> > + * @elem: The #VirtQueueElement > >> > + * @len: number of bytes written > >> > + * > >> > + * Pretend the most recent element wasn't popped from the virtqueue= =2E The next > >> > + * call to virtqueue_pop() will refetch the element. > >> > + */ > >> > void virtqueue_discard(VirtQueue *vq, const VirtQueueElement *elem, > >> > unsigned int len) > >> > { > >> > vq->last_avail_idx--; > >> > - vq->inuse--; > >> > - virtqueue_unmap_sg(vq, elem, len); > >> > + virtqueue_detach_element(vq, elem, len); > >> > >> Random comment, not directly related to this change. Would it be worth > >> adding an assert to this function that elem->index and > >> vq->last_avail_idx match? In other words, enforce the "most recent" > >> qualifier mentioned in the comment. As more virtqueue_* functions are > >> added and the complexity goes up, it is easy to get confused. Also, I > >> think that naming this function virtqueue_unpop instead of > >> virtqueue_discard would help. > > > > elem->index is a descriptor ring index. vq->last_avail_idx is an index > > into the available ring. They are different but your suggestion makes > > sense in general. >=20 > Oh, right, I didn't mean they would be identical but something like this: >=20 > g_assert(elem->index =3D=3D virtqueue_get_head(vq, vq->last_avail_idx)); >=20 > > We shouldn't read from vring memory again for an assertion so > > deferencing the available ring isn't possible (because we cannot rely on > > vring memory contents after processing the request). >=20 > Not sure I follow, shouldn't available ring memory at that index still > be the same? Basically I'd like to assert that the next virtqueue_pop > would return the same element. Assertions cannot be guest-triggerable. The guest can make the assertion fail by writing a new value to the available ring. That might not sound like an issue but consider a scenario where the virtio PCI device is passed through to a nested guest. Now the nested guest can kill the parent hypervisor and all sibling VMs. Stefan --7JfCtLOvnd9MIVvH Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEcBAEBAgAGBQJX6opbAAoJEJykq7OBq3PI56gH/iIgfQ4XSouAM34Ch+St91l8 bWNEqKoF1bC4VTLH0OMhgQLaASOigqe1nipoLclWgNN80Olx1IAvcKj/0AcMcbI/ hx2LQ/gNR/yw+V3C3rkEqqzDUAzghtGAwvP7r18AeoO2m06sU+w/T0/tUm5VeQWW ZtdnRMaFmXsuEXEvfQTI5FTVkQ8iRVm57TEZGe6LheDg89k5Gmr3MfuDqtpCphRP 3rEMCz1nHCMMHkn3X6SFShXT+IovGpzdpc5wohwTSoubFnepQoKYr6nXpW6XyfpW UTYzYFAUma6G+jQJ7+6nMJelnzRt5WRNpFXd/K0udqzs+23jb3o/Nb2+sVZU8TM= =NgV+ -----END PGP SIGNATURE----- --7JfCtLOvnd9MIVvH--