| Message ID | 20200217173452.15243-18-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. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > Reviewed-by: Andrew Jones <drjones@redhat.com> > --- Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
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. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > Reviewed-by: Andrew Jones <drjones@redhat.com> > --- > CC: peter.chubb@nicta.com.au > --- > hw/arm/integratorcp.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/hw/arm/integratorcp.c b/hw/arm/integratorcp.c > index 0cd94d9f09..cc845b8534 100644 > --- a/hw/arm/integratorcp.c > +++ b/hw/arm/integratorcp.c > @@ -585,7 +585,6 @@ static void integratorcp_init(MachineState *machine) > Object *cpuobj; > ARMCPU *cpu; > MemoryRegion *address_space_mem = get_system_memory(); > - MemoryRegion *ram = g_new(MemoryRegion, 1); > MemoryRegion *ram_alias = g_new(MemoryRegion, 1); > qemu_irq pic[32]; > DeviceState *dev, *sic, *icp; > @@ -605,14 +604,13 @@ static void integratorcp_init(MachineState *machine) > > cpu = ARM_CPU(cpuobj); > > - memory_region_allocate_system_memory(ram, NULL, "integrator.ram", > - ram_size); > /* ??? On a real system the first 1Mb is mapped as SSRAM or boot flash. */ > /* ??? RAM should repeat to fill physical memory space. */ > /* SDRAM at address zero*/ > - memory_region_add_subregion(address_space_mem, 0, ram); > + memory_region_add_subregion(address_space_mem, 0, machine->ram); > /* And again at address 0x80000000 */ > - memory_region_init_alias(ram_alias, NULL, "ram.alias", ram, 0, ram_size); > + memory_region_init_alias(ram_alias, NULL, "ram.alias", machine->ram, > + 0, ram_size); > memory_region_add_subregion(address_space_mem, 0x80000000, ram_alias); > > dev = qdev_create(NULL, TYPE_INTEGRATOR_CM); > @@ -660,6 +658,7 @@ static void integratorcp_machine_init(MachineClass *mc) > mc->init = integratorcp_init; > mc->ignore_memory_transaction_failures = true; > mc->default_cpu_type = ARM_CPU_TYPE_NAME("arm926"); > + mc->default_ram_id = "integrator.ram"; > } > > DEFINE_MACHINE("integratorcp", integratorcp_machine_init) > Looking at integratorcm_realize() this machine seems to handle at most 512MiB.
On Tue, 18 Feb 2020 07:55:14 +0100 Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > 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. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > Reviewed-by: Andrew Jones <drjones@redhat.com> > > --- > > CC: peter.chubb@nicta.com.au > > --- > > hw/arm/integratorcp.c | 9 ++++----- > > 1 file changed, 4 insertions(+), 5 deletions(-) > > > > diff --git a/hw/arm/integratorcp.c b/hw/arm/integratorcp.c > > index 0cd94d9f09..cc845b8534 100644 > > --- a/hw/arm/integratorcp.c > > +++ b/hw/arm/integratorcp.c > > @@ -585,7 +585,6 @@ static void integratorcp_init(MachineState *machine) > > Object *cpuobj; > > ARMCPU *cpu; > > MemoryRegion *address_space_mem = get_system_memory(); > > - MemoryRegion *ram = g_new(MemoryRegion, 1); > > MemoryRegion *ram_alias = g_new(MemoryRegion, 1); > > qemu_irq pic[32]; > > DeviceState *dev, *sic, *icp; > > @@ -605,14 +604,13 @@ static void integratorcp_init(MachineState *machine) > > > > cpu = ARM_CPU(cpuobj); > > > > - memory_region_allocate_system_memory(ram, NULL, "integrator.ram", > > - ram_size); > > /* ??? On a real system the first 1Mb is mapped as SSRAM or boot flash. */ > > /* ??? RAM should repeat to fill physical memory space. */ > > /* SDRAM at address zero*/ > > - memory_region_add_subregion(address_space_mem, 0, ram); > > + memory_region_add_subregion(address_space_mem, 0, machine->ram); > > /* And again at address 0x80000000 */ > > - memory_region_init_alias(ram_alias, NULL, "ram.alias", ram, 0, ram_size); > > + memory_region_init_alias(ram_alias, NULL, "ram.alias", machine->ram, > > + 0, ram_size); > > memory_region_add_subregion(address_space_mem, 0x80000000, ram_alias); > > > > dev = qdev_create(NULL, TYPE_INTEGRATOR_CM); > > @@ -660,6 +658,7 @@ static void integratorcp_machine_init(MachineClass *mc) > > mc->init = integratorcp_init; > > mc->ignore_memory_transaction_failures = true; > > mc->default_cpu_type = ARM_CPU_TYPE_NAME("arm926"); > > + mc->default_ram_id = "integrator.ram"; > > } > > > > DEFINE_MACHINE("integratorcp", integratorcp_machine_init) > > > > Looking at integratorcm_realize() this machine seems to handle at most > 512MiB. > I'll add a patch on top.
On Tue, 18 Feb 2020 07:55:14 +0100 Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > 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. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > Reviewed-by: Andrew Jones <drjones@redhat.com> > > --- > > CC: peter.chubb@nicta.com.au > > --- > > hw/arm/integratorcp.c | 9 ++++----- > > 1 file changed, 4 insertions(+), 5 deletions(-) > > > > diff --git a/hw/arm/integratorcp.c b/hw/arm/integratorcp.c > > index 0cd94d9f09..cc845b8534 100644 > > --- a/hw/arm/integratorcp.c > > +++ b/hw/arm/integratorcp.c > > @@ -585,7 +585,6 @@ static void integratorcp_init(MachineState *machine) > > Object *cpuobj; > > ARMCPU *cpu; > > MemoryRegion *address_space_mem = get_system_memory(); > > - MemoryRegion *ram = g_new(MemoryRegion, 1); > > MemoryRegion *ram_alias = g_new(MemoryRegion, 1); > > qemu_irq pic[32]; > > DeviceState *dev, *sic, *icp; > > @@ -605,14 +604,13 @@ static void integratorcp_init(MachineState *machine) > > > > cpu = ARM_CPU(cpuobj); > > > > - memory_region_allocate_system_memory(ram, NULL, "integrator.ram", > > - ram_size); > > /* ??? On a real system the first 1Mb is mapped as SSRAM or boot flash. */ > > /* ??? RAM should repeat to fill physical memory space. */ > > /* SDRAM at address zero*/ > > - memory_region_add_subregion(address_space_mem, 0, ram); > > + memory_region_add_subregion(address_space_mem, 0, machine->ram); > > /* And again at address 0x80000000 */ > > - memory_region_init_alias(ram_alias, NULL, "ram.alias", ram, 0, ram_size); > > + memory_region_init_alias(ram_alias, NULL, "ram.alias", machine->ram, > > + 0, ram_size); > > memory_region_add_subregion(address_space_mem, 0x80000000, ram_alias); > > > > dev = qdev_create(NULL, TYPE_INTEGRATOR_CM); > > @@ -660,6 +658,7 @@ static void integratorcp_machine_init(MachineClass *mc) > > mc->init = integratorcp_init; > > mc->ignore_memory_transaction_failures = true; > > mc->default_cpu_type = ARM_CPU_TYPE_NAME("arm926"); > > + mc->default_ram_id = "integrator.ram"; > > } > > > > DEFINE_MACHINE("integratorcp", integratorcp_machine_init) > > > > Looking at integratorcm_realize() this machine seems to handle at most > 512MiB. According to http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0159b/Cegeadbj.html it supports "optionally, 16 to 256MB of SDRAM plugged into the DIMM socket" so I'm not sure that realize is valid reference here. (well I don't know anything about arm boards, but it probably should be double checked by maintainer). PS: It should not hold this series (as check wasn't there to begin with), I'll post a patch on top to add check once we decide to what limit it should be set.
On 2/18/20 5:41 PM, Igor Mammedov wrote: > On Tue, 18 Feb 2020 07:55:14 +0100 > Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > >> 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. >>> >>> Signed-off-by: Igor Mammedov <imammedo@redhat.com> >>> Reviewed-by: Andrew Jones <drjones@redhat.com> >>> --- >>> CC: peter.chubb@nicta.com.au >>> --- >>> hw/arm/integratorcp.c | 9 ++++----- >>> 1 file changed, 4 insertions(+), 5 deletions(-) >>> >>> diff --git a/hw/arm/integratorcp.c b/hw/arm/integratorcp.c >>> index 0cd94d9f09..cc845b8534 100644 >>> --- a/hw/arm/integratorcp.c >>> +++ b/hw/arm/integratorcp.c >>> @@ -585,7 +585,6 @@ static void integratorcp_init(MachineState *machine) >>> Object *cpuobj; >>> ARMCPU *cpu; >>> MemoryRegion *address_space_mem = get_system_memory(); >>> - MemoryRegion *ram = g_new(MemoryRegion, 1); >>> MemoryRegion *ram_alias = g_new(MemoryRegion, 1); >>> qemu_irq pic[32]; >>> DeviceState *dev, *sic, *icp; >>> @@ -605,14 +604,13 @@ static void integratorcp_init(MachineState *machine) >>> >>> cpu = ARM_CPU(cpuobj); >>> >>> - memory_region_allocate_system_memory(ram, NULL, "integrator.ram", >>> - ram_size); >>> /* ??? On a real system the first 1Mb is mapped as SSRAM or boot flash. */ >>> /* ??? RAM should repeat to fill physical memory space. */ >>> /* SDRAM at address zero*/ >>> - memory_region_add_subregion(address_space_mem, 0, ram); >>> + memory_region_add_subregion(address_space_mem, 0, machine->ram); >>> /* And again at address 0x80000000 */ >>> - memory_region_init_alias(ram_alias, NULL, "ram.alias", ram, 0, ram_size); >>> + memory_region_init_alias(ram_alias, NULL, "ram.alias", machine->ram, >>> + 0, ram_size); >>> memory_region_add_subregion(address_space_mem, 0x80000000, ram_alias); >>> >>> dev = qdev_create(NULL, TYPE_INTEGRATOR_CM); >>> @@ -660,6 +658,7 @@ static void integratorcp_machine_init(MachineClass *mc) >>> mc->init = integratorcp_init; >>> mc->ignore_memory_transaction_failures = true; >>> mc->default_cpu_type = ARM_CPU_TYPE_NAME("arm926"); >>> + mc->default_ram_id = "integrator.ram"; >>> } >>> >>> DEFINE_MACHINE("integratorcp", integratorcp_machine_init) >>> >> >> Looking at integratorcm_realize() this machine seems to handle at most >> 512MiB. > > According to > http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0159b/Cegeadbj.html > > it supports "optionally, 16 to 256MB of SDRAM plugged into the DIMM socket" > so I'm not sure that realize is valid reference here. > (well I don't know anything about arm boards, but it probably should be > double checked by maintainer). > > PS: > It should not hold this series (as check wasn't there to begin with), > I'll post a patch on top to add check once we decide to what limit it > should be set. This is certainly not a blocking comment (neither am I waiting for you to fix this, I was just making a comment while reviewing the whole). Sorry if this was not clear.
diff --git a/hw/arm/integratorcp.c b/hw/arm/integratorcp.c index 0cd94d9f09..cc845b8534 100644 --- a/hw/arm/integratorcp.c +++ b/hw/arm/integratorcp.c @@ -585,7 +585,6 @@ static void integratorcp_init(MachineState *machine) Object *cpuobj; ARMCPU *cpu; MemoryRegion *address_space_mem = get_system_memory(); - MemoryRegion *ram = g_new(MemoryRegion, 1); MemoryRegion *ram_alias = g_new(MemoryRegion, 1); qemu_irq pic[32]; DeviceState *dev, *sic, *icp; @@ -605,14 +604,13 @@ static void integratorcp_init(MachineState *machine) cpu = ARM_CPU(cpuobj); - memory_region_allocate_system_memory(ram, NULL, "integrator.ram", - ram_size); /* ??? On a real system the first 1Mb is mapped as SSRAM or boot flash. */ /* ??? RAM should repeat to fill physical memory space. */ /* SDRAM at address zero*/ - memory_region_add_subregion(address_space_mem, 0, ram); + memory_region_add_subregion(address_space_mem, 0, machine->ram); /* And again at address 0x80000000 */ - memory_region_init_alias(ram_alias, NULL, "ram.alias", ram, 0, ram_size); + memory_region_init_alias(ram_alias, NULL, "ram.alias", machine->ram, + 0, ram_size); memory_region_add_subregion(address_space_mem, 0x80000000, ram_alias); dev = qdev_create(NULL, TYPE_INTEGRATOR_CM); @@ -660,6 +658,7 @@ static void integratorcp_machine_init(MachineClass *mc) mc->init = integratorcp_init; mc->ignore_memory_transaction_failures = true; mc->default_cpu_type = ARM_CPU_TYPE_NAME("arm926"); + mc->default_ram_id = "integrator.ram"; } DEFINE_MACHINE("integratorcp", integratorcp_machine_init)