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

2023-04-06 Thread Hang Xu
Hi,
On 2023-04-05 13:53,  Alistair Francis wrote:
>
 
 
 
>On Mon, Mar 13, 2023 at 11:12 PM 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.
>
>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 
>> ---
>>  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, >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);

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

2023-04-05 Thread Daniel Henrique Barboza




On 4/5/23 02:53, Alistair Francis wrote:

On Mon, Mar 13, 2023 at 11:12 PM 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.


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 
---
  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, >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, >soc.cpus,
memmap[IBEX_DEV_RAM].base,
- 

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

2023-04-04 Thread Alistair Francis
On Mon, Mar 13, 2023 at 11:12 PM 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.

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 
> ---
>  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, >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, >soc.cpus,
>memmap[IBEX_DEV_RAM].base,
> -  false, NULL);
> +  false, 0, 0, NULL);
>   

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

2023-03-13 Thread Daniel Henrique Barboza




On 3/13/23 12:49, Anup Patel wrote:

On Mon, Mar 13, 2023 at 7:49 AM 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 


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, >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, >soc.cpus,
memmap[IBEX_DEV_RAM].base,
-  false, NULL);
+  false, 0, 0, NULL);
  }
  }

diff --git 

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

2023-03-13 Thread Anup Patel
On Mon, Mar 13, 2023 at 7:49 AM 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 

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

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

2023-03-13 Thread Hang Xu
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 
---
 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, >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, >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, >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
+++ 

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

2023-03-13 Thread Daniel Henrique Barboza




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


Reviewed-by: Daniel Henrique Barboza 


  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, >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, >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, >soc.cpus,
memmap[SIFIVE_E_DEV_DTIM].base,
-  false, NULL);
+  false, 0, 0,