[Qemu-devel] [PATCH 0/7] memory: address_space_to_flatview needs RCU lock

[Qemu-devel] [PATCH 0/7] memory: address_space_to_flatview needs RCU lock

[Paolo Bonzini]

I noticed that the introduction of flatview_{read,write} placed
address_space_to_flatview outside the RCU lock.  This is wrong and has
to be fixed, because address_space_to_flatview does an atomic_rcu_read.
These patches fix this one function at a time.

Paolo Bonzini (7):
  openpic_kvm: drop address_space_to_flatview call
  memory: inline some performance-sensitive accessors
  address_space_write: address_space_to_flatview needs RCU lock
  address_space_read: address_space_to_flatview needs RCU lock
  address_space_access_valid: address_space_to_flatview needs RCU lock
  address_space_map: address_space_to_flatview needs RCU lock
  address_space_rw: address_space_to_flatview needs RCU lock

 exec.c                         | 90 +++++++++++++++++++++++++-----------------
 hw/intc/openpic_kvm.c          |  4 --
 include/exec/memory-internal.h | 13 ++++--
 include/exec/memory.h          | 47 ++++++++++++++--------
 memory.c                       | 30 --------------
 5 files changed, 93 insertions(+), 91 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH 1/7] openpic_kvm: drop address_space_to_flatview call

[Paolo Bonzini]

The MemoryListener is registered on address_space_memory, there is
not much to assert.  This currently works because the callback
is invoked only once when the listener is registered, but section->fv
is the _new_ FlatView, not the old one on later calls and that
would break.

This confines address_space_to_flatview to exec.c and memory.c.

