qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Ladi Prosek
Subject: Re: [Qemu-devel] [PATCH 1/3] virtio: add virtio_detach_element()
Date: Tue, 27 Sep 2016 17:24:14 +0200

On Tue, Sep 27, 2016 at 5:03 PM, Stefan Hajnoczi <address@hidden> 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 <address@hidden> 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 <address@hidden> 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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]