On Tue, Sep 27, 2016 at 02:12:09PM +0200, Ladi Prosek wrote:
> On Tue, Sep 27, 2016 at 12:08 PM, Stefan Hajnoczi <stefa...@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 <stefa...@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
signature.asc
Description: PGP signature
