From: "Hang Xu" <xuhang@eswincomputing.com>
To: "Alistair Francis" <alistair23@gmail.com>
Cc: qemu-riscv <qemu-riscv@nongnu.org>,
qemu-devel <qemu-devel@nongnu.org>, palmer <palmer@rivosinc.com>,
"Daniel Henrique Barboza" <dbarboza@ventanamicro.com>,
alistair.francis <alistair.francis@wdc.com>
Subject: Re: Re: [PATCH v3 1/1] hw/riscv: Fix max size limit when put initrd to RAM
Date: Fri, 7 Apr 2023 12:26:17 +0800 [thread overview]
Message-ID: <2023040712261724240210@eswincomputing.com> (raw)
In-Reply-To: CAKmqyKNuA-4SpfN+RNDo8wOFgVuD-s-tXGOg+1po4KYW8KfhUQ@mail.gmail.com
Hi,
On 2023-04-05 13:53, Alistair Francis wrote:
>
>On Mon, Mar 13, 2023 at 11:12 PM Hang Xu <xuhang@eswincomputing.com> wrote:
>>
>> Because the starting address of ram is not necessarily 0,
>> the remaining free space in ram is
>> ram_size - (start - ram_base) instead of ram_size-start.
>
>I think this could be clearer. It's not clear here that you mean the
>free space after the kernel (for in the initrd).
>
>>
>> Signed-off-by: Hang Xu <xuhang@eswincomputing.com>
>> ---
>> hw/riscv/boot.c | 19 +++++++++++++------
>> hw/riscv/microchip_pfsoc.c | 5 ++++-
>> hw/riscv/opentitan.c | 2 +-
>> hw/riscv/sifive_e.c | 2 +-
>> hw/riscv/sifive_u.c | 5 ++++-
>> hw/riscv/spike.c | 5 ++++-
>> hw/riscv/virt.c | 5 ++++-
>> include/hw/riscv/boot.h | 2 ++
>> 8 files changed, 33 insertions(+), 12 deletions(-)
>>
>> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
>> index 52bf8e67de..cfbc376a82 100644
>> --- a/hw/riscv/boot.c
>> +++ b/hw/riscv/boot.c
>> @@ -173,13 +173,14 @@ target_ulong riscv_load_firmware(const char *firmware_filename,
>> exit(1);
>> }
>>
>> -static void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
>> +static void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry,
>> + uint64_t ram_base, uint64_t ram_size)
>> {
>> const char *filename = machine->initrd_filename;
>> - uint64_t mem_size = machine->ram_size;
>> void *fdt = machine->fdt;
>> hwaddr start, end;
>> ssize_t size;
>> + uint64_t max_initrd;
>>
>> g_assert(filename != NULL);
>>
>> @@ -193,12 +194,16 @@ static void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
>> * So for boards with less than 256MB of RAM we put the initrd
>> * halfway into RAM, and for boards with 256MB of RAM or more we put
>> * the initrd at 128MB.
>> + * A ram_size == 0, usually from a MemMapEntry[].size element,
>> + * means that the RAM block goes all the way to ms->ram_size.
>> */
>> - start = kernel_entry + MIN(mem_size / 2, 128 * MiB);
>> + ram_size = ram_size ? MIN(machine->ram_size, ram_size) : machine->ram_size;
>
>This doesn't seem right. If machine->ram_size is greater then the
>board can support we should tell the user and have them set a correct
>size
>
Here is considering that some machines may divide machine->ram_size into
multiple non-contiguous ram_blocks, and the function parameter 'ram_size'
is only one of them, just like microchip_pfsoc does. In addition,
if ram_size == 0, usually it comes from a MemMapEntry[].size element ,which
uses machine->ram_size at this time. Consider the above two situations：
"ram_size = ram_size ? MIN(machine->ram_size, ram_size) : machine->ram_size"
Hang Xu
>> + start = kernel_entry + MIN(ram_size / 2, 128 * MiB);
>> + max_initrd = ram_size - (start - ram_base);
>
>Good catch. Passing the base address of memory is a good move here.
>
>Alistair
>
>>
>> - size = load_ramdisk(filename, start, mem_size - start);
>> + size = load_ramdisk(filename, start, max_initrd);
>> if (size == -1) {
>> - size = load_image_targphys(filename, start, mem_size - start);
>> + size = load_image_targphys(filename, start, max_initrd);
>> if (size == -1) {
>> error_report("could not load ramdisk '%s'", filename);
>> exit(1);
>> @@ -217,6 +222,8 @@ target_ulong riscv_load_kernel(MachineState *machine,
>> RISCVHartArrayState *harts,
>> target_ulong kernel_start_addr,
>> bool load_initrd,
>> + uint64_t ram_base,
>> + uint64_t ram_size,
>> symbol_fn_t sym_cb)
>> {
>> const char *kernel_filename = machine->kernel_filename;
>> @@ -263,7 +270,7 @@ out:
>> }
>>
>> if (load_initrd && machine->initrd_filename) {
>> - riscv_load_initrd(machine, kernel_entry);
>> + riscv_load_initrd(machine, kernel_entry, ram_base, ram_size);
>> }
>>
>> if (fdt && machine->kernel_cmdline && *machine->kernel_cmdline) {
>> diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
>> index e81bbd12df..b42d90b89e 100644
>> --- a/hw/riscv/microchip_pfsoc.c
>> +++ b/hw/riscv/microchip_pfsoc.c
>> @@ -630,7 +630,10 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
>> firmware_end_addr);
>>
>> kernel_entry = riscv_load_kernel(machine, &s->soc.u_cpus,
>> - kernel_start_addr, true, NULL);
>> + kernel_start_addr, true,
>> + memmap[MICROCHIP_PFSOC_DRAM_LO].base,
>> + memmap[MICROCHIP_PFSOC_DRAM_LO].size,
>> + NULL);
>>
>> /* Compute the fdt load address in dram */
>> fdt_load_addr = riscv_compute_fdt_addr(memmap[MICROCHIP_PFSOC_DRAM_LO].base,
>> diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c
>> index b06944d382..bb663523d5 100644
>> --- a/hw/riscv/opentitan.c
>> +++ b/hw/riscv/opentitan.c
>> @@ -103,7 +103,7 @@ static void opentitan_board_init(MachineState *machine)
>> if (machine->kernel_filename) {
>> riscv_load_kernel(machine, &s->soc.cpus,
>> memmap[IBEX_DEV_RAM].base,
>> - false, NULL);
>> + false, 0, 0, NULL);
>> }
>> }
>>
>> diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
>> index 04939b60c3..5b47d539a6 100644
>> --- a/hw/riscv/sifive_e.c
>> +++ b/hw/riscv/sifive_e.c
>> @@ -116,7 +116,7 @@ static void sifive_e_machine_init(MachineState *machine)
>> if (machine->kernel_filename) {
>> riscv_load_kernel(machine, &s->soc.cpus,
>> memmap[SIFIVE_E_DEV_DTIM].base,
>> - false, NULL);
>> + false, 0, 0, NULL);
>> }
>> }
>>
>> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
>> index 35a335b8d0..b45fdc968c 100644
>> --- a/hw/riscv/sifive_u.c
>> +++ b/hw/riscv/sifive_u.c
>> @@ -599,7 +599,10 @@ static void sifive_u_machine_init(MachineState *machine)
>> firmware_end_addr);
>>
>> kernel_entry = riscv_load_kernel(machine, &s->soc.u_cpus,
>> - kernel_start_addr, true, NULL);
>> + kernel_start_addr, true,
>> + memmap[SIFIVE_U_DEV_DRAM].base,
>> + memmap[SIFIVE_U_DEV_DRAM].size,
>> + NULL);
>> } else {
>> /*
>> * If dynamic firmware is used, it doesn't know where is the next mode
>> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
>> index a584d5b3a2..e322ed8506 100644
>> --- a/hw/riscv/spike.c
>> +++ b/hw/riscv/spike.c
>> @@ -307,7 +307,10 @@ static void spike_board_init(MachineState *machine)
>>
>> kernel_entry = riscv_load_kernel(machine, &s->soc[0],
>> kernel_start_addr,
>> - true, htif_symbol_callback);
>> + true,
>> + memmap[SPIKE_DRAM].base,
>> + memmap[SPIKE_DRAM].size,
>> + htif_symbol_callback);
>> } else {
>> /*
>> * If dynamic firmware is used, it doesn't know where is the next mode
>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>> index 4e3efbee16..11f26b0dc0 100644
>> --- a/hw/riscv/virt.c
>> +++ b/hw/riscv/virt.c
>> @@ -1287,7 +1287,10 @@ static void virt_machine_done(Notifier *notifier, void *data)
>> firmware_end_addr);
>>
>> kernel_entry = riscv_load_kernel(machine, &s->soc[0],
>> - kernel_start_addr, true, NULL);
>> + kernel_start_addr, true,
>> + memmap[VIRT_DRAM].base,
>> + memmap[VIRT_DRAM].size,
>> + NULL);
>> } else {
>> /*
>> * If dynamic firmware is used, it doesn't know where is the next mode
>> diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
>> index a2e4ae9cb0..987e1add38 100644
>> --- a/include/hw/riscv/boot.h
>> +++ b/include/hw/riscv/boot.h
>> @@ -47,6 +47,8 @@ target_ulong riscv_load_kernel(MachineState *machine,
>> RISCVHartArrayState *harts,
>> target_ulong firmware_end_addr,
>> bool load_initrd,
>> + uint64_t ram_base,
>> + uint64_t ram_size,
>> symbol_fn_t sym_cb);
>> uint64_t riscv_compute_fdt_addr(hwaddr dram_start, uint64_t dram_size,
>> MachineState *ms);
>> --
>> 2.17.1
>>
>>
prev parent reply other threads:[~2023-04-07 4:27 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-13 2:18 [PATCH v3 0/1] Fix max initrd size limit when put initrd to RAM Hang Xu
2023-03-13 2:18 ` [PATCH v3 1/1] hw/riscv: Fix max " Hang Xu
2023-03-13 12:29 ` Daniel Henrique Barboza
2023-03-13 15:49 ` Anup Patel
2023-03-13 21:32 ` Daniel Henrique Barboza
2023-04-05 5:53 ` Alistair Francis
2023-04-05 9:38 ` Daniel Henrique Barboza
2023-04-07 4:26 ` Hang Xu [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2023040712261724240210@eswincomputing.com \
--to=xuhang@eswincomputing.com \
--cc=alistair.francis@wdc.com \
--cc=alistair23@gmail.com \
--cc=dbarboza@ventanamicro.com \
--cc=palmer@rivosinc.com \
--cc=qemu-devel@nongnu.org \
--cc=qemu-riscv@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).