[Qemu-devel] [RFC] memory: use memory_region_init_ram() instead of memory_region_allocate_system_memory()

[Qemu-devel] [RFC] memory: use memory_region_init_ram() instead of memory_region_allocate_system_memory()

[Igor Mammedov]

I'm considering to deprecating -mem-path/prealloc CLI options and replacing
them with a single memdev Machine property to allow interested users to pick
used backend for initial RAM (fixes mixed -mem-path+hostmem backends issues)
and as a transition step to modeling initial as a Device instead of (ab)using
MemoryRegion APIs.

Currently most boards use memory_region_allocate_system_memory() to allocate
RAM and the interface tied up too much to MemoryRegion that makes changing API
to hostmem across tree is quite a bit of work sometimes requiring logic rewrite
to do it cleanly and I'm not sure it's worth the effort and that it really has
a merit to do in case of TCG only boards.

I suggest to get rid memory_region_allocate_system_memory() on TCG only boards
since most of them don't really need NUMA features provided by this API and/or
probably won't noticeably benefit from -mem-path/prealloc backed memory if at all
and replacing memory allocation with plain memory_region_init_ram() API.

It won't require extensive changes in TCG only boards we have now, since they would
continue to use MemoryRegion based API approach. And the boards, that really need
to use hugepages or numa features, could be amended to use new machine memdev
property and/or initial RAM being allocated by implictly (-m) using hostmem-ram backend.

Also some boards (ab)use memory_region_allocate_system_memory(), calling it several
times to allocate various fixed sized chunks of RAM and ROMs, which is problematic
to map to a single initial RAM Machine::memdev backend and is currently broken if
-mem-path points to a not hugepage pool.

This RFC attempts to cleanup things a bit on TCG only boards (only several ones)
side and test waters if it's acceptable approach.  If it looks acceptable, I'll send
a proper series to make usage of memory_region_allocate_system_memory() minimal across
the codebase and then convert boards that actually use numa/hugepages to initial RAM
memdev model (arm/virt, spapr, s390x, pc/q35).

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/arm/musicpal.c    |  4 ++--
 hw/arm/vexpress.c    |  4 ++--
 hw/sparc64/niagara.c | 30 +++++++++++++++---------------
 3 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
index d22532a11c..197e7d1282 100644
--- a/hw/arm/musicpal.c
+++ b/hw/arm/musicpal.c
@@ -1592,8 +1592,8 @@ static void musicpal_init(MachineState *machine)
     cpu = ARM_CPU(cpu_create(machine->cpu_type));
 
     /* For now we use a fixed - the original - RAM size */
