[PATCH 00/15] riscv: opensbi boot test and cleanups

[PATCH 00/15] riscv: opensbi boot test and cleanups

[Daniel Henrique Barboza]

Hi,

This series starts by adding a simple Avocado smoke test for RISC-V
machines that uses opensbi. The newly added test is then used to
validate the cleanups made along the way. With this test, running
'make check-avocado' after building all RISC-V targets will run the
test as follows:

 (06/17) tests/avocado/riscv_opensbi.py:RiscvOpensbi.test_riscv64_virt: PASS (0.05 s)
 (07/17) tests/avocado/riscv_opensbi.py:RiscvOpensbi.test_riscv64_spike: PASS (0.04 s)
 (08/17) tests/avocado/riscv_opensbi.py:RiscvOpensbi.test_riscv64_sifive_u: PASS (0.06 s)
 (09/17) tests/avocado/riscv_opensbi.py:RiscvOpensbi.test_riscv32_virt: PASS (0.05 s)
 (10/17) tests/avocado/riscv_opensbi.py:RiscvOpensbi.test_riscv32_sifive_u: PASS (0.06 s)

Note that there are other tests that aren't being run with RISC-V yet.
We'll enable them as needed later on.

After adding this test, our goal is then to reduce boot code repetition
between RISC-V boards and consolidate all boot activities related with
the -kernel option in a single function, riscv_load_kernel().

Aside from allowing all boards to load initrd if -initrd is used (see
patch 11), no other functional changes were intended.

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

Daniel Henrique Barboza (15):
  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: make riscv_find_firmware() static
  hw/riscv/boot.c: introduce riscv_default_firmware_name()
  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/spike.c: simplify create_fdt()
  hw/riscv/virt.c: simplify create_fdt()
  hw/riscv/sifive_u: simplify create_fdt()

 hw/riscv/boot.c                | 137 ++++++++++++++++++++-------------
 hw/riscv/microchip_pfsoc.c     |  19 +----
 hw/riscv/opentitan.c           |   3 +-
 hw/riscv/sifive_e.c            |   3 +-
 hw/riscv/sifive_u.c            |  51 ++++--------
 hw/riscv/spike.c               |  53 ++++---------
 hw/riscv/virt.c                |  38 ++-------
 include/hw/riscv/boot.h        |   6 +-
 include/hw/riscv/sifive_u.h    |   3 -
 include/hw/riscv/spike.h       |   2 -
 tests/avocado/riscv_opensbi.py |  65 ++++++++++++++++
 11 files changed, 189 insertions(+), 191 deletions(-)
 create mode 100644 tests/avocado/riscv_opensbi.py

-- 
2.38.1

[PATCH 01/15] 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.

Cc: Cleber Rosa <crosa@redhat.com>
Cc: Philippe Mathieu-Daudé <philmd@linaro.org>
Cc: Wainer dos Santos Moschetta <wainersm@redhat.com>
Cc: Beraldo Leal <bleal@redhat.com>
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..abc99ced30
--- /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_qemu import wait_for_console_pattern
+
+class RiscvOpensbi(QemuSystemTest):
+    """
+    :avocado: tags=accel:tcg
+    """
+    timeout = 5
+
+    def test_riscv64_virt(self):
+        """
+        :avocado: tags=arch:riscv64
+        :avocado: tags=machine:virt
+        """
+        self.vm.set_console()
+        self.vm.launch()
+        wait_for_console_pattern(self, 'Platform Name')
+        wait_for_console_pattern(self, 'Boot HART MEDELEG')
+
+    def test_riscv64_spike(self):
+        """
+        :avocado: tags=arch:riscv64
+        :avocado: tags=machine:spike
+        """
+        self.vm.set_console()
+        self.vm.launch()
+        wait_for_console_pattern(self, 'Platform Name')
+        wait_for_console_pattern(self, 'Boot HART MEDELEG')
+
+    def test_riscv64_sifive_u(self):
+        """
+        :avocado: tags=arch:riscv64
+        :avocado: tags=machine:sifive_u
+        """
+        self.vm.set_console()
+        self.vm.launch()
+        wait_for_console_pattern(self, 'Platform Name')
+        wait_for_console_pattern(self, 'Boot HART MEDELEG')
+
+    def test_riscv32_virt(self):
+        """
+        :avocado: tags=arch:riscv32
+        :avocado: tags=machine:virt
+        """
+        self.vm.set_console()
+        self.vm.launch()
+        wait_for_console_pattern(self, 'Platform Name')
+        wait_for_console_pattern(self, 'Boot HART MEDELEG')
+
+    def test_riscv32_sifive_u(self):
+        """
+        :avocado: tags=arch:riscv32
+        :avocado: tags=machine:sifive_u
+        """
+        self.vm.set_console()
+        self.vm.launch()
+        wait_for_console_pattern(self, 'Platform Name')
+        wait_for_console_pattern(self, 'Boot HART MEDELEG')
-- 
2.38.1

Re: [PATCH 01/15] tests/avocado: add RISC-V opensbi boot test

[Bin Meng]

On Thu, Dec 22, 2022 at 2:29 AM 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.
>
> Cc: Cleber Rosa <crosa@redhat.com>
> Cc: Philippe Mathieu-Daudé <philmd@linaro.org>
> Cc: Wainer dos Santos Moschetta <wainersm@redhat.com>
> Cc: Beraldo Leal <bleal@redhat.com>
> 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..abc99ced30
> --- /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_qemu import wait_for_console_pattern
> +
> +class RiscvOpensbi(QemuSystemTest):
> +    """
> +    :avocado: tags=accel:tcg
> +    """
> +    timeout = 5
> +
> +    def test_riscv64_virt(self):
> +        """
> +        :avocado: tags=arch:riscv64
> +        :avocado: tags=machine:virt
> +        """
> +        self.vm.set_console()
> +        self.vm.launch()
> +        wait_for_console_pattern(self, 'Platform Name')
> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
> +
> +    def test_riscv64_spike(self):
> +        """
> +        :avocado: tags=arch:riscv64
> +        :avocado: tags=machine:spike
> +        """
> +        self.vm.set_console()
> +        self.vm.launch()
> +        wait_for_console_pattern(self, 'Platform Name')
> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
> +
> +    def test_riscv64_sifive_u(self):
> +        """
> +        :avocado: tags=arch:riscv64
> +        :avocado: tags=machine:sifive_u
> +        """
> +        self.vm.set_console()
> +        self.vm.launch()
> +        wait_for_console_pattern(self, 'Platform Name')
> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
> +
> +    def test_riscv32_virt(self):
> +        """
> +        :avocado: tags=arch:riscv32
> +        :avocado: tags=machine:virt
> +        """
> +        self.vm.set_console()
> +        self.vm.launch()
> +        wait_for_console_pattern(self, 'Platform Name')
> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')

How about testing riscv32_spike too?

> +
> +    def test_riscv32_sifive_u(self):
> +        """
> +        :avocado: tags=arch:riscv32
> +        :avocado: tags=machine:sifive_u
> +        """
> +        self.vm.set_console()
> +        self.vm.launch()
> +        wait_for_console_pattern(self, 'Platform Name')
> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
> --

Regards,
Bin

Re: [PATCH 01/15] tests/avocado: add RISC-V opensbi boot test

[Daniel Henrique Barboza]

On 12/22/22 07:24, Bin Meng wrote:
> On Thu, Dec 22, 2022 at 2:29 AM 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.
>>
>> Cc: Cleber Rosa <crosa@redhat.com>
>> Cc: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Cc: Wainer dos Santos Moschetta <wainersm@redhat.com>
>> Cc: Beraldo Leal <bleal@redhat.com>
>> 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..abc99ced30
>> --- /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_qemu import wait_for_console_pattern
>> +
>> +class RiscvOpensbi(QemuSystemTest):
>> +    """
>> +    :avocado: tags=accel:tcg
>> +    """
>> +    timeout = 5
>> +
>> +    def test_riscv64_virt(self):
>> +        """
>> +        :avocado: tags=arch:riscv64
>> +        :avocado: tags=machine:virt
>> +        """
>> +        self.vm.set_console()
>> +        self.vm.launch()
>> +        wait_for_console_pattern(self, 'Platform Name')
>> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
>> +
>> +    def test_riscv64_spike(self):
>> +        """
>> +        :avocado: tags=arch:riscv64
>> +        :avocado: tags=machine:spike
>> +        """
>> +        self.vm.set_console()
>> +        self.vm.launch()
>> +        wait_for_console_pattern(self, 'Platform Name')
>> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
>> +
>> +    def test_riscv64_sifive_u(self):
>> +        """
>> +        :avocado: tags=arch:riscv64
>> +        :avocado: tags=machine:sifive_u
>> +        """
>> +        self.vm.set_console()
>> +        self.vm.launch()
>> +        wait_for_console_pattern(self, 'Platform Name')
>> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
>> +
>> +    def test_riscv32_virt(self):
>> +        """
>> +        :avocado: tags=arch:riscv32
>> +        :avocado: tags=machine:virt
>> +        """
>> +        self.vm.set_console()
>> +        self.vm.launch()
>> +        wait_for_console_pattern(self, 'Platform Name')
>> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
> How about testing riscv32_spike too?


I didn't manage to make it work. This riscv64 spark command line boots opensbi:


$ ./qemu-system-riscv64 -nographic -display none -vga none -machine spike

OpenSBI v1.1
    ____                    _____ ____ _____
   / __ \                  / ____|  _ \_   _|
  | |  | |_ __   ___ _ __ | (___ | |_) || |
  | |  | | '_ \ / _ \ '_ \ \___ \|  _ < | |
  | |__| | |_) |  __/ | | |____) | |_) || |_
   \____/| .__/ \___|_| |_|_____/|____/_____|
         | |
         |_|

(...)

The same command line doesn't boot riscv32 spark:

./qemu-system-riscv32 -nographic -display none -vga none -machine spike
(--- hangs indefinitely ---)

I debugged it a bit and, as far as boot code goes, it goes all the way and loads the
opensbi 32bit binary.

After that I tried to found any command line example that boots spike with riscv32
bit and didn't find any.  So I gave up digging it further because I became unsure
about whether 32-bit spike works.

If someone can verify that yes, 32-bit spike is supposed to work, then I believe it's
worth investigating why it's not the case ATM.


Thanks,


Daniel

>
>> +
>> +    def test_riscv32_sifive_u(self):
>> +        """
>> +        :avocado: tags=arch:riscv32
>> +        :avocado: tags=machine:sifive_u
>> +        """
>> +        self.vm.set_console()
>> +        self.vm.launch()
>> +        wait_for_console_pattern(self, 'Platform Name')
>> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
>> --
> Regards,
> Bin

Re: [PATCH 01/15] tests/avocado: add RISC-V opensbi boot test

[Bin Meng]

On Thu, Dec 22, 2022 at 6:47 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
>
>
> On 12/22/22 07:24, Bin Meng wrote:
> > On Thu, Dec 22, 2022 at 2:29 AM 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.
> >>
> >> Cc: Cleber Rosa <crosa@redhat.com>
> >> Cc: Philippe Mathieu-Daudé <philmd@linaro.org>
> >> Cc: Wainer dos Santos Moschetta <wainersm@redhat.com>
> >> Cc: Beraldo Leal <bleal@redhat.com>
> >> 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..abc99ced30
> >> --- /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_qemu import wait_for_console_pattern
> >> +
> >> +class RiscvOpensbi(QemuSystemTest):
> >> +    """
> >> +    :avocado: tags=accel:tcg
> >> +    """
> >> +    timeout = 5
> >> +
> >> +    def test_riscv64_virt(self):
> >> +        """
> >> +        :avocado: tags=arch:riscv64
> >> +        :avocado: tags=machine:virt
> >> +        """
> >> +        self.vm.set_console()
> >> +        self.vm.launch()
> >> +        wait_for_console_pattern(self, 'Platform Name')
> >> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
> >> +
> >> +    def test_riscv64_spike(self):
> >> +        """
> >> +        :avocado: tags=arch:riscv64
> >> +        :avocado: tags=machine:spike
> >> +        """
> >> +        self.vm.set_console()
> >> +        self.vm.launch()
> >> +        wait_for_console_pattern(self, 'Platform Name')
> >> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
> >> +
> >> +    def test_riscv64_sifive_u(self):
> >> +        """
> >> +        :avocado: tags=arch:riscv64
> >> +        :avocado: tags=machine:sifive_u
> >> +        """
> >> +        self.vm.set_console()
> >> +        self.vm.launch()
> >> +        wait_for_console_pattern(self, 'Platform Name')
> >> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
> >> +
> >> +    def test_riscv32_virt(self):
> >> +        """
> >> +        :avocado: tags=arch:riscv32
> >> +        :avocado: tags=machine:virt
> >> +        """
> >> +        self.vm.set_console()
> >> +        self.vm.launch()
> >> +        wait_for_console_pattern(self, 'Platform Name')
> >> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
> > How about testing riscv32_spike too?
>
>
> I didn't manage to make it work. This riscv64 spark command line boots opensbi:
>
>
> $ ./qemu-system-riscv64 -nographic -display none -vga none -machine spike
>
> OpenSBI v1.1
>     ____                    _____ ____ _____
>    / __ \                  / ____|  _ \_   _|
>   | |  | |_ __   ___ _ __ | (___ | |_) || |
>   | |  | | '_ \ / _ \ '_ \ \___ \|  _ < | |
>   | |__| | |_) |  __/ | | |____) | |_) || |_
>    \____/| .__/ \___|_| |_|_____/|____/_____|
>          | |
>          |_|
>
> (...)
>
> The same command line doesn't boot riscv32 spark:
>
> ./qemu-system-riscv32 -nographic -display none -vga none -machine spike
> (--- hangs indefinitely ---)
>
> I debugged it a bit and, as far as boot code goes, it goes all the way and loads the
> opensbi 32bit binary.
>
> After that I tried to found any command line example that boots spike with riscv32
> bit and didn't find any.  So I gave up digging it further because I became unsure
> about whether 32-bit spike works.
>
> If someone can verify that yes, 32-bit spike is supposed to work, then I believe it's
> worth investigating why it's not the case ATM.
>

