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
>