[PATCH 0/3] hw/virtio: Minor housekeeping patches

[PATCH 0/3] hw/virtio: Minor housekeeping patches

[Philippe Mathieu-Daudé]

Hi,

This series contains few patches I gathered while tooking notes
trying to understand issues #300-#302. Nothing very exciting :)

Philippe Mathieu-Daudé (3):
  hw/virtio: Document virtio_queue_packed_empty_rcu is called within RCU
  hw/virtio: Remove NULL check in virtio_free_region_cache()
  hw/virtio: Have virtqueue_get_avail_bytes() pass caches arg to callees

 hw/virtio/virtio.c | 36 +++++++++++++++---------------------
 1 file changed, 15 insertions(+), 21 deletions(-)

-- 
2.31.1

[PATCH 1/3] hw/virtio: Document virtio_queue_packed_empty_rcu is called within RCU

[Philippe Mathieu-Daudé]

While virtio_queue_packed_empty_rcu() uses the '_rcu' suffix,
it is not obvious it is called within rcu_read_lock(). All other
functions from this file called with the RCU locked have a comment
describing it. Document this one similarly for consistency.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/virtio/virtio.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 874377f37a7..a5214bca612 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -634,6 +634,7 @@ static int virtio_queue_split_empty(VirtQueue *vq)
     return empty;
 }
 
+/* Called within rcu_read_lock().  */
 static int virtio_queue_packed_empty_rcu(VirtQueue *vq)
 {
     struct VRingPackedDesc desc;
-- 
2.31.1

[PATCH 2/3] hw/virtio: Remove NULL check in virtio_free_region_cache()

[Philippe Mathieu-Daudé]

virtio_free_region_cache() is called within call_rcu(),
always with a non-NULL argument. Ensure new code keep it
that way by replacing the NULL check by an assertion.
Add a comment this function is called within call_rcu().

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/virtio/virtio.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index a5214bca612..3a1f6c520cb 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -133,12 +133,10 @@ struct VirtQueue
     QLIST_ENTRY(VirtQueue) node;
 };
 
