[09/21] hw: Let memory_region_allocate_system_memory take MachineState argument
diff mbox series

Message ID 20191020225650.3671-10-philmd@redhat.com
State New
Headers show
Series
  • hw: Let the machine be the owner of the system memory
Related show

Commit Message

Philippe Mathieu-Daudé Oct. 20, 2019, 10:56 p.m. UTC
All the codebase calls memory_region_allocate_system_memory() with
a NULL 'owner' from the board_init() function.
Let pass a MachineState argument, and enforce the QOM ownership of
the system memory.

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(-)

Comments

Thomas Huth Oct. 21, 2019, 7:27 a.m. UTC | #1
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
Philippe Mathieu-Daudé Oct. 21, 2019, 8:32 a.m. UTC | #2
On 10/21/19 9:27 AM, Thomas Huth wrote:
> 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.

I haven't thought of it and like your suggestion :)

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

Thanks :)

> 
>> 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
>
Alistair Francis Oct. 21, 2019, 11:19 p.m. UTC | #3
On Sun, Oct 20, 2019 at 4:12 PM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> All the codebase calls memory_region_allocate_system_memory() with
> a NULL 'owner' from the board_init() function.
> Let pass a MachineState argument, and enforce the QOM ownership of
> the system memory.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  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
>   * @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);
>
> --
> 2.21.0
>
>

Patch
diff mbox series

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
  * @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);