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

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

[Hang Xu]

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

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);
     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);
-- 
2.17.1

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

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

[Hang Xu]

Hi,
On 2023-02-27 21:29,  Daniel Henrique Barboza wrote:
>
>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.
> 

Thanks,I'll rebase this patch with the current master later.

>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 

Yes, we can get the total size of ram, but the starting address of ram is not necessarily 0x0, so the maximum absolute address of ram is 'ram_base + ram_size',
'start' is an absolute address, not an offset in ram. 
Therefore, the remaining size in ram is "ram_base + ram_size - start".
For example:
RAM starting address: ram_base=0x40000000,RAM size: ram_size=0x20000000,then the range address of ram is 0x40000000~0x60000000
initrd start position: start=0x5000000,
Then according to the previous calculation method, the remaining available size of ram (also the maximum size of initrd): max_size=ram_size - ram_base = -0x20000000,  which is wrong. 
Maybe everyone used to set ram_size to a large value when starting qemu, so the error did not come out.
In fact, the remaining available size of ram (also the maximum size of initrd): max_size = ram_base + ram_size - start=0x10000000

Thanks,
Hang Xu
>
>
>
>
>>       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);

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

[Daniel Henrique Barboza]

On 2/28/23 00:44, Hang Xu wrote:
> Hi,
> On 2023-02-27 21:29,  Daniel Henrique Barboza wrote:
>>
>> 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.
>>
> 
> Thanks,I'll rebase this patch with the current master later.
> 
>> 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
> 
> Yes, we can get the total size of ram, but the starting address of ram is not necessarily 0x0, so the maximum absolute address of ram is 'ram_base + ram_size',
> 'start' is an absolute address, not an offset in ram.
> Therefore, the remaining size in ram is "ram_base + ram_size - start".
> For example:
> RAM starting address: ram_base=0x40000000,RAM size: ram_size=0x20000000,then the range address of ram is 0x40000000~0x60000000
> initrd start position: start=0x5000000,
> Then according to the previous calculation method, the remaining available size of ram (also the maximum size of initrd): max_size=ram_size - ram_base = -0x20000000,  which is wrong.


Yeah, that makes sense. mem_size - start is giving a bogus result because 'start'
is not an offset of the RAM block in which initrd would be inserted.

However there's still one problem here. We're assuming that (mem_size - start) is
contiguous RAM. It is not. The PolarFire SOC board has two RAM blocks that are
separated with a gap and mem_size is the sum of the two regions. We had some
FDT changes based on that (see commit 4b402886ac89732f9,  "hw/riscv: change
riscv_compute_fdt_addr() semantics").

Note that this patch doesn't take that into account either.

> Maybe everyone used to set ram_size to a large value when starting qemu, so the error did not come out.

That's probably what is happening.

There are a few alternatives here. I see boards from other archs using a
theoretical/architectural max initrd size limit here, not an actual value that
can vary with the board memmap and kernel size. E.g. x86 defaults max initrd
size to UINT32_MAX.

If we want to fix what we already have then I suggest to go to a similar route
that riscv_compute_fdt_addr() is doing. We would need not just dram_base, but
also dram_size. And 'dram_size' in that case would be the amount of contiguous
RAM we have in the MemMapEntry.

Assuming we have dram_base and dram_size, and the caveat of dram_size == 0 meaning
that the block goes to the end of RAM (see riscv_compute_fdt_addr()), this patch
would become something like:

     start = kernel_entry + MIN(mem_size / 2, 128 * MiB);

     max_initrd = dram_size - (dram_base - start);

     size = load_ramdisk(filename, start, max_initrd);



Thanks,


Daniel


> In fact, the remaining available size of ram (also the maximum size of initrd): max_size = ram_base + ram_size - start=0x10000000
> 
> Thanks,
> Hang Xu
>>
>>
>>
>>
>>>         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);

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

[Hang Xu]

Hi,
On 2023-02-28 21:17,  Daniel Henrique Barboza wrote:
>
 
 
 
>
>
>On 2/28/23 00:44, Hang Xu wrote:
>> Hi,
>> On 2023-02-27 21:29,  Daniel Henrique Barboza wrote:
>>>
>>> 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.
>>>
>>
>> Thanks,I'll rebase this patch with the current master later.
>>
>>> 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
>>
>> Yes, we can get the total size of ram, but the starting address of ram is not necessarily 0x0, so the maximum absolute address of ram is 'ram_base + ram_size',
>> 'start' is an absolute address, not an offset in ram.
>> Therefore, the remaining size in ram is "ram_base + ram_size - start".
>> For example:
>> RAM starting address: ram_base=0x40000000,RAM size: ram_size=0x20000000,then the range address of ram is 0x40000000~0x60000000
>> initrd start position: start=0x5000000,
>> Then according to the previous calculation method, the remaining available size of ram (also the maximum size of initrd): max_size=ram_size - ram_base = -0x20000000,  which is wrong.
>
>
>Yeah, that makes sense. mem_size - start is giving a bogus result because 'start'
>is not an offset of the RAM block in which initrd would be inserted.
>
>However there's still one problem here. We're assuming that (mem_size - start) is
>contiguous RAM. It is not. The PolarFire SOC board has two RAM blocks that are
>separated with a gap and mem_size is the sum of the two regions. We had some
>FDT changes based on that (see commit 4b402886ac89732f9,  "hw/riscv: change
>riscv_compute_fdt_addr() semantics").
> 
You are right, this patch does still have the problem you mentioned. Your suggestion is good I will refer to the modification made
of fdt (commit 4b402886ac89732f9,  "hw/riscv: change riscv_compute_fdt_addr() semantics")
>Note that this patch doesn't take that into account either.
>
>> Maybe everyone used to set ram_size to a large value when starting qemu, so the error did not come out.
>
>That's probably what is happening.
>
>There are a few alternatives here. I see boards from other archs using a
>theoretical/architectural max initrd size limit here, not an actual value that
>can vary with the board memmap and kernel size. E.g. x86 defaults max initrd
>size to UINT32_MAX. 

>
>If we want to fix what we already have then I suggest to go to a similar route
>that riscv_compute_fdt_addr() is doing. We would need not just dram_base, but
>also dram_size. And 'dram_size' in that case would be the amount of contiguous
>RAM we have in the MemMapEntry. 

Thank you for providing two good methods, I think it is better to fix what we already have,
we need to consider the remaining ram space.
I'm going to add another appropriate dram_size parameter to the riscv_load_initrd() function as you suggested.

I'll rebase this patch with the newest master later(maybe with a new commit),
sincerely hope you can continue to provide advice


>Assuming we have dram_base and dram_size, and the caveat of dram_size == 0 meaning
>that the block goes to the end of RAM (see riscv_compute_fdt_addr()), this patch
>would become something like:
> 

>     start = kernel_entry + MIN(mem_size / 2, 128 * MiB);
>
>     max_initrd = dram_size - (dram_base - start);
>
>     size = load_ramdisk(filename, start, max_initrd);
> 

I'll rebase this patch with the newest master later(maybe with a new commit),
sincerely hope you can continue to provide advice


Thanks

Hang Xu
>
>
>Thanks,
>
>
>Daniel
>
>
>> In fact, the remaining available size of ram (also the maximum size of initrd): max_size = ram_base + ram_size - start=0x10000000
>>
>> Thanks,
>> Hang Xu
>>>
>>>
>>>
>>>
>>>>         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);