+/* Called within call_rcu().  */
 static void virtio_free_region_cache(VRingMemoryRegionCaches *caches)
 {
-    if (!caches) {
-        return;
-    }
-
+    assert(caches != NULL);
     address_space_cache_destroy(&caches->desc);
     address_space_cache_destroy(&caches->avail);
     address_space_cache_destroy(&caches->used);
-- 
2.31.1

[RFC PATCH 3/3] hw/virtio: Have virtqueue_get_avail_bytes() pass caches arg to callees

[Philippe Mathieu-Daudé]

Both virtqueue_packed_get_avail_bytes() and
virtqueue_split_get_avail_bytes() access the region cache, but
their caller also does. Simplify by having virtqueue_get_avail_bytes
calling both with RCU lock held, and passing the caches as argument.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
RFC because I'm not sure this is safe enough
---
 hw/virtio/virtio.c | 29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 3a1f6c520cb..8237693a567 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -984,28 +984,23 @@ static int virtqueue_split_read_next_desc(VirtIODevice *vdev, VRingDesc *desc,
     return VIRTQUEUE_READ_DESC_MORE;
 }
 
+/* Called within rcu_read_lock().  */
 static void virtqueue_split_get_avail_bytes(VirtQueue *vq,
                             unsigned int *in_bytes, unsigned int *out_bytes,
-                            unsigned max_in_bytes, unsigned max_out_bytes)
+                            unsigned max_in_bytes, unsigned max_out_bytes,
+                            VRingMemoryRegionCaches *caches)
 {
     VirtIODevice *vdev = vq->vdev;
     unsigned int max, idx;
     unsigned int total_bufs, in_total, out_total;
-    VRingMemoryRegionCaches *caches;
     MemoryRegionCache indirect_desc_cache = MEMORY_REGION_CACHE_INVALID;
     int64_t len = 0;
     int rc;
 
-    RCU_READ_LOCK_GUARD();
-
     idx = vq->last_avail_idx;
     total_bufs = in_total = out_total = 0;
 
     max = vq->vring.num;
-    caches = vring_get_region_caches(vq);
-    if (!caches) {
-        goto err;
-    }
 
     while ((rc = virtqueue_num_heads(vq, idx)) > 0) {
         MemoryRegionCache *desc_cache = &caches->desc;
@@ -1124,32 +1119,28 @@ static int virtqueue_packed_read_next_desc(VirtQueue *vq,
     return VIRTQUEUE_READ_DESC_MORE;
 }
 
+/* Called within rcu_read_lock().  */
 static void virtqueue_packed_get_avail_bytes(VirtQueue *vq,
                                              unsigned int *in_bytes,
                                              unsigned int *out_bytes,
                                              unsigned max_in_bytes,
-                                             unsigned max_out_bytes)
+                                             unsigned max_out_bytes,
+                                             VRingMemoryRegionCaches *caches)
 {
     VirtIODevice *vdev = vq->vdev;
     unsigned int max, idx;
     unsigned int total_bufs, in_total, out_total;
     MemoryRegionCache *desc_cache;
-    VRingMemoryRegionCaches *caches;
     MemoryRegionCache indirect_desc_cache = MEMORY_REGION_CACHE_INVALID;
     int64_t len = 0;
     VRingPackedDesc desc;
     bool wrap_counter;
 
-    RCU_READ_LOCK_GUARD();
     idx = vq->last_avail_idx;
     wrap_counter = vq->last_avail_wrap_counter;
     total_bufs = in_total = out_total = 0;
 
     max = vq->vring.num;
-    caches = vring_get_region_caches(vq);
-    if (!caches) {
-        goto err;
-    }
 
     for (;;) {
         unsigned int num_bufs = total_bufs;
@@ -1250,6 +1241,8 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
     uint16_t desc_size;
     VRingMemoryRegionCaches *caches;
 
+    RCU_READ_LOCK_GUARD();
+
     if (unlikely(!vq->vring.desc)) {
         goto err;
     }
@@ -1268,10 +1261,12 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
 
     if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) {
         virtqueue_packed_get_avail_bytes(vq, in_bytes, out_bytes,
-                                         max_in_bytes, max_out_bytes);
+                                         max_in_bytes, max_out_bytes,
+                                         caches);
     } else {
         virtqueue_split_get_avail_bytes(vq, in_bytes, out_bytes,
-                                        max_in_bytes, max_out_bytes);
+                                        max_in_bytes, max_out_bytes,
+                                        caches);
     }
 
     return;
-- 
2.31.1

Re: [RFC PATCH 3/3] hw/virtio: Have virtqueue_get_avail_bytes() pass caches arg to callees

[Stefano Garzarella]

On Thu, Aug 26, 2021 at 07:26:58PM +0200, Philippe Mathieu-Daudé wrote:
>Both virtqueue_packed_get_avail_bytes() and
>virtqueue_split_get_avail_bytes() access the region cache, but
>their caller also does. Simplify by having virtqueue_get_avail_bytes
>calling both with RCU lock held, and passing the caches as argument.
>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>---
>RFC because I'm not sure this is safe enough

It seems safe to me.

While reviewing I saw that vring_get_region_caches() has
/* Called within rcu_read_lock().  */ comment, but it seems to me that 
we call that function in places where we haven't acquired it, which 
shouldn't be a problem, but should we remove that comment?

Thanks,
Stefano

