|
| From: | Daniel Henrique Barboza |
| Subject: | Re: [PATCH] hw/riscv: Fix the bug of maximum size limit when put initrd to RAM |
| Date: | Tue, 28 Feb 2023 10:17:51 -0300 |
| User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.8.0 |
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, DanielYes, 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 Xuif (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);
| [Prev in Thread] | Current Thread | [Next in Thread] |