[Qemu-devel] [PATCH 0/3] virtio: detach VirtQueueElements freed by reset

[Qemu-devel] [PATCH 0/3] virtio: detach VirtQueueElements freed by reset

[Stefan Hajnoczi]

virtio-blk and virtio-serial need to free VirtQueueElements during device
reset.  Simply calling g_free(elem) is not enough because the scatter-gather
list should be unmapped and vq->inuse must be decremented.

These patches address the issue.  I am not including a patch that changes
vq->inuse = 0 to assert(!vq->inuse) in virtio_reset() yet because virtio-9p,
virtio-gpu, and virtio-net have code paths that do not decrement vq->inuse.

Stefan Hajnoczi (3):
  virtio: add virtio_detach_element()
  virtio-blk: add missing virtio_detach_element() call
  virtio-serial: add missing virtio_detach_element() call

 hw/block/virtio-blk.c       |  1 +
 hw/char/virtio-serial-bus.c | 14 ++++++++++++++
 hw/virtio/virtio.c          | 27 +++++++++++++++++++++++++--
 include/hw/virtio/virtio.h  |  2 ++
 4 files changed, 42 insertions(+), 2 deletions(-)

-- 
2.7.4

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

[Stefan Hajnoczi]

During device reset or similar situations a VirtQueueElement needs to be
freed without pushing it onto the used ring or rewinding the virtqueue.
Extract a new function to do this.

Later patches add virtio_detach_element() calls to existing device so
that scatter-gather lists are unmapped and vq->inuse goes back to zero
during device reset.  Currently some devices don't bother and simply
call g_free(elem) which is not a clean way to throw away a
VirtQueueElement.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/virtio/virtio.c         | 27 +++++++++++++++++++++++++--
 include/hw/virtio/virtio.h |  2 ++
 2 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index fcf3358..adcef45 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -264,12 +264,35 @@ static void virtqueue_unmap_sg(VirtQueue *vq, const VirtQueueElement *elem,
                                   0, elem->out_sg[i].iov_len);
 }
 