>---
> hw/virtio/virtio.c | 29 ++++++++++++-----------------
> 1 file changed, 12 insertions(+), 17 deletions(-)
>
>diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>index 3a1f6c520cb..8237693a567 100644
>--- a/hw/virtio/virtio.c
>+++ b/hw/virtio/virtio.c
>@@ -984,28 +984,23 @@ static int virtqueue_split_read_next_desc(VirtIODevice *vdev, VRingDesc *desc,
>     return VIRTQUEUE_READ_DESC_MORE;
> }
>
>+/* Called within rcu_read_lock().  */
> static void virtqueue_split_get_avail_bytes(VirtQueue *vq,
>                             unsigned int *in_bytes, unsigned int *out_bytes,
>-                            unsigned max_in_bytes, unsigned max_out_bytes)
>+                            unsigned max_in_bytes, unsigned max_out_bytes,
>+                            VRingMemoryRegionCaches *caches)
> {
>     VirtIODevice *vdev = vq->vdev;
>     unsigned int max, idx;
>     unsigned int total_bufs, in_total, out_total;
>-    VRingMemoryRegionCaches *caches;
>     MemoryRegionCache indirect_desc_cache = MEMORY_REGION_CACHE_INVALID;
>     int64_t len = 0;
>     int rc;
>
>-    RCU_READ_LOCK_GUARD();
>-
>     idx = vq->last_avail_idx;
>     total_bufs = in_total = out_total = 0;
>
>     max = vq->vring.num;
>-    caches = vring_get_region_caches(vq);
>-    if (!caches) {
>-        goto err;
>-    }
>
>     while ((rc = virtqueue_num_heads(vq, idx)) > 0) {
>         MemoryRegionCache *desc_cache = &caches->desc;
>@@ -1124,32 +1119,28 @@ static int virtqueue_packed_read_next_desc(VirtQueue *vq,
>     return VIRTQUEUE_READ_DESC_MORE;
> }
>
>+/* Called within rcu_read_lock().  */
> static void virtqueue_packed_get_avail_bytes(VirtQueue *vq,
>                                              unsigned int *in_bytes,
>                                              unsigned int *out_bytes,
>                                              unsigned max_in_bytes,
>-                                             unsigned max_out_bytes)
>+                                             unsigned max_out_bytes,
>+                                             VRingMemoryRegionCaches *caches)
> {
>     VirtIODevice *vdev = vq->vdev;
>     unsigned int max, idx;
>     unsigned int total_bufs, in_total, out_total;
>     MemoryRegionCache *desc_cache;
>-    VRingMemoryRegionCaches *caches;
>     MemoryRegionCache indirect_desc_cache = MEMORY_REGION_CACHE_INVALID;
>     int64_t len = 0;
>     VRingPackedDesc desc;
>     bool wrap_counter;
>
>-    RCU_READ_LOCK_GUARD();
>     idx = vq->last_avail_idx;
>     wrap_counter = vq->last_avail_wrap_counter;
>     total_bufs = in_total = out_total = 0;
>
>     max = vq->vring.num;
>-    caches = vring_get_region_caches(vq);
>-    if (!caches) {
>-        goto err;
>-    }
>
>     for (;;) {
>         unsigned int num_bufs = total_bufs;
>@@ -1250,6 +1241,8 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
>     uint16_t desc_size;
>     VRingMemoryRegionCaches *caches;
>
>+    RCU_READ_LOCK_GUARD();
>+
>     if (unlikely(!vq->vring.desc)) {
>         goto err;
>     }
>@@ -1268,10 +1261,12 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
>
>     if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) {
>         virtqueue_packed_get_avail_bytes(vq, in_bytes, out_bytes,
>-                                         max_in_bytes, max_out_bytes);
>+                                         max_in_bytes, max_out_bytes,
>+                                         caches);
>     } else {
>         virtqueue_split_get_avail_bytes(vq, in_bytes, out_bytes,
>-                                        max_in_bytes, max_out_bytes);
>+                                        max_in_bytes, max_out_bytes,
>+                                        caches);
>     }
>
>     return;
>-- 
>2.31.1
>
>

Re: [PATCH 1/3] hw/virtio: Document virtio_queue_packed_empty_rcu is called within RCU

[Stefano Garzarella]

