From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36495) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bouF2-0007TG-Ru for qemu-devel@nongnu.org; Tue, 27 Sep 2016 11:24:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bouEx-0003Ls-IB for qemu-devel@nongnu.org; Tue, 27 Sep 2016 11:24:19 -0400 Received: from mail-vk0-f50.google.com ([209.85.213.50]:34295) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bouEx-0003Lm-Dl for qemu-devel@nongnu.org; Tue, 27 Sep 2016 11:24:15 -0400 Received: by mail-vk0-f50.google.com with SMTP id v205so16564002vke.1 for ; Tue, 27 Sep 2016 08:24:15 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <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> <20160927150355.GA2835@stefanha-x1.localdomain> From: Ladi Prosek Date: Tue, 27 Sep 2016 17:24:14 +0200 Message-ID: Content-Type: text/plain; charset=UTF-8 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: Stefan Hajnoczi Cc: Stefan Hajnoczi , qemu-devel , "Michael S. Tsirkin" On Tue, Sep 27, 2016 at 5:03 PM, Stefan Hajnoczi wrote: > On Tue, Sep 27, 2016 at 02:12:09PM +0200, Ladi Prosek wrote: >> On Tue, Sep 27, 2016 at 12:08 PM, Stefan Hajnoczi wrote: >> > 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. 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. >> >> Oh, right, I didn't mean they would be identical but something like this: >> >> g_assert(elem->index == virtqueue_get_head(vq, vq->last_avail_idx)); >> >> > 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). >> >> 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. Got it, all clear now, thanks! > Stefan