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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

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
>