[Qemu-devel] [PATCH for-4.1 0/2] target/riscv: Improve virt machine kernel handling

[Qemu-devel] [PATCH for-4.1 0/2] target/riscv: Improve virt machine kernel handling

[Jonathan Behrens]

Jonathan Behrens (2):
  target/riscv: virt machine shouldn't assume ELF entry is DRAM base
  target/riscv: Add support for -bios "firmware_filename" flag

 hw/riscv/virt.c | 93 ++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 69 insertions(+), 24 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH for-4.1 1/2] target/riscv: virt machine shouldn't assume ELF entry is DRAM base

[Jonathan Behrens]

There are two components of this:

* The reset vector now uses the ELF entry point instead of the DRAM base
  address.
* Initrd now uses the DRAM base address to figure out the half way point of
  RAM. This is more in line with the comment in load_initrd, but an alternative
  strategy would be to somehow use the kernel_high address in that computation
  instead.

Signed-off-by: Jonathan Behrens <jonathan@fintelia.io>
---
 hw/riscv/virt.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index fc4c6b306e..87cc08016b 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -64,10 +64,10 @@ static const struct MemmapEntry {
 
 static target_ulong load_kernel(const char *kernel_filename)
 {
-    uint64_t kernel_entry, kernel_high;
+    uint64_t kernel_entry;
 
     if (load_elf(kernel_filename, NULL, NULL, NULL,
-                 &kernel_entry, NULL, &kernel_high,
+                 &kernel_entry, NULL, NULL,
                  0, EM_RISCV, 1, 0) < 0) {
         error_report("could not load kernel '%s'", kernel_filename);
         exit(1);
@@ -75,8 +75,8 @@ static target_ulong load_kernel(const char *kernel_filename)
     return kernel_entry;
 }
 
-static hwaddr load_initrd(const char *filename, uint64_t mem_size,
-                          uint64_t kernel_entry, hwaddr *start)
+static uint64_t load_initrd(const char *filename, uint64_t mem_base,
+                            uint64_t mem_size, uint64_t *start)
 {
     int size;
 
@@ -85,12 +85,12 @@ static hwaddr load_initrd(const char *filename, uint64_t mem_size,
      * 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.
+     * 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);
+    *start = mem_base + MIN(mem_size / 2, 128 * MiB);
 
     size = load_ramdisk(filename, *start, mem_size - *start);
     if (size == -1) {
@@ -422,14 +422,15 @@ static void riscv_virt_board_init(MachineState *machine)
     memory_region_add_subregion(system_memory, memmap[VIRT_MROM].base,
                                 mask_rom);
 
+    uint64_t entry = memmap[VIRT_DRAM].base;
     if (machine->kernel_filename) {
-        uint64_t kernel_entry = load_kernel(machine->kernel_filename);
+        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);
+            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",
@@ -449,8 +450,8 @@ static void riscv_virt_board_init(MachineState *machine)
 #endif
         0x00028067,                  /*     jr     t0 */
         0x00000000,
-        memmap[VIRT_DRAM].base,      /* start: .dword memmap[VIRT_DRAM].base */
-        0x00000000,
+        entry & 0xffffffff,          /* start: .qword entry */
+        entry >> 32,
                                      /* dtb: */
     };
 
-- 
2.20.1

[Qemu-devel] [PATCH for-4.1 2/2] target/riscv: Add support for -bios "firmware_filename" flag

[Jonathan Behrens]

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(-)

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 */
-- 
2.20.1

Re: [Qemu-devel] [PATCH for-4.1 2/2] target/riscv: Add support for -bios "firmware_filename" flag

[Alistair Francis]

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
>

Re: [Qemu-devel] [PATCH for-4.1 2/2] target/riscv: Add support for -bios "firmware_filename" flag

[Jonathan Behrens]

> 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

Re: [Qemu-devel] [PATCH for-4.1 2/2] target/riscv: Add support for -bios "firmware_filename" flag

[Alistair Francis]

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

Re: [Qemu-devel] [PATCH for-4.1 2/2] target/riscv: Add support for -bios "firmware_filename" flag

[Jonathan Behrens]

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
>

Re: [Qemu-devel] [PATCH for-4.1 2/2] target/riscv: Add support for -bios "firmware_filename" flag

[Alistair Francis]

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