Cc: qemu-stable@nongnu.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/intc/openpic_kvm.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/hw/intc/openpic_kvm.c b/hw/intc/openpic_kvm.c
index fa83420254..39a6f369c5 100644
--- a/hw/intc/openpic_kvm.c
+++ b/hw/intc/openpic_kvm.c
@@ -124,10 +124,6 @@ static void kvm_openpic_region_add(MemoryListener *listener,
     uint64_t reg_base;
     int ret;
 
-    if (section->fv != address_space_to_flatview(&address_space_memory)) {
-        abort();
-    }
-
     /* Ignore events on regions that are not us */
     if (section->mr != &opp->mem) {
         return;
-- 
2.14.3

Re: [Qemu-devel] [PATCH 1/7] openpic_kvm: drop address_space_to_flatview call

[David Gibson]

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

On Mon, Mar 05, 2018 at 09:36:49AM +0100, Paolo Bonzini wrote:
> The MemoryListener is registered on address_space_memory, there is
> not much to assert.  This currently works because the callback
> is invoked only once when the listener is registered, but section->fv
> is the _new_ FlatView, not the old one on later calls and that
> would break.
> 
> This confines address_space_to_flatview to exec.c and memory.c.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

Do you want me to take this through my tree?


> ---
>  hw/intc/openpic_kvm.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/hw/intc/openpic_kvm.c b/hw/intc/openpic_kvm.c
> index fa83420254..39a6f369c5 100644
> --- a/hw/intc/openpic_kvm.c
> +++ b/hw/intc/openpic_kvm.c
> @@ -124,10 +124,6 @@ static void kvm_openpic_region_add(MemoryListener *listener,
>      uint64_t reg_base;
>      int ret;
>  
> -    if (section->fv != address_space_to_flatview(&address_space_memory)) {
> -        abort();
> -    }
> -
>      /* Ignore events on regions that are not us */
>      if (section->mr != &opp->mem) {
>          return;

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

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

Re: [Qemu-devel] [PATCH 1/7] openpic_kvm: drop address_space_to_flatview call

[Paolo Bonzini]

On 06/03/2018 01:10, David Gibson wrote:
> On Mon, Mar 05, 2018 at 09:36:49AM +0100, Paolo Bonzini wrote:
>> The MemoryListener is registered on address_space_memory, there is
>> not much to assert.  This currently works because the callback
>> is invoked only once when the listener is registered, but section->fv
>> is the _new_ FlatView, not the old one on later calls and that
>> would break.
>>
>> This confines address_space_to_flatview to exec.c and memory.c.
>>
>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Acked-by: David Gibson <david@gibson.dropbear.id.au>
> 
> Do you want me to take this through my tree?

No need since Alexey has already reviewed the rest.  Thanks!

Paolo

> 
> 
>> ---
>>  hw/intc/openpic_kvm.c | 4 ----
>>  1 file changed, 4 deletions(-)
>>
>> diff --git a/hw/intc/openpic_kvm.c b/hw/intc/openpic_kvm.c
>> index fa83420254..39a6f369c5 100644
>> --- a/hw/intc/openpic_kvm.c
>> +++ b/hw/intc/openpic_kvm.c
>> @@ -124,10 +124,6 @@ static void kvm_openpic_region_add(MemoryListener *listener,
>>      uint64_t reg_base;
>>      int ret;
>>  
>> -    if (section->fv != address_space_to_flatview(&address_space_memory)) {
>> -        abort();
>> -    }
>> -
>>      /* Ignore events on regions that are not us */
>>      if (section->mr != &opp->mem) {
>>          return;
> 

Re: [Qemu-devel] [PATCH 1/7] openpic_kvm: drop address_space_to_flatview call

[Alexey Kardashevskiy]

On 05/03/18 19:36, Paolo Bonzini wrote:
> The MemoryListener is registered on address_space_memory, there is
> not much to assert.  This currently works because the callback
> is invoked only once when the listener is registered, but section->fv
> is the _new_ FlatView, not the old one on later calls and that
> would break.
> 
> This confines address_space_to_flatview to exec.c and memory.c.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>

> ---
>  hw/intc/openpic_kvm.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/hw/intc/openpic_kvm.c b/hw/intc/openpic_kvm.c
> index fa83420254..39a6f369c5 100644
> --- a/hw/intc/openpic_kvm.c
> +++ b/hw/intc/openpic_kvm.c
> @@ -124,10 +124,6 @@ static void kvm_openpic_region_add(MemoryListener *listener,
>      uint64_t reg_base;
>      int ret;
>  
> -    if (section->fv != address_space_to_flatview(&address_space_memory)) {
> -        abort();
> -    }
> -
>      /* Ignore events on regions that are not us */
>      if (section->mr != &opp->mem) {
>          return;
> 


-- 
Alexey

[Qemu-devel] [PATCH 2/7] memory: inline some performance-sensitive accessors

[Paolo Bonzini]

These accessors are called from inlined functions, and the call sequence
is much more expensive than just inlining the access.  Move the
struct declaration to memory-internal.h so that exec.c and memory.c
can both use an inline function.

Cc: qemu-stable@nongnu.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/exec/memory-internal.h | 13 +++++++++----
 include/exec/memory.h          | 22 +++++++++++++++++++++-
 memory.c                       | 30 ------------------------------
 3 files changed, 30 insertions(+), 35 deletions(-)

diff --git a/include/exec/memory-internal.h b/include/exec/memory-internal.h
index 4162474fd5..6a5ee42d36 100644
--- a/include/exec/memory-internal.h
+++ b/include/exec/memory-internal.h
@@ -21,7 +21,15 @@
 #define MEMORY_INTERNAL_H
 
 #ifndef CONFIG_USER_ONLY
-typedef struct AddressSpaceDispatch AddressSpaceDispatch;
+static inline AddressSpaceDispatch *flatview_to_dispatch(FlatView *fv)
+{
+    return fv->dispatch;
+}
+
+static inline AddressSpaceDispatch *address_space_to_dispatch(AddressSpace *as)
+{
+    return flatview_to_dispatch(address_space_to_flatview(as));
+}
 
 extern const MemoryRegionOps unassigned_mem_ops;
 
@@ -31,9 +39,6 @@ bool memory_region_access_valid(MemoryRegion *mr, hwaddr addr,
 void flatview_add_to_dispatch(FlatView *fv, MemoryRegionSection *section);
 AddressSpaceDispatch *address_space_dispatch_new(FlatView *fv);
 void address_space_dispatch_compact(AddressSpaceDispatch *d);
-
-AddressSpaceDispatch *address_space_to_dispatch(AddressSpace *as);
-AddressSpaceDispatch *flatview_to_dispatch(FlatView *fv);
 void address_space_dispatch_free(AddressSpaceDispatch *d);
 
 void mtree_print_dispatch(fprintf_function mon, void *f,
diff --git a/include/exec/memory.h b/include/exec/memory.h
index fff9b1d871..6c8e394675 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -326,7 +326,27 @@ struct AddressSpace {
     QTAILQ_ENTRY(AddressSpace) address_spaces_link;
 };
 
-FlatView *address_space_to_flatview(AddressSpace *as);
+typedef struct AddressSpaceDispatch AddressSpaceDispatch;
+typedef struct FlatRange FlatRange;
+
+/* Flattened global view of current active memory hierarchy.  Kept in sorted
+ * order.
+ */
+struct FlatView {
+    struct rcu_head rcu;
+    unsigned ref;
+    FlatRange *ranges;
+    unsigned nr;
+    unsigned nr_allocated;
+    struct AddressSpaceDispatch *dispatch;
+    MemoryRegion *root;
+};
+
+static inline FlatView *address_space_to_flatview(AddressSpace *as)
+{
+    return atomic_rcu_read(&as->current_map);
+}
+
 
 /**
  * MemoryRegionSection: describes a fragment of a #MemoryRegion
diff --git a/memory.c b/memory.c
index c7f6588452..78d07aa51d 100644
--- a/memory.c
+++ b/memory.c
@@ -210,8 +210,6 @@ static bool memory_region_ioeventfd_equal(MemoryRegionIoeventfd a,
         && !memory_region_ioeventfd_before(b, a);
 }
 
-typedef struct FlatRange FlatRange;
-
 /* Range of memory in the global map.  Addresses are absolute. */
 struct FlatRange {
     MemoryRegion *mr;
@@ -222,19 +220,6 @@ struct FlatRange {
     bool readonly;
 };
 
-/* Flattened global view of current active memory hierarchy.  Kept in sorted
- * order.
- */
-struct FlatView {
-    struct rcu_head rcu;
-    unsigned ref;
-    FlatRange *ranges;
-    unsigned nr;
-    unsigned nr_allocated;
-    struct AddressSpaceDispatch *dispatch;
-    MemoryRegion *root;
-};
-
 typedef struct AddressSpaceOps AddressSpaceOps;
 
 #define FOR_EACH_FLAT_RANGE(var, view)          \
@@ -322,21 +307,6 @@ static void flatview_unref(FlatView *view)
     }
 }
 
-FlatView *address_space_to_flatview(AddressSpace *as)
-{
-    return atomic_rcu_read(&as->current_map);
-}
-
-AddressSpaceDispatch *flatview_to_dispatch(FlatView *fv)
-{
-    return fv->dispatch;
-}
-
-AddressSpaceDispatch *address_space_to_dispatch(AddressSpace *as)
-{
-    return flatview_to_dispatch(address_space_to_flatview(as));
-}
-
 static bool can_merge(FlatRange *r1, FlatRange *r2)
 {
     return int128_eq(addrrange_end(r1->addr), r2->addr.start)
-- 
2.14.3

Re: [Qemu-devel] [PATCH 2/7] memory: inline some performance-sensitive accessors

[Alexey Kardashevskiy]

On 05/03/18 19:36, Paolo Bonzini wrote:
> These accessors are called from inlined functions, and the call sequence
> is much more expensive than just inlining the access.  Move the
> struct declaration to memory-internal.h so that exec.c and memory.c
> can both use an inline function.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>

> ---
>  include/exec/memory-internal.h | 13 +++++++++----
>  include/exec/memory.h          | 22 +++++++++++++++++++++-
>  memory.c                       | 30 ------------------------------
>  3 files changed, 30 insertions(+), 35 deletions(-)
> 
> diff --git a/include/exec/memory-internal.h b/include/exec/memory-internal.h
> index 4162474fd5..6a5ee42d36 100644
> --- a/include/exec/memory-internal.h
> +++ b/include/exec/memory-internal.h
> @@ -21,7 +21,15 @@
>  #define MEMORY_INTERNAL_H
>  
>  #ifndef CONFIG_USER_ONLY
> -typedef struct AddressSpaceDispatch AddressSpaceDispatch;
> +static inline AddressSpaceDispatch *flatview_to_dispatch(FlatView *fv)
> +{
> +    return fv->dispatch;
> +}
> +
> +static inline AddressSpaceDispatch *address_space_to_dispatch(AddressSpace *as)
> +{
> +    return flatview_to_dispatch(address_space_to_flatview(as));
> +}
>  
>  extern const MemoryRegionOps unassigned_mem_ops;
>  
> @@ -31,9 +39,6 @@ bool memory_region_access_valid(MemoryRegion *mr, hwaddr addr,
>  void flatview_add_to_dispatch(FlatView *fv, MemoryRegionSection *section);
>  AddressSpaceDispatch *address_space_dispatch_new(FlatView *fv);
>  void address_space_dispatch_compact(AddressSpaceDispatch *d);
> -
> -AddressSpaceDispatch *address_space_to_dispatch(AddressSpace *as);
> -AddressSpaceDispatch *flatview_to_dispatch(FlatView *fv);
>  void address_space_dispatch_free(AddressSpaceDispatch *d);
>  
>  void mtree_print_dispatch(fprintf_function mon, void *f,
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index fff9b1d871..6c8e394675 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -326,7 +326,27 @@ struct AddressSpace {
>      QTAILQ_ENTRY(AddressSpace) address_spaces_link;
>  };
>  
> -FlatView *address_space_to_flatview(AddressSpace *as);
> +typedef struct AddressSpaceDispatch AddressSpaceDispatch;
> +typedef struct FlatRange FlatRange;
> +
> +/* Flattened global view of current active memory hierarchy.  Kept in sorted
> + * order.
> + */
> +struct FlatView {
> +    struct rcu_head rcu;
> +    unsigned ref;
> +    FlatRange *ranges;
> +    unsigned nr;
> +    unsigned nr_allocated;
> +    struct AddressSpaceDispatch *dispatch;
> +    MemoryRegion *root;
> +};
> +
> +static inline FlatView *address_space_to_flatview(AddressSpace *as)
> +{
> +    return atomic_rcu_read(&as->current_map);
> +}
> +
>  
>  /**
>   * MemoryRegionSection: describes a fragment of a #MemoryRegion
> diff --git a/memory.c b/memory.c
> index c7f6588452..78d07aa51d 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -210,8 +210,6 @@ static bool memory_region_ioeventfd_equal(MemoryRegionIoeventfd a,
>          && !memory_region_ioeventfd_before(b, a);
>  }
>  
> -typedef struct FlatRange FlatRange;
> -
>  /* Range of memory in the global map.  Addresses are absolute. */
>  struct FlatRange {
>      MemoryRegion *mr;
> @@ -222,19 +220,6 @@ struct FlatRange {
>      bool readonly;
>  };
>  
> -/* Flattened global view of current active memory hierarchy.  Kept in sorted
> - * order.
> - */
> -struct FlatView {
> -    struct rcu_head rcu;
> -    unsigned ref;
> -    FlatRange *ranges;
> -    unsigned nr;
> -    unsigned nr_allocated;
> -    struct AddressSpaceDispatch *dispatch;
> -    MemoryRegion *root;
> -};
> -
>  typedef struct AddressSpaceOps AddressSpaceOps;
>  
>  #define FOR_EACH_FLAT_RANGE(var, view)          \
> @@ -322,21 +307,6 @@ static void flatview_unref(FlatView *view)
>      }
>  }
>  
> -FlatView *address_space_to_flatview(AddressSpace *as)
> -{
> -    return atomic_rcu_read(&as->current_map);
> -}
> -
> -AddressSpaceDispatch *flatview_to_dispatch(FlatView *fv)
> -{
> -    return fv->dispatch;
> -}
> -
> -AddressSpaceDispatch *address_space_to_dispatch(AddressSpace *as)
> -{
> -    return flatview_to_dispatch(address_space_to_flatview(as));
> -}
> -
>  static bool can_merge(FlatRange *r1, FlatRange *r2)
>  {
>      return int128_eq(addrrange_end(r1->addr), r2->addr.start)
> 


-- 
Alexey

[Qemu-devel] [PATCH 3/7] address_space_write: address_space_to_flatview needs RCU lock

[Paolo Bonzini]

address_space_write is calling address_space_to_flatview but it can
be called outside the RCU lock.  To fix it, push the rcu_read_lock/unlock
pair up from flatview_write to address_space_write.

Cc: qemu-stable@nongnu.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 exec.c | 37 ++++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/exec.c b/exec.c
index e8d7b335b6..0b74b58d45 100644
--- a/exec.c
+++ b/exec.c
@@ -3074,6 +3074,7 @@ static MemTxResult flatview_write_continue(FlatView *fv, hwaddr addr,
     return result;
 }
 
+/* Called from RCU critical section.  */
 static MemTxResult flatview_write(FlatView *fv, hwaddr addr, MemTxAttrs attrs,
                                   const uint8_t *buf, int len)
 {
@@ -3082,25 +3083,14 @@ static MemTxResult flatview_write(FlatView *fv, hwaddr addr, MemTxAttrs attrs,
     MemoryRegion *mr;
     MemTxResult result = MEMTX_OK;
 
-    if (len > 0) {
-        rcu_read_lock();
-        l = len;
-        mr = flatview_translate(fv, addr, &addr1, &l, true);
-        result = flatview_write_continue(fv, addr, attrs, buf, len,
-                                         addr1, l, mr);
-        rcu_read_unlock();
-    }
+    l = len;
+    mr = flatview_translate(fv, addr, &addr1, &l, true);
+    result = flatview_write_continue(fv, addr, attrs, buf, len,
+                                     addr1, l, mr);
 
     return result;
 }
 
-MemTxResult address_space_write(AddressSpace *as, hwaddr addr,
-                                              MemTxAttrs attrs,
-                                              const uint8_t *buf, int len)
-{
-    return flatview_write(address_space_to_flatview(as), addr, attrs, buf, len);
-}
-
 /* Called within RCU critical section.  */
 MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr,
                                    MemTxAttrs attrs, uint8_t *buf,
@@ -3209,6 +3199,23 @@ MemTxResult address_space_rw(AddressSpace *as, hwaddr addr,
                        addr, attrs, buf, len, is_write);
 }
 
+MemTxResult address_space_write(AddressSpace *as, hwaddr addr,
+                                MemTxAttrs attrs,
+                                const uint8_t *buf, int len)
+{
+    MemTxResult result = MEMTX_OK;
+    FlatView *fv;
+
+    if (len > 0) {
+        rcu_read_lock();
+        fv = address_space_to_flatview(as);
+        result = flatview_write(fv, addr, attrs, buf, len);
+        rcu_read_unlock();
+    }
+
+    return result;
+}
+
 void cpu_physical_memory_rw(hwaddr addr, uint8_t *buf,
                             int len, int is_write)
 {
-- 
2.14.3

Re: [Qemu-devel] [PATCH 3/7] address_space_write: address_space_to_flatview needs RCU lock

[Alexey Kardashevskiy]

On 05/03/18 19:36, Paolo Bonzini wrote:
> address_space_write is calling address_space_to_flatview but it can
> be called outside the RCU lock.  To fix it, push the rcu_read_lock/unlock
> pair up from flatview_write to address_space_write.
> 
> Cc: qemu-stable@nongnu.org

Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>


> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  exec.c | 37 ++++++++++++++++++++++---------------
>  1 file changed, 22 insertions(+), 15 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index e8d7b335b6..0b74b58d45 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -3074,6 +3074,7 @@ static MemTxResult flatview_write_continue(FlatView *fv, hwaddr addr,
>      return result;
>  }
>  
> +/* Called from RCU critical section.  */
>  static MemTxResult flatview_write(FlatView *fv, hwaddr addr, MemTxAttrs attrs,
>                                    const uint8_t *buf, int len)
>  {
> @@ -3082,25 +3083,14 @@ static MemTxResult flatview_write(FlatView *fv, hwaddr addr, MemTxAttrs attrs,
>      MemoryRegion *mr;
>      MemTxResult result = MEMTX_OK;
>  
> -    if (len > 0) {
> -        rcu_read_lock();
> -        l = len;
> -        mr = flatview_translate(fv, addr, &addr1, &l, true);
> -        result = flatview_write_continue(fv, addr, attrs, buf, len,
> -                                         addr1, l, mr);
> -        rcu_read_unlock();
> -    }
> +    l = len;
> +    mr = flatview_translate(fv, addr, &addr1, &l, true);
> +    result = flatview_write_continue(fv, addr, attrs, buf, len,
> +                                     addr1, l, mr);
>  
>      return result;
>  }
>  
> -MemTxResult address_space_write(AddressSpace *as, hwaddr addr,
> -                                              MemTxAttrs attrs,
> -                                              const uint8_t *buf, int len)
> -{
> -    return flatview_write(address_space_to_flatview(as), addr, attrs, buf, len);
> -}
> -
>  /* Called within RCU critical section.  */
>  MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr,
>                                     MemTxAttrs attrs, uint8_t *buf,
> @@ -3209,6 +3199,23 @@ MemTxResult address_space_rw(AddressSpace *as, hwaddr addr,
>                         addr, attrs, buf, len, is_write);
>  }
>  
> +MemTxResult address_space_write(AddressSpace *as, hwaddr addr,
> +                                MemTxAttrs attrs,
> +                                const uint8_t *buf, int len)
> +{
> +    MemTxResult result = MEMTX_OK;
> +    FlatView *fv;
> +
> +    if (len > 0) {
> +        rcu_read_lock();
> +        fv = address_space_to_flatview(as);
> +        result = flatview_write(fv, addr, attrs, buf, len);
> +        rcu_read_unlock();
> +    }
> +
> +    return result;
> +}
> +
>  void cpu_physical_memory_rw(hwaddr addr, uint8_t *buf,
>                              int len, int is_write)
>  {
> 


-- 
Alexey

[Qemu-devel] [PATCH 4/7] address_space_read: address_space_to_flatview needs RCU lock

[Paolo Bonzini]

address_space_read is calling address_space_to_flatview but it can
be called outside the RCU lock.  To fix it, push the rcu_read_lock/unlock
pair up from flatview_read_full to address_space_read's constant size
fast path and address_space_read_full.

Cc: qemu-stable@nongnu.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 exec.c                | 44 ++++++++++++++++++++++++++++----------------
 include/exec/memory.h | 25 ++++++++++---------------
 2 files changed, 38 insertions(+), 31 deletions(-)

diff --git a/exec.c b/exec.c
index 0b74b58d45..55b7452bd7 100644
--- a/exec.c
+++ b/exec.c
@@ -2612,6 +2612,8 @@ static const MemoryRegionOps watch_mem_ops = {
     },
 };
 
+static MemTxResult flatview_read(FlatView *fv, hwaddr addr,
+                                      MemTxAttrs attrs, uint8_t *buf, int len);
 static MemTxResult flatview_write(FlatView *fv, hwaddr addr, MemTxAttrs attrs,
                                   const uint8_t *buf, int len);
 static bool flatview_access_valid(FlatView *fv, hwaddr addr, int len,
@@ -3161,24 +3163,18 @@ MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr,
     return result;
 }
 
-MemTxResult flatview_read_full(FlatView *fv, hwaddr addr,
-                               MemTxAttrs attrs, uint8_t *buf, int len)
+/* Called from RCU critical section.  */
+static MemTxResult flatview_read(FlatView *fv, hwaddr addr,
+                                 MemTxAttrs attrs, uint8_t *buf, int len)
 {
     hwaddr l;
     hwaddr addr1;
     MemoryRegion *mr;
-    MemTxResult result = MEMTX_OK;
-
-    if (len > 0) {
-        rcu_read_lock();
-        l = len;
-        mr = flatview_translate(fv, addr, &addr1, &l, false);
-        result = flatview_read_continue(fv, addr, attrs, buf, len,
-                                        addr1, l, mr);
-        rcu_read_unlock();
-    }
 
-    return result;
+    l = len;
+    mr = flatview_translate(fv, addr, &addr1, &l, false);
+    return flatview_read_continue(fv, addr, attrs, buf, len,
+                                  addr1, l, mr);
 }
 
 static MemTxResult flatview_rw(FlatView *fv, hwaddr addr, MemTxAttrs attrs,
@@ -3199,6 +3195,22 @@ MemTxResult address_space_rw(AddressSpace *as, hwaddr addr,
                        addr, attrs, buf, len, is_write);
 }
 
+MemTxResult address_space_read_full(AddressSpace *as, hwaddr addr,
+                                    MemTxAttrs attrs, uint8_t *buf, int len)
+{
+    MemTxResult result = MEMTX_OK;
+    FlatView *fv;
+
+    if (len > 0) {
+        rcu_read_lock();
+        fv = address_space_to_flatview(as);
+        result = flatview_read(fv, addr, attrs, buf, len);
+        rcu_read_unlock();
+    }
+
+    return result;
+}
+
 MemTxResult address_space_write(AddressSpace *as, hwaddr addr,
                                 MemTxAttrs attrs,
                                 const uint8_t *buf, int len)
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 6c8e394675..54ff81fbd1 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1894,13 +1894,12 @@ void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len,
 
 
 /* Internal functions, part of the implementation of address_space_read.  */
+MemTxResult address_space_read_full(AddressSpace *as, hwaddr addr,
+                                    MemTxAttrs attrs, uint8_t *buf, int len);
 MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr,
                                    MemTxAttrs attrs, uint8_t *buf,
                                    int len, hwaddr addr1, hwaddr l,
                                    MemoryRegion *mr);
-
-MemTxResult flatview_read_full(FlatView *fv, hwaddr addr,
-                               MemTxAttrs attrs, uint8_t *buf, int len);
 void *qemu_map_ram_ptr(RAMBlock *ram_block, ram_addr_t addr);
 
 static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
@@ -1919,25 +1918,28 @@ static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
  *
  * Return a MemTxResult indicating whether the operation succeeded
  * or failed (eg unassigned memory, device rejected the transaction,
- * IOMMU fault).
+ * IOMMU fault).  Called within RCU critical section.
  *
- * @fv: #FlatView to be accessed
+ * @as: #AddressSpace to be accessed
  * @addr: address within that address space
  * @attrs: memory transaction attributes
  * @buf: buffer with the data transferred
  */
 static inline __attribute__((__always_inline__))
-MemTxResult flatview_read(FlatView *fv, hwaddr addr, MemTxAttrs attrs,
-                          uint8_t *buf, int len)
+MemTxResult address_space_read(AddressSpace *as, hwaddr addr,
+                               MemTxAttrs attrs, uint8_t *buf,
+                               int len)
 {
     MemTxResult result = MEMTX_OK;
     hwaddr l, addr1;
     void *ptr;
     MemoryRegion *mr;
+    FlatView *fv;
 
     if (__builtin_constant_p(len)) {
         if (len) {
             rcu_read_lock();
+            fv = address_space_to_flatview(as);
             l = len;
             mr = flatview_translate(fv, addr, &addr1, &l, false);
             if (len == l && memory_access_is_direct(mr, false)) {
@@ -1950,18 +1952,11 @@ MemTxResult flatview_read(FlatView *fv, hwaddr addr, MemTxAttrs attrs,
             rcu_read_unlock();
         }
     } else {
-        result = flatview_read_full(fv, addr, attrs, buf, len);
+        result = address_space_read_full(as, addr, attrs, buf, len);
     }
     return result;
 }
 
-static inline MemTxResult address_space_read(AddressSpace *as, hwaddr addr,
-                                             MemTxAttrs attrs, uint8_t *buf,
-                                             int len)
-{
-    return flatview_read(address_space_to_flatview(as), addr, attrs, buf, len);
-}
-
 /**
  * address_space_read_cached: read from a cached RAM region
  *
-- 
2.14.3

Re: [Qemu-devel] [PATCH 4/7] address_space_read: address_space_to_flatview needs RCU lock

[Alexey Kardashevskiy]

On 05/03/18 19:36, Paolo Bonzini wrote:
> address_space_read is calling address_space_to_flatview but it can
> be called outside the RCU lock.  To fix it, push the rcu_read_lock/unlock
> pair up from flatview_read_full to address_space_read's constant size
> fast path and address_space_read_full.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>

> ---
>  exec.c                | 44 ++++++++++++++++++++++++++++----------------
>  include/exec/memory.h | 25 ++++++++++---------------
>  2 files changed, 38 insertions(+), 31 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 0b74b58d45..55b7452bd7 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2612,6 +2612,8 @@ static const MemoryRegionOps watch_mem_ops = {
>      },
>  };
>  
> +static MemTxResult flatview_read(FlatView *fv, hwaddr addr,
> +                                      MemTxAttrs attrs, uint8_t *buf, int len);
>  static MemTxResult flatview_write(FlatView *fv, hwaddr addr, MemTxAttrs attrs,
>                                    const uint8_t *buf, int len);
>  static bool flatview_access_valid(FlatView *fv, hwaddr addr, int len,
> @@ -3161,24 +3163,18 @@ MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr,
>      return result;
>  }
>  
> -MemTxResult flatview_read_full(FlatView *fv, hwaddr addr,
> -                               MemTxAttrs attrs, uint8_t *buf, int len)
> +/* Called from RCU critical section.  */
> +static MemTxResult flatview_read(FlatView *fv, hwaddr addr,
> +                                 MemTxAttrs attrs, uint8_t *buf, int len)
>  {
>      hwaddr l;
>      hwaddr addr1;
>      MemoryRegion *mr;
> -    MemTxResult result = MEMTX_OK;
> -
> -    if (len > 0) {
> -        rcu_read_lock();
> -        l = len;
> -        mr = flatview_translate(fv, addr, &addr1, &l, false);
> -        result = flatview_read_continue(fv, addr, attrs, buf, len,
> -                                        addr1, l, mr);
> -        rcu_read_unlock();
> -    }
>  
> -    return result;
> +    l = len;
> +    mr = flatview_translate(fv, addr, &addr1, &l, false);
> +    return flatview_read_continue(fv, addr, attrs, buf, len,
> +                                  addr1, l, mr);
>  }
>  
>  static MemTxResult flatview_rw(FlatView *fv, hwaddr addr, MemTxAttrs attrs,
> @@ -3199,6 +3195,22 @@ MemTxResult address_space_rw(AddressSpace *as, hwaddr addr,
>                         addr, attrs, buf, len, is_write);
>  }
>  
> +MemTxResult address_space_read_full(AddressSpace *as, hwaddr addr,
> +                                    MemTxAttrs attrs, uint8_t *buf, int len)
> +{
> +    MemTxResult result = MEMTX_OK;
> +    FlatView *fv;
> +
> +    if (len > 0) {
> +        rcu_read_lock();
> +        fv = address_space_to_flatview(as);
> +        result = flatview_read(fv, addr, attrs, buf, len);
> +        rcu_read_unlock();
> +    }
> +
> +    return result;
> +}
> +
>  MemTxResult address_space_write(AddressSpace *as, hwaddr addr,
>                                  MemTxAttrs attrs,
>                                  const uint8_t *buf, int len)
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 6c8e394675..54ff81fbd1 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -1894,13 +1894,12 @@ void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len,
>  
>  
>  /* Internal functions, part of the implementation of address_space_read.  */
> +MemTxResult address_space_read_full(AddressSpace *as, hwaddr addr,
> +                                    MemTxAttrs attrs, uint8_t *buf, int len);
>  MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr,
>                                     MemTxAttrs attrs, uint8_t *buf,
>                                     int len, hwaddr addr1, hwaddr l,
>                                     MemoryRegion *mr);
> -
> -MemTxResult flatview_read_full(FlatView *fv, hwaddr addr,
> -                               MemTxAttrs attrs, uint8_t *buf, int len);
>  void *qemu_map_ram_ptr(RAMBlock *ram_block, ram_addr_t addr);
>  
>  static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
> @@ -1919,25 +1918,28 @@ static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
>   *
>   * Return a MemTxResult indicating whether the operation succeeded
>   * or failed (eg unassigned memory, device rejected the transaction,
> - * IOMMU fault).
> + * IOMMU fault).  Called within RCU critical section.
>   *
> - * @fv: #FlatView to be accessed
> + * @as: #AddressSpace to be accessed
>   * @addr: address within that address space
>   * @attrs: memory transaction attributes
>   * @buf: buffer with the data transferred
>   */
>  static inline __attribute__((__always_inline__))
> -MemTxResult flatview_read(FlatView *fv, hwaddr addr, MemTxAttrs attrs,
> -                          uint8_t *buf, int len)
> +MemTxResult address_space_read(AddressSpace *as, hwaddr addr,
> +                               MemTxAttrs attrs, uint8_t *buf,
> +                               int len)
>  {
>      MemTxResult result = MEMTX_OK;
>      hwaddr l, addr1;
>      void *ptr;
>      MemoryRegion *mr;
> +    FlatView *fv;
>  
>      if (__builtin_constant_p(len)) {
>          if (len) {
>              rcu_read_lock();
> +            fv = address_space_to_flatview(as);
>              l = len;
>              mr = flatview_translate(fv, addr, &addr1, &l, false);
>              if (len == l && memory_access_is_direct(mr, false)) {
> @@ -1950,18 +1952,11 @@ MemTxResult flatview_read(FlatView *fv, hwaddr addr, MemTxAttrs attrs,
>              rcu_read_unlock();
>          }
>      } else {
> -        result = flatview_read_full(fv, addr, attrs, buf, len);
> +        result = address_space_read_full(as, addr, attrs, buf, len);
>      }
>      return result;
>  }
>  
> -static inline MemTxResult address_space_read(AddressSpace *as, hwaddr addr,
> -                                             MemTxAttrs attrs, uint8_t *buf,
> -                                             int len)
> -{
> -    return flatview_read(address_space_to_flatview(as), addr, attrs, buf, len);
> -}
> -
>  /**
>   * address_space_read_cached: read from a cached RAM region
>   *
> 


-- 
Alexey

[Qemu-devel] [PATCH 5/7] address_space_access_valid: address_space_to_flatview needs RCU lock

[Paolo Bonzini]

address_space_access_valid is calling address_space_to_flatview but it can
be called outside the RCU lock.  To fix it, push the rcu_read_lock/unlock
pair up from flatview_access_valid to address_space_access_valid.

Cc: qemu-stable@nongnu.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 exec.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/exec.c b/exec.c
index 55b7452bd7..177583c2ee 100644
--- a/exec.c
+++ b/exec.c
@@ -3391,7 +3391,6 @@ static bool flatview_access_valid(FlatView *fv, hwaddr addr, int len,
     MemoryRegion *mr;
     hwaddr l, xlat;
 
-    rcu_read_lock();
     while (len > 0) {
         l = len;
         mr = flatview_translate(fv, addr, &xlat, &l, is_write);
@@ -3406,15 +3405,20 @@ static bool flatview_access_valid(FlatView *fv, hwaddr addr, int len,
         len -= l;
         addr += l;
     }
-    rcu_read_unlock();
     return true;
 }
 
 bool address_space_access_valid(AddressSpace *as, hwaddr addr,
                                 int len, bool is_write)
 {
-    return flatview_access_valid(address_space_to_flatview(as),
-                                 addr, len, is_write);
+    FlatView *fv;
+    bool result;
+
+    rcu_read_lock();
+    fv = address_space_to_flatview(as);
+    result = flatview_access_valid(fv, addr, len, is_write);
+    rcu_read_unlock();
+    return result;
 }
 
 static hwaddr
-- 
2.14.3

Re: [Qemu-devel] [PATCH 5/7] address_space_access_valid: address_space_to_flatview needs RCU lock

[Alexey Kardashevskiy]

On 05/03/18 19:36, Paolo Bonzini wrote:
> address_space_access_valid is calling address_space_to_flatview but it can
> be called outside the RCU lock.  To fix it, push the rcu_read_lock/unlock
> pair up from flatview_access_valid to address_space_access_valid.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>

> ---
>  exec.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 55b7452bd7..177583c2ee 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -3391,7 +3391,6 @@ static bool flatview_access_valid(FlatView *fv, hwaddr addr, int len,
>      MemoryRegion *mr;
>      hwaddr l, xlat;
>  
> -    rcu_read_lock();
>      while (len > 0) {
>          l = len;
>          mr = flatview_translate(fv, addr, &xlat, &l, is_write);
> @@ -3406,15 +3405,20 @@ static bool flatview_access_valid(FlatView *fv, hwaddr addr, int len,
>          len -= l;
>          addr += l;
>      }
> -    rcu_read_unlock();
>      return true;
>  }
>  
>  bool address_space_access_valid(AddressSpace *as, hwaddr addr,
>                                  int len, bool is_write)
>  {
> -    return flatview_access_valid(address_space_to_flatview(as),
> -                                 addr, len, is_write);
> +    FlatView *fv;
> +    bool result;
> +
> +    rcu_read_lock();
> +    fv = address_space_to_flatview(as);
> +    result = flatview_access_valid(fv, addr, len, is_write);
> +    rcu_read_unlock();
> +    return result;
>  }
>  
>  static hwaddr
> 


-- 
Alexey

[Qemu-devel] [PATCH 6/7] address_space_map: address_space_to_flatview needs RCU lock

[Paolo Bonzini]

address_space_map is calling address_space_to_flatview but it can
be called outside the RCU lock.  The function itself is calling
rcu_read_lock/rcu_read_unlock, just in the wrong place, so the
fix is easy.

Cc: qemu-stable@nongnu.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 exec.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/exec.c b/exec.c
index 177583c2ee..070eaff3e7 100644
--- a/exec.c
+++ b/exec.c
@@ -3464,7 +3464,7 @@ void *address_space_map(AddressSpace *as,
     hwaddr l, xlat;
     MemoryRegion *mr;
     void *ptr;
-    FlatView *fv = address_space_to_flatview(as);
+    FlatView *fv;
 
     if (len == 0) {
         return NULL;
@@ -3472,6 +3472,7 @@ void *address_space_map(AddressSpace *as,
 
     l = len;
     rcu_read_lock();
+    fv = address_space_to_flatview(as);
     mr = flatview_translate(fv, addr, &xlat, &l, is_write);
 
     if (!memory_access_is_direct(mr, is_write)) {
-- 
2.14.3

Re: [Qemu-devel] [PATCH 6/7] address_space_map: address_space_to_flatview needs RCU lock

[Alexey Kardashevskiy]

On 05/03/18 19:36, Paolo Bonzini wrote:
> address_space_map is calling address_space_to_flatview but it can
> be called outside the RCU lock.  The function itself is calling
> rcu_read_lock/rcu_read_unlock, just in the wrong place, so the
> fix is easy.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>


> ---
>  exec.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/exec.c b/exec.c
> index 177583c2ee..070eaff3e7 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -3464,7 +3464,7 @@ void *address_space_map(AddressSpace *as,
>      hwaddr l, xlat;
>      MemoryRegion *mr;
>      void *ptr;
> -    FlatView *fv = address_space_to_flatview(as);
> +    FlatView *fv;
>  
>      if (len == 0) {
>          return NULL;
> @@ -3472,6 +3472,7 @@ void *address_space_map(AddressSpace *as,
>  
>      l = len;
>      rcu_read_lock();
> +    fv = address_space_to_flatview(as);
>      mr = flatview_translate(fv, addr, &xlat, &l, is_write);
>  
>      if (!memory_access_is_direct(mr, is_write)) {
> 


-- 
Alexey

[Qemu-devel] [PATCH 7/7] address_space_rw: address_space_to_flatview needs RCU lock

[Paolo Bonzini]

address_space_rw is calling address_space_to_flatview but it can
be called outside the RCU lock.  To fix it, transform flatview_rw
into address_space_rw, since flatview_rw is otherwise unused.

Cc: qemu-stable@nongnu.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 exec.c | 28 ++++++++++------------------
 1 file changed, 10 insertions(+), 18 deletions(-)

diff --git a/exec.c b/exec.c
index 070eaff3e7..8a99114c69 100644
--- a/exec.c
+++ b/exec.c
@@ -3177,24 +3177,6 @@ static MemTxResult flatview_read(FlatView *fv, hwaddr addr,
                                   addr1, l, mr);
 }
 
-static MemTxResult flatview_rw(FlatView *fv, hwaddr addr, MemTxAttrs attrs,
-                               uint8_t *buf, int len, bool is_write)
-{
-    if (is_write) {
-        return flatview_write(fv, addr, attrs, (uint8_t *)buf, len);
-    } else {
-        return flatview_read(fv, addr, attrs, (uint8_t *)buf, len);
-    }
-}
-
-MemTxResult address_space_rw(AddressSpace *as, hwaddr addr,
-                             MemTxAttrs attrs, uint8_t *buf,
-                             int len, bool is_write)
-{
-    return flatview_rw(address_space_to_flatview(as),
-                       addr, attrs, buf, len, is_write);
-}
-
 MemTxResult address_space_read_full(AddressSpace *as, hwaddr addr,
                                     MemTxAttrs attrs, uint8_t *buf, int len)
 {
@@ -3228,6 +3210,16 @@ MemTxResult address_space_write(AddressSpace *as, hwaddr addr,
     return result;
 }
 
+MemTxResult address_space_rw(AddressSpace *as, hwaddr addr, MemTxAttrs attrs,
+                             uint8_t *buf, int len, bool is_write)
+{
+    if (is_write) {
+        return address_space_write(as, addr, attrs, buf, len);
+    } else {
+        return address_space_read_full(as, addr, attrs, buf, len);
+    }
+}
+
 void cpu_physical_memory_rw(hwaddr addr, uint8_t *buf,
                             int len, int is_write)
 {
-- 
2.14.3

Re: [Qemu-devel] [PATCH 7/7] address_space_rw: address_space_to_flatview needs RCU lock

[Alexey Kardashevskiy]

On 05/03/18 19:36, Paolo Bonzini wrote:
> address_space_rw is calling address_space_to_flatview but it can
> be called outside the RCU lock.  To fix it, transform flatview_rw
> into address_space_rw, since flatview_rw is otherwise unused.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>



> ---
>  exec.c | 28 ++++++++++------------------
>  1 file changed, 10 insertions(+), 18 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 070eaff3e7..8a99114c69 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -3177,24 +3177,6 @@ static MemTxResult flatview_read(FlatView *fv, hwaddr addr,
>                                    addr1, l, mr);
>  }
>  
> -static MemTxResult flatview_rw(FlatView *fv, hwaddr addr, MemTxAttrs attrs,
> -                               uint8_t *buf, int len, bool is_write)
> -{
> -    if (is_write) {
> -        return flatview_write(fv, addr, attrs, (uint8_t *)buf, len);
> -    } else {
> -        return flatview_read(fv, addr, attrs, (uint8_t *)buf, len);
> -    }
> -}
> -
> -MemTxResult address_space_rw(AddressSpace *as, hwaddr addr,
> -                             MemTxAttrs attrs, uint8_t *buf,
> -                             int len, bool is_write)
> -{
> -    return flatview_rw(address_space_to_flatview(as),
> -                       addr, attrs, buf, len, is_write);
> -}
> -
>  MemTxResult address_space_read_full(AddressSpace *as, hwaddr addr,
>                                      MemTxAttrs attrs, uint8_t *buf, int len)
>  {
> @@ -3228,6 +3210,16 @@ MemTxResult address_space_write(AddressSpace *as, hwaddr addr,
>      return result;
>  }
>  
> +MemTxResult address_space_rw(AddressSpace *as, hwaddr addr, MemTxAttrs attrs,
> +                             uint8_t *buf, int len, bool is_write)
> +{
> +    if (is_write) {
> +        return address_space_write(as, addr, attrs, buf, len);
> +    } else {
> +        return address_space_read_full(as, addr, attrs, buf, len);
> +    }
> +}
> +
>  void cpu_physical_memory_rw(hwaddr addr, uint8_t *buf,
>                              int len, int is_write)
>  {
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH 0/7] memory: address_space_to_flatview needs RCU lock

[Alexey Kardashevskiy]

On 05/03/18 19:36, Paolo Bonzini wrote:
> I noticed that the introduction of flatview_{read,write} placed
> address_space_to_flatview outside the RCU lock.  This is wrong and has
> to be fixed, because address_space_to_flatview does an atomic_rcu_read.
> These patches fix this one function at a time.


out of curiosity - has this caused any actual bug? should be hard to
reproduce, I suppose...


> 
> Paolo Bonzini (7):
>   openpic_kvm: drop address_space_to_flatview call
>   memory: inline some performance-sensitive accessors
>   address_space_write: address_space_to_flatview needs RCU lock
>   address_space_read: address_space_to_flatview needs RCU lock
>   address_space_access_valid: address_space_to_flatview needs RCU lock
>   address_space_map: address_space_to_flatview needs RCU lock
>   address_space_rw: address_space_to_flatview needs RCU lock
> 
>  exec.c                         | 90 +++++++++++++++++++++++++-----------------
>  hw/intc/openpic_kvm.c          |  4 --
>  include/exec/memory-internal.h | 13 ++++--
>  include/exec/memory.h          | 47 ++++++++++++++--------
>  memory.c                       | 30 --------------
>  5 files changed, 93 insertions(+), 91 deletions(-)
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH 0/7] memory: address_space_to_flatview needs RCU lock

[Paolo Bonzini]

On 06/03/2018 08:47, Alexey Kardashevskiy wrote:
> On 05/03/18 19:36, Paolo Bonzini wrote:
>> I noticed that the introduction of flatview_{read,write} placed
>> address_space_to_flatview outside the RCU lock.  This is wrong and has
>> to be fixed, because address_space_to_flatview does an atomic_rcu_read.
>> These patches fix this one function at a time.
> 
> out of curiosity - has this caused any actual bug? should be hard to
> reproduce, I suppose...

No, I just noticed it by code inspection, while looking at a
reimplementation of MemoryRegionCache (reverted in 2.9).  One of the
differences between address_space_read and address_space_read_cached was
that the latter expects to be called within rcu_read_lock, so it was
surprising that address_space_read was expecting the same. :)

Paolo