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