| Message ID | 20200217173452.15243-25-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | refactor main RAM allocation to use hostmem backend | expand |
On 2/17/20 9:33 AM, Igor Mammedov wrote: > memory_region_allocate_system_memory() API is going away, so > replace it with memdev allocated MemoryRegion. The later is > initialized by generic code, so board only needs to opt in > to memdev scheme by providing > MachineClass::default_ram_id > and using MachineState::ram instead of manually initializing > RAM memory region. > > PS: > while at it add check for user supplied RAM size and error > out if it mismatches board expected value. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > Reviewed-by: Andrew Jones <drjones@redhat.com> > --- Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > @@ -1589,16 +1590,21 @@ static void musicpal_init(MachineState *machine) > int i; > unsigned long flash_size; > DriveInfo *dinfo; > + MachineClass *mc = MACHINE_GET_CLASS(machine); > MemoryRegion *address_space_mem = get_system_memory(); > - MemoryRegion *ram = g_new(MemoryRegion, 1); > MemoryRegion *sram = g_new(MemoryRegion, 1); > > + /* For now we use a fixed - the original - RAM size */ > + if (machine->ram_size != mc->default_ram_size) { > + char *sz = size_to_str(mc->default_ram_size); > + error_report("Invalid RAM size, should be %s", sz); > + g_free(sz); > + exit(EXIT_FAILURE); > + } If for some reason you need to re-spin this series again, and considering my comment re arm/imx25_pdk, I think it would be worthwhile to create a common helper for this: void machine_memory_check_fixed_size(MachineState *machine) { MachineClass *mc = MACHINE_GET_CLASS(machine); if (machine->ram_size != mc->default_ram_size) { char *sz = size_to_str(mc->default_ram_size); error_report("Invalid RAM size, should be %s", sz); g_free(sz); exit(EXIT_FAILURE); } } That would keep the language consistent across the boards. r~
On 2/17/20 8:11 PM, Richard Henderson wrote: > On 2/17/20 9:33 AM, Igor Mammedov wrote: >> memory_region_allocate_system_memory() API is going away, so >> replace it with memdev allocated MemoryRegion. The later is >> initialized by generic code, so board only needs to opt in >> to memdev scheme by providing >> MachineClass::default_ram_id >> and using MachineState::ram instead of manually initializing >> RAM memory region. >> >> PS: >> while at it add check for user supplied RAM size and error >> out if it mismatches board expected value. >> >> Signed-off-by: Igor Mammedov <imammedo@redhat.com> >> Reviewed-by: Andrew Jones <drjones@redhat.com> >> --- > > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > >> @@ -1589,16 +1590,21 @@ static void musicpal_init(MachineState *machine) >> int i; >> unsigned long flash_size; >> DriveInfo *dinfo; >> + MachineClass *mc = MACHINE_GET_CLASS(machine); >> MemoryRegion *address_space_mem = get_system_memory(); >> - MemoryRegion *ram = g_new(MemoryRegion, 1); >> MemoryRegion *sram = g_new(MemoryRegion, 1); >> >> + /* For now we use a fixed - the original - RAM size */ >> + if (machine->ram_size != mc->default_ram_size) { >> + char *sz = size_to_str(mc->default_ram_size); >> + error_report("Invalid RAM size, should be %s", sz); >> + g_free(sz); >> + exit(EXIT_FAILURE); >> + } > > If for some reason you need to re-spin this series again, and considering my > comment re arm/imx25_pdk, I think it would be worthwhile to create a common > helper for this: > > void machine_memory_check_fixed_size(MachineState *machine) > { > MachineClass *mc = MACHINE_GET_CLASS(machine); > > if (machine->ram_size != mc->default_ram_size) { > char *sz = size_to_str(mc->default_ram_size); > error_report("Invalid RAM size, should be %s", sz); > g_free(sz); > exit(EXIT_FAILURE); > } > } > > That would keep the language consistent across the boards. Excellent idea. Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
On Mon, 17 Feb 2020 11:11:29 -0800 Richard Henderson <richard.henderson@linaro.org> wrote: > On 2/17/20 9:33 AM, Igor Mammedov wrote: > > memory_region_allocate_system_memory() API is going away, so > > replace it with memdev allocated MemoryRegion. The later is > > initialized by generic code, so board only needs to opt in > > to memdev scheme by providing > > MachineClass::default_ram_id > > and using MachineState::ram instead of manually initializing > > RAM memory region. > > > > PS: > > while at it add check for user supplied RAM size and error > > out if it mismatches board expected value. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > Reviewed-by: Andrew Jones <drjones@redhat.com> > > --- > > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Thanks! > > > @@ -1589,16 +1590,21 @@ static void musicpal_init(MachineState *machine) > > int i; > > unsigned long flash_size; > > DriveInfo *dinfo; > > + MachineClass *mc = MACHINE_GET_CLASS(machine); > > MemoryRegion *address_space_mem = get_system_memory(); > > - MemoryRegion *ram = g_new(MemoryRegion, 1); > > MemoryRegion *sram = g_new(MemoryRegion, 1); > > > > + /* For now we use a fixed - the original - RAM size */ > > + if (machine->ram_size != mc->default_ram_size) { > > + char *sz = size_to_str(mc->default_ram_size); > > + error_report("Invalid RAM size, should be %s", sz); > > + g_free(sz); > > + exit(EXIT_FAILURE); > > + } > > If for some reason you need to re-spin this series again, and considering my > comment re arm/imx25_pdk, I think it would be worthwhile to create a common > helper for this: > This check is temporary, I plan to replace it with a similar check in generic machine code and clean it up. The reason it is not done in this series is that generalizing it is not related to this series. Hence I'd prefer to keep current approach in this series to avoid touching already reviewed patches and generalize it later. > void machine_memory_check_fixed_size(MachineState *machine) > { > MachineClass *mc = MACHINE_GET_CLASS(machine); > > if (machine->ram_size != mc->default_ram_size) { > char *sz = size_to_str(mc->default_ram_size); > error_report("Invalid RAM size, should be %s", sz); > g_free(sz); > exit(EXIT_FAILURE); > } > } > > That would keep the language consistent across the boards. > > > r~ >
On 2/18/20 12:59 AM, Igor Mammedov wrote: >> If for some reason you need to re-spin this series again, and considering my >> comment re arm/imx25_pdk, I think it would be worthwhile to create a common >> helper for this: >> > > This check is temporary, I plan to replace it with a similar > check in generic machine code and clean it up. The reason it > is not done in this series is that generalizing it is not > related to this series. Hence I'd prefer to keep current > approach in this series to avoid touching already reviewed > patches and generalize it later. Fair enough. r~
diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c index dc551bb0c0..db8b03cb83 100644 --- a/hw/arm/musicpal.c +++ b/hw/arm/musicpal.c @@ -32,6 +32,7 @@ #include "sysemu/runstate.h" #include "exec/address-spaces.h" #include "ui/pixel_ops.h" +#include "qemu/cutils.h" #define MP_MISC_BASE 0x80002000 #define MP_MISC_SIZE 0x00001000 @@ -1589,16 +1590,21 @@ static void musicpal_init(MachineState *machine) int i; unsigned long flash_size; DriveInfo *dinfo; + MachineClass *mc = MACHINE_GET_CLASS(machine); MemoryRegion *address_space_mem = get_system_memory(); - MemoryRegion *ram = g_new(MemoryRegion, 1); MemoryRegion *sram = g_new(MemoryRegion, 1); + /* For now we use a fixed - the original - RAM size */ + if (machine->ram_size != mc->default_ram_size) { + char *sz = size_to_str(mc->default_ram_size); + error_report("Invalid RAM size, should be %s", sz); + g_free(sz); + exit(EXIT_FAILURE); + } + 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_add_subregion(address_space_mem, 0, ram); + memory_region_add_subregion(address_space_mem, 0, machine->ram); memory_region_init_ram(sram, NULL, "musicpal.sram", MP_SRAM_SIZE, &error_fatal); @@ -1714,6 +1720,8 @@ static void musicpal_machine_init(MachineClass *mc) mc->init = musicpal_init; mc->ignore_memory_transaction_failures = true; mc->default_cpu_type = ARM_CPU_TYPE_NAME("arm926"); + mc->default_ram_size = MP_RAM_DEFAULT_SIZE; + mc->default_ram_id = "musicpal.ram"; } DEFINE_MACHINE("musicpal", musicpal_machine_init)