* [PATCH 1/6] hw/riscv: microchip_pfsoc: Use MachineState::ram and MachineClass::default_ram_id
@ 2021-10-18 15:38 Bin Meng
2021-10-18 15:38 ` [PATCH 2/6] hw/riscv: opentitan: " Bin Meng
` (6 more replies)
0 siblings, 7 replies; 27+ messages in thread
From: Bin Meng @ 2021-10-18 15:38 UTC (permalink / raw)
To: Alistair Francis, Igor Mammedov, qemu-devel, qemu-riscv
Using memory_region_init_ram(), which can't possibly handle vhost-user,
and can't work as expected with '-numa node,memdev' options.
Use MachineState::ram instead of manually initializing RAM memory
region, as well as by providing MachineClass::default_ram_id to
opt in to memdev scheme.
Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---
hw/riscv/microchip_pfsoc.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
index e475b6d511..f10f55b488 100644
--- a/hw/riscv/microchip_pfsoc.c
+++ b/hw/riscv/microchip_pfsoc.c
@@ -459,7 +459,6 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
const MemMapEntry *memmap = microchip_pfsoc_memmap;
MicrochipIcicleKitState *s = MICROCHIP_ICICLE_KIT_MACHINE(machine);
MemoryRegion *system_memory = get_system_memory();
- MemoryRegion *mem_low = g_new(MemoryRegion, 1);
MemoryRegion *mem_low_alias = g_new(MemoryRegion, 1);
MemoryRegion *mem_high = g_new(MemoryRegion, 1);
MemoryRegion *mem_high_alias = g_new(MemoryRegion, 1);
@@ -486,16 +485,13 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
qdev_realize(DEVICE(&s->soc), NULL, &error_abort);
/* Register RAM */
- memory_region_init_ram(mem_low, NULL, "microchip.icicle.kit.ram_low",
- memmap[MICROCHIP_PFSOC_DRAM_LO].size,
- &error_fatal);
memory_region_init_alias(mem_low_alias, NULL,
"microchip.icicle.kit.ram_low.alias",
- mem_low, 0,
+ machine->ram, 0,
memmap[MICROCHIP_PFSOC_DRAM_LO_ALIAS].size);
memory_region_add_subregion(system_memory,
memmap[MICROCHIP_PFSOC_DRAM_LO].base,
- mem_low);
+ machine->ram);
memory_region_add_subregion(system_memory,
memmap[MICROCHIP_PFSOC_DRAM_LO_ALIAS].base,
mem_low_alias);
@@ -606,6 +602,7 @@ static void microchip_icicle_kit_machine_class_init(ObjectClass *oc, void *data)
MICROCHIP_PFSOC_COMPUTE_CPU_COUNT;
mc->min_cpus = MICROCHIP_PFSOC_MANAGEMENT_CPU_COUNT + 1;
mc->default_cpus = mc->min_cpus;
+ mc->default_ram_id = "microchip.icicle.kit.ram_low";
/*
* Map 513 MiB high memory, the mimimum required high memory size, because
--
2.25.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 2/6] hw/riscv: opentitan: Use MachineState::ram and MachineClass::default_ram_id
2021-10-18 15:38 [PATCH 1/6] hw/riscv: microchip_pfsoc: Use MachineState::ram and MachineClass::default_ram_id Bin Meng
@ 2021-10-18 15:38 ` Bin Meng
2021-10-19 7:11 ` Igor Mammedov
2021-10-18 15:38 ` [PATCH 3/6] hw/riscv: shakti_c: " Bin Meng
` (5 subsequent siblings)
6 siblings, 1 reply; 27+ messages in thread
From: Bin Meng @ 2021-10-18 15:38 UTC (permalink / raw)
To: Alistair Francis, Igor Mammedov, qemu-devel, qemu-riscv
Using memory_region_init_ram(), which can't possibly handle vhost-user,
and can't work as expected with '-numa node,memdev' options.
Use MachineState::ram instead of manually initializing RAM memory
region, as well as by providing MachineClass::default_ram_id to
opt in to memdev scheme.
Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---
hw/riscv/opentitan.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c
index 9803ae6d70..c356293d29 100644
--- a/hw/riscv/opentitan.c
+++ b/hw/riscv/opentitan.c
@@ -67,17 +67,14 @@ static void opentitan_board_init(MachineState *machine)
const MemMapEntry *memmap = ibex_memmap;
OpenTitanState *s = g_new0(OpenTitanState, 1);
MemoryRegion *sys_mem = get_system_memory();
- MemoryRegion *main_mem = g_new(MemoryRegion, 1);
/* Initialize SoC */
object_initialize_child(OBJECT(machine), "soc", &s->soc,
TYPE_RISCV_IBEX_SOC);
qdev_realize(DEVICE(&s->soc), NULL, &error_abort);
- memory_region_init_ram(main_mem, NULL, "riscv.lowrisc.ibex.ram",
- memmap[IBEX_DEV_RAM].size, &error_fatal);
memory_region_add_subregion(sys_mem,
- memmap[IBEX_DEV_RAM].base, main_mem);
+ memmap[IBEX_DEV_RAM].base, machine->ram);
if (machine->firmware) {
riscv_load_firmware(machine->firmware, memmap[IBEX_DEV_RAM].base, NULL);
@@ -95,6 +92,7 @@ static void opentitan_machine_init(MachineClass *mc)
mc->init = opentitan_board_init;
mc->max_cpus = 1;
mc->default_cpu_type = TYPE_RISCV_CPU_IBEX;
+ mc->default_ram_id = "riscv.lowrisc.ibex.ram";
}
DEFINE_MACHINE("opentitan", opentitan_machine_init)
--
2.25.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 3/6] hw/riscv: shakti_c: Use MachineState::ram and MachineClass::default_ram_id
2021-10-18 15:38 [PATCH 1/6] hw/riscv: microchip_pfsoc: Use MachineState::ram and MachineClass::default_ram_id Bin Meng
2021-10-18 15:38 ` [PATCH 2/6] hw/riscv: opentitan: " Bin Meng
@ 2021-10-18 15:38 ` Bin Meng
2021-10-19 7:12 ` Igor Mammedov
2021-10-18 15:38 ` [PATCH 4/6] hw/riscv: sifive_e: " Bin Meng
` (4 subsequent siblings)
6 siblings, 1 reply; 27+ messages in thread
From: Bin Meng @ 2021-10-18 15:38 UTC (permalink / raw)
To: Alistair Francis, Igor Mammedov, qemu-devel, qemu-riscv
Using memory_region_init_ram(), which can't possibly handle vhost-user,
and can't work as expected with '-numa node,memdev' options.
Use MachineState::ram instead of manually initializing RAM memory
region, as well as by providing MachineClass::default_ram_id to
opt in to memdev scheme.
Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---
hw/riscv/shakti_c.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/hw/riscv/shakti_c.c b/hw/riscv/shakti_c.c
index d7d1f91fa5..90e2cf609f 100644
--- a/hw/riscv/shakti_c.c
+++ b/hw/riscv/shakti_c.c
@@ -45,7 +45,6 @@ static void shakti_c_machine_state_init(MachineState *mstate)
{
ShaktiCMachineState *sms = RISCV_SHAKTI_MACHINE(mstate);
MemoryRegion *system_memory = get_system_memory();
- MemoryRegion *main_mem = g_new(MemoryRegion, 1);
/* Allow only Shakti C CPU for this platform */
if (strcmp(mstate->cpu_type, TYPE_RISCV_CPU_SHAKTI_C) != 0) {
@@ -59,11 +58,9 @@ static void shakti_c_machine_state_init(MachineState *mstate)
qdev_realize(DEVICE(&sms->soc), NULL, &error_abort);
/* register RAM */
- memory_region_init_ram(main_mem, NULL, "riscv.shakti.c.ram",
- mstate->ram_size, &error_fatal);
memory_region_add_subregion(system_memory,
shakti_c_memmap[SHAKTI_C_RAM].base,
- main_mem);
+ mstate->ram);
/* ROM reset vector */
riscv_setup_rom_reset_vec(mstate, &sms->soc.cpus,
@@ -88,6 +85,7 @@ static void shakti_c_machine_class_init(ObjectClass *klass, void *data)
mc->desc = "RISC-V Board compatible with Shakti SDK";
mc->init = shakti_c_machine_state_init;
mc->default_cpu_type = TYPE_RISCV_CPU_SHAKTI_C;
+ mc->default_ram_id = "riscv.shakti.c.ram";
}
static const TypeInfo shakti_c_machine_type_info = {
--
2.25.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 4/6] hw/riscv: sifive_e: Use MachineState::ram and MachineClass::default_ram_id
2021-10-18 15:38 [PATCH 1/6] hw/riscv: microchip_pfsoc: Use MachineState::ram and MachineClass::default_ram_id Bin Meng
2021-10-18 15:38 ` [PATCH 2/6] hw/riscv: opentitan: " Bin Meng
2021-10-18 15:38 ` [PATCH 3/6] hw/riscv: shakti_c: " Bin Meng
@ 2021-10-18 15:38 ` Bin Meng
2021-10-19 7:15 ` Igor Mammedov
2021-10-18 15:38 ` [PATCH 5/6] hw/riscv: sifive_u: " Bin Meng
` (3 subsequent siblings)
6 siblings, 1 reply; 27+ messages in thread
From: Bin Meng @ 2021-10-18 15:38 UTC (permalink / raw)
To: Alistair Francis, Igor Mammedov, qemu-devel, qemu-riscv
Using memory_region_init_ram(), which can't possibly handle vhost-user,
and can't work as expected with '-numa node,memdev' options.
Use MachineState::ram instead of manually initializing RAM memory
region, as well as by providing MachineClass::default_ram_id to
opt in to memdev scheme.
Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---
hw/riscv/sifive_e.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
index 6e95ea5896..caae860664 100644
--- a/hw/riscv/sifive_e.c
+++ b/hw/riscv/sifive_e.c
@@ -75,7 +75,6 @@ static void sifive_e_machine_init(MachineState *machine)
SiFiveEState *s = RISCV_E_MACHINE(machine);
MemoryRegion *sys_mem = get_system_memory();
- MemoryRegion *main_mem = g_new(MemoryRegion, 1);
int i;
/* Initialize SoC */
@@ -83,10 +82,8 @@ static void sifive_e_machine_init(MachineState *machine)
qdev_realize(DEVICE(&s->soc), NULL, &error_abort);
/* Data Tightly Integrated Memory */
- memory_region_init_ram(main_mem, NULL, "riscv.sifive.e.ram",
- memmap[SIFIVE_E_DEV_DTIM].size, &error_fatal);
memory_region_add_subregion(sys_mem,
- memmap[SIFIVE_E_DEV_DTIM].base, main_mem);
+ memmap[SIFIVE_E_DEV_DTIM].base, machine->ram);
/* Mask ROM reset vector */
uint32_t reset_vec[4];
@@ -142,6 +139,7 @@ static void sifive_e_machine_class_init(ObjectClass *oc, void *data)
mc->init = sifive_e_machine_init;
mc->max_cpus = 1;
mc->default_cpu_type = SIFIVE_E_CPU;
+ mc->default_ram_id = "riscv.sifive.e.ram";
object_class_property_add_bool(oc, "revb", sifive_e_machine_get_revb,
sifive_e_machine_set_revb);
--
2.25.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 5/6] hw/riscv: sifive_u: Use MachineState::ram and MachineClass::default_ram_id
2021-10-18 15:38 [PATCH 1/6] hw/riscv: microchip_pfsoc: Use MachineState::ram and MachineClass::default_ram_id Bin Meng
` (2 preceding siblings ...)
2021-10-18 15:38 ` [PATCH 4/6] hw/riscv: sifive_e: " Bin Meng
@ 2021-10-18 15:38 ` Bin Meng
2021-10-19 7:16 ` Igor Mammedov
2021-10-18 15:38 ` [PATCH 6/6] hw/riscv: spike: " Bin Meng
` (2 subsequent siblings)
6 siblings, 1 reply; 27+ messages in thread
From: Bin Meng @ 2021-10-18 15:38 UTC (permalink / raw)
To: Alistair Francis, Igor Mammedov, qemu-devel, qemu-riscv
Using memory_region_init_ram(), which can't possibly handle vhost-user,
and can't work as expected with '-numa node,memdev' options.
Use MachineState::ram instead of manually initializing RAM memory
region, as well as by providing MachineClass::default_ram_id to
opt in to memdev scheme.
Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---
hw/riscv/sifive_u.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index fc5790b8ce..0217006c27 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -528,7 +528,6 @@ static void sifive_u_machine_init(MachineState *machine)
const MemMapEntry *memmap = sifive_u_memmap;
SiFiveUState *s = RISCV_U_MACHINE(machine);
MemoryRegion *system_memory = get_system_memory();
- MemoryRegion *main_mem = g_new(MemoryRegion, 1);
MemoryRegion *flash0 = g_new(MemoryRegion, 1);
target_ulong start_addr = memmap[SIFIVE_U_DEV_DRAM].base;
target_ulong firmware_end_addr, kernel_start_addr;
@@ -549,10 +548,8 @@ static void sifive_u_machine_init(MachineState *machine)
qdev_realize(DEVICE(&s->soc), NULL, &error_abort);
/* register RAM */
- memory_region_init_ram(main_mem, NULL, "riscv.sifive.u.ram",
- machine->ram_size, &error_fatal);
memory_region_add_subregion(system_memory, memmap[SIFIVE_U_DEV_DRAM].base,
- main_mem);
+ machine->ram);
/* register QSPI0 Flash */
memory_region_init_ram(flash0, NULL, "riscv.sifive.u.flash0",
@@ -748,6 +745,7 @@ static void sifive_u_machine_class_init(ObjectClass *oc, void *data)
mc->min_cpus = SIFIVE_U_MANAGEMENT_CPU_COUNT + 1;
mc->default_cpu_type = SIFIVE_U_CPU;
mc->default_cpus = mc->min_cpus;
+ mc->default_ram_id = "riscv.sifive.u.ram";
object_class_property_add_bool(oc, "start-in-flash",
sifive_u_machine_get_start_in_flash,
--
2.25.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 6/6] hw/riscv: spike: Use MachineState::ram and MachineClass::default_ram_id
2021-10-18 15:38 [PATCH 1/6] hw/riscv: microchip_pfsoc: Use MachineState::ram and MachineClass::default_ram_id Bin Meng
` (3 preceding siblings ...)
2021-10-18 15:38 ` [PATCH 5/6] hw/riscv: sifive_u: " Bin Meng
@ 2021-10-18 15:38 ` Bin Meng
2021-10-19 7:17 ` Igor Mammedov
2021-10-18 15:51 ` [PATCH 1/6] hw/riscv: microchip_pfsoc: " Philippe Mathieu-Daudé
2021-10-19 7:39 ` Igor Mammedov
6 siblings, 1 reply; 27+ messages in thread
From: Bin Meng @ 2021-10-18 15:38 UTC (permalink / raw)
To: Alistair Francis, Igor Mammedov, qemu-devel, qemu-riscv
Using memory_region_init_ram(), which can't possibly handle vhost-user,
and can't work as expected with '-numa node,memdev' options.
Use MachineState::ram instead of manually initializing RAM memory
region, as well as by providing MachineClass::default_ram_id to
opt in to memdev scheme.
Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---
hw/riscv/spike.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index 79ae355ae2..288d69cd9f 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -180,7 +180,6 @@ static void spike_board_init(MachineState *machine)
const MemMapEntry *memmap = spike_memmap;
SpikeState *s = SPIKE_MACHINE(machine);
MemoryRegion *system_memory = get_system_memory();
- MemoryRegion *main_mem = g_new(MemoryRegion, 1);
MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
target_ulong firmware_end_addr, kernel_start_addr;
uint32_t fdt_load_addr;
@@ -239,10 +238,8 @@ static void spike_board_init(MachineState *machine)
}
/* register system main memory (actual RAM) */
- memory_region_init_ram(main_mem, NULL, "riscv.spike.ram",
- machine->ram_size, &error_fatal);
memory_region_add_subregion(system_memory, memmap[SPIKE_DRAM].base,
- main_mem);
+ machine->ram);
/* create device tree */
create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline,
@@ -326,6 +323,7 @@ static void spike_machine_class_init(ObjectClass *oc, void *data)
mc->cpu_index_to_instance_props = riscv_numa_cpu_index_to_props;
mc->get_default_cpu_node_id = riscv_numa_get_default_cpu_node_id;
mc->numa_mem_supported = true;
+ mc->default_ram_id = "riscv.spike.ram";
}
static const TypeInfo spike_machine_typeinfo = {
--
2.25.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH 1/6] hw/riscv: microchip_pfsoc: Use MachineState::ram and MachineClass::default_ram_id
2021-10-18 15:38 [PATCH 1/6] hw/riscv: microchip_pfsoc: Use MachineState::ram and MachineClass::default_ram_id Bin Meng
` (4 preceding siblings ...)
2021-10-18 15:38 ` [PATCH 6/6] hw/riscv: spike: " Bin Meng
@ 2021-10-18 15:51 ` Philippe Mathieu-Daudé
2021-10-18 16:00 ` Bin Meng
2021-10-19 7:39 ` Igor Mammedov
6 siblings, 1 reply; 27+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-10-18 15:51 UTC (permalink / raw)
To: Bin Meng, Alistair Francis, Igor Mammedov, qemu-devel, qemu-riscv
Hi Bin, is there a cover letter?
Series:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
On 10/18/21 17:38, Bin Meng wrote:
> Using memory_region_init_ram(), which can't possibly handle vhost-user,
> and can't work as expected with '-numa node,memdev' options.
>
> Use MachineState::ram instead of manually initializing RAM memory
> region, as well as by providing MachineClass::default_ram_id to
> opt in to memdev scheme.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
> hw/riscv/microchip_pfsoc.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/6] hw/riscv: microchip_pfsoc: Use MachineState::ram and MachineClass::default_ram_id
2021-10-18 15:51 ` [PATCH 1/6] hw/riscv: microchip_pfsoc: " Philippe Mathieu-Daudé
@ 2021-10-18 16:00 ` Bin Meng
0 siblings, 0 replies; 27+ messages in thread
From: Bin Meng @ 2021-10-18 16:00 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Igor Mammedov, Alistair Francis,
qemu-devel@nongnu.org Developers, open list:RISC-V
Hi Philippe,
On Mon, Oct 18, 2021 at 11:51 PM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> Hi Bin, is there a cover letter?
Sorry, I did not include a cover letter. I will need to remember this
next time :)
>
> Series:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>
Thanks for the review!
Regards,
Bin
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/6] hw/riscv: microchip_pfsoc: Use MachineState::ram and MachineClass::default_ram_id
@ 2021-10-18 16:00 ` Bin Meng
0 siblings, 0 replies; 27+ messages in thread
From: Bin Meng @ 2021-10-18 16:00 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Alistair Francis, Igor Mammedov,
qemu-devel@nongnu.org Developers, open list:RISC-V
Hi Philippe,
On Mon, Oct 18, 2021 at 11:51 PM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> Hi Bin, is there a cover letter?
Sorry, I did not include a cover letter. I will need to remember this
next time :)
>
> Series:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>
Thanks for the review!
Regards,
Bin
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/6] hw/riscv: opentitan: Use MachineState::ram and MachineClass::default_ram_id
2021-10-18 15:38 ` [PATCH 2/6] hw/riscv: opentitan: " Bin Meng
@ 2021-10-19 7:11 ` Igor Mammedov
0 siblings, 0 replies; 27+ messages in thread
From: Igor Mammedov @ 2021-10-19 7:11 UTC (permalink / raw)
To: Bin Meng; +Cc: qemu-riscv, Alistair Francis, qemu-devel
On Mon, 18 Oct 2021 23:38:25 +0800
Bin Meng <bmeng.cn@gmail.com> wrote:
> Using memory_region_init_ram(), which can't possibly handle vhost-user,
> and can't work as expected with '-numa node,memdev' options.
>
> Use MachineState::ram instead of manually initializing RAM memory
> region, as well as by providing MachineClass::default_ram_id to
> opt in to memdev scheme.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
> hw/riscv/opentitan.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c
> index 9803ae6d70..c356293d29 100644
> --- a/hw/riscv/opentitan.c
> +++ b/hw/riscv/opentitan.c
> @@ -67,17 +67,14 @@ static void opentitan_board_init(MachineState *machine)
> const MemMapEntry *memmap = ibex_memmap;
> OpenTitanState *s = g_new0(OpenTitanState, 1);
> MemoryRegion *sys_mem = get_system_memory();
> - MemoryRegion *main_mem = g_new(MemoryRegion, 1);
It is likely that you are missing fixed size check here
(looking at code it seems to me that board doesn't support variable RAM size)
See commit 00b9829f83c for example.
> /* Initialize SoC */
> object_initialize_child(OBJECT(machine), "soc", &s->soc,
> TYPE_RISCV_IBEX_SOC);
> qdev_realize(DEVICE(&s->soc), NULL, &error_abort);
>
> - memory_region_init_ram(main_mem, NULL, "riscv.lowrisc.ibex.ram",
> - memmap[IBEX_DEV_RAM].size, &error_fatal);
> memory_region_add_subregion(sys_mem,
> - memmap[IBEX_DEV_RAM].base, main_mem);
> + memmap[IBEX_DEV_RAM].base, machine->ram);
>
> if (machine->firmware) {
> riscv_load_firmware(machine->firmware, memmap[IBEX_DEV_RAM].base, NULL);
> @@ -95,6 +92,7 @@ static void opentitan_machine_init(MachineClass *mc)
> mc->init = opentitan_board_init;
> mc->max_cpus = 1;
> mc->default_cpu_type = TYPE_RISCV_CPU_IBEX;
> + mc->default_ram_id = "riscv.lowrisc.ibex.ram";
Are you missing "mc->default_ram_size = memmap[IBEX_DEV_RAM].size" here?
otherwise it will default to generic:
hw/core/machine.c: mc->default_ram_size = 128 * MiB;
> }
>
> DEFINE_MACHINE("opentitan", opentitan_machine_init)
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/6] hw/riscv: opentitan: Use MachineState::ram and MachineClass::default_ram_id
@ 2021-10-19 7:11 ` Igor Mammedov
0 siblings, 0 replies; 27+ messages in thread
From: Igor Mammedov @ 2021-10-19 7:11 UTC (permalink / raw)
To: Bin Meng; +Cc: Alistair Francis, qemu-devel, qemu-riscv
On Mon, 18 Oct 2021 23:38:25 +0800
Bin Meng <bmeng.cn@gmail.com> wrote:
> Using memory_region_init_ram(), which can't possibly handle vhost-user,
> and can't work as expected with '-numa node,memdev' options.
>
> Use MachineState::ram instead of manually initializing RAM memory
> region, as well as by providing MachineClass::default_ram_id to
> opt in to memdev scheme.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
> hw/riscv/opentitan.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c
> index 9803ae6d70..c356293d29 100644
> --- a/hw/riscv/opentitan.c
> +++ b/hw/riscv/opentitan.c
> @@ -67,17 +67,14 @@ static void opentitan_board_init(MachineState *machine)
> const MemMapEntry *memmap = ibex_memmap;
> OpenTitanState *s = g_new0(OpenTitanState, 1);
> MemoryRegion *sys_mem = get_system_memory();
> - MemoryRegion *main_mem = g_new(MemoryRegion, 1);
It is likely that you are missing fixed size check here
(looking at code it seems to me that board doesn't support variable RAM size)
See commit 00b9829f83c for example.
> /* Initialize SoC */
> object_initialize_child(OBJECT(machine), "soc", &s->soc,
> TYPE_RISCV_IBEX_SOC);
> qdev_realize(DEVICE(&s->soc), NULL, &error_abort);
>
> - memory_region_init_ram(main_mem, NULL, "riscv.lowrisc.ibex.ram",
> - memmap[IBEX_DEV_RAM].size, &error_fatal);
> memory_region_add_subregion(sys_mem,
> - memmap[IBEX_DEV_RAM].base, main_mem);
> + memmap[IBEX_DEV_RAM].base, machine->ram);
>
> if (machine->firmware) {
> riscv_load_firmware(machine->firmware, memmap[IBEX_DEV_RAM].base, NULL);
> @@ -95,6 +92,7 @@ static void opentitan_machine_init(MachineClass *mc)
> mc->init = opentitan_board_init;
> mc->max_cpus = 1;
> mc->default_cpu_type = TYPE_RISCV_CPU_IBEX;
> + mc->default_ram_id = "riscv.lowrisc.ibex.ram";
Are you missing "mc->default_ram_size = memmap[IBEX_DEV_RAM].size" here?
otherwise it will default to generic:
hw/core/machine.c: mc->default_ram_size = 128 * MiB;
> }
>
> DEFINE_MACHINE("opentitan", opentitan_machine_init)
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 3/6] hw/riscv: shakti_c: Use MachineState::ram and MachineClass::default_ram_id
2021-10-18 15:38 ` [PATCH 3/6] hw/riscv: shakti_c: " Bin Meng
@ 2021-10-19 7:12 ` Igor Mammedov
0 siblings, 0 replies; 27+ messages in thread
From: Igor Mammedov @ 2021-10-19 7:12 UTC (permalink / raw)
To: Bin Meng; +Cc: qemu-riscv, Alistair Francis, qemu-devel
On Mon, 18 Oct 2021 23:38:26 +0800
Bin Meng <bmeng.cn@gmail.com> wrote:
> Using memory_region_init_ram(), which can't possibly handle vhost-user,
> and can't work as expected with '-numa node,memdev' options.
>
> Use MachineState::ram instead of manually initializing RAM memory
> region, as well as by providing MachineClass::default_ram_id to
> opt in to memdev scheme.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> ---
>
> hw/riscv/shakti_c.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/hw/riscv/shakti_c.c b/hw/riscv/shakti_c.c
> index d7d1f91fa5..90e2cf609f 100644
> --- a/hw/riscv/shakti_c.c
> +++ b/hw/riscv/shakti_c.c
> @@ -45,7 +45,6 @@ static void shakti_c_machine_state_init(MachineState *mstate)
> {
> ShaktiCMachineState *sms = RISCV_SHAKTI_MACHINE(mstate);
> MemoryRegion *system_memory = get_system_memory();
> - MemoryRegion *main_mem = g_new(MemoryRegion, 1);
>
> /* Allow only Shakti C CPU for this platform */
> if (strcmp(mstate->cpu_type, TYPE_RISCV_CPU_SHAKTI_C) != 0) {
> @@ -59,11 +58,9 @@ static void shakti_c_machine_state_init(MachineState *mstate)
> qdev_realize(DEVICE(&sms->soc), NULL, &error_abort);
>
> /* register RAM */
> - memory_region_init_ram(main_mem, NULL, "riscv.shakti.c.ram",
> - mstate->ram_size, &error_fatal);
> memory_region_add_subregion(system_memory,
> shakti_c_memmap[SHAKTI_C_RAM].base,
> - main_mem);
> + mstate->ram);
>
> /* ROM reset vector */
> riscv_setup_rom_reset_vec(mstate, &sms->soc.cpus,
> @@ -88,6 +85,7 @@ static void shakti_c_machine_class_init(ObjectClass *klass, void *data)
> mc->desc = "RISC-V Board compatible with Shakti SDK";
> mc->init = shakti_c_machine_state_init;
> mc->default_cpu_type = TYPE_RISCV_CPU_SHAKTI_C;
> + mc->default_ram_id = "riscv.shakti.c.ram";
> }
>
> static const TypeInfo shakti_c_machine_type_info = {
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 3/6] hw/riscv: shakti_c: Use MachineState::ram and MachineClass::default_ram_id
@ 2021-10-19 7:12 ` Igor Mammedov
0 siblings, 0 replies; 27+ messages in thread
From: Igor Mammedov @ 2021-10-19 7:12 UTC (permalink / raw)
To: Bin Meng; +Cc: Alistair Francis, qemu-devel, qemu-riscv
On Mon, 18 Oct 2021 23:38:26 +0800
Bin Meng <bmeng.cn@gmail.com> wrote:
> Using memory_region_init_ram(), which can't possibly handle vhost-user,
> and can't work as expected with '-numa node,memdev' options.
>
> Use MachineState::ram instead of manually initializing RAM memory
> region, as well as by providing MachineClass::default_ram_id to
> opt in to memdev scheme.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> ---
>
> hw/riscv/shakti_c.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/hw/riscv/shakti_c.c b/hw/riscv/shakti_c.c
> index d7d1f91fa5..90e2cf609f 100644
> --- a/hw/riscv/shakti_c.c
> +++ b/hw/riscv/shakti_c.c
> @@ -45,7 +45,6 @@ static void shakti_c_machine_state_init(MachineState *mstate)
> {
> ShaktiCMachineState *sms = RISCV_SHAKTI_MACHINE(mstate);
> MemoryRegion *system_memory = get_system_memory();
> - MemoryRegion *main_mem = g_new(MemoryRegion, 1);
>
> /* Allow only Shakti C CPU for this platform */
> if (strcmp(mstate->cpu_type, TYPE_RISCV_CPU_SHAKTI_C) != 0) {
> @@ -59,11 +58,9 @@ static void shakti_c_machine_state_init(MachineState *mstate)
> qdev_realize(DEVICE(&sms->soc), NULL, &error_abort);
>
> /* register RAM */
> - memory_region_init_ram(main_mem, NULL, "riscv.shakti.c.ram",
> - mstate->ram_size, &error_fatal);
> memory_region_add_subregion(system_memory,
> shakti_c_memmap[SHAKTI_C_RAM].base,
> - main_mem);
> + mstate->ram);
>
> /* ROM reset vector */
> riscv_setup_rom_reset_vec(mstate, &sms->soc.cpus,
> @@ -88,6 +85,7 @@ static void shakti_c_machine_class_init(ObjectClass *klass, void *data)
> mc->desc = "RISC-V Board compatible with Shakti SDK";
> mc->init = shakti_c_machine_state_init;
> mc->default_cpu_type = TYPE_RISCV_CPU_SHAKTI_C;
> + mc->default_ram_id = "riscv.shakti.c.ram";
> }
>
> static const TypeInfo shakti_c_machine_type_info = {
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 4/6] hw/riscv: sifive_e: Use MachineState::ram and MachineClass::default_ram_id
2021-10-18 15:38 ` [PATCH 4/6] hw/riscv: sifive_e: " Bin Meng
@ 2021-10-19 7:15 ` Igor Mammedov
0 siblings, 0 replies; 27+ messages in thread
From: Igor Mammedov @ 2021-10-19 7:15 UTC (permalink / raw)
To: Bin Meng; +Cc: qemu-riscv, Alistair Francis, qemu-devel
On Mon, 18 Oct 2021 23:38:27 +0800
Bin Meng <bmeng.cn@gmail.com> wrote:
> Using memory_region_init_ram(), which can't possibly handle vhost-user,
> and can't work as expected with '-numa node,memdev' options.
>
> Use MachineState::ram instead of manually initializing RAM memory
> region, as well as by providing MachineClass::default_ram_id to
> opt in to memdev scheme.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
same issues as in 2/6
> ---
>
> hw/riscv/sifive_e.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
> index 6e95ea5896..caae860664 100644
> --- a/hw/riscv/sifive_e.c
> +++ b/hw/riscv/sifive_e.c
> @@ -75,7 +75,6 @@ static void sifive_e_machine_init(MachineState *machine)
>
> SiFiveEState *s = RISCV_E_MACHINE(machine);
> MemoryRegion *sys_mem = get_system_memory();
> - MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> int i;
>
> /* Initialize SoC */
> @@ -83,10 +82,8 @@ static void sifive_e_machine_init(MachineState *machine)
> qdev_realize(DEVICE(&s->soc), NULL, &error_abort);
>
> /* Data Tightly Integrated Memory */
> - memory_region_init_ram(main_mem, NULL, "riscv.sifive.e.ram",
> - memmap[SIFIVE_E_DEV_DTIM].size, &error_fatal);
> memory_region_add_subregion(sys_mem,
> - memmap[SIFIVE_E_DEV_DTIM].base, main_mem);
> + memmap[SIFIVE_E_DEV_DTIM].base, machine->ram);
>
> /* Mask ROM reset vector */
> uint32_t reset_vec[4];
> @@ -142,6 +139,7 @@ static void sifive_e_machine_class_init(ObjectClass *oc, void *data)
> mc->init = sifive_e_machine_init;
> mc->max_cpus = 1;
> mc->default_cpu_type = SIFIVE_E_CPU;
> + mc->default_ram_id = "riscv.sifive.e.ram";
>
> object_class_property_add_bool(oc, "revb", sifive_e_machine_get_revb,
> sifive_e_machine_set_revb);
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 4/6] hw/riscv: sifive_e: Use MachineState::ram and MachineClass::default_ram_id
@ 2021-10-19 7:15 ` Igor Mammedov
0 siblings, 0 replies; 27+ messages in thread
From: Igor Mammedov @ 2021-10-19 7:15 UTC (permalink / raw)
To: Bin Meng; +Cc: Alistair Francis, qemu-devel, qemu-riscv
On Mon, 18 Oct 2021 23:38:27 +0800
Bin Meng <bmeng.cn@gmail.com> wrote:
> Using memory_region_init_ram(), which can't possibly handle vhost-user,
> and can't work as expected with '-numa node,memdev' options.
>
> Use MachineState::ram instead of manually initializing RAM memory
> region, as well as by providing MachineClass::default_ram_id to
> opt in to memdev scheme.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
same issues as in 2/6
> ---
>
> hw/riscv/sifive_e.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
> index 6e95ea5896..caae860664 100644
> --- a/hw/riscv/sifive_e.c
> +++ b/hw/riscv/sifive_e.c
> @@ -75,7 +75,6 @@ static void sifive_e_machine_init(MachineState *machine)
>
> SiFiveEState *s = RISCV_E_MACHINE(machine);
> MemoryRegion *sys_mem = get_system_memory();
> - MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> int i;
>
> /* Initialize SoC */
> @@ -83,10 +82,8 @@ static void sifive_e_machine_init(MachineState *machine)
> qdev_realize(DEVICE(&s->soc), NULL, &error_abort);
>
> /* Data Tightly Integrated Memory */
> - memory_region_init_ram(main_mem, NULL, "riscv.sifive.e.ram",
> - memmap[SIFIVE_E_DEV_DTIM].size, &error_fatal);
> memory_region_add_subregion(sys_mem,
> - memmap[SIFIVE_E_DEV_DTIM].base, main_mem);
> + memmap[SIFIVE_E_DEV_DTIM].base, machine->ram);
>
> /* Mask ROM reset vector */
> uint32_t reset_vec[4];
> @@ -142,6 +139,7 @@ static void sifive_e_machine_class_init(ObjectClass *oc, void *data)
> mc->init = sifive_e_machine_init;
> mc->max_cpus = 1;
> mc->default_cpu_type = SIFIVE_E_CPU;
> + mc->default_ram_id = "riscv.sifive.e.ram";
>
> object_class_property_add_bool(oc, "revb", sifive_e_machine_get_revb,
> sifive_e_machine_set_revb);
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 5/6] hw/riscv: sifive_u: Use MachineState::ram and MachineClass::default_ram_id
2021-10-18 15:38 ` [PATCH 5/6] hw/riscv: sifive_u: " Bin Meng
@ 2021-10-19 7:16 ` Igor Mammedov
0 siblings, 0 replies; 27+ messages in thread
From: Igor Mammedov @ 2021-10-19 7:16 UTC (permalink / raw)
To: Bin Meng; +Cc: qemu-riscv, Alistair Francis, qemu-devel
On Mon, 18 Oct 2021 23:38:28 +0800
Bin Meng <bmeng.cn@gmail.com> wrote:
> Using memory_region_init_ram(), which can't possibly handle vhost-user,
> and can't work as expected with '-numa node,memdev' options.
>
> Use MachineState::ram instead of manually initializing RAM memory
> region, as well as by providing MachineClass::default_ram_id to
> opt in to memdev scheme.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> ---
>
> hw/riscv/sifive_u.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index fc5790b8ce..0217006c27 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -528,7 +528,6 @@ static void sifive_u_machine_init(MachineState *machine)
> const MemMapEntry *memmap = sifive_u_memmap;
> SiFiveUState *s = RISCV_U_MACHINE(machine);
> MemoryRegion *system_memory = get_system_memory();
> - MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> MemoryRegion *flash0 = g_new(MemoryRegion, 1);
> target_ulong start_addr = memmap[SIFIVE_U_DEV_DRAM].base;
> target_ulong firmware_end_addr, kernel_start_addr;
> @@ -549,10 +548,8 @@ static void sifive_u_machine_init(MachineState *machine)
> qdev_realize(DEVICE(&s->soc), NULL, &error_abort);
>
> /* register RAM */
> - memory_region_init_ram(main_mem, NULL, "riscv.sifive.u.ram",
> - machine->ram_size, &error_fatal);
> memory_region_add_subregion(system_memory, memmap[SIFIVE_U_DEV_DRAM].base,
> - main_mem);
> + machine->ram);
>
> /* register QSPI0 Flash */
> memory_region_init_ram(flash0, NULL, "riscv.sifive.u.flash0",
> @@ -748,6 +745,7 @@ static void sifive_u_machine_class_init(ObjectClass *oc, void *data)
> mc->min_cpus = SIFIVE_U_MANAGEMENT_CPU_COUNT + 1;
> mc->default_cpu_type = SIFIVE_U_CPU;
> mc->default_cpus = mc->min_cpus;
> + mc->default_ram_id = "riscv.sifive.u.ram";
>
> object_class_property_add_bool(oc, "start-in-flash",
> sifive_u_machine_get_start_in_flash,
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 5/6] hw/riscv: sifive_u: Use MachineState::ram and MachineClass::default_ram_id
@ 2021-10-19 7:16 ` Igor Mammedov
0 siblings, 0 replies; 27+ messages in thread
From: Igor Mammedov @ 2021-10-19 7:16 UTC (permalink / raw)
To: Bin Meng; +Cc: Alistair Francis, qemu-devel, qemu-riscv
On Mon, 18 Oct 2021 23:38:28 +0800
Bin Meng <bmeng.cn@gmail.com> wrote:
> Using memory_region_init_ram(), which can't possibly handle vhost-user,
> and can't work as expected with '-numa node,memdev' options.
>
> Use MachineState::ram instead of manually initializing RAM memory
> region, as well as by providing MachineClass::default_ram_id to
> opt in to memdev scheme.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> ---
>
> hw/riscv/sifive_u.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index fc5790b8ce..0217006c27 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -528,7 +528,6 @@ static void sifive_u_machine_init(MachineState *machine)
> const MemMapEntry *memmap = sifive_u_memmap;
> SiFiveUState *s = RISCV_U_MACHINE(machine);
> MemoryRegion *system_memory = get_system_memory();
> - MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> MemoryRegion *flash0 = g_new(MemoryRegion, 1);
> target_ulong start_addr = memmap[SIFIVE_U_DEV_DRAM].base;
> target_ulong firmware_end_addr, kernel_start_addr;
> @@ -549,10 +548,8 @@ static void sifive_u_machine_init(MachineState *machine)
> qdev_realize(DEVICE(&s->soc), NULL, &error_abort);
>
> /* register RAM */
> - memory_region_init_ram(main_mem, NULL, "riscv.sifive.u.ram",
> - machine->ram_size, &error_fatal);
> memory_region_add_subregion(system_memory, memmap[SIFIVE_U_DEV_DRAM].base,
> - main_mem);
> + machine->ram);
>
> /* register QSPI0 Flash */
> memory_region_init_ram(flash0, NULL, "riscv.sifive.u.flash0",
> @@ -748,6 +745,7 @@ static void sifive_u_machine_class_init(ObjectClass *oc, void *data)
> mc->min_cpus = SIFIVE_U_MANAGEMENT_CPU_COUNT + 1;
> mc->default_cpu_type = SIFIVE_U_CPU;
> mc->default_cpus = mc->min_cpus;
> + mc->default_ram_id = "riscv.sifive.u.ram";
>
> object_class_property_add_bool(oc, "start-in-flash",
> sifive_u_machine_get_start_in_flash,
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 6/6] hw/riscv: spike: Use MachineState::ram and MachineClass::default_ram_id
2021-10-18 15:38 ` [PATCH 6/6] hw/riscv: spike: " Bin Meng
@ 2021-10-19 7:17 ` Igor Mammedov
0 siblings, 0 replies; 27+ messages in thread
From: Igor Mammedov @ 2021-10-19 7:17 UTC (permalink / raw)
To: Bin Meng; +Cc: qemu-riscv, Alistair Francis, qemu-devel
On Mon, 18 Oct 2021 23:38:29 +0800
Bin Meng <bmeng.cn@gmail.com> wrote:
> Using memory_region_init_ram(), which can't possibly handle vhost-user,
> and can't work as expected with '-numa node,memdev' options.
>
> Use MachineState::ram instead of manually initializing RAM memory
> region, as well as by providing MachineClass::default_ram_id to
> opt in to memdev scheme.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> ---
>
> hw/riscv/spike.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> index 79ae355ae2..288d69cd9f 100644
> --- a/hw/riscv/spike.c
> +++ b/hw/riscv/spike.c
> @@ -180,7 +180,6 @@ static void spike_board_init(MachineState *machine)
> const MemMapEntry *memmap = spike_memmap;
> SpikeState *s = SPIKE_MACHINE(machine);
> MemoryRegion *system_memory = get_system_memory();
> - MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> target_ulong firmware_end_addr, kernel_start_addr;
> uint32_t fdt_load_addr;
> @@ -239,10 +238,8 @@ static void spike_board_init(MachineState *machine)
> }
>
> /* register system main memory (actual RAM) */
> - memory_region_init_ram(main_mem, NULL, "riscv.spike.ram",
> - machine->ram_size, &error_fatal);
> memory_region_add_subregion(system_memory, memmap[SPIKE_DRAM].base,
> - main_mem);
> + machine->ram);
>
> /* create device tree */
> create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline,
> @@ -326,6 +323,7 @@ static void spike_machine_class_init(ObjectClass *oc, void *data)
> mc->cpu_index_to_instance_props = riscv_numa_cpu_index_to_props;
> mc->get_default_cpu_node_id = riscv_numa_get_default_cpu_node_id;
> mc->numa_mem_supported = true;
> + mc->default_ram_id = "riscv.spike.ram";
> }
>
> static const TypeInfo spike_machine_typeinfo = {
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 6/6] hw/riscv: spike: Use MachineState::ram and MachineClass::default_ram_id
@ 2021-10-19 7:17 ` Igor Mammedov
0 siblings, 0 replies; 27+ messages in thread
From: Igor Mammedov @ 2021-10-19 7:17 UTC (permalink / raw)
To: Bin Meng; +Cc: Alistair Francis, qemu-devel, qemu-riscv
On Mon, 18 Oct 2021 23:38:29 +0800
Bin Meng <bmeng.cn@gmail.com> wrote:
> Using memory_region_init_ram(), which can't possibly handle vhost-user,
> and can't work as expected with '-numa node,memdev' options.
>
> Use MachineState::ram instead of manually initializing RAM memory
> region, as well as by providing MachineClass::default_ram_id to
> opt in to memdev scheme.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> ---
>
> hw/riscv/spike.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> index 79ae355ae2..288d69cd9f 100644
> --- a/hw/riscv/spike.c
> +++ b/hw/riscv/spike.c
> @@ -180,7 +180,6 @@ static void spike_board_init(MachineState *machine)
> const MemMapEntry *memmap = spike_memmap;
> SpikeState *s = SPIKE_MACHINE(machine);
> MemoryRegion *system_memory = get_system_memory();
> - MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> target_ulong firmware_end_addr, kernel_start_addr;
> uint32_t fdt_load_addr;
> @@ -239,10 +238,8 @@ static void spike_board_init(MachineState *machine)
> }
>
> /* register system main memory (actual RAM) */
> - memory_region_init_ram(main_mem, NULL, "riscv.spike.ram",
> - machine->ram_size, &error_fatal);
> memory_region_add_subregion(system_memory, memmap[SPIKE_DRAM].base,
> - main_mem);
> + machine->ram);
>
> /* create device tree */
> create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline,
> @@ -326,6 +323,7 @@ static void spike_machine_class_init(ObjectClass *oc, void *data)
> mc->cpu_index_to_instance_props = riscv_numa_cpu_index_to_props;
> mc->get_default_cpu_node_id = riscv_numa_get_default_cpu_node_id;
> mc->numa_mem_supported = true;
> + mc->default_ram_id = "riscv.spike.ram";
> }
>
> static const TypeInfo spike_machine_typeinfo = {
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/6] hw/riscv: microchip_pfsoc: Use MachineState::ram and MachineClass::default_ram_id
2021-10-18 15:38 [PATCH 1/6] hw/riscv: microchip_pfsoc: Use MachineState::ram and MachineClass::default_ram_id Bin Meng
@ 2021-10-19 7:39 ` Igor Mammedov
2021-10-18 15:38 ` [PATCH 3/6] hw/riscv: shakti_c: " Bin Meng
` (5 subsequent siblings)
6 siblings, 0 replies; 27+ messages in thread
From: Igor Mammedov @ 2021-10-19 7:39 UTC (permalink / raw)
To: Bin Meng; +Cc: qemu-riscv, Alistair Francis, qemu-devel
On Mon, 18 Oct 2021 23:38:24 +0800
Bin Meng <bmeng.cn@gmail.com> wrote:
> Using memory_region_init_ram(), which can't possibly handle vhost-user,
> and can't work as expected with '-numa node,memdev' options.
>
> Use MachineState::ram instead of manually initializing RAM memory
> region, as well as by providing MachineClass::default_ram_id to
> opt in to memdev scheme.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
> hw/riscv/microchip_pfsoc.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
> index e475b6d511..f10f55b488 100644
> --- a/hw/riscv/microchip_pfsoc.c
> +++ b/hw/riscv/microchip_pfsoc.c
> @@ -459,7 +459,6 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
> const MemMapEntry *memmap = microchip_pfsoc_memmap;
> MicrochipIcicleKitState *s = MICROCHIP_ICICLE_KIT_MACHINE(machine);
> MemoryRegion *system_memory = get_system_memory();
> - MemoryRegion *mem_low = g_new(MemoryRegion, 1);
> MemoryRegion *mem_low_alias = g_new(MemoryRegion, 1);
> MemoryRegion *mem_high = g_new(MemoryRegion, 1);
> MemoryRegion *mem_high_alias = g_new(MemoryRegion, 1);
> @@ -486,16 +485,13 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
> qdev_realize(DEVICE(&s->soc), NULL, &error_abort);
>
> /* Register RAM */
> - memory_region_init_ram(mem_low, NULL, "microchip.icicle.kit.ram_low",
> - memmap[MICROCHIP_PFSOC_DRAM_LO].size,
> - &error_fatal);
> memory_region_init_alias(mem_low_alias, NULL,
> "microchip.icicle.kit.ram_low.alias",
> - mem_low, 0,
> + machine->ram, 0,
> memmap[MICROCHIP_PFSOC_DRAM_LO_ALIAS].size);
> memory_region_add_subregion(system_memory,
> memmap[MICROCHIP_PFSOC_DRAM_LO].base,
> - mem_low);
> + machine->ram);
> memory_region_add_subregion(system_memory,
> memmap[MICROCHIP_PFSOC_DRAM_LO_ALIAS].base,
> mem_low_alias);
looking at code it seems RAM is split between low and high regions,
so converting only low region is wrong.
I'd suggest something similar to 2dc9ce13d210 : taihu_405ep_init().
i.e. ms->ram should hold whole RAM that is split between low and high
using aliases.
> @@ -606,6 +602,7 @@ static void microchip_icicle_kit_machine_class_init(ObjectClass *oc, void *data)
> MICROCHIP_PFSOC_COMPUTE_CPU_COUNT;
> mc->min_cpus = MICROCHIP_PFSOC_MANAGEMENT_CPU_COUNT + 1;
> mc->default_cpus = mc->min_cpus;
> + mc->default_ram_id = "microchip.icicle.kit.ram_low";
given it is not versioned machine, so we don't have to worry about
cross version migration here,
so I'd use "microchip.icicle.kit.ram" for the name here
and currently used "microchip.icicle.kit.ram_low" for corresponding alias
>
> /*
> * Map 513 MiB high memory, the mimimum required high memory size, because
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/6] hw/riscv: microchip_pfsoc: Use MachineState::ram and MachineClass::default_ram_id
@ 2021-10-19 7:39 ` Igor Mammedov
0 siblings, 0 replies; 27+ messages in thread
From: Igor Mammedov @ 2021-10-19 7:39 UTC (permalink / raw)
To: Bin Meng; +Cc: Alistair Francis, qemu-devel, qemu-riscv
On Mon, 18 Oct 2021 23:38:24 +0800
Bin Meng <bmeng.cn@gmail.com> wrote:
> Using memory_region_init_ram(), which can't possibly handle vhost-user,
> and can't work as expected with '-numa node,memdev' options.
>
> Use MachineState::ram instead of manually initializing RAM memory
> region, as well as by providing MachineClass::default_ram_id to
> opt in to memdev scheme.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
> hw/riscv/microchip_pfsoc.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
> index e475b6d511..f10f55b488 100644
> --- a/hw/riscv/microchip_pfsoc.c
> +++ b/hw/riscv/microchip_pfsoc.c
> @@ -459,7 +459,6 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
> const MemMapEntry *memmap = microchip_pfsoc_memmap;
> MicrochipIcicleKitState *s = MICROCHIP_ICICLE_KIT_MACHINE(machine);
> MemoryRegion *system_memory = get_system_memory();
> - MemoryRegion *mem_low = g_new(MemoryRegion, 1);
> MemoryRegion *mem_low_alias = g_new(MemoryRegion, 1);
> MemoryRegion *mem_high = g_new(MemoryRegion, 1);
> MemoryRegion *mem_high_alias = g_new(MemoryRegion, 1);
> @@ -486,16 +485,13 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
> qdev_realize(DEVICE(&s->soc), NULL, &error_abort);
>
> /* Register RAM */
> - memory_region_init_ram(mem_low, NULL, "microchip.icicle.kit.ram_low",
> - memmap[MICROCHIP_PFSOC_DRAM_LO].size,
> - &error_fatal);
> memory_region_init_alias(mem_low_alias, NULL,
> "microchip.icicle.kit.ram_low.alias",
> - mem_low, 0,
> + machine->ram, 0,
> memmap[MICROCHIP_PFSOC_DRAM_LO_ALIAS].size);
> memory_region_add_subregion(system_memory,
> memmap[MICROCHIP_PFSOC_DRAM_LO].base,
> - mem_low);
> + machine->ram);
> memory_region_add_subregion(system_memory,
> memmap[MICROCHIP_PFSOC_DRAM_LO_ALIAS].base,
> mem_low_alias);
looking at code it seems RAM is split between low and high regions,
so converting only low region is wrong.
I'd suggest something similar to 2dc9ce13d210 : taihu_405ep_init().
i.e. ms->ram should hold whole RAM that is split between low and high
using aliases.
> @@ -606,6 +602,7 @@ static void microchip_icicle_kit_machine_class_init(ObjectClass *oc, void *data)
> MICROCHIP_PFSOC_COMPUTE_CPU_COUNT;
> mc->min_cpus = MICROCHIP_PFSOC_MANAGEMENT_CPU_COUNT + 1;
> mc->default_cpus = mc->min_cpus;
> + mc->default_ram_id = "microchip.icicle.kit.ram_low";
given it is not versioned machine, so we don't have to worry about
cross version migration here,
so I'd use "microchip.icicle.kit.ram" for the name here
and currently used "microchip.icicle.kit.ram_low" for corresponding alias
>
> /*
> * Map 513 MiB high memory, the mimimum required high memory size, because
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/6] hw/riscv: opentitan: Use MachineState::ram and MachineClass::default_ram_id
2021-10-19 7:11 ` Igor Mammedov
@ 2021-10-19 12:57 ` Bin Meng
-1 siblings, 0 replies; 27+ messages in thread
From: Bin Meng @ 2021-10-19 12:57 UTC (permalink / raw)
To: Igor Mammedov
Cc: open list:RISC-V, Alistair Francis, qemu-devel@nongnu.org Developers
Hi Igor,
On Tue, Oct 19, 2021 at 3:11 PM Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Mon, 18 Oct 2021 23:38:25 +0800
> Bin Meng <bmeng.cn@gmail.com> wrote:
>
> > Using memory_region_init_ram(), which can't possibly handle vhost-user,
> > and can't work as expected with '-numa node,memdev' options.
> >
> > Use MachineState::ram instead of manually initializing RAM memory
> > region, as well as by providing MachineClass::default_ram_id to
> > opt in to memdev scheme.
> >
> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > ---
> >
> > hw/riscv/opentitan.c | 6 ++----
> > 1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c
> > index 9803ae6d70..c356293d29 100644
> > --- a/hw/riscv/opentitan.c
> > +++ b/hw/riscv/opentitan.c
> > @@ -67,17 +67,14 @@ static void opentitan_board_init(MachineState *machine)
> > const MemMapEntry *memmap = ibex_memmap;
> > OpenTitanState *s = g_new0(OpenTitanState, 1);
> > MemoryRegion *sys_mem = get_system_memory();
> > - MemoryRegion *main_mem = g_new(MemoryRegion, 1);
>
> It is likely that you are missing fixed size check here
> (looking at code it seems to me that board doesn't support variable RAM size)
> See commit 00b9829f83c for example.
Yep
>
> > /* Initialize SoC */
> > object_initialize_child(OBJECT(machine), "soc", &s->soc,
> > TYPE_RISCV_IBEX_SOC);
> > qdev_realize(DEVICE(&s->soc), NULL, &error_abort);
> >
> > - memory_region_init_ram(main_mem, NULL, "riscv.lowrisc.ibex.ram",
> > - memmap[IBEX_DEV_RAM].size, &error_fatal);
> > memory_region_add_subregion(sys_mem,
> > - memmap[IBEX_DEV_RAM].base, main_mem);
> > + memmap[IBEX_DEV_RAM].base, machine->ram);
> >
> > if (machine->firmware) {
> > riscv_load_firmware(machine->firmware, memmap[IBEX_DEV_RAM].base, NULL);
> > @@ -95,6 +92,7 @@ static void opentitan_machine_init(MachineClass *mc)
> > mc->init = opentitan_board_init;
> > mc->max_cpus = 1;
> > mc->default_cpu_type = TYPE_RISCV_CPU_IBEX;
> > + mc->default_ram_id = "riscv.lowrisc.ibex.ram";
>
> Are you missing "mc->default_ram_size = memmap[IBEX_DEV_RAM].size" here?
Indeed, thanks for the review!
>
> otherwise it will default to generic:
> hw/core/machine.c: mc->default_ram_size = 128 * MiB;
>
> > }
> >
> > DEFINE_MACHINE("opentitan", opentitan_machine_init)
>
Regards,
Bin
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/6] hw/riscv: opentitan: Use MachineState::ram and MachineClass::default_ram_id
@ 2021-10-19 12:57 ` Bin Meng
0 siblings, 0 replies; 27+ messages in thread
From: Bin Meng @ 2021-10-19 12:57 UTC (permalink / raw)
To: Igor Mammedov
Cc: Alistair Francis, qemu-devel@nongnu.org Developers, open list:RISC-V
Hi Igor,
On Tue, Oct 19, 2021 at 3:11 PM Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Mon, 18 Oct 2021 23:38:25 +0800
> Bin Meng <bmeng.cn@gmail.com> wrote:
>
> > Using memory_region_init_ram(), which can't possibly handle vhost-user,
> > and can't work as expected with '-numa node,memdev' options.
> >
> > Use MachineState::ram instead of manually initializing RAM memory
> > region, as well as by providing MachineClass::default_ram_id to
> > opt in to memdev scheme.
> >
> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > ---
> >
> > hw/riscv/opentitan.c | 6 ++----
> > 1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c
> > index 9803ae6d70..c356293d29 100644
> > --- a/hw/riscv/opentitan.c
> > +++ b/hw/riscv/opentitan.c
> > @@ -67,17 +67,14 @@ static void opentitan_board_init(MachineState *machine)
> > const MemMapEntry *memmap = ibex_memmap;
> > OpenTitanState *s = g_new0(OpenTitanState, 1);
> > MemoryRegion *sys_mem = get_system_memory();
> > - MemoryRegion *main_mem = g_new(MemoryRegion, 1);
>
> It is likely that you are missing fixed size check here
> (looking at code it seems to me that board doesn't support variable RAM size)
> See commit 00b9829f83c for example.
Yep
>
> > /* Initialize SoC */
> > object_initialize_child(OBJECT(machine), "soc", &s->soc,
> > TYPE_RISCV_IBEX_SOC);
> > qdev_realize(DEVICE(&s->soc), NULL, &error_abort);
> >
> > - memory_region_init_ram(main_mem, NULL, "riscv.lowrisc.ibex.ram",
> > - memmap[IBEX_DEV_RAM].size, &error_fatal);
> > memory_region_add_subregion(sys_mem,
> > - memmap[IBEX_DEV_RAM].base, main_mem);
> > + memmap[IBEX_DEV_RAM].base, machine->ram);
> >
> > if (machine->firmware) {
> > riscv_load_firmware(machine->firmware, memmap[IBEX_DEV_RAM].base, NULL);
> > @@ -95,6 +92,7 @@ static void opentitan_machine_init(MachineClass *mc)
> > mc->init = opentitan_board_init;
> > mc->max_cpus = 1;
> > mc->default_cpu_type = TYPE_RISCV_CPU_IBEX;
> > + mc->default_ram_id = "riscv.lowrisc.ibex.ram";
>
> Are you missing "mc->default_ram_size = memmap[IBEX_DEV_RAM].size" here?
Indeed, thanks for the review!
>
> otherwise it will default to generic:
> hw/core/machine.c: mc->default_ram_size = 128 * MiB;
>
> > }
> >
> > DEFINE_MACHINE("opentitan", opentitan_machine_init)
>
Regards,
Bin
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/6] hw/riscv: microchip_pfsoc: Use MachineState::ram and MachineClass::default_ram_id
2021-10-19 7:39 ` Igor Mammedov
@ 2021-10-20 1:55 ` Bin Meng
-1 siblings, 0 replies; 27+ messages in thread
From: Bin Meng @ 2021-10-20 1:55 UTC (permalink / raw)
To: Igor Mammedov
Cc: open list:RISC-V, Alistair Francis, qemu-devel@nongnu.org Developers
Hi Igor,
On Tue, Oct 19, 2021 at 3:39 PM Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Mon, 18 Oct 2021 23:38:24 +0800
> Bin Meng <bmeng.cn@gmail.com> wrote:
>
> > Using memory_region_init_ram(), which can't possibly handle vhost-user,
> > and can't work as expected with '-numa node,memdev' options.
> >
> > Use MachineState::ram instead of manually initializing RAM memory
> > region, as well as by providing MachineClass::default_ram_id to
> > opt in to memdev scheme.
> >
> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > ---
> >
> > hw/riscv/microchip_pfsoc.c | 9 +++------
> > 1 file changed, 3 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
> > index e475b6d511..f10f55b488 100644
> > --- a/hw/riscv/microchip_pfsoc.c
> > +++ b/hw/riscv/microchip_pfsoc.c
> > @@ -459,7 +459,6 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
> > const MemMapEntry *memmap = microchip_pfsoc_memmap;
> > MicrochipIcicleKitState *s = MICROCHIP_ICICLE_KIT_MACHINE(machine);
> > MemoryRegion *system_memory = get_system_memory();
> > - MemoryRegion *mem_low = g_new(MemoryRegion, 1);
> > MemoryRegion *mem_low_alias = g_new(MemoryRegion, 1);
> > MemoryRegion *mem_high = g_new(MemoryRegion, 1);
> > MemoryRegion *mem_high_alias = g_new(MemoryRegion, 1);
> > @@ -486,16 +485,13 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
> > qdev_realize(DEVICE(&s->soc), NULL, &error_abort);
> >
> > /* Register RAM */
> > - memory_region_init_ram(mem_low, NULL, "microchip.icicle.kit.ram_low",
> > - memmap[MICROCHIP_PFSOC_DRAM_LO].size,
> > - &error_fatal);
> > memory_region_init_alias(mem_low_alias, NULL,
> > "microchip.icicle.kit.ram_low.alias",
> > - mem_low, 0,
> > + machine->ram, 0,
> > memmap[MICROCHIP_PFSOC_DRAM_LO_ALIAS].size);
> > memory_region_add_subregion(system_memory,
> > memmap[MICROCHIP_PFSOC_DRAM_LO].base,
> > - mem_low);
> > + machine->ram);
> > memory_region_add_subregion(system_memory,
> > memmap[MICROCHIP_PFSOC_DRAM_LO_ALIAS].base,
> > mem_low_alias);
>
> looking at code it seems RAM is split between low and high regions,
> so converting only low region is wrong.
>
> I'd suggest something similar to 2dc9ce13d210 : taihu_405ep_init().
> i.e. ms->ram should hold whole RAM that is split between low and high
> using aliases.
Thank you for your pointers. I have just sent v2.
One note when looking at the taihu_405ep_init() implementation, the
following looks incorrect to me:
memory_region_init_alias(&ram_memories[1], NULL,
"taihu_405ep.ram-1", machine->ram, ram_bases[1],
ram_sizes[1]);
I think the 'offset' should be ram_sizes[0] instead of ram_bases[1],
although their values are the same which means they are two contiguous
regions, so it happens to work. But I might be nitpicking ...
>
> > @@ -606,6 +602,7 @@ static void microchip_icicle_kit_machine_class_init(ObjectClass *oc, void *data)
> > MICROCHIP_PFSOC_COMPUTE_CPU_COUNT;
> > mc->min_cpus = MICROCHIP_PFSOC_MANAGEMENT_CPU_COUNT + 1;
> > mc->default_cpus = mc->min_cpus;
> > + mc->default_ram_id = "microchip.icicle.kit.ram_low";
>
> given it is not versioned machine, so we don't have to worry about
> cross version migration here,
> so I'd use "microchip.icicle.kit.ram" for the name here
> and currently used "microchip.icicle.kit.ram_low" for corresponding alias
Regards,
Bin
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/6] hw/riscv: microchip_pfsoc: Use MachineState::ram and MachineClass::default_ram_id
@ 2021-10-20 1:55 ` Bin Meng
0 siblings, 0 replies; 27+ messages in thread
From: Bin Meng @ 2021-10-20 1:55 UTC (permalink / raw)
To: Igor Mammedov
Cc: Alistair Francis, qemu-devel@nongnu.org Developers, open list:RISC-V
Hi Igor,
On Tue, Oct 19, 2021 at 3:39 PM Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Mon, 18 Oct 2021 23:38:24 +0800
> Bin Meng <bmeng.cn@gmail.com> wrote:
>
> > Using memory_region_init_ram(), which can't possibly handle vhost-user,
> > and can't work as expected with '-numa node,memdev' options.
> >
> > Use MachineState::ram instead of manually initializing RAM memory
> > region, as well as by providing MachineClass::default_ram_id to
> > opt in to memdev scheme.
> >
> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > ---
> >
> > hw/riscv/microchip_pfsoc.c | 9 +++------
> > 1 file changed, 3 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
> > index e475b6d511..f10f55b488 100644
> > --- a/hw/riscv/microchip_pfsoc.c
> > +++ b/hw/riscv/microchip_pfsoc.c
> > @@ -459,7 +459,6 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
> > const MemMapEntry *memmap = microchip_pfsoc_memmap;
> > MicrochipIcicleKitState *s = MICROCHIP_ICICLE_KIT_MACHINE(machine);
> > MemoryRegion *system_memory = get_system_memory();
> > - MemoryRegion *mem_low = g_new(MemoryRegion, 1);
> > MemoryRegion *mem_low_alias = g_new(MemoryRegion, 1);
> > MemoryRegion *mem_high = g_new(MemoryRegion, 1);
> > MemoryRegion *mem_high_alias = g_new(MemoryRegion, 1);
> > @@ -486,16 +485,13 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
> > qdev_realize(DEVICE(&s->soc), NULL, &error_abort);
> >
> > /* Register RAM */
> > - memory_region_init_ram(mem_low, NULL, "microchip.icicle.kit.ram_low",
> > - memmap[MICROCHIP_PFSOC_DRAM_LO].size,
> > - &error_fatal);
> > memory_region_init_alias(mem_low_alias, NULL,
> > "microchip.icicle.kit.ram_low.alias",
> > - mem_low, 0,
> > + machine->ram, 0,
> > memmap[MICROCHIP_PFSOC_DRAM_LO_ALIAS].size);
> > memory_region_add_subregion(system_memory,
> > memmap[MICROCHIP_PFSOC_DRAM_LO].base,
> > - mem_low);
> > + machine->ram);
> > memory_region_add_subregion(system_memory,
> > memmap[MICROCHIP_PFSOC_DRAM_LO_ALIAS].base,
> > mem_low_alias);
>
> looking at code it seems RAM is split between low and high regions,
> so converting only low region is wrong.
>
> I'd suggest something similar to 2dc9ce13d210 : taihu_405ep_init().
> i.e. ms->ram should hold whole RAM that is split between low and high
> using aliases.
Thank you for your pointers. I have just sent v2.
One note when looking at the taihu_405ep_init() implementation, the
following looks incorrect to me:
memory_region_init_alias(&ram_memories[1], NULL,
"taihu_405ep.ram-1", machine->ram, ram_bases[1],
ram_sizes[1]);
I think the 'offset' should be ram_sizes[0] instead of ram_bases[1],
although their values are the same which means they are two contiguous
regions, so it happens to work. But I might be nitpicking ...
>
> > @@ -606,6 +602,7 @@ static void microchip_icicle_kit_machine_class_init(ObjectClass *oc, void *data)
> > MICROCHIP_PFSOC_COMPUTE_CPU_COUNT;
> > mc->min_cpus = MICROCHIP_PFSOC_MANAGEMENT_CPU_COUNT + 1;
> > mc->default_cpus = mc->min_cpus;
> > + mc->default_ram_id = "microchip.icicle.kit.ram_low";
>
> given it is not versioned machine, so we don't have to worry about
> cross version migration here,
> so I'd use "microchip.icicle.kit.ram" for the name here
> and currently used "microchip.icicle.kit.ram_low" for corresponding alias
Regards,
Bin
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/6] hw/riscv: microchip_pfsoc: Use MachineState::ram and MachineClass::default_ram_id
2021-10-20 1:55 ` Bin Meng
@ 2021-10-20 8:32 ` Igor Mammedov
-1 siblings, 0 replies; 27+ messages in thread
From: Igor Mammedov @ 2021-10-20 8:32 UTC (permalink / raw)
To: Bin Meng
Cc: thuth, open list:RISC-V, qemu-devel@nongnu.org Developers,
laurent, qemu-ppc, alistair.francis, david
On Wed, 20 Oct 2021 09:55:52 +0800
Bin Meng <bmeng.cn@gmail.com> wrote:
> Hi Igor,
>
> On Tue, Oct 19, 2021 at 3:39 PM Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > On Mon, 18 Oct 2021 23:38:24 +0800
> > Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > > Using memory_region_init_ram(), which can't possibly handle vhost-user,
> > > and can't work as expected with '-numa node,memdev' options.
> > >
> > > Use MachineState::ram instead of manually initializing RAM memory
> > > region, as well as by providing MachineClass::default_ram_id to
> > > opt in to memdev scheme.
> > >
> > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > > ---
> > >
> > > hw/riscv/microchip_pfsoc.c | 9 +++------
> > > 1 file changed, 3 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
> > > index e475b6d511..f10f55b488 100644
> > > --- a/hw/riscv/microchip_pfsoc.c
> > > +++ b/hw/riscv/microchip_pfsoc.c
> > > @@ -459,7 +459,6 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
> > > const MemMapEntry *memmap = microchip_pfsoc_memmap;
> > > MicrochipIcicleKitState *s = MICROCHIP_ICICLE_KIT_MACHINE(machine);
> > > MemoryRegion *system_memory = get_system_memory();
> > > - MemoryRegion *mem_low = g_new(MemoryRegion, 1);
> > > MemoryRegion *mem_low_alias = g_new(MemoryRegion, 1);
> > > MemoryRegion *mem_high = g_new(MemoryRegion, 1);
> > > MemoryRegion *mem_high_alias = g_new(MemoryRegion, 1);
> > > @@ -486,16 +485,13 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
> > > qdev_realize(DEVICE(&s->soc), NULL, &error_abort);
> > >
> > > /* Register RAM */
> > > - memory_region_init_ram(mem_low, NULL, "microchip.icicle.kit.ram_low",
> > > - memmap[MICROCHIP_PFSOC_DRAM_LO].size,
> > > - &error_fatal);
> > > memory_region_init_alias(mem_low_alias, NULL,
> > > "microchip.icicle.kit.ram_low.alias",
> > > - mem_low, 0,
> > > + machine->ram, 0,
> > > memmap[MICROCHIP_PFSOC_DRAM_LO_ALIAS].size);
> > > memory_region_add_subregion(system_memory,
> > > memmap[MICROCHIP_PFSOC_DRAM_LO].base,
> > > - mem_low);
> > > + machine->ram);
> > > memory_region_add_subregion(system_memory,
> > > memmap[MICROCHIP_PFSOC_DRAM_LO_ALIAS].base,
> > > mem_low_alias);
> >
> > looking at code it seems RAM is split between low and high regions,
> > so converting only low region is wrong.
> >
> > I'd suggest something similar to 2dc9ce13d210 : taihu_405ep_init().
> > i.e. ms->ram should hold whole RAM that is split between low and high
> > using aliases.
>
> Thank you for your pointers. I have just sent v2.
> One note when looking at the taihu_405ep_init() implementation, the
> following looks incorrect to me:
>
> memory_region_init_alias(&ram_memories[1], NULL,
> "taihu_405ep.ram-1", machine->ram, ram_bases[1],
> ram_sizes[1]);
>
> I think the 'offset' should be ram_sizes[0] instead of ram_bases[1],
> although their values are the same which means they are two contiguous
> regions, so it happens to work. But I might be nitpicking ...
I fail to see what's wrong there, from the way code is written
it looks like hardware has 2 memory banks with different base
address. It just happens that ram_bases[1] starts right after
ram_bases[0] but if it weren't then using ram_sizes[0]
for offset would be wrong. So current code looks fine to me.
Anyways,
CCing PPC folks to have a second look at it.
>
> >
> > > @@ -606,6 +602,7 @@ static void microchip_icicle_kit_machine_class_init(ObjectClass *oc, void *data)
> > > MICROCHIP_PFSOC_COMPUTE_CPU_COUNT;
> > > mc->min_cpus = MICROCHIP_PFSOC_MANAGEMENT_CPU_COUNT + 1;
> > > mc->default_cpus = mc->min_cpus;
> > > + mc->default_ram_id = "microchip.icicle.kit.ram_low";
> >
> > given it is not versioned machine, so we don't have to worry about
> > cross version migration here,
> > so I'd use "microchip.icicle.kit.ram" for the name here
> > and currently used "microchip.icicle.kit.ram_low" for corresponding alias
>
> Regards,
> Bin
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/6] hw/riscv: microchip_pfsoc: Use MachineState::ram and MachineClass::default_ram_id
@ 2021-10-20 8:32 ` Igor Mammedov
0 siblings, 0 replies; 27+ messages in thread
From: Igor Mammedov @ 2021-10-20 8:32 UTC (permalink / raw)
To: Bin Meng
Cc: Alistair Francis, qemu-devel@nongnu.org Developers,
open list:RISC-V, qemu-ppc, laurent, thuth, david,
alistair.francis
On Wed, 20 Oct 2021 09:55:52 +0800
Bin Meng <bmeng.cn@gmail.com> wrote:
> Hi Igor,
>
> On Tue, Oct 19, 2021 at 3:39 PM Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > On Mon, 18 Oct 2021 23:38:24 +0800
> > Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > > Using memory_region_init_ram(), which can't possibly handle vhost-user,
> > > and can't work as expected with '-numa node,memdev' options.
> > >
> > > Use MachineState::ram instead of manually initializing RAM memory
> > > region, as well as by providing MachineClass::default_ram_id to
> > > opt in to memdev scheme.
> > >
> > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > > ---
> > >
> > > hw/riscv/microchip_pfsoc.c | 9 +++------
> > > 1 file changed, 3 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
> > > index e475b6d511..f10f55b488 100644
> > > --- a/hw/riscv/microchip_pfsoc.c
> > > +++ b/hw/riscv/microchip_pfsoc.c
> > > @@ -459,7 +459,6 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
> > > const MemMapEntry *memmap = microchip_pfsoc_memmap;
> > > MicrochipIcicleKitState *s = MICROCHIP_ICICLE_KIT_MACHINE(machine);
> > > MemoryRegion *system_memory = get_system_memory();
> > > - MemoryRegion *mem_low = g_new(MemoryRegion, 1);
> > > MemoryRegion *mem_low_alias = g_new(MemoryRegion, 1);
> > > MemoryRegion *mem_high = g_new(MemoryRegion, 1);
> > > MemoryRegion *mem_high_alias = g_new(MemoryRegion, 1);
> > > @@ -486,16 +485,13 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
> > > qdev_realize(DEVICE(&s->soc), NULL, &error_abort);
> > >
> > > /* Register RAM */
> > > - memory_region_init_ram(mem_low, NULL, "microchip.icicle.kit.ram_low",
> > > - memmap[MICROCHIP_PFSOC_DRAM_LO].size,
> > > - &error_fatal);
> > > memory_region_init_alias(mem_low_alias, NULL,
> > > "microchip.icicle.kit.ram_low.alias",
> > > - mem_low, 0,
> > > + machine->ram, 0,
> > > memmap[MICROCHIP_PFSOC_DRAM_LO_ALIAS].size);
> > > memory_region_add_subregion(system_memory,
> > > memmap[MICROCHIP_PFSOC_DRAM_LO].base,
> > > - mem_low);
> > > + machine->ram);
> > > memory_region_add_subregion(system_memory,
> > > memmap[MICROCHIP_PFSOC_DRAM_LO_ALIAS].base,
> > > mem_low_alias);
> >
> > looking at code it seems RAM is split between low and high regions,
> > so converting only low region is wrong.
> >
> > I'd suggest something similar to 2dc9ce13d210 : taihu_405ep_init().
> > i.e. ms->ram should hold whole RAM that is split between low and high
> > using aliases.
>
> Thank you for your pointers. I have just sent v2.
> One note when looking at the taihu_405ep_init() implementation, the
> following looks incorrect to me:
>
> memory_region_init_alias(&ram_memories[1], NULL,
> "taihu_405ep.ram-1", machine->ram, ram_bases[1],
> ram_sizes[1]);
>
> I think the 'offset' should be ram_sizes[0] instead of ram_bases[1],
> although their values are the same which means they are two contiguous
> regions, so it happens to work. But I might be nitpicking ...
I fail to see what's wrong there, from the way code is written
it looks like hardware has 2 memory banks with different base
address. It just happens that ram_bases[1] starts right after
ram_bases[0] but if it weren't then using ram_sizes[0]
for offset would be wrong. So current code looks fine to me.
Anyways,
CCing PPC folks to have a second look at it.
>
> >
> > > @@ -606,6 +602,7 @@ static void microchip_icicle_kit_machine_class_init(ObjectClass *oc, void *data)
> > > MICROCHIP_PFSOC_COMPUTE_CPU_COUNT;
> > > mc->min_cpus = MICROCHIP_PFSOC_MANAGEMENT_CPU_COUNT + 1;
> > > mc->default_cpus = mc->min_cpus;
> > > + mc->default_ram_id = "microchip.icicle.kit.ram_low";
> >
> > given it is not versioned machine, so we don't have to worry about
> > cross version migration here,
> > so I'd use "microchip.icicle.kit.ram" for the name here
> > and currently used "microchip.icicle.kit.ram_low" for corresponding alias
>
> Regards,
> Bin
>
^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2021-10-20 8:33 UTC | newest]
Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-18 15:38 [PATCH 1/6] hw/riscv: microchip_pfsoc: Use MachineState::ram and MachineClass::default_ram_id Bin Meng
2021-10-18 15:38 ` [PATCH 2/6] hw/riscv: opentitan: " Bin Meng
2021-10-19 7:11 ` Igor Mammedov
2021-10-19 7:11 ` Igor Mammedov
2021-10-19 12:57 ` Bin Meng
2021-10-19 12:57 ` Bin Meng
2021-10-18 15:38 ` [PATCH 3/6] hw/riscv: shakti_c: " Bin Meng
2021-10-19 7:12 ` Igor Mammedov
2021-10-19 7:12 ` Igor Mammedov
2021-10-18 15:38 ` [PATCH 4/6] hw/riscv: sifive_e: " Bin Meng
2021-10-19 7:15 ` Igor Mammedov
2021-10-19 7:15 ` Igor Mammedov
2021-10-18 15:38 ` [PATCH 5/6] hw/riscv: sifive_u: " Bin Meng
2021-10-19 7:16 ` Igor Mammedov
2021-10-19 7:16 ` Igor Mammedov
2021-10-18 15:38 ` [PATCH 6/6] hw/riscv: spike: " Bin Meng
2021-10-19 7:17 ` Igor Mammedov
2021-10-19 7:17 ` Igor Mammedov
2021-10-18 15:51 ` [PATCH 1/6] hw/riscv: microchip_pfsoc: " Philippe Mathieu-Daudé
2021-10-18 16:00 ` Bin Meng
2021-10-18 16:00 ` Bin Meng
2021-10-19 7:39 ` Igor Mammedov
2021-10-19 7:39 ` Igor Mammedov
2021-10-20 1:55 ` Bin Meng
2021-10-20 1:55 ` Bin Meng
2021-10-20 8:32 ` Igor Mammedov
2021-10-20 8:32 ` Igor Mammedov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.