On 23.07.21 21:34, Peter Xu wrote:
Topology update could be wrongly triggered in memory region finalize() if there's bug somewhere else. It'll be a very confusing stack when it happens (e.g., sending KVM ioctl within the RCU thread, and we'll observe it only until it fails!).Instead of that, we use the push()/pop() helper to avoid memory transaction commit, at the same time we use assertions to make sure there's no pending updates or it's a nested transaction, so it could fail even earlier and in a more explicit way. Suggested-by: Paolo Bonzini <pbonz...@redhat.com> Signed-off-by: Peter Xu <pet...@redhat.com> --- softmmu/memory.c | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/softmmu/memory.c b/softmmu/memory.c index 1a3e9ff8ad..dfce4a2bda 100644 --- a/softmmu/memory.c +++ b/softmmu/memory.c @@ -170,6 +170,12 @@ struct MemoryRegionIoeventfd { EventNotifier *e; };+/* Returns whether there's any pending memory updates */+static bool memory_region_has_pending_update(void) +{ + return memory_region_update_pending || ioeventfd_update_pending; +} + static bool memory_region_ioeventfd_before(MemoryRegionIoeventfd *a, MemoryRegionIoeventfd *b) { @@ -1756,12 +1762,25 @@ static void memory_region_finalize(Object *obj) * and cause an infinite loop. */ mr->enabled = false; - memory_region_transaction_begin(); + + /* + * Use push()/pop() instead of begin()/commit() to make sure below block + * won't trigger any topology update (which should never happen, but it's + * still a safety belt). + */
Hmm, I wonder if we can just keep the begin/end semantics and just do an assertion before doing the commit? Does anything speak against that?
-- Thanks, David / dhildenb
