[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
RE: [PATCH v2 4/4] hw/riscv: Load the kernel after the firmware
|
From: |
Anup Patel |
|
Subject: |
RE: [PATCH v2 4/4] hw/riscv: Load the kernel after the firmware |
|
Date: |
Fri, 6 Nov 2020 04:15:38 +0000 |
> -----Original Message-----
> From: Qemu-riscv <qemu-riscv-
> bounces+anup.patel=wdc.com@nongnu.org> On Behalf Of Palmer Dabbelt
> Sent: 06 November 2020 08:19
> To: alistair23@gmail.com
> Cc: qemu-riscv@nongnu.org; bmeng.cn@gmail.com; Alistair Francis
> <Alistair.Francis@wdc.com>; qemu-devel@nongnu.org
> Subject: Re: [PATCH v2 4/4] hw/riscv: Load the kernel after the firmware
>
> On Tue, 20 Oct 2020 08:46:45 PDT (-0700), alistair23@gmail.com wrote:
> > On Mon, Oct 19, 2020 at 4:17 PM Palmer Dabbelt <palmer@dabbelt.com>
> wrote:
> >>
> >> On Tue, 13 Oct 2020 17:17:33 PDT (-0700), Alistair Francis wrote:
> >> > Instead of loading the kernel at a hardcoded start address, let's
> >> > load the kernel at the next alligned address after the end of the
> firmware.
> >> >
> >> > This should have no impact for current users of OpenSBI, but will
> >> > allow loading a noMMU kernel at the start of memory.
> >> >
> >> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> >> > ---
> >> > include/hw/riscv/boot.h | 3 +++
> >> > hw/riscv/boot.c | 19 ++++++++++++++-----
> >> > hw/riscv/opentitan.c | 3 ++-
> >> > hw/riscv/sifive_e.c | 3 ++-
> >> > hw/riscv/sifive_u.c | 10 ++++++++--
> >> > hw/riscv/spike.c | 11 ++++++++---
> >> > hw/riscv/virt.c | 11 ++++++++---
> >> > 7 files changed, 45 insertions(+), 15 deletions(-)
> >> >
> >> > diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
> >> > index 2975ed1a31..0b01988727 100644
> >> > --- a/include/hw/riscv/boot.h
> >> > +++ b/include/hw/riscv/boot.h
> >> > @@ -25,6 +25,8 @@
> >> >
> >> > bool riscv_is_32_bit(MachineState *machine);
> >> >
> >> > +target_ulong riscv_calc_kernel_start_addr(MachineState *machine,
> >> > + target_ulong
> >> > +firmware_end_addr);
> >> > target_ulong riscv_find_and_load_firmware(MachineState *machine,
> >> > const char
> >> > *default_machine_firmware,
> >> > hwaddr
> >> > firmware_load_addr, @@ -34,6 +36,7 @@ target_ulong
> riscv_load_firmware(const char *firmware_filename,
> >> > hwaddr firmware_load_addr,
> >> > symbol_fn_t sym_cb);
> >> > target_ulong riscv_load_kernel(const char *kernel_filename,
> >> > + target_ulong firmware_end_addr,
> >> > symbol_fn_t sym_cb); hwaddr
> >> > riscv_load_initrd(const char *filename, uint64_t mem_size,
> >> > uint64_t kernel_entry, hwaddr *start);
> >> > diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c index
> >> > 5dea644f47..9b3fe3fb1e 100644
> >> > --- a/hw/riscv/boot.c
> >> > +++ b/hw/riscv/boot.c
> >> > @@ -33,10 +33,8 @@
> >> > #include <libfdt.h>
> >> >
> >> > #if defined(TARGET_RISCV32)
> >> > -# define KERNEL_BOOT_ADDRESS 0x80400000
> >> > #define fw_dynamic_info_data(__val) cpu_to_le32(__val)
> >> > #else
> >> > -# define KERNEL_BOOT_ADDRESS 0x80200000
> >> > #define fw_dynamic_info_data(__val) cpu_to_le64(__val)
> >> > #endif
> >> >
> >> > @@ -49,6 +47,15 @@ bool riscv_is_32_bit(MachineState *machine)
> >> > }
> >> > }
> >> >
> >> > +target_ulong riscv_calc_kernel_start_addr(MachineState *machine,
> >> > + target_ulong
> >> > firmware_end_addr) {
> >> > + if (riscv_is_32_bit(machine)) {
> >> > + return QEMU_ALIGN_UP(firmware_end_addr, 4 * MiB);
> >> > + } else {
> >> > + return QEMU_ALIGN_UP(firmware_end_addr, 2 * MiB);
> >> > + }
> >> > +}
> >> > +
> >> > target_ulong riscv_find_and_load_firmware(MachineState *machine,
> >> > const char
> >> > *default_machine_firmware,
> >> > hwaddr
> >> > firmware_load_addr, @@ -123,7 +130,9 @@ target_ulong
> riscv_load_firmware(const char *firmware_filename,
> >> > exit(1);
> >> > }
> >> >
> >> > -target_ulong riscv_load_kernel(const char *kernel_filename,
> >> > symbol_fn_t sym_cb)
> >> > +target_ulong riscv_load_kernel(const char *kernel_filename,
> >> > + target_ulong kernel_start_addr,
> >> > + symbol_fn_t sym_cb)
> >> > {
> >> > uint64_t kernel_entry;
> >> >
> >> > @@ -138,9 +147,9 @@ target_ulong riscv_load_kernel(const char
> *kernel_filename, symbol_fn_t sym_cb)
> >> > return kernel_entry;
> >> > }
> >> >
> >> > - if (load_image_targphys_as(kernel_filename,
> KERNEL_BOOT_ADDRESS,
> >> > + if (load_image_targphys_as(kernel_filename, kernel_start_addr,
> >> > ram_size, NULL) > 0) {
> >> > - return KERNEL_BOOT_ADDRESS;
> >> > + return kernel_start_addr;
> >> > }
> >> >
> >> > error_report("could not load kernel '%s'", kernel_filename);
> >> > diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c index
> >> > 0531bd879b..cc758b78b8 100644
> >> > --- a/hw/riscv/opentitan.c
> >> > +++ b/hw/riscv/opentitan.c
> >> > @@ -75,7 +75,8 @@ static void opentitan_board_init(MachineState
> *machine)
> >> > }
> >> >
> >> > if (machine->kernel_filename) {
> >> > - riscv_load_kernel(machine->kernel_filename, NULL);
> >> > + riscv_load_kernel(machine->kernel_filename,
> >> > + memmap[IBEX_DEV_RAM].base, NULL);
> >> > }
> >> > }
> >> >
> >> > diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c index
> >> > fcfac16816..59bac4cc9a 100644
> >> > --- a/hw/riscv/sifive_e.c
> >> > +++ b/hw/riscv/sifive_e.c
> >> > @@ -114,7 +114,8 @@ static void sifive_e_machine_init(MachineState
> *machine)
> >> > memmap[SIFIVE_E_DEV_MROM].base,
> >> > &address_space_memory);
> >> >
> >> > if (machine->kernel_filename) {
> >> > - riscv_load_kernel(machine->kernel_filename, NULL);
> >> > + riscv_load_kernel(machine->kernel_filename,
> >> > + memmap[SIFIVE_E_DEV_DTIM].base, NULL);
> >> > }
> >> > }
> >> >
> >> > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c index
> >> > 5f3ad9bc0f..b2472c6627 100644
> >> > --- a/hw/riscv/sifive_u.c
> >> > +++ b/hw/riscv/sifive_u.c
> >> > @@ -415,6 +415,7 @@ static void sifive_u_machine_init(MachineState
> *machine)
> >> > MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> >> > MemoryRegion *flash0 = g_new(MemoryRegion, 1);
> >> > target_ulong start_addr = memmap[SIFIVE_U_DEV_DRAM].base;
> >> > + target_ulong firmware_end_addr, kernel_start_addr;
> >> > uint32_t start_addr_hi32 = 0x00000000;
> >> > int i;
> >> > uint32_t fdt_load_addr;
> >> > @@ -474,10 +475,15 @@ static void
> sifive_u_machine_init(MachineState *machine)
> >> > break;
> >> > }
> >> >
> >> > - riscv_find_and_load_firmware(machine, BIOS_FILENAME,
> start_addr, NULL);
> >> > + firmware_end_addr = riscv_find_and_load_firmware(machine,
> BIOS_FILENAME,
> >> > + start_addr,
> >> > + NULL);
> >> >
> >> > if (machine->kernel_filename) {
> >> > - kernel_entry = riscv_load_kernel(machine->kernel_filename,
> NULL);
> >> > + kernel_start_addr = riscv_calc_kernel_start_addr(machine,
> >> > +
> >> > + firmware_end_addr);
> >> > +
> >> > + kernel_entry = riscv_load_kernel(machine->kernel_filename,
> >> > + kernel_start_addr, NULL);
> >> >
> >> > if (machine->initrd_filename) {
> >> > hwaddr start;
> >> > diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c index
> >> > 3fd152a035..facac6e7d2 100644
> >> > --- a/hw/riscv/spike.c
> >> > +++ b/hw/riscv/spike.c
> >> > @@ -195,6 +195,7 @@ static void spike_board_init(MachineState
> *machine)
> >> > MemoryRegion *system_memory = get_system_memory();
> >> > MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> >> > MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> >> > + target_ulong firmware_end_addr, kernel_start_addr;
> >> > uint32_t fdt_load_addr;
> >> > uint64_t kernel_entry;
> >> > char *soc_name;
> >> > @@ -261,12 +262,16 @@ static void spike_board_init(MachineState
> *machine)
> >> > memory_region_add_subregion(system_memory,
> memmap[SPIKE_MROM].base,
> >> > mask_rom);
> >> >
> >> > - riscv_find_and_load_firmware(machine, BIOS_FILENAME,
> >> > - memmap[SPIKE_DRAM].base,
> >> > - htif_symbol_callback);
> >> > + firmware_end_addr = riscv_find_and_load_firmware(machine,
> BIOS_FILENAME,
> >> > +
> >> > memmap[SPIKE_DRAM].base,
> >> > +
> >> > + htif_symbol_callback);
> >> >
> >> > if (machine->kernel_filename) {
> >> > + kernel_start_addr = riscv_calc_kernel_start_addr(machine,
> >> > +
> >> > + firmware_end_addr);
> >> > +
> >> > kernel_entry = riscv_load_kernel(machine->kernel_filename,
> >> > + kernel_start_addr,
> >> > htif_symbol_callback);
> >> >
> >> > if (machine->initrd_filename) { diff --git
> >> > a/hw/riscv/virt.c b/hw/riscv/virt.c index 41bd2f38ba..6bfd10dfc7
> >> > 100644
> >> > --- a/hw/riscv/virt.c
> >> > +++ b/hw/riscv/virt.c
> >> > @@ -493,6 +493,7 @@ static void virt_machine_init(MachineState
> *machine)
> >> > char *plic_hart_config, *soc_name;
> >> > size_t plic_hart_config_len;
> >> > target_ulong start_addr = memmap[VIRT_DRAM].base;
> >> > + target_ulong firmware_end_addr, kernel_start_addr;
> >> > uint32_t fdt_load_addr;
> >> > uint64_t kernel_entry;
> >> > DeviceState *mmio_plic, *virtio_plic, *pcie_plic; @@ -602,11
> >> > +603,15 @@ static void virt_machine_init(MachineState *machine)
> >> > memory_region_add_subregion(system_memory,
> memmap[VIRT_MROM].base,
> >> > mask_rom);
> >> >
> >> > - riscv_find_and_load_firmware(machine, BIOS_FILENAME,
> >> > - memmap[VIRT_DRAM].base, NULL);
> >> > + firmware_end_addr = riscv_find_and_load_firmware(machine,
> BIOS_FILENAME,
> >> > + start_addr,
> >> > + NULL);
> >> >
> >> > if (machine->kernel_filename) {
> >> > - kernel_entry = riscv_load_kernel(machine->kernel_filename,
> NULL);
> >> > + kernel_start_addr = riscv_calc_kernel_start_addr(machine,
> >> > +
> >> > + firmware_end_addr);
> >> > +
> >> > + kernel_entry = riscv_load_kernel(machine->kernel_filename,
> >> > + kernel_start_addr, NULL);
> >> >
> >> > if (machine->initrd_filename) {
> >> > hwaddr start;
> >>
> >> It's probably worth going through and making sure we mark the right
> >> regions as reserved in the DT, as with these being mobile we might run
> into latent bugs.
> >
> > Do you mean mark where the firmware is as reserved?
>
> Ya. We have some things like Linux assuming that the first page in memory is
> reserved for the bootloader, which IIRC never really got written down in any
> specifications. That's an implicit S-mode ABI, so it doesn't apply directly,
> but
> I'd guess there's some of this floating around elsewhere in the stack.
Let's not do any DT reservation based on firmware load location because
firmwares can relocate itself to some other address. Further, firmware such
as OpenSBI can now create domains where only firmware knows how the
system is partitioned into domains and do DT reservations accordingly.
I suggest we let firmware add the required reservations in DT
>
> >> I haven't actually looked so maybe we're fine, but IIRC we sort of
> >> papered over a handful of memory layout agreements that weren't even
> >> in specs (or event meant to be in specs) that have stuck around for quite a
> while.
> >
> > For the virt machine or the sifive_u?
>
> My guess is that any of this would apply to both of these, as the issue would
> be assumptions within the firmware about the memory image provided to it
> when it's entered. That would be an ABI between the firmware and
> whatever loads it, but IIRC for both the virt board and the sifive_u we
> assume the firmware is just loaded directly my QEMU.
>
> The sifive_u board may be additionally constrained by SiFive's boot ROM, but
> I don't remember it really caring about any of this. Also not sure it's even
> used any more, as it was really just a vehicle to demonstrate initializing the
> widgets for which we couldn't release RTL.
>
> >> Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com>
> >
> > Thanks
> >
> > Alistair
Regards,
Anup