Re: [PATCH v3 1/1] hw/riscv: Fix max size limit when put initrd to RAM

[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
>
>