| Message ID | 20190517222342.26394-3-jonathan@fintelia.io (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/riscv: Improve virt machine kernel handling | expand |
On Fri, May 17, 2019 at 3:25 PM Jonathan Behrens <jonathan@fintelia.io> wrote: > > QEMU does not have any default firmware for RISC-V. However, it isn't possible > to run a normal kernel binary without firmware. Thus it has previously been > necessary to compile the two together into a single binary to pass with the > -kernel flag. This patch allows passing separate firmware and kernel binaries by > passing both the -bios and -kernel flags. I've never been fully convinced of this, why not just use the generic loader? This does match other architectures though so it's fine to go in. I think you will also get better in_asm output with this as well, which is something the loader doesn't give you. > > This is based on a previously proposed patch by Michael Clark: > https://patchwork.kernel.org/patch/10419975/ > > Signed-off-by: Jonathan Behrens <jonathan@fintelia.io> > --- > hw/riscv/virt.c | 66 ++++++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 55 insertions(+), 11 deletions(-) > > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c > index 87cc08016b..d7b1792b58 100644 > --- a/hw/riscv/virt.c > +++ b/hw/riscv/virt.c > @@ -62,6 +62,40 @@ static const struct MemmapEntry { > [VIRT_PCIE_ECAM] = { 0x30000000, 0x10000000 }, > }; > > + > +static target_ulong load_firmware_and_kernel(const char *firmware_filename, > + const char *kernel_filename, > + uint64_t mem_size, > + uint64_t* kernel_start, > + uint64_t* kernel_end) > +{ > + uint64_t firmware_entry, firmware_end; > + int size; > + > + if (load_elf(firmware_filename, NULL, NULL, NULL, > + &firmware_entry, NULL, &firmware_end, > + 0, EM_RISCV, 1, 0) < 0) { > + error_report("could not load firmware '%s'", firmware_filename); > + exit(1); > + } > + > + /* align kernel load address to the megapage after the firmware */ > +#if defined(TARGET_RISCV32) > + *kernel_start = (firmware_end + 0x3fffff) & ~0x3fffff; > +#else > + *kernel_start = (firmware_end + 0x1fffff) & ~0x1fffff; > +#endif > + > + size = load_image_targphys(kernel_filename, *kernel_start, > + mem_size - *kernel_start); > + if (size == -1) { > + error_report("could not load kernel '%s'", kernel_filename); > + exit(1); > + } > + *kernel_end = *kernel_start + size; > + return firmware_entry; > +} This should be in a generic boot.c file and support added to all RISC-V boards. Alistair > + > static target_ulong load_kernel(const char *kernel_filename) > { > uint64_t kernel_entry; > @@ -423,19 +457,29 @@ static void riscv_virt_board_init(MachineState *machine) > mask_rom); > > uint64_t entry = memmap[VIRT_DRAM].base; > - if (machine->kernel_filename) { > + if (machine->firmware && machine->kernel_filename) { > + uint64_t kernel_start, kernel_end; > + entry = load_firmware_and_kernel(machine->firmware, > + machine->kernel_filename, > + machine->ram_size, &kernel_start, > + &kernel_end); > + > + qemu_fdt_setprop_cells(fdt, "/chosen", "riscv,kernel-end", > + kernel_end >> 32, kernel_end); > + qemu_fdt_setprop_cells(fdt, "/chosen", "riscv,kernel-start", > + kernel_start >> 32, kernel_start); > + } else if (machine->kernel_filename) { > entry = load_kernel(machine->kernel_filename); > + } > > - if (machine->initrd_filename) { > - uint64_t start; > - uint64_t end = load_initrd(machine->initrd_filename, > - memmap[VIRT_DRAM].base, machine->ram_size, > - &start); > - qemu_fdt_setprop_cell(fdt, "/chosen", > - "linux,initrd-start", start); > - qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-end", > - end); > - } > + if (machine->kernel_filename && machine->initrd_filename) { > + uint64_t start; > + uint64_t end = load_initrd(machine->initrd_filename, > + memmap[VIRT_DRAM].base, machine->ram_size, > + &start); > + > + qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-start", start); > + qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-end", end); > } > > /* reset vector */ > -- > 2.20.1 >
> I've never been fully convinced of this, why not just use the generic loader? If I understand you are proposing passing bbl (or other firmware) with the -kernel flag, and then vmlinux (or another kernel) with the -initrd flag? Wouldn't this result in losing the ability to pass a real init ramdisk to Linux? It also seems to open the possibility for strange bugs/compatibility issues later if firmware starts recognizing any "initrd" entries in the device tree as kernel code to jump into. I do wonder though how compatible the current design is with providing default firmware for riscv in the future. > This should be in a generic boot.c file and support added to all RISC-V boards. I can do this for v2. Jonathan
On Sat, May 18, 2019 at 2:57 PM Jonathan Behrens <jonathan@fintelia.io> wrote: > > > I've never been fully convinced of this, why not just use the generic loader? > > If I understand you are proposing passing bbl (or other firmware) with the -kernel flag, and then vmlinux (or another kernel) with the -initrd flag? Wouldn't this result in losing the ability to pass a real init ramdisk to Linux? It also seems to open the possibility for strange bugs/compatibility issues later if firmware starts recognizing any "initrd" entries in the device tree as kernel code to jump into. No I mean passing in OpenSBI (or some other boot loader) via the -kernel option and then passing in the kernel with QEMU's generic device loader. This is documented as part of the OpenSBI boot flow: https://github.com/riscv/opensbi/blob/master/docs/platform/qemu_virt.md The only disadvantage with that is that we don't get debug symbols from the kernel, but it does mean that the boot loader in QEMU is much simpler. > > I do wonder though how compatible the current design is with providing default firmware for riscv in the future. > > > This should be in a generic boot.c file and support added to all RISC-V boards. > > I can do this for v2. Thanks Alistair > > Jonathan
I've thought some more about this issue, and long term I think there are a
couple different useful configurations:
- For end users, having default firmware that loaded the OS from a block
device would be easiest
- Current invocation: impossible
- Proposed: empty command line (i.e. pass neither -bios nor -kernel)
- Custom firmware support would be good to test possible firmware
improvements or if the default is missing something
- Current invocation: -kernel firmware.elf
- Proposed: -bios firmware.elf
- A kernel developer may want to test a kernel binary without having
to make a full disk image or bundle firmware (on x86 and perhaps other
architectures this is done with the -kernel parameter, but for RISC-V that
invocation currently is used to load M-mode code rather than supervisor
code)
- Current invocation: impossible
- Proposed: -bios firmware.elf -kernel kernel.bin
- Ideally `-kernel kernel.bin` be the same except using default
firmware, but I don't know if QEMU would be willing to deprecate the
current syntax to allow it
For now, it is probably too early to add default firmware (but perhaps
not?) which would leave only the firmware only and firmware + kernel
variants. What do other people think about this?
Jonathan
On Mon, May 20, 2019 at 12:56 PM Alistair Francis <alistair23@gmail.com>
wrote:
> On Sat, May 18, 2019 at 2:57 PM Jonathan Behrens <jonathan@fintelia.io>
> wrote:
> >
> > > I've never been fully convinced of this, why not just use the generic
> loader?
> >
> > If I understand you are proposing passing bbl (or other firmware) with
> the -kernel flag, and then vmlinux (or another kernel) with the -initrd
> flag? Wouldn't this result in losing the ability to pass a real init
> ramdisk to Linux? It also seems to open the possibility for strange
> bugs/compatibility issues later if firmware starts recognizing any "initrd"
> entries in the device tree as kernel code to jump into.
>
> No I mean passing in OpenSBI (or some other boot loader) via the
> -kernel option and then passing in the kernel with QEMU's generic
> device loader. This is documented as part of the OpenSBI boot flow:
> https://github.com/riscv/opensbi/blob/master/docs/platform/qemu_virt.md
>
> The only disadvantage with that is that we don't get debug symbols
> from the kernel, but it does mean that the boot loader in QEMU is much
> simpler.
>
> >
> > I do wonder though how compatible the current design is with providing
> default firmware for riscv in the future.
> >
> > > This should be in a generic boot.c file and support added to all
> RISC-V boards.
> >
> > I can do this for v2.
>
> Thanks
>
> Alistair
>
> >
> > Jonathan
>
On Fri, May 31, 2019 at 3:38 PM Jonathan Behrens <jonathan@fintelia.io> wrote: > > I've thought some more about this issue, and long term I think there are a couple different useful configurations: > > For end users, having default firmware that loaded the OS from a block device would be easiest > > Current invocation: impossible > Proposed: empty command line (i.e. pass neither -bios nor -kernel) > > Custom firmware support would be good to test possible firmware improvements or if the default is missing something > > Current invocation: -kernel firmware.elf > Proposed: -bios firmware.elf > > A kernel developer may want to test a kernel binary without having to make a full disk image or bundle firmware (on x86 and perhaps other architectures this is done with the -kernel parameter, but for RISC-V that invocation currently is used to load M-mode code rather than supervisor code) > > Current invocation: impossible > Proposed: -bios firmware.elf -kernel kernel.bin > Ideally `-kernel kernel.bin` be the same except using default firmware, but I don't know if QEMU would be willing to deprecate the current syntax to allow it > > For now, it is probably too early to add default firmware (but perhaps not?) which would leave only the firmware only and firmware + kernel variants. What do other people think about this? I generally agree with what you are saying. I know that x86 includes seabios (although I'm not sure how) and maybe that is something we can look at. There is now a default RISC-V firmware which would be useful to include so that users can just boot their kernel. We would have to make sure it's possible to overwrite this default one so that people can test their own. The hard part becomes building the firmware as we don't expect people to have a RISC-V toolchain installed. I think the best bet here is to look at what x86 does for their BIOS and we can start to move towards that. Alistair > > Jonathan > > On Mon, May 20, 2019 at 12:56 PM Alistair Francis <alistair23@gmail.com> wrote: >> >> On Sat, May 18, 2019 at 2:57 PM Jonathan Behrens <jonathan@fintelia.io> wrote: >> > >> > > I've never been fully convinced of this, why not just use the generic loader? >> > >> > If I understand you are proposing passing bbl (or other firmware) with the -kernel flag, and then vmlinux (or another kernel) with the -initrd flag? Wouldn't this result in losing the ability to pass a real init ramdisk to Linux? It also seems to open the possibility for strange bugs/compatibility issues later if firmware starts recognizing any "initrd" entries in the device tree as kernel code to jump into. >> >> No I mean passing in OpenSBI (or some other boot loader) via the >> -kernel option and then passing in the kernel with QEMU's generic >> device loader. This is documented as part of the OpenSBI boot flow: >> https://github.com/riscv/opensbi/blob/master/docs/platform/qemu_virt.md >> >> The only disadvantage with that is that we don't get debug symbols >> from the kernel, but it does mean that the boot loader in QEMU is much >> simpler. >> >> > >> > I do wonder though how compatible the current design is with providing default firmware for riscv in the future. >> > >> > > This should be in a generic boot.c file and support added to all RISC-V boards. >> > >> > I can do this for v2. >> >> Thanks >> >> Alistair >> >> > >> > Jonathan
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c index 87cc08016b..d7b1792b58 100644 --- a/hw/riscv/virt.c +++ b/hw/riscv/virt.c @@ -62,6 +62,40 @@ static const struct MemmapEntry { [VIRT_PCIE_ECAM] = { 0x30000000, 0x10000000 }, }; + +static target_ulong load_firmware_and_kernel(const char *firmware_filename, + const char *kernel_filename, + uint64_t mem_size, + uint64_t* kernel_start, + uint64_t* kernel_end) +{ + uint64_t firmware_entry, firmware_end; + int size; + + if (load_elf(firmware_filename, NULL, NULL, NULL, + &firmware_entry, NULL, &firmware_end, + 0, EM_RISCV, 1, 0) < 0) { + error_report("could not load firmware '%s'", firmware_filename); + exit(1); + } + + /* align kernel load address to the megapage after the firmware */ +#if defined(TARGET_RISCV32) + *kernel_start = (firmware_end + 0x3fffff) & ~0x3fffff; +#else + *kernel_start = (firmware_end + 0x1fffff) & ~0x1fffff; +#endif + + size = load_image_targphys(kernel_filename, *kernel_start, + mem_size - *kernel_start); + if (size == -1) { + error_report("could not load kernel '%s'", kernel_filename); + exit(1); + } + *kernel_end = *kernel_start + size; + return firmware_entry; +} + static target_ulong load_kernel(const char *kernel_filename) { uint64_t kernel_entry; @@ -423,19 +457,29 @@ static void riscv_virt_board_init(MachineState *machine) mask_rom); uint64_t entry = memmap[VIRT_DRAM].base; - if (machine->kernel_filename) { + if (machine->firmware && machine->kernel_filename) { + uint64_t kernel_start, kernel_end; + entry = load_firmware_and_kernel(machine->firmware, + machine->kernel_filename, + machine->ram_size, &kernel_start, + &kernel_end); + + qemu_fdt_setprop_cells(fdt, "/chosen", "riscv,kernel-end", + kernel_end >> 32, kernel_end); + qemu_fdt_setprop_cells(fdt, "/chosen", "riscv,kernel-start", + kernel_start >> 32, kernel_start); + } else if (machine->kernel_filename) { entry = load_kernel(machine->kernel_filename); + } - if (machine->initrd_filename) { - uint64_t start; - uint64_t end = load_initrd(machine->initrd_filename, - memmap[VIRT_DRAM].base, machine->ram_size, - &start); - qemu_fdt_setprop_cell(fdt, "/chosen", - "linux,initrd-start", start); - qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-end", - end); - } + if (machine->kernel_filename && machine->initrd_filename) { + uint64_t start; + uint64_t end = load_initrd(machine->initrd_filename, + memmap[VIRT_DRAM].base, machine->ram_size, + &start); + + qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-start", start); + qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-end", end); } /* reset vector */
QEMU does not have any default firmware for RISC-V. However, it isn't possible to run a normal kernel binary without firmware. Thus it has previously been necessary to compile the two together into a single binary to pass with the -kernel flag. This patch allows passing separate firmware and kernel binaries by passing both the -bios and -kernel flags. This is based on a previously proposed patch by Michael Clark: https://patchwork.kernel.org/patch/10419975/ Signed-off-by: Jonathan Behrens <jonathan@fintelia.io> --- hw/riscv/virt.c | 66 ++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 55 insertions(+), 11 deletions(-)