+Anup who might know if QEMU spike 32-bit machine works with opensbi
32-bit generic image.

Regards,
Bin

Re: [PATCH 01/15] tests/avocado: add RISC-V opensbi boot test

[Anup Patel]

On Thu, Dec 22, 2022 at 6:27 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Thu, Dec 22, 2022 at 6:47 PM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
> >
> >
> >
> > On 12/22/22 07:24, Bin Meng wrote:
> > > On Thu, Dec 22, 2022 at 2:29 AM 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.
> > >>
> > >> Cc: Cleber Rosa <crosa@redhat.com>
> > >> Cc: Philippe Mathieu-Daudé <philmd@linaro.org>
> > >> Cc: Wainer dos Santos Moschetta <wainersm@redhat.com>
> > >> Cc: Beraldo Leal <bleal@redhat.com>
> > >> 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..abc99ced30
> > >> --- /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_qemu import wait_for_console_pattern
> > >> +
> > >> +class RiscvOpensbi(QemuSystemTest):
> > >> +    """
> > >> +    :avocado: tags=accel:tcg
> > >> +    """
> > >> +    timeout = 5
> > >> +
> > >> +    def test_riscv64_virt(self):
> > >> +        """
> > >> +        :avocado: tags=arch:riscv64
> > >> +        :avocado: tags=machine:virt
> > >> +        """
> > >> +        self.vm.set_console()
> > >> +        self.vm.launch()
> > >> +        wait_for_console_pattern(self, 'Platform Name')
> > >> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
> > >> +
> > >> +    def test_riscv64_spike(self):
> > >> +        """
> > >> +        :avocado: tags=arch:riscv64
> > >> +        :avocado: tags=machine:spike
> > >> +        """
> > >> +        self.vm.set_console()
> > >> +        self.vm.launch()
> > >> +        wait_for_console_pattern(self, 'Platform Name')
> > >> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
> > >> +
> > >> +    def test_riscv64_sifive_u(self):
> > >> +        """
> > >> +        :avocado: tags=arch:riscv64
> > >> +        :avocado: tags=machine:sifive_u
> > >> +        """
> > >> +        self.vm.set_console()
> > >> +        self.vm.launch()
> > >> +        wait_for_console_pattern(self, 'Platform Name')
> > >> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
> > >> +
> > >> +    def test_riscv32_virt(self):
> > >> +        """
> > >> +        :avocado: tags=arch:riscv32
> > >> +        :avocado: tags=machine:virt
> > >> +        """
> > >> +        self.vm.set_console()
> > >> +        self.vm.launch()
> > >> +        wait_for_console_pattern(self, 'Platform Name')
> > >> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
> > > How about testing riscv32_spike too?
> >
> >
> > I didn't manage to make it work. This riscv64 spark command line boots opensbi:
> >
> >
> > $ ./qemu-system-riscv64 -nographic -display none -vga none -machine spike
> >
> > OpenSBI v1.1
> >     ____                    _____ ____ _____
> >    / __ \                  / ____|  _ \_   _|
> >   | |  | |_ __   ___ _ __ | (___ | |_) || |
> >   | |  | | '_ \ / _ \ '_ \ \___ \|  _ < | |
> >   | |__| | |_) |  __/ | | |____) | |_) || |_
> >    \____/| .__/ \___|_| |_|_____/|____/_____|
> >          | |
> >          |_|
> >
> > (...)
> >
> > The same command line doesn't boot riscv32 spark:
> >
> > ./qemu-system-riscv32 -nographic -display none -vga none -machine spike
> > (--- hangs indefinitely ---)
> >
> > I debugged it a bit and, as far as boot code goes, it goes all the way and loads the
> > opensbi 32bit binary.
> >
> > After that I tried to found any command line example that boots spike with riscv32
> > bit and didn't find any.  So I gave up digging it further because I became unsure
> > about whether 32-bit spike works.
> >
> > If someone can verify that yes, 32-bit spike is supposed to work, then I believe it's
> > worth investigating why it's not the case ATM.
> >
>
> +Anup who might know if QEMU spike 32-bit machine works with opensbi
> 32-bit generic image.

We never got HTIF putc() working on QEMU RV32 Spike but it works
perfectly fine on QEMU RV64 Spike.

See below log of QEMU RV64 Spike ...

Regards,
Anup

anup@anup-ubuntu-vm:~/Work/riscv-test/opensbi$ qemu-system-riscv64 -M
spike -m 256M -nographic -bios
/home/anup/Work/riscv-test/opensbi/build/platform/generic/firmware/fw_payload.elf

OpenSBI v1.1-124-gb848d87
   ____                    _____ ____ _____
  / __ \                  / ____|  _ \_   _|
 | |  | |_ __   ___ _ __ | (___ | |_) || |
 | |  | | '_ \ / _ \ '_ \ \___ \|  _ < | |
 | |__| | |_) |  __/ | | |____) | |_) || |_
  \____/| .__/ \___|_| |_|_____/|____/_____|
        | |
        |_|

Platform Name             : ucbbar,spike-bare,qemu
Platform Features         : medeleg
Platform HART Count       : 1
Platform IPI Device       : aclint-mswi
Platform Timer Device     : aclint-mtimer @ 10000000Hz
Platform Console Device   : htif
Platform HSM Device       : ---
Platform PMU Device       : ---
Platform Reboot Device    : htif
Platform Shutdown Device  : htif
Firmware Base             : 0x80000000
Firmware Size             : 212 KB
Runtime SBI Version       : 1.0

Domain0 Name              : root
Domain0 Boot HART         : 0
Domain0 HARTs             : 0*
Domain0 Region00          : 0x0000000002000000-0x000000000200ffff (I)
Domain0 Region01          : 0x0000000080000000-0x000000008003ffff ()
Domain0 Region02          : 0x0000000000000000-0xffffffffffffffff (R,W,X)
Domain0 Next Address      : 0x0000000080200000
Domain0 Next Arg1         : 0x0000000082200000
Domain0 Next Mode         : S-mode
Domain0 SysReset          : yes

Boot HART ID              : 0
Boot HART Domain          : root
Boot HART Priv Version    : v1.12
Boot HART Base ISA        : rv64imafdch
Boot HART ISA Extensions  : none
Boot HART PMP Count       : 16
Boot HART PMP Granularity : 4
Boot HART PMP Address Bits: 54
Boot HART MHPM Count      : 16
Boot HART MIDELEG         : 0x0000000000001666
Boot HART MEDELEG         : 0x0000000000f0b509

Test payload running

Re: [PATCH 01/15] tests/avocado: add RISC-V opensbi boot test

[Daniel Henrique Barboza]

On 12/22/22 13:56, Anup Patel wrote:
> On Thu, Dec 22, 2022 at 6:27 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>> On Thu, Dec 22, 2022 at 6:47 PM Daniel Henrique Barboza
>> <dbarboza@ventanamicro.com> wrote:
>>>
>>>
>>> On 12/22/22 07:24, Bin Meng wrote:
>>>> On Thu, Dec 22, 2022 at 2:29 AM 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.
>>>>>
>>>>> Cc: Cleber Rosa <crosa@redhat.com>
>>>>> Cc: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>> Cc: Wainer dos Santos Moschetta <wainersm@redhat.com>
>>>>> Cc: Beraldo Leal <bleal@redhat.com>
>>>>> 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..abc99ced30
>>>>> --- /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_qemu import wait_for_console_pattern
>>>>> +
>>>>> +class RiscvOpensbi(QemuSystemTest):
>>>>> +    """
>>>>> +    :avocado: tags=accel:tcg
>>>>> +    """
>>>>> +    timeout = 5
>>>>> +
>>>>> +    def test_riscv64_virt(self):
>>>>> +        """
>>>>> +        :avocado: tags=arch:riscv64
>>>>> +        :avocado: tags=machine:virt
>>>>> +        """
>>>>> +        self.vm.set_console()
>>>>> +        self.vm.launch()
>>>>> +        wait_for_console_pattern(self, 'Platform Name')
>>>>> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
>>>>> +
>>>>> +    def test_riscv64_spike(self):
>>>>> +        """
>>>>> +        :avocado: tags=arch:riscv64
>>>>> +        :avocado: tags=machine:spike
>>>>> +        """
>>>>> +        self.vm.set_console()
>>>>> +        self.vm.launch()
>>>>> +        wait_for_console_pattern(self, 'Platform Name')
>>>>> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
>>>>> +
>>>>> +    def test_riscv64_sifive_u(self):
>>>>> +        """
>>>>> +        :avocado: tags=arch:riscv64
>>>>> +        :avocado: tags=machine:sifive_u
>>>>> +        """
>>>>> +        self.vm.set_console()
>>>>> +        self.vm.launch()
>>>>> +        wait_for_console_pattern(self, 'Platform Name')
>>>>> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
>>>>> +
>>>>> +    def test_riscv32_virt(self):
>>>>> +        """
>>>>> +        :avocado: tags=arch:riscv32
>>>>> +        :avocado: tags=machine:virt
>>>>> +        """
>>>>> +        self.vm.set_console()
>>>>> +        self.vm.launch()
>>>>> +        wait_for_console_pattern(self, 'Platform Name')
>>>>> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
>>>> How about testing riscv32_spike too?
>>>
>>> I didn't manage to make it work. This riscv64 spark command line boots opensbi:
>>>
>>>
>>> $ ./qemu-system-riscv64 -nographic -display none -vga none -machine spike
>>>
>>> OpenSBI v1.1
>>>      ____                    _____ ____ _____
>>>     / __ \                  / ____|  _ \_   _|
>>>    | |  | |_ __   ___ _ __ | (___ | |_) || |
>>>    | |  | | '_ \ / _ \ '_ \ \___ \|  _ < | |
>>>    | |__| | |_) |  __/ | | |____) | |_) || |_
>>>     \____/| .__/ \___|_| |_|_____/|____/_____|
>>>           | |
>>>           |_|
>>>
>>> (...)
>>>
>>> The same command line doesn't boot riscv32 spark:
>>>
>>> ./qemu-system-riscv32 -nographic -display none -vga none -machine spike
>>> (--- hangs indefinitely ---)
>>>
>>> I debugged it a bit and, as far as boot code goes, it goes all the way and loads the
>>> opensbi 32bit binary.
>>>
>>> After that I tried to found any command line example that boots spike with riscv32
>>> bit and didn't find any.  So I gave up digging it further because I became unsure
>>> about whether 32-bit spike works.
>>>
>>> If someone can verify that yes, 32-bit spike is supposed to work, then I believe it's
>>> worth investigating why it's not the case ATM.
>>>
>> +Anup who might know if QEMU spike 32-bit machine works with opensbi
>> 32-bit generic image.
> We never got HTIF putc() working on QEMU RV32 Spike but it works
> perfectly fine on QEMU RV64 Spike.

Thanks for the info Anup!

I'll add this information in the commit msg/avocado file to document why we're not
testing spike 32 bits in this test that requires console output.


Daniel

