Re: [Qemu-devel] [PATCH] memory: unregister AddressSpace MemoryListener within BQL

[Alex Williamson <alex.williamson@redhat.com>]

On Tue, 2015-02-10 at 13:52 +0100, Paolo Bonzini wrote:
> address_space_destroy_dispatch is called from an RCU callback and hence
> outside the iothread mutex (BQL).  However, after address_space_destroy
> no new accesses can hit the destroyed AddressSpace so it is not necessary
> to observe changes to the memory map.  Move the memory_listener_unregister
> call earlier, to make it thread-safe again.
> 
> Reported-by: Alex Williamson <alex.williamson@redhat.com>
> Fixes: 374f2981d1f10bc4307f250f24b2a7ddb9b14be0
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  exec.c                         | 6 +++++-
>  include/exec/memory-internal.h | 1 +
>  memory.c                       | 1 +
>  3 files changed, 7 insertions(+), 1 deletion(-)

Seems to fix it, Thanks!

Tested-by: Alex Williamson <alex.williamson@redhat.com>


> diff --git a/exec.c b/exec.c
> index 6b79ad1..6dff7bc 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2059,11 +2059,15 @@ void address_space_init_dispatch(AddressSpace *as)
>      memory_listener_register(&as->dispatch_listener, as);
>  }
>  
> +void address_space_unregister(AddressSpace *as)
> +{
> +    memory_listener_unregister(&as->dispatch_listener);
> +}
> +
>  void address_space_destroy_dispatch(AddressSpace *as)
>  {
>      AddressSpaceDispatch *d = as->dispatch;
>  
> -    memory_listener_unregister(&as->dispatch_listener);
>      g_free(d);
>      as->dispatch = NULL;
>  }
> diff --git a/include/exec/memory-internal.h b/include/exec/memory-internal.h
> index 25c43c0..fb467ac 100644
> --- a/include/exec/memory-internal.h
> +++ b/include/exec/memory-internal.h
> @@ -23,6 +23,7 @@
>  typedef struct AddressSpaceDispatch AddressSpaceDispatch;
>  
>  void address_space_init_dispatch(AddressSpace *as);
> +void address_space_unregister(AddressSpace *as);
>  void address_space_destroy_dispatch(AddressSpace *as);
>  
>  extern const MemoryRegionOps unassigned_mem_ops;
> diff --git a/memory.c b/memory.c
> index 9b91243..130152c 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1978,6 +1978,7 @@ void address_space_destroy(AddressSpace *as)
>      as->root = NULL;
>      memory_region_transaction_commit();
>      QTAILQ_REMOVE(&address_spaces, as, address_spaces_link);
> +    address_space_unregister(as);
>  
>      /* At this point, as->dispatch and as->current_map are dummy
>       * entries that the guest should never use.  Wait for the old