+/* virtqueue_detach_element:
+ * @vq: The #VirtQueue
+ * @elem: The #VirtQueueElement
+ * @len: number of bytes written
+ *
+ * Detach the element from the virtqueue.  This function is suitable for device
+ * reset or other situations where a #VirtQueueElement is simply freed and will
+ * not be pushed or discarded.
+ */
+void virtqueue_detach_element(VirtQueue *vq, const VirtQueueElement *elem,
+                              unsigned int len)
+{
+    vq->inuse--;
+    virtqueue_unmap_sg(vq, elem, len);
+}
+
+/* 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);
 }
 
 /* virtqueue_rewind:
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index f05559d..ad1e2d6 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -152,6 +152,8 @@ void *virtqueue_alloc_element(size_t sz, unsigned out_num, unsigned in_num);
 void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem,
                     unsigned int len);
 void virtqueue_flush(VirtQueue *vq, unsigned int count);
+void virtqueue_detach_element(VirtQueue *vq, const VirtQueueElement *elem,
+                              unsigned int len);
 void virtqueue_discard(VirtQueue *vq, const VirtQueueElement *elem,
                        unsigned int len);
 bool virtqueue_rewind(VirtQueue *vq, unsigned int num);
-- 
2.7.4

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

[Greg Kurz]

On Mon, 19 Sep 2016 14:28:03 +0100
Stefan Hajnoczi <stefanha@redhat.com> wrote:

> During device reset or similar situations a VirtQueueElement needs to be
> freed without pushing it onto the used ring or rewinding the virtqueue.
> Extract a new function to do this.
> 
> Later patches add virtio_detach_element() calls to existing device so
> that scatter-gather lists are unmapped and vq->inuse goes back to zero
> during device reset.  Currently some devices don't bother and simply
> call g_free(elem) which is not a clean way to throw away a
> VirtQueueElement.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---

FWIW

Acked-by: Greg Kurz <groug@kaod.org>

>  hw/virtio/virtio.c         | 27 +++++++++++++++++++++++++--
>  include/hw/virtio/virtio.h |  2 ++
>  2 files changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index fcf3358..adcef45 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -264,12 +264,35 @@ static void virtqueue_unmap_sg(VirtQueue *vq, const VirtQueueElement *elem,
>                                    0, elem->out_sg[i].iov_len);
>  }
>  
> +/* virtqueue_detach_element:
> + * @vq: The #VirtQueue
> + * @elem: The #VirtQueueElement
> + * @len: number of bytes written
> + *
> + * Detach the element from the virtqueue.  This function is suitable for device
> + * reset or other situations where a #VirtQueueElement is simply freed and will
> + * not be pushed or discarded.
> + */
> +void virtqueue_detach_element(VirtQueue *vq, const VirtQueueElement *elem,
> +                              unsigned int len)
> +{
> +    vq->inuse--;
> +    virtqueue_unmap_sg(vq, elem, len);
> +}
> +
> +/* 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);
>  }
>  
>  /* virtqueue_rewind:
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index f05559d..ad1e2d6 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -152,6 +152,8 @@ void *virtqueue_alloc_element(size_t sz, unsigned out_num, unsigned in_num);
>  void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem,
>                      unsigned int len);
>  void virtqueue_flush(VirtQueue *vq, unsigned int count);
> +void virtqueue_detach_element(VirtQueue *vq, const VirtQueueElement *elem,
> +                              unsigned int len);
>  void virtqueue_discard(VirtQueue *vq, const VirtQueueElement *elem,
>                         unsigned int len);
>  bool virtqueue_rewind(VirtQueue *vq, unsigned int num);

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

[Ladi Prosek]

On Mon, Sep 19, 2016 at 3:28 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> During device reset or similar situations a VirtQueueElement needs to be
> freed without pushing it onto the used ring or rewinding the virtqueue.
> Extract a new function to do this.
>
> Later patches add virtio_detach_element() calls to existing device so
> that scatter-gather lists are unmapped and vq->inuse goes back to zero
> during device reset.  Currently some devices don't bother and simply
> call g_free(elem) which is not a clean way to throw away a
> VirtQueueElement.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  hw/virtio/virtio.c         | 27 +++++++++++++++++++++++++--
>  include/hw/virtio/virtio.h |  2 ++
>  2 files changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index fcf3358..adcef45 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -264,12 +264,35 @@ static void virtqueue_unmap_sg(VirtQueue *vq, const VirtQueueElement *elem,
>                                    0, elem->out_sg[i].iov_len);
>  }
>
> +/* virtqueue_detach_element:
> + * @vq: The #VirtQueue
> + * @elem: The #VirtQueueElement
> + * @len: number of bytes written
> + *
> + * Detach the element from the virtqueue.  This function is suitable for device
> + * reset or other situations where a #VirtQueueElement is simply freed and will
> + * not be pushed or discarded.
> + */
> +void virtqueue_detach_element(VirtQueue *vq, const VirtQueueElement *elem,
> +                              unsigned int len)
> +{
> +    vq->inuse--;
> +    virtqueue_unmap_sg(vq, elem, len);
> +}
> +
> +/* 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.

>  }
>
>  /* virtqueue_rewind:
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index f05559d..ad1e2d6 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -152,6 +152,8 @@ void *virtqueue_alloc_element(size_t sz, unsigned out_num, unsigned in_num);
>  void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem,
>                      unsigned int len);
>  void virtqueue_flush(VirtQueue *vq, unsigned int count);
> +void virtqueue_detach_element(VirtQueue *vq, const VirtQueueElement *elem,
> +                              unsigned int len);
>  void virtqueue_discard(VirtQueue *vq, const VirtQueueElement *elem,
>                         unsigned int len);
>  bool virtqueue_rewind(VirtQueue *vq, unsigned int num);
> --
> 2.7.4
>

Reviewed-by: Ladi Prosek <lprosek@redhat.com>

Thanks!
Ladi

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

[Stefan Hajnoczi]

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

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.

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).  One way to
implement the assertion is to put VirtQueueElements on a linked list
(vq->inuse_elems) but that probably needs live migration support.

I agree that renaming to unpop makes the semantics clearer.

Would you like to submit a patch for either or both?

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

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

[Ladi Prosek]

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.

> One way to
> implement the assertion is to put VirtQueueElements on a linked list
> (vq->inuse_elems) but that probably needs live migration support.
>
> I agree that renaming to unpop makes the semantics clearer.
>
> Would you like to submit a patch for either or both?

Yes, I'll do both.

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

[Stefan Hajnoczi]

[-- 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 --]

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

[Ladi Prosek]

On Tue, Sep 27, 2016 at 5:03 PM, Stefan Hajnoczi <stefanha@redhat.com> 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 <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.

Got it, all clear now, thanks!

> Stefan

[Qemu-devel] [PATCH 2/3] virtio-blk: add missing virtio_detach_element() call

[Stefan Hajnoczi]

Make sure to unmap the scatter-gather list and decrement vq->inuse
before freeing requests in virtio_blk_reset().

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/block/virtio-blk.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 3a6112f..c7ca4d6 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -665,6 +665,7 @@ static void virtio_blk_reset(VirtIODevice *vdev)
     while (s->rq) {
         req = s->rq;
         s->rq = req->next;
+        virtqueue_detach_element(req->vq, &req->elem, 0);
         virtio_blk_free_request(req);
     }
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH 2/3] virtio-blk: add missing virtio_detach_element() call

[Ladi Prosek]

On Mon, Sep 19, 2016 at 3:28 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> Make sure to unmap the scatter-gather list and decrement vq->inuse
> before freeing requests in virtio_blk_reset().
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  hw/block/virtio-blk.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 3a6112f..c7ca4d6 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -665,6 +665,7 @@ static void virtio_blk_reset(VirtIODevice *vdev)
>      while (s->rq) {
>          req = s->rq;
>          s->rq = req->next;
> +        virtqueue_detach_element(req->vq, &req->elem, 0);
>          virtio_blk_free_request(req);

Random observation. virtio_blk_free_request should be static and
removed from the header file. Or maybe removed altogether because
g_free takes NULL pointers just fine.

>      }
>
> --
> 2.7.4
>

Reviewed-by: Ladi Prosek <lprosek@redhat.com>

Thanks!

Re: [Qemu-devel] [PATCH 2/3] virtio-blk: add missing virtio_detach_element() call

[Greg Kurz]

On Tue, 27 Sep 2016 09:49:17 +0200
Ladi Prosek <lprosek@redhat.com> wrote:

> On Mon, Sep 19, 2016 at 3:28 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > Make sure to unmap the scatter-gather list and decrement vq->inuse
> > before freeing requests in virtio_blk_reset().
> >
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  hw/block/virtio-blk.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > index 3a6112f..c7ca4d6 100644
> > --- a/hw/block/virtio-blk.c
> > +++ b/hw/block/virtio-blk.c
> > @@ -665,6 +665,7 @@ static void virtio_blk_reset(VirtIODevice *vdev)
> >      while (s->rq) {
> >          req = s->rq;
> >          s->rq = req->next;
> > +        virtqueue_detach_element(req->vq, &req->elem, 0);
> >          virtio_blk_free_request(req);  
> 
> Random observation. virtio_blk_free_request should be static and
> removed from the header file. 

I've sent a followup patch for this:

<147487884587.6679.6170297932839464278.stgit@bahia>

> Or maybe removed altogether because g_free takes NULL pointers just fine.
> 

virtio_blk_free_request() does not seem useful indeed... :)

Cheers.

--
Greg

> >      }
> >
> > --
> > 2.7.4
> >  
> 
> Reviewed-by: Ladi Prosek <lprosek@redhat.com>
> 
> Thanks!
> 

[Qemu-devel] [PATCH 3/3] virtio-serial: add missing virtio_detach_element() call

[Stefan Hajnoczi]

Ports enter a "throttled" state when writing to the chardev would block.
The current output VirtQueueElement is kept around until the chardev
becomes writable again.

There are several places in the virtio-serial lifecycle where the
VirtQueueElement should be thrown away.  For example, if the virtio
device is reset then virtqueue elements are no longer valid.

This patch adds the discard_throttle_data() function to unmap the
scatter-gather list and decrement vq->inuse.  This ensures that the
VirtQueueElement is freed properly.

Cc: amit.shah@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/char/virtio-serial-bus.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index db57a38..35aa3c7 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -132,6 +132,15 @@ static void discard_vq_data(VirtQueue *vq, VirtIODevice *vdev)
     virtio_notify(vdev, vq);
 }
 
+static void discard_throttle_data(VirtIOSerialPort *port)
+{
+    if (port->elem) {
+        virtqueue_detach_element(port->ovq, port->elem, 0);
+        g_free(port->elem);
+        port->elem = NULL;
+    }
+}
+
 static void do_flush_queued_data(VirtIOSerialPort *port, VirtQueue *vq,
                                  VirtIODevice *vdev)
 {
@@ -254,6 +263,7 @@ int virtio_serial_close(VirtIOSerialPort *port)
      * consume, reset the throttling flag and discard the data.
      */
     port->throttled = false;
