[PATCH v5 00/11] riscv: OpenSBI boot test and cleanups

[PATCH v5 00/11] riscv: OpenSBI boot test and cleanups

[Daniel Henrique Barboza]

Hi,

This new version is still rebased on top of [1]:

"[PATCH v2 00/12] hw/riscv: Improve Spike HTIF emulation fidelity"

from Bin Meng.

The change from v4 is on patch 9 where we added an extra flag in
riscv_load_kernel() to allow for boards that don't load initrd
(e.g. opentitan and sifive_e) to opt out from loading it altogether.

* Patch without reviews: 9

Changes from v4:
- patch 9:
  - added a 'load_init' flag in riscv_load_kernel() to control whether
    the function should execute riscv_load_initrd() or not
v4 link: https://lists.gnu.org/archive/html/qemu-devel/2022-12/msg04652.html

Changes from v3:
- patch 1:
  - fixed more instances of 'opensbi' and 'Opensbi' to 'OpenSBI'
  - changed tests order
- patch 4 (new):
  - added a g_assert(filename) guard in riscv_load_initrd() and
    riscv_load_kernel()
v3 link: https://mail.gnu.org/archive/html/qemu-devel/2022-12/msg04491.html 

Changes from v2:
- patch 1:
  - reduced code repetition with a boot_opensbi() helper
  - renamed 'opensbi' to 'OpenSBI' in the file header
- patch 9:
  - renamed riscv_load_kernel() to riscv_load_kernel_and_initrd()
v2 link: https://mail.gnu.org/archive/html/qemu-devel/2022-12/msg04466.html


Changes from v1:
- patches were rebased with [1]
- patches 13-15: removed
  * will be re-sent in a follow-up series
- patches 4-5: removed since they're picked by Bin in [1]
- patch 1:
  - added a 'skip' riscv32 spike test
v1 link: https://mail.gnu.org/archive/html/qemu-devel/2022-12/msg03860.html


Based-on: <20221227064812.1903326-1-bmeng@tinylab.org>

Cc: Alistair Francis <alistair.francis@wdc.com>
Cc: Bin Meng <bin.meng@windriver.com>

[1] https://patchwork.ozlabs.org/project/qemu-devel/list/?series=334352

Daniel Henrique Barboza (11):
  tests/avocado: add RISC-V OpenSBI boot test
  hw/riscv/spike: use 'fdt' from MachineState
  hw/riscv/sifive_u: use 'fdt' from MachineState
  hw/riscv/boot.c: exit early if filename is NULL in load functions
  hw/riscv/spike.c: load initrd right after riscv_load_kernel()
  hw/riscv: write initrd 'chosen' FDT inside riscv_load_initrd()
  hw/riscv: write bootargs 'chosen' FDT after riscv_load_kernel()
  hw/riscv/boot.c: use MachineState in riscv_load_initrd()
  hw/riscv/boot.c: use MachineState in riscv_load_kernel()
  hw/riscv/boot.c: consolidate all kernel init in riscv_load_kernel()
  hw/riscv/boot.c: make riscv_load_initrd() static

 hw/riscv/boot.c                | 91 +++++++++++++++++++++++-----------
 hw/riscv/microchip_pfsoc.c     | 20 +-------
 hw/riscv/opentitan.c           |  3 +-
 hw/riscv/sifive_e.c            |  4 +-
 hw/riscv/sifive_u.c            | 32 +++---------
 hw/riscv/spike.c               | 37 ++++----------
 hw/riscv/virt.c                | 21 +-------
 include/hw/riscv/boot.h        |  5 +-
 include/hw/riscv/sifive_u.h    |  3 --
 include/hw/riscv/spike.h       |  2 -
 tests/avocado/riscv_opensbi.py | 65 ++++++++++++++++++++++++
 11 files changed, 150 insertions(+), 133 deletions(-)
 create mode 100644 tests/avocado/riscv_opensbi.py

-- 
2.39.0

[PATCH v5 01/11] tests/avocado: add RISC-V OpenSBI boot test

[Daniel Henrique Barboza]

This test is used to do a quick sanity check to ensure that we're able
to run the existing QEMU FW image.

'sifive_u', 'spike' and 'virt' riscv64 machines, and 'sifive_u' and
'virt' 32 bit machines are able to run the default RISCV64_BIOS_BIN |
RISCV32_BIOS_BIN firmware with minimal options.

The riscv32 'spike' machine isn't bootable at this moment, requiring an
OpenSBI fix [1] and QEMU side changes [2]. We could just leave at that
or add a 'skip' test to remind us about it. To work as a reminder that
we have a riscv32 'spike' test that should be enabled as soon as OpenSBI
QEMU rom receives the fix, we're adding a 'skip' test:

(06/18) tests/avocado/riscv_opensbi.py:RiscvOpenSBI.test_riscv32_spike:
        SKIP: requires OpenSBI fix to work

[1] https://patchwork.ozlabs.org/project/opensbi/patch/20221226033603.1860569-1-bmeng@tinylab.org/
[2] https://patchwork.ozlabs.org/project/qemu-devel/list/?series=334159

Cc: Cleber Rosa <crosa@redhat.com>
Cc: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Bin Meng <bmeng@tinylab.org>
Tested-by: Bin Meng <bmeng@tinylab.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 tests/avocado/riscv_opensbi.py | 65 ++++++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)
 create mode 100644 tests/avocado/riscv_opensbi.py

diff --git a/tests/avocado/riscv_opensbi.py b/tests/avocado/riscv_opensbi.py
new file mode 100644
index 0000000000..e02f0d404a
--- /dev/null
+++ b/tests/avocado/riscv_opensbi.py
@@ -0,0 +1,65 @@
+# OpenSBI boot test for RISC-V machines
+#
+# Copyright (c) 2022, Ventana Micro
+#
+# This work is licensed under the terms of the GNU GPL, version 2 or
+# later.  See the COPYING file in the top-level directory.
+
+from avocado_qemu import QemuSystemTest
+from avocado import skip
+from avocado_qemu import wait_for_console_pattern
+
+class RiscvOpenSBI(QemuSystemTest):
+    """
+    :avocado: tags=accel:tcg
+    """
+    timeout = 5
+
+    def boot_opensbi(self):
+        self.vm.set_console()
+        self.vm.launch()
+        wait_for_console_pattern(self, 'Platform Name')
+        wait_for_console_pattern(self, 'Boot HART MEDELEG')
+
+    @skip("requires OpenSBI fix to work")
+    def test_riscv32_spike(self):
+        """
+        :avocado: tags=arch:riscv32
+        :avocado: tags=machine:spike
+        """
+        self.boot_opensbi()
+
+    def test_riscv64_spike(self):
+        """
+        :avocado: tags=arch:riscv64
+        :avocado: tags=machine:spike
+        """
+        self.boot_opensbi()
+
+    def test_riscv32_sifive_u(self):
+        """
+        :avocado: tags=arch:riscv32
+        :avocado: tags=machine:sifive_u
+        """
+        self.boot_opensbi()
+
+    def test_riscv64_sifive_u(self):
+        """
+        :avocado: tags=arch:riscv64
+        :avocado: tags=machine:sifive_u
+        """
+        self.boot_opensbi()
+
+    def test_riscv32_virt(self):
+        """
+        :avocado: tags=arch:riscv32
+        :avocado: tags=machine:virt
+        """
+        self.boot_opensbi()
+
+    def test_riscv64_virt(self):
+        """
+        :avocado: tags=arch:riscv64
+        :avocado: tags=machine:virt
+        """
+        self.boot_opensbi()
-- 
2.39.0

[PATCH v5 02/11] hw/riscv/spike: use 'fdt' from MachineState

[Daniel Henrique Barboza]

The MachineState object provides a 'fdt' pointer that is already being
used by other RISC-V machines, and it's also used by the 'dumpdtb' QMP
command.

