Re: [PATCH 09/21] hw: Let memory_region_allocate_system_memory take MachineState argument

[Thomas Huth <th.huth@posteo.de>]

On 21/10/2019 00.56, Philippe Mathieu-Daudé wrote:
> All the codebase calls memory_region_allocate_system_memory() with
> a NULL 'owner' from the board_init() function.

It's a little bit weird that you first changed the owner to NULL in
patch 7, just to change the type of the parameter here and then change
the parameter back to the machine afterwards. I think I'd rather squash
pash 7 (and the follow-up patches like 14) into this one here - it's
just four files that need to be adapted, so I think that's still fine,
and finally that's less churn to be reviewed.

> Let pass a MachineState argument, and enforce the QOM ownership of
> the system memory.

BTW, good idea!

> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/core/numa.c      | 11 +++++++----
>  include/hw/boards.h |  6 ++++--
>  2 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/core/numa.c b/hw/core/numa.c
> index 038c96d4ab..2e29e4bfe0 100644
> --- a/hw/core/numa.c
> +++ b/hw/core/numa.c
> @@ -520,21 +520,24 @@ static void allocate_system_memory_nonnuma(MemoryRegion *mr, Object *owner,
>      vmstate_register_ram_global(mr);
>  }
>  
> -void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner,
> +void memory_region_allocate_system_memory(MemoryRegion *mr, MachineState *ms,
>                                            const char *name,
>                                            uint64_t ram_size)
>  {
>      uint64_t addr = 0;
>      int i;
> -    MachineState *ms = MACHINE(qdev_get_machine());
> +
> +    if (!ms) {
> +        ms = MACHINE(qdev_get_machine());
> +    }
>  
>      if (ms->numa_state == NULL ||
>          ms->numa_state->num_nodes == 0 || !have_memdevs) {
> -        allocate_system_memory_nonnuma(mr, owner, name, ram_size);
> +        allocate_system_memory_nonnuma(mr, OBJECT(ms), name, ram_size);
>          return;
>      }
>  
> -    memory_region_init(mr, owner, name, ram_size);
> +    memory_region_init(mr, OBJECT(ms), name, ram_size);
>      for (i = 0; i < ms->numa_state->num_nodes; i++) {
>          uint64_t size = ms->numa_state->nodes[i].node_mem;
>          HostMemoryBackend *backend = ms->numa_state->nodes[i].node_memdev;
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index de45087f34..3b6cb82b6c 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -14,7 +14,7 @@
>  /**
>   * memory_region_allocate_system_memory - Allocate a board's main memory
>   * @mr: the #MemoryRegion to be initialized
> - * @owner: the object that tracks the region's reference count
> + * @ms: the #MachineState object that own the system memory

s/own/owns/

>   * @name: name of the memory region
>   * @ram_size: size of the region in bytes
>   *
> @@ -38,8 +38,10 @@
>   * Smaller pieces of memory (display RAM, static RAMs, etc) don't need
>   * to be backed via the -mem-path memory backend and can simply
>   * be created via memory_region_init_ram().
> + *
> + * The #MachineState object will track the region's reference count.
>   */
> -void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner,
> +void memory_region_allocate_system_memory(MemoryRegion *mr, MachineState *ms,
>                                            const char *name,
>                                            uint64_t ram_size);
>  
> 

 Thomas