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