>
> See below log of QEMU RV64 Spike ...
>
> Regards,
> Anup
>
> anup@anup-ubuntu-vm:~/Work/riscv-test/opensbi$ qemu-system-riscv64 -M
> spike -m 256M -nographic -bios
> /home/anup/Work/riscv-test/opensbi/build/platform/generic/firmware/fw_payload.elf
>
> OpenSBI v1.1-124-gb848d87
>     ____                    _____ ____ _____
>    / __ \                  / ____|  _ \_   _|
>   | |  | |_ __   ___ _ __ | (___ | |_) || |
>   | |  | | '_ \ / _ \ '_ \ \___ \|  _ < | |
>   | |__| | |_) |  __/ | | |____) | |_) || |_
>    \____/| .__/ \___|_| |_|_____/|____/_____|
>          | |
>          |_|
>
> Platform Name             : ucbbar,spike-bare,qemu
> Platform Features         : medeleg
> Platform HART Count       : 1
> Platform IPI Device       : aclint-mswi
> Platform Timer Device     : aclint-mtimer @ 10000000Hz
> Platform Console Device   : htif
> Platform HSM Device       : ---
> Platform PMU Device       : ---
> Platform Reboot Device    : htif
> Platform Shutdown Device  : htif
> Firmware Base             : 0x80000000
> Firmware Size             : 212 KB
> Runtime SBI Version       : 1.0
>
> Domain0 Name              : root
> Domain0 Boot HART         : 0
> Domain0 HARTs             : 0*
> Domain0 Region00          : 0x0000000002000000-0x000000000200ffff (I)
> Domain0 Region01          : 0x0000000080000000-0x000000008003ffff ()
> Domain0 Region02          : 0x0000000000000000-0xffffffffffffffff (R,W,X)
> Domain0 Next Address      : 0x0000000080200000
> Domain0 Next Arg1         : 0x0000000082200000
> Domain0 Next Mode         : S-mode
> Domain0 SysReset          : yes
>
> Boot HART ID              : 0
> Boot HART Domain          : root
> Boot HART Priv Version    : v1.12
> Boot HART Base ISA        : rv64imafdch
> Boot HART ISA Extensions  : none
> Boot HART PMP Count       : 16
> Boot HART PMP Granularity : 4
> Boot HART PMP Address Bits: 54
> Boot HART MHPM Count      : 16
> Boot HART MIDELEG         : 0x0000000000001666
> Boot HART MEDELEG         : 0x0000000000f0b509
>
> Test payload running

Re: [PATCH 01/15] tests/avocado: add RISC-V opensbi boot test

[Bin Meng]

Hi Anup,

On Fri, Dec 23, 2022 at 12:56 AM Anup Patel <anup@brainfault.org> wrote:
>
> On Thu, Dec 22, 2022 at 6:27 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > On Thu, Dec 22, 2022 at 6:47 PM Daniel Henrique Barboza
> > <dbarboza@ventanamicro.com> wrote:
> > >
> > >
> > >
> > > On 12/22/22 07:24, Bin Meng wrote:
> > > > On Thu, Dec 22, 2022 at 2:29 AM 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.
> > > >>
> > > >> Cc: Cleber Rosa <crosa@redhat.com>
> > > >> Cc: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > >> Cc: Wainer dos Santos Moschetta <wainersm@redhat.com>
> > > >> Cc: Beraldo Leal <bleal@redhat.com>
> > > >> 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..abc99ced30
> > > >> --- /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_qemu import wait_for_console_pattern
> > > >> +
> > > >> +class RiscvOpensbi(QemuSystemTest):
> > > >> +    """
> > > >> +    :avocado: tags=accel:tcg
> > > >> +    """
> > > >> +    timeout = 5
> > > >> +
> > > >> +    def test_riscv64_virt(self):
> > > >> +        """
> > > >> +        :avocado: tags=arch:riscv64
> > > >> +        :avocado: tags=machine:virt
> > > >> +        """
> > > >> +        self.vm.set_console()
> > > >> +        self.vm.launch()
> > > >> +        wait_for_console_pattern(self, 'Platform Name')
> > > >> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
> > > >> +
> > > >> +    def test_riscv64_spike(self):
> > > >> +        """
> > > >> +        :avocado: tags=arch:riscv64
> > > >> +        :avocado: tags=machine:spike
> > > >> +        """
> > > >> +        self.vm.set_console()
> > > >> +        self.vm.launch()
> > > >> +        wait_for_console_pattern(self, 'Platform Name')
> > > >> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
> > > >> +
> > > >> +    def test_riscv64_sifive_u(self):
> > > >> +        """
> > > >> +        :avocado: tags=arch:riscv64
> > > >> +        :avocado: tags=machine:sifive_u
> > > >> +        """
> > > >> +        self.vm.set_console()
> > > >> +        self.vm.launch()
> > > >> +        wait_for_console_pattern(self, 'Platform Name')
> > > >> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
> > > >> +
> > > >> +    def test_riscv32_virt(self):
> > > >> +        """
> > > >> +        :avocado: tags=arch:riscv32
> > > >> +        :avocado: tags=machine:virt
> > > >> +        """
> > > >> +        self.vm.set_console()
> > > >> +        self.vm.launch()
> > > >> +        wait_for_console_pattern(self, 'Platform Name')
> > > >> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
> > > > How about testing riscv32_spike too?
> > >
> > >
> > > I didn't manage to make it work. This riscv64 spark command line boots opensbi:
> > >
> > >
> > > $ ./qemu-system-riscv64 -nographic -display none -vga none -machine spike
> > >
> > > OpenSBI v1.1
> > >     ____                    _____ ____ _____
> > >    / __ \                  / ____|  _ \_   _|
> > >   | |  | |_ __   ___ _ __ | (___ | |_) || |
> > >   | |  | | '_ \ / _ \ '_ \ \___ \|  _ < | |
> > >   | |__| | |_) |  __/ | | |____) | |_) || |_
> > >    \____/| .__/ \___|_| |_|_____/|____/_____|
> > >          | |
> > >          |_|
> > >
> > > (...)
> > >
> > > The same command line doesn't boot riscv32 spark:
> > >
> > > ./qemu-system-riscv32 -nographic -display none -vga none -machine spike
> > > (--- hangs indefinitely ---)
> > >
> > > I debugged it a bit and, as far as boot code goes, it goes all the way and loads the
> > > opensbi 32bit binary.
> > >
> > > After that I tried to found any command line example that boots spike with riscv32
> > > bit and didn't find any.  So I gave up digging it further because I became unsure
> > > about whether 32-bit spike works.
> > >
> > > If someone can verify that yes, 32-bit spike is supposed to work, then I believe it's
> > > worth investigating why it's not the case ATM.
> > >
> >
> > +Anup who might know if QEMU spike 32-bit machine works with opensbi
> > 32-bit generic image.
>
> We never got HTIF putc() working on QEMU RV32 Spike but it works
> perfectly fine on QEMU RV64 Spike.

Where is the problem for the 32-bit? Is it in OpenSBI or in QEMU?

>
> See below log of QEMU RV64 Spike ...
>

If we cannot get Spike 32-bit to work in QEMU, should we drop the
32-bit support? @Alistair Francis

Regards,
Bin

Re: [PATCH 01/15] tests/avocado: add RISC-V opensbi boot test

[Bin Meng]

Hi,

On Fri, Dec 23, 2022 at 2:25 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Anup,
>
> On Fri, Dec 23, 2022 at 12:56 AM Anup Patel <anup@brainfault.org> wrote:
> >
> > On Thu, Dec 22, 2022 at 6:27 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > On Thu, Dec 22, 2022 at 6:47 PM Daniel Henrique Barboza
> > > <dbarboza@ventanamicro.com> wrote:
> > > >
> > > >
> > > >
> > > > On 12/22/22 07:24, Bin Meng wrote:
> > > > > On Thu, Dec 22, 2022 at 2:29 AM 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.
> > > > >>
> > > > >> Cc: Cleber Rosa <crosa@redhat.com>
> > > > >> Cc: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > > >> Cc: Wainer dos Santos Moschetta <wainersm@redhat.com>
> > > > >> Cc: Beraldo Leal <bleal@redhat.com>
> > > > >> 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..abc99ced30
> > > > >> --- /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_qemu import wait_for_console_pattern
> > > > >> +
> > > > >> +class RiscvOpensbi(QemuSystemTest):
> > > > >> +    """
> > > > >> +    :avocado: tags=accel:tcg
> > > > >> +    """
> > > > >> +    timeout = 5
> > > > >> +
> > > > >> +    def test_riscv64_virt(self):
> > > > >> +        """
> > > > >> +        :avocado: tags=arch:riscv64
> > > > >> +        :avocado: tags=machine:virt
> > > > >> +        """
> > > > >> +        self.vm.set_console()
> > > > >> +        self.vm.launch()
> > > > >> +        wait_for_console_pattern(self, 'Platform Name')
> > > > >> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
> > > > >> +
> > > > >> +    def test_riscv64_spike(self):
> > > > >> +        """
> > > > >> +        :avocado: tags=arch:riscv64
> > > > >> +        :avocado: tags=machine:spike
> > > > >> +        """
> > > > >> +        self.vm.set_console()
> > > > >> +        self.vm.launch()
> > > > >> +        wait_for_console_pattern(self, 'Platform Name')
> > > > >> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
> > > > >> +
> > > > >> +    def test_riscv64_sifive_u(self):
> > > > >> +        """
> > > > >> +        :avocado: tags=arch:riscv64
> > > > >> +        :avocado: tags=machine:sifive_u
> > > > >> +        """
> > > > >> +        self.vm.set_console()
> > > > >> +        self.vm.launch()
> > > > >> +        wait_for_console_pattern(self, 'Platform Name')
> > > > >> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
> > > > >> +
> > > > >> +    def test_riscv32_virt(self):
> > > > >> +        """
> > > > >> +        :avocado: tags=arch:riscv32
> > > > >> +        :avocado: tags=machine:virt
> > > > >> +        """
> > > > >> +        self.vm.set_console()
> > > > >> +        self.vm.launch()
> > > > >> +        wait_for_console_pattern(self, 'Platform Name')
> > > > >> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
> > > > > How about testing riscv32_spike too?
> > > >
> > > >
> > > > I didn't manage to make it work. This riscv64 spark command line boots opensbi:
> > > >
> > > >
> > > > $ ./qemu-system-riscv64 -nographic -display none -vga none -machine spike
> > > >
> > > > OpenSBI v1.1
> > > >     ____                    _____ ____ _____
> > > >    / __ \                  / ____|  _ \_   _|
> > > >   | |  | |_ __   ___ _ __ | (___ | |_) || |
> > > >   | |  | | '_ \ / _ \ '_ \ \___ \|  _ < | |
> > > >   | |__| | |_) |  __/ | | |____) | |_) || |_
> > > >    \____/| .__/ \___|_| |_|_____/|____/_____|
> > > >          | |
> > > >          |_|
> > > >
> > > > (...)
> > > >
> > > > The same command line doesn't boot riscv32 spark:
> > > >
> > > > ./qemu-system-riscv32 -nographic -display none -vga none -machine spike
> > > > (--- hangs indefinitely ---)
> > > >
> > > > I debugged it a bit and, as far as boot code goes, it goes all the way and loads the
> > > > opensbi 32bit binary.
> > > >
> > > > After that I tried to found any command line example that boots spike with riscv32
> > > > bit and didn't find any.  So I gave up digging it further because I became unsure
> > > > about whether 32-bit spike works.
> > > >
> > > > If someone can verify that yes, 32-bit spike is supposed to work, then I believe it's
> > > > worth investigating why it's not the case ATM.
> > > >
> > >
> > > +Anup who might know if QEMU spike 32-bit machine works with opensbi
> > > 32-bit generic image.
> >
> > We never got HTIF putc() working on QEMU RV32 Spike but it works
> > perfectly fine on QEMU RV64 Spike.
>
> Where is the problem for the 32-bit? Is it in OpenSBI or in QEMU?
>
> >
> > See below log of QEMU RV64 Spike ...
> >
>
> If we cannot get Spike 32-bit to work in QEMU, should we drop the
> 32-bit support? @Alistair Francis

I got a deeper look at the 32-bit spike issue and I believe it is a
problem of QEMU HTIF emulation.

I will see if I can spin a patch to fix this.

Regards,
Bin

Re: [PATCH 01/15] tests/avocado: add RISC-V opensbi boot test

[Bin Meng]

