| Message ID | 20191020225650.3671-10-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw: Let the machine be the owner of the system memory | expand |
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
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 >
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 > >
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);
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(-)