Re: [Qemu-riscv] [PATCH v2 1/4] RISC-V: Move firmware loading logic to a separate file

[Alistair Francis]

On Sun, 2018-12-16 at 18:12 -0500, Stefan O'Rear wrote:
> Signed-off-by: Stefan O'Rear <address@hidden>
> ---
>  hw/riscv/Makefile.objs  |  1 +
>  hw/riscv/boot.c         | 85
> +++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/riscv/virt.c         | 58 ++-------------------------------
>  include/hw/riscv/boot.h | 24 ++++++++++++++
>  4 files changed, 112 insertions(+), 56 deletions(-)
>  create mode 100644 hw/riscv/boot.c
>  create mode 100644 include/hw/riscv/boot.h
> 
> diff --git a/hw/riscv/Makefile.objs b/hw/riscv/Makefile.objs
> index 1dde01d..d36b004 100644
> --- a/hw/riscv/Makefile.objs
> +++ b/hw/riscv/Makefile.objs
> @@ -1,3 +1,4 @@
> +obj-y += boot.o
>  obj-y += riscv_htif.o
>  obj-y += riscv_hart.o
>  obj-y += sifive_e.o
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> new file mode 100644
> index 0000000..1e4d017
> --- /dev/null
> +++ b/hw/riscv/boot.c
> @@ -0,0 +1,85 @@
> +/*
> + * QEMU RISCV firmware and kernel loader
> + *
> + * Copyright (c) 2017-2018 SiFive, Inc.
> + *
> + * This program is free software; you can redistribute it and/or
> modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2 or later, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but
> WITHOUT
> + * ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public
> License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License
> along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>;.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/units.h"
> +#include "qemu/error-report.h"
> +#include "hw/loader.h"
> +#include "hw/boards.h"
> +#include "sysemu/device_tree.h"
> +#include "elf.h"
> +#include "hw/riscv/boot.h"
> +
> +static uint64_t load_kernel(const char *kernel_filename)
> +{

This is going to conflict with this patch:

https://patchwork.kernel.org/patch/10701667/

It'll just be a simple change though.

Alistair

> +    uint64_t kernel_entry, kernel_high;
> +
> +    if (load_elf(kernel_filename, NULL, NULL,
> +                 &kernel_entry, NULL, &kernel_high,
> +                 0, EM_RISCV, 1, 0) < 0) {
> +        error_report("could not load kernel '%s'", kernel_filename);
> +        exit(1);
> +    }
> +    return kernel_entry;
> +}
> +
> +static hwaddr load_initrd(const char *filename, uint64_t mem_size,
> +                          uint64_t kernel_entry, hwaddr *start)
> +{
> +    int size;
> +
> +    /* We want to put the initrd far enough into RAM that when the
> +     * kernel is uncompressed it will not clobber the initrd.
> However
> +     * on boards without much RAM we must ensure that we still leave
> +     * enough room for a decent sized initrd, and on boards with
> large
> +     * amounts of RAM we must avoid the initrd being so far up in
> RAM
> +     * that it is outside lowmem and inaccessible to the kernel.
> +     * 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.
> +     */
> +    *start = kernel_entry + MIN(mem_size / 2, 128 * MiB);
> +
> +    size = load_ramdisk(filename, *start, mem_size - *start);
> +    if (size == -1) {
> +        size = load_image_targphys(filename, *start, mem_size -
> *start);
> +        if (size == -1) {
> +            error_report("could not load ramdisk '%s'", filename);
> +            exit(1);
> +        }
> +    }
> +    return *start + size;
> +}
> +
> +void riscv_load_firmware_kernel_initrd(MachineState *machine, void
> *fdt)
> +{
> +    if (machine->kernel_filename) {
> +        uint64_t kernel_entry = load_kernel(machine-
> >kernel_filename);
> +
> +        if (machine->initrd_filename) {
> +            hwaddr start;
> +            hwaddr end = load_initrd(machine->initrd_filename,
> +                                     machine->ram_size,
> kernel_entry,
> +                                     &start);
> +            qemu_fdt_setprop_cell(fdt, "/chosen",
> +                                  "linux,initrd-start", start);
> +            qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-
> end",
> +                                  end);
> +        }
> +    }
> +}
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 2b38f89..4c2e894 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -34,6 +34,7 @@
>  #include "hw/riscv/sifive_plic.h"
>  #include "hw/riscv/sifive_clint.h"
>  #include "hw/riscv/sifive_test.h"
> +#include "hw/riscv/boot.h"
>  #include "hw/riscv/virt.h"
>  #include "chardev/char.h"
>  #include "sysemu/arch_init.h"
> @@ -57,47 +58,6 @@ static const struct MemmapEntry {
>      [VIRT_DRAM] =     { 0x80000000,        0x0 },
>  };
>  
> -static uint64_t load_kernel(const char *kernel_filename)
> -{
> -    uint64_t kernel_entry, kernel_high;
> -
> -    if (load_elf(kernel_filename, NULL, NULL,
> -                 &kernel_entry, NULL, &kernel_high,
> -                 0, EM_RISCV, 1, 0) < 0) {
> -        error_report("could not load kernel '%s'", kernel_filename);
> -        exit(1);
> -    }
> -    return kernel_entry;
> -}
> -
> -static hwaddr load_initrd(const char *filename, uint64_t mem_size,
> -                          uint64_t kernel_entry, hwaddr *start)
> -{
> -    int size;
> -
> -    /* We want to put the initrd far enough into RAM that when the
> -     * kernel is uncompressed it will not clobber the initrd.
> However
> -     * on boards without much RAM we must ensure that we still leave
> -     * enough room for a decent sized initrd, and on boards with
> large
> -     * amounts of RAM we must avoid the initrd being so far up in
> RAM
> -     * that it is outside lowmem and inaccessible to the kernel.
> -     * 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.
> -     */
> -    *start = kernel_entry + MIN(mem_size / 2, 128 * MiB);
> -
> -    size = load_ramdisk(filename, *start, mem_size - *start);
> -    if (size == -1) {
> -        size = load_image_targphys(filename, *start, mem_size -
> *start);
> -        if (size == -1) {
> -            error_report("could not load ramdisk '%s'", filename);
> -            exit(1);
> -        }
> -    }
> -    return *start + size;
> -}
> -
>  static void *create_fdt(RISCVVirtState *s, const struct MemmapEntry
> *memmap,
>      uint64_t mem_size, const char *cmdline)
>  {
> @@ -301,21 +261,7 @@ static void riscv_virt_board_init(MachineState
> *machine)
>      memory_region_add_subregion(system_memory,
> memmap[VIRT_MROM].base,
>                                  mask_rom);
>  
> -    if (machine->kernel_filename) {
> -        uint64_t kernel_entry = load_kernel(machine-
> >kernel_filename);
> -
> -        if (machine->initrd_filename) {
> -            hwaddr start;
> -            hwaddr end = load_initrd(machine->initrd_filename,
> -                                     machine->ram_size,
> kernel_entry,
> -                                     &start);
> -            qemu_fdt_setprop_cell(fdt, "/chosen",
> -                                  "linux,initrd-start", start);
> -            qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-
> end",
> -                                  end);
> -        }
> -    }
> -
> +    riscv_load_firmware_kernel_initrd(machine, fdt);
>      /* reset vector */
>      uint32_t reset_vec[8] = {
>          0x00000297,                  /* 1:  auipc  t0,
> %pcrel_hi(dtb) */
> diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
> new file mode 100644
> index 0000000..92948f3
> --- /dev/null
> +++ b/include/hw/riscv/boot.h
> @@ -0,0 +1,24 @@
> +/*
> + * QEMU RISCV firmware and kernel loader interface
> + *
> + * Copyright (c) 2017-2018 SiFive, Inc.
> + *
> + * This program is free software; you can redistribute it and/or
> modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2 or later, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but
> WITHOUT
> + * ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public
> License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License
> along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>;.
> + */
> +
> +#ifndef HW_RISCV_BOOT_H
> +#define HW_RISCV_BOOT_H
> +
> +void riscv_load_firmware_kernel_initrd(MachineState *machine, void
> *fdt);
> +
> +#endif