On Thu, Aug 26, 2021 at 07:26:56PM +0200, Philippe Mathieu-Daudé wrote:
>While virtio_queue_packed_empty_rcu() uses the '_rcu' suffix,
>it is not obvious it is called within rcu_read_lock(). All other
>functions from this file called with the RCU locked have a comment
>describing it. Document this one similarly for consistency.
>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>---
> hw/virtio/virtio.c | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>index 874377f37a7..a5214bca612 100644
>--- a/hw/virtio/virtio.c
>+++ b/hw/virtio/virtio.c
>@@ -634,6 +634,7 @@ static int virtio_queue_split_empty(VirtQueue *vq)
>     return empty;
> }
>
>+/* Called within rcu_read_lock().  */
> static int virtio_queue_packed_empty_rcu(VirtQueue *vq)
> {
>     struct VRingPackedDesc desc;
>-- 
>2.31.1
>
>

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

Re: [PATCH 2/3] hw/virtio: Remove NULL check in virtio_free_region_cache()

[Stefano Garzarella]

On Thu, Aug 26, 2021 at 07:26:57PM +0200, Philippe Mathieu-Daudé wrote:
>virtio_free_region_cache() is called within call_rcu(),
>always with a non-NULL argument. Ensure new code keep it
>that way by replacing the NULL check by an assertion.
>Add a comment this function is called within call_rcu().
>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>---
> hw/virtio/virtio.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
>diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>index a5214bca612..3a1f6c520cb 100644
>--- a/hw/virtio/virtio.c
>+++ b/hw/virtio/virtio.c
>@@ -133,12 +133,10 @@ struct VirtQueue
>     QLIST_ENTRY(VirtQueue) node;
> };
>
>+/* Called within call_rcu().  */
> static void virtio_free_region_cache(VRingMemoryRegionCaches *caches)
> {
>-    if (!caches) {
>-        return;
>-    }
>-
>+    assert(caches != NULL);
>     address_space_cache_destroy(&caches->desc);
>     address_space_cache_destroy(&caches->avail);
>     address_space_cache_destroy(&caches->used);
>-- 
>2.31.1
>
>

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

Re: [PATCH 2/3] hw/virtio: Remove NULL check in virtio_free_region_cache()

[Stefan Hajnoczi]

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

On Thu, Aug 26, 2021 at 07:26:57PM +0200, Philippe Mathieu-Daudé wrote:
> virtio_free_region_cache() is called within call_rcu(),
> always with a non-NULL argument. Ensure new code keep it
> that way by replacing the NULL check by an assertion.
> Add a comment this function is called within call_rcu().
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/virtio/virtio.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index a5214bca612..3a1f6c520cb 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -133,12 +133,10 @@ struct VirtQueue
>      QLIST_ENTRY(VirtQueue) node;
>  };
>  
> +/* Called within call_rcu().  */
>  static void virtio_free_region_cache(VRingMemoryRegionCaches *caches)
>  {
> -    if (!caches) {
> -        return;
> -    }
> -
> +    assert(caches != NULL);
>      address_space_cache_destroy(&caches->desc);
>      address_space_cache_destroy(&caches->avail);
>      address_space_cache_destroy(&caches->used);

Looks like an artifact that was left in when the code was originally
introduced in commit c611c76417f52b335ecaab01c61743e3b705eb7c ("virtio:
add MemoryListener to cache ring translations"). Paolo could confirm
this.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

Re: [PATCH 1/3] hw/virtio: Document virtio_queue_packed_empty_rcu is called within RCU

[Stefan Hajnoczi]

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

On Thu, Aug 26, 2021 at 07:26:56PM +0200, Philippe Mathieu-Daudé wrote:
> While virtio_queue_packed_empty_rcu() uses the '_rcu' suffix,
> it is not obvious it is called within rcu_read_lock(). All other
> functions from this file called with the RCU locked have a comment
> describing it. Document this one similarly for consistency.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/virtio/virtio.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

Re: [RFC PATCH 3/3] hw/virtio: Have virtqueue_get_avail_bytes() pass caches arg to callees

[Stefan Hajnoczi]

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

On Wed, Sep 01, 2021 at 05:55:38PM +0200, Stefano Garzarella wrote:
> On Thu, Aug 26, 2021 at 07:26:58PM +0200, Philippe Mathieu-Daudé wrote:
> > Both virtqueue_packed_get_avail_bytes() and
> > virtqueue_split_get_avail_bytes() access the region cache, but
> > their caller also does. Simplify by having virtqueue_get_avail_bytes
> > calling both with RCU lock held, and passing the caches as argument.
> > 
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > ---
> > RFC because I'm not sure this is safe enough
> 
> It seems safe to me.
> 
> While reviewing I saw that vring_get_region_caches() has
> /* Called within rcu_read_lock().  */ comment, but it seems to me that we
> call that function in places where we haven't acquired it, which shouldn't
> be a problem, but should we remove that comment?

Do you have specific examples? That sounds worrying because the caller
can't do much with the returned pointer if it was fetched outside the
RCU read lock.

Stefan

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

Re: [RFC PATCH 3/3] hw/virtio: Have virtqueue_get_avail_bytes() pass caches arg to callees

[Stefano Garzarella]

On Thu, Sep 02, 2021 at 01:12:33PM +0100, Stefan Hajnoczi wrote:
>On Wed, Sep 01, 2021 at 05:55:38PM +0200, Stefano Garzarella wrote:
>> On Thu, Aug 26, 2021 at 07:26:58PM +0200, Philippe Mathieu-Daudé wrote:
>> > Both virtqueue_packed_get_avail_bytes() and
>> > virtqueue_split_get_avail_bytes() access the region cache, but
>> > their caller also does. Simplify by having virtqueue_get_avail_bytes
>> > calling both with RCU lock held, and passing the caches as argument.
>> >
>> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> > ---
>> > RFC because I'm not sure this is safe enough
>>
>> It seems safe to me.
>>
>> While reviewing I saw that vring_get_region_caches() has
>> /* Called within rcu_read_lock().  */ comment, but it seems to me 
>> that we
>> call that function in places where we haven't acquired it, which shouldn't
>> be a problem, but should we remove that comment?
>
>Do you have specific examples? That sounds worrying because the caller
>can't do much with the returned pointer if it was fetched outside the
>RCU read lock.
>

One was the virtqueue_get_avail_bytes(), but Phil is fixing it in this 
patch.

Should we fix it in a separate patch to backport to stable branches?

Other suspicious places seem to be these:
- virtqueue_packed_fill_desc()
- virtqueue_packed_drop_all()

Stefano

Re: [RFC PATCH 3/3] hw/virtio: Have virtqueue_get_avail_bytes() pass caches arg to callees

[Stefan Hajnoczi]

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

On Thu, Sep 02, 2021 at 03:09:54PM +0200, Stefano Garzarella wrote:
> On Thu, Sep 02, 2021 at 01:12:33PM +0100, Stefan Hajnoczi wrote:
> > On Wed, Sep 01, 2021 at 05:55:38PM +0200, Stefano Garzarella wrote:
> > > On Thu, Aug 26, 2021 at 07:26:58PM +0200, Philippe Mathieu-Daudé wrote:
> > > > Both virtqueue_packed_get_avail_bytes() and
> > > > virtqueue_split_get_avail_bytes() access the region cache, but
> > > > their caller also does. Simplify by having virtqueue_get_avail_bytes
> > > > calling both with RCU lock held, and passing the caches as argument.
> > > >
> > > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > > > ---
> > > > RFC because I'm not sure this is safe enough
> > > 
> > > It seems safe to me.
> > > 
> > > While reviewing I saw that vring_get_region_caches() has
> > > /* Called within rcu_read_lock().  */ comment, but it seems to me
> > > that we
> > > call that function in places where we haven't acquired it, which shouldn't
> > > be a problem, but should we remove that comment?
> > 
> > Do you have specific examples? That sounds worrying because the caller
> > can't do much with the returned pointer if it was fetched outside the
> > RCU read lock.
> > 
> 
> One was the virtqueue_get_avail_bytes(), but Phil is fixing it in this
> patch.
> 
> Should we fix it in a separate patch to backport to stable branches?
> 
> Other suspicious places seem to be these:
> - virtqueue_packed_fill_desc()

This seems to be safe in practice: only
hw/net/virtio-net.c:virtio_net_receive_rcu() calls it via
virtqueue_flush(). virtqueue_flush() needs a doc comment indicating that
it must be called under the RCU read lock though.

> - virtqueue_packed_drop_all()

This looks broken.

Stefan

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