Remove the 'fdt' pointer from SpikeState and use MachineState::fdt
instead.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Bin Meng <bmeng@tinylab.org>
---
 hw/riscv/spike.c         | 12 +++++-------
 include/hw/riscv/spike.h |  2 --
 2 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index 1679c325d5..25c5420ee6 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -53,6 +53,7 @@ static void create_fdt(SpikeState *s, const MemMapEntry *memmap,
                        bool is_32_bit, bool htif_custom_base)
 {
     void *fdt;
+    int fdt_size;
     uint64_t addr, size;
     unsigned long clint_addr;
     int cpu, socket;
@@ -65,7 +66,7 @@ static void create_fdt(SpikeState *s, const MemMapEntry *memmap,
         "sifive,clint0", "riscv,clint0"
     };
 
-    fdt = s->fdt = create_device_tree(&s->fdt_size);
+    fdt = mc->fdt = create_device_tree(&fdt_size);
     if (!fdt) {
         error_report("create_device_tree() failed");
         exit(1);
@@ -327,18 +328,15 @@ static void spike_board_init(MachineState *machine)
         hwaddr end = riscv_load_initrd(machine->initrd_filename,
                                        machine->ram_size, kernel_entry,
                                        &start);
-        qemu_fdt_setprop_cell(s->fdt, "/chosen",
+        qemu_fdt_setprop_cell(machine->fdt, "/chosen",
                               "linux,initrd-start", start);
-        qemu_fdt_setprop_cell(s->fdt, "/chosen", "linux,initrd-end",
+        qemu_fdt_setprop_cell(machine->fdt, "/chosen", "linux,initrd-end",
                               end);
     }
 
     /* Compute the fdt load address in dram */
     fdt_load_addr = riscv_load_fdt(memmap[SPIKE_DRAM].base,
-                                   machine->ram_size, s->fdt);
-
-    /* Set machine->fdt for 'dumpdtb' QMP/HMP command */
-    machine->fdt = s->fdt;
+                                   machine->ram_size, machine->fdt);
 
     /* load the reset vector */
     riscv_setup_rom_reset_vec(machine, &s->soc[0], memmap[SPIKE_DRAM].base,
diff --git a/include/hw/riscv/spike.h b/include/hw/riscv/spike.h
index 73d69234de..d13a147942 100644
--- a/include/hw/riscv/spike.h
+++ b/include/hw/riscv/spike.h
@@ -37,8 +37,6 @@ struct SpikeState {
 
     /*< public >*/
     RISCVHartArrayState soc[SPIKE_SOCKETS_MAX];
-    void *fdt;
-    int fdt_size;
 };
 
 enum {
-- 
2.39.0

[PATCH v5 03/11] hw/riscv/sifive_u: use 'fdt' from MachineState

[Daniel Henrique Barboza]

The MachineState object provides a 'fdt' pointer that is already being
used by other RISC-V machines, and it's also used by the 'dumpdtb' QMP
command.

Remove the 'fdt' pointer from SiFiveUState and use MachineState::fdt
instead.

Cc: Palmer Dabbelt <palmer@dabbelt.com>
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Bin Meng <bmeng@tinylab.org>
---
 hw/riscv/sifive_u.c         | 15 ++++++---------
 include/hw/riscv/sifive_u.h |  3 ---
 2 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index a58ddb36ac..ddceb750ea 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -98,7 +98,7 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry *memmap,
 {
     MachineState *ms = MACHINE(qdev_get_machine());
     void *fdt;
-    int cpu;
+    int cpu, fdt_size;
     uint32_t *cells;
     char *nodename;
     uint32_t plic_phandle, prci_phandle, gpio_phandle, phandle = 1;
@@ -112,14 +112,14 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry *memmap,
     };
 
     if (ms->dtb) {
-        fdt = s->fdt = load_device_tree(ms->dtb, &s->fdt_size);
+        fdt = ms->fdt = load_device_tree(ms->dtb, &fdt_size);
         if (!fdt) {
             error_report("load_device_tree() failed");
             exit(1);
         }
         goto update_bootargs;
     } else {
-        fdt = s->fdt = create_device_tree(&s->fdt_size);
+        fdt = ms->fdt = create_device_tree(&fdt_size);
         if (!fdt) {
             error_report("create_device_tree() failed");
             exit(1);
@@ -612,9 +612,9 @@ static void sifive_u_machine_init(MachineState *machine)
             hwaddr end = riscv_load_initrd(machine->initrd_filename,
                                            machine->ram_size, kernel_entry,
                                            &start);
-            qemu_fdt_setprop_cell(s->fdt, "/chosen",
+            qemu_fdt_setprop_cell(machine->fdt, "/chosen",
                                   "linux,initrd-start", start);
-            qemu_fdt_setprop_cell(s->fdt, "/chosen", "linux,initrd-end",
+            qemu_fdt_setprop_cell(machine->fdt, "/chosen", "linux,initrd-end",
                                   end);
         }
     } else {
@@ -627,14 +627,11 @@ static void sifive_u_machine_init(MachineState *machine)
 
     /* Compute the fdt load address in dram */
     fdt_load_addr = riscv_load_fdt(memmap[SIFIVE_U_DEV_DRAM].base,
-                                   machine->ram_size, s->fdt);
+                                   machine->ram_size, machine->fdt);
     if (!riscv_is_32bit(&s->soc.u_cpus)) {
         start_addr_hi32 = (uint64_t)start_addr >> 32;
     }
 
-    /* Set machine->fdt for 'dumpdtb' QMP/HMP command */
-    machine->fdt = s->fdt;
-
     /* reset vector */
     uint32_t reset_vec[12] = {
         s->msel,                       /* MSEL pin state */
diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h
index e680d61ece..4a8828a30e 100644
--- a/include/hw/riscv/sifive_u.h
+++ b/include/hw/riscv/sifive_u.h
@@ -67,9 +67,6 @@ typedef struct SiFiveUState {
     /*< public >*/
     SiFiveUSoCState soc;
 
-    void *fdt;
-    int fdt_size;
-
     bool start_in_flash;
     uint32_t msel;
     uint32_t serial;
-- 
2.39.0

[PATCH v5 04/11] hw/riscv/boot.c: exit early if filename is NULL in load functions

[Daniel Henrique Barboza]

riscv_load_firmware(), riscv_load_initrd() and riscv_load_kernel() works
under the assumption that a 'filename' parameter is always not NULL.

This is currently the case since all callers of these functions are
checking for NULL before calling them. Add an g_assert() to make sure
that a NULL value in these cases are to be considered a bug.

Suggested-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/riscv/boot.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index 98b80af51b..31aa3385a0 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -153,6 +153,8 @@ target_ulong riscv_load_firmware(const char *firmware_filename,
     uint64_t firmware_entry, firmware_end;
     ssize_t firmware_size;
 
+    g_assert(firmware_filename != NULL);
+
     if (load_elf_ram_sym(firmware_filename, NULL, NULL, NULL,
                          &firmware_entry, NULL, &firmware_end, NULL,
                          0, EM_RISCV, 1, 0, NULL, true, sym_cb) > 0) {
@@ -177,6 +179,8 @@ target_ulong riscv_load_kernel(const char *kernel_filename,
 {
     uint64_t kernel_load_base, kernel_entry;
 
+    g_assert(kernel_filename != NULL);
+
     /*
      * NB: Use low address not ELF entry point to ensure that the fw_dynamic
      * behaviour when loading an ELF matches the fw_payload, fw_jump and BBL
@@ -209,6 +213,8 @@ hwaddr riscv_load_initrd(const char *filename, uint64_t mem_size,
 {
     ssize_t size;
 
+    g_assert(filename != NULL);
+
     /*
      * We want to put the initrd far enough into RAM that when the
      * kernel is uncompressed it will not clobber the initrd. However
-- 
2.39.0

[PATCH v5 05/11] hw/riscv/spike.c: load initrd right after riscv_load_kernel()

[Daniel Henrique Barboza]

This will make the code more in line with what the other boards are
doing. We'll also avoid an extra check to machine->kernel_filename since
we already checked that before executing riscv_load_kernel().

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Bin Meng <bmeng@tinylab.org>
---
 hw/riscv/spike.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index 25c5420ee6..004dfb2d5b 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -302,6 +302,10 @@ static void spike_board_init(MachineState *machine)
         g_free(firmware_name);
     }
 
+    /* Create device tree */
+    create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline,
+               riscv_is_32bit(&s->soc[0]), htif_custom_base);
+
     /* Load kernel */
     if (machine->kernel_filename) {
         kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc[0],
@@ -310,6 +314,17 @@ static void spike_board_init(MachineState *machine)
         kernel_entry = riscv_load_kernel(machine->kernel_filename,
                                          kernel_start_addr,
                                          htif_symbol_callback);
+
+        if (machine->initrd_filename) {
+            hwaddr start;
+            hwaddr end = riscv_load_initrd(machine->initrd_filename,
+                                           machine->ram_size, kernel_entry,
+                                           &start);
+            qemu_fdt_setprop_cell(machine->fdt, "/chosen",
+                                  "linux,initrd-start", start);
+            qemu_fdt_setprop_cell(machine->fdt, "/chosen", "linux,initrd-end",
+                                  end);
+        }
     } else {
        /*
         * If dynamic firmware is used, it doesn't know where is the next mode
@@ -318,22 +333,6 @@ static void spike_board_init(MachineState *machine)
         kernel_entry = 0;
     }
 
-    /* Create device tree */
-    create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline,
-               riscv_is_32bit(&s->soc[0]), htif_custom_base);
-
-    /* Load initrd */
-    if (machine->kernel_filename && machine->initrd_filename) {
-        hwaddr start;
-        hwaddr end = riscv_load_initrd(machine->initrd_filename,
-                                       machine->ram_size, kernel_entry,
-                                       &start);
-        qemu_fdt_setprop_cell(machine->fdt, "/chosen",
-                              "linux,initrd-start", start);
-        qemu_fdt_setprop_cell(machine->fdt, "/chosen", "linux,initrd-end",
-                              end);
-    }
-
     /* Compute the fdt load address in dram */
     fdt_load_addr = riscv_load_fdt(memmap[SPIKE_DRAM].base,
                                    machine->ram_size, machine->fdt);
-- 
2.39.0

[PATCH v5 06/11] hw/riscv: write initrd 'chosen' FDT inside riscv_load_initrd()

[Daniel Henrique Barboza]

riscv_load_initrd() returns the initrd end addr while also writing a
'start' var to mark the addr start. These informations are being used
just to write the initrd FDT node. Every existing caller of
riscv_load_initrd() is writing the FDT in the same manner.

We can simplify things by writing the FDT inside riscv_load_initrd(),
sparing callers from having to manage start/end addrs to write the FDT
themselves.

An 'if (fdt)' check is already inserted at the end of the function
because we'll end up using it later on with other boards that doesn´t
have a FDT.

Cc: Palmer Dabbelt <palmer@dabbelt.com>
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Bin Meng <bmeng@tinylab.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/riscv/boot.c            | 18 ++++++++++++------
 hw/riscv/microchip_pfsoc.c | 10 ++--------
 hw/riscv/sifive_u.c        | 10 ++--------
 hw/riscv/spike.c           | 10 ++--------
 hw/riscv/virt.c            | 10 ++--------
 include/hw/riscv/boot.h    |  4 ++--
 6 files changed, 22 insertions(+), 40 deletions(-)

diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index 31aa3385a0..6b948d1c9e 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -208,9 +208,10 @@ target_ulong riscv_load_kernel(const char *kernel_filename,
     exit(1);
 }
 
-hwaddr riscv_load_initrd(const char *filename, uint64_t mem_size,
-                         uint64_t kernel_entry, hwaddr *start)
+void riscv_load_initrd(const char *filename, uint64_t mem_size,
+                       uint64_t kernel_entry, void *fdt)
 {
+    hwaddr start, end;
     ssize_t size;
 
     g_assert(filename != NULL);
@@ -226,18 +227,23 @@ hwaddr riscv_load_initrd(const char *filename, uint64_t mem_size,
      * 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 = kernel_entry + MIN(mem_size / 2, 128 * MiB);
 
-    size = load_ramdisk(filename, *start, mem_size - *start);
+    size = load_ramdisk(filename, start, mem_size - start);
     if (size == -1) {
-        size = load_image_targphys(filename, *start, mem_size - *start);
+        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;
+    /* Some RISC-V machines (e.g. opentitan) don't have a fdt. */
+    if (fdt) {
+        end = start + size;
+        qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-start", start);
+        qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-end", end);
+    }
 }
 
 uint64_t riscv_load_fdt(hwaddr dram_base, uint64_t mem_size, void *fdt)
diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
index b10321b564..593a799549 100644
--- a/hw/riscv/microchip_pfsoc.c
+++ b/hw/riscv/microchip_pfsoc.c
@@ -633,14 +633,8 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
                                          kernel_start_addr, NULL);
 
         if (machine->initrd_filename) {
-            hwaddr start;
-            hwaddr end = riscv_load_initrd(machine->initrd_filename,
-                                           machine->ram_size, kernel_entry,
-                                           &start);
-            qemu_fdt_setprop_cell(machine->fdt, "/chosen",
-                                  "linux,initrd-start", start);
-            qemu_fdt_setprop_cell(machine->fdt, "/chosen",
-                                  "linux,initrd-end", end);
+            riscv_load_initrd(machine->initrd_filename, machine->ram_size,
+                              kernel_entry, machine->fdt);
         }
 
         if (machine->kernel_cmdline && *machine->kernel_cmdline) {
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index ddceb750ea..37f5087172 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -608,14 +608,8 @@ static void sifive_u_machine_init(MachineState *machine)
                                          kernel_start_addr, NULL);
 
         if (machine->initrd_filename) {
-            hwaddr start;
-            hwaddr end = riscv_load_initrd(machine->initrd_filename,
-                                           machine->ram_size, kernel_entry,
-                                           &start);
-            qemu_fdt_setprop_cell(machine->fdt, "/chosen",
-                                  "linux,initrd-start", start);
-            qemu_fdt_setprop_cell(machine->fdt, "/chosen", "linux,initrd-end",
-                                  end);
+            riscv_load_initrd(machine->initrd_filename, machine->ram_size,
+                              kernel_entry, machine->fdt);
         }
     } else {
        /*
diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index 004dfb2d5b..5668fe0694 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -316,14 +316,8 @@ static void spike_board_init(MachineState *machine)
                                          htif_symbol_callback);
 
         if (machine->initrd_filename) {
-            hwaddr start;
-            hwaddr end = riscv_load_initrd(machine->initrd_filename,
-                                           machine->ram_size, kernel_entry,
-                                           &start);
-            qemu_fdt_setprop_cell(machine->fdt, "/chosen",
-                                  "linux,initrd-start", start);
-            qemu_fdt_setprop_cell(machine->fdt, "/chosen", "linux,initrd-end",
-                                  end);
+            riscv_load_initrd(machine->initrd_filename, machine->ram_size,
+                              kernel_entry, machine->fdt);
         }
     } else {
        /*
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 408f7a2256..5967b136b4 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1291,14 +1291,8 @@ static void virt_machine_done(Notifier *notifier, void *data)
                                          kernel_start_addr, NULL);
 
         if (machine->initrd_filename) {
-            hwaddr start;
-            hwaddr end = riscv_load_initrd(machine->initrd_filename,
-                                           machine->ram_size, kernel_entry,
-                                           &start);
-            qemu_fdt_setprop_cell(machine->fdt, "/chosen",
-                                  "linux,initrd-start", start);
-            qemu_fdt_setprop_cell(machine->fdt, "/chosen", "linux,initrd-end",
-                                  end);
+            riscv_load_initrd(machine->initrd_filename, machine->ram_size,
+                              kernel_entry, machine->fdt);
         }
     } else {
        /*
diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
index b273ab22f7..e37e1d1238 100644
--- a/include/hw/riscv/boot.h
+++ b/include/hw/riscv/boot.h
@@ -46,8 +46,8 @@ target_ulong riscv_load_firmware(const char *firmware_filename,
 target_ulong riscv_load_kernel(const char *kernel_filename,
                                target_ulong firmware_end_addr,
                                symbol_fn_t sym_cb);
-hwaddr riscv_load_initrd(const char *filename, uint64_t mem_size,
-                         uint64_t kernel_entry, hwaddr *start);
+void riscv_load_initrd(const char *filename, uint64_t mem_size,
+                       uint64_t kernel_entry, void *fdt);
 uint64_t riscv_load_fdt(hwaddr dram_start, uint64_t dram_size, void *fdt);
 void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts,
                                hwaddr saddr,
-- 
2.39.0

[PATCH v5 07/11] hw/riscv: write bootargs 'chosen' FDT after riscv_load_kernel()

[Daniel Henrique Barboza]

The sifive_u, spike and virt machines are writing the 'bootargs' FDT
node during their respective create_fdt().

Given that bootargs is written only when '-append' is used, and this
option is only allowed with the '-kernel' option, which in turn is
already being check before executing riscv_load_kernel(), write
'bootargs' in the same code path as riscv_load_kernel().

Cc: Palmer Dabbelt <palmer@dabbelt.com>
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Bin Meng <bmeng@tinylab.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/riscv/sifive_u.c | 11 +++++------
 hw/riscv/spike.c    |  9 +++++----
 hw/riscv/virt.c     | 11 +++++------
 3 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index 37f5087172..3e6df87b5b 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -117,7 +117,6 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry *memmap,
             error_report("load_device_tree() failed");
             exit(1);
         }
-        goto update_bootargs;
     } else {
         fdt = ms->fdt = create_device_tree(&fdt_size);
         if (!fdt) {
@@ -510,11 +509,6 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry *memmap,
     qemu_fdt_setprop_string(fdt, "/aliases", "serial0", nodename);
 
     g_free(nodename);
-
-update_bootargs:
-    if (cmdline && *cmdline) {
-        qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline);
-    }
 }
 
 static void sifive_u_machine_reset(void *opaque, int n, int level)
@@ -611,6 +605,11 @@ static void sifive_u_machine_init(MachineState *machine)
             riscv_load_initrd(machine->initrd_filename, machine->ram_size,
                               kernel_entry, machine->fdt);
         }
+
+        if (machine->kernel_cmdline && *machine->kernel_cmdline) {
+            qemu_fdt_setprop_string(machine->fdt, "/chosen", "bootargs",
+                                    machine->kernel_cmdline);
+        }
     } else {
        /*
         * If dynamic firmware is used, it doesn't know where is the next mode
diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index 5668fe0694..60e2912be5 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -179,10 +179,6 @@ static void create_fdt(SpikeState *s, const MemMapEntry *memmap,
 
     qemu_fdt_add_subnode(fdt, "/chosen");
     qemu_fdt_setprop_string(fdt, "/chosen", "stdout-path", "/htif");
-
-    if (cmdline && *cmdline) {
-        qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline);
-    }
 }
 
 static bool spike_test_elf_image(char *filename)
@@ -319,6 +315,11 @@ static void spike_board_init(MachineState *machine)
             riscv_load_initrd(machine->initrd_filename, machine->ram_size,
                               kernel_entry, machine->fdt);
         }
+
+        if (machine->kernel_cmdline && *machine->kernel_cmdline) {
+            qemu_fdt_setprop_string(machine->fdt, "/chosen", "bootargs",
+                                    machine->kernel_cmdline);
+        }
     } else {
        /*
         * If dynamic firmware is used, it doesn't know where is the next mode
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 5967b136b4..6c946b6def 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1012,7 +1012,6 @@ static void create_fdt(RISCVVirtState *s, const MemMapEntry *memmap,
             error_report("load_device_tree() failed");
             exit(1);
         }
-        goto update_bootargs;
     } else {
         mc->fdt = create_device_tree(&s->fdt_size);
         if (!mc->fdt) {
@@ -1050,11 +1049,6 @@ static void create_fdt(RISCVVirtState *s, const MemMapEntry *memmap,
     create_fdt_fw_cfg(s, memmap);
     create_fdt_pmu(s);
 
-update_bootargs:
-    if (cmdline && *cmdline) {
-        qemu_fdt_setprop_string(mc->fdt, "/chosen", "bootargs", cmdline);
-    }
-
     /* Pass seed to RNG */
     qemu_guest_getrandom_nofail(rng_seed, sizeof(rng_seed));
     qemu_fdt_setprop(mc->fdt, "/chosen", "rng-seed", rng_seed, sizeof(rng_seed));
@@ -1294,6 +1288,11 @@ static void virt_machine_done(Notifier *notifier, void *data)
             riscv_load_initrd(machine->initrd_filename, machine->ram_size,
                               kernel_entry, machine->fdt);
         }
+
+        if (machine->kernel_cmdline && *machine->kernel_cmdline) {
+            qemu_fdt_setprop_string(machine->fdt, "/chosen", "bootargs",
+                                    machine->kernel_cmdline);
+        }
     } else {
        /*
         * If dynamic firmware is used, it doesn't know where is the next mode
-- 
2.39.0

[PATCH v5 08/11] hw/riscv/boot.c: use MachineState in riscv_load_initrd()

[Daniel Henrique Barboza]

'filename', 'mem_size' and 'fdt' from riscv_load_initrd() can all be
retrieved by the MachineState object for all callers.

Cc: Palmer Dabbelt <palmer@dabbelt.com>
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Bin Meng <bmeng@tinylab.org>
---
 hw/riscv/boot.c            | 6 ++++--
 hw/riscv/microchip_pfsoc.c | 3 +--
 hw/riscv/sifive_u.c        | 3 +--
 hw/riscv/spike.c           | 3 +--
 hw/riscv/virt.c            | 3 +--
 include/hw/riscv/boot.h    | 3 +--
 6 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index 6b948d1c9e..d3e780c3b6 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -208,9 +208,11 @@ target_ulong riscv_load_kernel(const char *kernel_filename,
     exit(1);
 }
 
-void riscv_load_initrd(const char *filename, uint64_t mem_size,
-                       uint64_t kernel_entry, void *fdt)
+void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
 {
+    const char *filename = machine->initrd_filename;
+    uint64_t mem_size = machine->ram_size;
+    void *fdt = machine->fdt;
     hwaddr start, end;
     ssize_t size;
 
diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
index 593a799549..1e9b0a420e 100644
--- a/hw/riscv/microchip_pfsoc.c
+++ b/hw/riscv/microchip_pfsoc.c
@@ -633,8 +633,7 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
                                          kernel_start_addr, NULL);
 
         if (machine->initrd_filename) {
-            riscv_load_initrd(machine->initrd_filename, machine->ram_size,
-                              kernel_entry, machine->fdt);
+            riscv_load_initrd(machine, kernel_entry);
         }
 
         if (machine->kernel_cmdline && *machine->kernel_cmdline) {
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index 3e6df87b5b..c40885ed5c 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -602,8 +602,7 @@ static void sifive_u_machine_init(MachineState *machine)
                                          kernel_start_addr, NULL);
 
         if (machine->initrd_filename) {
-            riscv_load_initrd(machine->initrd_filename, machine->ram_size,
-                              kernel_entry, machine->fdt);
+            riscv_load_initrd(machine, kernel_entry);
         }
 
         if (machine->kernel_cmdline && *machine->kernel_cmdline) {
diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index 60e2912be5..99dec74fe8 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -312,8 +312,7 @@ static void spike_board_init(MachineState *machine)
                                          htif_symbol_callback);
 
         if (machine->initrd_filename) {
-            riscv_load_initrd(machine->initrd_filename, machine->ram_size,
-                              kernel_entry, machine->fdt);
+            riscv_load_initrd(machine, kernel_entry);
         }
 
         if (machine->kernel_cmdline && *machine->kernel_cmdline) {
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 6c946b6def..02f1369843 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1285,8 +1285,7 @@ static void virt_machine_done(Notifier *notifier, void *data)
                                          kernel_start_addr, NULL);
 
         if (machine->initrd_filename) {
-            riscv_load_initrd(machine->initrd_filename, machine->ram_size,
-                              kernel_entry, machine->fdt);
+            riscv_load_initrd(machine, kernel_entry);
         }
 
         if (machine->kernel_cmdline && *machine->kernel_cmdline) {
diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
index e37e1d1238..cfd72ecabf 100644
--- a/include/hw/riscv/boot.h
+++ b/include/hw/riscv/boot.h
@@ -46,8 +46,7 @@ target_ulong riscv_load_firmware(const char *firmware_filename,
 target_ulong riscv_load_kernel(const char *kernel_filename,
                                target_ulong firmware_end_addr,
                                symbol_fn_t sym_cb);
-void riscv_load_initrd(const char *filename, uint64_t mem_size,
-                       uint64_t kernel_entry, void *fdt);
+void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry);
 uint64_t riscv_load_fdt(hwaddr dram_start, uint64_t dram_size, void *fdt);
 void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts,
                                hwaddr saddr,
-- 
2.39.0

[PATCH v5 09/11] hw/riscv/boot.c: use MachineState in riscv_load_kernel()

[Daniel Henrique Barboza]

All callers are using kernel_filename as machine->kernel_filename.

This will also simplify the changes in riscv_load_kernel() that we're
going to do next.

Cc: Palmer Dabbelt <palmer@dabbelt.com>
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Bin Meng <bmeng@tinylab.org>
---
 hw/riscv/boot.c            | 3 ++-
 hw/riscv/microchip_pfsoc.c | 3 +--
 hw/riscv/opentitan.c       | 3 +--
 hw/riscv/sifive_e.c        | 3 +--
 hw/riscv/sifive_u.c        | 3 +--
 hw/riscv/spike.c           | 3 +--
 hw/riscv/virt.c            | 3 +--
 include/hw/riscv/boot.h    | 2 +-
 8 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index d3e780c3b6..2594276223 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -173,10 +173,11 @@ target_ulong riscv_load_firmware(const char *firmware_filename,
     exit(1);
 }
 
-target_ulong riscv_load_kernel(const char *kernel_filename,
+target_ulong riscv_load_kernel(MachineState *machine,
                                target_ulong kernel_start_addr,
                                symbol_fn_t sym_cb)
 {
+    const char *kernel_filename = machine->kernel_filename;
     uint64_t kernel_load_base, kernel_entry;
 
     g_assert(kernel_filename != NULL);
diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
index 1e9b0a420e..82ae5e7023 100644
--- a/hw/riscv/microchip_pfsoc.c
+++ b/hw/riscv/microchip_pfsoc.c
@@ -629,8 +629,7 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
         kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc.u_cpus,
                                                          firmware_end_addr);
 
-        kernel_entry = riscv_load_kernel(machine->kernel_filename,
-                                         kernel_start_addr, NULL);
+        kernel_entry = riscv_load_kernel(machine, kernel_start_addr, NULL);
 
         if (machine->initrd_filename) {
             riscv_load_initrd(machine, kernel_entry);
diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c
index 85ffdac5be..64d5d435b9 100644
--- a/hw/riscv/opentitan.c
+++ b/hw/riscv/opentitan.c
@@ -101,8 +101,7 @@ static void opentitan_board_init(MachineState *machine)
     }
 
     if (machine->kernel_filename) {
-        riscv_load_kernel(machine->kernel_filename,
-                          memmap[IBEX_DEV_RAM].base, NULL);
+        riscv_load_kernel(machine, memmap[IBEX_DEV_RAM].base, NULL);
     }
 }
 
diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
index d65d2fd869..3e3f4b0088 100644
--- a/hw/riscv/sifive_e.c
+++ b/hw/riscv/sifive_e.c
@@ -114,8 +114,7 @@ static void sifive_e_machine_init(MachineState *machine)
                           memmap[SIFIVE_E_DEV_MROM].base, &address_space_memory);
 
     if (machine->kernel_filename) {
-        riscv_load_kernel(machine->kernel_filename,
-                          memmap[SIFIVE_E_DEV_DTIM].base, NULL);
+        riscv_load_kernel(machine, memmap[SIFIVE_E_DEV_DTIM].base, NULL);
     }
 }
 
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index c40885ed5c..bac394c959 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -598,8 +598,7 @@ static void sifive_u_machine_init(MachineState *machine)
         kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc.u_cpus,
                                                          firmware_end_addr);
 
-        kernel_entry = riscv_load_kernel(machine->kernel_filename,
-                                         kernel_start_addr, NULL);
+        kernel_entry = riscv_load_kernel(machine, kernel_start_addr, NULL);
 
         if (machine->initrd_filename) {
             riscv_load_initrd(machine, kernel_entry);
diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index 99dec74fe8..bff9475686 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -307,8 +307,7 @@ static void spike_board_init(MachineState *machine)
         kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc[0],
                                                          firmware_end_addr);
 
-        kernel_entry = riscv_load_kernel(machine->kernel_filename,
-                                         kernel_start_addr,
+        kernel_entry = riscv_load_kernel(machine, kernel_start_addr,
                                          htif_symbol_callback);
 
         if (machine->initrd_filename) {
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 02f1369843..c8e35f861e 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1281,8 +1281,7 @@ static void virt_machine_done(Notifier *notifier, void *data)
         kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc[0],
                                                          firmware_end_addr);
 
-        kernel_entry = riscv_load_kernel(machine->kernel_filename,
-                                         kernel_start_addr, NULL);
+        kernel_entry = riscv_load_kernel(machine, kernel_start_addr, NULL);
 
         if (machine->initrd_filename) {
             riscv_load_initrd(machine, kernel_entry);
diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
index cfd72ecabf..f94653a09b 100644
--- a/include/hw/riscv/boot.h
+++ b/include/hw/riscv/boot.h
@@ -43,7 +43,7 @@ char *riscv_find_firmware(const char *firmware_filename,
 target_ulong riscv_load_firmware(const char *firmware_filename,
                                  hwaddr firmware_load_addr,
                                  symbol_fn_t sym_cb);
-target_ulong riscv_load_kernel(const char *kernel_filename,
+target_ulong riscv_load_kernel(MachineState *machine,
                                target_ulong firmware_end_addr,
                                symbol_fn_t sym_cb);
 void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry);
-- 
2.39.0

[PATCH v5 10/11] hw/riscv/boot.c: consolidate all kernel init in riscv_load_kernel()

[Daniel Henrique Barboza]

The microchip_icicle_kit, sifive_u, spike and virt boards are now doing
the same steps when '-kernel' is used:

- execute load_kernel()
- load init_rd()
- write kernel_cmdline

Let's fold everything inside riscv_load_kernel() to avoid code
repetition. To not change the behavior of boards that aren't calling
riscv_load_init(), add an 'load_initrd' flag to riscv_load_kernel() and
allow these boards to opt out from initrd loading.

Cc: Palmer Dabbelt <palmer@dabbelt.com>
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/riscv/boot.c            | 22 +++++++++++++++++++---
 hw/riscv/microchip_pfsoc.c | 12 ++----------
 hw/riscv/opentitan.c       |  2 +-
 hw/riscv/sifive_e.c        |  3 ++-
 hw/riscv/sifive_u.c        | 12 ++----------
 hw/riscv/spike.c           | 11 +----------
 hw/riscv/virt.c            | 12 ++----------
 include/hw/riscv/boot.h    |  1 +
 8 files changed, 30 insertions(+), 45 deletions(-)

diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index 2594276223..4888d5c1e0 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -175,10 +175,12 @@ target_ulong riscv_load_firmware(const char *firmware_filename,
 
 target_ulong riscv_load_kernel(MachineState *machine,
                                target_ulong kernel_start_addr,
+                               bool load_initrd,
                                symbol_fn_t sym_cb)
 {
     const char *kernel_filename = machine->kernel_filename;
     uint64_t kernel_load_base, kernel_entry;
+    void *fdt = machine->fdt;
 
     g_assert(kernel_filename != NULL);
 
@@ -192,21 +194,35 @@ target_ulong riscv_load_kernel(MachineState *machine,
     if (load_elf_ram_sym(kernel_filename, NULL, NULL, NULL,
                          NULL, &kernel_load_base, NULL, NULL, 0,
                          EM_RISCV, 1, 0, NULL, true, sym_cb) > 0) {
-        return kernel_load_base;
+        kernel_entry = kernel_load_base;
+        goto out;
     }
 
     if (load_uimage_as(kernel_filename, &kernel_entry, NULL, NULL,
                        NULL, NULL, NULL) > 0) {
-        return kernel_entry;
+        goto out;
     }
 
     if (load_image_targphys_as(kernel_filename, kernel_start_addr,
                                current_machine->ram_size, NULL) > 0) {
-        return kernel_start_addr;
+        kernel_entry = kernel_start_addr;
+        goto out;
     }
 
     error_report("could not load kernel '%s'", kernel_filename);
     exit(1);
+
+out:
+    if (load_initrd && machine->initrd_filename) {
+        riscv_load_initrd(machine, kernel_entry);
+    }
+
+    if (fdt && machine->kernel_cmdline && *machine->kernel_cmdline) {
+        qemu_fdt_setprop_string(fdt, "/chosen", "bootargs",
+                                machine->kernel_cmdline);
+    }
+
+    return kernel_entry;
 }
 
 void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
index 82ae5e7023..c45023a2b1 100644
--- a/hw/riscv/microchip_pfsoc.c
+++ b/hw/riscv/microchip_pfsoc.c
@@ -629,16 +629,8 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
         kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc.u_cpus,
                                                          firmware_end_addr);
 
-        kernel_entry = riscv_load_kernel(machine, kernel_start_addr, NULL);
-
-        if (machine->initrd_filename) {
-            riscv_load_initrd(machine, kernel_entry);
-        }
-
-        if (machine->kernel_cmdline && *machine->kernel_cmdline) {
-            qemu_fdt_setprop_string(machine->fdt, "/chosen",
-                                    "bootargs", machine->kernel_cmdline);
-        }
+        kernel_entry = riscv_load_kernel(machine, kernel_start_addr,
+                                         true, NULL);
 
         /* Compute the fdt load address in dram */
         fdt_load_addr = riscv_load_fdt(memmap[MICROCHIP_PFSOC_DRAM_LO].base,
diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c
index 64d5d435b9..f6fd9725a5 100644
--- a/hw/riscv/opentitan.c
+++ b/hw/riscv/opentitan.c
@@ -101,7 +101,7 @@ static void opentitan_board_init(MachineState *machine)
     }
 
     if (machine->kernel_filename) {
-        riscv_load_kernel(machine, memmap[IBEX_DEV_RAM].base, NULL);
+        riscv_load_kernel(machine, memmap[IBEX_DEV_RAM].base, false, NULL);
     }
 }
 
diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
index 3e3f4b0088..6835d1c807 100644
--- a/hw/riscv/sifive_e.c
+++ b/hw/riscv/sifive_e.c
@@ -114,7 +114,8 @@ static void sifive_e_machine_init(MachineState *machine)
                           memmap[SIFIVE_E_DEV_MROM].base, &address_space_memory);
 
     if (machine->kernel_filename) {
-        riscv_load_kernel(machine, memmap[SIFIVE_E_DEV_DTIM].base, NULL);
+        riscv_load_kernel(machine, memmap[SIFIVE_E_DEV_DTIM].base,
+                          false, NULL);
     }
 }
 
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index bac394c959..9a75d4aa62 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -598,16 +598,8 @@ static void sifive_u_machine_init(MachineState *machine)
         kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc.u_cpus,
                                                          firmware_end_addr);
 
-        kernel_entry = riscv_load_kernel(machine, kernel_start_addr, NULL);
-
-        if (machine->initrd_filename) {
-            riscv_load_initrd(machine, kernel_entry);
-        }
-
-        if (machine->kernel_cmdline && *machine->kernel_cmdline) {
-            qemu_fdt_setprop_string(machine->fdt, "/chosen", "bootargs",
-                                    machine->kernel_cmdline);
-        }
+        kernel_entry = riscv_load_kernel(machine, kernel_start_addr,
+                                         true, NULL);
     } else {
        /*
         * If dynamic firmware is used, it doesn't know where is the next mode
diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index bff9475686..c517885e6e 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -308,16 +308,7 @@ static void spike_board_init(MachineState *machine)
                                                          firmware_end_addr);
 
         kernel_entry = riscv_load_kernel(machine, kernel_start_addr,
-                                         htif_symbol_callback);
-
-        if (machine->initrd_filename) {
-            riscv_load_initrd(machine, kernel_entry);
-        }
-
-        if (machine->kernel_cmdline && *machine->kernel_cmdline) {
-            qemu_fdt_setprop_string(machine->fdt, "/chosen", "bootargs",
-                                    machine->kernel_cmdline);
-        }
+                                         true, htif_symbol_callback);
     } else {
        /*
         * If dynamic firmware is used, it doesn't know where is the next mode
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index c8e35f861e..a931ed05ab 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1281,16 +1281,8 @@ static void virt_machine_done(Notifier *notifier, void *data)
         kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc[0],
                                                          firmware_end_addr);
 
-        kernel_entry = riscv_load_kernel(machine, kernel_start_addr, NULL);
-
-        if (machine->initrd_filename) {
-            riscv_load_initrd(machine, kernel_entry);
-        }
-
-        if (machine->kernel_cmdline && *machine->kernel_cmdline) {
-            qemu_fdt_setprop_string(machine->fdt, "/chosen", "bootargs",
-                                    machine->kernel_cmdline);
-        }
+        kernel_entry = riscv_load_kernel(machine, kernel_start_addr,
+                                         true, NULL);
     } else {
        /*
         * If dynamic firmware is used, it doesn't know where is the next mode
diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
index f94653a09b..c3de897371 100644
--- a/include/hw/riscv/boot.h
+++ b/include/hw/riscv/boot.h
@@ -45,6 +45,7 @@ target_ulong riscv_load_firmware(const char *firmware_filename,
                                  symbol_fn_t sym_cb);
 target_ulong riscv_load_kernel(MachineState *machine,
                                target_ulong firmware_end_addr,
+                               bool load_initrd,
                                symbol_fn_t sym_cb);
 void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry);
 uint64_t riscv_load_fdt(hwaddr dram_start, uint64_t dram_size, void *fdt);
-- 
2.39.0

[PATCH v5 11/11] hw/riscv/boot.c: make riscv_load_initrd() static

[Daniel Henrique Barboza]

The only remaining caller is riscv_load_kernel_and_initrd() which
belongs to the same file.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Bin Meng <bmeng@tinylab.org>
---
 hw/riscv/boot.c         | 80 ++++++++++++++++++++---------------------
 include/hw/riscv/boot.h |  1 -
 2 files changed, 40 insertions(+), 41 deletions(-)

diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index 4888d5c1e0..e868fb6ade 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -173,6 +173,46 @@ target_ulong riscv_load_firmware(const char *firmware_filename,
     exit(1);
 }
 
+static void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
+{
+    const char *filename = machine->initrd_filename;
+    uint64_t mem_size = machine->ram_size;
+    void *fdt = machine->fdt;
+    hwaddr start, end;
+    ssize_t size;
+
+    g_assert(filename != NULL);
+
+    /*
+     * 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);
+        }
+    }
+
+    /* Some RISC-V machines (e.g. opentitan) don't have a fdt. */
+    if (fdt) {
+        end = start + size;
+        qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-start", start);
+        qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-end", end);
+    }
+}
+
 target_ulong riscv_load_kernel(MachineState *machine,
                                target_ulong kernel_start_addr,
                                bool load_initrd,
@@ -225,46 +265,6 @@ out:
     return kernel_entry;
 }
 
-void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
-{
-    const char *filename = machine->initrd_filename;
-    uint64_t mem_size = machine->ram_size;
-    void *fdt = machine->fdt;
-    hwaddr start, end;
-    ssize_t size;
-
-    g_assert(filename != NULL);
-
-    /*
-     * 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);
-        }
-    }
-
-    /* Some RISC-V machines (e.g. opentitan) don't have a fdt. */
-    if (fdt) {
-        end = start + size;
-        qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-start", start);
-        qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-end", end);
-    }
-}
-
 uint64_t riscv_load_fdt(hwaddr dram_base, uint64_t mem_size, void *fdt)
 {
     uint64_t temp, fdt_addr;
diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
index c3de897371..cbd131bad7 100644
--- a/include/hw/riscv/boot.h
+++ b/include/hw/riscv/boot.h
@@ -47,7 +47,6 @@ target_ulong riscv_load_kernel(MachineState *machine,
                                target_ulong firmware_end_addr,
                                bool load_initrd,
                                symbol_fn_t sym_cb);
-void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry);
 uint64_t riscv_load_fdt(hwaddr dram_start, uint64_t dram_size, void *fdt);
 void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts,
                                hwaddr saddr,
-- 
2.39.0

Re: [PATCH v5 04/11] hw/riscv/boot.c: exit early if filename is NULL in load functions

[Bin Meng]

On Mon, Jan 2, 2023 at 7:54 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> riscv_load_firmware(), riscv_load_initrd() and riscv_load_kernel() works
> under the assumption that a 'filename' parameter is always not NULL.
>
> This is currently the case since all callers of these functions are
> checking for NULL before calling them. Add an g_assert() to make sure
> that a NULL value in these cases are to be considered a bug.
>
> Suggested-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  hw/riscv/boot.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>

Reviewed-by: Bin Meng <bmeng@tinylab.org>

Re: [PATCH v5 10/11] hw/riscv/boot.c: consolidate all kernel init in riscv_load_kernel()

[Bin Meng]

On Mon, Jan 2, 2023 at 7:55 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> The microchip_icicle_kit, sifive_u, spike and virt boards are now doing
> the same steps when '-kernel' is used:
>
> - execute load_kernel()
> - load init_rd()
> - write kernel_cmdline
>
> Let's fold everything inside riscv_load_kernel() to avoid code
> repetition. To not change the behavior of boards that aren't calling
> riscv_load_init(), add an 'load_initrd' flag to riscv_load_kernel() and

typo: should be riscv_load_initrd()

> allow these boards to opt out from initrd loading.
>
> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  hw/riscv/boot.c            | 22 +++++++++++++++++++---
>  hw/riscv/microchip_pfsoc.c | 12 ++----------
>  hw/riscv/opentitan.c       |  2 +-
>  hw/riscv/sifive_e.c        |  3 ++-
>  hw/riscv/sifive_u.c        | 12 ++----------
>  hw/riscv/spike.c           | 11 +----------
>  hw/riscv/virt.c            | 12 ++----------
>  include/hw/riscv/boot.h    |  1 +
>  8 files changed, 30 insertions(+), 45 deletions(-)
>

Otherwise,
Reviewed-by: Bin Meng <bmeng@tinylab.org>

Re: [PATCH v5 10/11] hw/riscv/boot.c: consolidate all kernel init in riscv_load_kernel()

[Daniel Henrique Barboza]

On 1/8/23 00:33, Bin Meng wrote:
> On Mon, Jan 2, 2023 at 7:55 PM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
>> The microchip_icicle_kit, sifive_u, spike and virt boards are now doing
>> the same steps when '-kernel' is used:
>>
>> - execute load_kernel()
>> - load init_rd()
>> - write kernel_cmdline
>>
>> Let's fold everything inside riscv_load_kernel() to avoid code
>> repetition. To not change the behavior of boards that aren't calling
>> riscv_load_init(), add an 'load_initrd' flag to riscv_load_kernel() and
> typo: should be riscv_load_initrd()
>
>> allow these boards to opt out from initrd loading.
>>
>> Cc: Palmer Dabbelt <palmer@dabbelt.com>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>>   hw/riscv/boot.c            | 22 +++++++++++++++++++---
>>   hw/riscv/microchip_pfsoc.c | 12 ++----------
>>   hw/riscv/opentitan.c       |  2 +-
>>   hw/riscv/sifive_e.c        |  3 ++-
>>   hw/riscv/sifive_u.c        | 12 ++----------
>>   hw/riscv/spike.c           | 11 +----------
>>   hw/riscv/virt.c            | 12 ++----------
>>   include/hw/riscv/boot.h    |  1 +
>>   8 files changed, 30 insertions(+), 45 deletions(-)
>>
> Otherwise,
> Reviewed-by: Bin Meng <bmeng@tinylab.org>

Thanks!

Alistair, let me know if you want me to send another version with the commit
message typo fixed. I might as well take the change to rebase it with
riscv-to-apply.next.


Daniel

Re: [PATCH v5 10/11] hw/riscv/boot.c: consolidate all kernel init in riscv_load_kernel()

[Daniel Henrique Barboza]

On 1/10/23 08:43, Daniel Henrique Barboza wrote:
>
>
> On 1/8/23 00:33, Bin Meng wrote:
>> On Mon, Jan 2, 2023 at 7:55 PM Daniel Henrique Barboza
>> <dbarboza@ventanamicro.com> wrote:
>>> The microchip_icicle_kit, sifive_u, spike and virt boards are now doing
>>> the same steps when '-kernel' is used:
>>>
>>> - execute load_kernel()
>>> - load init_rd()
>>> - write kernel_cmdline
>>>
>>> Let's fold everything inside riscv_load_kernel() to avoid code
>>> repetition. To not change the behavior of boards that aren't calling
>>> riscv_load_init(), add an 'load_initrd' flag to riscv_load_kernel() and
>> typo: should be riscv_load_initrd()
>>
>>> allow these boards to opt out from initrd loading.
>>>
>>> Cc: Palmer Dabbelt <palmer@dabbelt.com>
>>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>>> ---
>>>   hw/riscv/boot.c            | 22 +++++++++++++++++++---
>>>   hw/riscv/microchip_pfsoc.c | 12 ++----------
>>>   hw/riscv/opentitan.c       |  2 +-
>>>   hw/riscv/sifive_e.c        |  3 ++-
>>>   hw/riscv/sifive_u.c        | 12 ++----------
>>>   hw/riscv/spike.c           | 11 +----------
>>>   hw/riscv/virt.c            | 12 ++----------
>>>   include/hw/riscv/boot.h    |  1 +
>>>   8 files changed, 30 insertions(+), 45 deletions(-)
>>>
>> Otherwise,
>> Reviewed-by: Bin Meng <bmeng@tinylab.org>
>
> Thanks!
>
> Alistair, let me know if you want me to send another version with the commit
> message typo fixed. I might as well take the change to rebase it with
> riscv-to-apply.next.

While rebasing these patches on top of riscv-to-apply.next, the avocado tests
I've introduced here started to fail both sifive_u tests:

tests/avocado/riscv_opensbi.py:RiscvOpenSBI.test_riscv32_sifive_u: INTERRUPTED:
Test interrupted by SIGTERM\nRunner error occurred: ... (5.07 s)
  (09/18) tests/avocado/riscv_opensbi.py:RiscvOpenSBI.test_riscv64_sifive_u: INTERRUPTED:
Test interrupted by SIGTERM\nRunner error occurred: ... (5.05 s)


I proposed a fix here:

https://lists.gnu.org/archive/html/qemu-devel/2023-01/msg02035.html

I can re-send this series after we get that problem figure out. Otherwise we're
going to add 2 avocado tests that are failing right from the start hehe.

Thanks,

Daniel


>
>
> Daniel
>

Re: [PATCH v5 01/11] tests/avocado: add RISC-V OpenSBI boot test

[Alistair Francis]

On Mon, Jan 2, 2023 at 9:53 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> This test is used to do a quick sanity check to ensure that we're able
> to run the existing QEMU FW image.
>
> 'sifive_u', 'spike' and 'virt' riscv64 machines, and 'sifive_u' and
> 'virt' 32 bit machines are able to run the default RISCV64_BIOS_BIN |
> RISCV32_BIOS_BIN firmware with minimal options.
>
> The riscv32 'spike' machine isn't bootable at this moment, requiring an
> OpenSBI fix [1] and QEMU side changes [2]. We could just leave at that
> or add a 'skip' test to remind us about it. To work as a reminder that
> we have a riscv32 'spike' test that should be enabled as soon as OpenSBI
> QEMU rom receives the fix, we're adding a 'skip' test:
>
> (06/18) tests/avocado/riscv_opensbi.py:RiscvOpenSBI.test_riscv32_spike:
>         SKIP: requires OpenSBI fix to work
>
> [1] https://patchwork.ozlabs.org/project/opensbi/patch/20221226033603.1860569-1-bmeng@tinylab.org/
> [2] https://patchwork.ozlabs.org/project/qemu-devel/list/?series=334159
>
> Cc: Cleber Rosa <crosa@redhat.com>
> Cc: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Bin Meng <bmeng@tinylab.org>
> Tested-by: Bin Meng <bmeng@tinylab.org>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Acked-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  tests/avocado/riscv_opensbi.py | 65 ++++++++++++++++++++++++++++++++++
>  1 file changed, 65 insertions(+)
>  create mode 100644 tests/avocado/riscv_opensbi.py
>
> diff --git a/tests/avocado/riscv_opensbi.py b/tests/avocado/riscv_opensbi.py
> new file mode 100644
> index 0000000000..e02f0d404a
> --- /dev/null
> +++ b/tests/avocado/riscv_opensbi.py
> @@ -0,0 +1,65 @@
> +# OpenSBI boot test for RISC-V machines
> +#
> +# Copyright (c) 2022, Ventana Micro
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2 or
> +# later.  See the COPYING file in the top-level directory.
> +
> +from avocado_qemu import QemuSystemTest
> +from avocado import skip
> +from avocado_qemu import wait_for_console_pattern
> +
> +class RiscvOpenSBI(QemuSystemTest):
> +    """
> +    :avocado: tags=accel:tcg
> +    """
> +    timeout = 5
> +
> +    def boot_opensbi(self):
> +        self.vm.set_console()
> +        self.vm.launch()
> +        wait_for_console_pattern(self, 'Platform Name')
> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
> +
> +    @skip("requires OpenSBI fix to work")
> +    def test_riscv32_spike(self):
> +        """
> +        :avocado: tags=arch:riscv32
> +        :avocado: tags=machine:spike
> +        """
> +        self.boot_opensbi()
> +
> +    def test_riscv64_spike(self):
> +        """
> +        :avocado: tags=arch:riscv64
> +        :avocado: tags=machine:spike
> +        """
> +        self.boot_opensbi()
> +
> +    def test_riscv32_sifive_u(self):
> +        """
> +        :avocado: tags=arch:riscv32
> +        :avocado: tags=machine:sifive_u
> +        """
> +        self.boot_opensbi()
> +
> +    def test_riscv64_sifive_u(self):
> +        """
> +        :avocado: tags=arch:riscv64
> +        :avocado: tags=machine:sifive_u
> +        """
> +        self.boot_opensbi()
> +
> +    def test_riscv32_virt(self):
> +        """
> +        :avocado: tags=arch:riscv32
> +        :avocado: tags=machine:virt
> +        """
> +        self.boot_opensbi()
> +
> +    def test_riscv64_virt(self):
> +        """
> +        :avocado: tags=arch:riscv64
> +        :avocado: tags=machine:virt
> +        """
> +        self.boot_opensbi()
> --
> 2.39.0
>
>

Re: [PATCH v5 04/11] hw/riscv/boot.c: exit early if filename is NULL in load functions

[Alistair Francis]

On Mon, Jan 2, 2023 at 9:54 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> riscv_load_firmware(), riscv_load_initrd() and riscv_load_kernel() works
> under the assumption that a 'filename' parameter is always not NULL.
>
> This is currently the case since all callers of these functions are
> checking for NULL before calling them. Add an g_assert() to make sure
> that a NULL value in these cases are to be considered a bug.
>
> Suggested-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/riscv/boot.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 98b80af51b..31aa3385a0 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -153,6 +153,8 @@ target_ulong riscv_load_firmware(const char *firmware_filename,
>      uint64_t firmware_entry, firmware_end;
>      ssize_t firmware_size;
>
> +    g_assert(firmware_filename != NULL);
> +
>      if (load_elf_ram_sym(firmware_filename, NULL, NULL, NULL,
>                           &firmware_entry, NULL, &firmware_end, NULL,
>                           0, EM_RISCV, 1, 0, NULL, true, sym_cb) > 0) {
> @@ -177,6 +179,8 @@ target_ulong riscv_load_kernel(const char *kernel_filename,
>  {
>      uint64_t kernel_load_base, kernel_entry;
>
> +    g_assert(kernel_filename != NULL);
> +
>      /*
>       * NB: Use low address not ELF entry point to ensure that the fw_dynamic
>       * behaviour when loading an ELF matches the fw_payload, fw_jump and BBL
> @@ -209,6 +213,8 @@ hwaddr riscv_load_initrd(const char *filename, uint64_t mem_size,
>  {
>      ssize_t size;
>
> +    g_assert(filename != NULL);
> +
>      /*
>       * We want to put the initrd far enough into RAM that when the
>       * kernel is uncompressed it will not clobber the initrd. However
> --
> 2.39.0
>
>

Re: [PATCH v5 06/11] hw/riscv: write initrd 'chosen' FDT inside riscv_load_initrd()

[Alistair Francis]

On Mon, Jan 2, 2023 at 9:54 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> riscv_load_initrd() returns the initrd end addr while also writing a
> 'start' var to mark the addr start. These informations are being used
> just to write the initrd FDT node. Every existing caller of
> riscv_load_initrd() is writing the FDT in the same manner.
>
> We can simplify things by writing the FDT inside riscv_load_initrd(),
> sparing callers from having to manage start/end addrs to write the FDT
> themselves.
>
> An 'if (fdt)' check is already inserted at the end of the function
> because we'll end up using it later on with other boards that doesn´t
> have a FDT.
>
> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Reviewed-by: Bin Meng <bmeng@tinylab.org>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/riscv/boot.c            | 18 ++++++++++++------
>  hw/riscv/microchip_pfsoc.c | 10 ++--------
>  hw/riscv/sifive_u.c        | 10 ++--------
>  hw/riscv/spike.c           | 10 ++--------
>  hw/riscv/virt.c            | 10 ++--------
>  include/hw/riscv/boot.h    |  4 ++--
>  6 files changed, 22 insertions(+), 40 deletions(-)
>
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 31aa3385a0..6b948d1c9e 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -208,9 +208,10 @@ target_ulong riscv_load_kernel(const char *kernel_filename,
>      exit(1);
>  }
>
> -hwaddr riscv_load_initrd(const char *filename, uint64_t mem_size,
> -                         uint64_t kernel_entry, hwaddr *start)
> +void riscv_load_initrd(const char *filename, uint64_t mem_size,
> +                       uint64_t kernel_entry, void *fdt)
>  {
> +    hwaddr start, end;
>      ssize_t size;
>
>      g_assert(filename != NULL);
> @@ -226,18 +227,23 @@ hwaddr riscv_load_initrd(const char *filename, uint64_t mem_size,
>       * 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 = kernel_entry + MIN(mem_size / 2, 128 * MiB);
>
> -    size = load_ramdisk(filename, *start, mem_size - *start);
> +    size = load_ramdisk(filename, start, mem_size - start);
>      if (size == -1) {
> -        size = load_image_targphys(filename, *start, mem_size - *start);
> +        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;
> +    /* Some RISC-V machines (e.g. opentitan) don't have a fdt. */
> +    if (fdt) {
> +        end = start + size;
> +        qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-start", start);
> +        qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-end", end);
> +    }
>  }
>
>  uint64_t riscv_load_fdt(hwaddr dram_base, uint64_t mem_size, void *fdt)
> diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
> index b10321b564..593a799549 100644
> --- a/hw/riscv/microchip_pfsoc.c
> +++ b/hw/riscv/microchip_pfsoc.c
> @@ -633,14 +633,8 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
>                                           kernel_start_addr, NULL);
>
>          if (machine->initrd_filename) {
> -            hwaddr start;
> -            hwaddr end = riscv_load_initrd(machine->initrd_filename,
> -                                           machine->ram_size, kernel_entry,
> -                                           &start);
> -            qemu_fdt_setprop_cell(machine->fdt, "/chosen",
> -                                  "linux,initrd-start", start);
> -            qemu_fdt_setprop_cell(machine->fdt, "/chosen",
> -                                  "linux,initrd-end", end);
> +            riscv_load_initrd(machine->initrd_filename, machine->ram_size,
> +                              kernel_entry, machine->fdt);
>          }
>
>          if (machine->kernel_cmdline && *machine->kernel_cmdline) {
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index ddceb750ea..37f5087172 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -608,14 +608,8 @@ static void sifive_u_machine_init(MachineState *machine)
>                                           kernel_start_addr, NULL);
>
>          if (machine->initrd_filename) {
> -            hwaddr start;
> -            hwaddr end = riscv_load_initrd(machine->initrd_filename,
> -                                           machine->ram_size, kernel_entry,
> -                                           &start);
> -            qemu_fdt_setprop_cell(machine->fdt, "/chosen",
> -                                  "linux,initrd-start", start);
> -            qemu_fdt_setprop_cell(machine->fdt, "/chosen", "linux,initrd-end",
> -                                  end);
> +            riscv_load_initrd(machine->initrd_filename, machine->ram_size,
> +                              kernel_entry, machine->fdt);
>          }
>      } else {
>         /*
> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> index 004dfb2d5b..5668fe0694 100644
> --- a/hw/riscv/spike.c
> +++ b/hw/riscv/spike.c
> @@ -316,14 +316,8 @@ static void spike_board_init(MachineState *machine)
>                                           htif_symbol_callback);
>
>          if (machine->initrd_filename) {
> -            hwaddr start;
> -            hwaddr end = riscv_load_initrd(machine->initrd_filename,
> -                                           machine->ram_size, kernel_entry,
> -                                           &start);
> -            qemu_fdt_setprop_cell(machine->fdt, "/chosen",
> -                                  "linux,initrd-start", start);
> -            qemu_fdt_setprop_cell(machine->fdt, "/chosen", "linux,initrd-end",
> -                                  end);
> +            riscv_load_initrd(machine->initrd_filename, machine->ram_size,
> +                              kernel_entry, machine->fdt);
>          }
>      } else {
>         /*
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 408f7a2256..5967b136b4 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -1291,14 +1291,8 @@ static void virt_machine_done(Notifier *notifier, void *data)
>                                           kernel_start_addr, NULL);
>
>          if (machine->initrd_filename) {
> -            hwaddr start;
> -            hwaddr end = riscv_load_initrd(machine->initrd_filename,
> -                                           machine->ram_size, kernel_entry,
> -                                           &start);
> -            qemu_fdt_setprop_cell(machine->fdt, "/chosen",
> -                                  "linux,initrd-start", start);
> -            qemu_fdt_setprop_cell(machine->fdt, "/chosen", "linux,initrd-end",
> -                                  end);
> +            riscv_load_initrd(machine->initrd_filename, machine->ram_size,
> +                              kernel_entry, machine->fdt);
>          }
>      } else {
>         /*
> diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
> index b273ab22f7..e37e1d1238 100644
> --- a/include/hw/riscv/boot.h
> +++ b/include/hw/riscv/boot.h
> @@ -46,8 +46,8 @@ target_ulong riscv_load_firmware(const char *firmware_filename,
>  target_ulong riscv_load_kernel(const char *kernel_filename,
>                                 target_ulong firmware_end_addr,
>                                 symbol_fn_t sym_cb);
> -hwaddr riscv_load_initrd(const char *filename, uint64_t mem_size,
> -                         uint64_t kernel_entry, hwaddr *start);
> +void riscv_load_initrd(const char *filename, uint64_t mem_size,
> +                       uint64_t kernel_entry, void *fdt);
>  uint64_t riscv_load_fdt(hwaddr dram_start, uint64_t dram_size, void *fdt);
>  void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts,
>                                 hwaddr saddr,
> --
> 2.39.0
>
>

Re: [PATCH v5 07/11] hw/riscv: write bootargs 'chosen' FDT after riscv_load_kernel()

[Alistair Francis]

On Mon, Jan 2, 2023 at 9:55 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> The sifive_u, spike and virt machines are writing the 'bootargs' FDT
> node during their respective create_fdt().
>
> Given that bootargs is written only when '-append' is used, and this
> option is only allowed with the '-kernel' option, which in turn is
> already being check before executing riscv_load_kernel(), write
> 'bootargs' in the same code path as riscv_load_kernel().
>
> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Reviewed-by: Bin Meng <bmeng@tinylab.org>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/riscv/sifive_u.c | 11 +++++------
>  hw/riscv/spike.c    |  9 +++++----
>  hw/riscv/virt.c     | 11 +++++------
>  3 files changed, 15 insertions(+), 16 deletions(-)
>
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index 37f5087172..3e6df87b5b 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -117,7 +117,6 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry *memmap,
>              error_report("load_device_tree() failed");
>              exit(1);
>          }
> -        goto update_bootargs;
>      } else {
>          fdt = ms->fdt = create_device_tree(&fdt_size);
>          if (!fdt) {
> @@ -510,11 +509,6 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry *memmap,
>      qemu_fdt_setprop_string(fdt, "/aliases", "serial0", nodename);
>
>      g_free(nodename);
> -
> -update_bootargs:
> -    if (cmdline && *cmdline) {
> -        qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline);
> -    }
>  }
>
>  static void sifive_u_machine_reset(void *opaque, int n, int level)
> @@ -611,6 +605,11 @@ static void sifive_u_machine_init(MachineState *machine)
>              riscv_load_initrd(machine->initrd_filename, machine->ram_size,
>                                kernel_entry, machine->fdt);
>          }
> +
> +        if (machine->kernel_cmdline && *machine->kernel_cmdline) {
> +            qemu_fdt_setprop_string(machine->fdt, "/chosen", "bootargs",
> +                                    machine->kernel_cmdline);
> +        }
>      } else {
>         /*
>          * If dynamic firmware is used, it doesn't know where is the next mode
> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> index 5668fe0694..60e2912be5 100644
> --- a/hw/riscv/spike.c
> +++ b/hw/riscv/spike.c
> @@ -179,10 +179,6 @@ static void create_fdt(SpikeState *s, const MemMapEntry *memmap,
>
>      qemu_fdt_add_subnode(fdt, "/chosen");
>      qemu_fdt_setprop_string(fdt, "/chosen", "stdout-path", "/htif");
> -
> -    if (cmdline && *cmdline) {
> -        qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline);
> -    }
>  }
>
>  static bool spike_test_elf_image(char *filename)
> @@ -319,6 +315,11 @@ static void spike_board_init(MachineState *machine)
>              riscv_load_initrd(machine->initrd_filename, machine->ram_size,
>                                kernel_entry, machine->fdt);
>          }
> +
> +        if (machine->kernel_cmdline && *machine->kernel_cmdline) {
> +            qemu_fdt_setprop_string(machine->fdt, "/chosen", "bootargs",
> +                                    machine->kernel_cmdline);
> +        }
>      } else {
>         /*
>          * If dynamic firmware is used, it doesn't know where is the next mode
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 5967b136b4..6c946b6def 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -1012,7 +1012,6 @@ static void create_fdt(RISCVVirtState *s, const MemMapEntry *memmap,
>              error_report("load_device_tree() failed");
>              exit(1);
>          }
> -        goto update_bootargs;
>      } else {
>          mc->fdt = create_device_tree(&s->fdt_size);
>          if (!mc->fdt) {
> @@ -1050,11 +1049,6 @@ static void create_fdt(RISCVVirtState *s, const MemMapEntry *memmap,
>      create_fdt_fw_cfg(s, memmap);
>      create_fdt_pmu(s);
>
> -update_bootargs:
> -    if (cmdline && *cmdline) {
> -        qemu_fdt_setprop_string(mc->fdt, "/chosen", "bootargs", cmdline);
> -    }
> -
>      /* Pass seed to RNG */
>      qemu_guest_getrandom_nofail(rng_seed, sizeof(rng_seed));
>      qemu_fdt_setprop(mc->fdt, "/chosen", "rng-seed", rng_seed, sizeof(rng_seed));
> @@ -1294,6 +1288,11 @@ static void virt_machine_done(Notifier *notifier, void *data)
>              riscv_load_initrd(machine->initrd_filename, machine->ram_size,
>                                kernel_entry, machine->fdt);
>          }
> +
> +        if (machine->kernel_cmdline && *machine->kernel_cmdline) {
> +            qemu_fdt_setprop_string(machine->fdt, "/chosen", "bootargs",
> +                                    machine->kernel_cmdline);
> +        }
>      } else {
>         /*
>          * If dynamic firmware is used, it doesn't know where is the next mode
> --
> 2.39.0
>
>

Re: [PATCH v5 08/11] hw/riscv/boot.c: use MachineState in riscv_load_initrd()

[Alistair Francis]

On Mon, Jan 2, 2023 at 9:55 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> 'filename', 'mem_size' and 'fdt' from riscv_load_initrd() can all be
> retrieved by the MachineState object for all callers.
>
> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Bin Meng <bmeng@tinylab.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/riscv/boot.c            | 6 ++++--
>  hw/riscv/microchip_pfsoc.c | 3 +--
>  hw/riscv/sifive_u.c        | 3 +--
>  hw/riscv/spike.c           | 3 +--
>  hw/riscv/virt.c            | 3 +--
>  include/hw/riscv/boot.h    | 3 +--
>  6 files changed, 9 insertions(+), 12 deletions(-)
>
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 6b948d1c9e..d3e780c3b6 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -208,9 +208,11 @@ target_ulong riscv_load_kernel(const char *kernel_filename,
>      exit(1);
>  }
>
> -void riscv_load_initrd(const char *filename, uint64_t mem_size,
> -                       uint64_t kernel_entry, void *fdt)
> +void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
>  {
> +    const char *filename = machine->initrd_filename;
> +    uint64_t mem_size = machine->ram_size;
> +    void *fdt = machine->fdt;
>      hwaddr start, end;
>      ssize_t size;
>
> diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
> index 593a799549..1e9b0a420e 100644
> --- a/hw/riscv/microchip_pfsoc.c
> +++ b/hw/riscv/microchip_pfsoc.c
> @@ -633,8 +633,7 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
>                                           kernel_start_addr, NULL);
>
>          if (machine->initrd_filename) {
> -            riscv_load_initrd(machine->initrd_filename, machine->ram_size,
> -                              kernel_entry, machine->fdt);
> +            riscv_load_initrd(machine, kernel_entry);
>          }
>
>          if (machine->kernel_cmdline && *machine->kernel_cmdline) {
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index 3e6df87b5b..c40885ed5c 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -602,8 +602,7 @@ static void sifive_u_machine_init(MachineState *machine)
>                                           kernel_start_addr, NULL);
>
>          if (machine->initrd_filename) {
> -            riscv_load_initrd(machine->initrd_filename, machine->ram_size,
> -                              kernel_entry, machine->fdt);
> +            riscv_load_initrd(machine, kernel_entry);
>          }
>
>          if (machine->kernel_cmdline && *machine->kernel_cmdline) {
> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> index 60e2912be5..99dec74fe8 100644
> --- a/hw/riscv/spike.c
> +++ b/hw/riscv/spike.c
> @@ -312,8 +312,7 @@ static void spike_board_init(MachineState *machine)
>                                           htif_symbol_callback);
>
>          if (machine->initrd_filename) {
> -            riscv_load_initrd(machine->initrd_filename, machine->ram_size,
> -                              kernel_entry, machine->fdt);
> +            riscv_load_initrd(machine, kernel_entry);
>          }
>
>          if (machine->kernel_cmdline && *machine->kernel_cmdline) {
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 6c946b6def..02f1369843 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -1285,8 +1285,7 @@ static void virt_machine_done(Notifier *notifier, void *data)
>                                           kernel_start_addr, NULL);
>
>          if (machine->initrd_filename) {
> -            riscv_load_initrd(machine->initrd_filename, machine->ram_size,
> -                              kernel_entry, machine->fdt);
> +            riscv_load_initrd(machine, kernel_entry);
>          }
>
>          if (machine->kernel_cmdline && *machine->kernel_cmdline) {
> diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
> index e37e1d1238..cfd72ecabf 100644
> --- a/include/hw/riscv/boot.h
> +++ b/include/hw/riscv/boot.h
> @@ -46,8 +46,7 @@ target_ulong riscv_load_firmware(const char *firmware_filename,
>  target_ulong riscv_load_kernel(const char *kernel_filename,
>                                 target_ulong firmware_end_addr,
>                                 symbol_fn_t sym_cb);
> -void riscv_load_initrd(const char *filename, uint64_t mem_size,
> -                       uint64_t kernel_entry, void *fdt);
> +void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry);
>  uint64_t riscv_load_fdt(hwaddr dram_start, uint64_t dram_size, void *fdt);
>  void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts,
>                                 hwaddr saddr,
> --
> 2.39.0
>
>

Re: [PATCH v5 09/11] hw/riscv/boot.c: use MachineState in riscv_load_kernel()

[Alistair Francis]

On Mon, Jan 2, 2023 at 9:55 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> All callers are using kernel_filename as machine->kernel_filename.
>
> This will also simplify the changes in riscv_load_kernel() that we're
> going to do next.
>
> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Bin Meng <bmeng@tinylab.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/riscv/boot.c            | 3 ++-
>  hw/riscv/microchip_pfsoc.c | 3 +--
>  hw/riscv/opentitan.c       | 3 +--
>  hw/riscv/sifive_e.c        | 3 +--
>  hw/riscv/sifive_u.c        | 3 +--
>  hw/riscv/spike.c           | 3 +--
>  hw/riscv/virt.c            | 3 +--
>  include/hw/riscv/boot.h    | 2 +-
>  8 files changed, 9 insertions(+), 14 deletions(-)
>
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index d3e780c3b6..2594276223 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -173,10 +173,11 @@ target_ulong riscv_load_firmware(const char *firmware_filename,
>      exit(1);
>  }
>
> -target_ulong riscv_load_kernel(const char *kernel_filename,
> +target_ulong riscv_load_kernel(MachineState *machine,
>                                 target_ulong kernel_start_addr,
>                                 symbol_fn_t sym_cb)
>  {
> +    const char *kernel_filename = machine->kernel_filename;
>      uint64_t kernel_load_base, kernel_entry;
>
>      g_assert(kernel_filename != NULL);
> diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
> index 1e9b0a420e..82ae5e7023 100644
> --- a/hw/riscv/microchip_pfsoc.c
> +++ b/hw/riscv/microchip_pfsoc.c
> @@ -629,8 +629,7 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
>          kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc.u_cpus,
>                                                           firmware_end_addr);
>
> -        kernel_entry = riscv_load_kernel(machine->kernel_filename,
> -                                         kernel_start_addr, NULL);
> +        kernel_entry = riscv_load_kernel(machine, kernel_start_addr, NULL);
>
>          if (machine->initrd_filename) {
>              riscv_load_initrd(machine, kernel_entry);
> diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c
> index 85ffdac5be..64d5d435b9 100644
> --- a/hw/riscv/opentitan.c
> +++ b/hw/riscv/opentitan.c
> @@ -101,8 +101,7 @@ static void opentitan_board_init(MachineState *machine)
>      }
>
>      if (machine->kernel_filename) {
> -        riscv_load_kernel(machine->kernel_filename,
> -                          memmap[IBEX_DEV_RAM].base, NULL);
> +        riscv_load_kernel(machine, memmap[IBEX_DEV_RAM].base, NULL);
>      }
>  }
>
> diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
> index d65d2fd869..3e3f4b0088 100644
> --- a/hw/riscv/sifive_e.c
> +++ b/hw/riscv/sifive_e.c
> @@ -114,8 +114,7 @@ static void sifive_e_machine_init(MachineState *machine)
>                            memmap[SIFIVE_E_DEV_MROM].base, &address_space_memory);
>
>      if (machine->kernel_filename) {
> -        riscv_load_kernel(machine->kernel_filename,
> -                          memmap[SIFIVE_E_DEV_DTIM].base, NULL);
> +        riscv_load_kernel(machine, memmap[SIFIVE_E_DEV_DTIM].base, NULL);
>      }
>  }
>
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index c40885ed5c..bac394c959 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -598,8 +598,7 @@ static void sifive_u_machine_init(MachineState *machine)
>          kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc.u_cpus,
>                                                           firmware_end_addr);
>
> -        kernel_entry = riscv_load_kernel(machine->kernel_filename,
> -                                         kernel_start_addr, NULL);
> +        kernel_entry = riscv_load_kernel(machine, kernel_start_addr, NULL);
>
>          if (machine->initrd_filename) {
>              riscv_load_initrd(machine, kernel_entry);
> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> index 99dec74fe8..bff9475686 100644
> --- a/hw/riscv/spike.c
> +++ b/hw/riscv/spike.c
> @@ -307,8 +307,7 @@ static void spike_board_init(MachineState *machine)
>          kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc[0],
>                                                           firmware_end_addr);
>
> -        kernel_entry = riscv_load_kernel(machine->kernel_filename,
> -                                         kernel_start_addr,
> +        kernel_entry = riscv_load_kernel(machine, kernel_start_addr,
>                                           htif_symbol_callback);
>
>          if (machine->initrd_filename) {
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 02f1369843..c8e35f861e 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -1281,8 +1281,7 @@ static void virt_machine_done(Notifier *notifier, void *data)
>          kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc[0],
>                                                           firmware_end_addr);
>
> -        kernel_entry = riscv_load_kernel(machine->kernel_filename,
> -                                         kernel_start_addr, NULL);
> +        kernel_entry = riscv_load_kernel(machine, kernel_start_addr, NULL);
>
>          if (machine->initrd_filename) {
>              riscv_load_initrd(machine, kernel_entry);
> diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
> index cfd72ecabf..f94653a09b 100644
> --- a/include/hw/riscv/boot.h
> +++ b/include/hw/riscv/boot.h
> @@ -43,7 +43,7 @@ char *riscv_find_firmware(const char *firmware_filename,
>  target_ulong riscv_load_firmware(const char *firmware_filename,
>                                   hwaddr firmware_load_addr,
>                                   symbol_fn_t sym_cb);
> -target_ulong riscv_load_kernel(const char *kernel_filename,
> +target_ulong riscv_load_kernel(MachineState *machine,
>                                 target_ulong firmware_end_addr,
>                                 symbol_fn_t sym_cb);
>  void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry);
> --
> 2.39.0
>
>

Re: [PATCH v5 11/11] hw/riscv/boot.c: make riscv_load_initrd() static

[Alistair Francis]

On Mon, Jan 2, 2023 at 9:57 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> The only remaining caller is riscv_load_kernel_and_initrd() which
> belongs to the same file.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Bin Meng <bmeng@tinylab.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/riscv/boot.c         | 80 ++++++++++++++++++++---------------------
>  include/hw/riscv/boot.h |  1 -
>  2 files changed, 40 insertions(+), 41 deletions(-)
>
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 4888d5c1e0..e868fb6ade 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -173,6 +173,46 @@ target_ulong riscv_load_firmware(const char *firmware_filename,
>      exit(1);
>  }
>
> +static void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
> +{
> +    const char *filename = machine->initrd_filename;
> +    uint64_t mem_size = machine->ram_size;
> +    void *fdt = machine->fdt;
> +    hwaddr start, end;
> +    ssize_t size;
> +
> +    g_assert(filename != NULL);
> +
> +    /*
> +     * 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);
> +        }
> +    }
> +
> +    /* Some RISC-V machines (e.g. opentitan) don't have a fdt. */
> +    if (fdt) {
> +        end = start + size;
> +        qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-start", start);
> +        qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-end", end);
> +    }
> +}
> +
>  target_ulong riscv_load_kernel(MachineState *machine,
>                                 target_ulong kernel_start_addr,
>                                 bool load_initrd,
> @@ -225,46 +265,6 @@ out:
>      return kernel_entry;
>  }
>
> -void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
> -{
> -    const char *filename = machine->initrd_filename;
> -    uint64_t mem_size = machine->ram_size;
> -    void *fdt = machine->fdt;
> -    hwaddr start, end;
> -    ssize_t size;
> -
> -    g_assert(filename != NULL);
> -
> -    /*
> -     * 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);
> -        }
> -    }
> -
> -    /* Some RISC-V machines (e.g. opentitan) don't have a fdt. */
> -    if (fdt) {
> -        end = start + size;
> -        qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-start", start);
> -        qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-end", end);
> -    }
> -}
> -
>  uint64_t riscv_load_fdt(hwaddr dram_base, uint64_t mem_size, void *fdt)
>  {
>      uint64_t temp, fdt_addr;
> diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
> index c3de897371..cbd131bad7 100644
> --- a/include/hw/riscv/boot.h
> +++ b/include/hw/riscv/boot.h
> @@ -47,7 +47,6 @@ target_ulong riscv_load_kernel(MachineState *machine,
>                                 target_ulong firmware_end_addr,
>                                 bool load_initrd,
>                                 symbol_fn_t sym_cb);
> -void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry);
>  uint64_t riscv_load_fdt(hwaddr dram_start, uint64_t dram_size, void *fdt);
>  void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts,
>                                 hwaddr saddr,
> --
> 2.39.0
>
>

Re: [PATCH v5 10/11] hw/riscv/boot.c: consolidate all kernel init in riscv_load_kernel()

[Alistair Francis]

On Mon, Jan 2, 2023 at 9:55 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> The microchip_icicle_kit, sifive_u, spike and virt boards are now doing
> the same steps when '-kernel' is used:
>
> - execute load_kernel()
> - load init_rd()
> - write kernel_cmdline
>
> Let's fold everything inside riscv_load_kernel() to avoid code
> repetition. To not change the behavior of boards that aren't calling
> riscv_load_init(), add an 'load_initrd' flag to riscv_load_kernel() and
> allow these boards to opt out from initrd loading.
>
> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/riscv/boot.c            | 22 +++++++++++++++++++---
>  hw/riscv/microchip_pfsoc.c | 12 ++----------
>  hw/riscv/opentitan.c       |  2 +-
>  hw/riscv/sifive_e.c        |  3 ++-
>  hw/riscv/sifive_u.c        | 12 ++----------
>  hw/riscv/spike.c           | 11 +----------
>  hw/riscv/virt.c            | 12 ++----------
>  include/hw/riscv/boot.h    |  1 +
>  8 files changed, 30 insertions(+), 45 deletions(-)
>
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 2594276223..4888d5c1e0 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -175,10 +175,12 @@ target_ulong riscv_load_firmware(const char *firmware_filename,
>
>  target_ulong riscv_load_kernel(MachineState *machine,
>                                 target_ulong kernel_start_addr,
> +                               bool load_initrd,
>                                 symbol_fn_t sym_cb)
>  {
>      const char *kernel_filename = machine->kernel_filename;
>      uint64_t kernel_load_base, kernel_entry;
> +    void *fdt = machine->fdt;
>
>      g_assert(kernel_filename != NULL);
>
> @@ -192,21 +194,35 @@ target_ulong riscv_load_kernel(MachineState *machine,
>      if (load_elf_ram_sym(kernel_filename, NULL, NULL, NULL,
>                           NULL, &kernel_load_base, NULL, NULL, 0,
>                           EM_RISCV, 1, 0, NULL, true, sym_cb) > 0) {
> -        return kernel_load_base;
> +        kernel_entry = kernel_load_base;
> +        goto out;
>      }
>
>      if (load_uimage_as(kernel_filename, &kernel_entry, NULL, NULL,
>                         NULL, NULL, NULL) > 0) {
> -        return kernel_entry;
> +        goto out;
>      }
>
>      if (load_image_targphys_as(kernel_filename, kernel_start_addr,
>                                 current_machine->ram_size, NULL) > 0) {
> -        return kernel_start_addr;
> +        kernel_entry = kernel_start_addr;
> +        goto out;
>      }
>
>      error_report("could not load kernel '%s'", kernel_filename);
>      exit(1);
> +
> +out:
> +    if (load_initrd && machine->initrd_filename) {
> +        riscv_load_initrd(machine, kernel_entry);
> +    }
> +
> +    if (fdt && machine->kernel_cmdline && *machine->kernel_cmdline) {
> +        qemu_fdt_setprop_string(fdt, "/chosen", "bootargs",
> +                                machine->kernel_cmdline);
> +    }
> +
> +    return kernel_entry;
>  }
>
>  void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
> diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
> index 82ae5e7023..c45023a2b1 100644
> --- a/hw/riscv/microchip_pfsoc.c
> +++ b/hw/riscv/microchip_pfsoc.c
> @@ -629,16 +629,8 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
>          kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc.u_cpus,
>                                                           firmware_end_addr);
>
> -        kernel_entry = riscv_load_kernel(machine, kernel_start_addr, NULL);
> -
> -        if (machine->initrd_filename) {
> -            riscv_load_initrd(machine, kernel_entry);
> -        }
> -
> -        if (machine->kernel_cmdline && *machine->kernel_cmdline) {
> -            qemu_fdt_setprop_string(machine->fdt, "/chosen",
> -                                    "bootargs", machine->kernel_cmdline);
> -        }
> +        kernel_entry = riscv_load_kernel(machine, kernel_start_addr,
> +                                         true, NULL);
>
>          /* Compute the fdt load address in dram */
>          fdt_load_addr = riscv_load_fdt(memmap[MICROCHIP_PFSOC_DRAM_LO].base,
> diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c
> index 64d5d435b9..f6fd9725a5 100644
> --- a/hw/riscv/opentitan.c
> +++ b/hw/riscv/opentitan.c
> @@ -101,7 +101,7 @@ static void opentitan_board_init(MachineState *machine)
>      }
>
>      if (machine->kernel_filename) {
> -        riscv_load_kernel(machine, memmap[IBEX_DEV_RAM].base, NULL);
> +        riscv_load_kernel(machine, memmap[IBEX_DEV_RAM].base, false, NULL);
>      }
>  }
>
> diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
> index 3e3f4b0088..6835d1c807 100644
> --- a/hw/riscv/sifive_e.c
> +++ b/hw/riscv/sifive_e.c
> @@ -114,7 +114,8 @@ static void sifive_e_machine_init(MachineState *machine)
>                            memmap[SIFIVE_E_DEV_MROM].base, &address_space_memory);
>
>      if (machine->kernel_filename) {
> -        riscv_load_kernel(machine, memmap[SIFIVE_E_DEV_DTIM].base, NULL);
> +        riscv_load_kernel(machine, memmap[SIFIVE_E_DEV_DTIM].base,
> +                          false, NULL);
>      }
>  }
>
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index bac394c959..9a75d4aa62 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -598,16 +598,8 @@ static void sifive_u_machine_init(MachineState *machine)
>          kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc.u_cpus,
>                                                           firmware_end_addr);
>
> -        kernel_entry = riscv_load_kernel(machine, kernel_start_addr, NULL);
> -
> -        if (machine->initrd_filename) {
> -            riscv_load_initrd(machine, kernel_entry);
> -        }
> -
> -        if (machine->kernel_cmdline && *machine->kernel_cmdline) {
> -            qemu_fdt_setprop_string(machine->fdt, "/chosen", "bootargs",
> -                                    machine->kernel_cmdline);
> -        }
> +        kernel_entry = riscv_load_kernel(machine, kernel_start_addr,
> +                                         true, NULL);
>      } else {
>         /*
>          * If dynamic firmware is used, it doesn't know where is the next mode
> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> index bff9475686..c517885e6e 100644
> --- a/hw/riscv/spike.c
> +++ b/hw/riscv/spike.c
> @@ -308,16 +308,7 @@ static void spike_board_init(MachineState *machine)
>                                                           firmware_end_addr);
>
>          kernel_entry = riscv_load_kernel(machine, kernel_start_addr,
> -                                         htif_symbol_callback);
> -
> -        if (machine->initrd_filename) {
> -            riscv_load_initrd(machine, kernel_entry);
> -        }
> -
> -        if (machine->kernel_cmdline && *machine->kernel_cmdline) {
> -            qemu_fdt_setprop_string(machine->fdt, "/chosen", "bootargs",
> -                                    machine->kernel_cmdline);
> -        }
> +                                         true, htif_symbol_callback);
>      } else {
>         /*
>          * If dynamic firmware is used, it doesn't know where is the next mode
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index c8e35f861e..a931ed05ab 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -1281,16 +1281,8 @@ static void virt_machine_done(Notifier *notifier, void *data)
>          kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc[0],
>                                                           firmware_end_addr);
>
> -        kernel_entry = riscv_load_kernel(machine, kernel_start_addr, NULL);
> -
> -        if (machine->initrd_filename) {
> -            riscv_load_initrd(machine, kernel_entry);
> -        }
> -
> -        if (machine->kernel_cmdline && *machine->kernel_cmdline) {
> -            qemu_fdt_setprop_string(machine->fdt, "/chosen", "bootargs",
> -                                    machine->kernel_cmdline);
> -        }
> +        kernel_entry = riscv_load_kernel(machine, kernel_start_addr,
> +                                         true, NULL);
>      } else {
>         /*
>          * If dynamic firmware is used, it doesn't know where is the next mode
> diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
> index f94653a09b..c3de897371 100644
> --- a/include/hw/riscv/boot.h
> +++ b/include/hw/riscv/boot.h
> @@ -45,6 +45,7 @@ target_ulong riscv_load_firmware(const char *firmware_filename,
>                                   symbol_fn_t sym_cb);
>  target_ulong riscv_load_kernel(MachineState *machine,
>                                 target_ulong firmware_end_addr,
> +                               bool load_initrd,
>                                 symbol_fn_t sym_cb);
>  void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry);
>  uint64_t riscv_load_fdt(hwaddr dram_start, uint64_t dram_size, void *fdt);
> --
> 2.39.0
>
>

Re: [PATCH v5 10/11] hw/riscv/boot.c: consolidate all kernel init in riscv_load_kernel()

[Alistair Francis]

On Wed, Jan 11, 2023 at 6:21 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
>
>
> On 1/10/23 08:43, Daniel Henrique Barboza wrote:
> >
> >
> > On 1/8/23 00:33, Bin Meng wrote:
> >> On Mon, Jan 2, 2023 at 7:55 PM Daniel Henrique Barboza
> >> <dbarboza@ventanamicro.com> wrote:
> >>> The microchip_icicle_kit, sifive_u, spike and virt boards are now doing
> >>> the same steps when '-kernel' is used:
> >>>
> >>> - execute load_kernel()
> >>> - load init_rd()
> >>> - write kernel_cmdline
> >>>
> >>> Let's fold everything inside riscv_load_kernel() to avoid code
> >>> repetition. To not change the behavior of boards that aren't calling
> >>> riscv_load_init(), add an 'load_initrd' flag to riscv_load_kernel() and
> >> typo: should be riscv_load_initrd()
> >>
> >>> allow these boards to opt out from initrd loading.
> >>>
> >>> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> >>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> >>> ---
> >>>   hw/riscv/boot.c            | 22 +++++++++++++++++++---
> >>>   hw/riscv/microchip_pfsoc.c | 12 ++----------
> >>>   hw/riscv/opentitan.c       |  2 +-
> >>>   hw/riscv/sifive_e.c        |  3 ++-
> >>>   hw/riscv/sifive_u.c        | 12 ++----------
> >>>   hw/riscv/spike.c           | 11 +----------
> >>>   hw/riscv/virt.c            | 12 ++----------
> >>>   include/hw/riscv/boot.h    |  1 +
> >>>   8 files changed, 30 insertions(+), 45 deletions(-)
> >>>
> >> Otherwise,
> >> Reviewed-by: Bin Meng <bmeng@tinylab.org>
> >
> > Thanks!
> >
> > Alistair, let me know if you want me to send another version with the commit
> > message typo fixed. I might as well take the change to rebase it with
> > riscv-to-apply.next.
>
> While rebasing these patches on top of riscv-to-apply.next, the avocado tests
> I've introduced here started to fail both sifive_u tests:
>
> tests/avocado/riscv_opensbi.py:RiscvOpenSBI.test_riscv32_sifive_u: INTERRUPTED:
> Test interrupted by SIGTERM\nRunner error occurred: ... (5.07 s)
>   (09/18) tests/avocado/riscv_opensbi.py:RiscvOpenSBI.test_riscv64_sifive_u: INTERRUPTED:
> Test interrupted by SIGTERM\nRunner error occurred: ... (5.05 s)
>
>
> I proposed a fix here:
>
> https://lists.gnu.org/archive/html/qemu-devel/2023-01/msg02035.html

Thanks!

I generally push riscv-to-apply.next before running tests, so it's
possible to break. I'm seeing similar failures.

Generally when I see failures from a series I just drop the series,
but if you have a fix that's even better :)

Alistair

>
> I can re-send this series after we get that problem figure out. Otherwise we're
> going to add 2 avocado tests that are failing right from the start hehe.
>
> Thanks,
>
> Daniel
>
>
> >
> >
> > Daniel
> >
>
>

Re: [PATCH v5 00/11] riscv: OpenSBI boot test and cleanups

[Alistair Francis]

On Mon, Jan 2, 2023 at 9:54 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Hi,
>
> This new version is still rebased on top of [1]:
>
> "[PATCH v2 00/12] hw/riscv: Improve Spike HTIF emulation fidelity"
>
> from Bin Meng.
>
> The change from v4 is on patch 9 where we added an extra flag in
> riscv_load_kernel() to allow for boards that don't load initrd
> (e.g. opentitan and sifive_e) to opt out from loading it altogether.
>
> * Patch without reviews: 9
>
> Changes from v4:
> - patch 9:
>   - added a 'load_init' flag in riscv_load_kernel() to control whether
>     the function should execute riscv_load_initrd() or not
> v4 link: https://lists.gnu.org/archive/html/qemu-devel/2022-12/msg04652.html
>
> Changes from v3:
> - patch 1:
>   - fixed more instances of 'opensbi' and 'Opensbi' to 'OpenSBI'
>   - changed tests order
> - patch 4 (new):
>   - added a g_assert(filename) guard in riscv_load_initrd() and
>     riscv_load_kernel()
> v3 link: https://mail.gnu.org/archive/html/qemu-devel/2022-12/msg04491.html
>
> Changes from v2:
> - patch 1:
>   - reduced code repetition with a boot_opensbi() helper
>   - renamed 'opensbi' to 'OpenSBI' in the file header
> - patch 9:
>   - renamed riscv_load_kernel() to riscv_load_kernel_and_initrd()
> v2 link: https://mail.gnu.org/archive/html/qemu-devel/2022-12/msg04466.html
>
>
> Changes from v1:
> - patches were rebased with [1]
> - patches 13-15: removed
>   * will be re-sent in a follow-up series
> - patches 4-5: removed since they're picked by Bin in [1]
> - patch 1:
>   - added a 'skip' riscv32 spike test
> v1 link: https://mail.gnu.org/archive/html/qemu-devel/2022-12/msg03860.html
>
>
> Based-on: <20221227064812.1903326-1-bmeng@tinylab.org>
>
> Cc: Alistair Francis <alistair.francis@wdc.com>
> Cc: Bin Meng <bin.meng@windriver.com>
>
> [1] https://patchwork.ozlabs.org/project/qemu-devel/list/?series=334352
>
> Daniel Henrique Barboza (11):
>   tests/avocado: add RISC-V OpenSBI boot test
>   hw/riscv/spike: use 'fdt' from MachineState
>   hw/riscv/sifive_u: use 'fdt' from MachineState
>   hw/riscv/boot.c: exit early if filename is NULL in load functions
>   hw/riscv/spike.c: load initrd right after riscv_load_kernel()
>   hw/riscv: write initrd 'chosen' FDT inside riscv_load_initrd()
>   hw/riscv: write bootargs 'chosen' FDT after riscv_load_kernel()
>   hw/riscv/boot.c: use MachineState in riscv_load_initrd()
>   hw/riscv/boot.c: use MachineState in riscv_load_kernel()
>   hw/riscv/boot.c: consolidate all kernel init in riscv_load_kernel()
>   hw/riscv/boot.c: make riscv_load_initrd() static

Thanks!

Applied to riscv-to-apply.next

Alistair

>
>  hw/riscv/boot.c                | 91 +++++++++++++++++++++++-----------
>  hw/riscv/microchip_pfsoc.c     | 20 +-------
>  hw/riscv/opentitan.c           |  3 +-
>  hw/riscv/sifive_e.c            |  4 +-
>  hw/riscv/sifive_u.c            | 32 +++---------
>  hw/riscv/spike.c               | 37 ++++----------
>  hw/riscv/virt.c                | 21 +-------
>  include/hw/riscv/boot.h        |  5 +-
>  include/hw/riscv/sifive_u.h    |  3 --
>  include/hw/riscv/spike.h       |  2 -
>  tests/avocado/riscv_opensbi.py | 65 ++++++++++++++++++++++++
>  11 files changed, 150 insertions(+), 133 deletions(-)
>  create mode 100644 tests/avocado/riscv_opensbi.py
>
> --
> 2.39.0
>
>

Re: [PATCH v5 10/11] hw/riscv/boot.c: consolidate all kernel init in riscv_load_kernel()

[Alistair Francis]

On Mon, Jan 2, 2023 at 9:55 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> The microchip_icicle_kit, sifive_u, spike and virt boards are now doing
> the same steps when '-kernel' is used:
>
> - execute load_kernel()
> - load init_rd()
> - write kernel_cmdline
>
> Let's fold everything inside riscv_load_kernel() to avoid code
> repetition. To not change the behavior of boards that aren't calling
> riscv_load_init(), add an 'load_initrd' flag to riscv_load_kernel() and
> allow these boards to opt out from initrd loading.
>
> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  hw/riscv/boot.c            | 22 +++++++++++++++++++---
>  hw/riscv/microchip_pfsoc.c | 12 ++----------
>  hw/riscv/opentitan.c       |  2 +-
>  hw/riscv/sifive_e.c        |  3 ++-
>  hw/riscv/sifive_u.c        | 12 ++----------
>  hw/riscv/spike.c           | 11 +----------
>  hw/riscv/virt.c            | 12 ++----------
>  include/hw/riscv/boot.h    |  1 +
>  8 files changed, 30 insertions(+), 45 deletions(-)
>
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 2594276223..4888d5c1e0 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -175,10 +175,12 @@ target_ulong riscv_load_firmware(const char *firmware_filename,
>
>  target_ulong riscv_load_kernel(MachineState *machine,
>                                 target_ulong kernel_start_addr,
> +                               bool load_initrd,
>                                 symbol_fn_t sym_cb)
>  {
>      const char *kernel_filename = machine->kernel_filename;
>      uint64_t kernel_load_base, kernel_entry;
> +    void *fdt = machine->fdt;
>
>      g_assert(kernel_filename != NULL);
>
> @@ -192,21 +194,35 @@ target_ulong riscv_load_kernel(MachineState *machine,
>      if (load_elf_ram_sym(kernel_filename, NULL, NULL, NULL,
>                           NULL, &kernel_load_base, NULL, NULL, 0,
>                           EM_RISCV, 1, 0, NULL, true, sym_cb) > 0) {
> -        return kernel_load_base;
> +        kernel_entry = kernel_load_base;

This breaks 32-bit Xvisor loading. It seems that for the 32-bit guest
we get a value of 0xffffffff80000000.

Previously the top bits would be lost as we return a target_ulong from
this function, but with this change we pass the value
0xffffffff80000000 to riscv_load_initrd() which causes failures.

This diff fixes the failure for me

diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index 4888d5c1e0..f08ed44b97 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -194,7 +194,7 @@ target_ulong riscv_load_kernel(MachineState *machine,
    if (load_elf_ram_sym(kernel_filename, NULL, NULL, NULL,
                         NULL, &kernel_load_base, NULL, NULL, 0,
                         EM_RISCV, 1, 0, NULL, true, sym_cb) > 0) {
-        kernel_entry = kernel_load_base;
+        kernel_entry = (target_ulong) kernel_load_base;
        goto out;
    }


but I don't think that's the right fix. We should instead look at the
CPU XLEN and drop the high bits if required.

I'm going to drop this patch, do you mind looking into a proper fix?

Alistair

Re: [PATCH v5 10/11] hw/riscv/boot.c: consolidate all kernel init in riscv_load_kernel()

[Daniel Henrique Barboza]

On 1/11/23 21:34, Alistair Francis wrote:
> On Mon, Jan 2, 2023 at 9:55 PM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
>> The microchip_icicle_kit, sifive_u, spike and virt boards are now doing
>> the same steps when '-kernel' is used:
>>
>> - execute load_kernel()
>> - load init_rd()
>> - write kernel_cmdline
>>
>> Let's fold everything inside riscv_load_kernel() to avoid code
>> repetition. To not change the behavior of boards that aren't calling
>> riscv_load_init(), add an 'load_initrd' flag to riscv_load_kernel() and
>> allow these boards to opt out from initrd loading.
>>
>> Cc: Palmer Dabbelt <palmer@dabbelt.com>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>>   hw/riscv/boot.c            | 22 +++++++++++++++++++---
>>   hw/riscv/microchip_pfsoc.c | 12 ++----------
>>   hw/riscv/opentitan.c       |  2 +-
>>   hw/riscv/sifive_e.c        |  3 ++-
>>   hw/riscv/sifive_u.c        | 12 ++----------
>>   hw/riscv/spike.c           | 11 +----------
>>   hw/riscv/virt.c            | 12 ++----------
>>   include/hw/riscv/boot.h    |  1 +
>>   8 files changed, 30 insertions(+), 45 deletions(-)
>>
>> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
>> index 2594276223..4888d5c1e0 100644
>> --- a/hw/riscv/boot.c
>> +++ b/hw/riscv/boot.c
>> @@ -175,10 +175,12 @@ target_ulong riscv_load_firmware(const char *firmware_filename,
>>
>>   target_ulong riscv_load_kernel(MachineState *machine,
>>                                  target_ulong kernel_start_addr,
>> +                               bool load_initrd,
>>                                  symbol_fn_t sym_cb)
>>   {
>>       const char *kernel_filename = machine->kernel_filename;
>>       uint64_t kernel_load_base, kernel_entry;
>> +    void *fdt = machine->fdt;
>>
>>       g_assert(kernel_filename != NULL);
>>
>> @@ -192,21 +194,35 @@ target_ulong riscv_load_kernel(MachineState *machine,
>>       if (load_elf_ram_sym(kernel_filename, NULL, NULL, NULL,
>>                            NULL, &kernel_load_base, NULL, NULL, 0,
>>                            EM_RISCV, 1, 0, NULL, true, sym_cb) > 0) {
>> -        return kernel_load_base;
>> +        kernel_entry = kernel_load_base;
> This breaks 32-bit Xvisor loading. It seems that for the 32-bit guest
> we get a value of 0xffffffff80000000.
>
> Previously the top bits would be lost as we return a target_ulong from
> this function, but with this change we pass the value
> 0xffffffff80000000 to riscv_load_initrd() which causes failures.
>
> This diff fixes the failure for me
>
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 4888d5c1e0..f08ed44b97 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -194,7 +194,7 @@ target_ulong riscv_load_kernel(MachineState *machine,
>      if (load_elf_ram_sym(kernel_filename, NULL, NULL, NULL,
>                           NULL, &kernel_load_base, NULL, NULL, 0,
>                           EM_RISCV, 1, 0, NULL, true, sym_cb) > 0) {
> -        kernel_entry = kernel_load_base;
> +        kernel_entry = (target_ulong) kernel_load_base;
>          goto out;
>      }
>
>
> but I don't think that's the right fix. We should instead look at the
> CPU XLEN and drop the high bits if required.
>
> I'm going to drop this patch, do you mind looking into a proper fix?

Thanks for looking this up!

I 'll fix it and resend patches 10 and 11.

Daniel

>
> Alistair

Re: [PATCH v5 10/11] hw/riscv/boot.c: consolidate all kernel init in riscv_load_kernel()

[Bin Meng]

Hi Alistair,

On Thu, Jan 12, 2023 at 8:36 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Mon, Jan 2, 2023 at 9:55 PM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
> >
> > The microchip_icicle_kit, sifive_u, spike and virt boards are now doing
> > the same steps when '-kernel' is used:
> >
> > - execute load_kernel()
> > - load init_rd()
> > - write kernel_cmdline
> >
> > Let's fold everything inside riscv_load_kernel() to avoid code
> > repetition. To not change the behavior of boards that aren't calling
> > riscv_load_init(), add an 'load_initrd' flag to riscv_load_kernel() and
> > allow these boards to opt out from initrd loading.
> >
> > Cc: Palmer Dabbelt <palmer@dabbelt.com>
> > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> > ---
> >  hw/riscv/boot.c            | 22 +++++++++++++++++++---
> >  hw/riscv/microchip_pfsoc.c | 12 ++----------
> >  hw/riscv/opentitan.c       |  2 +-
> >  hw/riscv/sifive_e.c        |  3 ++-
> >  hw/riscv/sifive_u.c        | 12 ++----------
> >  hw/riscv/spike.c           | 11 +----------
> >  hw/riscv/virt.c            | 12 ++----------
> >  include/hw/riscv/boot.h    |  1 +
> >  8 files changed, 30 insertions(+), 45 deletions(-)
> >
> > diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> > index 2594276223..4888d5c1e0 100644
> > --- a/hw/riscv/boot.c
> > +++ b/hw/riscv/boot.c
> > @@ -175,10 +175,12 @@ target_ulong riscv_load_firmware(const char *firmware_filename,
> >
> >  target_ulong riscv_load_kernel(MachineState *machine,
> >                                 target_ulong kernel_start_addr,
> > +                               bool load_initrd,
> >                                 symbol_fn_t sym_cb)
> >  {
> >      const char *kernel_filename = machine->kernel_filename;
> >      uint64_t kernel_load_base, kernel_entry;
> > +    void *fdt = machine->fdt;
> >
> >      g_assert(kernel_filename != NULL);
> >
> > @@ -192,21 +194,35 @@ target_ulong riscv_load_kernel(MachineState *machine,
> >      if (load_elf_ram_sym(kernel_filename, NULL, NULL, NULL,
> >                           NULL, &kernel_load_base, NULL, NULL, 0,
> >                           EM_RISCV, 1, 0, NULL, true, sym_cb) > 0) {
> > -        return kernel_load_base;
> > +        kernel_entry = kernel_load_base;
>
> This breaks 32-bit Xvisor loading. It seems that for the 32-bit guest
> we get a value of 0xffffffff80000000.

Shouldn't the bug be the 32-bit Xvisor image? How can a 32-bit image
return an address of 0xffffffff80000000?

>
> Previously the top bits would be lost as we return a target_ulong from
> this function, but with this change we pass the value
> 0xffffffff80000000 to riscv_load_initrd() which causes failures.
>
> This diff fixes the failure for me
>
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 4888d5c1e0..f08ed44b97 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -194,7 +194,7 @@ target_ulong riscv_load_kernel(MachineState *machine,
>     if (load_elf_ram_sym(kernel_filename, NULL, NULL, NULL,
>                          NULL, &kernel_load_base, NULL, NULL, 0,
>                          EM_RISCV, 1, 0, NULL, true, sym_cb) > 0) {
> -        kernel_entry = kernel_load_base;
> +        kernel_entry = (target_ulong) kernel_load_base;
>         goto out;
>     }
>
>
> but I don't think that's the right fix. We should instead look at the
> CPU XLEN and drop the high bits if required.
>
> I'm going to drop this patch, do you mind looking into a proper fix?
>

Regards,
Bin

Re: [PATCH v5 10/11] hw/riscv/boot.c: consolidate all kernel init in riscv_load_kernel()

[Philippe Mathieu-Daudé]

On 12/1/23 01:34, Alistair Francis wrote:
> On Mon, Jan 2, 2023 at 9:55 PM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
>>
>> The microchip_icicle_kit, sifive_u, spike and virt boards are now doing
>> the same steps when '-kernel' is used:
>>
>> - execute load_kernel()
>> - load init_rd()
>> - write kernel_cmdline
>>
>> Let's fold everything inside riscv_load_kernel() to avoid code
>> repetition. To not change the behavior of boards that aren't calling
>> riscv_load_init(), add an 'load_initrd' flag to riscv_load_kernel() and
>> allow these boards to opt out from initrd loading.
>>
>> Cc: Palmer Dabbelt <palmer@dabbelt.com>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>>   hw/riscv/boot.c            | 22 +++++++++++++++++++---
>>   hw/riscv/microchip_pfsoc.c | 12 ++----------
>>   hw/riscv/opentitan.c       |  2 +-
>>   hw/riscv/sifive_e.c        |  3 ++-
>>   hw/riscv/sifive_u.c        | 12 ++----------
>>   hw/riscv/spike.c           | 11 +----------
>>   hw/riscv/virt.c            | 12 ++----------
>>   include/hw/riscv/boot.h    |  1 +
>>   8 files changed, 30 insertions(+), 45 deletions(-)

>> @@ -192,21 +194,35 @@ target_ulong riscv_load_kernel(MachineState *machine,
>>       if (load_elf_ram_sym(kernel_filename, NULL, NULL, NULL,
>>                            NULL, &kernel_load_base, NULL, NULL, 0,
>>                            EM_RISCV, 1, 0, NULL, true, sym_cb) > 0) {
>> -        return kernel_load_base;
>> +        kernel_entry = kernel_load_base;
> 
> This breaks 32-bit Xvisor loading. It seems that for the 32-bit guest
> we get a value of 0xffffffff80000000.
> 
> Previously the top bits would be lost as we return a target_ulong from
> this function, but with this change we pass the value
> 0xffffffff80000000 to riscv_load_initrd() which causes failures.
> 
> This diff fixes the failure for me
> 
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 4888d5c1e0..f08ed44b97 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -194,7 +194,7 @@ target_ulong riscv_load_kernel(MachineState *machine,
>      if (load_elf_ram_sym(kernel_filename, NULL, NULL, NULL,
>                           NULL, &kernel_load_base, NULL, NULL, 0,
>                           EM_RISCV, 1, 0, NULL, true, sym_cb) > 0) {
> -        kernel_entry = kernel_load_base;
> +        kernel_entry = (target_ulong) kernel_load_base;
>          goto out;
>      }
> 
> 
> but I don't think that's the right fix. We should instead look at the
> CPU XLEN and drop the high bits if required.

Ah, that is what should be done in load_elf_ram_sym()'s missing
translate_fn() handler.

Re: [PATCH v5 10/11] hw/riscv/boot.c: consolidate all kernel init in riscv_load_kernel()

[Daniel Henrique Barboza]

On 1/13/23 04:16, Philippe Mathieu-Daudé wrote:
> On 12/1/23 01:34, Alistair Francis wrote:
>> On Mon, Jan 2, 2023 at 9:55 PM Daniel Henrique Barboza
>> <dbarboza@ventanamicro.com> wrote:
>>>
>>> The microchip_icicle_kit, sifive_u, spike and virt boards are now doing
>>> the same steps when '-kernel' is used:
>>>
>>> - execute load_kernel()
>>> - load init_rd()
>>> - write kernel_cmdline
>>>
>>> Let's fold everything inside riscv_load_kernel() to avoid code
>>> repetition. To not change the behavior of boards that aren't calling
>>> riscv_load_init(), add an 'load_initrd' flag to riscv_load_kernel() and
>>> allow these boards to opt out from initrd loading.
>>>
>>> Cc: Palmer Dabbelt <palmer@dabbelt.com>
>>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>>> ---
>>>   hw/riscv/boot.c            | 22 +++++++++++++++++++---
>>>   hw/riscv/microchip_pfsoc.c | 12 ++----------
>>>   hw/riscv/opentitan.c       |  2 +-
>>>   hw/riscv/sifive_e.c        |  3 ++-
>>>   hw/riscv/sifive_u.c        | 12 ++----------
>>>   hw/riscv/spike.c           | 11 +----------
>>>   hw/riscv/virt.c            | 12 ++----------
>>>   include/hw/riscv/boot.h    |  1 +
>>>   8 files changed, 30 insertions(+), 45 deletions(-)
>
>>> @@ -192,21 +194,35 @@ target_ulong riscv_load_kernel(MachineState *machine,
>>>       if (load_elf_ram_sym(kernel_filename, NULL, NULL, NULL,
>>>                            NULL, &kernel_load_base, NULL, NULL, 0,
>>>                            EM_RISCV, 1, 0, NULL, true, sym_cb) > 0) {
>>> -        return kernel_load_base;
>>> +        kernel_entry = kernel_load_base;
>>
>> This breaks 32-bit Xvisor loading. It seems that for the 32-bit guest
>> we get a value of 0xffffffff80000000.
>>
>> Previously the top bits would be lost as we return a target_ulong from
>> this function, but with this change we pass the value
>> 0xffffffff80000000 to riscv_load_initrd() which causes failures.
>>
>> This diff fixes the failure for me
>>
>> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
>> index 4888d5c1e0..f08ed44b97 100644
>> --- a/hw/riscv/boot.c
>> +++ b/hw/riscv/boot.c
>> @@ -194,7 +194,7 @@ target_ulong riscv_load_kernel(MachineState *machine,
>>      if (load_elf_ram_sym(kernel_filename, NULL, NULL, NULL,
>>                           NULL, &kernel_load_base, NULL, NULL, 0,
>>                           EM_RISCV, 1, 0, NULL, true, sym_cb) > 0) {
>> -        kernel_entry = kernel_load_base;
>> +        kernel_entry = (target_ulong) kernel_load_base;
>>          goto out;
>>      }
>>
>>
>> but I don't think that's the right fix. We should instead look at the
>> CPU XLEN and drop the high bits if required.
>
> Ah, that is what should be done in load_elf_ram_sym()'s missing
> translate_fn() handler.

Interesting. I'll try it again and re-send.


Daniel

Re: [PATCH v5 10/11] hw/riscv/boot.c: consolidate all kernel init in riscv_load_kernel()

[Bin Meng]

On Fri, Jan 13, 2023 at 6:23 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
>
>
> On 1/13/23 04:16, Philippe Mathieu-Daudé wrote:
> > On 12/1/23 01:34, Alistair Francis wrote:
> >> On Mon, Jan 2, 2023 at 9:55 PM Daniel Henrique Barboza
> >> <dbarboza@ventanamicro.com> wrote:
> >>>
> >>> The microchip_icicle_kit, sifive_u, spike and virt boards are now doing
> >>> the same steps when '-kernel' is used:
> >>>
> >>> - execute load_kernel()
> >>> - load init_rd()
> >>> - write kernel_cmdline
> >>>
> >>> Let's fold everything inside riscv_load_kernel() to avoid code
> >>> repetition. To not change the behavior of boards that aren't calling
> >>> riscv_load_init(), add an 'load_initrd' flag to riscv_load_kernel() and
> >>> allow these boards to opt out from initrd loading.
> >>>
> >>> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> >>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> >>> ---
> >>>   hw/riscv/boot.c            | 22 +++++++++++++++++++---
> >>>   hw/riscv/microchip_pfsoc.c | 12 ++----------
> >>>   hw/riscv/opentitan.c       |  2 +-
> >>>   hw/riscv/sifive_e.c        |  3 ++-
> >>>   hw/riscv/sifive_u.c        | 12 ++----------
> >>>   hw/riscv/spike.c           | 11 +----------
> >>>   hw/riscv/virt.c            | 12 ++----------
> >>>   include/hw/riscv/boot.h    |  1 +
> >>>   8 files changed, 30 insertions(+), 45 deletions(-)
> >
> >>> @@ -192,21 +194,35 @@ target_ulong riscv_load_kernel(MachineState *machine,
> >>>       if (load_elf_ram_sym(kernel_filename, NULL, NULL, NULL,
> >>>                            NULL, &kernel_load_base, NULL, NULL, 0,
> >>>                            EM_RISCV, 1, 0, NULL, true, sym_cb) > 0) {
> >>> -        return kernel_load_base;
> >>> +        kernel_entry = kernel_load_base;
> >>
> >> This breaks 32-bit Xvisor loading. It seems that for the 32-bit guest
> >> we get a value of 0xffffffff80000000.
> >>
> >> Previously the top bits would be lost as we return a target_ulong from
> >> this function, but with this change we pass the value
> >> 0xffffffff80000000 to riscv_load_initrd() which causes failures.
> >>
> >> This diff fixes the failure for me
> >>
> >> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> >> index 4888d5c1e0..f08ed44b97 100644
> >> --- a/hw/riscv/boot.c
> >> +++ b/hw/riscv/boot.c
> >> @@ -194,7 +194,7 @@ target_ulong riscv_load_kernel(MachineState *machine,
> >>      if (load_elf_ram_sym(kernel_filename, NULL, NULL, NULL,
> >>                           NULL, &kernel_load_base, NULL, NULL, 0,
> >>                           EM_RISCV, 1, 0, NULL, true, sym_cb) > 0) {
> >> -        kernel_entry = kernel_load_base;
> >> +        kernel_entry = (target_ulong) kernel_load_base;
> >>          goto out;
> >>      }
> >>
> >>
> >> but I don't think that's the right fix. We should instead look at the
> >> CPU XLEN and drop the high bits if required.
> >
> > Ah, that is what should be done in load_elf_ram_sym()'s missing
> > translate_fn() handler.
>
> Interesting. I'll try it again and re-send.
>

If that fixes the problem, it should be a separate patch.

I still don't understand why 32-bit xvisor image has a 64-bit address encoded?

Regards,
Bin

Re: [PATCH v5 10/11] hw/riscv/boot.c: consolidate all kernel init in riscv_load_kernel()

[Daniel Henrique Barboza]

On 1/13/23 07:30, Bin Meng wrote:
> On Fri, Jan 13, 2023 at 6:23 PM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
>>
>>
>> On 1/13/23 04:16, Philippe Mathieu-Daudé wrote:
>>> On 12/1/23 01:34, Alistair Francis wrote:
>>>> On Mon, Jan 2, 2023 at 9:55 PM Daniel Henrique Barboza
>>>> <dbarboza@ventanamicro.com> wrote:
>>>>> The microchip_icicle_kit, sifive_u, spike and virt boards are now doing
>>>>> the same steps when '-kernel' is used:
>>>>>
>>>>> - execute load_kernel()
>>>>> - load init_rd()
>>>>> - write kernel_cmdline
>>>>>
>>>>> Let's fold everything inside riscv_load_kernel() to avoid code
>>>>> repetition. To not change the behavior of boards that aren't calling
>>>>> riscv_load_init(), add an 'load_initrd' flag to riscv_load_kernel() and
>>>>> allow these boards to opt out from initrd loading.
>>>>>
>>>>> Cc: Palmer Dabbelt <palmer@dabbelt.com>
>>>>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>>>>> ---
>>>>>    hw/riscv/boot.c            | 22 +++++++++++++++++++---
>>>>>    hw/riscv/microchip_pfsoc.c | 12 ++----------
>>>>>    hw/riscv/opentitan.c       |  2 +-
>>>>>    hw/riscv/sifive_e.c        |  3 ++-
>>>>>    hw/riscv/sifive_u.c        | 12 ++----------
>>>>>    hw/riscv/spike.c           | 11 +----------
>>>>>    hw/riscv/virt.c            | 12 ++----------
>>>>>    include/hw/riscv/boot.h    |  1 +
>>>>>    8 files changed, 30 insertions(+), 45 deletions(-)
>>>>> @@ -192,21 +194,35 @@ target_ulong riscv_load_kernel(MachineState *machine,
>>>>>        if (load_elf_ram_sym(kernel_filename, NULL, NULL, NULL,
>>>>>                             NULL, &kernel_load_base, NULL, NULL, 0,
>>>>>                             EM_RISCV, 1, 0, NULL, true, sym_cb) > 0) {
>>>>> -        return kernel_load_base;
>>>>> +        kernel_entry = kernel_load_base;
>>>> This breaks 32-bit Xvisor loading. It seems that for the 32-bit guest
>>>> we get a value of 0xffffffff80000000.
>>>>
>>>> Previously the top bits would be lost as we return a target_ulong from
>>>> this function, but with this change we pass the value
>>>> 0xffffffff80000000 to riscv_load_initrd() which causes failures.
>>>>
>>>> This diff fixes the failure for me
>>>>
>>>> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
>>>> index 4888d5c1e0..f08ed44b97 100644
>>>> --- a/hw/riscv/boot.c
>>>> +++ b/hw/riscv/boot.c
>>>> @@ -194,7 +194,7 @@ target_ulong riscv_load_kernel(MachineState *machine,
>>>>       if (load_elf_ram_sym(kernel_filename, NULL, NULL, NULL,
>>>>                            NULL, &kernel_load_base, NULL, NULL, 0,
>>>>                            EM_RISCV, 1, 0, NULL, true, sym_cb) > 0) {
>>>> -        kernel_entry = kernel_load_base;
>>>> +        kernel_entry = (target_ulong) kernel_load_base;
>>>>           goto out;
>>>>       }
>>>>
>>>>
>>>> but I don't think that's the right fix. We should instead look at the
>>>> CPU XLEN and drop the high bits if required.
>>> Ah, that is what should be done in load_elf_ram_sym()'s missing
>>> translate_fn() handler.
>> Interesting. I'll try it again and re-send.
>>
> If that fixes the problem, it should be a separate patch.

Fair enough. I'll keep this patch as is and fix it in a separated patch.

Daniel

>
> I still don't understand why 32-bit xvisor image has a 64-bit address encoded?
>
> Regards,
> Bin