* [Qemu-devel] [PATCH 0/7] memory: address_space_to_flatview needs RCU lock
@ 2018-03-05 8:36 Paolo Bonzini
2018-03-05 8:36 ` [Qemu-devel] [PATCH 1/7] openpic_kvm: drop address_space_to_flatview call Paolo Bonzini
` (7 more replies)
0 siblings, 8 replies; 19+ messages in thread
From: Paolo Bonzini @ 2018-03-05 8:36 UTC (permalink / raw)
To: qemu-devel; +Cc: David Gibson, Alexey Kardashevskiy
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH 1/7] openpic_kvm: drop address_space_to_flatview call
2018-03-05 8:36 [Qemu-devel] [PATCH 0/7] memory: address_space_to_flatview needs RCU lock Paolo Bonzini
@ 2018-03-05 8:36 ` Paolo Bonzini
2018-03-06 0:10 ` David Gibson
2018-03-06 7:46 ` Alexey Kardashevskiy
2018-03-05 8:36 ` [Qemu-devel] [PATCH 2/7] memory: inline some performance-sensitive accessors Paolo Bonzini
` (6 subsequent siblings)
7 siblings, 2 replies; 19+ messages in thread
From: Paolo Bonzini @ 2018-03-05 8:36 UTC (permalink / raw)
To: qemu-devel; +Cc: David Gibson, Alexey Kardashevskiy, qemu-stable
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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH 2/7] memory: inline some performance-sensitive accessors
2018-03-05 8:36 [Qemu-devel] [PATCH 0/7] memory: address_space_to_flatview needs RCU lock Paolo Bonzini
2018-03-05 8:36 ` [Qemu-devel] [PATCH 1/7] openpic_kvm: drop address_space_to_flatview call Paolo Bonzini
@ 2018-03-05 8:36 ` Paolo Bonzini
2018-03-06 7:46 ` Alexey Kardashevskiy
2018-03-05 8:36 ` [Qemu-devel] [PATCH 3/7] address_space_write: address_space_to_flatview needs RCU lock Paolo Bonzini
` (5 subsequent siblings)
7 siblings, 1 reply; 19+ messages in thread
From: Paolo Bonzini @ 2018-03-05 8:36 UTC (permalink / raw)
To: qemu-devel; +Cc: David Gibson, Alexey Kardashevskiy, qemu-stable
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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH 3/7] address_space_write: address_space_to_flatview needs RCU lock
2018-03-05 8:36 [Qemu-devel] [PATCH 0/7] memory: address_space_to_flatview needs RCU lock Paolo Bonzini
2018-03-05 8:36 ` [Qemu-devel] [PATCH 1/7] openpic_kvm: drop address_space_to_flatview call Paolo Bonzini
2018-03-05 8:36 ` [Qemu-devel] [PATCH 2/7] memory: inline some performance-sensitive accessors Paolo Bonzini
@ 2018-03-05 8:36 ` Paolo Bonzini
2018-03-06 7:46 ` Alexey Kardashevskiy
2018-03-05 8:36 ` [Qemu-devel] [PATCH 4/7] address_space_read: " Paolo Bonzini
` (4 subsequent siblings)
7 siblings, 1 reply; 19+ messages in thread
From: Paolo Bonzini @ 2018-03-05 8:36 UTC (permalink / raw)
To: qemu-devel; +Cc: David Gibson, Alexey Kardashevskiy, qemu-stable
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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH 4/7] address_space_read: address_space_to_flatview needs RCU lock
2018-03-05 8:36 [Qemu-devel] [PATCH 0/7] memory: address_space_to_flatview needs RCU lock Paolo Bonzini
` (2 preceding siblings ...)
2018-03-05 8:36 ` [Qemu-devel] [PATCH 3/7] address_space_write: address_space_to_flatview needs RCU lock Paolo Bonzini
@ 2018-03-05 8:36 ` Paolo Bonzini
2018-03-06 7:46 ` Alexey Kardashevskiy
2018-03-05 8:36 ` [Qemu-devel] [PATCH 5/7] address_space_access_valid: " Paolo Bonzini
` (3 subsequent siblings)
7 siblings, 1 reply; 19+ messages in thread
From: Paolo Bonzini @ 2018-03-05 8:36 UTC (permalink / raw)
To: qemu-devel; +Cc: David Gibson, Alexey Kardashevskiy, qemu-stable
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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH 5/7] address_space_access_valid: address_space_to_flatview needs RCU lock
2018-03-05 8:36 [Qemu-devel] [PATCH 0/7] memory: address_space_to_flatview needs RCU lock Paolo Bonzini
` (3 preceding siblings ...)
2018-03-05 8:36 ` [Qemu-devel] [PATCH 4/7] address_space_read: " Paolo Bonzini
@ 2018-03-05 8:36 ` Paolo Bonzini
2018-03-06 7:46 ` Alexey Kardashevskiy
2018-03-05 8:36 ` [Qemu-devel] [PATCH 6/7] address_space_map: " Paolo Bonzini
` (2 subsequent siblings)
7 siblings, 1 reply; 19+ messages in thread
From: Paolo Bonzini @ 2018-03-05 8:36 UTC (permalink / raw)
To: qemu-devel; +Cc: David Gibson, Alexey Kardashevskiy, qemu-stable
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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH 6/7] address_space_map: address_space_to_flatview needs RCU lock
2018-03-05 8:36 [Qemu-devel] [PATCH 0/7] memory: address_space_to_flatview needs RCU lock Paolo Bonzini
` (4 preceding siblings ...)
2018-03-05 8:36 ` [Qemu-devel] [PATCH 5/7] address_space_access_valid: " Paolo Bonzini
@ 2018-03-05 8:36 ` Paolo Bonzini
2018-03-06 7:46 ` Alexey Kardashevskiy
2018-03-05 8:36 ` [Qemu-devel] [PATCH 7/7] address_space_rw: " Paolo Bonzini
2018-03-06 7:47 ` [Qemu-devel] [PATCH 0/7] memory: " Alexey Kardashevskiy
7 siblings, 1 reply; 19+ messages in thread
From: Paolo Bonzini @ 2018-03-05 8:36 UTC (permalink / raw)
To: qemu-devel; +Cc: David Gibson, Alexey Kardashevskiy, qemu-stable
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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH 7/7] address_space_rw: address_space_to_flatview needs RCU lock
2018-03-05 8:36 [Qemu-devel] [PATCH 0/7] memory: address_space_to_flatview needs RCU lock Paolo Bonzini
` (5 preceding siblings ...)
2018-03-05 8:36 ` [Qemu-devel] [PATCH 6/7] address_space_map: " Paolo Bonzini
@ 2018-03-05 8:36 ` Paolo Bonzini
2018-03-06 7:47 ` Alexey Kardashevskiy
2018-03-06 7:47 ` [Qemu-devel] [PATCH 0/7] memory: " Alexey Kardashevskiy
7 siblings, 1 reply; 19+ messages in thread
From: Paolo Bonzini @ 2018-03-05 8:36 UTC (permalink / raw)
To: qemu-devel; +Cc: David Gibson, Alexey Kardashevskiy, qemu-stable
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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 1/7] openpic_kvm: drop address_space_to_flatview call
2018-03-05 8:36 ` [Qemu-devel] [PATCH 1/7] openpic_kvm: drop address_space_to_flatview call Paolo Bonzini
@ 2018-03-06 0:10 ` David Gibson
2018-03-06 12:25 ` Paolo Bonzini
2018-03-06 7:46 ` Alexey Kardashevskiy
1 sibling, 1 reply; 19+ messages in thread
From: David Gibson @ 2018-03-06 0:10 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, Alexey Kardashevskiy, qemu-stable
[-- 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 --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 1/7] openpic_kvm: drop address_space_to_flatview call
2018-03-05 8:36 ` [Qemu-devel] [PATCH 1/7] openpic_kvm: drop address_space_to_flatview call Paolo Bonzini
2018-03-06 0:10 ` David Gibson
@ 2018-03-06 7:46 ` Alexey Kardashevskiy
1 sibling, 0 replies; 19+ messages in thread
From: Alexey Kardashevskiy @ 2018-03-06 7:46 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: David Gibson, qemu-stable
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 2/7] memory: inline some performance-sensitive accessors
2018-03-05 8:36 ` [Qemu-devel] [PATCH 2/7] memory: inline some performance-sensitive accessors Paolo Bonzini
@ 2018-03-06 7:46 ` Alexey Kardashevskiy
0 siblings, 0 replies; 19+ messages in thread
From: Alexey Kardashevskiy @ 2018-03-06 7:46 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: David Gibson, qemu-stable
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 3/7] address_space_write: address_space_to_flatview needs RCU lock
2018-03-05 8:36 ` [Qemu-devel] [PATCH 3/7] address_space_write: address_space_to_flatview needs RCU lock Paolo Bonzini
@ 2018-03-06 7:46 ` Alexey Kardashevskiy
0 siblings, 0 replies; 19+ messages in thread
From: Alexey Kardashevskiy @ 2018-03-06 7:46 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: David Gibson, qemu-stable
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 4/7] address_space_read: address_space_to_flatview needs RCU lock
2018-03-05 8:36 ` [Qemu-devel] [PATCH 4/7] address_space_read: " Paolo Bonzini
@ 2018-03-06 7:46 ` Alexey Kardashevskiy
0 siblings, 0 replies; 19+ messages in thread
From: Alexey Kardashevskiy @ 2018-03-06 7:46 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: David Gibson, qemu-stable
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 5/7] address_space_access_valid: address_space_to_flatview needs RCU lock
2018-03-05 8:36 ` [Qemu-devel] [PATCH 5/7] address_space_access_valid: " Paolo Bonzini
@ 2018-03-06 7:46 ` Alexey Kardashevskiy
0 siblings, 0 replies; 19+ messages in thread
From: Alexey Kardashevskiy @ 2018-03-06 7:46 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: David Gibson, qemu-stable
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 6/7] address_space_map: address_space_to_flatview needs RCU lock
2018-03-05 8:36 ` [Qemu-devel] [PATCH 6/7] address_space_map: " Paolo Bonzini
@ 2018-03-06 7:46 ` Alexey Kardashevskiy
0 siblings, 0 replies; 19+ messages in thread
From: Alexey Kardashevskiy @ 2018-03-06 7:46 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: David Gibson, qemu-stable
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] address_space_rw: address_space_to_flatview needs RCU lock
2018-03-05 8:36 ` [Qemu-devel] [PATCH 7/7] address_space_rw: " Paolo Bonzini
@ 2018-03-06 7:47 ` Alexey Kardashevskiy
0 siblings, 0 replies; 19+ messages in thread
From: Alexey Kardashevskiy @ 2018-03-06 7:47 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: David Gibson, qemu-stable
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 0/7] memory: address_space_to_flatview needs RCU lock
2018-03-05 8:36 [Qemu-devel] [PATCH 0/7] memory: address_space_to_flatview needs RCU lock Paolo Bonzini
` (6 preceding siblings ...)
2018-03-05 8:36 ` [Qemu-devel] [PATCH 7/7] address_space_rw: " Paolo Bonzini
@ 2018-03-06 7:47 ` Alexey Kardashevskiy
2018-03-06 8:16 ` Paolo Bonzini
7 siblings, 1 reply; 19+ messages in thread
From: Alexey Kardashevskiy @ 2018-03-06 7:47 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: David Gibson
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 0/7] memory: address_space_to_flatview needs RCU lock
2018-03-06 7:47 ` [Qemu-devel] [PATCH 0/7] memory: " Alexey Kardashevskiy
@ 2018-03-06 8:16 ` Paolo Bonzini
0 siblings, 0 replies; 19+ messages in thread
From: Paolo Bonzini @ 2018-03-06 8:16 UTC (permalink / raw)
To: Alexey Kardashevskiy, qemu-devel; +Cc: David Gibson
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 1/7] openpic_kvm: drop address_space_to_flatview call
2018-03-06 0:10 ` David Gibson
@ 2018-03-06 12:25 ` Paolo Bonzini
0 siblings, 0 replies; 19+ messages in thread
From: Paolo Bonzini @ 2018-03-06 12:25 UTC (permalink / raw)
To: David Gibson; +Cc: qemu-devel, Alexey Kardashevskiy, qemu-stable
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;
>
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2018-03-06 12:25 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-05 8:36 [Qemu-devel] [PATCH 0/7] memory: address_space_to_flatview needs RCU lock Paolo Bonzini
2018-03-05 8:36 ` [Qemu-devel] [PATCH 1/7] openpic_kvm: drop address_space_to_flatview call Paolo Bonzini
2018-03-06 0:10 ` David Gibson
2018-03-06 12:25 ` Paolo Bonzini
2018-03-06 7:46 ` Alexey Kardashevskiy
2018-03-05 8:36 ` [Qemu-devel] [PATCH 2/7] memory: inline some performance-sensitive accessors Paolo Bonzini
2018-03-06 7:46 ` Alexey Kardashevskiy
2018-03-05 8:36 ` [Qemu-devel] [PATCH 3/7] address_space_write: address_space_to_flatview needs RCU lock Paolo Bonzini
2018-03-06 7:46 ` Alexey Kardashevskiy
2018-03-05 8:36 ` [Qemu-devel] [PATCH 4/7] address_space_read: " Paolo Bonzini
2018-03-06 7:46 ` Alexey Kardashevskiy
2018-03-05 8:36 ` [Qemu-devel] [PATCH 5/7] address_space_access_valid: " Paolo Bonzini
2018-03-06 7:46 ` Alexey Kardashevskiy
2018-03-05 8:36 ` [Qemu-devel] [PATCH 6/7] address_space_map: " Paolo Bonzini
2018-03-06 7:46 ` Alexey Kardashevskiy
2018-03-05 8:36 ` [Qemu-devel] [PATCH 7/7] address_space_rw: " Paolo Bonzini
2018-03-06 7:47 ` Alexey Kardashevskiy
2018-03-06 7:47 ` [Qemu-devel] [PATCH 0/7] memory: " Alexey Kardashevskiy
2018-03-06 8:16 ` Paolo Bonzini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.