+    discard_throttle_data(port);
     discard_vq_data(port->ovq, VIRTIO_DEVICE(port->vser));
 
     send_control_event(port->vser, port->id, VIRTIO_CONSOLE_PORT_OPEN, 0);
@@ -554,6 +564,9 @@ static void guest_reset(VirtIOSerial *vser)
 
     QTAILQ_FOREACH(port, &vser->ports, next) {
         vsc = VIRTIO_SERIAL_PORT_GET_CLASS(port);
+
+        discard_throttle_data(port);
+
         if (port->guest_connected) {
             port->guest_connected = false;
             if (vsc->set_guest_connected) {
@@ -864,6 +877,7 @@ static void remove_port(VirtIOSerial *vser, uint32_t port_id)
     assert(port);
 
     /* Flush out any unconsumed buffers first */
+    discard_throttle_data(port);
     discard_vq_data(port->ovq, VIRTIO_DEVICE(port->vser));
 
     send_control_event(vser, port->id, VIRTIO_CONSOLE_PORT_REMOVE, 1);
-- 
2.7.4

Re: [Qemu-devel] [PATCH 3/3] virtio-serial: add missing virtio_detach_element() call

[Ladi Prosek]

On Mon, Sep 19, 2016 at 3:28 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> Ports enter a "throttled" state when writing to the chardev would block.
> The current output VirtQueueElement is kept around until the chardev
> becomes writable again.
>
> There are several places in the virtio-serial lifecycle where the
> VirtQueueElement should be thrown away.  For example, if the virtio
> device is reset then virtqueue elements are no longer valid.
>
> This patch adds the discard_throttle_data() function to unmap the
> scatter-gather list and decrement vq->inuse.  This ensures that the
> VirtQueueElement is freed properly.
>
> Cc: amit.shah@redhat.com
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

I have tested this patch with a simple serial stress test to exercise
the code paths.

Tested-by: Ladi Prosek <lprosek@redhat.com>
Reviewed-by: Ladi Prosek <lprosek@redhat.com>

Re: [Qemu-devel] [PATCH 0/3] virtio: detach VirtQueueElements freed by reset

[Stefan Hajnoczi]

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

On Mon, Sep 19, 2016 at 02:28:02PM +0100, Stefan Hajnoczi wrote:
> virtio-blk and virtio-serial need to free VirtQueueElements during device
> reset.  Simply calling g_free(elem) is not enough because the scatter-gather
> list should be unmapped and vq->inuse must be decremented.
> 
> These patches address the issue.  I am not including a patch that changes
> vq->inuse = 0 to assert(!vq->inuse) in virtio_reset() yet because virtio-9p,
> virtio-gpu, and virtio-net have code paths that do not decrement vq->inuse.
> 
> Stefan Hajnoczi (3):
>   virtio: add virtio_detach_element()
>   virtio-blk: add missing virtio_detach_element() call
>   virtio-serial: add missing virtio_detach_element() call
> 
>  hw/block/virtio-blk.c       |  1 +
>  hw/char/virtio-serial-bus.c | 14 ++++++++++++++
>  hw/virtio/virtio.c          | 27 +++++++++++++++++++++++++--
>  include/hw/virtio/virtio.h  |  2 ++
>  4 files changed, 42 insertions(+), 2 deletions(-)

Ping?

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

Re: [Qemu-devel] [PATCH 0/3] virtio: detach VirtQueueElements freed by reset

[Greg Kurz]

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

On Tue, 27 Sep 2016 11:08:53 +0100
Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Mon, Sep 19, 2016 at 02:28:02PM +0100, Stefan Hajnoczi wrote:
> > virtio-blk and virtio-serial need to free VirtQueueElements during device
> > reset.  Simply calling g_free(elem) is not enough because the scatter-gather
> > list should be unmapped and vq->inuse must be decremented.
> > 
> > These patches address the issue.  I am not including a patch that changes
> > vq->inuse = 0 to assert(!vq->inuse) in virtio_reset() yet because virtio-9p,
> > virtio-gpu, and virtio-net have code paths that do not decrement vq->inuse.
> > 
> > Stefan Hajnoczi (3):
> >   virtio: add virtio_detach_element()
> >   virtio-blk: add missing virtio_detach_element() call
> >   virtio-serial: add missing virtio_detach_element() call
> > 
> >  hw/block/virtio-blk.c       |  1 +
> >  hw/char/virtio-serial-bus.c | 14 ++++++++++++++
> >  hw/virtio/virtio.c          | 27 +++++++++++++++++++++++++--
> >  include/hw/virtio/virtio.h  |  2 ++
> >  4 files changed, 42 insertions(+), 2 deletions(-)  
> 
> Ping?

+1 because patch 1/3 of this series is required by:

Subject: [PATCH v4 0/9] virtio: avoid inappropriate QEMU termination in device code
Date: Fri, 30 Sep 2016 17:12:32 +0200
Message-Id: <147524835229.953.16853431367911587573.stgit@bahia>

Cheers.

--
Greg

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]