Re: [Qemu-riscv] [PATCH v2 4/4] RISC-V: Support separate firmware and kernel payload

[Alistair Francis]

On Sun, 2018-12-16 at 18:12 -0500, Stefan O'Rear wrote:
> Support for separate firmware and kernel payload is added
> by updating BBL to read optional preloaded kernel address
> attributes from device-tree using a similar mechanism to
> that used to pass init ramdisk addresses to linux kernel.
> 
>     chosen {
>         riscv,kernel-start = <0x00000000 0x80200000>;
>         riscv,kernel-end = <0x00000000 0x80590634>;
>     };
> 
> These attributes are added by QEMU and read by BBL when combining
> -bios <firmware-image> and -kernel <kernel-image> options. e.g.
> 
> $ qemu-system-riscv64 -machine virt -bios bbl -kernel vmlinux
> 
> With this change, bbl can be compiled without --with-payload
> and the dummy payload alignment is altered to make the memory
> footprint of the firmware-only bbl smaller. The dummy payload
> message is updated to indicate the alternative load method.
> 
> This load method could also be supported by a first stage boot
> loader that reads seperate firmware and kernel from SPI flash.
> The main advantage of this new mechanism is that it eases kernel
> development by avoiding the riscv-pk packaging step after kernel
> builds, makes building per repository artefacts for CI simpler,
> and mimics bootloaders on other platforms that can load a kernel
> image file directly. Ultimately BBL should use an SPI driver to
> load the kernel image however this mechanism supports use cases
> such such as QEMU's -bios, -kernel and -initrd options following
> examples from other platforms that pass kernel entry to firmware
> via device-tree.
> 
> Signed-off-by: Michael Clark <address@hidden>
> Signed-off-by: Stefan O'Rear <address@hidden>
> ---
>  hw/riscv/boot.c | 78
> ++++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 72 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index e51e7f9..43f328d 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -32,16 +32,70 @@
>          fprintf(stderr, "boot: %s: "fs, __func__, ##__VA_ARGS__); \
>      }
>  
> -static uint64_t load_kernel(const char *kernel_filename)
> +static uint64_t kernel_offset;
> +
> +static uint64_t kernel_translate(void *opaque, uint64_t addr)
>  {
> -    uint64_t kernel_entry, kernel_high;
> +    /* mask kernel virtual address and offset by load address */
> +    if (kernel_offset) {
> +        return (addr & 0x7fffffff) + kernel_offset;
> +    } else {
> +        return addr;
> +    }
> +}
>  
> -    if (load_elf(kernel_filename, NULL, NULL,
> -                 &kernel_entry, NULL, &kernel_high,
> +static hwaddr riscv_load_firmware(const char *filename)
> +{
> +    uint64_t firmware_entry, firmware_start, firmware_end;
> +
> +    if (load_elf(filename, NULL, NULL,
> +                 &firmware_entry, &firmware_start, &firmware_end,
>                   0, EM_RISCV, 1, 0) < 0) {
> -        error_report("could not load kernel '%s'", kernel_filename);
> +        error_report("could not load firmware '%s'", filename);
>          exit(1);
>      }
> +
> +    /* align kernel load address to the megapage after the firmware
> */
> +#if defined(TARGET_RISCV32)
> +    kernel_offset = (firmware_end + 0x3fffff) & ~0x3fffff;
> +#else
> +    kernel_offset = (firmware_end + 0x1fffff) & ~0x1fffff;
> +#endif
> +
> +    boot_debug("entry=0x" TARGET_FMT_plx " start=0x" TARGET_FMT_plx
> " "
> +               "end=0x" TARGET_FMT_plx " kernel_offset=0x"
> TARGET_FMT_plx "\n",
> +               firmware_entry, firmware_start, firmware_end,
> kernel_offset);
> +
> +    return firmware_entry;
> +}
> +
> +static hwaddr riscv_load_kernel(const char *filename, void *fdt)
> +{
> +    uint64_t kernel_entry, kernel_start, kernel_end;
> +
> +    if (load_elf(filename, kernel_translate, NULL,
> +                 &kernel_entry, &kernel_start, &kernel_end,
> +                 0, EM_RISCV, 1, 0) < 0) {
> +        error_report("could not load kernel '%s'", filename);
> +        exit(1);
> +    }
> +
> +    boot_debug("entry=0x" TARGET_FMT_plx " start=0x" TARGET_FMT_plx
> " "
> +               "end=0x" TARGET_FMT_plx "\n", kernel_entry,
> kernel_start,
> +               kernel_end);
> +
> +    /*
> +     * pass kernel load address via device-tree to firmware
> +     *
> +     * BBL reads the kernel address from device-tree
> +     */

I don't think we should have bbl specific code in QEMU. bbl is not a
long term solution so it seems like a bad idea to handle it forever in
QEMU.

Do you mind fixing the comments I made to patch 1 and 2 and then
sending the series to the QEMU mailing list?

If you would still like this patch then you can keep it in and we can
have the discussion on the list about it's merrits.

Thanks for the series, the this is very helpful to QEMU and RISC-V
support!

Alistair

> +    if (fdt) {
> +        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);
> +    }
> +
>      return kernel_entry;
>  }
>  
> @@ -91,8 +145,20 @@ hwaddr
> riscv_load_firmware_kernel_initrd(MachineState *machine, void *fdt)
>  {
>      hwaddr firmware_entry = 0;
>  
> +    /* load firmware e.g. -bios bbl */
> +    if (machine->firmware) {
> +        firmware_entry = riscv_load_firmware(machine->firmware);
> +    }
> +
> +    /* load combined bbl+kernel or separate kernel */
>      if (machine->kernel_filename) {
> -        firmware_entry = load_kernel(machine->kernel_filename);
> +        if (machine->firmware) {
> +            /* load separate bios and kernel e.g. -bios bbl -kernel
> vmlinux */
> +            riscv_load_kernel(machine->kernel_filename, fdt);
> +        } else {
> +            /* load traditional combined bbl+kernel e.g. -kernel
> bbl_vmlimux */
> +            firmware_entry = riscv_load_kernel(machine-
> >kernel_filename, NULL);
> +        }
>          if (machine->initrd_filename) {
>              /* load separate initrd */
>              riscv_load_initrd(machine->initrd_filename, machine-
> >ram_size,