Re: [Qemu-riscv] [Qemu-devel] [PATCH v1 2/2] hw/riscv: Load OpenSBI as the default firmware

[Alistair Francis]

On Wed, Jul 10, 2019 at 3:46 PM Guenter Roeck <address@hidden> wrote:
>
> On Mon, Jul 08, 2019 at 11:49:40AM -0700, Alistair Francis wrote:
> > If the user hasn't specified a firmware to load (with -bios) or
> > specified no bios (with -bios none) then load OpenSBI by default. This
> > allows users to boot a RISC-V kernel with just -kernel.
> >
> > Signed-off-by: Alistair Francis <address@hidden>
> > Reviewed-by: Bin Meng <address@hidden>
> > Tested-by: Bin Meng <address@hidden>
> > ---
> >  hw/riscv/boot.c         | 49 +++++++++++++++++++++++++++++++++++++++++
> >  hw/riscv/sifive_u.c     |  7 +++---
> >  hw/riscv/virt.c         | 11 ++++++---
> >  include/hw/riscv/boot.h |  3 +++
> >  qemu-deprecated.texi    | 20 +++++++++++++++++
> >  5 files changed, 84 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> > index ff023f42d0..c7d72f682f 100644
> > --- a/hw/riscv/boot.c
> > +++ b/hw/riscv/boot.c
> > @@ -18,6 +18,7 @@
> >   */
> >
> >  #include "qemu/osdep.h"
> > +#include "qemu-common.h"
> >  #include "qemu/units.h"
> >  #include "qemu/error-report.h"
> >  #include "exec/cpu-defs.h"
> > @@ -32,6 +33,54 @@
> >  # define KERNEL_BOOT_ADDRESS 0x80200000
> >  #endif
> >
> > +void riscv_find_and_load_firmware(MachineState *machine,
> > +                                  const char *default_machine_firmware,
> > +                                  hwaddr firmware_load_addr)
> > +{
> > +    char *firmware_filename;
> > +
> > +    if (!machine->firmware) {
> > +        /*
> > +         * The user didn't specify -bios.
> > +         * At the moment we default to loading nothing when this hapens.
> > +         * In the future this defaul will change to loading the prebuilt
> > +         * OpenSBI firmware. Let's warn the user and then continue.
> > +        */
> > +        warn_report("No -bios option specified. Not loading a firmware.");
> > +        warn_report("This default will change in QEMU 4.3. Please use the 
> > " \
> > +                    "-bios option to aviod breakages when this happens.");
> > +        warn_report("See QEMU's deprecation documentation for details");
> > +        return;
> > +    }
> > +
> > +    if (!strcmp(machine->firmware, "default")) {
> > +        /*
> > +         * The user has specified "-bios default". That means we are going 
> > to
> > +         * load the OpenSBI binary included in the QEMU source.
> > +         *
> > +         * We can't load the binary by default as it will break existing 
> > users
> > +         * as users are already loading their own firmware.
> > +         *
> > +         * Let's try to get everyone to specify the -bios option at all 
> > times,
> > +         * so then in the future we can make "-bios default" the default 
> > option
> > +         * if no -bios option is set without breaking anything.
> > +         */
> > +        firmware_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS,
> > +                                           default_machine_firmware);
>
> This can return NULL if the file is not found (such as after a bad install) 
> ...
>
> > +    } else {
> > +        firmware_filename = machine->firmware;
> > +    }
> > +
> > +    if (strcmp(firmware_filename, "none")) {
>
> ... which will then crash here.

Yep, you are right. I'll send a v2

Alistair

>
> > +        /* If not "none" load the firmware */
> > +        riscv_load_firmware(firmware_filename, firmware_load_addr);
> > +    }
> > +
> > +    if (!strcmp(machine->firmware, "default")) {
> > +        g_free(firmware_filename);
> > +    }
> > +}
> > +
> >  target_ulong riscv_load_firmware(const char *firmware_filename,
> >                                   hwaddr firmware_load_addr)
> >  {
> > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> > index ca53a9290d..71b8083c05 100644
> > --- a/hw/riscv/sifive_u.c
> > +++ b/hw/riscv/sifive_u.c
> > @@ -49,6 +49,8 @@
> >
> >  #include <libfdt.h>
> >
> > +#define BIOS_FILENAME "opensbi-riscv64-sifive_u-fw_jump.bin"
> > +
> >  static const struct MemmapEntry {
> >      hwaddr base;
> >      hwaddr size;
> > @@ -269,9 +271,8 @@ static void riscv_sifive_u_init(MachineState *machine)
> >      /* create device tree */
> >      create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline);
> >
> > -    if (machine->firmware) {
> > -        riscv_load_firmware(machine->firmware, memmap[SIFIVE_U_DRAM].base);
> > -    }
> > +    riscv_find_and_load_firmware(machine, BIOS_FILENAME,
> > +                                 memmap[SIFIVE_U_DRAM].base);
> >
> >      if (machine->kernel_filename) {
> >          riscv_load_kernel(machine->kernel_filename);
> > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> > index ecdc77d728..25faf3b417 100644
> > --- a/hw/riscv/virt.c
> > +++ b/hw/riscv/virt.c
> > @@ -44,6 +44,12 @@
> >
> >  #include <libfdt.h>
> >
> > +#if defined(TARGET_RISCV32)
> > +# define BIOS_FILENAME "opensbi-riscv32-virt-fw_jump.bin"
> > +#else
> > +# define BIOS_FILENAME "opensbi-riscv64-virt-fw_jump.bin"
> > +#endif
> > +
> >  static const struct MemmapEntry {
> >      hwaddr base;
> >      hwaddr size;
> > @@ -399,9 +405,8 @@ static void riscv_virt_board_init(MachineState *machine)
> >      memory_region_add_subregion(system_memory, memmap[VIRT_MROM].base,
> >                                  mask_rom);
> >
> > -    if (machine->firmware) {
> > -        riscv_load_firmware(machine->firmware, memmap[VIRT_DRAM].base);
> > -    }
> > +    riscv_find_and_load_firmware(machine, BIOS_FILENAME,
> > +                                 memmap[VIRT_DRAM].base);
> >
> >      if (machine->kernel_filename) {
> >          uint64_t kernel_entry = 
> > riscv_load_kernel(machine->kernel_filename);
> > diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
> > index daa179b600..d56f2ae3eb 100644
> > --- a/include/hw/riscv/boot.h
> > +++ b/include/hw/riscv/boot.h
> > @@ -20,6 +20,9 @@
> >  #ifndef RISCV_BOOT_H
> >  #define RISCV_BOOT_H
> >
> > +void riscv_find_and_load_firmware(MachineState *machine,
> > +                                  const char *default_machine_firmware,
> > +                                  hwaddr firmware_load_addr);
> >  target_ulong riscv_load_firmware(const char *firmware_filename,
> >                                   hwaddr firmware_load_addr);
> >  target_ulong riscv_load_kernel(const char *kernel_filename);
> > diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> > index c90b08d553..fff07bb2a3 100644
> > --- a/qemu-deprecated.texi
> > +++ b/qemu-deprecated.texi
> > @@ -121,6 +121,26 @@ the backing storage specified with @option{-mem-path} 
> > can actually provide
> >  the guest RAM configured with @option{-m} and QEMU will fail to start up if
> >  RAM allocation is unsuccessful.
> >
> > +@subsection RISC-V -bios (since 4.1)
> > +
> > +QEMU 4.1 introduced support for the -bios option in QEMU for RISC-V for the
> > +RISC-V virt machine and sifive_u machine.
> > +
> > +QEMU 4.1 has no changes to the default behaviour to avoid breakages. This
> > +default will change in a future QEMU release, so please prepare now. All 
> > users
> > +of the virt or sifive_u machine must change their command line usage.
> > +
> > +QEMU 4.1 has three options, please migrate to one of these three:
> > + 1. ``-bios none`` - This is the current default behavior if no -bios 
> > option
> > +      is included. QEMU will not automatically load any firmware. It is up
> > +      to the user to load all the images they need.
> > + 2. ``-bios default`` - In a future QEMU release this will become the 
> > default
> > +      behaviour if no -bios option is specified. This option will load the
> > +      default OpenSBI firmware automatically. The firmware is included with
> > +      the QEMU release and no user interaction is required. All a user 
> > needs
> > +      to do is specify the kernel they want to boot with the -kernel option
> > + 3. ``-bios <file>`` - Tells QEMU to load the specified file as the 
> > firmwrae.
> > +
> >  @section QEMU Machine Protocol (QMP) commands
> >
> >  @subsection block-dirty-bitmap-add "autoload" parameter (since 2.12.0)