* [PATCH] hw/riscv: Fix the bug of maximum size limit when put initrd to RAM @ 2023-02-27 2:39 Hang Xu 2023-02-27 13:29 ` Daniel Henrique Barboza 0 siblings, 1 reply; 5+ messages in thread From: Hang Xu @ 2023-02-27 2:39 UTC (permalink / raw) To: qemu-riscv; +Cc: alistair.francis, bmeng, philmd, 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] hw/riscv: Fix the bug of maximum size limit when put initrd to RAM 2023-02-27 2:39 [PATCH] hw/riscv: Fix the bug of maximum size limit when put initrd to RAM Hang Xu @ 2023-02-27 13:29 ` Daniel Henrique Barboza 2023-02-28 3:44 ` Hang Xu 0 siblings, 1 reply; 5+ messages in thread From: Daniel Henrique Barboza @ 2023-02-27 13:29 UTC (permalink / raw) To: Hang Xu, qemu-riscv; +Cc: alistair.francis, bmeng, philmd 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); ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Re: [PATCH] hw/riscv: Fix the bug of maximum size limit when put initrd to RAM 2023-02-27 13:29 ` Daniel Henrique Barboza @ 2023-02-28 3:44 ` Hang Xu 2023-02-28 13:17 ` Daniel Henrique Barboza 0 siblings, 1 reply; 5+ messages in thread From: Hang Xu @ 2023-02-28 3:44 UTC (permalink / raw) To: Daniel Henrique Barboza, qemu-riscv; +Cc: alistair.francis, bmeng, philmd 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); ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] hw/riscv: Fix the bug of maximum size limit when put initrd to RAM 2023-02-28 3:44 ` Hang Xu @ 2023-02-28 13:17 ` Daniel Henrique Barboza 2023-03-05 11:11 ` Hang Xu 0 siblings, 1 reply; 5+ messages in thread From: Daniel Henrique Barboza @ 2023-02-28 13:17 UTC (permalink / raw) To: Hang Xu, qemu-riscv; +Cc: alistair.francis, bmeng, philmd 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); ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] hw/riscv: Fix the bug of maximum size limit when put initrd to RAM 2023-02-28 13:17 ` Daniel Henrique Barboza @ 2023-03-05 11:11 ` Hang Xu 0 siblings, 0 replies; 5+ messages in thread From: Hang Xu @ 2023-03-05 11:11 UTC (permalink / raw) To: Daniel Henrique Barboza, qemu-riscv; +Cc: alistair.francis, bmeng, philmd 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); ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-03-05 11:11 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-02-27 2:39 [PATCH] hw/riscv: Fix the bug of maximum size limit when put initrd to RAM Hang Xu 2023-02-27 13:29 ` Daniel Henrique Barboza 2023-02-28 3:44 ` Hang Xu 2023-02-28 13:17 ` Daniel Henrique Barboza 2023-03-05 11:11 ` Hang Xu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.