Re: [Qemu-devel] [PATCH 04/15] memory: MemoryRegion topology must be stable when updating

[Marcelo Tosatti <mtosatti@redhat.com>]

On Thu, Aug 09, 2012 at 03:28:44PM +0800, liu ping fan wrote:
> On Wed, Aug 8, 2012 at 5:13 PM, Avi Kivity <avi@redhat.com> wrote:
> > On 08/08/2012 09:25 AM, Liu Ping Fan wrote:
> >> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> >>
> >> Using mem_map_lock to protect among updaters. So we can get the intact
> >> snapshot of mem topology -- FlatView & radix-tree.
> >>
> >> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> >> ---
> >>  exec.c   |    3 +++
> >>  memory.c |   22 ++++++++++++++++++++++
> >>  memory.h |    2 ++
> >>  3 files changed, 27 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/exec.c b/exec.c
> >> index 8244d54..0e29ef9 100644
> >> --- a/exec.c
> >> +++ b/exec.c
> >> @@ -210,6 +210,8 @@ static unsigned phys_map_nodes_nb, phys_map_nodes_nb_alloc;
> >>     The bottom level has pointers to MemoryRegionSections.  */
> >>  static PhysPageEntry phys_map = { .ptr = PHYS_MAP_NODE_NIL, .is_leaf = 0 };
> >>
> >> +QemuMutex mem_map_lock;
> >> +
> >>  static void io_mem_init(void);
> >>  static void memory_map_init(void);
> >>
> >> @@ -637,6 +639,7 @@ void cpu_exec_init_all(void)
> >>  #if !defined(CONFIG_USER_ONLY)
> >>      memory_map_init();
> >>      io_mem_init();
> >> +    qemu_mutex_init(&mem_map_lock);
> >>  #endif
> >>  }
> >>
> >> diff --git a/memory.c b/memory.c
> >> index aab4a31..5986532 100644
> >> --- a/memory.c
> >> +++ b/memory.c
> >> @@ -761,7 +761,9 @@ void memory_region_transaction_commit(void)
> >>      assert(memory_region_transaction_depth);
> >>      --memory_region_transaction_depth;
> >>      if (!memory_region_transaction_depth && memory_region_update_pending) {
> >> +        qemu_mutex_lock(&mem_map_lock);
> >>          memory_region_update_topology(NULL);
> >> +        qemu_mutex_unlock(&mem_map_lock);
> >>      }
> >>  }
> >
> > Seems to me that nothing in memory.c can susceptible to races.  It must
> > already be called under the big qemu lock, and with the exception of
> > mutators (memory_region_set_*), changes aren't directly visible.
> >
> Yes, what I want to do is "prepare unplug out of protection of global
> lock".  When io-dispatch and mmio-dispatch are all out of big lock, we
> will run into the following scene:
>     In vcpu context A, qdev_unplug_complete()-> delete subregion;
>     In context B, write pci bar --> pci mapping update    -> add subregion

Per device lock should protect that.