Re: [Qemu-arm] [PATCH 11/16] memory: Add address_space_init_shareable()

[Edgar E. Iglesias]

On Thu, Nov 05, 2015 at 06:15:53PM +0000, Peter Maydell wrote:
> From: Peter Crosthwaite <address@hidden>
> 
> This will either create a new AS or return a pointer to an
> already existing equivalent one, if we have already created
> an AS for the specified root memory region.
> 
> The motivation is to reuse address spaces as much as possible.
> It's going to be quite common that bus masters out in device land
> have pointers to the same memory region for their mastering yet
> each will need to create its own address space. Let the memory
> API implement sharing for them.
> 
> Aside from the perf optimisations, this should reduce the amount
> of redundant output on info mtree as well.
> 
> Thee returned value will be malloced, but the malloc will be
> automatically freed when the AS runs out of refs.
> 
> Signed-off-by: Peter Crosthwaite <address@hidden>
> [PMM: dropped check for NULL root as unused; added doc-comment;
>  squashed Peter C's reference-counting patch into this one;
>  don't compare name string when deciding if we can share ASes;
>  read as->malloced before the unref of as->root to avoid possible
>  read-after-free if as->root was the owner of as]
> Signed-off-by: Peter Maydell <address@hidden>
> ---
>  include/exec/memory.h | 18 ++++++++++++++++++
>  memory.c              | 27 +++++++++++++++++++++++++++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index e5d98f8..1d1740a 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -236,6 +236,8 @@ struct AddressSpace {
>      struct rcu_head rcu;
>      char *name;
>      MemoryRegion *root;
> +    int ref_count;
> +    bool malloced;
>  
>      /* Accessed via RCU.  */
>      struct FlatView *current_map;
> @@ -1167,6 +1169,22 @@ MemTxResult memory_region_dispatch_write(MemoryRegion 
> *mr,
>   */
>  void address_space_init(AddressSpace *as, MemoryRegion *root, const char 
> *name);
>  
> +/**
> + * address_space_init_shareable: return an address space for a memory region,
> + *                               creating it if it does not already exist
> + *
> + * @root: a #MemoryRegion that routes addresses for the address space
> + * @name: an address space name.  The name is only used for debugging
> + *        output.
> + *
> + * This function will return a pointer to an existing AddressSpace
> + * which was initialized with the specified MemoryRegion, or it will
> + * create and initialize one if it does not already exist. The ASes
> + * are reference-counted, so the memory will be freed automatically
> + * when the AddressSpace is destroyed via address_space_destroy.
> + */
> +AddressSpace *address_space_init_shareable(MemoryRegion *root,
> +                                           const char *name);
>  
>  /**
>   * address_space_destroy: destroy an address space
> diff --git a/memory.c b/memory.c
> index c435c88..6c98a71 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -2101,7 +2101,9 @@ void address_space_init(AddressSpace *as, MemoryRegion 
> *root, const char *name)
>  {
>      memory_region_ref(root);
>      memory_region_transaction_begin();
> +    as->ref_count = 1;
>      as->root = root;
> +    as->malloced = false;
>      as->current_map = g_new(FlatView, 1);
>      flatview_init(as->current_map);
>      as->ioeventfd_nb = 0;
> @@ -2116,6 +2118,7 @@ void address_space_init(AddressSpace *as, MemoryRegion 
> *root, const char *name)
>  static void do_address_space_destroy(AddressSpace *as)
>  {
>      MemoryListener *listener;
> +    bool do_free = as->malloced;
>  
>      address_space_destroy_dispatch(as);
>  
> @@ -2127,12 +2130,36 @@ static void do_address_space_destroy(AddressSpace *as)
>      g_free(as->name);
>      g_free(as->ioeventfds);
>      memory_region_unref(as->root);
> +    if (do_free) {
> +        g_free(as);
> +    }
> +}
> +
> +AddressSpace *address_space_init_shareable(MemoryRegion *root, const char 
> *name)
> +{
> +    AddressSpace *as;
> +
> +    QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
> +        if (root == as->root) {
> +            as->ref_count++;
> +            return as;
> +        }
> +    }
> +
> +    as = g_malloc0(sizeof *as);
> +    address_space_init(as, root, name);

Nit-pick but IIUC, address_space_init does not need AS to be zeroed
so this could be changed to a g_malloc().

either-way:

Reviewed-by: Edgar E. Iglesias <address@hidden>



> +    as->malloced = true;
> +    return as;
>  }
>  
>  void address_space_destroy(AddressSpace *as)
>  {
>      MemoryRegion *root = as->root;
>  
> +    as->ref_count--;
> +    if (as->ref_count) {
> +        return;
> +    }
>      /* Flush out anything from MemoryListeners listening in on this */
>      memory_region_transaction_begin();
>      as->root = NULL;
> -- 
> 1.9.1
>