Re: [PATCH] hw/riscv: Fix the bug of maximum size limit when put initrd to RAM

[Daniel Henrique Barboza]

Hi,

On 2/26/23 23:39, Hang Xu wrote:
> The space available for initrd is "ram_size + ram_base - start"
> instead of "ram_size - start"
>
> Signed-off-by: Hang Xu <xuhang@eswincomputing.com>
> ---
>   hw/riscv/boot.c            | 8 +++++---
>   hw/riscv/microchip_pfsoc.c | 2 +-
>   hw/riscv/sifive_u.c        | 2 +-
>   hw/riscv/spike.c           | 2 +-
>   hw/riscv/virt.c            | 2 +-
>   include/hw/riscv/boot.h    | 3 ++-
>   6 files changed, 11 insertions(+), 8 deletions(-)

You'll need to rebase this patch with current master. riscv_load_initrd() is a 
static
function inside target/riscv/boot.c since 8b64475bd529, which landed upstream 
today.

One more thing:

> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index c7e0e50bd8..749dba5360 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -209,7 +209,8 @@ target_ulong riscv_load_kernel(MachineState *machine,
>       exit(1);
>   }
>   
> -void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
> +void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry,
> +                       uint64_t mem_base)
>   {
>       const char *filename = machine->initrd_filename;
>       uint64_t mem_size = machine->ram_size;
> @@ -231,10 +232,11 @@ void riscv_load_initrd(MachineState *machine, uint64_t 
> kernel_entry)
>        * the initrd at 128MB.
>        */
>       start = kernel_entry + MIN(mem_size / 2, 128 * MiB);
> +    uint64_t max_size = mem_size + mem_base - start;
>   
> -    size = load_ramdisk(filename, start, mem_size - start);
> +    size = load_ramdisk(filename, start, max_size);


I don't understand this logic. If we want initrd to be loaded at 'start', and 
we have a
total amount of RAM of mem_size (since it's == machine->ram_size), then the 
initrd can't
really be bigger than mem_size - start.

It would help if you can clarify a bit more on what's the initrd maximum size 
limit bug
you're seeing. There is a chance that the problem is with how we're calculating 
'start'.


Thanks,

Daniel




>       if (size == -1) {
> -        size = load_image_targphys(filename, start, mem_size - start);
> +        size = load_image_targphys(filename, start, max_size);
>           if (size == -1) {
>               error_report("could not load ramdisk '%s'", filename);
>               exit(1);
> diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
> index 2b91e49561..965d155fd3 100644
> --- a/hw/riscv/microchip_pfsoc.c
> +++ b/hw/riscv/microchip_pfsoc.c
> @@ -632,7 +632,7 @@ static void microchip_icicle_kit_machine_init(MachineState 
> *machine)
>           kernel_entry = riscv_load_kernel(machine, kernel_start_addr, NULL);
>   
>           if (machine->initrd_filename) {
> -            riscv_load_initrd(machine, kernel_entry);
> +            riscv_load_initrd(machine, kernel_entry, 
> memmap[MICROCHIP_PFSOC_DRAM_LO].base);
>           }
>   
>           if (machine->kernel_cmdline && *machine->kernel_cmdline) {
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index d3ab7a9cda..aa5d5bc610 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -601,7 +601,7 @@ static void sifive_u_machine_init(MachineState *machine)
>           kernel_entry = riscv_load_kernel(machine, kernel_start_addr, NULL);
>   
>           if (machine->initrd_filename) {
> -            riscv_load_initrd(machine, kernel_entry);
> +            riscv_load_initrd(machine, kernel_entry, 
> memmap[SIFIVE_U_DEV_DRAM].base);
>           }
>   
>           if (machine->kernel_cmdline && *machine->kernel_cmdline) {
> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> index cc3f6dac17..729f6f91fd 100644
> --- a/hw/riscv/spike.c
> +++ b/hw/riscv/spike.c
> @@ -309,7 +309,7 @@ static void spike_board_init(MachineState *machine)
>                                            htif_symbol_callback);
>   
>           if (machine->initrd_filename) {
> -            riscv_load_initrd(machine, kernel_entry);
> +            riscv_load_initrd(machine, kernel_entry, memmap[SPIKE_DRAM].base);
>           }
>   
>           if (machine->kernel_cmdline && *machine->kernel_cmdline) {
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index b81081c70b..2342de646e 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -1280,7 +1280,7 @@ static void virt_machine_done(Notifier *notifier, void 
> *data)
>           kernel_entry = riscv_load_kernel(machine, kernel_start_addr, NULL);
>   
>           if (machine->initrd_filename) {
> -            riscv_load_initrd(machine, kernel_entry);
> +            riscv_load_initrd(machine, kernel_entry, memmap[VIRT_DRAM].base);
>           }
>   
>           if (machine->kernel_cmdline && *machine->kernel_cmdline) {
> diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
> index 511390f60e..14db53ef13 100644
> --- a/include/hw/riscv/boot.h
> +++ b/include/hw/riscv/boot.h
> @@ -46,7 +46,8 @@ target_ulong riscv_load_firmware(const char 
> *firmware_filename,
>   target_ulong riscv_load_kernel(MachineState *machine,
>                                  target_ulong firmware_end_addr,
>                                  symbol_fn_t sym_cb);
> -void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry);
> +void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry,
> +                       uint64_t mem_base);
>   uint64_t riscv_compute_fdt_addr(hwaddr dram_start, uint64_t dram_size,
>                                   MachineState *ms);
>   void riscv_load_fdt(hwaddr fdt_addr, void *fdt);