-    memory_region_allocate_system_memory(ram, NULL, "musicpal.ram",
-                                         MP_RAM_DEFAULT_SIZE);
+    memory_region_init_ram(ram, NULL, "musicpal.ram", MP_RAM_DEFAULT_SIZE,
+                           &error_fatal);
     memory_region_add_subregion(address_space_mem, 0, ram);
 
     memory_region_init_ram(sram, NULL, "musicpal.sram", MP_SRAM_SIZE,
diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
index c02d18ee61..a2cf5c0af2 100644
--- a/hw/arm/vexpress.c
+++ b/hw/arm/vexpress.c
@@ -280,8 +280,8 @@ static void a9_daughterboard_init(const VexpressMachineState *vms,
         exit(1);
     }
 
-    memory_region_allocate_system_memory(ram, NULL, "vexpress.highmem",
-                                         ram_size);
+    memory_region_init_ram(ram, NULL, "vexpress.highmem", ram_size,
+                           &error_fatal);
     low_ram_size = ram_size;
     if (low_ram_size > 0x4000000) {
         low_ram_size = 0x4000000;
diff --git a/hw/sparc64/niagara.c b/hw/sparc64/niagara.c
index f8a856f611..62e0348d5f 100644
--- a/hw/sparc64/niagara.c
+++ b/hw/sparc64/niagara.c
@@ -37,6 +37,7 @@
 #include "sysemu/block-backend.h"
 #include "qemu/error-report.h"
 #include "sysemu/qtest.h"
+#include "qapi/error.h"
 
 
 typedef struct NiagaraBoardState {
@@ -108,27 +109,26 @@ static void niagara_init(MachineState *machine)
     /* init CPUs */
     sparc64_cpu_devinit(machine->cpu_type, NIAGARA_PROM_BASE);
     /* set up devices */
-    memory_region_allocate_system_memory(&s->hv_ram, NULL, "sun4v-hv.ram",
-                                         NIAGARA_HV_RAM_SIZE);
+    memory_region_init_ram(&s->hv_ram, NULL, "sun4v-hv.ram",
+                                         NIAGARA_HV_RAM_SIZE, &error_fatal);
     memory_region_add_subregion(sysmem, NIAGARA_HV_RAM_BASE, &s->hv_ram);
 
-    memory_region_allocate_system_memory(&s->partition_ram, NULL,
-                                         "sun4v-partition.ram",
-                                         machine->ram_size);
+    memory_region_init_ram(&s->partition_ram, NULL, "sun4v-partition.ram",
+                                         machine->ram_size, &error_fatal);
     memory_region_add_subregion(sysmem, NIAGARA_PARTITION_RAM_BASE,
                                 &s->partition_ram);
 
-    memory_region_allocate_system_memory(&s->nvram, NULL,
-                                         "sun4v.nvram", NIAGARA_NVRAM_SIZE);
+    memory_region_init_ram(&s->nvram, NULL, "sun4v.nvram", NIAGARA_NVRAM_SIZE,
+                           &error_fatal);
     memory_region_add_subregion(sysmem, NIAGARA_NVRAM_BASE, &s->nvram);
-    memory_region_allocate_system_memory(&s->md_rom, NULL,
-                                         "sun4v-md.rom", NIAGARA_MD_ROM_SIZE);
+    memory_region_init_ram(&s->md_rom, NULL, "sun4v-md.rom",
+                           NIAGARA_MD_ROM_SIZE, &error_fatal);
     memory_region_add_subregion(sysmem, NIAGARA_MD_ROM_BASE, &s->md_rom);
-    memory_region_allocate_system_memory(&s->hv_rom, NULL,
-                                         "sun4v-hv.rom", NIAGARA_HV_ROM_SIZE);
+    memory_region_init_ram(&s->hv_rom, NULL, "sun4v-hv.rom",
+                           NIAGARA_HV_ROM_SIZE, &error_fatal);
     memory_region_add_subregion(sysmem, NIAGARA_HV_ROM_BASE, &s->hv_rom);
-    memory_region_allocate_system_memory(&s->prom, NULL,
-                                         "sun4v.prom", PROM_SIZE_MAX);
+    memory_region_init_ram(&s->prom, NULL, "sun4v.prom",
+                                         PROM_SIZE_MAX, &error_fatal);
     memory_region_add_subregion(sysmem, NIAGARA_PROM_BASE, &s->prom);
 
     add_rom_or_fail("nvram1", NIAGARA_NVRAM_BASE);
@@ -145,8 +145,8 @@ static void niagara_init(MachineState *machine)
         BlockBackend *blk = blk_by_legacy_dinfo(dinfo);
         int size = blk_getlength(blk);
         if (size > 0) {
-            memory_region_allocate_system_memory(&s->vdisk_ram, NULL,
-                                                 "sun4v_vdisk.ram", size);
+            memory_region_init_ram(&s->vdisk_ram, NULL, "sun4v_vdisk.ram", size,
+                                   &error_fatal);
             memory_region_add_subregion(get_system_memory(),
                                         NIAGARA_VDISK_BASE, &s->vdisk_ram);
             dinfo->is_default = 1;
-- 
2.18.1

Re: [Qemu-devel] [RFC] memory: use memory_region_init_ram() instead of memory_region_allocate_system_memory()

[Paolo Bonzini]

On 14/02/19 15:07, Igor Mammedov wrote:
> Also some boards (ab)use memory_region_allocate_system_memory(), calling it several
> times to allocate various fixed sized chunks of RAM and ROMs, which is problematic
> to map to a single initial RAM Machine::memdev backend and is currently broken if
> -mem-path points to a not hugepage pool.

This is certainly a good idea.  However, I'm not sure why you would need
a memdev property on the Machine instead of just allowing 1 -numa node,
which is what really is.

Thanks,

Paolo

Re: [Qemu-devel] [RFC] memory: use memory_region_init_ram() instead of memory_region_allocate_system_memory()

[Peter Maydell]

On Thu, 14 Feb 2019 at 14:07, Igor Mammedov <imammedo@redhat.com> wrote:
> Also some boards (ab)use memory_region_allocate_system_memory(), calling it several
> times to allocate various fixed sized chunks of RAM and ROMs, which is problematic
> to map to a single initial RAM Machine::memdev backend and is currently broken if
> -mem-path points to a not hugepage pool.

These boards are buggy and we could fix them, if we wanted to
keep the existing API. We should in that case add assertions
that memory_region_allocate_system_memory() is called once and
only once, which would let "make check" enforce the rule.

thanks
- PMM

Re: [Qemu-devel] [RFC] memory: use memory_region_init_ram() instead of memory_region_allocate_system_memory()

[Igor Mammedov]

On Thu, 14 Feb 2019 19:11:27 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 14/02/19 15:07, Igor Mammedov wrote:
> > Also some boards (ab)use memory_region_allocate_system_memory(), calling it several
> > times to allocate various fixed sized chunks of RAM and ROMs, which is problematic
> > to map to a single initial RAM Machine::memdev backend and is currently broken if
> > -mem-path points to a not hugepage pool.
> 
> This is certainly a good idea.  However, I'm not sure why you would need
> a memdev property on the Machine instead of just allowing 1 -numa node,
> which is what really is.

using '-numa node' would be confusing to user when he/she is not interested in numa usecase
it also would enable numa fdt/acpi parts generated automatically (fixable but then again
it adds more to confusion) and in the end there are boards that do not support numa at all
(s390x).

Machine memdev (initial-ram-memdev) property looked generic enough to me that it could be
used with every board, but could go on a bit further and instead of memdev an add initial-ram
property that would reference something like -device builtin-ram (I think I've seen some board
using a sysbus based ram device for initial ram).
If you ask why it's machine property, then I'm following '-m' semantics which is basically
a machine property and I don't have a better idea how to tell board on CLI to use some
device/memdev as initial ram.


> Thanks,
> 
> Paolo
> 

Re: [Qemu-devel] [RFC] memory: use memory_region_init_ram() instead of memory_region_allocate_system_memory()

[Igor Mammedov]

On Thu, 14 Feb 2019 18:30:37 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> On Thu, 14 Feb 2019 at 14:07, Igor Mammedov <imammedo@redhat.com> wrote:
> > Also some boards (ab)use memory_region_allocate_system_memory(), calling it several
> > times to allocate various fixed sized chunks of RAM and ROMs, which is problematic
> > to map to a single initial RAM Machine::memdev backend and is currently broken if
> > -mem-path points to a not hugepage pool.
> 
> These boards are buggy and we could fix them, if we wanted to
> keep the existing API. We should in that case add assertions
> that memory_region_allocate_system_memory() is called once and
> only once, which would let "make check" enforce the rule.
we can do this, but I'd rather remove memory_region_allocate_system_memory() API
altogether which looks like overkill for most boards and use simpler memory_region_init_ram().
Then boards that need numa/hugepages could be switched to newer memdev/device model
instead of using memory_region_init_ram() directly.

My end-goal in all this exercise is to switch initial RAM to frontend/backend
model and replace adhoc legacy numa memory code (-numa node,mem=size  & default numa splitting)
handling with a generic device or memdev based one.

> 
> thanks
> - PMM

Re: [Qemu-devel] [RFC] memory: use memory_region_init_ram() instead of memory_region_allocate_system_memory()

[Paolo Bonzini]

On 15/02/19 12:33, Igor Mammedov wrote:
> On Thu, 14 Feb 2019 19:11:27 +0100
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
>> On 14/02/19 15:07, Igor Mammedov wrote:
>>> Also some boards (ab)use memory_region_allocate_system_memory(), calling it several
>>> times to allocate various fixed sized chunks of RAM and ROMs, which is problematic
>>> to map to a single initial RAM Machine::memdev backend and is currently broken if
>>> -mem-path points to a not hugepage pool.
>>
>> This is certainly a good idea.  However, I'm not sure why you would need
>> a memdev property on the Machine instead of just allowing 1 -numa node,
>> which is what really is.
> 
> using '-numa node' would be confusing to user when he/she is not interested in numa usecase
> it also would enable numa fdt/acpi parts generated automatically (fixable but then again
> it adds more to confusion) and in the end there are boards that do not support numa at all
> (s390x).

Fair enough.

What about -m, too?  Then you'd specify a memdev instead of the initial
memory size.

Paolo

Re: [Qemu-devel] [RFC] memory: use memory_region_init_ram() instead of memory_region_allocate_system_memory()

[Igor Mammedov]

On Fri, 15 Feb 2019 15:48:43 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 15/02/19 12:33, Igor Mammedov wrote:
> > On Thu, 14 Feb 2019 19:11:27 +0100
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> > 
> >> On 14/02/19 15:07, Igor Mammedov wrote:
> >>> Also some boards (ab)use memory_region_allocate_system_memory(), calling it several
> >>> times to allocate various fixed sized chunks of RAM and ROMs, which is problematic
> >>> to map to a single initial RAM Machine::memdev backend and is currently broken if
> >>> -mem-path points to a not hugepage pool.
> >>
> >> This is certainly a good idea.  However, I'm not sure why you would need
> >> a memdev property on the Machine instead of just allowing 1 -numa node,
> >> which is what really is.
> > 
> > using '-numa node' would be confusing to user when he/she is not interested in numa usecase
> > it also would enable numa fdt/acpi parts generated automatically (fixable but then again
> > it adds more to confusion) and in the end there are boards that do not support numa at all
> > (s390x).
> 
> Fair enough.
> 
> What about -m, too?  Then you'd specify a memdev instead of the initial
> memory size.
that somewhat what I've planned,
make -m X translate into -object memory-backend-ram,id=magically-get-what-board-uses-now,size=X -machine memdev=thatid

one more reason for memdev vs device is that -numa now uses memdevs and so far it
doesn't look that non-numa initial RAM would get immediate benefits from using -device
on most boards (well, I couldn't come up with any modulo backend/frontend consistent usage)

> 
> Paolo
> 

Re: [Qemu-devel] [RFC] memory: use memory_region_init_ram() instead of memory_region_allocate_system_memory()

[Paolo Bonzini]

On 15/02/19 16:42, Igor Mammedov wrote:
>>
>> What about -m, too?  Then you'd specify a memdev instead of the initial
>> memory size.
> that somewhat what I've planned,
> make -m X translate into -object memory-backend-ram,id=magically-get-what-board-uses-now,size=X -machine memdev=thatid
> 
> one more reason for memdev vs device is that -numa now uses memdevs and so far it
> doesn't look that non-numa initial RAM would get immediate benefits from using -device
> on most boards (well, I couldn't come up with any modulo backend/frontend consistent usage)

What I meant is what about making the non-NUMA option -m memdev=thatid
(and the automatic translation you mention too).  Yes, bikeshedding. :)

Paolo

Re: [Qemu-devel] [RFC] memory: use memory_region_init_ram() instead of memory_region_allocate_system_memory()

[Igor Mammedov]

On Fri, 15 Feb 2019 19:18:50 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 15/02/19 16:42, Igor Mammedov wrote:
> >>
> >> What about -m, too?  Then you'd specify a memdev instead of the initial
> >> memory size.
> > that somewhat what I've planned,
> > make -m X translate into -object memory-backend-ram,id=magically-get-what-board-uses-now,size=X -machine memdev=thatid
> > 
> > one more reason for memdev vs device is that -numa now uses memdevs and so far it
> > doesn't look that non-numa initial RAM would get immediate benefits from using -device
> > on most boards (well, I couldn't come up with any modulo backend/frontend consistent usage)
> 
> What I meant is what about making the non-NUMA option -m memdev=thatid
> (and the automatic translation you mention too).  Yes, bikeshedding. :)
sure, I can add it,
it would be just an alias to machine.memdev


> 
> Paolo
>