On Sat, Dec 24, 2022 at 11:52 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi,
>
> On Fri, Dec 23, 2022 at 2:25 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Anup,
> >
> > On Fri, Dec 23, 2022 at 12:56 AM Anup Patel <anup@brainfault.org> wrote:
> > >
> > > On Thu, Dec 22, 2022 at 6:27 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> > > >
> > > > On Thu, Dec 22, 2022 at 6:47 PM Daniel Henrique Barboza
> > > > <dbarboza@ventanamicro.com> wrote:
> > > > >
> > > > >
> > > > >
> > > > > On 12/22/22 07:24, Bin Meng wrote:
> > > > > > On Thu, Dec 22, 2022 at 2:29 AM 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.
> > > > > >>
> > > > > >> Cc: Cleber Rosa <crosa@redhat.com>
> > > > > >> Cc: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > > > >> Cc: Wainer dos Santos Moschetta <wainersm@redhat.com>
> > > > > >> Cc: Beraldo Leal <bleal@redhat.com>
> > > > > >> 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..abc99ced30
> > > > > >> --- /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_qemu import wait_for_console_pattern
> > > > > >> +
> > > > > >> +class RiscvOpensbi(QemuSystemTest):
> > > > > >> +    """
> > > > > >> +    :avocado: tags=accel:tcg
> > > > > >> +    """
> > > > > >> +    timeout = 5
> > > > > >> +
> > > > > >> +    def test_riscv64_virt(self):
> > > > > >> +        """
> > > > > >> +        :avocado: tags=arch:riscv64
> > > > > >> +        :avocado: tags=machine:virt
> > > > > >> +        """
> > > > > >> +        self.vm.set_console()
> > > > > >> +        self.vm.launch()
> > > > > >> +        wait_for_console_pattern(self, 'Platform Name')
> > > > > >> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
> > > > > >> +
> > > > > >> +    def test_riscv64_spike(self):
> > > > > >> +        """
> > > > > >> +        :avocado: tags=arch:riscv64
> > > > > >> +        :avocado: tags=machine:spike
> > > > > >> +        """
> > > > > >> +        self.vm.set_console()
> > > > > >> +        self.vm.launch()
> > > > > >> +        wait_for_console_pattern(self, 'Platform Name')
> > > > > >> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
> > > > > >> +
> > > > > >> +    def test_riscv64_sifive_u(self):
> > > > > >> +        """
> > > > > >> +        :avocado: tags=arch:riscv64
> > > > > >> +        :avocado: tags=machine:sifive_u
> > > > > >> +        """
> > > > > >> +        self.vm.set_console()
> > > > > >> +        self.vm.launch()
> > > > > >> +        wait_for_console_pattern(self, 'Platform Name')
> > > > > >> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
> > > > > >> +
> > > > > >> +    def test_riscv32_virt(self):
> > > > > >> +        """
> > > > > >> +        :avocado: tags=arch:riscv32
> > > > > >> +        :avocado: tags=machine:virt
> > > > > >> +        """
> > > > > >> +        self.vm.set_console()
> > > > > >> +        self.vm.launch()
> > > > > >> +        wait_for_console_pattern(self, 'Platform Name')
> > > > > >> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
> > > > > > How about testing riscv32_spike too?
> > > > >
> > > > >
> > > > > I didn't manage to make it work. This riscv64 spark command line boots opensbi:
> > > > >
> > > > >
> > > > > $ ./qemu-system-riscv64 -nographic -display none -vga none -machine spike
> > > > >
> > > > > OpenSBI v1.1
> > > > >     ____                    _____ ____ _____
> > > > >    / __ \                  / ____|  _ \_   _|
> > > > >   | |  | |_ __   ___ _ __ | (___ | |_) || |
> > > > >   | |  | | '_ \ / _ \ '_ \ \___ \|  _ < | |
> > > > >   | |__| | |_) |  __/ | | |____) | |_) || |_
> > > > >    \____/| .__/ \___|_| |_|_____/|____/_____|
> > > > >          | |
> > > > >          |_|
> > > > >
> > > > > (...)
> > > > >
> > > > > The same command line doesn't boot riscv32 spark:
> > > > >
> > > > > ./qemu-system-riscv32 -nographic -display none -vga none -machine spike
> > > > > (--- hangs indefinitely ---)
> > > > >
> > > > > I debugged it a bit and, as far as boot code goes, it goes all the way and loads the
> > > > > opensbi 32bit binary.
> > > > >
> > > > > After that I tried to found any command line example that boots spike with riscv32
> > > > > bit and didn't find any.  So I gave up digging it further because I became unsure
> > > > > about whether 32-bit spike works.
> > > > >
> > > > > If someone can verify that yes, 32-bit spike is supposed to work, then I believe it's
> > > > > worth investigating why it's not the case ATM.
> > > > >
> > > >
> > > > +Anup who might know if QEMU spike 32-bit machine works with opensbi
> > > > 32-bit generic image.
> > >
> > > We never got HTIF putc() working on QEMU RV32 Spike but it works
> > > perfectly fine on QEMU RV64 Spike.
> >
> > Where is the problem for the 32-bit? Is it in OpenSBI or in QEMU?
> >
> > >
> > > See below log of QEMU RV64 Spike ...
> > >
> >
> > If we cannot get Spike 32-bit to work in QEMU, should we drop the
> > 32-bit support? @Alistair Francis
>
> I got a deeper look at the 32-bit spike issue and I believe it is a
> problem of QEMU HTIF emulation.
>
> I will see if I can spin a patch to fix this.
>

It turns out there is a bug in OpenSBI too when booting 32-bit BIN
image on Spike.

For ELF & BIN image boot on QEMU, QEMU changes are needed. I will send
the QEMU patches soon.

Regards,
Bin

Re: [PATCH 01/15] tests/avocado: add RISC-V opensbi boot test

[Daniel Henrique Barboza]

On 12/26/22 10:56, Bin Meng wrote:
> On Sat, Dec 24, 2022 at 11:52 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>> Hi,
>>
>> On Fri, Dec 23, 2022 at 2:25 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>>> Hi Anup,
>>>
>>> On Fri, Dec 23, 2022 at 12:56 AM Anup Patel <anup@brainfault.org> wrote:
>>>> On Thu, Dec 22, 2022 at 6:27 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>>>>> On Thu, Dec 22, 2022 at 6:47 PM Daniel Henrique Barboza
>>>>> <dbarboza@ventanamicro.com> wrote:
>>>>>>
>>>>>>
>>>>>> On 12/22/22 07:24, Bin Meng wrote:
>>>>>>> On Thu, Dec 22, 2022 at 2:29 AM 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.
>>>>>>>>
>>>>>>>> Cc: Cleber Rosa <crosa@redhat.com>
>>>>>>>> Cc: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>>>> Cc: Wainer dos Santos Moschetta <wainersm@redhat.com>
>>>>>>>> Cc: Beraldo Leal <bleal@redhat.com>
>>>>>>>> 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..abc99ced30
>>>>>>>> --- /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_qemu import wait_for_console_pattern
>>>>>>>> +
>>>>>>>> +class RiscvOpensbi(QemuSystemTest):
>>>>>>>> +    """
>>>>>>>> +    :avocado: tags=accel:tcg
>>>>>>>> +    """
>>>>>>>> +    timeout = 5
>>>>>>>> +
>>>>>>>> +    def test_riscv64_virt(self):
>>>>>>>> +        """
>>>>>>>> +        :avocado: tags=arch:riscv64
>>>>>>>> +        :avocado: tags=machine:virt
>>>>>>>> +        """
>>>>>>>> +        self.vm.set_console()
>>>>>>>> +        self.vm.launch()
>>>>>>>> +        wait_for_console_pattern(self, 'Platform Name')
>>>>>>>> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
>>>>>>>> +
>>>>>>>> +    def test_riscv64_spike(self):
>>>>>>>> +        """
>>>>>>>> +        :avocado: tags=arch:riscv64
>>>>>>>> +        :avocado: tags=machine:spike
>>>>>>>> +        """
>>>>>>>> +        self.vm.set_console()
>>>>>>>> +        self.vm.launch()
>>>>>>>> +        wait_for_console_pattern(self, 'Platform Name')
>>>>>>>> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
>>>>>>>> +
>>>>>>>> +    def test_riscv64_sifive_u(self):
>>>>>>>> +        """
>>>>>>>> +        :avocado: tags=arch:riscv64
>>>>>>>> +        :avocado: tags=machine:sifive_u
>>>>>>>> +        """
>>>>>>>> +        self.vm.set_console()
>>>>>>>> +        self.vm.launch()
>>>>>>>> +        wait_for_console_pattern(self, 'Platform Name')
>>>>>>>> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
>>>>>>>> +
>>>>>>>> +    def test_riscv32_virt(self):
>>>>>>>> +        """
>>>>>>>> +        :avocado: tags=arch:riscv32
>>>>>>>> +        :avocado: tags=machine:virt
>>>>>>>> +        """
>>>>>>>> +        self.vm.set_console()
>>>>>>>> +        self.vm.launch()
>>>>>>>> +        wait_for_console_pattern(self, 'Platform Name')
>>>>>>>> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
>>>>>>> How about testing riscv32_spike too?
>>>>>>
>>>>>> I didn't manage to make it work. This riscv64 spark command line boots opensbi:
>>>>>>
>>>>>>
>>>>>> $ ./qemu-system-riscv64 -nographic -display none -vga none -machine spike
>>>>>>
>>>>>> OpenSBI v1.1
>>>>>>      ____                    _____ ____ _____
>>>>>>     / __ \                  / ____|  _ \_   _|
>>>>>>    | |  | |_ __   ___ _ __ | (___ | |_) || |
>>>>>>    | |  | | '_ \ / _ \ '_ \ \___ \|  _ < | |
>>>>>>    | |__| | |_) |  __/ | | |____) | |_) || |_
>>>>>>     \____/| .__/ \___|_| |_|_____/|____/_____|
>>>>>>           | |
>>>>>>           |_|
>>>>>>
>>>>>> (...)
>>>>>>
>>>>>> The same command line doesn't boot riscv32 spark:
>>>>>>
>>>>>> ./qemu-system-riscv32 -nographic -display none -vga none -machine spike
>>>>>> (--- hangs indefinitely ---)
>>>>>>
>>>>>> I debugged it a bit and, as far as boot code goes, it goes all the way and loads the
>>>>>> opensbi 32bit binary.
>>>>>>
>>>>>> After that I tried to found any command line example that boots spike with riscv32
>>>>>> bit and didn't find any.  So I gave up digging it further because I became unsure
>>>>>> about whether 32-bit spike works.
>>>>>>
>>>>>> If someone can verify that yes, 32-bit spike is supposed to work, then I believe it's
>>>>>> worth investigating why it's not the case ATM.
>>>>>>
>>>>> +Anup who might know if QEMU spike 32-bit machine works with opensbi
>>>>> 32-bit generic image.
>>>> We never got HTIF putc() working on QEMU RV32 Spike but it works
>>>> perfectly fine on QEMU RV64 Spike.
>>> Where is the problem for the 32-bit? Is it in OpenSBI or in QEMU?
>>>
>>>> See below log of QEMU RV64 Spike ...
>>>>
>>> If we cannot get Spike 32-bit to work in QEMU, should we drop the
>>> 32-bit support? @Alistair Francis
>> I got a deeper look at the 32-bit spike issue and I believe it is a
>> problem of QEMU HTIF emulation.
>>
>> I will see if I can spin a patch to fix this.
>>
> It turns out there is a bug in OpenSBI too when booting 32-bit BIN
> image on Spike.
>
> For ELF & BIN image boot on QEMU, QEMU changes are needed. I will send
> the QEMU patches soon.

I'll wait for your patches to re-send this series. If you can get spike 32-bit
to work then I'll re-send the v2 with the 32 bit spike tests enabled.


Thanks,


Daniel

>
> Regards,
> Bin

Re: [PATCH 01/15] tests/avocado: add RISC-V opensbi boot test

[Daniel Henrique Barboza]

Bin,

On 12/26/22 10:56, Bin Meng wrote:
> On Sat, Dec 24, 2022 at 11:52 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>> Hi,
>>
>> On Fri, Dec 23, 2022 at 2:25 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>>> Hi Anup,
>>>
>>> On Fri, Dec 23, 2022 at 12:56 AM Anup Patel <anup@brainfault.org> wrote:
>>>> On Thu, Dec 22, 2022 at 6:27 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>>>>> On Thu, Dec 22, 2022 at 6:47 PM Daniel Henrique Barboza
>>>>> <dbarboza@ventanamicro.com> wrote:
>>>>>>
>>>>>>
>>>>>> On 12/22/22 07:24, Bin Meng wrote:
>>>>>>> On Thu, Dec 22, 2022 at 2:29 AM 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.

[.....]



>>>>>>>> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
>>>>>>> How about testing riscv32_spike too?
>>>>>>
>>>>>> I didn't manage to make it work. This riscv64 spark command line boots opensbi:
>>>>>>
>>>>>>
>>>>>> $ ./qemu-system-riscv64 -nographic -display none -vga none -machine spike
>>>>>>
>>>>>> OpenSBI v1.1
>>>>>>      ____                    _____ ____ _____
>>>>>>     / __ \                  / ____|  _ \_   _|
>>>>>>    | |  | |_ __   ___ _ __ | (___ | |_) || |
>>>>>>    | |  | | '_ \ / _ \ '_ \ \___ \|  _ < | |
>>>>>>    | |__| | |_) |  __/ | | |____) | |_) || |_
>>>>>>     \____/| .__/ \___|_| |_|_____/|____/_____|
>>>>>>           | |
>>>>>>           |_|
>>>>>>
>>>>>> (...)
>>>>>>
>>>>>> The same command line doesn't boot riscv32 spark:
>>>>>>
>>>>>> ./qemu-system-riscv32 -nographic -display none -vga none -machine spike
>>>>>> (--- hangs indefinitely ---)
>>>>>>
>>>>>> I debugged it a bit and, as far as boot code goes, it goes all the way and loads the
>>>>>> opensbi 32bit binary.
>>>>>>
>>>>>> After that I tried to found any command line example that boots spike with riscv32
>>>>>> bit and didn't find any.  So I gave up digging it further because I became unsure
>>>>>> about whether 32-bit spike works.
>>>>>>
>>>>>> If someone can verify that yes, 32-bit spike is supposed to work, then I believe it's
>>>>>> worth investigating why it's not the case ATM.
>>>>>>
>>>>> +Anup who might know if QEMU spike 32-bit machine works with opensbi
>>>>> 32-bit generic image.
>>>> We never got HTIF putc() working on QEMU RV32 Spike but it works
>>>> perfectly fine on QEMU RV64 Spike.
>>> Where is the problem for the 32-bit? Is it in OpenSBI or in QEMU?
>>>
>>>> See below log of QEMU RV64 Spike ...
>>>>
>>> If we cannot get Spike 32-bit to work in QEMU, should we drop the
>>> 32-bit support? @Alistair Francis
>> I got a deeper look at the 32-bit spike issue and I believe it is a
>> problem of QEMU HTIF emulation.
>>
>> I will see if I can spin a patch to fix this.
>>
> It turns out there is a bug in OpenSBI too when booting 32-bit BIN
> image on Spike.
>
> For ELF & BIN image boot on QEMU, QEMU changes are needed. I will send
> the QEMU patches soon.

