* Re: [PATCH v3 1/1] hw/riscv: Fix max size limit when put initrd to RAM
2023-03-13 2:18 ` [PATCH v3 1/1] hw/riscv: Fix max " Hang Xu
@ 2023-03-13 12:29 ` Daniel Henrique Barboza
2023-03-13 15:49 ` Anup Patel
2023-04-05 5:53 ` Alistair Francis
2 siblings, 0 replies; 8+ messages in thread
From: Daniel Henrique Barboza @ 2023-03-13 12:29 UTC (permalink / raw)
To: Hang Xu, qemu-riscv; +Cc: qemu-devel, palmer, alistair.francis
On 3/12/23 23:18, Hang Xu wrote:
> Because the starting address of ram is not necessarily 0,
> the remaining free space in ram is
> ram_size - (start - ram_base) instead of ram_size-start.
>
> Signed-off-by: Hang Xu <xuhang@eswincomputing.com>
> ---
Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> hw/riscv/boot.c | 19 +++++++++++++------
> hw/riscv/microchip_pfsoc.c | 5 ++++-
> hw/riscv/opentitan.c | 2 +-
> hw/riscv/sifive_e.c | 2 +-
> hw/riscv/sifive_u.c | 5 ++++-
> hw/riscv/spike.c | 5 ++++-
> hw/riscv/virt.c | 5 ++++-
> include/hw/riscv/boot.h | 2 ++
> 8 files changed, 33 insertions(+), 12 deletions(-)
>
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 52bf8e67de..cfbc376a82 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -173,13 +173,14 @@ target_ulong riscv_load_firmware(const char *firmware_filename,
> exit(1);
> }
>
> -static void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
> +static void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry,
> + uint64_t ram_base, uint64_t ram_size)
> {
> const char *filename = machine->initrd_filename;
> - uint64_t mem_size = machine->ram_size;
> void *fdt = machine->fdt;
> hwaddr start, end;
> ssize_t size;
> + uint64_t max_initrd;
>
> g_assert(filename != NULL);
>
> @@ -193,12 +194,16 @@ static void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
> * So for boards with less than 256MB of RAM we put the initrd
> * halfway into RAM, and for boards with 256MB of RAM or more we put
> * the initrd at 128MB.
> + * A ram_size == 0, usually from a MemMapEntry[].size element,
> + * means that the RAM block goes all the way to ms->ram_size.
> */
> - start = kernel_entry + MIN(mem_size / 2, 128 * MiB);
> + ram_size = ram_size ? MIN(machine->ram_size, ram_size) : machine->ram_size;
> + start = kernel_entry + MIN(ram_size / 2, 128 * MiB);
> + max_initrd = ram_size - (start - ram_base);
>
> - size = load_ramdisk(filename, start, mem_size - start);
> + size = load_ramdisk(filename, start, max_initrd);
> if (size == -1) {
> - size = load_image_targphys(filename, start, mem_size - start);
> + size = load_image_targphys(filename, start, max_initrd);
> if (size == -1) {
> error_report("could not load ramdisk '%s'", filename);
> exit(1);
> @@ -217,6 +222,8 @@ target_ulong riscv_load_kernel(MachineState *machine,
> RISCVHartArrayState *harts,
> target_ulong kernel_start_addr,
> bool load_initrd,
> + uint64_t ram_base,
> + uint64_t ram_size,
> symbol_fn_t sym_cb)
> {
> const char *kernel_filename = machine->kernel_filename;
> @@ -263,7 +270,7 @@ out:
> }
>
> if (load_initrd && machine->initrd_filename) {
> - riscv_load_initrd(machine, kernel_entry);
> + riscv_load_initrd(machine, kernel_entry, ram_base, ram_size);
> }
>
> if (fdt && machine->kernel_cmdline && *machine->kernel_cmdline) {
> diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
> index e81bbd12df..b42d90b89e 100644
> --- a/hw/riscv/microchip_pfsoc.c
> +++ b/hw/riscv/microchip_pfsoc.c
> @@ -630,7 +630,10 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
> firmware_end_addr);
>
> kernel_entry = riscv_load_kernel(machine, &s->soc.u_cpus,
> - kernel_start_addr, true, NULL);
> + kernel_start_addr, true,
> + memmap[MICROCHIP_PFSOC_DRAM_LO].base,
> + memmap[MICROCHIP_PFSOC_DRAM_LO].size,
> + NULL);
>
> /* Compute the fdt load address in dram */
> fdt_load_addr = riscv_compute_fdt_addr(memmap[MICROCHIP_PFSOC_DRAM_LO].base,
> diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c
> index b06944d382..bb663523d5 100644
> --- a/hw/riscv/opentitan.c
> +++ b/hw/riscv/opentitan.c
> @@ -103,7 +103,7 @@ static void opentitan_board_init(MachineState *machine)
> if (machine->kernel_filename) {
> riscv_load_kernel(machine, &s->soc.cpus,
> memmap[IBEX_DEV_RAM].base,
> - false, NULL);
> + false, 0, 0, NULL);
> }
> }
>
> diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
> index 04939b60c3..5b47d539a6 100644
> --- a/hw/riscv/sifive_e.c
> +++ b/hw/riscv/sifive_e.c
> @@ -116,7 +116,7 @@ static void sifive_e_machine_init(MachineState *machine)
> if (machine->kernel_filename) {
> riscv_load_kernel(machine, &s->soc.cpus,
> memmap[SIFIVE_E_DEV_DTIM].base,
> - false, NULL);
> + false, 0, 0, NULL);
> }
> }
>
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index 35a335b8d0..b45fdc968c 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -599,7 +599,10 @@ static void sifive_u_machine_init(MachineState *machine)
> firmware_end_addr);
>
> kernel_entry = riscv_load_kernel(machine, &s->soc.u_cpus,
> - kernel_start_addr, true, NULL);
> + kernel_start_addr, true,
> + memmap[SIFIVE_U_DEV_DRAM].base,
> + memmap[SIFIVE_U_DEV_DRAM].size,
> + NULL);
> } else {
> /*
> * If dynamic firmware is used, it doesn't know where is the next mode
> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> index a584d5b3a2..e322ed8506 100644
> --- a/hw/riscv/spike.c
> +++ b/hw/riscv/spike.c
> @@ -307,7 +307,10 @@ static void spike_board_init(MachineState *machine)
>
> kernel_entry = riscv_load_kernel(machine, &s->soc[0],
> kernel_start_addr,
> - true, htif_symbol_callback);
> + true,
> + memmap[SPIKE_DRAM].base,
> + memmap[SPIKE_DRAM].size,
> + htif_symbol_callback);
> } else {
> /*
> * If dynamic firmware is used, it doesn't know where is the next mode
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 4e3efbee16..11f26b0dc0 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -1287,7 +1287,10 @@ static void virt_machine_done(Notifier *notifier, void *data)
> firmware_end_addr);
>
> kernel_entry = riscv_load_kernel(machine, &s->soc[0],
> - kernel_start_addr, true, NULL);
> + kernel_start_addr, true,
> + memmap[VIRT_DRAM].base,
> + memmap[VIRT_DRAM].size,
> + NULL);
> } else {
> /*
> * If dynamic firmware is used, it doesn't know where is the next mode
> diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
> index a2e4ae9cb0..987e1add38 100644
> --- a/include/hw/riscv/boot.h
> +++ b/include/hw/riscv/boot.h
> @@ -47,6 +47,8 @@ target_ulong riscv_load_kernel(MachineState *machine,
> RISCVHartArrayState *harts,
> target_ulong firmware_end_addr,
> bool load_initrd,
> + uint64_t ram_base,
> + uint64_t ram_size,
> symbol_fn_t sym_cb);
> uint64_t riscv_compute_fdt_addr(hwaddr dram_start, uint64_t dram_size,
> MachineState *ms);
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v3 1/1] hw/riscv: Fix max size limit when put initrd to RAM
2023-03-13 2:18 ` [PATCH v3 1/1] hw/riscv: Fix max " Hang Xu
2023-03-13 12:29 ` Daniel Henrique Barboza
@ 2023-03-13 15:49 ` Anup Patel
2023-03-13 21:32 ` Daniel Henrique Barboza
2023-04-05 5:53 ` Alistair Francis
2 siblings, 1 reply; 8+ messages in thread
From: Anup Patel @ 2023-03-13 15:49 UTC (permalink / raw)
To: Hang Xu; +Cc: qemu-riscv, qemu-devel, palmer, dbarboza, alistair.francis
On Mon, Mar 13, 2023 at 7:49 AM Hang Xu <xuhang@eswincomputing.com> wrote:
>
> Because the starting address of ram is not necessarily 0,
> the remaining free space in ram is
> ram_size - (start - ram_base) instead of ram_size-start.
>
> Signed-off-by: Hang Xu <xuhang@eswincomputing.com>
What happens in-case a platform has multiple RAM banks ?
Regards,
Anup
> ---
> hw/riscv/boot.c | 19 +++++++++++++------
> hw/riscv/microchip_pfsoc.c | 5 ++++-
> hw/riscv/opentitan.c | 2 +-
> hw/riscv/sifive_e.c | 2 +-
> hw/riscv/sifive_u.c | 5 ++++-
> hw/riscv/spike.c | 5 ++++-
> hw/riscv/virt.c | 5 ++++-
> include/hw/riscv/boot.h | 2 ++
> 8 files changed, 33 insertions(+), 12 deletions(-)
>
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 52bf8e67de..cfbc376a82 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -173,13 +173,14 @@ target_ulong riscv_load_firmware(const char *firmware_filename,
> exit(1);
> }
>
> -static void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
> +static void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry,
> + uint64_t ram_base, uint64_t ram_size)
> {
> const char *filename = machine->initrd_filename;
> - uint64_t mem_size = machine->ram_size;
> void *fdt = machine->fdt;
> hwaddr start, end;
> ssize_t size;
> + uint64_t max_initrd;
>
> g_assert(filename != NULL);
>
> @@ -193,12 +194,16 @@ static void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
> * So for boards with less than 256MB of RAM we put the initrd
> * halfway into RAM, and for boards with 256MB of RAM or more we put
> * the initrd at 128MB.
> + * A ram_size == 0, usually from a MemMapEntry[].size element,
> + * means that the RAM block goes all the way to ms->ram_size.
> */
> - start = kernel_entry + MIN(mem_size / 2, 128 * MiB);
> + ram_size = ram_size ? MIN(machine->ram_size, ram_size) : machine->ram_size;
> + start = kernel_entry + MIN(ram_size / 2, 128 * MiB);
> + max_initrd = ram_size - (start - ram_base);
>
> - size = load_ramdisk(filename, start, mem_size - start);
> + size = load_ramdisk(filename, start, max_initrd);
> if (size == -1) {
> - size = load_image_targphys(filename, start, mem_size - start);
> + size = load_image_targphys(filename, start, max_initrd);
> if (size == -1) {
> error_report("could not load ramdisk '%s'", filename);
> exit(1);
> @@ -217,6 +222,8 @@ target_ulong riscv_load_kernel(MachineState *machine,
> RISCVHartArrayState *harts,
> target_ulong kernel_start_addr,
> bool load_initrd,
> + uint64_t ram_base,
> + uint64_t ram_size,
> symbol_fn_t sym_cb)
> {
> const char *kernel_filename = machine->kernel_filename;
> @@ -263,7 +270,7 @@ out:
> }
>
> if (load_initrd && machine->initrd_filename) {
> - riscv_load_initrd(machine, kernel_entry);
> + riscv_load_initrd(machine, kernel_entry, ram_base, ram_size);
> }
>
> if (fdt && machine->kernel_cmdline && *machine->kernel_cmdline) {
> diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
> index e81bbd12df..b42d90b89e 100644
> --- a/hw/riscv/microchip_pfsoc.c
> +++ b/hw/riscv/microchip_pfsoc.c
> @@ -630,7 +630,10 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
> firmware_end_addr);
>
> kernel_entry = riscv_load_kernel(machine, &s->soc.u_cpus,
> - kernel_start_addr, true, NULL);
> + kernel_start_addr, true,
> + memmap[MICROCHIP_PFSOC_DRAM_LO].base,
> + memmap[MICROCHIP_PFSOC_DRAM_LO].size,
> + NULL);
>
> /* Compute the fdt load address in dram */
> fdt_load_addr = riscv_compute_fdt_addr(memmap[MICROCHIP_PFSOC_DRAM_LO].base,
> diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c
> index b06944d382..bb663523d5 100644
> --- a/hw/riscv/opentitan.c
> +++ b/hw/riscv/opentitan.c
> @@ -103,7 +103,7 @@ static void opentitan_board_init(MachineState *machine)
> if (machine->kernel_filename) {
> riscv_load_kernel(machine, &s->soc.cpus,
> memmap[IBEX_DEV_RAM].base,
> - false, NULL);
> + false, 0, 0, NULL);
> }
> }
>
> diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
> index 04939b60c3..5b47d539a6 100644
> --- a/hw/riscv/sifive_e.c
> +++ b/hw/riscv/sifive_e.c
> @@ -116,7 +116,7 @@ static void sifive_e_machine_init(MachineState *machine)
> if (machine->kernel_filename) {
> riscv_load_kernel(machine, &s->soc.cpus,
> memmap[SIFIVE_E_DEV_DTIM].base,
> - false, NULL);
> + false, 0, 0, NULL);
> }
> }
>
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index 35a335b8d0..b45fdc968c 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -599,7 +599,10 @@ static void sifive_u_machine_init(MachineState *machine)
> firmware_end_addr);
>
> kernel_entry = riscv_load_kernel(machine, &s->soc.u_cpus,
> - kernel_start_addr, true, NULL);
> + kernel_start_addr, true,
> + memmap[SIFIVE_U_DEV_DRAM].base,
> + memmap[SIFIVE_U_DEV_DRAM].size,
> + NULL);
> } else {
> /*
> * If dynamic firmware is used, it doesn't know where is the next mode
> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> index a584d5b3a2..e322ed8506 100644
> --- a/hw/riscv/spike.c
> +++ b/hw/riscv/spike.c
> @@ -307,7 +307,10 @@ static void spike_board_init(MachineState *machine)
>
> kernel_entry = riscv_load_kernel(machine, &s->soc[0],
> kernel_start_addr,
> - true, htif_symbol_callback);
> + true,
> + memmap[SPIKE_DRAM].base,
> + memmap[SPIKE_DRAM].size,
> + htif_symbol_callback);
> } else {
> /*
> * If dynamic firmware is used, it doesn't know where is the next mode
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 4e3efbee16..11f26b0dc0 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -1287,7 +1287,10 @@ static void virt_machine_done(Notifier *notifier, void *data)
> firmware_end_addr);
>
> kernel_entry = riscv_load_kernel(machine, &s->soc[0],
> - kernel_start_addr, true, NULL);
> + kernel_start_addr, true,
> + memmap[VIRT_DRAM].base,
> + memmap[VIRT_DRAM].size,
> + NULL);
> } else {
> /*
> * If dynamic firmware is used, it doesn't know where is the next mode
> diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
> index a2e4ae9cb0..987e1add38 100644
> --- a/include/hw/riscv/boot.h
> +++ b/include/hw/riscv/boot.h
> @@ -47,6 +47,8 @@ target_ulong riscv_load_kernel(MachineState *machine,
> RISCVHartArrayState *harts,
> target_ulong firmware_end_addr,
> bool load_initrd,
> + uint64_t ram_base,
> + uint64_t ram_size,
> symbol_fn_t sym_cb);
> uint64_t riscv_compute_fdt_addr(hwaddr dram_start, uint64_t dram_size,
> MachineState *ms);
> --
> 2.17.1
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v3 1/1] hw/riscv: Fix max size limit when put initrd to RAM
2023-03-13 15:49 ` Anup Patel
@ 2023-03-13 21:32 ` Daniel Henrique Barboza
0 siblings, 0 replies; 8+ messages in thread
From: Daniel Henrique Barboza @ 2023-03-13 21:32 UTC (permalink / raw)
To: Anup Patel, Hang Xu; +Cc: qemu-riscv, qemu-devel, palmer, alistair.francis
On 3/13/23 12:49, Anup Patel wrote:
> On Mon, Mar 13, 2023 at 7:49 AM Hang Xu <xuhang@eswincomputing.com> wrote:
>>
>> Because the starting address of ram is not necessarily 0,
>> the remaining free space in ram is
>> ram_size - (start - ram_base) instead of ram_size-start.
>>
>> Signed-off-by: Hang Xu <xuhang@eswincomputing.com>
>
> What happens in-case a platform has multiple RAM banks ?
In this case the board must specify a contiguous RAM region to be used. It's
not restricted to a single RAM bank - as long as it is contiguous RAM it can
spam multiple RAM banks.
This was done to accomodate boards that has RAM gaps (at this moment the
microchip board) and where we can't use the whole RAM to determine where
to put the initrd/fdt.
Thanks,
Daniel
>
> Regards,
> Anup
>
>> ---
>> hw/riscv/boot.c | 19 +++++++++++++------
>> hw/riscv/microchip_pfsoc.c | 5 ++++-
>> hw/riscv/opentitan.c | 2 +-
>> hw/riscv/sifive_e.c | 2 +-
>> hw/riscv/sifive_u.c | 5 ++++-
>> hw/riscv/spike.c | 5 ++++-
>> hw/riscv/virt.c | 5 ++++-
>> include/hw/riscv/boot.h | 2 ++
>> 8 files changed, 33 insertions(+), 12 deletions(-)
>>
>> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
>> index 52bf8e67de..cfbc376a82 100644
>> --- a/hw/riscv/boot.c
>> +++ b/hw/riscv/boot.c
>> @@ -173,13 +173,14 @@ target_ulong riscv_load_firmware(const char *firmware_filename,
>> exit(1);
>> }
>>
>> -static void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
>> +static void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry,
>> + uint64_t ram_base, uint64_t ram_size)
>> {
>> const char *filename = machine->initrd_filename;
>> - uint64_t mem_size = machine->ram_size;
>> void *fdt = machine->fdt;
>> hwaddr start, end;
>> ssize_t size;
>> + uint64_t max_initrd;
>>
>> g_assert(filename != NULL);
>>
>> @@ -193,12 +194,16 @@ static void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
>> * So for boards with less than 256MB of RAM we put the initrd
>> * halfway into RAM, and for boards with 256MB of RAM or more we put
>> * the initrd at 128MB.
>> + * A ram_size == 0, usually from a MemMapEntry[].size element,
>> + * means that the RAM block goes all the way to ms->ram_size.
>> */
>> - start = kernel_entry + MIN(mem_size / 2, 128 * MiB);
>> + ram_size = ram_size ? MIN(machine->ram_size, ram_size) : machine->ram_size;
>> + start = kernel_entry + MIN(ram_size / 2, 128 * MiB);
>> + max_initrd = ram_size - (start - ram_base);
>>
>> - size = load_ramdisk(filename, start, mem_size - start);
>> + size = load_ramdisk(filename, start, max_initrd);
>> if (size == -1) {
>> - size = load_image_targphys(filename, start, mem_size - start);
>> + size = load_image_targphys(filename, start, max_initrd);
>> if (size == -1) {
>> error_report("could not load ramdisk '%s'", filename);
>> exit(1);
>> @@ -217,6 +222,8 @@ target_ulong riscv_load_kernel(MachineState *machine,
>> RISCVHartArrayState *harts,
>> target_ulong kernel_start_addr,
>> bool load_initrd,
>> + uint64_t ram_base,
>> + uint64_t ram_size,
>> symbol_fn_t sym_cb)
>> {
>> const char *kernel_filename = machine->kernel_filename;
>> @@ -263,7 +270,7 @@ out:
>> }
>>
>> if (load_initrd && machine->initrd_filename) {
>> - riscv_load_initrd(machine, kernel_entry);
>> + riscv_load_initrd(machine, kernel_entry, ram_base, ram_size);
>> }
>>
>> if (fdt && machine->kernel_cmdline && *machine->kernel_cmdline) {
>> diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
>> index e81bbd12df..b42d90b89e 100644
>> --- a/hw/riscv/microchip_pfsoc.c
>> +++ b/hw/riscv/microchip_pfsoc.c
>> @@ -630,7 +630,10 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
>> firmware_end_addr);
>>
>> kernel_entry = riscv_load_kernel(machine, &s->soc.u_cpus,
>> - kernel_start_addr, true, NULL);
>> + kernel_start_addr, true,
>> + memmap[MICROCHIP_PFSOC_DRAM_LO].base,
>> + memmap[MICROCHIP_PFSOC_DRAM_LO].size,
>> + NULL);
>>
>> /* Compute the fdt load address in dram */
>> fdt_load_addr = riscv_compute_fdt_addr(memmap[MICROCHIP_PFSOC_DRAM_LO].base,
>> diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c
>> index b06944d382..bb663523d5 100644
>> --- a/hw/riscv/opentitan.c
>> +++ b/hw/riscv/opentitan.c
>> @@ -103,7 +103,7 @@ static void opentitan_board_init(MachineState *machine)
>> if (machine->kernel_filename) {
>> riscv_load_kernel(machine, &s->soc.cpus,
>> memmap[IBEX_DEV_RAM].base,
>> - false, NULL);
>> + false, 0, 0, NULL);
>> }
>> }
>>
>> diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
>> index 04939b60c3..5b47d539a6 100644
>> --- a/hw/riscv/sifive_e.c
>> +++ b/hw/riscv/sifive_e.c
>> @@ -116,7 +116,7 @@ static void sifive_e_machine_init(MachineState *machine)
>> if (machine->kernel_filename) {
>> riscv_load_kernel(machine, &s->soc.cpus,
>> memmap[SIFIVE_E_DEV_DTIM].base,
>> - false, NULL);
>> + false, 0, 0, NULL);
>> }
>> }
>>
>> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
>> index 35a335b8d0..b45fdc968c 100644
>> --- a/hw/riscv/sifive_u.c
>> +++ b/hw/riscv/sifive_u.c
>> @@ -599,7 +599,10 @@ static void sifive_u_machine_init(MachineState *machine)
>> firmware_end_addr);
>>
>> kernel_entry = riscv_load_kernel(machine, &s->soc.u_cpus,
>> - kernel_start_addr, true, NULL);
>> + kernel_start_addr, true,
>> + memmap[SIFIVE_U_DEV_DRAM].base,
>> + memmap[SIFIVE_U_DEV_DRAM].size,
>> + NULL);
>> } else {
>> /*
>> * If dynamic firmware is used, it doesn't know where is the next mode
>> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
>> index a584d5b3a2..e322ed8506 100644
>> --- a/hw/riscv/spike.c
>> +++ b/hw/riscv/spike.c
>> @@ -307,7 +307,10 @@ static void spike_board_init(MachineState *machine)
>>
>> kernel_entry = riscv_load_kernel(machine, &s->soc[0],
>> kernel_start_addr,
>> - true, htif_symbol_callback);
>> + true,
>> + memmap[SPIKE_DRAM].base,
>> + memmap[SPIKE_DRAM].size,
>> + htif_symbol_callback);
>> } else {
>> /*
>> * If dynamic firmware is used, it doesn't know where is the next mode
>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>> index 4e3efbee16..11f26b0dc0 100644
>> --- a/hw/riscv/virt.c
>> +++ b/hw/riscv/virt.c
>> @@ -1287,7 +1287,10 @@ static void virt_machine_done(Notifier *notifier, void *data)
>> firmware_end_addr);
>>
>> kernel_entry = riscv_load_kernel(machine, &s->soc[0],
>> - kernel_start_addr, true, NULL);
>> + kernel_start_addr, true,
>> + memmap[VIRT_DRAM].base,
>> + memmap[VIRT_DRAM].size,
>> + NULL);
>> } else {
>> /*
>> * If dynamic firmware is used, it doesn't know where is the next mode
>> diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
>> index a2e4ae9cb0..987e1add38 100644
>> --- a/include/hw/riscv/boot.h
>> +++ b/include/hw/riscv/boot.h
>> @@ -47,6 +47,8 @@ target_ulong riscv_load_kernel(MachineState *machine,
>> RISCVHartArrayState *harts,
>> target_ulong firmware_end_addr,
>> bool load_initrd,
>> + uint64_t ram_base,
>> + uint64_t ram_size,
>> symbol_fn_t sym_cb);
>> uint64_t riscv_compute_fdt_addr(hwaddr dram_start, uint64_t dram_size,
>> MachineState *ms);
>> --
>> 2.17.1
>>
>>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 1/1] hw/riscv: Fix max size limit when put initrd to RAM
2023-03-13 2:18 ` [PATCH v3 1/1] hw/riscv: Fix max " Hang Xu
2023-03-13 12:29 ` Daniel Henrique Barboza
2023-03-13 15:49 ` Anup Patel
@ 2023-04-05 5:53 ` Alistair Francis
2023-04-05 9:38 ` Daniel Henrique Barboza
2023-04-07 4:26 ` Hang Xu
2 siblings, 2 replies; 8+ messages in thread
From: Alistair Francis @ 2023-04-05 5:53 UTC (permalink / raw)
To: Hang Xu; +Cc: qemu-riscv, qemu-devel, palmer, dbarboza, alistair.francis
On Mon, Mar 13, 2023 at 11:12 PM Hang Xu <xuhang@eswincomputing.com> wrote:
>
> Because the starting address of ram is not necessarily 0,
> the remaining free space in ram is
> ram_size - (start - ram_base) instead of ram_size-start.
I think this could be clearer. It's not clear here that you mean the
free space after the kernel (for in the initrd).
>
> Signed-off-by: Hang Xu <xuhang@eswincomputing.com>
> ---
> hw/riscv/boot.c | 19 +++++++++++++------
> hw/riscv/microchip_pfsoc.c | 5 ++++-
> hw/riscv/opentitan.c | 2 +-
> hw/riscv/sifive_e.c | 2 +-
> hw/riscv/sifive_u.c | 5 ++++-
> hw/riscv/spike.c | 5 ++++-
> hw/riscv/virt.c | 5 ++++-
> include/hw/riscv/boot.h | 2 ++
> 8 files changed, 33 insertions(+), 12 deletions(-)
>
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 52bf8e67de..cfbc376a82 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -173,13 +173,14 @@ target_ulong riscv_load_firmware(const char *firmware_filename,
> exit(1);
> }
>
> -static void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
> +static void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry,
> + uint64_t ram_base, uint64_t ram_size)
> {
> const char *filename = machine->initrd_filename;
> - uint64_t mem_size = machine->ram_size;
> void *fdt = machine->fdt;
> hwaddr start, end;
> ssize_t size;
> + uint64_t max_initrd;
>
> g_assert(filename != NULL);
>
> @@ -193,12 +194,16 @@ static void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
> * So for boards with less than 256MB of RAM we put the initrd
> * halfway into RAM, and for boards with 256MB of RAM or more we put
> * the initrd at 128MB.
> + * A ram_size == 0, usually from a MemMapEntry[].size element,
> + * means that the RAM block goes all the way to ms->ram_size.
> */
> - start = kernel_entry + MIN(mem_size / 2, 128 * MiB);
> + ram_size = ram_size ? MIN(machine->ram_size, ram_size) : machine->ram_size;
This doesn't seem right. If machine->ram_size is greater then the
board can support we should tell the user and have them set a correct
size
> + start = kernel_entry + MIN(ram_size / 2, 128 * MiB);
> + max_initrd = ram_size - (start - ram_base);
Good catch. Passing the base address of memory is a good move here.
Alistair
>
> - size = load_ramdisk(filename, start, mem_size - start);
> + size = load_ramdisk(filename, start, max_initrd);
> if (size == -1) {
> - size = load_image_targphys(filename, start, mem_size - start);
> + size = load_image_targphys(filename, start, max_initrd);
> if (size == -1) {
> error_report("could not load ramdisk '%s'", filename);
> exit(1);
> @@ -217,6 +222,8 @@ target_ulong riscv_load_kernel(MachineState *machine,
> RISCVHartArrayState *harts,
> target_ulong kernel_start_addr,
> bool load_initrd,
> + uint64_t ram_base,
> + uint64_t ram_size,
> symbol_fn_t sym_cb)
> {
> const char *kernel_filename = machine->kernel_filename;
> @@ -263,7 +270,7 @@ out:
> }
>
> if (load_initrd && machine->initrd_filename) {
> - riscv_load_initrd(machine, kernel_entry);
> + riscv_load_initrd(machine, kernel_entry, ram_base, ram_size);
> }
>
> if (fdt && machine->kernel_cmdline && *machine->kernel_cmdline) {
> diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
> index e81bbd12df..b42d90b89e 100644
> --- a/hw/riscv/microchip_pfsoc.c
> +++ b/hw/riscv/microchip_pfsoc.c
> @@ -630,7 +630,10 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
> firmware_end_addr);
>
> kernel_entry = riscv_load_kernel(machine, &s->soc.u_cpus,
> - kernel_start_addr, true, NULL);
> + kernel_start_addr, true,
> + memmap[MICROCHIP_PFSOC_DRAM_LO].base,
> + memmap[MICROCHIP_PFSOC_DRAM_LO].size,
> + NULL);
>
> /* Compute the fdt load address in dram */
> fdt_load_addr = riscv_compute_fdt_addr(memmap[MICROCHIP_PFSOC_DRAM_LO].base,
> diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c
> index b06944d382..bb663523d5 100644
> --- a/hw/riscv/opentitan.c
> +++ b/hw/riscv/opentitan.c
> @@ -103,7 +103,7 @@ static void opentitan_board_init(MachineState *machine)
> if (machine->kernel_filename) {
> riscv_load_kernel(machine, &s->soc.cpus,
> memmap[IBEX_DEV_RAM].base,
> - false, NULL);
> + false, 0, 0, NULL);
> }
> }
>
> diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
> index 04939b60c3..5b47d539a6 100644
> --- a/hw/riscv/sifive_e.c
> +++ b/hw/riscv/sifive_e.c
> @@ -116,7 +116,7 @@ static void sifive_e_machine_init(MachineState *machine)
> if (machine->kernel_filename) {
> riscv_load_kernel(machine, &s->soc.cpus,
> memmap[SIFIVE_E_DEV_DTIM].base,
> - false, NULL);
> + false, 0, 0, NULL);
> }
> }
>
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index 35a335b8d0..b45fdc968c 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -599,7 +599,10 @@ static void sifive_u_machine_init(MachineState *machine)
> firmware_end_addr);
>
> kernel_entry = riscv_load_kernel(machine, &s->soc.u_cpus,
> - kernel_start_addr, true, NULL);
> + kernel_start_addr, true,
> + memmap[SIFIVE_U_DEV_DRAM].base,
> + memmap[SIFIVE_U_DEV_DRAM].size,
> + NULL);
> } else {
> /*
> * If dynamic firmware is used, it doesn't know where is the next mode
> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> index a584d5b3a2..e322ed8506 100644
> --- a/hw/riscv/spike.c
> +++ b/hw/riscv/spike.c
> @@ -307,7 +307,10 @@ static void spike_board_init(MachineState *machine)
>
> kernel_entry = riscv_load_kernel(machine, &s->soc[0],
> kernel_start_addr,
> - true, htif_symbol_callback);
> + true,
> + memmap[SPIKE_DRAM].base,
> + memmap[SPIKE_DRAM].size,
> + htif_symbol_callback);
> } else {
> /*
> * If dynamic firmware is used, it doesn't know where is the next mode
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 4e3efbee16..11f26b0dc0 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -1287,7 +1287,10 @@ static void virt_machine_done(Notifier *notifier, void *data)
> firmware_end_addr);
>
> kernel_entry = riscv_load_kernel(machine, &s->soc[0],
> - kernel_start_addr, true, NULL);
> + kernel_start_addr, true,
> + memmap[VIRT_DRAM].base,
> + memmap[VIRT_DRAM].size,
> + NULL);
> } else {
> /*
> * If dynamic firmware is used, it doesn't know where is the next mode
> diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
> index a2e4ae9cb0..987e1add38 100644
> --- a/include/hw/riscv/boot.h
> +++ b/include/hw/riscv/boot.h
> @@ -47,6 +47,8 @@ target_ulong riscv_load_kernel(MachineState *machine,
> RISCVHartArrayState *harts,
> target_ulong firmware_end_addr,
> bool load_initrd,
> + uint64_t ram_base,
> + uint64_t ram_size,
> symbol_fn_t sym_cb);
> uint64_t riscv_compute_fdt_addr(hwaddr dram_start, uint64_t dram_size,
> MachineState *ms);
> --
> 2.17.1
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v3 1/1] hw/riscv: Fix max size limit when put initrd to RAM
2023-04-05 5:53 ` Alistair Francis
@ 2023-04-05 9:38 ` Daniel Henrique Barboza
2023-04-07 4:26 ` Hang Xu
1 sibling, 0 replies; 8+ messages in thread
From: Daniel Henrique Barboza @ 2023-04-05 9:38 UTC (permalink / raw)
To: Alistair Francis, Hang Xu
Cc: qemu-riscv, qemu-devel, palmer, alistair.francis
On 4/5/23 02:53, Alistair Francis wrote:
> On Mon, Mar 13, 2023 at 11:12 PM Hang Xu <xuhang@eswincomputing.com> wrote:
>>
>> Because the starting address of ram is not necessarily 0,
>> the remaining free space in ram is
>> ram_size - (start - ram_base) instead of ram_size-start.
>
> I think this could be clearer. It's not clear here that you mean the
> free space after the kernel (for in the initrd).
>
>>
>> Signed-off-by: Hang Xu <xuhang@eswincomputing.com>
>> ---
>> hw/riscv/boot.c | 19 +++++++++++++------
>> hw/riscv/microchip_pfsoc.c | 5 ++++-
>> hw/riscv/opentitan.c | 2 +-
>> hw/riscv/sifive_e.c | 2 +-
>> hw/riscv/sifive_u.c | 5 ++++-
>> hw/riscv/spike.c | 5 ++++-
>> hw/riscv/virt.c | 5 ++++-
>> include/hw/riscv/boot.h | 2 ++
>> 8 files changed, 33 insertions(+), 12 deletions(-)
>>
>> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
>> index 52bf8e67de..cfbc376a82 100644
>> --- a/hw/riscv/boot.c
>> +++ b/hw/riscv/boot.c
>> @@ -173,13 +173,14 @@ target_ulong riscv_load_firmware(const char *firmware_filename,
>> exit(1);
>> }
>>
>> -static void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
>> +static void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry,
>> + uint64_t ram_base, uint64_t ram_size)
>> {
>> const char *filename = machine->initrd_filename;
>> - uint64_t mem_size = machine->ram_size;
>> void *fdt = machine->fdt;
>> hwaddr start, end;
>> ssize_t size;
>> + uint64_t max_initrd;
>>
>> g_assert(filename != NULL);
>>
>> @@ -193,12 +194,16 @@ static void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
>> * So for boards with less than 256MB of RAM we put the initrd
>> * halfway into RAM, and for boards with 256MB of RAM or more we put
>> * the initrd at 128MB.
>> + * A ram_size == 0, usually from a MemMapEntry[].size element,
>> + * means that the RAM block goes all the way to ms->ram_size.
>> */
>> - start = kernel_entry + MIN(mem_size / 2, 128 * MiB);
>> + ram_size = ram_size ? MIN(machine->ram_size, ram_size) : machine->ram_size;
>
> This doesn't seem right. If machine->ram_size is greater then the
> board can support we should tell the user and have them set a correct
> size
'ram_size' here is the size of the memory block that the board will use to put
initrd into.
Perhaps this var can be renamed to 'ram_block_size' or 'memmap_size' for clarity.
Daniel
>
>> + start = kernel_entry + MIN(ram_size / 2, 128 * MiB);
>> + max_initrd = ram_size - (start - ram_base);
>
> Good catch. Passing the base address of memory is a good move here.
>
> Alistair
>
>>
>> - size = load_ramdisk(filename, start, mem_size - start);
>> + size = load_ramdisk(filename, start, max_initrd);
>> if (size == -1) {
>> - size = load_image_targphys(filename, start, mem_size - start);
>> + size = load_image_targphys(filename, start, max_initrd);
>> if (size == -1) {
>> error_report("could not load ramdisk '%s'", filename);
>> exit(1);
>> @@ -217,6 +222,8 @@ target_ulong riscv_load_kernel(MachineState *machine,
>> RISCVHartArrayState *harts,
>> target_ulong kernel_start_addr,
>> bool load_initrd,
>> + uint64_t ram_base,
>> + uint64_t ram_size,
>> symbol_fn_t sym_cb)
>> {
>> const char *kernel_filename = machine->kernel_filename;
>> @@ -263,7 +270,7 @@ out:
>> }
>>
>> if (load_initrd && machine->initrd_filename) {
>> - riscv_load_initrd(machine, kernel_entry);
>> + riscv_load_initrd(machine, kernel_entry, ram_base, ram_size);
>> }
>>
>> if (fdt && machine->kernel_cmdline && *machine->kernel_cmdline) {
>> diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
>> index e81bbd12df..b42d90b89e 100644
>> --- a/hw/riscv/microchip_pfsoc.c
>> +++ b/hw/riscv/microchip_pfsoc.c
>> @@ -630,7 +630,10 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
>> firmware_end_addr);
>>
>> kernel_entry = riscv_load_kernel(machine, &s->soc.u_cpus,
>> - kernel_start_addr, true, NULL);
>> + kernel_start_addr, true,
>> + memmap[MICROCHIP_PFSOC_DRAM_LO].base,
>> + memmap[MICROCHIP_PFSOC_DRAM_LO].size,
>> + NULL);
>>
>> /* Compute the fdt load address in dram */
>> fdt_load_addr = riscv_compute_fdt_addr(memmap[MICROCHIP_PFSOC_DRAM_LO].base,
>> diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c
>> index b06944d382..bb663523d5 100644
>> --- a/hw/riscv/opentitan.c
>> +++ b/hw/riscv/opentitan.c
>> @@ -103,7 +103,7 @@ static void opentitan_board_init(MachineState *machine)
>> if (machine->kernel_filename) {
>> riscv_load_kernel(machine, &s->soc.cpus,
>> memmap[IBEX_DEV_RAM].base,
>> - false, NULL);
>> + false, 0, 0, NULL);
>> }
>> }
>>
>> diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
>> index 04939b60c3..5b47d539a6 100644
>> --- a/hw/riscv/sifive_e.c
>> +++ b/hw/riscv/sifive_e.c
>> @@ -116,7 +116,7 @@ static void sifive_e_machine_init(MachineState *machine)
>> if (machine->kernel_filename) {
>> riscv_load_kernel(machine, &s->soc.cpus,
>> memmap[SIFIVE_E_DEV_DTIM].base,
>> - false, NULL);
>> + false, 0, 0, NULL);
>> }
>> }
>>
>> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
>> index 35a335b8d0..b45fdc968c 100644
>> --- a/hw/riscv/sifive_u.c
>> +++ b/hw/riscv/sifive_u.c
>> @@ -599,7 +599,10 @@ static void sifive_u_machine_init(MachineState *machine)
>> firmware_end_addr);
>>
>> kernel_entry = riscv_load_kernel(machine, &s->soc.u_cpus,
>> - kernel_start_addr, true, NULL);
>> + kernel_start_addr, true,
>> + memmap[SIFIVE_U_DEV_DRAM].base,
>> + memmap[SIFIVE_U_DEV_DRAM].size,
>> + NULL);
>> } else {
>> /*
>> * If dynamic firmware is used, it doesn't know where is the next mode
>> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
>> index a584d5b3a2..e322ed8506 100644
>> --- a/hw/riscv/spike.c
>> +++ b/hw/riscv/spike.c
>> @@ -307,7 +307,10 @@ static void spike_board_init(MachineState *machine)
>>
>> kernel_entry = riscv_load_kernel(machine, &s->soc[0],
>> kernel_start_addr,
>> - true, htif_symbol_callback);
>> + true,
>> + memmap[SPIKE_DRAM].base,
>> + memmap[SPIKE_DRAM].size,
>> + htif_symbol_callback);
>> } else {
>> /*
>> * If dynamic firmware is used, it doesn't know where is the next mode
>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>> index 4e3efbee16..11f26b0dc0 100644
>> --- a/hw/riscv/virt.c
>> +++ b/hw/riscv/virt.c
>> @@ -1287,7 +1287,10 @@ static void virt_machine_done(Notifier *notifier, void *data)
>> firmware_end_addr);
>>
>> kernel_entry = riscv_load_kernel(machine, &s->soc[0],
>> - kernel_start_addr, true, NULL);
>> + kernel_start_addr, true,
>> + memmap[VIRT_DRAM].base,
>> + memmap[VIRT_DRAM].size,
>> + NULL);
>> } else {
>> /*
>> * If dynamic firmware is used, it doesn't know where is the next mode
>> diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
>> index a2e4ae9cb0..987e1add38 100644
>> --- a/include/hw/riscv/boot.h
>> +++ b/include/hw/riscv/boot.h
>> @@ -47,6 +47,8 @@ target_ulong riscv_load_kernel(MachineState *machine,
>> RISCVHartArrayState *harts,
>> target_ulong firmware_end_addr,
>> bool load_initrd,
>> + uint64_t ram_base,
>> + uint64_t ram_size,
>> symbol_fn_t sym_cb);
>> uint64_t riscv_compute_fdt_addr(hwaddr dram_start, uint64_t dram_size,
>> MachineState *ms);
>> --
>> 2.17.1
>>
>>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: Re: [PATCH v3 1/1] hw/riscv: Fix max size limit when put initrd to RAM
2023-04-05 5:53 ` Alistair Francis
2023-04-05 9:38 ` Daniel Henrique Barboza
@ 2023-04-07 4:26 ` Hang Xu
1 sibling, 0 replies; 8+ messages in thread
From: Hang Xu @ 2023-04-07 4:26 UTC (permalink / raw)
To: Alistair Francis
Cc: qemu-riscv, qemu-devel, palmer, Daniel Henrique Barboza,
alistair.francis
Hi,
On 2023-04-05 13:53, Alistair Francis wrote:
>
>On Mon, Mar 13, 2023 at 11:12 PM Hang Xu <xuhang@eswincomputing.com> wrote:
>>
>> Because the starting address of ram is not necessarily 0,
>> the remaining free space in ram is
>> ram_size - (start - ram_base) instead of ram_size-start.
>
>I think this could be clearer. It's not clear here that you mean the
>free space after the kernel (for in the initrd).
>
>>
>> Signed-off-by: Hang Xu <xuhang@eswincomputing.com>
>> ---
>> hw/riscv/boot.c | 19 +++++++++++++------
>> hw/riscv/microchip_pfsoc.c | 5 ++++-
>> hw/riscv/opentitan.c | 2 +-
>> hw/riscv/sifive_e.c | 2 +-
>> hw/riscv/sifive_u.c | 5 ++++-
>> hw/riscv/spike.c | 5 ++++-
>> hw/riscv/virt.c | 5 ++++-
>> include/hw/riscv/boot.h | 2 ++
>> 8 files changed, 33 insertions(+), 12 deletions(-)
>>
>> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
>> index 52bf8e67de..cfbc376a82 100644
>> --- a/hw/riscv/boot.c
>> +++ b/hw/riscv/boot.c
>> @@ -173,13 +173,14 @@ target_ulong riscv_load_firmware(const char *firmware_filename,
>> exit(1);
>> }
>>
>> -static void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
>> +static void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry,
>> + uint64_t ram_base, uint64_t ram_size)
>> {
>> const char *filename = machine->initrd_filename;
>> - uint64_t mem_size = machine->ram_size;
>> void *fdt = machine->fdt;
>> hwaddr start, end;
>> ssize_t size;
>> + uint64_t max_initrd;
>>
>> g_assert(filename != NULL);
>>
>> @@ -193,12 +194,16 @@ static void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
>> * So for boards with less than 256MB of RAM we put the initrd
>> * halfway into RAM, and for boards with 256MB of RAM or more we put
>> * the initrd at 128MB.
>> + * A ram_size == 0, usually from a MemMapEntry[].size element,
>> + * means that the RAM block goes all the way to ms->ram_size.
>> */
>> - start = kernel_entry + MIN(mem_size / 2, 128 * MiB);
>> + ram_size = ram_size ? MIN(machine->ram_size, ram_size) : machine->ram_size;
>
>This doesn't seem right. If machine->ram_size is greater then the
>board can support we should tell the user and have them set a correct
>size
>
Here is considering that some machines may divide machine->ram_size into
multiple non-contiguous ram_blocks, and the function parameter 'ram_size'
is only one of them, just like microchip_pfsoc does. In addition,
if ram_size == 0, usually it comes from a MemMapEntry[].size element ,which
uses machine->ram_size at this time. Consider the above two situations：
"ram_size = ram_size ? MIN(machine->ram_size, ram_size) : machine->ram_size"
Hang Xu
>> + start = kernel_entry + MIN(ram_size / 2, 128 * MiB);
>> + max_initrd = ram_size - (start - ram_base);
>
>Good catch. Passing the base address of memory is a good move here.
>
>Alistair
>
>>
>> - size = load_ramdisk(filename, start, mem_size - start);
>> + size = load_ramdisk(filename, start, max_initrd);
>> if (size == -1) {
>> - size = load_image_targphys(filename, start, mem_size - start);
>> + size = load_image_targphys(filename, start, max_initrd);
>> if (size == -1) {
>> error_report("could not load ramdisk '%s'", filename);
>> exit(1);
>> @@ -217,6 +222,8 @@ target_ulong riscv_load_kernel(MachineState *machine,
>> RISCVHartArrayState *harts,
>> target_ulong kernel_start_addr,
>> bool load_initrd,
>> + uint64_t ram_base,
>> + uint64_t ram_size,
>> symbol_fn_t sym_cb)
>> {
>> const char *kernel_filename = machine->kernel_filename;
>> @@ -263,7 +270,7 @@ out:
>> }
>>
>> if (load_initrd && machine->initrd_filename) {
>> - riscv_load_initrd(machine, kernel_entry);
>> + riscv_load_initrd(machine, kernel_entry, ram_base, ram_size);
>> }
>>
>> if (fdt && machine->kernel_cmdline && *machine->kernel_cmdline) {
>> diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
>> index e81bbd12df..b42d90b89e 100644
>> --- a/hw/riscv/microchip_pfsoc.c
>> +++ b/hw/riscv/microchip_pfsoc.c
>> @@ -630,7 +630,10 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
>> firmware_end_addr);
>>
>> kernel_entry = riscv_load_kernel(machine, &s->soc.u_cpus,
>> - kernel_start_addr, true, NULL);
>> + kernel_start_addr, true,
>> + memmap[MICROCHIP_PFSOC_DRAM_LO].base,
>> + memmap[MICROCHIP_PFSOC_DRAM_LO].size,
>> + NULL);
>>
>> /* Compute the fdt load address in dram */
>> fdt_load_addr = riscv_compute_fdt_addr(memmap[MICROCHIP_PFSOC_DRAM_LO].base,
>> diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c
>> index b06944d382..bb663523d5 100644
>> --- a/hw/riscv/opentitan.c
>> +++ b/hw/riscv/opentitan.c
>> @@ -103,7 +103,7 @@ static void opentitan_board_init(MachineState *machine)
>> if (machine->kernel_filename) {
>> riscv_load_kernel(machine, &s->soc.cpus,
>> memmap[IBEX_DEV_RAM].base,
>> - false, NULL);
>> + false, 0, 0, NULL);
>> }
>> }
>>
>> diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
>> index 04939b60c3..5b47d539a6 100644
>> --- a/hw/riscv/sifive_e.c
>> +++ b/hw/riscv/sifive_e.c
>> @@ -116,7 +116,7 @@ static void sifive_e_machine_init(MachineState *machine)
>> if (machine->kernel_filename) {
>> riscv_load_kernel(machine, &s->soc.cpus,
>> memmap[SIFIVE_E_DEV_DTIM].base,
>> - false, NULL);
>> + false, 0, 0, NULL);
>> }
>> }
>>
>> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
>> index 35a335b8d0..b45fdc968c 100644
>> --- a/hw/riscv/sifive_u.c
>> +++ b/hw/riscv/sifive_u.c
>> @@ -599,7 +599,10 @@ static void sifive_u_machine_init(MachineState *machine)
>> firmware_end_addr);
>>
>> kernel_entry = riscv_load_kernel(machine, &s->soc.u_cpus,
>> - kernel_start_addr, true, NULL);
>> + kernel_start_addr, true,
>> + memmap[SIFIVE_U_DEV_DRAM].base,
>> + memmap[SIFIVE_U_DEV_DRAM].size,
>> + NULL);
>> } else {
>> /*
>> * If dynamic firmware is used, it doesn't know where is the next mode
>> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
>> index a584d5b3a2..e322ed8506 100644
>> --- a/hw/riscv/spike.c
>> +++ b/hw/riscv/spike.c
>> @@ -307,7 +307,10 @@ static void spike_board_init(MachineState *machine)
>>
>> kernel_entry = riscv_load_kernel(machine, &s->soc[0],
>> kernel_start_addr,
>> - true, htif_symbol_callback);
>> + true,
>> + memmap[SPIKE_DRAM].base,
>> + memmap[SPIKE_DRAM].size,
>> + htif_symbol_callback);
>> } else {
>> /*
>> * If dynamic firmware is used, it doesn't know where is the next mode
>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>> index 4e3efbee16..11f26b0dc0 100644
>> --- a/hw/riscv/virt.c
>> +++ b/hw/riscv/virt.c
>> @@ -1287,7 +1287,10 @@ static void virt_machine_done(Notifier *notifier, void *data)
>> firmware_end_addr);
>>
>> kernel_entry = riscv_load_kernel(machine, &s->soc[0],
>> - kernel_start_addr, true, NULL);
>> + kernel_start_addr, true,
>> + memmap[VIRT_DRAM].base,
>> + memmap[VIRT_DRAM].size,
>> + NULL);
>> } else {
>> /*
>> * If dynamic firmware is used, it doesn't know where is the next mode
>> diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
>> index a2e4ae9cb0..987e1add38 100644
>> --- a/include/hw/riscv/boot.h
>> +++ b/include/hw/riscv/boot.h
>> @@ -47,6 +47,8 @@ target_ulong riscv_load_kernel(MachineState *machine,
>> RISCVHartArrayState *harts,
>> target_ulong firmware_end_addr,
>> bool load_initrd,
>> + uint64_t ram_base,
>> + uint64_t ram_size,
>> symbol_fn_t sym_cb);
>> uint64_t riscv_compute_fdt_addr(hwaddr dram_start, uint64_t dram_size,
>> MachineState *ms);
>> --
>> 2.17.1
>>
>>
^ permalink raw reply [flat|nested] 8+ messages in thread