Re: [Qemu-devel] [PATCH 1/3] virtio: add virtio_detach_element()

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2481 bytes --]

On Tue, Sep 27, 2016 at 02:12:09PM +0200, Ladi Prosek wrote:
> On Tue, Sep 27, 2016 at 12:08 PM, Stefan Hajnoczi <stefanha@gmail.com> 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 <stefanha@redhat.com> 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.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]