I reviewed the QEMU patches and sent a tested-by ack in the opensbi fix
as well. LGTM.

As I commented in your QEMU patches earlier [1], it would be good if we could
have an opensbi 1.2 rom with the fix already applied. If that's not possible then
what we could do, w.r.t this patch, is to add the spike 32bit test with a skip note
mentioning that we need an opensbi update before enabling it.

[1] https://mail.gnu.org/archive/html/qemu-devel/2022-12/msg04414.html


Thanks,


Daniel


>
> Regards,
> Bin

Re: [PATCH 01/15] tests/avocado: add RISC-V opensbi boot test

[Alistair Francis]

On Thu, Dec 22, 2022 at 4:29 AM 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.
>
> Cc: Cleber Rosa <crosa@redhat.com>
> Cc: Philippe Mathieu-Daudé <philmd@linaro.org>
> Cc: Wainer dos Santos Moschetta <wainersm@redhat.com>
> Cc: Beraldo Leal <bleal@redhat.com>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Reviewed-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..abc99ced30
> --- /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_qemu import wait_for_console_pattern
> +
> +class RiscvOpensbi(QemuSystemTest):
> +    """
> +    :avocado: tags=accel:tcg
> +    """
> +    timeout = 5
> +
> +    def test_riscv64_virt(self):
> +        """
> +        :avocado: tags=arch:riscv64
> +        :avocado: tags=machine:virt
> +        """
> +        self.vm.set_console()
> +        self.vm.launch()
> +        wait_for_console_pattern(self, 'Platform Name')
> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
> +
> +    def test_riscv64_spike(self):
> +        """
> +        :avocado: tags=arch:riscv64
> +        :avocado: tags=machine:spike
> +        """
> +        self.vm.set_console()
> +        self.vm.launch()
> +        wait_for_console_pattern(self, 'Platform Name')
> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
> +
> +    def test_riscv64_sifive_u(self):
> +        """
> +        :avocado: tags=arch:riscv64
> +        :avocado: tags=machine:sifive_u
> +        """
> +        self.vm.set_console()
> +        self.vm.launch()
> +        wait_for_console_pattern(self, 'Platform Name')
> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
> +
> +    def test_riscv32_virt(self):
> +        """
> +        :avocado: tags=arch:riscv32
> +        :avocado: tags=machine:virt
> +        """
> +        self.vm.set_console()
> +        self.vm.launch()
> +        wait_for_console_pattern(self, 'Platform Name')
> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
> +
> +    def test_riscv32_sifive_u(self):
> +        """
> +        :avocado: tags=arch:riscv32
> +        :avocado: tags=machine:sifive_u
> +        """
> +        self.vm.set_console()
> +        self.vm.launch()
> +        wait_for_console_pattern(self, 'Platform Name')
> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
> --
> 2.38.1
>
>

Re: [PATCH 01/15] tests/avocado: add RISC-V opensbi boot test

[Wainer dos Santos Moschetta]

Hi Daniel,

On 12/21/22 15:22, Daniel Henrique Barboza 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.
>
> Cc: Cleber Rosa <crosa@redhat.com>
> Cc: Philippe Mathieu-Daudé <philmd@linaro.org>
> Cc: Wainer dos Santos Moschetta <wainersm@redhat.com>
> Cc: Beraldo Leal <bleal@redhat.com>
> 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

It looks good to me. Thanks!

Reviewed-by: Wainer dos Santos Moschetta <wainersm@redhat.com>

>
> diff --git a/tests/avocado/riscv_opensbi.py b/tests/avocado/riscv_opensbi.py
> new file mode 100644
> index 0000000000..abc99ced30
> --- /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_qemu import wait_for_console_pattern
> +
> +class RiscvOpensbi(QemuSystemTest):
> +    """
> +    :avocado: tags=accel:tcg
> +    """
> +    timeout = 5
> +
> +    def test_riscv64_virt(self):
> +        """
> +        :avocado: tags=arch:riscv64
> +        :avocado: tags=machine:virt
> +        """
> +        self.vm.set_console()
> +        self.vm.launch()
> +        wait_for_console_pattern(self, 'Platform Name')
> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
> +
> +    def test_riscv64_spike(self):
> +        """
> +        :avocado: tags=arch:riscv64
> +        :avocado: tags=machine:spike
> +        """
> +        self.vm.set_console()
> +        self.vm.launch()
> +        wait_for_console_pattern(self, 'Platform Name')
> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
> +
> +    def test_riscv64_sifive_u(self):
> +        """
> +        :avocado: tags=arch:riscv64
> +        :avocado: tags=machine:sifive_u
> +        """
> +        self.vm.set_console()
> +        self.vm.launch()
> +        wait_for_console_pattern(self, 'Platform Name')
> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
> +
> +    def test_riscv32_virt(self):
> +        """
> +        :avocado: tags=arch:riscv32
> +        :avocado: tags=machine:virt
> +        """
> +        self.vm.set_console()
> +        self.vm.launch()
> +        wait_for_console_pattern(self, 'Platform Name')
> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')
> +
> +    def test_riscv32_sifive_u(self):
> +        """
> +        :avocado: tags=arch:riscv32
> +        :avocado: tags=machine:sifive_u
> +        """
> +        self.vm.set_console()
> +        self.vm.launch()
> +        wait_for_console_pattern(self, 'Platform Name')
> +        wait_for_console_pattern(self, 'Boot HART MEDELEG')

[PATCH 02/15] 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>
---
 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 13946acf0d..d96f013e2e 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -52,6 +52,7 @@ static void create_fdt(SpikeState *s, const MemMapEntry *memmap,
                        uint64_t mem_size, const char *cmdline, bool is_32_bit)
 {
     void *fdt;
+    int fdt_size;
     uint64_t addr, size;
     unsigned long clint_addr;
     int cpu, socket;
@@ -64,7 +65,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);
@@ -296,18 +297,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.38.1

Re: [PATCH 02/15] hw/riscv/spike: use 'fdt' from MachineState

[Philippe Mathieu-Daudé]

On 21/12/22 19:22, Daniel Henrique Barboza wrote:
> 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>
> ---
>   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 13946acf0d..d96f013e2e 100644
> --- a/hw/riscv/spike.c
> +++ b/hw/riscv/spike.c
> @@ -52,6 +52,7 @@ static void create_fdt(SpikeState *s, const MemMapEntry *memmap,
>                          uint64_t mem_size, const char *cmdline, bool is_32_bit)
>   {
>       void *fdt;
> +    int fdt_size;
>       uint64_t addr, size;
>       unsigned long clint_addr;
>       int cpu, socket;
> @@ -64,7 +65,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);

We use 'ms' for MachineState and 'mc' for MachineClass. I first got
scared while looking at that patch that a class field was used. The
variable is simply badly named. Possible future cleanup: s/mc/ms/.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 02/15] hw/riscv/spike: use 'fdt' from MachineState

[Daniel Henrique Barboza]

On 12/22/22 11:25, Philippe Mathieu-Daudé wrote:
> On 21/12/22 19:22, Daniel Henrique Barboza wrote:
>> 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>
>> ---
>>   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 13946acf0d..d96f013e2e 100644
>> --- a/hw/riscv/spike.c
>> +++ b/hw/riscv/spike.c
>> @@ -52,6 +52,7 @@ static void create_fdt(SpikeState *s, const MemMapEntry *memmap,
>>                          uint64_t mem_size, const char *cmdline, bool is_32_bit)
>>   {
>>       void *fdt;
>> +    int fdt_size;
>>       uint64_t addr, size;
>>       unsigned long clint_addr;
>>       int cpu, socket;
>> @@ -64,7 +65,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);
>
> We use 'ms' for MachineState and 'mc' for MachineClass. I first got
> scared while looking at that patch that a class field was used. The
> variable is simply badly named. Possible future cleanup: s/mc/ms/.

Thanks for the ack Phil!


And yeah, I think I'll drop a patch with your suggestion later on.


Daniel

> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>

Re: [PATCH 02/15] hw/riscv/spike: use 'fdt' from MachineState

[Alistair Francis]

On Thu, Dec 22, 2022 at 4:30 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> 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: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  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 13946acf0d..d96f013e2e 100644
> --- a/hw/riscv/spike.c
> +++ b/hw/riscv/spike.c
> @@ -52,6 +52,7 @@ static void create_fdt(SpikeState *s, const MemMapEntry *memmap,
>                         uint64_t mem_size, const char *cmdline, bool is_32_bit)
>  {
>      void *fdt;
> +    int fdt_size;
>      uint64_t addr, size;
>      unsigned long clint_addr;
>      int cpu, socket;
> @@ -64,7 +65,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);
> @@ -296,18 +297,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.38.1
>
>

Re: [PATCH 02/15] hw/riscv/spike: use 'fdt' from MachineState

[Bin Meng]

On Thu, Dec 22, 2022 at 2:24 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> 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>
> ---
>  hw/riscv/spike.c         | 12 +++++-------
>  include/hw/riscv/spike.h |  2 --
>  2 files changed, 5 insertions(+), 9 deletions(-)
>

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

[PATCH 03/15] 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>
---
 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 b40a4767e2..9cf66957ab 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);
@@ -615,9 +615,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 {
@@ -630,14 +630,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.38.1

Re: [PATCH 03/15] hw/riscv/sifive_u: use 'fdt' from MachineState

[Philippe Mathieu-Daudé]

On 21/12/22 19:22, Daniel Henrique Barboza wrote:
> 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>
> ---
>   hw/riscv/sifive_u.c         | 15 ++++++---------
>   include/hw/riscv/sifive_u.h |  3 ---
>   2 files changed, 6 insertions(+), 12 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 03/15] hw/riscv/sifive_u: use 'fdt' from MachineState

[Alistair Francis]

On Thu, Dec 22, 2022 at 4:29 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> 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: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  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 b40a4767e2..9cf66957ab 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);
> @@ -615,9 +615,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 {
> @@ -630,14 +630,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.38.1
>
>

Re: [PATCH 03/15] hw/riscv/sifive_u: use 'fdt' from MachineState

[Bin Meng]

On Thu, Dec 22, 2022 at 2:29 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> 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>
> ---
>  hw/riscv/sifive_u.c         | 15 ++++++---------
>  include/hw/riscv/sifive_u.h |  3 ---
>  2 files changed, 6 insertions(+), 12 deletions(-)
>

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

[PATCH 04/15] hw/riscv/boot.c: make riscv_find_firmware() static

[Daniel Henrique Barboza]

The only caller is riscv_find_and_load_firmware(), which is in the same
file.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/riscv/boot.c         | 44 ++++++++++++++++++++---------------------
 include/hw/riscv/boot.h |  1 -
 2 files changed, 22 insertions(+), 23 deletions(-)

diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index ebd351c840..7361d5c0d8 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -75,6 +75,28 @@ target_ulong riscv_calc_kernel_start_addr(RISCVHartArrayState *harts,
     }
 }
 
+static char *riscv_find_firmware(const char *firmware_filename)
+{
+    char *filename;
+
+    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, firmware_filename);
+    if (filename == NULL) {
+        if (!qtest_enabled()) {
+            /*
+             * We only ship OpenSBI binary bios images in the QEMU source.
+             * For machines that use images other than the default bios,
+             * running QEMU test will complain hence let's suppress the error
+             * report for QEMU testing.
+             */
+            error_report("Unable to load the RISC-V firmware \"%s\"",
+                         firmware_filename);
+            exit(1);
+        }
+    }
+
+    return filename;
+}
+
 target_ulong riscv_find_and_load_firmware(MachineState *machine,
                                           const char *default_machine_firmware,
                                           hwaddr firmware_load_addr,
@@ -104,28 +126,6 @@ target_ulong riscv_find_and_load_firmware(MachineState *machine,
     return firmware_end_addr;
 }
 
-char *riscv_find_firmware(const char *firmware_filename)
-{
-    char *filename;
-
-    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, firmware_filename);
-    if (filename == NULL) {
-        if (!qtest_enabled()) {
-            /*
-             * We only ship OpenSBI binary bios images in the QEMU source.
-             * For machines that use images other than the default bios,
-             * running QEMU test will complain hence let's suppress the error
-             * report for QEMU testing.
-             */
-            error_report("Unable to load the RISC-V firmware \"%s\"",
-                         firmware_filename);
-            exit(1);
-        }
-    }
-
-    return filename;
-}
-
 target_ulong riscv_load_firmware(const char *firmware_filename,
                                  hwaddr firmware_load_addr,
                                  symbol_fn_t sym_cb)
diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
index 93e5f8760d..c03e4e74c5 100644
--- a/include/hw/riscv/boot.h
+++ b/include/hw/riscv/boot.h
@@ -37,7 +37,6 @@ target_ulong riscv_find_and_load_firmware(MachineState *machine,
                                           const char *default_machine_firmware,
                                           hwaddr firmware_load_addr,
                                           symbol_fn_t sym_cb);
-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);
-- 
2.38.1

Re: [PATCH 04/15] hw/riscv/boot.c: make riscv_find_firmware() static

[Philippe Mathieu-Daudé]

On 21/12/22 19:22, Daniel Henrique Barboza wrote:
> The only caller is riscv_find_and_load_firmware(), which is in the same
> file.
> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>   hw/riscv/boot.c         | 44 ++++++++++++++++++++---------------------
>   include/hw/riscv/boot.h |  1 -
>   2 files changed, 22 insertions(+), 23 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 04/15] hw/riscv/boot.c: make riscv_find_firmware() static

[Alistair Francis]

On Thu, Dec 22, 2022 at 4:26 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> The only caller is riscv_find_and_load_firmware(), which is in the same
> file.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

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

Alistair

> ---
>  hw/riscv/boot.c         | 44 ++++++++++++++++++++---------------------
>  include/hw/riscv/boot.h |  1 -
>  2 files changed, 22 insertions(+), 23 deletions(-)
>
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index ebd351c840..7361d5c0d8 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -75,6 +75,28 @@ target_ulong riscv_calc_kernel_start_addr(RISCVHartArrayState *harts,
>      }
>  }
>
> +static char *riscv_find_firmware(const char *firmware_filename)
> +{
> +    char *filename;
> +
> +    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, firmware_filename);
> +    if (filename == NULL) {
> +        if (!qtest_enabled()) {
> +            /*
> +             * We only ship OpenSBI binary bios images in the QEMU source.
> +             * For machines that use images other than the default bios,
> +             * running QEMU test will complain hence let's suppress the error
> +             * report for QEMU testing.
> +             */
> +            error_report("Unable to load the RISC-V firmware \"%s\"",
> +                         firmware_filename);
> +            exit(1);
> +        }
> +    }
> +
> +    return filename;
> +}
> +
>  target_ulong riscv_find_and_load_firmware(MachineState *machine,
>                                            const char *default_machine_firmware,
>                                            hwaddr firmware_load_addr,
> @@ -104,28 +126,6 @@ target_ulong riscv_find_and_load_firmware(MachineState *machine,
>      return firmware_end_addr;
>  }
>
> -char *riscv_find_firmware(const char *firmware_filename)
> -{
> -    char *filename;
> -
> -    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, firmware_filename);
> -    if (filename == NULL) {
> -        if (!qtest_enabled()) {
> -            /*
> -             * We only ship OpenSBI binary bios images in the QEMU source.
> -             * For machines that use images other than the default bios,
> -             * running QEMU test will complain hence let's suppress the error
> -             * report for QEMU testing.
> -             */
> -            error_report("Unable to load the RISC-V firmware \"%s\"",
> -                         firmware_filename);
> -            exit(1);
> -        }
> -    }
> -
> -    return filename;
> -}
> -
>  target_ulong riscv_load_firmware(const char *firmware_filename,
>                                   hwaddr firmware_load_addr,
>                                   symbol_fn_t sym_cb)
> diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
> index 93e5f8760d..c03e4e74c5 100644
> --- a/include/hw/riscv/boot.h
> +++ b/include/hw/riscv/boot.h
> @@ -37,7 +37,6 @@ target_ulong riscv_find_and_load_firmware(MachineState *machine,
>                                            const char *default_machine_firmware,
>                                            hwaddr firmware_load_addr,
>                                            symbol_fn_t sym_cb);
> -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);
> --
> 2.38.1
>
>

Re: [PATCH 04/15] hw/riscv/boot.c: make riscv_find_firmware() static

[Bin Meng]

On Thu, Dec 22, 2022 at 2:27 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> The only caller is riscv_find_and_load_firmware(), which is in the same
> file.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  hw/riscv/boot.c         | 44 ++++++++++++++++++++---------------------
>  include/hw/riscv/boot.h |  1 -
>  2 files changed, 22 insertions(+), 23 deletions(-)
>

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

[PATCH 05/15] hw/riscv/boot.c: introduce riscv_default_firmware_name()

[Daniel Henrique Barboza]

Some boards are duplicating the 'riscv_find_and_load_firmware' call
because the 32 and 64 bits images have different names. Create
a function to handle this detail instead of hardcoding it in the boards.

Ideally we would bake this logic inside riscv_find_and_load_firmware(),
or even create a riscv_load_default_firmware(), but at this moment we
cannot infer whether the machine is running 32 or 64 bits without
accessing RISCVHartArrayState, which in turn can't be accessed via the
common code from boot.c. In the end we would exchange 'firmware_name'
for a flag with riscv_is_32bit(), which isn't much better than what we
already have today.

Cc: Palmer Dabbelt <palmer@dabbelt.com>
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/riscv/boot.c         |  9 +++++++++
 hw/riscv/sifive_u.c     | 11 ++++-------
 hw/riscv/spike.c        | 14 +++++---------
 hw/riscv/virt.c         | 10 +++-------
 include/hw/riscv/boot.h |  1 +
 5 files changed, 22 insertions(+), 23 deletions(-)

diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index 7361d5c0d8..e1a544b1d9 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -75,6 +75,15 @@ target_ulong riscv_calc_kernel_start_addr(RISCVHartArrayState *harts,
     }
 }
 
+const char *riscv_default_firmware_name(RISCVHartArrayState *harts)
+{
+    if (riscv_is_32bit(harts)) {
+        return RISCV32_BIOS_BIN;
+    }
+
+    return RISCV64_BIOS_BIN;
+}
+
 static char *riscv_find_firmware(const char *firmware_filename)
 {
     char *filename;
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index 9cf66957ab..ddceb750ea 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -533,6 +533,7 @@ static void sifive_u_machine_init(MachineState *machine)
     MemoryRegion *flash0 = g_new(MemoryRegion, 1);
     target_ulong start_addr = memmap[SIFIVE_U_DEV_DRAM].base;
     target_ulong firmware_end_addr, kernel_start_addr;
+    const char *firmware_name;
     uint32_t start_addr_hi32 = 0x00000000;
     int i;
     uint32_t fdt_load_addr;
@@ -595,13 +596,9 @@ static void sifive_u_machine_init(MachineState *machine)
         break;
     }
 
-    if (riscv_is_32bit(&s->soc.u_cpus)) {
-        firmware_end_addr = riscv_find_and_load_firmware(machine,
-                                    RISCV32_BIOS_BIN, start_addr, NULL);
-    } else {
-        firmware_end_addr = riscv_find_and_load_firmware(machine,
-                                    RISCV64_BIOS_BIN, start_addr, NULL);
-    }
+    firmware_name = riscv_default_firmware_name(&s->soc.u_cpus);
+    firmware_end_addr = riscv_find_and_load_firmware(machine, firmware_name,
+                                                     start_addr, NULL);
 
     if (machine->kernel_filename) {
         kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc.u_cpus,
diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index d96f013e2e..43341c20b6 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -191,6 +191,7 @@ static void spike_board_init(MachineState *machine)
     MemoryRegion *system_memory = get_system_memory();
     MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
     target_ulong firmware_end_addr, kernel_start_addr;
+    const char *firmware_name;
     uint32_t fdt_load_addr;
     uint64_t kernel_entry;
     char *soc_name;
@@ -261,15 +262,10 @@ static void spike_board_init(MachineState *machine)
      * keeping ELF files here was intentional because BIN files don't work
      * for the Spike machine as HTIF emulation depends on ELF parsing.
      */
-    if (riscv_is_32bit(&s->soc[0])) {
-        firmware_end_addr = riscv_find_and_load_firmware(machine,
-                                    RISCV32_BIOS_BIN, memmap[SPIKE_DRAM].base,
-                                    htif_symbol_callback);
-    } else {
-        firmware_end_addr = riscv_find_and_load_firmware(machine,
-                                    RISCV64_BIOS_BIN, memmap[SPIKE_DRAM].base,
-                                    htif_symbol_callback);
-    }
+    firmware_name = riscv_default_firmware_name(&s->soc[0]);
+    firmware_end_addr = riscv_find_and_load_firmware(machine, firmware_name,
+                                                     memmap[SPIKE_DRAM].base,
+                                                     htif_symbol_callback);
 
     /* Load kernel */
     if (machine->kernel_filename) {
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 94ff2a1584..408f7a2256 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1237,6 +1237,7 @@ static void virt_machine_done(Notifier *notifier, void *data)
     MachineState *machine = MACHINE(s);
     target_ulong start_addr = memmap[VIRT_DRAM].base;
     target_ulong firmware_end_addr, kernel_start_addr;
+    const char *firmware_name = riscv_default_firmware_name(&s->soc[0]);
     uint32_t fdt_load_addr;
     uint64_t kernel_entry;
 
@@ -1256,13 +1257,8 @@ static void virt_machine_done(Notifier *notifier, void *data)
         }
     }
 
