As we are going to share FlatView's between AddressSpace's,
and AddressSpaceDispatch is a structure to perform quick lookup
in FlatView, this moves ASD to FlatView.
After previosly open coded ASD rendering, we can also remove
as->next_dispatch as the new FlatView pointer is stored
on a stack and set to an AS atomically.
flatview_destroy() is executed under RCU instead of
address_space_dispatch_free() now.
This makes mem_begin/mem_commit to work with ASD and mem_add with FV
as later on mem_add will be taking FV as an argument anyway.
This should cause no behavioural change.
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Changes:
v4:
* since FlatView::rcu is used now to dispose FV, call_rcu() in
address_space_update_topology() is replaced with direct call to
flatview_unref()
---
include/exec/memory-internal.h | 12 +++++++-----
include/exec/memory.h | 2 --
exec.c | 41 +++++++++++------------------------------
memory.c | 31 ++++++++++++++++++++++++-------
4 files changed, 42 insertions(+), 44 deletions(-)
diff --git a/include/exec/memory-internal.h b/include/exec/memory-internal.h
index 9abde2f11c..6e08eda256 100644
--- a/include/exec/memory-internal.h
+++ b/include/exec/memory-internal.h
@@ -22,16 +22,18 @@
#ifndef CONFIG_USER_ONLY
typedef struct AddressSpaceDispatch AddressSpaceDispatch;
-void address_space_destroy_dispatch(AddressSpace *as);
-
extern const MemoryRegionOps unassigned_mem_ops;
bool memory_region_access_valid(MemoryRegion *mr, hwaddr addr,
unsigned size, bool is_write);
-void mem_add(AddressSpace *as, MemoryRegionSection *section);
-void mem_begin(AddressSpace *as);
-void mem_commit(AddressSpace *as);
+void mem_add(AddressSpace *as, FlatView *fv, MemoryRegionSection *section);
+AddressSpaceDispatch *mem_begin(AddressSpace *as);
+void mem_commit(AddressSpaceDispatch *d);
+
+AddressSpaceDispatch *address_space_to_dispatch(AddressSpace *as);
+AddressSpaceDispatch *flatview_to_dispatch(FlatView *fv);
+void address_space_dispatch_free(AddressSpaceDispatch *d);
#endif
#endif
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 9581f7a7db..2346f8b863 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -316,8 +316,6 @@ struct AddressSpace {
int ioeventfd_nb;
struct MemoryRegionIoeventfd *ioeventfds;
- struct AddressSpaceDispatch *dispatch;
- struct AddressSpaceDispatch *next_dispatch;
QTAILQ_HEAD(memory_listeners_as, MemoryListener) listeners;
QTAILQ_ENTRY(AddressSpace) address_spaces_link;
};
diff --git a/exec.c b/exec.c
index 1626d254bb..afd64127e6 100644
--- a/exec.c
+++ b/exec.c
@@ -187,8 +187,6 @@ typedef struct PhysPageMap {
} PhysPageMap;
struct AddressSpaceDispatch {
- struct rcu_head rcu;
-
MemoryRegionSection *mru_section;
/* This is a multi-level map on the physical address space.
* The bottom level has pointers to MemoryRegionSections.
@@ -485,7 +483,7 @@ static MemoryRegionSection address_space_do_translate(AddressSpace *as,
IOMMUMemoryRegionClass *imrc;
for (;;) {
- AddressSpaceDispatch *d = atomic_rcu_read(&as->dispatch);
+ AddressSpaceDispatch *d = address_space_to_dispatch(as);
section = address_space_translate_internal(d, addr, &addr, plen, is_mmio);
iommu_mr = memory_region_get_iommu(section->mr);
@@ -1222,7 +1220,7 @@ hwaddr memory_region_section_get_iotlb(CPUState *cpu,
} else {
AddressSpaceDispatch *d;
- d = atomic_rcu_read(§ion->address_space->dispatch);
+ d = address_space_to_dispatch(section->address_space);
iotlb = section - d->map.sections;
iotlb += xlat;
}
@@ -1347,9 +1345,9 @@ static void register_multipage(AddressSpaceDispatch *d,
phys_page_set(d, start_addr >> TARGET_PAGE_BITS, num_pages, section_index);
}
-void mem_add(AddressSpace *as, MemoryRegionSection *section)
+void mem_add(AddressSpace *as, FlatView *fv, MemoryRegionSection *section)
{
- AddressSpaceDispatch *d = as->next_dispatch;
+ AddressSpaceDispatch *d = flatview_to_dispatch(fv);
MemoryRegionSection now = *section, remain = *section;
Int128 page_size = int128_make64(TARGET_PAGE_SIZE);
@@ -2672,7 +2670,7 @@ static void io_mem_init(void)
NULL, UINT64_MAX);
}
-void mem_begin(AddressSpace *as)
+AddressSpaceDispatch *mem_begin(AddressSpace *as)
{
AddressSpaceDispatch *d = g_new0(AddressSpaceDispatch, 1);
uint16_t n;
@@ -2688,26 +2686,19 @@ void mem_begin(AddressSpace *as)
d->phys_map = (PhysPageEntry) { .ptr = PHYS_MAP_NODE_NIL, .skip = 1 };
d->as = as;
- as->next_dispatch = d;
+
+ return d;
}
-static void address_space_dispatch_free(AddressSpaceDispatch *d)
+void address_space_dispatch_free(AddressSpaceDispatch *d)
{
phys_sections_free(&d->map);
g_free(d);
}
-void mem_commit(AddressSpace *as)
+void mem_commit(AddressSpaceDispatch *d)
{
- AddressSpaceDispatch *cur = as->dispatch;
- AddressSpaceDispatch *next = as->next_dispatch;
-
- phys_page_compact_all(next, next->map.nodes_nb);
-
- atomic_rcu_set(&as->dispatch, next);
- if (cur) {
- call_rcu(cur, address_space_dispatch_free, rcu);
- }
+ phys_page_compact_all(d, d->map.nodes_nb);
}
static void tcg_commit(MemoryListener *listener)
@@ -2723,21 +2714,11 @@ static void tcg_commit(MemoryListener *listener)
* We reload the dispatch pointer now because cpu_reloading_memory_map()
* may have split the RCU critical section.
*/
- d = atomic_rcu_read(&cpuas->as->dispatch);
+ d = address_space_to_dispatch(cpuas->as);
atomic_rcu_set(&cpuas->memory_dispatch, d);
tlb_flush(cpuas->cpu);
}
-void address_space_destroy_dispatch(AddressSpace *as)
-{
- AddressSpaceDispatch *d = as->dispatch;
-
- atomic_rcu_set(&as->dispatch, NULL);
- if (d) {
- call_rcu(d, address_space_dispatch_free, rcu);
- }
-}
-
static void memory_map_init(void)
{
system_memory = g_malloc(sizeof(*system_memory));
diff --git a/memory.c b/memory.c
index 9babd7d423..27d7aeffc2 100644
--- a/memory.c
+++ b/memory.c
@@ -229,6 +229,7 @@ struct FlatView {
FlatRange *ranges;
unsigned nr;
unsigned nr_allocated;
+ struct AddressSpaceDispatch *dispatch;
};
typedef struct AddressSpaceOps AddressSpaceOps;
@@ -289,6 +290,9 @@ static void flatview_destroy(FlatView *view)
{
int i;
+ if (view->dispatch) {
+ address_space_dispatch_free(view->dispatch);
+ }
for (i = 0; i < view->nr; i++) {
memory_region_unref(view->ranges[i].mr);
}
@@ -304,10 +308,25 @@ static void flatview_ref(FlatView *view)
static void flatview_unref(FlatView *view)
{
if (atomic_fetch_dec(&view->ref) == 1) {
- flatview_destroy(view);
+ call_rcu(view, flatview_destroy, rcu);
}
}
+static 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)
@@ -886,13 +905,13 @@ static void address_space_update_topology(AddressSpace *as)
FlatView *new_view = generate_memory_topology(as->root);
int i;
- mem_begin(as);
+ new_view->dispatch = mem_begin(as);
for (i = 0; i < new_view->nr; i++) {
MemoryRegionSection mrs =
section_from_flat_range(&new_view->ranges[i], as);
- mem_add(as, &mrs);
+ mem_add(as, new_view, &mrs);
}
- mem_commit(as);
+ mem_commit(new_view->dispatch);
if (!QTAILQ_EMPTY(&as->listeners)) {
address_space_update_topology_pass(as, old_view, new_view, false);
@@ -901,7 +920,7 @@ static void address_space_update_topology(AddressSpace *as)
/* Writes are protected by the BQL. */
atomic_rcu_set(&as->current_map, new_view);
- call_rcu(old_view, flatview_unref, rcu);
+ flatview_unref(old_view);
/* Note that all the old MemoryRegions are still alive up to this
* point. This relieves most MemoryListeners from the need to
@@ -2631,7 +2650,6 @@ void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name)
QTAILQ_INIT(&as->listeners);
QTAILQ_INSERT_TAIL(&address_spaces, as, address_spaces_link);
as->name = g_strdup(name ? name : "anonymous");
- as->dispatch = NULL;
memory_region_update_pending |= root->enabled;
memory_region_transaction_commit();
}
@@ -2640,7 +2658,6 @@ static void do_address_space_destroy(AddressSpace *as)
{
bool do_free = as->malloced;
- address_space_destroy_dispatch(as);
assert(QTAILQ_EMPTY(&as->listeners));
flatview_unref(as->current_map);
--
2.11.0
On 21/09/2017 10:50, Alexey Kardashevskiy wrote:
> * since FlatView::rcu is used now to dispose FV, call_rcu() in
> address_space_update_topology() is replaced with direct call to
> flatview_unref()
Hmm, this is not correct, as you could have
thread 1 thread 2 RCU thread
-------------------------------------------------------------
rcu_read_lock
read as->current_map
set as->current_map
flatview_unref
'--> call_rcu
flatview_ref
rcu_read_unlock
flatview_destroy
I need to think a bit more about this (and possibly ask Paul...).
Paolo
On 21/09/17 21:51, Paolo Bonzini wrote:
> On 21/09/2017 10:50, Alexey Kardashevskiy wrote:
>> * since FlatView::rcu is used now to dispose FV, call_rcu() in
>> address_space_update_topology() is replaced with direct call to
>> flatview_unref()
>
> Hmm, this is not correct, as you could have
>
>
> thread 1 thread 2 RCU thread
> -------------------------------------------------------------
> rcu_read_lock
> read as->current_map
> set as->current_map
> flatview_unref
> '--> call_rcu
> flatview_ref
> rcu_read_unlock
> flatview_destroy
>
> I need to think a bit more about this (and possibly ask Paul...).
>
> Paolo
>
Nah, you're right, it should be like this:
diff --git a/memory.c b/memory.c
index 35b2fc5f7f..689bf53866 100644
--- a/memory.c
+++ b/memory.c
@@ -317,7 +317,7 @@ static void flatview_ref(FlatView *view)
static void flatview_unref(FlatView *view)
{
if (atomic_fetch_dec(&view->ref) == 1) {
- call_rcu(view, flatview_destroy, rcu);
+ flatview_destroy(view);
}
}
@@ -768,7 +768,7 @@ static FlatView *generate_memory_topology(MemoryRegion *mr)
flatview_simplify(view);
if (!view->nr) {
- flatview_destroy(view);
+ flatview_unref(view);
use_empty = true;
}
}
@@ -1026,7 +1026,7 @@ static void address_space_set_flatview(AddressSpace *as)
/* Writes are protected by the BQL. */
atomic_rcu_set(&as->current_map, new_view);
if (old_view) {
- flatview_unref(old_view);
+ call_rcu(view, flatview_unref, rcu);
}
--
Alexey
On 21/09/2017 15:44, Alexey Kardashevskiy wrote:
> On 21/09/17 21:51, Paolo Bonzini wrote:
>> On 21/09/2017 10:50, Alexey Kardashevskiy wrote:
>>> * since FlatView::rcu is used now to dispose FV, call_rcu() in
>>> address_space_update_topology() is replaced with direct call to
>>> flatview_unref()
>>
>> Hmm, this is not correct, as you could have
>>
>>
>> thread 1 thread 2 RCU thread
>> -------------------------------------------------------------
>> rcu_read_lock
>> read as->current_map
>> set as->current_map
>> flatview_unref
>> '--> call_rcu
>> flatview_ref
>> rcu_read_unlock
>> flatview_destroy
>>
>> I need to think a bit more about this (and possibly ask Paul...).
>>
>> Paolo
>>
>
> Nah, you're right, it should be like this:
>
>
> diff --git a/memory.c b/memory.c
> index 35b2fc5f7f..689bf53866 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -317,7 +317,7 @@ static void flatview_ref(FlatView *view)
> static void flatview_unref(FlatView *view)
> {
> if (atomic_fetch_dec(&view->ref) == 1) {
> - call_rcu(view, flatview_destroy, rcu);
> + flatview_destroy(view);
> }
> }
>
> @@ -768,7 +768,7 @@ static FlatView *generate_memory_topology(MemoryRegion *mr)
> flatview_simplify(view);
>
> if (!view->nr) {
> - flatview_destroy(view);
> + flatview_unref(view);
> use_empty = true;
> }
> }
> @@ -1026,7 +1026,7 @@ static void address_space_set_flatview(AddressSpace *as)
> /* Writes are protected by the BQL. */
> atomic_rcu_set(&as->current_map, new_view);
> if (old_view) {
> - flatview_unref(old_view);
> + call_rcu(view, flatview_unref, rcu);
> }
This still doesn't cover address_space_get_flatview, i.e. it is a
pre-existing bug.
I found a similar case in Linux, here is how they solved it:
commit 358136532dd29e9ed96e0e523d2d510e71bda003
Author: Paolo Bonzini <pbonzini@redhat.com>
Date: Thu Sep 21 14:32:47 2017 +0200
memory: avoid "resurrection" of dead FlatViews
It's possible for address_space_get_flatview() as it currently stands
to cause a use-after-free for the returned FlatView, if the reference
count is incremented after the FlatView has been replaced by a writer:
thread 1 thread 2 RCU thread
-------------------------------------------------------------
rcu_read_lock
read as->current_map
set as->current_map
flatview_unref
'--> call_rcu
flatview_ref
[ref=1]
rcu_read_unlock
flatview_destroy
<badness>
Since FlatViews are not updated very often, we can just detect the
situation using a new atomic op atomic_fetch_inc_nonzero, similar to
Linux's atomic_inc_not_zero, which performs the refcount increment only if
it hasn't already hit zero. This is similar to Linux commit de09a9771a53
("CRED: Fix get_task_cred() and task_state() to not resurrect dead
credentials", 2010-07-29).
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
diff --git a/docs/devel/atomics.txt b/docs/devel/atomics.txt
index 048e5f23cb..10c5fa37e8 100644
--- a/docs/devel/atomics.txt
+++ b/docs/devel/atomics.txt
@@ -64,6 +64,7 @@ operations:
typeof(*ptr) atomic_fetch_and(ptr, val)
typeof(*ptr) atomic_fetch_or(ptr, val)
typeof(*ptr) atomic_fetch_xor(ptr, val)
+ typeof(*ptr) atomic_fetch_inc_nonzero(ptr)
typeof(*ptr) atomic_xchg(ptr, val)
typeof(*ptr) atomic_cmpxchg(ptr, old, new)
diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h
index b6b62fb771..44ad1e6c32 100644
--- a/include/qemu/atomic.h
+++ b/include/qemu/atomic.h
@@ -197,6 +197,15 @@
atomic_cmpxchg__nocheck(ptr, old, new); \
})
+#define atomic_fetch_inc_nonzero(ptr) ({ \
+ QEMU_BUILD_BUG_ON(sizeof(*ptr) > ATOMIC_REG_SIZE); \
+ typeof_strip_qual(*ptr) _oldn = atomic_read(ptr); \
+ while (_oldn && atomic_cmpxchg(ptr, _oldn, _oldn + 1) != _oldn) { \
+ _oldn = atomic_read(ptr); \
+ } \
+ _oldn; \
+})
+
/* Provide shorter names for GCC atomic builtins, return old value */
#define atomic_fetch_inc(ptr) __atomic_fetch_add(ptr, 1, __ATOMIC_SEQ_CST)
#define atomic_fetch_dec(ptr) __atomic_fetch_sub(ptr, 1, __ATOMIC_SEQ_CST)
diff --git a/memory.c b/memory.c
index 2b90117c60..51f54ab430 100644
--- a/memory.c
+++ b/memory.c
@@ -294,9 +294,9 @@ static void flatview_destroy(FlatView *view)
g_free(view);
}
-static void flatview_ref(FlatView *view)
+static bool flatview_ref(FlatView *view)
{
- atomic_inc(&view->ref);
+ return atomic_fetch_inc_nonzero(&view->ref) > 0;
}
static void flatview_unref(FlatView *view)
@@ -773,8 +773,12 @@ static FlatView *address_space_get_flatview(AddressSpace *as)
FlatView *view;
rcu_read_lock();
- view = atomic_rcu_read(&as->current_map);
- flatview_ref(view);
+ do {
+ view = atomic_rcu_read(&as->current_map);
+ /* If somebody has replaced as->current_map concurrently,
+ * flatview_ref returns false.
+ */
+ } while (!flatview_ref(view));
rcu_read_unlock();
return view;
}
© 2016 - 2025 Red Hat, Inc.