| Message ID | 20200217173452.15243-12-imammedo@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| 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. > - introduce RAM_ADDR_UFMT to avoid build errors on 32-bit hosts > when specifying format string for ram_addr_t type > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > Reviewed-by: Andrew Jones <drjones@redhat.com> > --- > v2: > * fix format string causing build failure on 32-bit host > (Philippe Mathieu-Daudé <philmd@redhat.com>) > v3: > * instead of RAM_ADDR_UFMT adding use size_to_str() > Philippe Mathieu-Daudé <philmd@redhat.com> > --- Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
Hi Igor, On 2/17/20 6:33 PM, 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. > - introduce RAM_ADDR_UFMT to avoid build errors on 32-bit hosts > when specifying format string for ram_addr_t type Since v3 the 2nd comment is not valid anymore (also in other patches from this series). > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > Reviewed-by: Andrew Jones <drjones@redhat.com> > --- > v2: > * fix format string causing build failure on 32-bit host > (Philippe Mathieu-Daudé <philmd@redhat.com>) > v3: > * instead of RAM_ADDR_UFMT adding use size_to_str() > Philippe Mathieu-Daudé <philmd@redhat.com> > --- > hw/arm/collie.c | 17 ++++++++++++----- > 1 file changed, 12 insertions(+), 5 deletions(-) > > diff --git a/hw/arm/collie.c b/hw/arm/collie.c > index 970a4405cc..024893fc9e 100644 > --- a/hw/arm/collie.c > +++ b/hw/arm/collie.c > @@ -10,6 +10,7 @@ > */ > #include "qemu/osdep.h" > #include "qemu/units.h" > +#include "qemu/cutils.h" > #include "hw/sysbus.h" > #include "hw/boards.h" > #include "strongarm.h" > @@ -20,20 +21,24 @@ > > static struct arm_boot_info collie_binfo = { > .loader_start = SA_SDCS0, > - .ram_size = 0x20000000, This doesn't seem correct, this field is used do_cpu_reset() -> set_kernel_args() (see hw/arm/boot.c). > }; > > static void collie_init(MachineState *machine) > { > StrongARMState *s; > DriveInfo *dinfo; > - MemoryRegion *sdram = g_new(MemoryRegion, 1); > + 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); > + } > > s = sa1110_init(machine->cpu_type); > > - memory_region_allocate_system_memory(sdram, NULL, "strongarm.sdram", > - collie_binfo.ram_size); > - memory_region_add_subregion(get_system_memory(), SA_SDCS0, sdram); > + memory_region_add_subregion(get_system_memory(), SA_SDCS0, machine->ram); > > dinfo = drive_get(IF_PFLASH, 0, 0); > pflash_cfi01_register(SA_CS0, "collie.fl1", 0x02000000, > @@ -57,6 +62,8 @@ static void collie_machine_init(MachineClass *mc) > mc->init = collie_init; > mc->ignore_memory_transaction_failures = true; > mc->default_cpu_type = ARM_CPU_TYPE_NAME("sa1110"); > + mc->default_ram_size = 0x20000000; > + mc->default_ram_id = "strongarm.sdram"; > } > > DEFINE_MACHINE("collie", collie_machine_init) >
On Tue, 18 Feb 2020 18:16:14 +0100 Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > Hi Igor, > > On 2/17/20 6:33 PM, 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. > > - introduce RAM_ADDR_UFMT to avoid build errors on 32-bit hosts > > when specifying format string for ram_addr_t type > > Since v3 the 2nd comment is not valid anymore (also in other patches > from this series). I looks like the last remnant of RAM_ADDR_UFMT I'll remove it in v6 > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > Reviewed-by: Andrew Jones <drjones@redhat.com> > > --- > > v2: > > * fix format string causing build failure on 32-bit host > > (Philippe Mathieu-Daudé <philmd@redhat.com>) > > v3: > > * instead of RAM_ADDR_UFMT adding use size_to_str() > > Philippe Mathieu-Daudé <philmd@redhat.com> > > --- > > hw/arm/collie.c | 17 ++++++++++++----- > > 1 file changed, 12 insertions(+), 5 deletions(-) > > > > diff --git a/hw/arm/collie.c b/hw/arm/collie.c > > index 970a4405cc..024893fc9e 100644 > > --- a/hw/arm/collie.c > > +++ b/hw/arm/collie.c > > @@ -10,6 +10,7 @@ > > */ > > #include "qemu/osdep.h" > > #include "qemu/units.h" > > +#include "qemu/cutils.h" > > #include "hw/sysbus.h" > > #include "hw/boards.h" > > #include "strongarm.h" > > @@ -20,20 +21,24 @@ > > > > static struct arm_boot_info collie_binfo = { > > .loader_start = SA_SDCS0, > > - .ram_size = 0x20000000, > > This doesn't seem correct, this field is used do_cpu_reset() -> > set_kernel_args() (see hw/arm/boot.c). Thanks, fixed in v6. I'll respin series as already several amended patches accumulated by this time. > > > }; > > > > static void collie_init(MachineState *machine) > > { > > StrongARMState *s; > > DriveInfo *dinfo; > > - MemoryRegion *sdram = g_new(MemoryRegion, 1); > > + 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); > > + } > > > > s = sa1110_init(machine->cpu_type); > > > > - memory_region_allocate_system_memory(sdram, NULL, "strongarm.sdram", > > - collie_binfo.ram_size); > > - memory_region_add_subregion(get_system_memory(), SA_SDCS0, sdram); > > + memory_region_add_subregion(get_system_memory(), SA_SDCS0, machine->ram); > > > > dinfo = drive_get(IF_PFLASH, 0, 0); > > pflash_cfi01_register(SA_CS0, "collie.fl1", 0x02000000, > > @@ -57,6 +62,8 @@ static void collie_machine_init(MachineClass *mc) > > mc->init = collie_init; > > mc->ignore_memory_transaction_failures = true; > > mc->default_cpu_type = ARM_CPU_TYPE_NAME("sa1110"); > > + mc->default_ram_size = 0x20000000; > > + mc->default_ram_id = "strongarm.sdram"; > > } > > > > DEFINE_MACHINE("collie", collie_machine_init) > > > >
On 2/19/20 2:44 PM, Igor Mammedov wrote: > On Tue, 18 Feb 2020 18:16:14 +0100 > Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > >> Hi Igor, >> >> On 2/17/20 6:33 PM, 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. >>> - introduce RAM_ADDR_UFMT to avoid build errors on 32-bit hosts >>> when specifying format string for ram_addr_t type >> >> Since v3 the 2nd comment is not valid anymore (also in other patches >> from this series). > > I looks like the last remnant of RAM_ADDR_UFMT > I'll remove it in v6 Thanks. > >>> >>> Signed-off-by: Igor Mammedov <imammedo@redhat.com> >>> Reviewed-by: Andrew Jones <drjones@redhat.com> >>> --- >>> v2: >>> * fix format string causing build failure on 32-bit host >>> (Philippe Mathieu-Daudé <philmd@redhat.com>) >>> v3: >>> * instead of RAM_ADDR_UFMT adding use size_to_str() >>> Philippe Mathieu-Daudé <philmd@redhat.com> >>> --- >>> hw/arm/collie.c | 17 ++++++++++++----- >>> 1 file changed, 12 insertions(+), 5 deletions(-) >>> >>> diff --git a/hw/arm/collie.c b/hw/arm/collie.c >>> index 970a4405cc..024893fc9e 100644 >>> --- a/hw/arm/collie.c >>> +++ b/hw/arm/collie.c >>> @@ -10,6 +10,7 @@ >>> */ >>> #include "qemu/osdep.h" >>> #include "qemu/units.h" >>> +#include "qemu/cutils.h" >>> #include "hw/sysbus.h" >>> #include "hw/boards.h" >>> #include "strongarm.h" >>> @@ -20,20 +21,24 @@ >>> >>> static struct arm_boot_info collie_binfo = { >>> .loader_start = SA_SDCS0, >>> - .ram_size = 0x20000000, >> >> This doesn't seem correct, this field is used do_cpu_reset() -> >> set_kernel_args() (see hw/arm/boot.c). > > Thanks, > fixed in v6. > > I'll respin series as already several amended patches accumulated by this time. With the arm_boot_info ram_size unmodified, please add Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > >> >>> }; >>> >>> static void collie_init(MachineState *machine) >>> { >>> StrongARMState *s; >>> DriveInfo *dinfo; >>> - MemoryRegion *sdram = g_new(MemoryRegion, 1); >>> + 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); >>> + } >>> >>> s = sa1110_init(machine->cpu_type); >>> >>> - memory_region_allocate_system_memory(sdram, NULL, "strongarm.sdram", >>> - collie_binfo.ram_size); >>> - memory_region_add_subregion(get_system_memory(), SA_SDCS0, sdram); >>> + memory_region_add_subregion(get_system_memory(), SA_SDCS0, machine->ram); >>> >>> dinfo = drive_get(IF_PFLASH, 0, 0); >>> pflash_cfi01_register(SA_CS0, "collie.fl1", 0x02000000, >>> @@ -57,6 +62,8 @@ static void collie_machine_init(MachineClass *mc) >>> mc->init = collie_init; >>> mc->ignore_memory_transaction_failures = true; >>> mc->default_cpu_type = ARM_CPU_TYPE_NAME("sa1110"); >>> + mc->default_ram_size = 0x20000000; >>> + mc->default_ram_id = "strongarm.sdram"; >>> } >>> >>> DEFINE_MACHINE("collie", collie_machine_init) >>> >> >> >
diff --git a/hw/arm/collie.c b/hw/arm/collie.c index 970a4405cc..024893fc9e 100644 --- a/hw/arm/collie.c +++ b/hw/arm/collie.c @@ -10,6 +10,7 @@ */ #include "qemu/osdep.h" #include "qemu/units.h" +#include "qemu/cutils.h" #include "hw/sysbus.h" #include "hw/boards.h" #include "strongarm.h" @@ -20,20 +21,24 @@ static struct arm_boot_info collie_binfo = { .loader_start = SA_SDCS0, - .ram_size = 0x20000000, }; static void collie_init(MachineState *machine) { StrongARMState *s; DriveInfo *dinfo; - MemoryRegion *sdram = g_new(MemoryRegion, 1); + 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); + } s = sa1110_init(machine->cpu_type); - memory_region_allocate_system_memory(sdram, NULL, "strongarm.sdram", - collie_binfo.ram_size); - memory_region_add_subregion(get_system_memory(), SA_SDCS0, sdram); + memory_region_add_subregion(get_system_memory(), SA_SDCS0, machine->ram); dinfo = drive_get(IF_PFLASH, 0, 0); pflash_cfi01_register(SA_CS0, "collie.fl1", 0x02000000, @@ -57,6 +62,8 @@ static void collie_machine_init(MachineClass *mc) mc->init = collie_init; mc->ignore_memory_transaction_failures = true; mc->default_cpu_type = ARM_CPU_TYPE_NAME("sa1110"); + mc->default_ram_size = 0x20000000; + mc->default_ram_id = "strongarm.sdram"; } DEFINE_MACHINE("collie", collie_machine_init)