-    if (riscv_is_32bit(&s->soc[0])) {
-        firmware_end_addr = riscv_find_and_load_firmware(machine,
-                                    RISCV32_BIOS_BIN, start_addr, NULL);
-    } else {
-        firmware_end_addr = riscv_find_and_load_firmware(machine,
-                                    RISCV64_BIOS_BIN, start_addr, NULL);
-    }
+    firmware_end_addr = riscv_find_and_load_firmware(machine, firmware_name,
+                                                     start_addr, NULL);
 
     /*
      * Init fw_cfg.  Must be done before riscv_load_fdt, otherwise the device
diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
index c03e4e74c5..60cf320c88 100644
--- a/include/hw/riscv/boot.h
+++ b/include/hw/riscv/boot.h
@@ -37,6 +37,7 @@ target_ulong riscv_find_and_load_firmware(MachineState *machine,
                                           const char *default_machine_firmware,
                                           hwaddr firmware_load_addr,
                                           symbol_fn_t sym_cb);
+const char *riscv_default_firmware_name(RISCVHartArrayState *harts);
 target_ulong riscv_load_firmware(const char *firmware_filename,
                                  hwaddr firmware_load_addr,
                                  symbol_fn_t sym_cb);
-- 
2.38.1

Re: [PATCH 05/15] hw/riscv/boot.c: introduce riscv_default_firmware_name()

[Alistair Francis]

On Thu, Dec 22, 2022 at 4:28 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Some boards are duplicating the 'riscv_find_and_load_firmware' call
> because the 32 and 64 bits images have different names. Create
> a function to handle this detail instead of hardcoding it in the boards.
>
> Ideally we would bake this logic inside riscv_find_and_load_firmware(),
> or even create a riscv_load_default_firmware(), but at this moment we
> cannot infer whether the machine is running 32 or 64 bits without
> accessing RISCVHartArrayState, which in turn can't be accessed via the
> common code from boot.c. In the end we would exchange 'firmware_name'
> for a flag with riscv_is_32bit(), which isn't much better than what we
> already have today.
>
> 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         |  9 +++++++++
>  hw/riscv/sifive_u.c     | 11 ++++-------
>  hw/riscv/spike.c        | 14 +++++---------
>  hw/riscv/virt.c         | 10 +++-------
>  include/hw/riscv/boot.h |  1 +
>  5 files changed, 22 insertions(+), 23 deletions(-)
>
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 7361d5c0d8..e1a544b1d9 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -75,6 +75,15 @@ target_ulong riscv_calc_kernel_start_addr(RISCVHartArrayState *harts,
>      }
>  }
>
> +const char *riscv_default_firmware_name(RISCVHartArrayState *harts)
> +{
> +    if (riscv_is_32bit(harts)) {
> +        return RISCV32_BIOS_BIN;
> +    }
> +
> +    return RISCV64_BIOS_BIN;
> +}
> +
>  static char *riscv_find_firmware(const char *firmware_filename)
>  {
>      char *filename;
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index 9cf66957ab..ddceb750ea 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -533,6 +533,7 @@ static void sifive_u_machine_init(MachineState *machine)
>      MemoryRegion *flash0 = g_new(MemoryRegion, 1);
>      target_ulong start_addr = memmap[SIFIVE_U_DEV_DRAM].base;
>      target_ulong firmware_end_addr, kernel_start_addr;
> +    const char *firmware_name;
>      uint32_t start_addr_hi32 = 0x00000000;
>      int i;
>      uint32_t fdt_load_addr;
> @@ -595,13 +596,9 @@ static void sifive_u_machine_init(MachineState *machine)
>          break;
>      }
>
> -    if (riscv_is_32bit(&s->soc.u_cpus)) {
> -        firmware_end_addr = riscv_find_and_load_firmware(machine,
> -                                    RISCV32_BIOS_BIN, start_addr, NULL);
> -    } else {
> -        firmware_end_addr = riscv_find_and_load_firmware(machine,
> -                                    RISCV64_BIOS_BIN, start_addr, NULL);
> -    }
> +    firmware_name = riscv_default_firmware_name(&s->soc.u_cpus);
> +    firmware_end_addr = riscv_find_and_load_firmware(machine, firmware_name,
> +                                                     start_addr, NULL);
>
>      if (machine->kernel_filename) {
>          kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc.u_cpus,
> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> index d96f013e2e..43341c20b6 100644
> --- a/hw/riscv/spike.c
> +++ b/hw/riscv/spike.c
> @@ -191,6 +191,7 @@ static void spike_board_init(MachineState *machine)
>      MemoryRegion *system_memory = get_system_memory();
>      MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
>      target_ulong firmware_end_addr, kernel_start_addr;
> +    const char *firmware_name;
>      uint32_t fdt_load_addr;
>      uint64_t kernel_entry;
>      char *soc_name;
> @@ -261,15 +262,10 @@ static void spike_board_init(MachineState *machine)
>       * keeping ELF files here was intentional because BIN files don't work
>       * for the Spike machine as HTIF emulation depends on ELF parsing.
>       */
> -    if (riscv_is_32bit(&s->soc[0])) {
> -        firmware_end_addr = riscv_find_and_load_firmware(machine,
> -                                    RISCV32_BIOS_BIN, memmap[SPIKE_DRAM].base,
> -                                    htif_symbol_callback);
> -    } else {
> -        firmware_end_addr = riscv_find_and_load_firmware(machine,
> -                                    RISCV64_BIOS_BIN, memmap[SPIKE_DRAM].base,
> -                                    htif_symbol_callback);
> -    }
> +    firmware_name = riscv_default_firmware_name(&s->soc[0]);
> +    firmware_end_addr = riscv_find_and_load_firmware(machine, firmware_name,
> +                                                     memmap[SPIKE_DRAM].base,
> +                                                     htif_symbol_callback);
>
>      /* Load kernel */
>      if (machine->kernel_filename) {
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 94ff2a1584..408f7a2256 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -1237,6 +1237,7 @@ static void virt_machine_done(Notifier *notifier, void *data)
>      MachineState *machine = MACHINE(s);
>      target_ulong start_addr = memmap[VIRT_DRAM].base;
>      target_ulong firmware_end_addr, kernel_start_addr;
> +    const char *firmware_name = riscv_default_firmware_name(&s->soc[0]);
>      uint32_t fdt_load_addr;
>      uint64_t kernel_entry;
>
> @@ -1256,13 +1257,8 @@ static void virt_machine_done(Notifier *notifier, void *data)
>          }
>      }
>
> -    if (riscv_is_32bit(&s->soc[0])) {
> -        firmware_end_addr = riscv_find_and_load_firmware(machine,
> -                                    RISCV32_BIOS_BIN, start_addr, NULL);
> -    } else {
> -        firmware_end_addr = riscv_find_and_load_firmware(machine,
> -                                    RISCV64_BIOS_BIN, start_addr, NULL);
> -    }
> +    firmware_end_addr = riscv_find_and_load_firmware(machine, firmware_name,
> +                                                     start_addr, NULL);
>
>      /*
>       * Init fw_cfg.  Must be done before riscv_load_fdt, otherwise the device
> diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
> index c03e4e74c5..60cf320c88 100644
> --- a/include/hw/riscv/boot.h
> +++ b/include/hw/riscv/boot.h
> @@ -37,6 +37,7 @@ target_ulong riscv_find_and_load_firmware(MachineState *machine,
>                                            const char *default_machine_firmware,
>                                            hwaddr firmware_load_addr,
>                                            symbol_fn_t sym_cb);
> +const char *riscv_default_firmware_name(RISCVHartArrayState *harts);
>  target_ulong riscv_load_firmware(const char *firmware_filename,
>                                   hwaddr firmware_load_addr,
>                                   symbol_fn_t sym_cb);
> --
> 2.38.1
>
>

Re: [PATCH 05/15] hw/riscv/boot.c: introduce riscv_default_firmware_name()

[Bin Meng]

On Thu, Dec 22, 2022 at 2:28 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Some boards are duplicating the 'riscv_find_and_load_firmware' call
> because the 32 and 64 bits images have different names. Create
> a function to handle this detail instead of hardcoding it in the boards.
>
> Ideally we would bake this logic inside riscv_find_and_load_firmware(),
> or even create a riscv_load_default_firmware(), but at this moment we
> cannot infer whether the machine is running 32 or 64 bits without
> accessing RISCVHartArrayState, which in turn can't be accessed via the
> common code from boot.c. In the end we would exchange 'firmware_name'
> for a flag with riscv_is_32bit(), which isn't much better than what we
> already have today.
>
> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  hw/riscv/boot.c         |  9 +++++++++
>  hw/riscv/sifive_u.c     | 11 ++++-------
>  hw/riscv/spike.c        | 14 +++++---------
>  hw/riscv/virt.c         | 10 +++-------
>  include/hw/riscv/boot.h |  1 +
>  5 files changed, 22 insertions(+), 23 deletions(-)
>

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

[PATCH 06/15] 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>
---
 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 43341c20b6..f37a9bebbf 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -257,6 +257,10 @@ static void spike_board_init(MachineState *machine)
     memory_region_add_subregion(system_memory, memmap[SPIKE_MROM].base,
                                 mask_rom);
 
+    /* Create device tree */
+    create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline,
+               riscv_is_32bit(&s->soc[0]));
+
     /*
      * Not like other RISC-V machines that use plain binary bios images,
      * keeping ELF files here was intentional because BIN files don't work
@@ -275,6 +279,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
@@ -283,22 +298,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]));
-
-    /* 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.38.1

Re: [PATCH 06/15] hw/riscv/spike.c: load initrd right after riscv_load_kernel()

[Philippe Mathieu-Daudé]

On 21/12/22 19:22, Daniel Henrique Barboza wrote:
> 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>
> ---
>   hw/riscv/spike.c | 31 +++++++++++++++----------------
>   1 file changed, 15 insertions(+), 16 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 06/15] hw/riscv/spike.c: load initrd right after riscv_load_kernel()

[Alistair Francis]

On Thu, Dec 22, 2022 at 4:28 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> 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: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  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 43341c20b6..f37a9bebbf 100644
> --- a/hw/riscv/spike.c
> +++ b/hw/riscv/spike.c
> @@ -257,6 +257,10 @@ static void spike_board_init(MachineState *machine)
>      memory_region_add_subregion(system_memory, memmap[SPIKE_MROM].base,
>                                  mask_rom);
>
> +    /* Create device tree */
> +    create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline,
> +               riscv_is_32bit(&s->soc[0]));
> +
>      /*
>       * Not like other RISC-V machines that use plain binary bios images,
>       * keeping ELF files here was intentional because BIN files don't work
> @@ -275,6 +279,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
> @@ -283,22 +298,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]));
> -
> -    /* 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.38.1
>
>

Re: [PATCH 06/15] hw/riscv/spike.c: load initrd right after riscv_load_kernel()

[Bin Meng]

On Thu, Dec 22, 2022 at 2:28 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> 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>
> ---
>  hw/riscv/spike.c | 31 +++++++++++++++----------------
>  1 file changed, 15 insertions(+), 16 deletions(-)
>

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

Re: [PATCH 06/15] hw/riscv/spike.c: load initrd right after riscv_load_kernel()

[Bin Meng]

On Fri, Dec 23, 2022 at 6:04 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Thu, Dec 22, 2022 at 2:28 AM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
> >
> > 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>
> > ---
> >  hw/riscv/spike.c | 31 +++++++++++++++----------------
> >  1 file changed, 15 insertions(+), 16 deletions(-)
> >
>
> Reviewed-by: Bin Meng <bmeng@tinylab.org>

This change unfortunately breaks the ELF boot on Spike.

I will propose a patch to fix such unexpected dependency.

Regards,
Bin

Re: [PATCH 06/15] hw/riscv/spike.c: load initrd right after riscv_load_kernel()

[Daniel Henrique Barboza]

On 12/26/22 10:49, Bin Meng wrote:
> On Fri, Dec 23, 2022 at 6:04 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>> On Thu, Dec 22, 2022 at 2:28 AM Daniel Henrique Barboza
>> <dbarboza@ventanamicro.com> wrote:
>>> 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>
>>> ---
>>>   hw/riscv/spike.c | 31 +++++++++++++++----------------
>>>   1 file changed, 15 insertions(+), 16 deletions(-)
>>>
>> Reviewed-by: Bin Meng <bmeng@tinylab.org>
> This change unfortunately breaks the ELF boot on Spike.
>
> I will propose a patch to fix such unexpected dependency.

Interesting.  This is one of the most benign changes I did, or so I thought.

I believe we should wait for you patch fixing it first.


Daniel


>
> Regards,
> Bin

[PATCH 07/15] 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>
---
 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 e1a544b1d9..8aed803d8c 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -193,9 +193,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;
 
     /*
@@ -209,18 +210,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 f37a9bebbf..e08de61205 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -281,14 +281,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 60cf320c88..6f4c606edc 100644
--- a/include/hw/riscv/boot.h
+++ b/include/hw/riscv/boot.h
@@ -44,8 +44,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.38.1

Re: [PATCH 07/15] hw/riscv: write initrd 'chosen' FDT inside riscv_load_initrd()

[Bin Meng]

On Thu, Dec 22, 2022 at 2:24 AM 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

doesn't

> have a FDT.
>
> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  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(-)
>

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

[PATCH 08/15] 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>
---
 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 e08de61205..6d50abd425 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -178,10 +178,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 void spike_board_init(MachineState *machine)
@@ -284,6 +280,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.38.1

Re: [PATCH 08/15] hw/riscv: write bootargs 'chosen' FDT after riscv_load_kernel()

[Bin Meng]

On Thu, Dec 22, 2022 at 2:24 AM 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

being checked

> '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>
> ---
>  hw/riscv/sifive_u.c | 11 +++++------
>  hw/riscv/spike.c    |  9 +++++----
>  hw/riscv/virt.c     | 11 +++++------
>  3 files changed, 15 insertions(+), 16 deletions(-)
>

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

[PATCH 09/15] 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>
---
 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 8aed803d8c..4b46a9c51b 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -193,9 +193,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 6d50abd425..1eeee8f349 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -277,8 +277,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 6f4c606edc..a2861c9431 100644
--- a/include/hw/riscv/boot.h
+++ b/include/hw/riscv/boot.h
@@ -44,8 +44,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.38.1

Re: [PATCH 09/15] hw/riscv/boot.c: use MachineState in riscv_load_initrd()

[Philippe Mathieu-Daudé]

On 21/12/22 19:22, Daniel Henrique Barboza 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>
> ---
>   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(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 09/15] hw/riscv/boot.c: use MachineState in riscv_load_initrd()

[Bin Meng]

On Thu, Dec 22, 2022 at 2:24 AM 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>
> ---
>  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(-)
>

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

[PATCH 10/15] 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>
---
 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 4b46a9c51b..c79a08e6fe 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -160,10 +160,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;
 
     /*
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 1eeee8f349..c9c69f92fc 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -272,8 +272,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 a2861c9431..2256b04986 100644
--- a/include/hw/riscv/boot.h
+++ b/include/hw/riscv/boot.h
@@ -41,7 +41,7 @@ const char *riscv_default_firmware_name(RISCVHartArrayState *harts);
 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.38.1

Re: [PATCH 10/15] hw/riscv/boot.c: use MachineState in riscv_load_kernel()

[Philippe Mathieu-Daudé]

On 21/12/22 19:22, Daniel Henrique Barboza 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>
> ---
>   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(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 10/15] hw/riscv/boot.c: use MachineState in riscv_load_kernel()

[Bin Meng]

On Thu, Dec 22, 2022 at 2:24 AM 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>
> ---
>  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(-)
>

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

[PATCH 11/15] 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. Every other board that uses riscv_load_kernel() will have
this same behavior, including boards that doesn't have a valid FDT, so
we need to take care to not do FDT operations without checking it first.

Cc: Palmer Dabbelt <palmer@dabbelt.com>
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/riscv/boot.c            | 21 ++++++++++++++++++---
 hw/riscv/microchip_pfsoc.c |  9 ---------
 hw/riscv/sifive_u.c        |  9 ---------
 hw/riscv/spike.c           |  9 ---------
 hw/riscv/virt.c            |  9 ---------
 5 files changed, 18 insertions(+), 39 deletions(-)

diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index c79a08e6fe..afe5bae03d 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -166,6 +166,7 @@ target_ulong riscv_load_kernel(MachineState *machine,
 {
     const char *kernel_filename = machine->kernel_filename;
     uint64_t kernel_load_base, kernel_entry;
+    void *fdt = machine->fdt;
 
     /*
      * NB: Use low address not ELF entry point to ensure that the fw_dynamic
@@ -177,21 +178,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 (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..57fd6739a5 100644
--- a/hw/riscv/microchip_pfsoc.c
+++ b/hw/riscv/microchip_pfsoc.c
@@ -631,15 +631,6 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
 
         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);
-        }
-
         /* Compute the fdt load address in dram */
         fdt_load_addr = riscv_load_fdt(memmap[MICROCHIP_PFSOC_DRAM_LO].base,
                                        machine->ram_size, machine->fdt);
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index bac394c959..0c9bf7fe6a 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -599,15 +599,6 @@ static void sifive_u_machine_init(MachineState *machine)
                                                          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);
-        }
     } 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 c9c69f92fc..2b9af5689e 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -274,15 +274,6 @@ static void spike_board_init(MachineState *machine)
 
         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);
-        }
     } 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..11c903a212 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1282,15 +1282,6 @@ static void virt_machine_done(Notifier *notifier, void *data)
                                                          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);
-        }
     } else {
        /*
         * If dynamic firmware is used, it doesn't know where is the next mode
-- 
2.38.1

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

[Bin Meng]

On Thu, Dec 22, 2022 at 2:29 AM 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. Every other board that uses riscv_load_kernel() will have
> this same behavior, including boards that doesn't have a valid FDT, so
> we need to take care to not do FDT operations without checking it first.
>
> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  hw/riscv/boot.c            | 21 ++++++++++++++++++---
>  hw/riscv/microchip_pfsoc.c |  9 ---------
>  hw/riscv/sifive_u.c        |  9 ---------
>  hw/riscv/spike.c           |  9 ---------
>  hw/riscv/virt.c            |  9 ---------
>  5 files changed, 18 insertions(+), 39 deletions(-)
>

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

[PATCH 12/15] hw/riscv/boot.c: make riscv_load_initrd() static

[Daniel Henrique Barboza]

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

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/riscv/boot.c         | 76 ++++++++++++++++++++---------------------
 include/hw/riscv/boot.h |  1 -
 2 files changed, 38 insertions(+), 39 deletions(-)

diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index afe5bae03d..55a3fc1a51 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -160,6 +160,44 @@ 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;
+
+    /*
+     * 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,
                                symbol_fn_t sym_cb)
@@ -209,44 +247,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;
-
-    /*
-     * 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 2256b04986..fde0633573 100644
--- a/include/hw/riscv/boot.h
+++ b/include/hw/riscv/boot.h
@@ -44,7 +44,6 @@ target_ulong riscv_load_firmware(const char *firmware_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);
 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.38.1

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

[Philippe Mathieu-Daudé]

On 21/12/22 19:22, Daniel Henrique Barboza wrote:
> The only remaining caller is riscv_load_kernel() which belongs to the
> same file.
> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>   hw/riscv/boot.c         | 76 ++++++++++++++++++++---------------------
>   include/hw/riscv/boot.h |  1 -
>   2 files changed, 38 insertions(+), 39 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

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

[Bin Meng]

On Thu, Dec 22, 2022 at 2:25 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> The only remaining caller is riscv_load_kernel() which belongs to the
> same file.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  hw/riscv/boot.c         | 76 ++++++++++++++++++++---------------------
>  include/hw/riscv/boot.h |  1 -
>  2 files changed, 38 insertions(+), 39 deletions(-)
>

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

[PATCH 13/15] hw/riscv/spike.c: simplify create_fdt()

[Daniel Henrique Barboza]

'mem_size' and 'cmdline' aren't being used and the MachineState pointer
is being retrieved via a MACHINE() macro.

Remove 'mem_size' and 'cmdline' and add MachineState as a parameter.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/riscv/spike.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index 2b9af5689e..181bf394a0 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -48,15 +48,14 @@ static const MemMapEntry spike_memmap[] = {
     [SPIKE_DRAM] =     { 0x80000000,        0x0 },
 };
 
-static void create_fdt(SpikeState *s, const MemMapEntry *memmap,
-                       uint64_t mem_size, const char *cmdline, bool is_32_bit)
+static void create_fdt(MachineState *mc, SpikeState *s,
+                       const MemMapEntry *memmap, bool is_32_bit)
 {
     void *fdt;
     int fdt_size;
     uint64_t addr, size;
     unsigned long clint_addr;
     int cpu, socket;
-    MachineState *mc = MACHINE(s);
     uint32_t *clint_cells;
     uint32_t cpu_phandle, intc_phandle, phandle = 1;
     char *name, *mem_name, *clint_name, *clust_name;
@@ -254,8 +253,7 @@ static void spike_board_init(MachineState *machine)
                                 mask_rom);
 
     /* Create device tree */
-    create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline,
-               riscv_is_32bit(&s->soc[0]));
+    create_fdt(machine, s, memmap, riscv_is_32bit(&s->soc[0]));
 
     /*
      * Not like other RISC-V machines that use plain binary bios images,
-- 
2.38.1

Re: [PATCH 13/15] hw/riscv/spike.c: simplify create_fdt()

[Bin Meng]

On Thu, Dec 22, 2022 at 2:29 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> 'mem_size' and 'cmdline' aren't being used and the MachineState pointer
> is being retrieved via a MACHINE() macro.
>
> Remove 'mem_size' and 'cmdline' and add MachineState as a parameter.

Why do you add MachineState as a parameter? What's the problem of
using the MACHINE() macro?

>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  hw/riscv/spike.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> index 2b9af5689e..181bf394a0 100644
> --- a/hw/riscv/spike.c
> +++ b/hw/riscv/spike.c
> @@ -48,15 +48,14 @@ static const MemMapEntry spike_memmap[] = {
>      [SPIKE_DRAM] =     { 0x80000000,        0x0 },
>  };
>
> -static void create_fdt(SpikeState *s, const MemMapEntry *memmap,
> -                       uint64_t mem_size, const char *cmdline, bool is_32_bit)
> +static void create_fdt(MachineState *mc, SpikeState *s,
> +                       const MemMapEntry *memmap, bool is_32_bit)
>  {
>      void *fdt;
>      int fdt_size;
>      uint64_t addr, size;
>      unsigned long clint_addr;
>      int cpu, socket;
> -    MachineState *mc = MACHINE(s);
>      uint32_t *clint_cells;
>      uint32_t cpu_phandle, intc_phandle, phandle = 1;
>      char *name, *mem_name, *clint_name, *clust_name;
> @@ -254,8 +253,7 @@ static void spike_board_init(MachineState *machine)
>                                  mask_rom);
>
>      /* Create device tree */
> -    create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline,
> -               riscv_is_32bit(&s->soc[0]));
> +    create_fdt(machine, s, memmap, riscv_is_32bit(&s->soc[0]));
>
>      /*
>       * Not like other RISC-V machines that use plain binary bios images,
> --

Regards,
Bin

Re: [PATCH 13/15] hw/riscv/spike.c: simplify create_fdt()

[Daniel Henrique Barboza]

On 12/23/22 10:06, Bin Meng wrote:
> On Thu, Dec 22, 2022 at 2:29 AM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
>> 'mem_size' and 'cmdline' aren't being used and the MachineState pointer
>> is being retrieved via a MACHINE() macro.
>>
>> Remove 'mem_size' and 'cmdline' and add MachineState as a parameter.
> Why do you add MachineState as a parameter? What's the problem of
> using the MACHINE() macro?

Yeah, I went overboard with the macro removal in this case and in patch 14.
I'll also redo patch 15 to avoid the qdev_get_machine() call but keeping the
macro.



Daniel



>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>>   hw/riscv/spike.c | 8 +++-----
>>   1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
>> index 2b9af5689e..181bf394a0 100644
>> --- a/hw/riscv/spike.c
>> +++ b/hw/riscv/spike.c
>> @@ -48,15 +48,14 @@ static const MemMapEntry spike_memmap[] = {
>>       [SPIKE_DRAM] =     { 0x80000000,        0x0 },
>>   };
>>
>> -static void create_fdt(SpikeState *s, const MemMapEntry *memmap,
>> -                       uint64_t mem_size, const char *cmdline, bool is_32_bit)
>> +static void create_fdt(MachineState *mc, SpikeState *s,
>> +                       const MemMapEntry *memmap, bool is_32_bit)
>>   {
>>       void *fdt;
>>       int fdt_size;
>>       uint64_t addr, size;
>>       unsigned long clint_addr;
>>       int cpu, socket;
>> -    MachineState *mc = MACHINE(s);
>>       uint32_t *clint_cells;
>>       uint32_t cpu_phandle, intc_phandle, phandle = 1;
>>       char *name, *mem_name, *clint_name, *clust_name;
>> @@ -254,8 +253,7 @@ static void spike_board_init(MachineState *machine)
>>                                   mask_rom);
>>
>>       /* Create device tree */
>> -    create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline,
>> -               riscv_is_32bit(&s->soc[0]));
>> +    create_fdt(machine, s, memmap, riscv_is_32bit(&s->soc[0]));
>>
>>       /*
>>        * Not like other RISC-V machines that use plain binary bios images,
>> --
> Regards,
> Bin

[PATCH 14/15] hw/riscv/virt.c: simplify create_fdt()

[Daniel Henrique Barboza]

'mem_size' and 'cmdline' aren't being used and the MachineState pointer
is being retrieved via a MACHINE() macro.

Remove 'mem_size' and 'cmdline' and add MachineState as a parameter.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/riscv/virt.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 11c903a212..b13299cee8 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -998,10 +998,9 @@ static void create_fdt_fw_cfg(RISCVVirtState *s, const MemMapEntry *memmap)
     g_free(nodename);
 }
 
-static void create_fdt(RISCVVirtState *s, const MemMapEntry *memmap,
-                       uint64_t mem_size, const char *cmdline, bool is_32_bit)
+static void create_fdt(MachineState *mc, RISCVVirtState *s,
+                       const MemMapEntry *memmap, bool is_32_bit)
 {
-    MachineState *mc = MACHINE(s);
     uint32_t phandle = 1, irq_mmio_phandle = 1, msi_pcie_phandle = 1;
     uint32_t irq_pcie_phandle = 1, irq_virtio_phandle = 1;
     uint8_t rng_seed[32];
@@ -1498,8 +1497,7 @@ static void virt_machine_init(MachineState *machine)
     virt_flash_map(s, system_memory);
 
     /* create device tree */
-    create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline,
-               riscv_is_32bit(&s->soc[0]));
+    create_fdt(machine, s, memmap, riscv_is_32bit(&s->soc[0]));
 
     s->machine_done.notify = virt_machine_done;
     qemu_add_machine_init_done_notifier(&s->machine_done);
-- 
2.38.1

[PATCH 15/15] hw/riscv/sifive_u: simplify create_fdt()

[Daniel Henrique Barboza]

cmdline' isn't being used. A MachineState pointer is being retrieved via
a MACHINE() macro calling qdev_get_machine(). 'mem_size' is being set as
machine->ram_size.

Remove the 'cmdline' and 'mem_size' parameters, add MachineState as a
parameter to avoid the cast and retrieve 'mem_size' via ms->ram_size.

Cc: Palmer Dabbelt <palmer@dabbelt.com>
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/riscv/sifive_u.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index 0c9bf7fe6a..97a561dc52 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -93,10 +93,10 @@ static const MemMapEntry sifive_u_memmap[] = {
 #define OTP_SERIAL          1
 #define GEM_REVISION        0x10070109
 
-static void create_fdt(SiFiveUState *s, const MemMapEntry *memmap,
-                       uint64_t mem_size, const char *cmdline, bool is_32_bit)
+static void create_fdt(MachineState *ms, SiFiveUState *s,
+                       const MemMapEntry *memmap, bool is_32_bit)
 {
-    MachineState *ms = MACHINE(qdev_get_machine());
+    uint64_t mem_size = ms->ram_size;
     void *fdt;
     int cpu, fdt_size;
     uint32_t *cells;
@@ -560,8 +560,7 @@ static void sifive_u_machine_init(MachineState *machine)
                           qemu_allocate_irq(sifive_u_machine_reset, NULL, 0));
 
     /* create device tree */
-    create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline,
-               riscv_is_32bit(&s->soc.u_cpus));
+    create_fdt(machine, s, memmap, riscv_is_32bit(&s->soc.u_cpus));
 
     if (s->start_in_flash) {
         /*
-- 
2.38.1