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

All of lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH 00/12] hw/riscv: Improve Spike HTIF emulation fidelity
@ 2022-12-27  6:48 Bin Meng
  2022-12-27  6:48 ` [PATCH 01/12] hw/char: riscv_htif: Avoid using magic numbers Bin Meng
                   ` (12 more replies)
  0 siblings, 13 replies; 35+ messages in thread
From: Bin Meng @ 2022-12-27  6:48 UTC (permalink / raw)
  To: Alistair Francis, qemu-devel
  Cc: Daniel Henrique Barboza, Alistair Francis, Anup Patel, Bin Meng,
	Marc-André Lureau, Palmer Dabbelt, Paolo Bonzini,
	qemu-riscv

At present the 32-bit OpenSBI generic firmware image does not boot on
Spike, only 64-bit image can. This is due to the HTIF emulation does
not implement the proxy syscall interface which is required for the
32-bit HTIF console output.

An OpenSBI bug fix [1] is also needed when booting the plain binary image.

With this series plus the above OpenSBI fix, both 32-bit OpenSBI BIN & ELF
images can boot on QEMU 'spike' machine.

[1] https://patchwork.ozlabs.org/project/opensbi/patch/20221226033603.1860569-1-bmeng@tinylab.org/


Bin Meng (10):
  hw/char: riscv_htif: Avoid using magic numbers
  hw/char: riscv_htif: Drop {to,from}host_size in HTIFState
  hw/char: riscv_htif: Drop useless assignment of memory region
  hw/char: riscv_htif: Use conventional 's' for HTIFState
  hw/char: riscv_htif: Move registers from CPUArchState to HTIFState
  hw/char: riscv_htif: Remove forward declarations for non-existent
    variables
  hw/char: riscv_htif: Support console output via proxy syscall
  hw/riscv: spike: Remove the out-of-date comments
  hw/riscv/boot.c: Introduce riscv_find_firmware()
  hw/riscv: spike: Decouple create_fdt() dependency to ELF loading

Daniel Henrique Barboza (2):
  hw/riscv/boot.c: make riscv_find_firmware() static
  hw/riscv/boot.c: introduce riscv_default_firmware_name()

 include/hw/char/riscv_htif.h |  19 +---
 include/hw/riscv/boot.h      |   4 +-
 target/riscv/cpu.h           |   4 -
 hw/char/riscv_htif.c         | 172 +++++++++++++++++++++--------------
 hw/riscv/boot.c              |  76 ++++++++++------
 hw/riscv/sifive_u.c          |  11 +--
 hw/riscv/spike.c             |  59 ++++++++----
 hw/riscv/virt.c              |  10 +-
 target/riscv/machine.c       |   6 +-
 9 files changed, 212 insertions(+), 149 deletions(-)

-- 
2.34.1



^ permalink raw reply	[flat|nested] 35+ messages in thread

* [PATCH 01/12] hw/char: riscv_htif: Avoid using magic numbers
  2022-12-27  6:48 [PATCH 00/12] hw/riscv: Improve Spike HTIF emulation fidelity Bin Meng
@ 2022-12-27  6:48 ` Bin Meng
  2022-12-27 17:28   ` Daniel Henrique Barboza
  2022-12-28  3:31   ` Alistair Francis
  2022-12-27  6:48 ` [PATCH 02/12] hw/char: riscv_htif: Drop {to, from}host_size in HTIFState Bin Meng
                   ` (11 subsequent siblings)
  12 siblings, 2 replies; 35+ messages in thread
From: Bin Meng @ 2022-12-27  6:48 UTC (permalink / raw)
  To: Alistair Francis, qemu-devel
  Cc: Daniel Henrique Barboza, Marc-André Lureau, Paolo Bonzini

The Spike HTIF is poorly documented. The only relevant info we can
get from the internet is from Andrew Waterman at [1].

Add a comment block before htif_handle_tohost_write() to explain
the tohost register format, and use meaningful macros intead of
magic numbers in the codes.

While we are here, corret 2 multi-line comment blocks that have
wrong format.

Link: https://github.com/riscv-software-src/riscv-isa-sim/issues/364 [1]
Signed-off-by: Bin Meng <bmeng@tinylab.org>
---

 hw/char/riscv_htif.c | 72 ++++++++++++++++++++++++++++++++------------
 1 file changed, 52 insertions(+), 20 deletions(-)

diff --git a/hw/char/riscv_htif.c b/hw/char/riscv_htif.c
index 6577f0e640..088556bb04 100644
--- a/hw/char/riscv_htif.c
+++ b/hw/char/riscv_htif.c
@@ -38,6 +38,16 @@
         }                                                                      \
     } while (0)
 
+#define HTIF_DEV_SHIFT          56
+#define HTIF_CMD_SHIFT          48
+
+#define HTIF_DEV_SYSTEM         0
+#define HTIF_DEV_CONSOLE        1
+
+#define HTIF_SYSTEM_CMD_SYSCALL 0
+#define HTIF_CONSOLE_CMD_GETC   0
+#define HTIF_CONSOLE_CMD_PUTC   1
+
 static uint64_t fromhost_addr, tohost_addr;
 static int address_symbol_set;
 
@@ -81,9 +91,11 @@ static void htif_recv(void *opaque, const uint8_t *buf, int size)
         return;
     }
 
-    /* TODO - we need to check whether mfromhost is zero which indicates
-              the device is ready to receive. The current implementation
-              will drop characters */
+    /*
+     * TODO - we need to check whether mfromhost is zero which indicates
+     *        the device is ready to receive. The current implementation
+     *        will drop characters
+     */
 
     uint64_t val_written = htifstate->pending_read;
     uint64_t resp = 0x100 | *buf;
@@ -110,10 +122,30 @@ static int htif_be_change(void *opaque)
     return 0;
 }
 
+/*
+ * See below the tohost register format.
+ *
+ * Bits 63:56 indicate the "device".
+ * Bits 55:48 indicate the "command".
+ *
+ * Device 0 is the syscall device, which is used to emulate Unixy syscalls.
+ * It only implements command 0, which has two subfunctions:
+ * - If bit 0 is clear, then bits 47:0 represent a pointer to a struct
+ *   describing the syscall.
+ * - If bit 1 is set, then bits 47:1 represent an exit code, with a zero
+ *   value indicating success and other values indicating failure.
+ *
+ * Device 1 is the blocking character device.
+ * - Command 0 reads a character
+ * - Command 1 writes a character from the 8 LSBs of tohost
+ *
+ * For RV32, the tohost register is zero-extended, so only device=0 and
+ * command=0 (i.e. HTIF syscalls/exit codes) are supported.
+ */
 static void htif_handle_tohost_write(HTIFState *htifstate, uint64_t val_written)
 {
-    uint8_t device = val_written >> 56;
-    uint8_t cmd = val_written >> 48;
+    uint8_t device = val_written >> HTIF_DEV_SHIFT;
+    uint8_t cmd = val_written >> HTIF_CMD_SHIFT;
     uint64_t payload = val_written & 0xFFFFFFFFFFFFULL;
     int resp = 0;
 
@@ -125,9 +157,9 @@ static void htif_handle_tohost_write(HTIFState *htifstate, uint64_t val_written)
      * 0: riscv-tests Pass/Fail Reporting Only (no syscall proxy)
      * 1: Console
      */
-    if (unlikely(device == 0x0)) {
+    if (unlikely(device == HTIF_DEV_SYSTEM)) {
         /* frontend syscall handler, shutdown and exit code support */
-        if (cmd == 0x0) {
+        if (cmd == HTIF_SYSTEM_CMD_SYSCALL) {
             if (payload & 0x1) {
                 /* exit code */
                 int exit_code = payload >> 1;
@@ -138,14 +170,14 @@ static void htif_handle_tohost_write(HTIFState *htifstate, uint64_t val_written)
         } else {
             qemu_log("HTIF device %d: unknown command\n", device);
         }
-    } else if (likely(device == 0x1)) {
+    } else if (likely(device == HTIF_DEV_CONSOLE)) {
         /* HTIF Console */
-        if (cmd == 0x0) {
+        if (cmd == HTIF_CONSOLE_CMD_GETC) {
             /* this should be a queue, but not yet implemented as such */
             htifstate->pending_read = val_written;
             htifstate->env->mtohost = 0; /* clear to indicate we read */
             return;
-        } else if (cmd == 0x1) {
+        } else if (cmd == HTIF_CONSOLE_CMD_PUTC) {
             qemu_chr_fe_write(&htifstate->chr, (uint8_t *)&payload, 1);
             resp = 0x100 | (uint8_t)payload;
         } else {
@@ -157,15 +189,15 @@ static void htif_handle_tohost_write(HTIFState *htifstate, uint64_t val_written)
             " payload: %016" PRIx64, device, cmd, payload & 0xFF, payload);
     }
     /*
-     * - latest bbl does not set fromhost to 0 if there is a value in tohost
-     * - with this code enabled, qemu hangs waiting for fromhost to go to 0
-     * - with this code disabled, qemu works with bbl priv v1.9.1 and v1.10
-     * - HTIF needs protocol documentation and a more complete state machine
-
-        while (!htifstate->fromhost_inprogress &&
-            htifstate->env->mfromhost != 0x0) {
-        }
-    */
+     * Latest bbl does not set fromhost to 0 if there is a value in tohost.
+     * With this code enabled, qemu hangs waiting for fromhost to go to 0.
+     * With this code disabled, qemu works with bbl priv v1.9.1 and v1.10.
+     * HTIF needs protocol documentation and a more complete state machine.
+     *
+     *  while (!htifstate->fromhost_inprogress &&
+     *      htifstate->env->mfromhost != 0x0) {
+     *  }
+     */
     htifstate->env->mfromhost = (val_written >> 48 << 48) | (resp << 16 >> 16);
     htifstate->env->mtohost = 0; /* clear to indicate we read */
 }
@@ -196,7 +228,7 @@ static uint64_t htif_mm_read(void *opaque, hwaddr addr, unsigned size)
 
 /* CPU wrote to an HTIF register */
 static void htif_mm_write(void *opaque, hwaddr addr,
-                            uint64_t value, unsigned size)
+                          uint64_t value, unsigned size)
 {
     HTIFState *htifstate = opaque;
     if (addr == TOHOST_OFFSET1) {
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH 02/12] hw/char: riscv_htif: Drop {to, from}host_size in HTIFState
  2022-12-27  6:48 [PATCH 00/12] hw/riscv: Improve Spike HTIF emulation fidelity Bin Meng
  2022-12-27  6:48 ` [PATCH 01/12] hw/char: riscv_htif: Avoid using magic numbers Bin Meng
@ 2022-12-27  6:48 ` Bin Meng
  2022-12-27 17:29   ` [PATCH 02/12] hw/char: riscv_htif: Drop {to,from}host_size " Daniel Henrique Barboza
  2022-12-28  3:32   ` [PATCH 02/12] hw/char: riscv_htif: Drop {to, from}host_size " Alistair Francis
  2022-12-27  6:48 ` [PATCH 03/12] hw/char: riscv_htif: Drop useless assignment of memory region Bin Meng
                   ` (10 subsequent siblings)
  12 siblings, 2 replies; 35+ messages in thread
From: Bin Meng @ 2022-12-27  6:48 UTC (permalink / raw)
  To: Alistair Francis, qemu-devel
  Cc: Daniel Henrique Barboza, Alistair Francis, Anup Patel

These are not used anywhere. Drop them.

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

 include/hw/char/riscv_htif.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/include/hw/char/riscv_htif.h b/include/hw/char/riscv_htif.h
index f888ac1b30..3eccc1914f 100644
--- a/include/hw/char/riscv_htif.h
+++ b/include/hw/char/riscv_htif.h
@@ -33,8 +33,6 @@ typedef struct HTIFState {
 
     hwaddr tohost_offset;
     hwaddr fromhost_offset;
-    uint64_t tohost_size;
-    uint64_t fromhost_size;
     MemoryRegion mmio;
     MemoryRegion *address_space;
     MemoryRegion *main_mem;
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH 03/12] hw/char: riscv_htif: Drop useless assignment of memory region
  2022-12-27  6:48 [PATCH 00/12] hw/riscv: Improve Spike HTIF emulation fidelity Bin Meng
  2022-12-27  6:48 ` [PATCH 01/12] hw/char: riscv_htif: Avoid using magic numbers Bin Meng
  2022-12-27  6:48 ` [PATCH 02/12] hw/char: riscv_htif: Drop {to, from}host_size in HTIFState Bin Meng
@ 2022-12-27  6:48 ` Bin Meng
  2022-12-27 17:30   ` Daniel Henrique Barboza
  2022-12-28  3:33   ` Alistair Francis
  2022-12-27  6:48 ` [PATCH 04/12] hw/char: riscv_htif: Use conventional 's' for HTIFState Bin Meng
                   ` (9 subsequent siblings)
  12 siblings, 2 replies; 35+ messages in thread
From: Bin Meng @ 2022-12-27  6:48 UTC (permalink / raw)
  To: Alistair Francis, qemu-devel
  Cc: Daniel Henrique Barboza, Alistair Francis, Bin Meng,
	Marc-André Lureau, Palmer Dabbelt, Paolo Bonzini,
	qemu-riscv

struct HTIFState has 3 members for address space and memory region,
and are initialized during htif_mm_init(). But they are actually
useless. Drop them.

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

 include/hw/char/riscv_htif.h | 7 ++-----
 hw/char/riscv_htif.c         | 7 ++-----
 hw/riscv/spike.c             | 5 ++---
 3 files changed, 6 insertions(+), 13 deletions(-)

diff --git a/include/hw/char/riscv_htif.h b/include/hw/char/riscv_htif.h
index 3eccc1914f..6d172ebd6d 100644
--- a/include/hw/char/riscv_htif.h
+++ b/include/hw/char/riscv_htif.h
@@ -34,9 +34,6 @@ typedef struct HTIFState {
     hwaddr tohost_offset;
     hwaddr fromhost_offset;
     MemoryRegion mmio;
-    MemoryRegion *address_space;
-    MemoryRegion *main_mem;
-    void *main_mem_ram_ptr;
 
     CPURISCVState *env;
     CharBackend chr;
@@ -54,7 +51,7 @@ void htif_symbol_callback(const char *st_name, int st_info, uint64_t st_value,
 bool htif_uses_elf_symbols(void);
 
 /* legacy pre qom */
-HTIFState *htif_mm_init(MemoryRegion *address_space, MemoryRegion *main_mem,
-    CPURISCVState *env, Chardev *chr, uint64_t nonelf_base);
+HTIFState *htif_mm_init(MemoryRegion *address_space, CPURISCVState *env,
+                        Chardev *chr, uint64_t nonelf_base);
 
 #endif
diff --git a/hw/char/riscv_htif.c b/hw/char/riscv_htif.c
index 088556bb04..e7e319ca1d 100644
--- a/hw/char/riscv_htif.c
+++ b/hw/char/riscv_htif.c
@@ -265,8 +265,8 @@ bool htif_uses_elf_symbols(void)
     return (address_symbol_set == 3) ? true : false;
 }
 
-HTIFState *htif_mm_init(MemoryRegion *address_space, MemoryRegion *main_mem,
-    CPURISCVState *env, Chardev *chr, uint64_t nonelf_base)
+HTIFState *htif_mm_init(MemoryRegion *address_space, CPURISCVState *env,
+                        Chardev *chr, uint64_t nonelf_base)
 {
     uint64_t base, size, tohost_offset, fromhost_offset;
 
@@ -281,9 +281,6 @@ HTIFState *htif_mm_init(MemoryRegion *address_space, MemoryRegion *main_mem,
     fromhost_offset = fromhost_addr - base;
 
     HTIFState *s = g_new0(HTIFState, 1);
-    s->address_space = address_space;
-    s->main_mem = main_mem;
-    s->main_mem_ram_ptr = memory_region_get_ram_ptr(main_mem);
     s->env = env;
     s->tohost_offset = tohost_offset;
     s->fromhost_offset = fromhost_offset;
diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index 1e1d752c00..82cf41ac27 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -317,9 +317,8 @@ static void spike_board_init(MachineState *machine)
                               fdt_load_addr);
 
     /* initialize HTIF using symbols found in load_kernel */
-    htif_mm_init(system_memory, mask_rom,
-                 &s->soc[0].harts[0].env, serial_hd(0),
-                 memmap[SPIKE_HTIF].base);
+    htif_mm_init(system_memory, &s->soc[0].harts[0].env,
+                 serial_hd(0), memmap[SPIKE_HTIF].base);
 }
 
 static void spike_machine_instance_init(Object *obj)
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH 04/12] hw/char: riscv_htif: Use conventional 's' for HTIFState
  2022-12-27  6:48 [PATCH 00/12] hw/riscv: Improve Spike HTIF emulation fidelity Bin Meng
                   ` (2 preceding siblings ...)
  2022-12-27  6:48 ` [PATCH 03/12] hw/char: riscv_htif: Drop useless assignment of memory region Bin Meng
@ 2022-12-27  6:48 ` Bin Meng
  2022-12-27 17:32   ` Daniel Henrique Barboza
  2022-12-28  3:35   ` Alistair Francis
  2022-12-27  6:48 ` [PATCH 05/12] hw/char: riscv_htif: Move registers from CPUArchState to HTIFState Bin Meng
                   ` (8 subsequent siblings)
  12 siblings, 2 replies; 35+ messages in thread
From: Bin Meng @ 2022-12-27  6:48 UTC (permalink / raw)
  To: Alistair Francis, qemu-devel
  Cc: Daniel Henrique Barboza, Marc-André Lureau, Paolo Bonzini

QEMU source codes tend to use 's' to represent the hardware state.
Let's use it for HTIFState.

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

 hw/char/riscv_htif.c | 64 ++++++++++++++++++++++----------------------
 1 file changed, 32 insertions(+), 32 deletions(-)

diff --git a/hw/char/riscv_htif.c b/hw/char/riscv_htif.c
index e7e319ca1d..f28976b110 100644
--- a/hw/char/riscv_htif.c
+++ b/hw/char/riscv_htif.c
@@ -85,7 +85,7 @@ static int htif_can_recv(void *opaque)
  */
 static void htif_recv(void *opaque, const uint8_t *buf, int size)
 {
-    HTIFState *htifstate = opaque;
+    HTIFState *s = opaque;
 
     if (size != 1) {
         return;
@@ -97,10 +97,10 @@ static void htif_recv(void *opaque, const uint8_t *buf, int size)
      *        will drop characters
      */
 
-    uint64_t val_written = htifstate->pending_read;
+    uint64_t val_written = s->pending_read;
     uint64_t resp = 0x100 | *buf;
 
-    htifstate->env->mfromhost = (val_written >> 48 << 48) | (resp << 16 >> 16);
+    s->env->mfromhost = (val_written >> 48 << 48) | (resp << 16 >> 16);
 }
 
 /*
@@ -142,7 +142,7 @@ static int htif_be_change(void *opaque)
  * For RV32, the tohost register is zero-extended, so only device=0 and
  * command=0 (i.e. HTIF syscalls/exit codes) are supported.
  */
-static void htif_handle_tohost_write(HTIFState *htifstate, uint64_t val_written)
+static void htif_handle_tohost_write(HTIFState *s, uint64_t val_written)
 {
     uint8_t device = val_written >> HTIF_DEV_SHIFT;
     uint8_t cmd = val_written >> HTIF_CMD_SHIFT;
@@ -174,11 +174,11 @@ static void htif_handle_tohost_write(HTIFState *htifstate, uint64_t val_written)
         /* HTIF Console */
         if (cmd == HTIF_CONSOLE_CMD_GETC) {
             /* this should be a queue, but not yet implemented as such */
-            htifstate->pending_read = val_written;
-            htifstate->env->mtohost = 0; /* clear to indicate we read */
+            s->pending_read = val_written;
+            s->env->mtohost = 0; /* clear to indicate we read */
             return;
         } else if (cmd == HTIF_CONSOLE_CMD_PUTC) {
-            qemu_chr_fe_write(&htifstate->chr, (uint8_t *)&payload, 1);
+            qemu_chr_fe_write(&s->chr, (uint8_t *)&payload, 1);
             resp = 0x100 | (uint8_t)payload;
         } else {
             qemu_log("HTIF device %d: unknown command\n", device);
@@ -194,31 +194,31 @@ static void htif_handle_tohost_write(HTIFState *htifstate, uint64_t val_written)
      * With this code disabled, qemu works with bbl priv v1.9.1 and v1.10.
      * HTIF needs protocol documentation and a more complete state machine.
      *
-     *  while (!htifstate->fromhost_inprogress &&
-     *      htifstate->env->mfromhost != 0x0) {
+     *  while (!s->fromhost_inprogress &&
+     *      s->env->mfromhost != 0x0) {
      *  }
      */
-    htifstate->env->mfromhost = (val_written >> 48 << 48) | (resp << 16 >> 16);
-    htifstate->env->mtohost = 0; /* clear to indicate we read */
+    s->env->mfromhost = (val_written >> 48 << 48) | (resp << 16 >> 16);
+    s->env->mtohost = 0; /* clear to indicate we read */
 }
 
-#define TOHOST_OFFSET1 (htifstate->tohost_offset)
-#define TOHOST_OFFSET2 (htifstate->tohost_offset + 4)
-#define FROMHOST_OFFSET1 (htifstate->fromhost_offset)
-#define FROMHOST_OFFSET2 (htifstate->fromhost_offset + 4)
+#define TOHOST_OFFSET1      (s->tohost_offset)
+#define TOHOST_OFFSET2      (s->tohost_offset + 4)
+#define FROMHOST_OFFSET1    (s->fromhost_offset)
+#define FROMHOST_OFFSET2    (s->fromhost_offset + 4)
 
 /* CPU wants to read an HTIF register */
 static uint64_t htif_mm_read(void *opaque, hwaddr addr, unsigned size)
 {
-    HTIFState *htifstate = opaque;
+    HTIFState *s = opaque;
     if (addr == TOHOST_OFFSET1) {
-        return htifstate->env->mtohost & 0xFFFFFFFF;
+        return s->env->mtohost & 0xFFFFFFFF;
     } else if (addr == TOHOST_OFFSET2) {
-        return (htifstate->env->mtohost >> 32) & 0xFFFFFFFF;
+        return (s->env->mtohost >> 32) & 0xFFFFFFFF;
     } else if (addr == FROMHOST_OFFSET1) {
-        return htifstate->env->mfromhost & 0xFFFFFFFF;
+        return s->env->mfromhost & 0xFFFFFFFF;
     } else if (addr == FROMHOST_OFFSET2) {
-        return (htifstate->env->mfromhost >> 32) & 0xFFFFFFFF;
+        return (s->env->mfromhost >> 32) & 0xFFFFFFFF;
     } else {
         qemu_log("Invalid htif read: address %016" PRIx64 "\n",
             (uint64_t)addr);
@@ -230,25 +230,25 @@ static uint64_t htif_mm_read(void *opaque, hwaddr addr, unsigned size)
 static void htif_mm_write(void *opaque, hwaddr addr,
                           uint64_t value, unsigned size)
 {
-    HTIFState *htifstate = opaque;
+    HTIFState *s = opaque;
     if (addr == TOHOST_OFFSET1) {
-        if (htifstate->env->mtohost == 0x0) {
-            htifstate->allow_tohost = 1;
-            htifstate->env->mtohost = value & 0xFFFFFFFF;
+        if (s->env->mtohost == 0x0) {
+            s->allow_tohost = 1;
+            s->env->mtohost = value & 0xFFFFFFFF;
         } else {
-            htifstate->allow_tohost = 0;
+            s->allow_tohost = 0;
         }
     } else if (addr == TOHOST_OFFSET2) {
-        if (htifstate->allow_tohost) {
-            htifstate->env->mtohost |= value << 32;
-            htif_handle_tohost_write(htifstate, htifstate->env->mtohost);
+        if (s->allow_tohost) {
+            s->env->mtohost |= value << 32;
+            htif_handle_tohost_write(s, s->env->mtohost);
         }
     } else if (addr == FROMHOST_OFFSET1) {
-        htifstate->fromhost_inprogress = 1;
-        htifstate->env->mfromhost = value & 0xFFFFFFFF;
+        s->fromhost_inprogress = 1;
+        s->env->mfromhost = value & 0xFFFFFFFF;
     } else if (addr == FROMHOST_OFFSET2) {
-        htifstate->env->mfromhost |= value << 32;
-        htifstate->fromhost_inprogress = 0;
+        s->env->mfromhost |= value << 32;
+        s->fromhost_inprogress = 0;
     } else {
         qemu_log("Invalid htif write: address %016" PRIx64 "\n",
             (uint64_t)addr);
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH 05/12] hw/char: riscv_htif: Move registers from CPUArchState to HTIFState
  2022-12-27  6:48 [PATCH 00/12] hw/riscv: Improve Spike HTIF emulation fidelity Bin Meng
                   ` (3 preceding siblings ...)
  2022-12-27  6:48 ` [PATCH 04/12] hw/char: riscv_htif: Use conventional 's' for HTIFState Bin Meng
@ 2022-12-27  6:48 ` Bin Meng
  2022-12-27 17:33   ` Daniel Henrique Barboza
  2022-12-28  4:19   ` Alistair Francis
  2022-12-27  6:48 ` [PATCH 06/12] hw/char: riscv_htif: Remove forward declarations for non-existent variables Bin Meng
                   ` (7 subsequent siblings)
  12 siblings, 2 replies; 35+ messages in thread
From: Bin Meng @ 2022-12-27  6:48 UTC (permalink / raw)
  To: Alistair Francis, qemu-devel
  Cc: Daniel Henrique Barboza, Alistair Francis, Bin Meng,
	Marc-André Lureau, Palmer Dabbelt, Paolo Bonzini,
	qemu-riscv

At present for some unknown reason the HTIF registers (fromhost &
tohost) are defined in the RISC-V CPUArchState. It should really
be put in the HTIFState struct as it is only meaningful to HTIF.

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

 include/hw/char/riscv_htif.h |  8 ++++----
 target/riscv/cpu.h           |  4 ----
 hw/char/riscv_htif.c         | 35 +++++++++++++++++------------------
 hw/riscv/spike.c             |  3 +--
 target/riscv/machine.c       |  6 ++----
 5 files changed, 24 insertions(+), 32 deletions(-)

diff --git a/include/hw/char/riscv_htif.h b/include/hw/char/riscv_htif.h
index 6d172ebd6d..55cc352331 100644
--- a/include/hw/char/riscv_htif.h
+++ b/include/hw/char/riscv_htif.h
@@ -23,7 +23,6 @@
 #include "chardev/char.h"
 #include "chardev/char-fe.h"
 #include "exec/memory.h"
-#include "target/riscv/cpu.h"
 
 #define TYPE_HTIF_UART "riscv.htif.uart"
 
@@ -31,11 +30,12 @@ typedef struct HTIFState {
     int allow_tohost;
     int fromhost_inprogress;
 
+    uint64_t tohost;
+    uint64_t fromhost;
     hwaddr tohost_offset;
     hwaddr fromhost_offset;
     MemoryRegion mmio;
 
-    CPURISCVState *env;
     CharBackend chr;
     uint64_t pending_read;
 } HTIFState;
@@ -51,7 +51,7 @@ void htif_symbol_callback(const char *st_name, int st_info, uint64_t st_value,
 bool htif_uses_elf_symbols(void);
 
 /* legacy pre qom */
-HTIFState *htif_mm_init(MemoryRegion *address_space, CPURISCVState *env,
-                        Chardev *chr, uint64_t nonelf_base);
+HTIFState *htif_mm_init(MemoryRegion *address_space, Chardev *chr,
+                        uint64_t nonelf_base);
 
 #endif
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 443d15a47c..6f04d853dd 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -309,10 +309,6 @@ struct CPUArchState {
     target_ulong sscratch;
     target_ulong mscratch;
 
-    /* temporary htif regs */
-    uint64_t mfromhost;
-    uint64_t mtohost;
-
     /* Sstc CSRs */
     uint64_t stimecmp;
 
diff --git a/hw/char/riscv_htif.c b/hw/char/riscv_htif.c
index f28976b110..3bb0a37a3e 100644
--- a/hw/char/riscv_htif.c
+++ b/hw/char/riscv_htif.c
@@ -100,7 +100,7 @@ static void htif_recv(void *opaque, const uint8_t *buf, int size)
     uint64_t val_written = s->pending_read;
     uint64_t resp = 0x100 | *buf;
 
-    s->env->mfromhost = (val_written >> 48 << 48) | (resp << 16 >> 16);
+    s->fromhost = (val_written >> 48 << 48) | (resp << 16 >> 16);
 }
 
 /*
@@ -175,7 +175,7 @@ static void htif_handle_tohost_write(HTIFState *s, uint64_t val_written)
         if (cmd == HTIF_CONSOLE_CMD_GETC) {
             /* this should be a queue, but not yet implemented as such */
             s->pending_read = val_written;
-            s->env->mtohost = 0; /* clear to indicate we read */
+            s->tohost = 0; /* clear to indicate we read */
             return;
         } else if (cmd == HTIF_CONSOLE_CMD_PUTC) {
             qemu_chr_fe_write(&s->chr, (uint8_t *)&payload, 1);
@@ -195,11 +195,11 @@ static void htif_handle_tohost_write(HTIFState *s, uint64_t val_written)
      * HTIF needs protocol documentation and a more complete state machine.
      *
      *  while (!s->fromhost_inprogress &&
-     *      s->env->mfromhost != 0x0) {
+     *      s->fromhost != 0x0) {
      *  }
      */
-    s->env->mfromhost = (val_written >> 48 << 48) | (resp << 16 >> 16);
-    s->env->mtohost = 0; /* clear to indicate we read */
+    s->fromhost = (val_written >> 48 << 48) | (resp << 16 >> 16);
+    s->tohost = 0; /* clear to indicate we read */
 }
 
 #define TOHOST_OFFSET1      (s->tohost_offset)
@@ -212,13 +212,13 @@ static uint64_t htif_mm_read(void *opaque, hwaddr addr, unsigned size)
 {
     HTIFState *s = opaque;
     if (addr == TOHOST_OFFSET1) {
-        return s->env->mtohost & 0xFFFFFFFF;
+        return s->tohost & 0xFFFFFFFF;
     } else if (addr == TOHOST_OFFSET2) {
-        return (s->env->mtohost >> 32) & 0xFFFFFFFF;
+        return (s->tohost >> 32) & 0xFFFFFFFF;
     } else if (addr == FROMHOST_OFFSET1) {
-        return s->env->mfromhost & 0xFFFFFFFF;
+        return s->fromhost & 0xFFFFFFFF;
     } else if (addr == FROMHOST_OFFSET2) {
-        return (s->env->mfromhost >> 32) & 0xFFFFFFFF;
+        return (s->fromhost >> 32) & 0xFFFFFFFF;
     } else {
         qemu_log("Invalid htif read: address %016" PRIx64 "\n",
             (uint64_t)addr);
@@ -232,22 +232,22 @@ static void htif_mm_write(void *opaque, hwaddr addr,
 {
     HTIFState *s = opaque;
     if (addr == TOHOST_OFFSET1) {
-        if (s->env->mtohost == 0x0) {
+        if (s->tohost == 0x0) {
             s->allow_tohost = 1;
-            s->env->mtohost = value & 0xFFFFFFFF;
+            s->tohost = value & 0xFFFFFFFF;
         } else {
             s->allow_tohost = 0;
         }
     } else if (addr == TOHOST_OFFSET2) {
         if (s->allow_tohost) {
-            s->env->mtohost |= value << 32;
-            htif_handle_tohost_write(s, s->env->mtohost);
+            s->tohost |= value << 32;
+            htif_handle_tohost_write(s, s->tohost);
         }
     } else if (addr == FROMHOST_OFFSET1) {
         s->fromhost_inprogress = 1;
-        s->env->mfromhost = value & 0xFFFFFFFF;
+        s->fromhost = value & 0xFFFFFFFF;
     } else if (addr == FROMHOST_OFFSET2) {
-        s->env->mfromhost |= value << 32;
+        s->fromhost |= value << 32;
         s->fromhost_inprogress = 0;
     } else {
         qemu_log("Invalid htif write: address %016" PRIx64 "\n",
@@ -265,8 +265,8 @@ bool htif_uses_elf_symbols(void)
     return (address_symbol_set == 3) ? true : false;
 }
 
-HTIFState *htif_mm_init(MemoryRegion *address_space, CPURISCVState *env,
-                        Chardev *chr, uint64_t nonelf_base)
+HTIFState *htif_mm_init(MemoryRegion *address_space, Chardev *chr,
+                        uint64_t nonelf_base)
 {
     uint64_t base, size, tohost_offset, fromhost_offset;
 
@@ -281,7 +281,6 @@ HTIFState *htif_mm_init(MemoryRegion *address_space, CPURISCVState *env,
     fromhost_offset = fromhost_addr - base;
 
     HTIFState *s = g_new0(HTIFState, 1);
-    s->env = env;
     s->tohost_offset = tohost_offset;
     s->fromhost_offset = fromhost_offset;
     s->pending_read = 0;
diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index 82cf41ac27..8606331f61 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -317,8 +317,7 @@ static void spike_board_init(MachineState *machine)
                               fdt_load_addr);
 
     /* initialize HTIF using symbols found in load_kernel */
-    htif_mm_init(system_memory, &s->soc[0].harts[0].env,
-                 serial_hd(0), memmap[SPIKE_HTIF].base);
+    htif_mm_init(system_memory, serial_hd(0), memmap[SPIKE_HTIF].base);
 }
 
 static void spike_machine_instance_init(Object *obj)
diff --git a/target/riscv/machine.c b/target/riscv/machine.c
index c2a94a82b3..2e8beef06e 100644
--- a/target/riscv/machine.c
+++ b/target/riscv/machine.c
@@ -298,8 +298,8 @@ static const VMStateDescription vmstate_pmu_ctr_state = {
 
 const VMStateDescription vmstate_riscv_cpu = {
     .name = "cpu",
-    .version_id = 5,
-    .minimum_version_id = 5,
+    .version_id = 6,
+    .minimum_version_id = 6,
     .post_load = riscv_cpu_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_UINTTL_ARRAY(env.gpr, RISCVCPU, 32),
@@ -349,8 +349,6 @@ const VMStateDescription vmstate_riscv_cpu = {
         VMSTATE_UINTTL_ARRAY(env.mhpmeventh_val, RISCVCPU, RV_MAX_MHPMEVENTS),
         VMSTATE_UINTTL(env.sscratch, RISCVCPU),
         VMSTATE_UINTTL(env.mscratch, RISCVCPU),
-        VMSTATE_UINT64(env.mfromhost, RISCVCPU),
-        VMSTATE_UINT64(env.mtohost, RISCVCPU),
         VMSTATE_UINT64(env.stimecmp, RISCVCPU),
 
         VMSTATE_END_OF_LIST()
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH 06/12] hw/char: riscv_htif: Remove forward declarations for non-existent variables
  2022-12-27  6:48 [PATCH 00/12] hw/riscv: Improve Spike HTIF emulation fidelity Bin Meng
                   ` (4 preceding siblings ...)
  2022-12-27  6:48 ` [PATCH 05/12] hw/char: riscv_htif: Move registers from CPUArchState to HTIFState Bin Meng
@ 2022-12-27  6:48 ` Bin Meng
  2022-12-27 17:35   ` Daniel Henrique Barboza
  2022-12-28  4:22   ` Alistair Francis
  2022-12-27  6:48 ` [PATCH 07/12] hw/char: riscv_htif: Support console output via proxy syscall Bin Meng
                   ` (6 subsequent siblings)
  12 siblings, 2 replies; 35+ messages in thread
From: Bin Meng @ 2022-12-27  6:48 UTC (permalink / raw)
  To: Alistair Francis, qemu-devel
  Cc: Daniel Henrique Barboza, Alistair Francis, Anup Patel

There are forward declarations for 'vmstate_htif' and 'htif_io_ops'
in riscv_htif.h however there are no definitions in the C codes.

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

 include/hw/char/riscv_htif.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/include/hw/char/riscv_htif.h b/include/hw/char/riscv_htif.h
index 55cc352331..9e8ebbe017 100644
--- a/include/hw/char/riscv_htif.h
+++ b/include/hw/char/riscv_htif.h
@@ -40,9 +40,6 @@ typedef struct HTIFState {
     uint64_t pending_read;
 } HTIFState;
 
-extern const VMStateDescription vmstate_htif;
-extern const MemoryRegionOps htif_io_ops;
-
 /* HTIF symbol callback */
 void htif_symbol_callback(const char *st_name, int st_info, uint64_t st_value,
     uint64_t st_size);
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH 07/12] hw/char: riscv_htif: Support console output via proxy syscall
  2022-12-27  6:48 [PATCH 00/12] hw/riscv: Improve Spike HTIF emulation fidelity Bin Meng
                   ` (5 preceding siblings ...)
  2022-12-27  6:48 ` [PATCH 06/12] hw/char: riscv_htif: Remove forward declarations for non-existent variables Bin Meng
@ 2022-12-27  6:48 ` Bin Meng
  2022-12-27 17:37   ` Daniel Henrique Barboza
  2022-12-28  4:30   ` Alistair Francis
  2022-12-27  6:48 ` [PATCH 08/12] hw/riscv: spike: Remove the out-of-date comments Bin Meng
                   ` (5 subsequent siblings)
  12 siblings, 2 replies; 35+ messages in thread
From: Bin Meng @ 2022-12-27  6:48 UTC (permalink / raw)
  To: Alistair Francis, qemu-devel
  Cc: Daniel Henrique Barboza, Marc-André Lureau, Paolo Bonzini

At present the HTIF proxy syscall is unsupported. On RV32, only
device 0 is supported so there is no console device for RV32.
The only way to implement console funtionality on RV32 is to
support the SYS_WRITE syscall.

With this commit, the Spike machine is able to boot the 32-bit
OpenSBI generic image.

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

 hw/char/riscv_htif.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/hw/char/riscv_htif.c b/hw/char/riscv_htif.c
index 3bb0a37a3e..1477fc0090 100644
--- a/hw/char/riscv_htif.c
+++ b/hw/char/riscv_htif.c
@@ -48,6 +48,9 @@
 #define HTIF_CONSOLE_CMD_GETC   0
 #define HTIF_CONSOLE_CMD_PUTC   1
 
+/* PK system call number */
+#define PK_SYS_WRITE            64
+
 static uint64_t fromhost_addr, tohost_addr;
 static int address_symbol_set;
 
@@ -165,7 +168,19 @@ static void htif_handle_tohost_write(HTIFState *s, uint64_t val_written)
                 int exit_code = payload >> 1;
                 exit(exit_code);
             } else {
-                qemu_log_mask(LOG_UNIMP, "pk syscall proxy not supported\n");
+                uint64_t syscall[8];
+                cpu_physical_memory_read(payload, syscall, sizeof(syscall));
+                if (syscall[0] == PK_SYS_WRITE &&
+                    syscall[1] == HTIF_DEV_CONSOLE &&
+                    syscall[3] == HTIF_CONSOLE_CMD_PUTC) {
+                    uint8_t ch;
+                    cpu_physical_memory_read(syscall[2], &ch, 1);
+                    qemu_chr_fe_write(&s->chr, &ch, 1);
+                    resp = 0x100 | (uint8_t)payload;
+                } else {
+                    qemu_log_mask(LOG_UNIMP,
+                                  "pk syscall proxy not supported\n");
+                }
             }
         } else {
             qemu_log("HTIF device %d: unknown command\n", device);
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH 08/12] hw/riscv: spike: Remove the out-of-date comments
  2022-12-27  6:48 [PATCH 00/12] hw/riscv: Improve Spike HTIF emulation fidelity Bin Meng
                   ` (6 preceding siblings ...)
  2022-12-27  6:48 ` [PATCH 07/12] hw/char: riscv_htif: Support console output via proxy syscall Bin Meng
@ 2022-12-27  6:48 ` Bin Meng
  2022-12-27 17:37   ` Daniel Henrique Barboza
  2022-12-28  4:30   ` Alistair Francis
  2022-12-27  6:48 ` [PATCH 09/12] hw/riscv/boot.c: make riscv_find_firmware() static Bin Meng
                   ` (4 subsequent siblings)
  12 siblings, 2 replies; 35+ messages in thread
From: Bin Meng @ 2022-12-27  6:48 UTC (permalink / raw)
  To: Alistair Francis, qemu-devel
  Cc: Daniel Henrique Barboza, Alistair Francis, Bin Meng,
	Palmer Dabbelt, qemu-riscv

Spike machine now supports OpenSBI plain binary bios image, so the
comments are no longer valid.

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

 hw/riscv/spike.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index 8606331f61..ab0a945f8b 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -256,11 +256,6 @@ static void spike_board_init(MachineState *machine)
     memory_region_add_subregion(system_memory, memmap[SPIKE_MROM].base,
                                 mask_rom);
 
-    /*
-     * Not like other RISC-V machines that use plain binary bios images,
-     * 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,
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH 09/12] hw/riscv/boot.c: make riscv_find_firmware() static
  2022-12-27  6:48 [PATCH 00/12] hw/riscv: Improve Spike HTIF emulation fidelity Bin Meng
                   ` (7 preceding siblings ...)
  2022-12-27  6:48 ` [PATCH 08/12] hw/riscv: spike: Remove the out-of-date comments Bin Meng
@ 2022-12-27  6:48 ` Bin Meng
  2022-12-27  6:48 ` [PATCH 10/12] hw/riscv/boot.c: introduce riscv_default_firmware_name() Bin Meng
                   ` (3 subsequent siblings)
  12 siblings, 0 replies; 35+ messages in thread
From: Bin Meng @ 2022-12-27  6:48 UTC (permalink / raw)
  To: Alistair Francis, qemu-devel
  Cc: Daniel Henrique Barboza, Philippe Mathieu-Daudé,
	Alistair Francis, Bin Meng, Palmer Dabbelt, qemu-riscv

From: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

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: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Bin Meng <bmeng@tinylab.org>
Message-Id: <20221221182300.307900-5-dbarboza@ventanamicro.com>
Signed-off-by: Bin Meng <bmeng@tinylab.org>
---

 include/hw/riscv/boot.h |  1 -
 hw/riscv/boot.c         | 44 ++++++++++++++++++++---------------------
 2 files changed, 22 insertions(+), 23 deletions(-)

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);
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)
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH 10/12] hw/riscv/boot.c: introduce riscv_default_firmware_name()
  2022-12-27  6:48 [PATCH 00/12] hw/riscv: Improve Spike HTIF emulation fidelity Bin Meng
                   ` (8 preceding siblings ...)
  2022-12-27  6:48 ` [PATCH 09/12] hw/riscv/boot.c: make riscv_find_firmware() static Bin Meng
@ 2022-12-27  6:48 ` Bin Meng
  2022-12-27  6:48 ` [PATCH 11/12] hw/riscv/boot.c: Introduce riscv_find_firmware() Bin Meng
                   ` (2 subsequent siblings)
  12 siblings, 0 replies; 35+ messages in thread
From: Bin Meng @ 2022-12-27  6:48 UTC (permalink / raw)
  To: Alistair Francis, qemu-devel
  Cc: Daniel Henrique Barboza, Palmer Dabbelt, Alistair Francis,
	Bin Meng, qemu-riscv

From: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

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>
Reviewed-by: Bin Meng <bmeng@tinylab.org>
Message-Id: <20221221182300.307900-6-dbarboza@ventanamicro.com>
Signed-off-by: Bin Meng <bmeng@tinylab.org>
---

 include/hw/riscv/boot.h |  1 +
 hw/riscv/boot.c         |  9 +++++++++
 hw/riscv/sifive_u.c     | 11 ++++-------
 hw/riscv/spike.c        | 14 +++++---------
 hw/riscv/virt.c         | 10 +++-------
 5 files changed, 22 insertions(+), 23 deletions(-)

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);
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 b139824aab..662ddf366d 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -532,6 +532,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;
@@ -594,13 +595,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 ab0a945f8b..810a18f283 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;
@@ -256,15 +257,10 @@ static void spike_board_init(MachineState *machine)
     memory_region_add_subregion(system_memory, memmap[SPIKE_MROM].base,
                                 mask_rom);
 
-    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 a5bc7353b4..d8cf6385b5 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1240,6 +1240,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;
 
@@ -1259,13 +1260,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
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH 11/12] hw/riscv/boot.c: Introduce riscv_find_firmware()
  2022-12-27  6:48 [PATCH 00/12] hw/riscv: Improve Spike HTIF emulation fidelity Bin Meng
                   ` (9 preceding siblings ...)
  2022-12-27  6:48 ` [PATCH 10/12] hw/riscv/boot.c: introduce riscv_default_firmware_name() Bin Meng
@ 2022-12-27  6:48 ` Bin Meng
  2022-12-27 17:40   ` Daniel Henrique Barboza
  2022-12-28  4:35   ` Alistair Francis
  2022-12-27  6:48 ` [PATCH 12/12] hw/riscv: spike: Decouple create_fdt() dependency to ELF loading Bin Meng
  2022-12-27 17:51 ` [PATCH 00/12] hw/riscv: Improve Spike HTIF emulation fidelity Daniel Henrique Barboza
  12 siblings, 2 replies; 35+ messages in thread
From: Bin Meng @ 2022-12-27  6:48 UTC (permalink / raw)
  To: Alistair Francis, qemu-devel
  Cc: Daniel Henrique Barboza, Alistair Francis, Bin Meng,
	Palmer Dabbelt, qemu-riscv

Rename previous riscv_find_firmware() to riscv_find_bios(), and
introduce a new riscv_find_firmware() to implement the first half
part of the work done in riscv_find_and_load_firmware().

This new API is helpful for machine that wants to know the final
chosen firmware file name but does not want to load it.

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

 include/hw/riscv/boot.h |  2 ++
 hw/riscv/boot.c         | 39 +++++++++++++++++++++++++--------------
 2 files changed, 27 insertions(+), 14 deletions(-)

diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
index 60cf320c88..b273ab22f7 100644
--- a/include/hw/riscv/boot.h
+++ b/include/hw/riscv/boot.h
@@ -38,6 +38,8 @@ target_ulong riscv_find_and_load_firmware(MachineState *machine,
                                           hwaddr firmware_load_addr,
                                           symbol_fn_t sym_cb);
 const char *riscv_default_firmware_name(RISCVHartArrayState *harts);
+char *riscv_find_firmware(const char *firmware_filename,
+                          const char *default_machine_firmware);
 target_ulong riscv_load_firmware(const char *firmware_filename,
                                  hwaddr firmware_load_addr,
                                  symbol_fn_t sym_cb);
diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index e1a544b1d9..98b80af51b 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -84,11 +84,11 @@ const char *riscv_default_firmware_name(RISCVHartArrayState *harts)
     return RISCV64_BIOS_BIN;
 }
 
-static char *riscv_find_firmware(const char *firmware_filename)
+static char *riscv_find_bios(const char *bios_filename)
 {
     char *filename;
 
-    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, firmware_filename);
+    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_filename);
     if (filename == NULL) {
         if (!qtest_enabled()) {
             /*
@@ -97,8 +97,8 @@ static char *riscv_find_firmware(const char *firmware_filename)
              * 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);
+            error_report("Unable to find the RISC-V BIOS \"%s\"",
+                         bios_filename);
             exit(1);
         }
     }
@@ -106,25 +106,36 @@ static char *riscv_find_firmware(const char *firmware_filename)
     return filename;
 }
 
-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,
+                          const char *default_machine_firmware)
 {
-    char *firmware_filename = NULL;
-    target_ulong firmware_end_addr = firmware_load_addr;
+    char *filename = NULL;
 
-    if ((!machine->firmware) || (!strcmp(machine->firmware, "default"))) {
+    if ((!firmware_filename) || (!strcmp(firmware_filename, "default"))) {
         /*
          * The user didn't specify -bios, or has specified "-bios default".
          * That means we are going to load the OpenSBI binary included in
          * the QEMU source.
          */
-        firmware_filename = riscv_find_firmware(default_machine_firmware);
-    } else if (strcmp(machine->firmware, "none")) {
-        firmware_filename = riscv_find_firmware(machine->firmware);
+        filename = riscv_find_bios(default_machine_firmware);
+    } else if (strcmp(firmware_filename, "none")) {
+        filename = riscv_find_bios(firmware_filename);
     }
 
+    return filename;
+}
+
+target_ulong riscv_find_and_load_firmware(MachineState *machine,
+                                          const char *default_machine_firmware,
+                                          hwaddr firmware_load_addr,
+                                          symbol_fn_t sym_cb)
+{
+    char *firmware_filename;
+    target_ulong firmware_end_addr = firmware_load_addr;
+
+    firmware_filename = riscv_find_firmware(machine->firmware,
+                                            default_machine_firmware);
+
     if (firmware_filename) {
         /* If not "none" load the firmware */
         firmware_end_addr = riscv_load_firmware(firmware_filename,
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH 12/12] hw/riscv: spike: Decouple create_fdt() dependency to ELF loading
  2022-12-27  6:48 [PATCH 00/12] hw/riscv: Improve Spike HTIF emulation fidelity Bin Meng
                   ` (10 preceding siblings ...)
  2022-12-27  6:48 ` [PATCH 11/12] hw/riscv/boot.c: Introduce riscv_find_firmware() Bin Meng
@ 2022-12-27  6:48 ` Bin Meng
  2022-12-27 14:33   ` Daniel Henrique Barboza
  2022-12-27 17:51 ` [PATCH 00/12] hw/riscv: Improve Spike HTIF emulation fidelity Daniel Henrique Barboza
  12 siblings, 1 reply; 35+ messages in thread
From: Bin Meng @ 2022-12-27  6:48 UTC (permalink / raw)
  To: Alistair Francis, qemu-devel
  Cc: Daniel Henrique Barboza, Alistair Francis, Bin Meng,
	Marc-André Lureau, Palmer Dabbelt, Paolo Bonzini,
	qemu-riscv

At present create_fdt() calls htif_uses_elf_symbols() to determine
whether to insert a <reg> property for the HTIF. This unfortunately
creates a hidden dependency to riscv_load_{firmware,kernel} that
create_fdt() must be called after the ELF {firmware,kernel} image
has been loaded.

Decouple such dependency be adding a new parameter to create_fdt(),
whether custom HTIF base address is used. The flag will be set if
non ELF {firmware,kernel} image is given by user.

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

---

 include/hw/char/riscv_htif.h |  5 +---
 hw/char/riscv_htif.c         | 17 +++++-------
 hw/riscv/spike.c             | 54 ++++++++++++++++++++++++++++++------
 3 files changed, 53 insertions(+), 23 deletions(-)

diff --git a/include/hw/char/riscv_htif.h b/include/hw/char/riscv_htif.h
index 9e8ebbe017..5958c5b986 100644
--- a/include/hw/char/riscv_htif.h
+++ b/include/hw/char/riscv_htif.h
@@ -44,11 +44,8 @@ typedef struct HTIFState {
 void htif_symbol_callback(const char *st_name, int st_info, uint64_t st_value,
     uint64_t st_size);
 
-/* Check if HTIF uses ELF symbols */
-bool htif_uses_elf_symbols(void);
-
 /* legacy pre qom */
 HTIFState *htif_mm_init(MemoryRegion *address_space, Chardev *chr,
-                        uint64_t nonelf_base);
+                        uint64_t nonelf_base, bool custom_base);
 
 #endif
diff --git a/hw/char/riscv_htif.c b/hw/char/riscv_htif.c
index 1477fc0090..098de50e35 100644
--- a/hw/char/riscv_htif.c
+++ b/hw/char/riscv_htif.c
@@ -52,20 +52,17 @@
 #define PK_SYS_WRITE            64
 
 static uint64_t fromhost_addr, tohost_addr;
-static int address_symbol_set;
 
 void htif_symbol_callback(const char *st_name, int st_info, uint64_t st_value,
                           uint64_t st_size)
 {
     if (strcmp("fromhost", st_name) == 0) {
-        address_symbol_set |= 1;
         fromhost_addr = st_value;
         if (st_size != 8) {
             error_report("HTIF fromhost must be 8 bytes");
             exit(1);
         }
     } else if (strcmp("tohost", st_name) == 0) {
-        address_symbol_set |= 2;
         tohost_addr = st_value;
         if (st_size != 8) {
             error_report("HTIF tohost must be 8 bytes");
@@ -275,19 +272,19 @@ static const MemoryRegionOps htif_mm_ops = {
     .write = htif_mm_write,
 };
 
-bool htif_uses_elf_symbols(void)
-{
-    return (address_symbol_set == 3) ? true : false;
-}
-
 HTIFState *htif_mm_init(MemoryRegion *address_space, Chardev *chr,
-                        uint64_t nonelf_base)
+                        uint64_t nonelf_base, bool custom_base)
 {
     uint64_t base, size, tohost_offset, fromhost_offset;
 
-    if (!htif_uses_elf_symbols()) {
+    if (custom_base) {
         fromhost_addr = nonelf_base;
         tohost_addr = nonelf_base + 8;
+    } else {
+        if (!fromhost_addr || !tohost_addr) {
+            error_report("Invalid HTIF fromhost or tohost address");
+            exit(1);
+        }
     }
 
     base = MIN(tohost_addr, fromhost_addr);
diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index 810a18f283..90f9e581e4 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -50,7 +50,8 @@ static const MemMapEntry spike_memmap[] = {
 };
 
 static void create_fdt(SpikeState *s, const MemMapEntry *memmap,
-                       uint64_t mem_size, const char *cmdline, bool is_32_bit)
+                       uint64_t mem_size, const char *cmdline,
+                       bool is_32_bit, bool htif_custom_base)
 {
     void *fdt;
     uint64_t addr, size;
@@ -78,7 +79,7 @@ static void create_fdt(SpikeState *s, const MemMapEntry *memmap,
 
     qemu_fdt_add_subnode(fdt, "/htif");
     qemu_fdt_setprop_string(fdt, "/htif", "compatible", "ucb,htif0");
-    if (!htif_uses_elf_symbols()) {
+    if (htif_custom_base) {
         qemu_fdt_setprop_cells(fdt, "/htif", "reg",
             0x0, memmap[SPIKE_HTIF].base, 0x0, memmap[SPIKE_HTIF].size);
     }
@@ -184,6 +185,21 @@ static void create_fdt(SpikeState *s, const MemMapEntry *memmap,
     }
 }
 
+static bool spike_test_elf_image(char *filename)
+{
+    Error *err = NULL;
+
+    if (filename) {
+        load_elf_hdr(filename, NULL, NULL, &err);
+        if (err) {
+            error_free(err);
+            return false;
+        }
+    }
+
+    return true;
+}
+
 static void spike_board_init(MachineState *machine)
 {
     const MemMapEntry *memmap = spike_memmap;
@@ -191,11 +207,12 @@ 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;
+    char *firmware_name;
     uint32_t fdt_load_addr;
     uint64_t kernel_entry;
     char *soc_name;
     int i, base_hartid, hart_count;
+    bool htif_custom_base;
 
     /* Check socket count limit */
     if (SPIKE_SOCKETS_MAX < riscv_socket_count(machine)) {
@@ -257,10 +274,28 @@ static void spike_board_init(MachineState *machine)
     memory_region_add_subregion(system_memory, memmap[SPIKE_MROM].base,
                                 mask_rom);
 
-    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);
+    /* Find firmware */
+    firmware_name = riscv_find_firmware(machine->firmware,
+                        riscv_default_firmware_name(&s->soc[0]));
+
+    /*
+     * Test the given firmware or kernel file to see if it is an ELF image.
+     * If it is an ELF, we assume it contains the symbols required for
+     * the HTIF console, otherwise we fall back to use the custom base
+     * passed from device tree for the HTIF console.
+     */
+    htif_custom_base = !spike_test_elf_image(firmware_name);
+    if (!htif_custom_base) {
+        htif_custom_base = !spike_test_elf_image(machine->kernel_filename);
+    }
+
+    /* Load firmware */
+    if (firmware_name) {
+        firmware_end_addr = riscv_load_firmware(firmware_name,
+                                                memmap[SPIKE_DRAM].base,
+                                                htif_symbol_callback);
+        g_free(firmware_name);
+    }
 
     /* Load kernel */
     if (machine->kernel_filename) {
@@ -280,7 +315,7 @@ static void spike_board_init(MachineState *machine)
 
     /* Create device tree */
     create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline,
-               riscv_is_32bit(&s->soc[0]));
+               riscv_is_32bit(&s->soc[0]), htif_custom_base);
 
     /* Load initrd */
     if (machine->kernel_filename && machine->initrd_filename) {
@@ -308,7 +343,8 @@ static void spike_board_init(MachineState *machine)
                               fdt_load_addr);
 
     /* initialize HTIF using symbols found in load_kernel */
-    htif_mm_init(system_memory, serial_hd(0), memmap[SPIKE_HTIF].base);
+    htif_mm_init(system_memory, serial_hd(0), memmap[SPIKE_HTIF].base,
+                 htif_custom_base);
 }
 
 static void spike_machine_instance_init(Object *obj)
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 35+ messages in thread

* Re: [PATCH 12/12] hw/riscv: spike: Decouple create_fdt() dependency to ELF loading
  2022-12-27  6:48 ` [PATCH 12/12] hw/riscv: spike: Decouple create_fdt() dependency to ELF loading Bin Meng
@ 2022-12-27 14:33   ` Daniel Henrique Barboza
  0 siblings, 0 replies; 35+ messages in thread
From: Daniel Henrique Barboza @ 2022-12-27 14:33 UTC (permalink / raw)
  To: Bin Meng, Alistair Francis, qemu-devel
  Cc: Bin Meng, Marc-André Lureau, Palmer Dabbelt, Paolo Bonzini,
	qemu-riscv

Hi Bin,

On 12/27/22 03:48, Bin Meng wrote:
> At present create_fdt() calls htif_uses_elf_symbols() to determine
> whether to insert a <reg> property for the HTIF. This unfortunately
> creates a hidden dependency to riscv_load_{firmware,kernel} that
> create_fdt() must be called after the ELF {firmware,kernel} image
> has been loaded.
>
> Decouple such dependency be adding a new parameter to create_fdt(),
> whether custom HTIF base address is used. The flag will be set if
> non ELF {firmware,kernel} image is given by user.
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
>
> ---
>
>   include/hw/char/riscv_htif.h |  5 +---
>   hw/char/riscv_htif.c         | 17 +++++-------
>   hw/riscv/spike.c             | 54 ++++++++++++++++++++++++++++++------
>   3 files changed, 53 insertions(+), 23 deletions(-)
>
> diff --git a/include/hw/char/riscv_htif.h b/include/hw/char/riscv_htif.h
> index 9e8ebbe017..5958c5b986 100644
> --- a/include/hw/char/riscv_htif.h
> +++ b/include/hw/char/riscv_htif.h
> @@ -44,11 +44,8 @@ typedef struct HTIFState {
>   void htif_symbol_callback(const char *st_name, int st_info, uint64_t st_value,
>       uint64_t st_size);
>   
> -/* Check if HTIF uses ELF symbols */
> -bool htif_uses_elf_symbols(void);
> -
>   /* legacy pre qom */
>   HTIFState *htif_mm_init(MemoryRegion *address_space, Chardev *chr,
> -                        uint64_t nonelf_base);
> +                        uint64_t nonelf_base, bool custom_base);
>   
>   #endif
> diff --git a/hw/char/riscv_htif.c b/hw/char/riscv_htif.c
> index 1477fc0090..098de50e35 100644
> --- a/hw/char/riscv_htif.c
> +++ b/hw/char/riscv_htif.c
> @@ -52,20 +52,17 @@
>   #define PK_SYS_WRITE            64
>   
>   static uint64_t fromhost_addr, tohost_addr;
> -static int address_symbol_set;
>   
>   void htif_symbol_callback(const char *st_name, int st_info, uint64_t st_value,
>                             uint64_t st_size)
>   {
>       if (strcmp("fromhost", st_name) == 0) {
> -        address_symbol_set |= 1;
>           fromhost_addr = st_value;
>           if (st_size != 8) {
>               error_report("HTIF fromhost must be 8 bytes");
>               exit(1);
>           }
>       } else if (strcmp("tohost", st_name) == 0) {
> -        address_symbol_set |= 2;
>           tohost_addr = st_value;
>           if (st_size != 8) {
>               error_report("HTIF tohost must be 8 bytes");
> @@ -275,19 +272,19 @@ static const MemoryRegionOps htif_mm_ops = {
>       .write = htif_mm_write,
>   };
>   
> -bool htif_uses_elf_symbols(void)
> -{
> -    return (address_symbol_set == 3) ? true : false;
> -}
> -
>   HTIFState *htif_mm_init(MemoryRegion *address_space, Chardev *chr,
> -                        uint64_t nonelf_base)
> +                        uint64_t nonelf_base, bool custom_base)
>   {
>       uint64_t base, size, tohost_offset, fromhost_offset;
>   
> -    if (!htif_uses_elf_symbols()) {
> +    if (custom_base) {
>           fromhost_addr = nonelf_base;
>           tohost_addr = nonelf_base + 8;
> +    } else {
> +        if (!fromhost_addr || !tohost_addr) {
> +            error_report("Invalid HTIF fromhost or tohost address");
> +            exit(1);
> +        }
>       }
>   
>       base = MIN(tohost_addr, fromhost_addr);
> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> index 810a18f283..90f9e581e4 100644
> --- a/hw/riscv/spike.c
> +++ b/hw/riscv/spike.c
> @@ -50,7 +50,8 @@ static const MemMapEntry spike_memmap[] = {
>   };
>   
>   static void create_fdt(SpikeState *s, const MemMapEntry *memmap,
> -                       uint64_t mem_size, const char *cmdline, bool is_32_bit)
> +                       uint64_t mem_size, const char *cmdline,
> +                       bool is_32_bit, bool htif_custom_base)
>   {
>       void *fdt;
>       uint64_t addr, size;
> @@ -78,7 +79,7 @@ static void create_fdt(SpikeState *s, const MemMapEntry *memmap,
>   
>       qemu_fdt_add_subnode(fdt, "/htif");
>       qemu_fdt_setprop_string(fdt, "/htif", "compatible", "ucb,htif0");
> -    if (!htif_uses_elf_symbols()) {
> +    if (htif_custom_base) {
>           qemu_fdt_setprop_cells(fdt, "/htif", "reg",
>               0x0, memmap[SPIKE_HTIF].base, 0x0, memmap[SPIKE_HTIF].size);
>       }
> @@ -184,6 +185,21 @@ static void create_fdt(SpikeState *s, const MemMapEntry *memmap,
>       }
>   }
>   
> +static bool spike_test_elf_image(char *filename)
> +{
> +    Error *err = NULL;
> +
> +    if (filename) {
> +        load_elf_hdr(filename, NULL, NULL, &err);
> +        if (err) {
> +            error_free(err);
> +            return false;
> +        }
> +    }
> +
> +    return true;
> +}
> +
>   static void spike_board_init(MachineState *machine)
>   {
>       const MemMapEntry *memmap = spike_memmap;
> @@ -191,11 +207,12 @@ 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;
> +    char *firmware_name;
>       uint32_t fdt_load_addr;
>       uint64_t kernel_entry;
>       char *soc_name;
>       int i, base_hartid, hart_count;
> +    bool htif_custom_base;
>   
>       /* Check socket count limit */
>       if (SPIKE_SOCKETS_MAX < riscv_socket_count(machine)) {
> @@ -257,10 +274,28 @@ static void spike_board_init(MachineState *machine)
>       memory_region_add_subregion(system_memory, memmap[SPIKE_MROM].base,
>                                   mask_rom);
>   
> -    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);
> +    /* Find firmware */
> +    firmware_name = riscv_find_firmware(machine->firmware,
> +                        riscv_default_firmware_name(&s->soc[0]));
> +
> +    /*
> +     * Test the given firmware or kernel file to see if it is an ELF image.
> +     * If it is an ELF, we assume it contains the symbols required for
> +     * the HTIF console, otherwise we fall back to use the custom base
> +     * passed from device tree for the HTIF console.
> +     */
> +    htif_custom_base = !spike_test_elf_image(firmware_name);
> +    if (!htif_custom_base) {
> +        htif_custom_base = !spike_test_elf_image(machine->kernel_filename);
> +    }
> +
> +    /* Load firmware */
> +    if (firmware_name) {
> +        firmware_end_addr = riscv_load_firmware(firmware_name,
> +                                                memmap[SPIKE_DRAM].base,
> +                                                htif_symbol_callback);
> +        g_free(firmware_name);
> +    }
>   
>       /* Load kernel */
>       if (machine->kernel_filename) {

'make' is giving me a maybe-uninitialized error in this point:

../hw/riscv/spike.c: In function ‘spike_board_init’:
../hw/riscv/spike.c:301:29: error: ‘firmware_end_addr’ may be used uninitialized [-Werror=maybe-uninitialized]
   301 |         kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc[0],
       | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   302 | firmware_end_addr);
       | ~~~~~~~~~~~~~~~~~~
../hw/riscv/spike.c:208:18: note: ‘firmware_end_addr’ was declared here
   208 |     target_ulong firmware_end_addr, kernel_start_addr;
       |                  ^~~~~~~~~~~~~~~~~


The full context:


     /* Load firmware */
     if (firmware_name) {
         firmware_end_addr = riscv_load_firmware(firmware_name,
memmap[SPIKE_DRAM].base,
htif_symbol_callback);
         g_free(firmware_name);
     }

     /* Load kernel */
     if (machine->kernel_filename) {
         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,
                                          htif_symbol_callback);
     } else {


The error is happening because, with this patch, 'firmware_end_addr' may not be
initialized via riscv_load_firmware() because we're not guaranteeing that
'firmware_name' will be valid.

riscv_load_firmware() is guaranteed to either return > 0 or error out with exit(1),
so a simple fix would be to initialize 'firmware_end_addr' with 0.



Thanks,

Daniel


> @@ -280,7 +315,7 @@ static void spike_board_init(MachineState *machine)
>   
>       /* Create device tree */
>       create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline,
> -               riscv_is_32bit(&s->soc[0]));
> +               riscv_is_32bit(&s->soc[0]), htif_custom_base);
>   
>       /* Load initrd */
>       if (machine->kernel_filename && machine->initrd_filename) {
> @@ -308,7 +343,8 @@ static void spike_board_init(MachineState *machine)
>                                 fdt_load_addr);
>   
>       /* initialize HTIF using symbols found in load_kernel */
> -    htif_mm_init(system_memory, serial_hd(0), memmap[SPIKE_HTIF].base);
> +    htif_mm_init(system_memory, serial_hd(0), memmap[SPIKE_HTIF].base,
> +                 htif_custom_base);
>   }
>   
>   static void spike_machine_instance_init(Object *obj)



^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 01/12] hw/char: riscv_htif: Avoid using magic numbers
  2022-12-27  6:48 ` [PATCH 01/12] hw/char: riscv_htif: Avoid using magic numbers Bin Meng
@ 2022-12-27 17:28   ` Daniel Henrique Barboza
  2022-12-28  3:31   ` Alistair Francis
  1 sibling, 0 replies; 35+ messages in thread
From: Daniel Henrique Barboza @ 2022-12-27 17:28 UTC (permalink / raw)
  To: Bin Meng, Alistair Francis, qemu-devel
  Cc: Marc-André Lureau, Paolo Bonzini



On 12/27/22 03:48, Bin Meng wrote:
> The Spike HTIF is poorly documented. The only relevant info we can
> get from the internet is from Andrew Waterman at [1].
>
> Add a comment block before htif_handle_tohost_write() to explain
> the tohost register format, and use meaningful macros intead of
s/intead/instead

> magic numbers in the codes.
>
> While we are here, corret 2 multi-line comment blocks that have

s/corret/correct


Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

> wrong format.
>
> Link: https://github.com/riscv-software-src/riscv-isa-sim/issues/364 [1]
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
> ---
>
>   hw/char/riscv_htif.c | 72 ++++++++++++++++++++++++++++++++------------
>   1 file changed, 52 insertions(+), 20 deletions(-)
>
> diff --git a/hw/char/riscv_htif.c b/hw/char/riscv_htif.c
> index 6577f0e640..088556bb04 100644
> --- a/hw/char/riscv_htif.c
> +++ b/hw/char/riscv_htif.c
> @@ -38,6 +38,16 @@
>           }                                                                      \
>       } while (0)
>   
> +#define HTIF_DEV_SHIFT          56
> +#define HTIF_CMD_SHIFT          48
> +
> +#define HTIF_DEV_SYSTEM         0
> +#define HTIF_DEV_CONSOLE        1
> +
> +#define HTIF_SYSTEM_CMD_SYSCALL 0
> +#define HTIF_CONSOLE_CMD_GETC   0
> +#define HTIF_CONSOLE_CMD_PUTC   1
> +
>   static uint64_t fromhost_addr, tohost_addr;
>   static int address_symbol_set;
>   
> @@ -81,9 +91,11 @@ static void htif_recv(void *opaque, const uint8_t *buf, int size)
>           return;
>       }
>   
> -    /* TODO - we need to check whether mfromhost is zero which indicates
> -              the device is ready to receive. The current implementation
> -              will drop characters */
> +    /*
> +     * TODO - we need to check whether mfromhost is zero which indicates
> +     *        the device is ready to receive. The current implementation
> +     *        will drop characters
> +     */
>   
>       uint64_t val_written = htifstate->pending_read;
>       uint64_t resp = 0x100 | *buf;
> @@ -110,10 +122,30 @@ static int htif_be_change(void *opaque)
>       return 0;
>   }
>   
> +/*
> + * See below the tohost register format.
> + *
> + * Bits 63:56 indicate the "device".
> + * Bits 55:48 indicate the "command".
> + *
> + * Device 0 is the syscall device, which is used to emulate Unixy syscalls.
> + * It only implements command 0, which has two subfunctions:
> + * - If bit 0 is clear, then bits 47:0 represent a pointer to a struct
> + *   describing the syscall.
> + * - If bit 1 is set, then bits 47:1 represent an exit code, with a zero
> + *   value indicating success and other values indicating failure.
> + *
> + * Device 1 is the blocking character device.
> + * - Command 0 reads a character
> + * - Command 1 writes a character from the 8 LSBs of tohost
> + *
> + * For RV32, the tohost register is zero-extended, so only device=0 and
> + * command=0 (i.e. HTIF syscalls/exit codes) are supported.
> + */
>   static void htif_handle_tohost_write(HTIFState *htifstate, uint64_t val_written)
>   {
> -    uint8_t device = val_written >> 56;
> -    uint8_t cmd = val_written >> 48;
> +    uint8_t device = val_written >> HTIF_DEV_SHIFT;
> +    uint8_t cmd = val_written >> HTIF_CMD_SHIFT;
>       uint64_t payload = val_written & 0xFFFFFFFFFFFFULL;
>       int resp = 0;
>   
> @@ -125,9 +157,9 @@ static void htif_handle_tohost_write(HTIFState *htifstate, uint64_t val_written)
>        * 0: riscv-tests Pass/Fail Reporting Only (no syscall proxy)
>        * 1: Console
>        */
> -    if (unlikely(device == 0x0)) {
> +    if (unlikely(device == HTIF_DEV_SYSTEM)) {
>           /* frontend syscall handler, shutdown and exit code support */
> -        if (cmd == 0x0) {
> +        if (cmd == HTIF_SYSTEM_CMD_SYSCALL) {
>               if (payload & 0x1) {
>                   /* exit code */
>                   int exit_code = payload >> 1;
> @@ -138,14 +170,14 @@ static void htif_handle_tohost_write(HTIFState *htifstate, uint64_t val_written)
>           } else {
>               qemu_log("HTIF device %d: unknown command\n", device);
>           }
> -    } else if (likely(device == 0x1)) {
> +    } else if (likely(device == HTIF_DEV_CONSOLE)) {
>           /* HTIF Console */
> -        if (cmd == 0x0) {
> +        if (cmd == HTIF_CONSOLE_CMD_GETC) {
>               /* this should be a queue, but not yet implemented as such */
>               htifstate->pending_read = val_written;
>               htifstate->env->mtohost = 0; /* clear to indicate we read */
>               return;
> -        } else if (cmd == 0x1) {
> +        } else if (cmd == HTIF_CONSOLE_CMD_PUTC) {
>               qemu_chr_fe_write(&htifstate->chr, (uint8_t *)&payload, 1);
>               resp = 0x100 | (uint8_t)payload;
>           } else {
> @@ -157,15 +189,15 @@ static void htif_handle_tohost_write(HTIFState *htifstate, uint64_t val_written)
>               " payload: %016" PRIx64, device, cmd, payload & 0xFF, payload);
>       }
>       /*
> -     * - latest bbl does not set fromhost to 0 if there is a value in tohost
> -     * - with this code enabled, qemu hangs waiting for fromhost to go to 0
> -     * - with this code disabled, qemu works with bbl priv v1.9.1 and v1.10
> -     * - HTIF needs protocol documentation and a more complete state machine
> -
> -        while (!htifstate->fromhost_inprogress &&
> -            htifstate->env->mfromhost != 0x0) {
> -        }
> -    */
> +     * Latest bbl does not set fromhost to 0 if there is a value in tohost.
> +     * With this code enabled, qemu hangs waiting for fromhost to go to 0.
> +     * With this code disabled, qemu works with bbl priv v1.9.1 and v1.10.
> +     * HTIF needs protocol documentation and a more complete state machine.
> +     *
> +     *  while (!htifstate->fromhost_inprogress &&
> +     *      htifstate->env->mfromhost != 0x0) {
> +     *  }
> +     */
>       htifstate->env->mfromhost = (val_written >> 48 << 48) | (resp << 16 >> 16);
>       htifstate->env->mtohost = 0; /* clear to indicate we read */
>   }
> @@ -196,7 +228,7 @@ static uint64_t htif_mm_read(void *opaque, hwaddr addr, unsigned size)
>   
>   /* CPU wrote to an HTIF register */
>   static void htif_mm_write(void *opaque, hwaddr addr,
> -                            uint64_t value, unsigned size)
> +                          uint64_t value, unsigned size)
>   {
>       HTIFState *htifstate = opaque;
>       if (addr == TOHOST_OFFSET1) {



^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 02/12] hw/char: riscv_htif: Drop {to,from}host_size in HTIFState
  2022-12-27  6:48 ` [PATCH 02/12] hw/char: riscv_htif: Drop {to, from}host_size in HTIFState Bin Meng
@ 2022-12-27 17:29   ` Daniel Henrique Barboza
  2022-12-28  3:32   ` [PATCH 02/12] hw/char: riscv_htif: Drop {to, from}host_size " Alistair Francis
  1 sibling, 0 replies; 35+ messages in thread
From: Daniel Henrique Barboza @ 2022-12-27 17:29 UTC (permalink / raw)
  To: Bin Meng, Alistair Francis, qemu-devel; +Cc: Anup Patel



On 12/27/22 03:48, Bin Meng wrote:
> These are not used anywhere. Drop them.
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
> ---

Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

>
>   include/hw/char/riscv_htif.h | 2 --
>   1 file changed, 2 deletions(-)
>
> diff --git a/include/hw/char/riscv_htif.h b/include/hw/char/riscv_htif.h
> index f888ac1b30..3eccc1914f 100644
> --- a/include/hw/char/riscv_htif.h
> +++ b/include/hw/char/riscv_htif.h
> @@ -33,8 +33,6 @@ typedef struct HTIFState {
>   
>       hwaddr tohost_offset;
>       hwaddr fromhost_offset;
> -    uint64_t tohost_size;
> -    uint64_t fromhost_size;
>       MemoryRegion mmio;
>       MemoryRegion *address_space;
>       MemoryRegion *main_mem;



^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 03/12] hw/char: riscv_htif: Drop useless assignment of memory region
  2022-12-27  6:48 ` [PATCH 03/12] hw/char: riscv_htif: Drop useless assignment of memory region Bin Meng
@ 2022-12-27 17:30   ` Daniel Henrique Barboza
  2022-12-28  3:33   ` Alistair Francis
  1 sibling, 0 replies; 35+ messages in thread
From: Daniel Henrique Barboza @ 2022-12-27 17:30 UTC (permalink / raw)
  To: Bin Meng, Alistair Francis, qemu-devel
  Cc: Bin Meng, Marc-André Lureau, Palmer Dabbelt, Paolo Bonzini,
	qemu-riscv



On 12/27/22 03:48, Bin Meng wrote:
> struct HTIFState has 3 members for address space and memory region,
> and are initialized during htif_mm_init(). But they are actually
> useless. Drop them.
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
> ---

Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

>
>   include/hw/char/riscv_htif.h | 7 ++-----
>   hw/char/riscv_htif.c         | 7 ++-----
>   hw/riscv/spike.c             | 5 ++---
>   3 files changed, 6 insertions(+), 13 deletions(-)
>
> diff --git a/include/hw/char/riscv_htif.h b/include/hw/char/riscv_htif.h
> index 3eccc1914f..6d172ebd6d 100644
> --- a/include/hw/char/riscv_htif.h
> +++ b/include/hw/char/riscv_htif.h
> @@ -34,9 +34,6 @@ typedef struct HTIFState {
>       hwaddr tohost_offset;
>       hwaddr fromhost_offset;
>       MemoryRegion mmio;
> -    MemoryRegion *address_space;
> -    MemoryRegion *main_mem;
> -    void *main_mem_ram_ptr;
>   
>       CPURISCVState *env;
>       CharBackend chr;
> @@ -54,7 +51,7 @@ void htif_symbol_callback(const char *st_name, int st_info, uint64_t st_value,
>   bool htif_uses_elf_symbols(void);
>   
>   /* legacy pre qom */
> -HTIFState *htif_mm_init(MemoryRegion *address_space, MemoryRegion *main_mem,
> -    CPURISCVState *env, Chardev *chr, uint64_t nonelf_base);
> +HTIFState *htif_mm_init(MemoryRegion *address_space, CPURISCVState *env,
> +                        Chardev *chr, uint64_t nonelf_base);
>   
>   #endif
> diff --git a/hw/char/riscv_htif.c b/hw/char/riscv_htif.c
> index 088556bb04..e7e319ca1d 100644
> --- a/hw/char/riscv_htif.c
> +++ b/hw/char/riscv_htif.c
> @@ -265,8 +265,8 @@ bool htif_uses_elf_symbols(void)
>       return (address_symbol_set == 3) ? true : false;
>   }
>   
> -HTIFState *htif_mm_init(MemoryRegion *address_space, MemoryRegion *main_mem,
> -    CPURISCVState *env, Chardev *chr, uint64_t nonelf_base)
> +HTIFState *htif_mm_init(MemoryRegion *address_space, CPURISCVState *env,
> +                        Chardev *chr, uint64_t nonelf_base)
>   {
>       uint64_t base, size, tohost_offset, fromhost_offset;
>   
> @@ -281,9 +281,6 @@ HTIFState *htif_mm_init(MemoryRegion *address_space, MemoryRegion *main_mem,
>       fromhost_offset = fromhost_addr - base;
>   
>       HTIFState *s = g_new0(HTIFState, 1);
> -    s->address_space = address_space;
> -    s->main_mem = main_mem;
> -    s->main_mem_ram_ptr = memory_region_get_ram_ptr(main_mem);
>       s->env = env;
>       s->tohost_offset = tohost_offset;
>       s->fromhost_offset = fromhost_offset;
> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> index 1e1d752c00..82cf41ac27 100644
> --- a/hw/riscv/spike.c
> +++ b/hw/riscv/spike.c
> @@ -317,9 +317,8 @@ static void spike_board_init(MachineState *machine)
>                                 fdt_load_addr);
>   
>       /* initialize HTIF using symbols found in load_kernel */
> -    htif_mm_init(system_memory, mask_rom,
> -                 &s->soc[0].harts[0].env, serial_hd(0),
> -                 memmap[SPIKE_HTIF].base);
> +    htif_mm_init(system_memory, &s->soc[0].harts[0].env,
> +                 serial_hd(0), memmap[SPIKE_HTIF].base);
>   }
>   
>   static void spike_machine_instance_init(Object *obj)



^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 04/12] hw/char: riscv_htif: Use conventional 's' for HTIFState
  2022-12-27  6:48 ` [PATCH 04/12] hw/char: riscv_htif: Use conventional 's' for HTIFState Bin Meng
@ 2022-12-27 17:32   ` Daniel Henrique Barboza
  2022-12-28  3:35   ` Alistair Francis
  1 sibling, 0 replies; 35+ messages in thread
From: Daniel Henrique Barboza @ 2022-12-27 17:32 UTC (permalink / raw)
  To: Bin Meng, Alistair Francis, qemu-devel
  Cc: Marc-André Lureau, Paolo Bonzini



On 12/27/22 03:48, Bin Meng wrote:
> QEMU source codes tend to use 's' to represent the hardware state.
> Let's use it for HTIFState.
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
> ---

Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

>
>   hw/char/riscv_htif.c | 64 ++++++++++++++++++++++----------------------
>   1 file changed, 32 insertions(+), 32 deletions(-)
>
> diff --git a/hw/char/riscv_htif.c b/hw/char/riscv_htif.c
> index e7e319ca1d..f28976b110 100644
> --- a/hw/char/riscv_htif.c
> +++ b/hw/char/riscv_htif.c
> @@ -85,7 +85,7 @@ static int htif_can_recv(void *opaque)
>    */
>   static void htif_recv(void *opaque, const uint8_t *buf, int size)
>   {
> -    HTIFState *htifstate = opaque;
> +    HTIFState *s = opaque;
>   
>       if (size != 1) {
>           return;
> @@ -97,10 +97,10 @@ static void htif_recv(void *opaque, const uint8_t *buf, int size)
>        *        will drop characters
>        */
>   
> -    uint64_t val_written = htifstate->pending_read;
> +    uint64_t val_written = s->pending_read;
>       uint64_t resp = 0x100 | *buf;
>   
> -    htifstate->env->mfromhost = (val_written >> 48 << 48) | (resp << 16 >> 16);
> +    s->env->mfromhost = (val_written >> 48 << 48) | (resp << 16 >> 16);
>   }
>   
>   /*
> @@ -142,7 +142,7 @@ static int htif_be_change(void *opaque)
>    * For RV32, the tohost register is zero-extended, so only device=0 and
>    * command=0 (i.e. HTIF syscalls/exit codes) are supported.
>    */
> -static void htif_handle_tohost_write(HTIFState *htifstate, uint64_t val_written)
> +static void htif_handle_tohost_write(HTIFState *s, uint64_t val_written)
>   {
>       uint8_t device = val_written >> HTIF_DEV_SHIFT;
>       uint8_t cmd = val_written >> HTIF_CMD_SHIFT;
> @@ -174,11 +174,11 @@ static void htif_handle_tohost_write(HTIFState *htifstate, uint64_t val_written)
>           /* HTIF Console */
>           if (cmd == HTIF_CONSOLE_CMD_GETC) {
>               /* this should be a queue, but not yet implemented as such */
> -            htifstate->pending_read = val_written;
> -            htifstate->env->mtohost = 0; /* clear to indicate we read */
> +            s->pending_read = val_written;
> +            s->env->mtohost = 0; /* clear to indicate we read */
>               return;
>           } else if (cmd == HTIF_CONSOLE_CMD_PUTC) {
> -            qemu_chr_fe_write(&htifstate->chr, (uint8_t *)&payload, 1);
> +            qemu_chr_fe_write(&s->chr, (uint8_t *)&payload, 1);
>               resp = 0x100 | (uint8_t)payload;
>           } else {
>               qemu_log("HTIF device %d: unknown command\n", device);
> @@ -194,31 +194,31 @@ static void htif_handle_tohost_write(HTIFState *htifstate, uint64_t val_written)
>        * With this code disabled, qemu works with bbl priv v1.9.1 and v1.10.
>        * HTIF needs protocol documentation and a more complete state machine.
>        *
> -     *  while (!htifstate->fromhost_inprogress &&
> -     *      htifstate->env->mfromhost != 0x0) {
> +     *  while (!s->fromhost_inprogress &&
> +     *      s->env->mfromhost != 0x0) {
>        *  }
>        */
> -    htifstate->env->mfromhost = (val_written >> 48 << 48) | (resp << 16 >> 16);
> -    htifstate->env->mtohost = 0; /* clear to indicate we read */
> +    s->env->mfromhost = (val_written >> 48 << 48) | (resp << 16 >> 16);
> +    s->env->mtohost = 0; /* clear to indicate we read */
>   }
>   
> -#define TOHOST_OFFSET1 (htifstate->tohost_offset)
> -#define TOHOST_OFFSET2 (htifstate->tohost_offset + 4)
> -#define FROMHOST_OFFSET1 (htifstate->fromhost_offset)
> -#define FROMHOST_OFFSET2 (htifstate->fromhost_offset + 4)
> +#define TOHOST_OFFSET1      (s->tohost_offset)
> +#define TOHOST_OFFSET2      (s->tohost_offset + 4)
> +#define FROMHOST_OFFSET1    (s->fromhost_offset)
> +#define FROMHOST_OFFSET2    (s->fromhost_offset + 4)
>   
>   /* CPU wants to read an HTIF register */
>   static uint64_t htif_mm_read(void *opaque, hwaddr addr, unsigned size)
>   {
> -    HTIFState *htifstate = opaque;
> +    HTIFState *s = opaque;
>       if (addr == TOHOST_OFFSET1) {
> -        return htifstate->env->mtohost & 0xFFFFFFFF;
> +        return s->env->mtohost & 0xFFFFFFFF;
>       } else if (addr == TOHOST_OFFSET2) {
> -        return (htifstate->env->mtohost >> 32) & 0xFFFFFFFF;
> +        return (s->env->mtohost >> 32) & 0xFFFFFFFF;
>       } else if (addr == FROMHOST_OFFSET1) {
> -        return htifstate->env->mfromhost & 0xFFFFFFFF;
> +        return s->env->mfromhost & 0xFFFFFFFF;
>       } else if (addr == FROMHOST_OFFSET2) {
> -        return (htifstate->env->mfromhost >> 32) & 0xFFFFFFFF;
> +        return (s->env->mfromhost >> 32) & 0xFFFFFFFF;
>       } else {
>           qemu_log("Invalid htif read: address %016" PRIx64 "\n",
>               (uint64_t)addr);
> @@ -230,25 +230,25 @@ static uint64_t htif_mm_read(void *opaque, hwaddr addr, unsigned size)
>   static void htif_mm_write(void *opaque, hwaddr addr,
>                             uint64_t value, unsigned size)
>   {
> -    HTIFState *htifstate = opaque;
> +    HTIFState *s = opaque;
>       if (addr == TOHOST_OFFSET1) {
> -        if (htifstate->env->mtohost == 0x0) {
> -            htifstate->allow_tohost = 1;
> -            htifstate->env->mtohost = value & 0xFFFFFFFF;
> +        if (s->env->mtohost == 0x0) {
> +            s->allow_tohost = 1;
> +            s->env->mtohost = value & 0xFFFFFFFF;
>           } else {
> -            htifstate->allow_tohost = 0;
> +            s->allow_tohost = 0;
>           }
>       } else if (addr == TOHOST_OFFSET2) {
> -        if (htifstate->allow_tohost) {
> -            htifstate->env->mtohost |= value << 32;
> -            htif_handle_tohost_write(htifstate, htifstate->env->mtohost);
> +        if (s->allow_tohost) {
> +            s->env->mtohost |= value << 32;
> +            htif_handle_tohost_write(s, s->env->mtohost);
>           }
>       } else if (addr == FROMHOST_OFFSET1) {
> -        htifstate->fromhost_inprogress = 1;
> -        htifstate->env->mfromhost = value & 0xFFFFFFFF;
> +        s->fromhost_inprogress = 1;
> +        s->env->mfromhost = value & 0xFFFFFFFF;
>       } else if (addr == FROMHOST_OFFSET2) {
> -        htifstate->env->mfromhost |= value << 32;
> -        htifstate->fromhost_inprogress = 0;
> +        s->env->mfromhost |= value << 32;
> +        s->fromhost_inprogress = 0;
>       } else {
>           qemu_log("Invalid htif write: address %016" PRIx64 "\n",
>               (uint64_t)addr);



^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 05/12] hw/char: riscv_htif: Move registers from CPUArchState to HTIFState
  2022-12-27  6:48 ` [PATCH 05/12] hw/char: riscv_htif: Move registers from CPUArchState to HTIFState Bin Meng
@ 2022-12-27 17:33   ` Daniel Henrique Barboza
  2022-12-28  4:19   ` Alistair Francis
  1 sibling, 0 replies; 35+ messages in thread
From: Daniel Henrique Barboza @ 2022-12-27 17:33 UTC (permalink / raw)
  To: Bin Meng, Alistair Francis, qemu-devel
  Cc: Bin Meng, Marc-André Lureau, Palmer Dabbelt, Paolo Bonzini,
	qemu-riscv



On 12/27/22 03:48, Bin Meng wrote:
> At present for some unknown reason the HTIF registers (fromhost &
> tohost) are defined in the RISC-V CPUArchState. It should really
> be put in the HTIFState struct as it is only meaningful to HTIF.
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
> ---

Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

>
>   include/hw/char/riscv_htif.h |  8 ++++----
>   target/riscv/cpu.h           |  4 ----
>   hw/char/riscv_htif.c         | 35 +++++++++++++++++------------------
>   hw/riscv/spike.c             |  3 +--
>   target/riscv/machine.c       |  6 ++----
>   5 files changed, 24 insertions(+), 32 deletions(-)
>
> diff --git a/include/hw/char/riscv_htif.h b/include/hw/char/riscv_htif.h
> index 6d172ebd6d..55cc352331 100644
> --- a/include/hw/char/riscv_htif.h
> +++ b/include/hw/char/riscv_htif.h
> @@ -23,7 +23,6 @@
>   #include "chardev/char.h"
>   #include "chardev/char-fe.h"
>   #include "exec/memory.h"
> -#include "target/riscv/cpu.h"
>   
>   #define TYPE_HTIF_UART "riscv.htif.uart"
>   
> @@ -31,11 +30,12 @@ typedef struct HTIFState {
>       int allow_tohost;
>       int fromhost_inprogress;
>   
> +    uint64_t tohost;
> +    uint64_t fromhost;
>       hwaddr tohost_offset;
>       hwaddr fromhost_offset;
>       MemoryRegion mmio;
>   
> -    CPURISCVState *env;
>       CharBackend chr;
>       uint64_t pending_read;
>   } HTIFState;
> @@ -51,7 +51,7 @@ void htif_symbol_callback(const char *st_name, int st_info, uint64_t st_value,
>   bool htif_uses_elf_symbols(void);
>   
>   /* legacy pre qom */
> -HTIFState *htif_mm_init(MemoryRegion *address_space, CPURISCVState *env,
> -                        Chardev *chr, uint64_t nonelf_base);
> +HTIFState *htif_mm_init(MemoryRegion *address_space, Chardev *chr,
> +                        uint64_t nonelf_base);
>   
>   #endif
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 443d15a47c..6f04d853dd 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -309,10 +309,6 @@ struct CPUArchState {
>       target_ulong sscratch;
>       target_ulong mscratch;
>   
> -    /* temporary htif regs */
> -    uint64_t mfromhost;
> -    uint64_t mtohost;
> -
>       /* Sstc CSRs */
>       uint64_t stimecmp;
>   
> diff --git a/hw/char/riscv_htif.c b/hw/char/riscv_htif.c
> index f28976b110..3bb0a37a3e 100644
> --- a/hw/char/riscv_htif.c
> +++ b/hw/char/riscv_htif.c
> @@ -100,7 +100,7 @@ static void htif_recv(void *opaque, const uint8_t *buf, int size)
>       uint64_t val_written = s->pending_read;
>       uint64_t resp = 0x100 | *buf;
>   
> -    s->env->mfromhost = (val_written >> 48 << 48) | (resp << 16 >> 16);
> +    s->fromhost = (val_written >> 48 << 48) | (resp << 16 >> 16);
>   }
>   
>   /*
> @@ -175,7 +175,7 @@ static void htif_handle_tohost_write(HTIFState *s, uint64_t val_written)
>           if (cmd == HTIF_CONSOLE_CMD_GETC) {
>               /* this should be a queue, but not yet implemented as such */
>               s->pending_read = val_written;
> -            s->env->mtohost = 0; /* clear to indicate we read */
> +            s->tohost = 0; /* clear to indicate we read */
>               return;
>           } else if (cmd == HTIF_CONSOLE_CMD_PUTC) {
>               qemu_chr_fe_write(&s->chr, (uint8_t *)&payload, 1);
> @@ -195,11 +195,11 @@ static void htif_handle_tohost_write(HTIFState *s, uint64_t val_written)
>        * HTIF needs protocol documentation and a more complete state machine.
>        *
>        *  while (!s->fromhost_inprogress &&
> -     *      s->env->mfromhost != 0x0) {
> +     *      s->fromhost != 0x0) {
>        *  }
>        */
> -    s->env->mfromhost = (val_written >> 48 << 48) | (resp << 16 >> 16);
> -    s->env->mtohost = 0; /* clear to indicate we read */
> +    s->fromhost = (val_written >> 48 << 48) | (resp << 16 >> 16);
> +    s->tohost = 0; /* clear to indicate we read */
>   }
>   
>   #define TOHOST_OFFSET1      (s->tohost_offset)
> @@ -212,13 +212,13 @@ static uint64_t htif_mm_read(void *opaque, hwaddr addr, unsigned size)
>   {
>       HTIFState *s = opaque;
>       if (addr == TOHOST_OFFSET1) {
> -        return s->env->mtohost & 0xFFFFFFFF;
> +        return s->tohost & 0xFFFFFFFF;
>       } else if (addr == TOHOST_OFFSET2) {
> -        return (s->env->mtohost >> 32) & 0xFFFFFFFF;
> +        return (s->tohost >> 32) & 0xFFFFFFFF;
>       } else if (addr == FROMHOST_OFFSET1) {
> -        return s->env->mfromhost & 0xFFFFFFFF;
> +        return s->fromhost & 0xFFFFFFFF;
>       } else if (addr == FROMHOST_OFFSET2) {
> -        return (s->env->mfromhost >> 32) & 0xFFFFFFFF;
> +        return (s->fromhost >> 32) & 0xFFFFFFFF;
>       } else {
>           qemu_log("Invalid htif read: address %016" PRIx64 "\n",
>               (uint64_t)addr);
> @@ -232,22 +232,22 @@ static void htif_mm_write(void *opaque, hwaddr addr,
>   {
>       HTIFState *s = opaque;
>       if (addr == TOHOST_OFFSET1) {
> -        if (s->env->mtohost == 0x0) {
> +        if (s->tohost == 0x0) {
>               s->allow_tohost = 1;
> -            s->env->mtohost = value & 0xFFFFFFFF;
> +            s->tohost = value & 0xFFFFFFFF;
>           } else {
>               s->allow_tohost = 0;
>           }
>       } else if (addr == TOHOST_OFFSET2) {
>           if (s->allow_tohost) {
> -            s->env->mtohost |= value << 32;
> -            htif_handle_tohost_write(s, s->env->mtohost);
> +            s->tohost |= value << 32;
> +            htif_handle_tohost_write(s, s->tohost);
>           }
>       } else if (addr == FROMHOST_OFFSET1) {
>           s->fromhost_inprogress = 1;
> -        s->env->mfromhost = value & 0xFFFFFFFF;
> +        s->fromhost = value & 0xFFFFFFFF;
>       } else if (addr == FROMHOST_OFFSET2) {
> -        s->env->mfromhost |= value << 32;
> +        s->fromhost |= value << 32;
>           s->fromhost_inprogress = 0;
>       } else {
>           qemu_log("Invalid htif write: address %016" PRIx64 "\n",
> @@ -265,8 +265,8 @@ bool htif_uses_elf_symbols(void)
>       return (address_symbol_set == 3) ? true : false;
>   }
>   
> -HTIFState *htif_mm_init(MemoryRegion *address_space, CPURISCVState *env,
> -                        Chardev *chr, uint64_t nonelf_base)
> +HTIFState *htif_mm_init(MemoryRegion *address_space, Chardev *chr,
> +                        uint64_t nonelf_base)
>   {
>       uint64_t base, size, tohost_offset, fromhost_offset;
>   
> @@ -281,7 +281,6 @@ HTIFState *htif_mm_init(MemoryRegion *address_space, CPURISCVState *env,
>       fromhost_offset = fromhost_addr - base;
>   
>       HTIFState *s = g_new0(HTIFState, 1);
> -    s->env = env;
>       s->tohost_offset = tohost_offset;
>       s->fromhost_offset = fromhost_offset;
>       s->pending_read = 0;
> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> index 82cf41ac27..8606331f61 100644
> --- a/hw/riscv/spike.c
> +++ b/hw/riscv/spike.c
> @@ -317,8 +317,7 @@ static void spike_board_init(MachineState *machine)
>                                 fdt_load_addr);
>   
>       /* initialize HTIF using symbols found in load_kernel */
> -    htif_mm_init(system_memory, &s->soc[0].harts[0].env,
> -                 serial_hd(0), memmap[SPIKE_HTIF].base);
> +    htif_mm_init(system_memory, serial_hd(0), memmap[SPIKE_HTIF].base);
>   }
>   
>   static void spike_machine_instance_init(Object *obj)
> diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> index c2a94a82b3..2e8beef06e 100644
> --- a/target/riscv/machine.c
> +++ b/target/riscv/machine.c
> @@ -298,8 +298,8 @@ static const VMStateDescription vmstate_pmu_ctr_state = {
>   
>   const VMStateDescription vmstate_riscv_cpu = {
>       .name = "cpu",
> -    .version_id = 5,
> -    .minimum_version_id = 5,
> +    .version_id = 6,
> +    .minimum_version_id = 6,
>       .post_load = riscv_cpu_post_load,
>       .fields = (VMStateField[]) {
>           VMSTATE_UINTTL_ARRAY(env.gpr, RISCVCPU, 32),
> @@ -349,8 +349,6 @@ const VMStateDescription vmstate_riscv_cpu = {
>           VMSTATE_UINTTL_ARRAY(env.mhpmeventh_val, RISCVCPU, RV_MAX_MHPMEVENTS),
>           VMSTATE_UINTTL(env.sscratch, RISCVCPU),
>           VMSTATE_UINTTL(env.mscratch, RISCVCPU),
> -        VMSTATE_UINT64(env.mfromhost, RISCVCPU),
> -        VMSTATE_UINT64(env.mtohost, RISCVCPU),
>           VMSTATE_UINT64(env.stimecmp, RISCVCPU),
>   
>           VMSTATE_END_OF_LIST()



^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 06/12] hw/char: riscv_htif: Remove forward declarations for non-existent variables
  2022-12-27  6:48 ` [PATCH 06/12] hw/char: riscv_htif: Remove forward declarations for non-existent variables Bin Meng
@ 2022-12-27 17:35   ` Daniel Henrique Barboza
  2022-12-28  4:22   ` Alistair Francis
  1 sibling, 0 replies; 35+ messages in thread
From: Daniel Henrique Barboza @ 2022-12-27 17:35 UTC (permalink / raw)
  To: Bin Meng, Alistair Francis, qemu-devel; +Cc: Anup Patel



On 12/27/22 03:48, Bin Meng wrote:
> There are forward declarations for 'vmstate_htif' and 'htif_io_ops'
> in riscv_htif.h however there are no definitions in the C codes.
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
> ---

Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

>
>   include/hw/char/riscv_htif.h | 3 ---
>   1 file changed, 3 deletions(-)
>
> diff --git a/include/hw/char/riscv_htif.h b/include/hw/char/riscv_htif.h
> index 55cc352331..9e8ebbe017 100644
> --- a/include/hw/char/riscv_htif.h
> +++ b/include/hw/char/riscv_htif.h
> @@ -40,9 +40,6 @@ typedef struct HTIFState {
>       uint64_t pending_read;
>   } HTIFState;
>   
> -extern const VMStateDescription vmstate_htif;
> -extern const MemoryRegionOps htif_io_ops;
> -
>   /* HTIF symbol callback */
>   void htif_symbol_callback(const char *st_name, int st_info, uint64_t st_value,
>       uint64_t st_size);



^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 07/12] hw/char: riscv_htif: Support console output via proxy syscall
  2022-12-27  6:48 ` [PATCH 07/12] hw/char: riscv_htif: Support console output via proxy syscall Bin Meng
@ 2022-12-27 17:37   ` Daniel Henrique Barboza
  2022-12-28  4:30   ` Alistair Francis
  1 sibling, 0 replies; 35+ messages in thread
From: Daniel Henrique Barboza @ 2022-12-27 17:37 UTC (permalink / raw)
  To: Bin Meng, Alistair Francis, qemu-devel
  Cc: Marc-André Lureau, Paolo Bonzini



On 12/27/22 03:48, Bin Meng wrote:
> At present the HTIF proxy syscall is unsupported. On RV32, only
> device 0 is supported so there is no console device for RV32.
> The only way to implement console funtionality on RV32 is to
> support the SYS_WRITE syscall.
>
> With this commit, the Spike machine is able to boot the 32-bit
> OpenSBI generic image.
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
> ---

Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

>
>   hw/char/riscv_htif.c | 17 ++++++++++++++++-
>   1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/hw/char/riscv_htif.c b/hw/char/riscv_htif.c
> index 3bb0a37a3e..1477fc0090 100644
> --- a/hw/char/riscv_htif.c
> +++ b/hw/char/riscv_htif.c
> @@ -48,6 +48,9 @@
>   #define HTIF_CONSOLE_CMD_GETC   0
>   #define HTIF_CONSOLE_CMD_PUTC   1
>   
> +/* PK system call number */
> +#define PK_SYS_WRITE            64
> +
>   static uint64_t fromhost_addr, tohost_addr;
>   static int address_symbol_set;
>   
> @@ -165,7 +168,19 @@ static void htif_handle_tohost_write(HTIFState *s, uint64_t val_written)
>                   int exit_code = payload >> 1;
>                   exit(exit_code);
>               } else {
> -                qemu_log_mask(LOG_UNIMP, "pk syscall proxy not supported\n");
> +                uint64_t syscall[8];
> +                cpu_physical_memory_read(payload, syscall, sizeof(syscall));
> +                if (syscall[0] == PK_SYS_WRITE &&
> +                    syscall[1] == HTIF_DEV_CONSOLE &&
> +                    syscall[3] == HTIF_CONSOLE_CMD_PUTC) {
> +                    uint8_t ch;
> +                    cpu_physical_memory_read(syscall[2], &ch, 1);
> +                    qemu_chr_fe_write(&s->chr, &ch, 1);
> +                    resp = 0x100 | (uint8_t)payload;
> +                } else {
> +                    qemu_log_mask(LOG_UNIMP,
> +                                  "pk syscall proxy not supported\n");
> +                }
>               }
>           } else {
>               qemu_log("HTIF device %d: unknown command\n", device);



^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 08/12] hw/riscv: spike: Remove the out-of-date comments
  2022-12-27  6:48 ` [PATCH 08/12] hw/riscv: spike: Remove the out-of-date comments Bin Meng
@ 2022-12-27 17:37   ` Daniel Henrique Barboza
  2022-12-28  4:30   ` Alistair Francis
  1 sibling, 0 replies; 35+ messages in thread
From: Daniel Henrique Barboza @ 2022-12-27 17:37 UTC (permalink / raw)
  To: Bin Meng, Alistair Francis, qemu-devel
  Cc: Bin Meng, Palmer Dabbelt, qemu-riscv



On 12/27/22 03:48, Bin Meng wrote:
> Spike machine now supports OpenSBI plain binary bios image, so the
> comments are no longer valid.
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
> ---

Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

>
>   hw/riscv/spike.c | 5 -----
>   1 file changed, 5 deletions(-)
>
> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> index 8606331f61..ab0a945f8b 100644
> --- a/hw/riscv/spike.c
> +++ b/hw/riscv/spike.c
> @@ -256,11 +256,6 @@ static void spike_board_init(MachineState *machine)
>       memory_region_add_subregion(system_memory, memmap[SPIKE_MROM].base,
>                                   mask_rom);
>   
> -    /*
> -     * Not like other RISC-V machines that use plain binary bios images,
> -     * 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,



^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 11/12] hw/riscv/boot.c: Introduce riscv_find_firmware()
  2022-12-27  6:48 ` [PATCH 11/12] hw/riscv/boot.c: Introduce riscv_find_firmware() Bin Meng
@ 2022-12-27 17:40   ` Daniel Henrique Barboza
  2022-12-28  4:35   ` Alistair Francis
  1 sibling, 0 replies; 35+ messages in thread
From: Daniel Henrique Barboza @ 2022-12-27 17:40 UTC (permalink / raw)
  To: Bin Meng, Alistair Francis, qemu-devel
  Cc: Bin Meng, Palmer Dabbelt, qemu-riscv



On 12/27/22 03:48, Bin Meng wrote:
> Rename previous riscv_find_firmware() to riscv_find_bios(), and
> introduce a new riscv_find_firmware() to implement the first half
> part of the work done in riscv_find_and_load_firmware().
>
> This new API is helpful for machine that wants to know the final
> chosen firmware file name but does not want to load it.
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
> ---

Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

>
>   include/hw/riscv/boot.h |  2 ++
>   hw/riscv/boot.c         | 39 +++++++++++++++++++++++++--------------
>   2 files changed, 27 insertions(+), 14 deletions(-)
>
> diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
> index 60cf320c88..b273ab22f7 100644
> --- a/include/hw/riscv/boot.h
> +++ b/include/hw/riscv/boot.h
> @@ -38,6 +38,8 @@ target_ulong riscv_find_and_load_firmware(MachineState *machine,
>                                             hwaddr firmware_load_addr,
>                                             symbol_fn_t sym_cb);
>   const char *riscv_default_firmware_name(RISCVHartArrayState *harts);
> +char *riscv_find_firmware(const char *firmware_filename,
> +                          const char *default_machine_firmware);
>   target_ulong riscv_load_firmware(const char *firmware_filename,
>                                    hwaddr firmware_load_addr,
>                                    symbol_fn_t sym_cb);
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index e1a544b1d9..98b80af51b 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -84,11 +84,11 @@ const char *riscv_default_firmware_name(RISCVHartArrayState *harts)
>       return RISCV64_BIOS_BIN;
>   }
>   
> -static char *riscv_find_firmware(const char *firmware_filename)
> +static char *riscv_find_bios(const char *bios_filename)
>   {
>       char *filename;
>   
> -    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, firmware_filename);
> +    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_filename);
>       if (filename == NULL) {
>           if (!qtest_enabled()) {
>               /*
> @@ -97,8 +97,8 @@ static char *riscv_find_firmware(const char *firmware_filename)
>                * 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);
> +            error_report("Unable to find the RISC-V BIOS \"%s\"",
> +                         bios_filename);
>               exit(1);
>           }
>       }
> @@ -106,25 +106,36 @@ static char *riscv_find_firmware(const char *firmware_filename)
>       return filename;
>   }
>   
> -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,
> +                          const char *default_machine_firmware)
>   {
> -    char *firmware_filename = NULL;
> -    target_ulong firmware_end_addr = firmware_load_addr;
> +    char *filename = NULL;
>   
> -    if ((!machine->firmware) || (!strcmp(machine->firmware, "default"))) {
> +    if ((!firmware_filename) || (!strcmp(firmware_filename, "default"))) {
>           /*
>            * The user didn't specify -bios, or has specified "-bios default".
>            * That means we are going to load the OpenSBI binary included in
>            * the QEMU source.
>            */
> -        firmware_filename = riscv_find_firmware(default_machine_firmware);
> -    } else if (strcmp(machine->firmware, "none")) {
> -        firmware_filename = riscv_find_firmware(machine->firmware);
> +        filename = riscv_find_bios(default_machine_firmware);
> +    } else if (strcmp(firmware_filename, "none")) {
> +        filename = riscv_find_bios(firmware_filename);
>       }
>   
> +    return filename;
> +}
> +
> +target_ulong riscv_find_and_load_firmware(MachineState *machine,
> +                                          const char *default_machine_firmware,
> +                                          hwaddr firmware_load_addr,
> +                                          symbol_fn_t sym_cb)
> +{
> +    char *firmware_filename;
> +    target_ulong firmware_end_addr = firmware_load_addr;
> +
> +    firmware_filename = riscv_find_firmware(machine->firmware,
> +                                            default_machine_firmware);
> +
>       if (firmware_filename) {
>           /* If not "none" load the firmware */
>           firmware_end_addr = riscv_load_firmware(firmware_filename,



^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 00/12] hw/riscv: Improve Spike HTIF emulation fidelity
  2022-12-27  6:48 [PATCH 00/12] hw/riscv: Improve Spike HTIF emulation fidelity Bin Meng
                   ` (11 preceding siblings ...)
  2022-12-27  6:48 ` [PATCH 12/12] hw/riscv: spike: Decouple create_fdt() dependency to ELF loading Bin Meng
@ 2022-12-27 17:51 ` Daniel Henrique Barboza
  2022-12-28  3:58   ` Bin Meng
  12 siblings, 1 reply; 35+ messages in thread
From: Daniel Henrique Barboza @ 2022-12-27 17:51 UTC (permalink / raw)
  To: Bin Meng, Alistair Francis, qemu-devel
  Cc: Anup Patel, Bin Meng, Marc-André Lureau, Palmer Dabbelt,
	Paolo Bonzini, qemu-riscv



On 12/27/22 03:48, Bin Meng wrote:
> At present the 32-bit OpenSBI generic firmware image does not boot on
> Spike, only 64-bit image can. This is due to the HTIF emulation does
> not implement the proxy syscall interface which is required for the
> 32-bit HTIF console output.
>
> An OpenSBI bug fix [1] is also needed when booting the plain binary image.
>
> With this series plus the above OpenSBI fix, both 32-bit OpenSBI BIN & ELF
> images can boot on QEMU 'spike' machine.
>
> [1] https://patchwork.ozlabs.org/project/opensbi/patch/20221226033603.1860569-1-bmeng@tinylab.org/

Aside from a nit in patch 12/12, LGTM. I've tested with a patched version of
Opensbi including [1] and I can get terminal output with riscv32 spike:

$ ./qemu-system-riscv32 -M spike -display none -nographic
-bios ../../opensbi/build/platform/generic/firmware/fw_payload.bin

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


Speaking of [1], it seems like the fix went a bit too late for the opensbi 1.2 release.
Assuming that [1] is accepted, it would be nice if we could bake in this fix on top of the
1.2 release when updating the QEMU roms.


Thanks,

Daniel

>
>
> Bin Meng (10):
>    hw/char: riscv_htif: Avoid using magic numbers
>    hw/char: riscv_htif: Drop {to,from}host_size in HTIFState
>    hw/char: riscv_htif: Drop useless assignment of memory region
>    hw/char: riscv_htif: Use conventional 's' for HTIFState
>    hw/char: riscv_htif: Move registers from CPUArchState to HTIFState
>    hw/char: riscv_htif: Remove forward declarations for non-existent
>      variables
>    hw/char: riscv_htif: Support console output via proxy syscall
>    hw/riscv: spike: Remove the out-of-date comments
>    hw/riscv/boot.c: Introduce riscv_find_firmware()
>    hw/riscv: spike: Decouple create_fdt() dependency to ELF loading
>
> Daniel Henrique Barboza (2):
>    hw/riscv/boot.c: make riscv_find_firmware() static
>    hw/riscv/boot.c: introduce riscv_default_firmware_name()
>
>   include/hw/char/riscv_htif.h |  19 +---
>   include/hw/riscv/boot.h      |   4 +-
>   target/riscv/cpu.h           |   4 -
>   hw/char/riscv_htif.c         | 172 +++++++++++++++++++++--------------
>   hw/riscv/boot.c              |  76 ++++++++++------
>   hw/riscv/sifive_u.c          |  11 +--
>   hw/riscv/spike.c             |  59 ++++++++----
>   hw/riscv/virt.c              |  10 +-
>   target/riscv/machine.c       |   6 +-
>   9 files changed, 212 insertions(+), 149 deletions(-)
>



^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 01/12] hw/char: riscv_htif: Avoid using magic numbers
  2022-12-27  6:48 ` [PATCH 01/12] hw/char: riscv_htif: Avoid using magic numbers Bin Meng
  2022-12-27 17:28   ` Daniel Henrique Barboza
@ 2022-12-28  3:31   ` Alistair Francis
  1 sibling, 0 replies; 35+ messages in thread
From: Alistair Francis @ 2022-12-28  3:31 UTC (permalink / raw)
  To: Bin Meng
  Cc: Alistair Francis, qemu-devel, Daniel Henrique Barboza,
	Marc-André Lureau, Paolo Bonzini

On Tue, Dec 27, 2022 at 4:50 PM Bin Meng <bmeng@tinylab.org> wrote:
>
> The Spike HTIF is poorly documented. The only relevant info we can
> get from the internet is from Andrew Waterman at [1].
>
> Add a comment block before htif_handle_tohost_write() to explain
> the tohost register format, and use meaningful macros intead of
> magic numbers in the codes.
>
> While we are here, corret 2 multi-line comment blocks that have
> wrong format.
>
> Link: https://github.com/riscv-software-src/riscv-isa-sim/issues/364 [1]
> Signed-off-by: Bin Meng <bmeng@tinylab.org>

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

Alistair

> ---
>
>  hw/char/riscv_htif.c | 72 ++++++++++++++++++++++++++++++++------------
>  1 file changed, 52 insertions(+), 20 deletions(-)
>
> diff --git a/hw/char/riscv_htif.c b/hw/char/riscv_htif.c
> index 6577f0e640..088556bb04 100644
> --- a/hw/char/riscv_htif.c
> +++ b/hw/char/riscv_htif.c
> @@ -38,6 +38,16 @@
>          }                                                                      \
>      } while (0)
>
> +#define HTIF_DEV_SHIFT          56
> +#define HTIF_CMD_SHIFT          48
> +
> +#define HTIF_DEV_SYSTEM         0
> +#define HTIF_DEV_CONSOLE        1
> +
> +#define HTIF_SYSTEM_CMD_SYSCALL 0
> +#define HTIF_CONSOLE_CMD_GETC   0
> +#define HTIF_CONSOLE_CMD_PUTC   1
> +
>  static uint64_t fromhost_addr, tohost_addr;
>  static int address_symbol_set;
>
> @@ -81,9 +91,11 @@ static void htif_recv(void *opaque, const uint8_t *buf, int size)
>          return;
>      }
>
> -    /* TODO - we need to check whether mfromhost is zero which indicates
> -              the device is ready to receive. The current implementation
> -              will drop characters */
> +    /*
> +     * TODO - we need to check whether mfromhost is zero which indicates
> +     *        the device is ready to receive. The current implementation
> +     *        will drop characters
> +     */
>
>      uint64_t val_written = htifstate->pending_read;
>      uint64_t resp = 0x100 | *buf;
> @@ -110,10 +122,30 @@ static int htif_be_change(void *opaque)
>      return 0;
>  }
>
> +/*
> + * See below the tohost register format.
> + *
> + * Bits 63:56 indicate the "device".
> + * Bits 55:48 indicate the "command".
> + *
> + * Device 0 is the syscall device, which is used to emulate Unixy syscalls.
> + * It only implements command 0, which has two subfunctions:
> + * - If bit 0 is clear, then bits 47:0 represent a pointer to a struct
> + *   describing the syscall.
> + * - If bit 1 is set, then bits 47:1 represent an exit code, with a zero
> + *   value indicating success and other values indicating failure.
> + *
> + * Device 1 is the blocking character device.
> + * - Command 0 reads a character
> + * - Command 1 writes a character from the 8 LSBs of tohost
> + *
> + * For RV32, the tohost register is zero-extended, so only device=0 and
> + * command=0 (i.e. HTIF syscalls/exit codes) are supported.
> + */
>  static void htif_handle_tohost_write(HTIFState *htifstate, uint64_t val_written)
>  {
> -    uint8_t device = val_written >> 56;
> -    uint8_t cmd = val_written >> 48;
> +    uint8_t device = val_written >> HTIF_DEV_SHIFT;
> +    uint8_t cmd = val_written >> HTIF_CMD_SHIFT;
>      uint64_t payload = val_written & 0xFFFFFFFFFFFFULL;
>      int resp = 0;
>
> @@ -125,9 +157,9 @@ static void htif_handle_tohost_write(HTIFState *htifstate, uint64_t val_written)
>       * 0: riscv-tests Pass/Fail Reporting Only (no syscall proxy)
>       * 1: Console
>       */
> -    if (unlikely(device == 0x0)) {
> +    if (unlikely(device == HTIF_DEV_SYSTEM)) {
>          /* frontend syscall handler, shutdown and exit code support */
> -        if (cmd == 0x0) {
> +        if (cmd == HTIF_SYSTEM_CMD_SYSCALL) {
>              if (payload & 0x1) {
>                  /* exit code */
>                  int exit_code = payload >> 1;
> @@ -138,14 +170,14 @@ static void htif_handle_tohost_write(HTIFState *htifstate, uint64_t val_written)
>          } else {
>              qemu_log("HTIF device %d: unknown command\n", device);
>          }
> -    } else if (likely(device == 0x1)) {
> +    } else if (likely(device == HTIF_DEV_CONSOLE)) {
>          /* HTIF Console */
> -        if (cmd == 0x0) {
> +        if (cmd == HTIF_CONSOLE_CMD_GETC) {
>              /* this should be a queue, but not yet implemented as such */
>              htifstate->pending_read = val_written;
>              htifstate->env->mtohost = 0; /* clear to indicate we read */
>              return;
> -        } else if (cmd == 0x1) {
> +        } else if (cmd == HTIF_CONSOLE_CMD_PUTC) {
>              qemu_chr_fe_write(&htifstate->chr, (uint8_t *)&payload, 1);
>              resp = 0x100 | (uint8_t)payload;
>          } else {
> @@ -157,15 +189,15 @@ static void htif_handle_tohost_write(HTIFState *htifstate, uint64_t val_written)
>              " payload: %016" PRIx64, device, cmd, payload & 0xFF, payload);
>      }
>      /*
> -     * - latest bbl does not set fromhost to 0 if there is a value in tohost
> -     * - with this code enabled, qemu hangs waiting for fromhost to go to 0
> -     * - with this code disabled, qemu works with bbl priv v1.9.1 and v1.10
> -     * - HTIF needs protocol documentation and a more complete state machine
> -
> -        while (!htifstate->fromhost_inprogress &&
> -            htifstate->env->mfromhost != 0x0) {
> -        }
> -    */
> +     * Latest bbl does not set fromhost to 0 if there is a value in tohost.
> +     * With this code enabled, qemu hangs waiting for fromhost to go to 0.
> +     * With this code disabled, qemu works with bbl priv v1.9.1 and v1.10.
> +     * HTIF needs protocol documentation and a more complete state machine.
> +     *
> +     *  while (!htifstate->fromhost_inprogress &&
> +     *      htifstate->env->mfromhost != 0x0) {
> +     *  }
> +     */
>      htifstate->env->mfromhost = (val_written >> 48 << 48) | (resp << 16 >> 16);
>      htifstate->env->mtohost = 0; /* clear to indicate we read */
>  }
> @@ -196,7 +228,7 @@ static uint64_t htif_mm_read(void *opaque, hwaddr addr, unsigned size)
>
>  /* CPU wrote to an HTIF register */
>  static void htif_mm_write(void *opaque, hwaddr addr,
> -                            uint64_t value, unsigned size)
> +                          uint64_t value, unsigned size)
>  {
>      HTIFState *htifstate = opaque;
>      if (addr == TOHOST_OFFSET1) {
> --
> 2.34.1
>
>


^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 02/12] hw/char: riscv_htif: Drop {to, from}host_size in HTIFState
  2022-12-27  6:48 ` [PATCH 02/12] hw/char: riscv_htif: Drop {to, from}host_size in HTIFState Bin Meng
  2022-12-27 17:29   ` [PATCH 02/12] hw/char: riscv_htif: Drop {to,from}host_size " Daniel Henrique Barboza
@ 2022-12-28  3:32   ` Alistair Francis
  1 sibling, 0 replies; 35+ messages in thread
From: Alistair Francis @ 2022-12-28  3:32 UTC (permalink / raw)
  To: Bin Meng
  Cc: Alistair Francis, qemu-devel, Daniel Henrique Barboza, Anup Patel

On Tue, Dec 27, 2022 at 4:56 PM Bin Meng <bmeng@tinylab.org> wrote:
>
> These are not used anywhere. Drop them.
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>

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

Alistair

> ---
>
>  include/hw/char/riscv_htif.h | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/include/hw/char/riscv_htif.h b/include/hw/char/riscv_htif.h
> index f888ac1b30..3eccc1914f 100644
> --- a/include/hw/char/riscv_htif.h
> +++ b/include/hw/char/riscv_htif.h
> @@ -33,8 +33,6 @@ typedef struct HTIFState {
>
>      hwaddr tohost_offset;
>      hwaddr fromhost_offset;
> -    uint64_t tohost_size;
> -    uint64_t fromhost_size;
>      MemoryRegion mmio;
>      MemoryRegion *address_space;
>      MemoryRegion *main_mem;
> --
> 2.34.1
>
>


^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 03/12] hw/char: riscv_htif: Drop useless assignment of memory region
  2022-12-27  6:48 ` [PATCH 03/12] hw/char: riscv_htif: Drop useless assignment of memory region Bin Meng
  2022-12-27 17:30   ` Daniel Henrique Barboza
@ 2022-12-28  3:33   ` Alistair Francis
  1 sibling, 0 replies; 35+ messages in thread
From: Alistair Francis @ 2022-12-28  3:33 UTC (permalink / raw)
  To: Bin Meng
  Cc: Alistair Francis, qemu-devel, Daniel Henrique Barboza, Bin Meng,
	Marc-André Lureau, Palmer Dabbelt, Paolo Bonzini,
	qemu-riscv

On Tue, Dec 27, 2022 at 4:50 PM Bin Meng <bmeng@tinylab.org> wrote:
>
> struct HTIFState has 3 members for address space and memory region,
> and are initialized during htif_mm_init(). But they are actually
> useless. Drop them.
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>

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

Alistair

> ---
>
>  include/hw/char/riscv_htif.h | 7 ++-----
>  hw/char/riscv_htif.c         | 7 ++-----
>  hw/riscv/spike.c             | 5 ++---
>  3 files changed, 6 insertions(+), 13 deletions(-)
>
> diff --git a/include/hw/char/riscv_htif.h b/include/hw/char/riscv_htif.h
> index 3eccc1914f..6d172ebd6d 100644
> --- a/include/hw/char/riscv_htif.h
> +++ b/include/hw/char/riscv_htif.h
> @@ -34,9 +34,6 @@ typedef struct HTIFState {
>      hwaddr tohost_offset;
>      hwaddr fromhost_offset;
>      MemoryRegion mmio;
> -    MemoryRegion *address_space;
> -    MemoryRegion *main_mem;
> -    void *main_mem_ram_ptr;
>
>      CPURISCVState *env;
>      CharBackend chr;
> @@ -54,7 +51,7 @@ void htif_symbol_callback(const char *st_name, int st_info, uint64_t st_value,
>  bool htif_uses_elf_symbols(void);
>
>  /* legacy pre qom */
> -HTIFState *htif_mm_init(MemoryRegion *address_space, MemoryRegion *main_mem,
> -    CPURISCVState *env, Chardev *chr, uint64_t nonelf_base);
> +HTIFState *htif_mm_init(MemoryRegion *address_space, CPURISCVState *env,
> +                        Chardev *chr, uint64_t nonelf_base);
>
>  #endif
> diff --git a/hw/char/riscv_htif.c b/hw/char/riscv_htif.c
> index 088556bb04..e7e319ca1d 100644
> --- a/hw/char/riscv_htif.c
> +++ b/hw/char/riscv_htif.c
> @@ -265,8 +265,8 @@ bool htif_uses_elf_symbols(void)
>      return (address_symbol_set == 3) ? true : false;
>  }
>
> -HTIFState *htif_mm_init(MemoryRegion *address_space, MemoryRegion *main_mem,
> -    CPURISCVState *env, Chardev *chr, uint64_t nonelf_base)
> +HTIFState *htif_mm_init(MemoryRegion *address_space, CPURISCVState *env,
> +                        Chardev *chr, uint64_t nonelf_base)
>  {
>      uint64_t base, size, tohost_offset, fromhost_offset;
>
> @@ -281,9 +281,6 @@ HTIFState *htif_mm_init(MemoryRegion *address_space, MemoryRegion *main_mem,
>      fromhost_offset = fromhost_addr - base;
>
>      HTIFState *s = g_new0(HTIFState, 1);
> -    s->address_space = address_space;
> -    s->main_mem = main_mem;
> -    s->main_mem_ram_ptr = memory_region_get_ram_ptr(main_mem);
>      s->env = env;
>      s->tohost_offset = tohost_offset;
>      s->fromhost_offset = fromhost_offset;
> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> index 1e1d752c00..82cf41ac27 100644
> --- a/hw/riscv/spike.c
> +++ b/hw/riscv/spike.c
> @@ -317,9 +317,8 @@ static void spike_board_init(MachineState *machine)
>                                fdt_load_addr);
>
>      /* initialize HTIF using symbols found in load_kernel */
> -    htif_mm_init(system_memory, mask_rom,
> -                 &s->soc[0].harts[0].env, serial_hd(0),
> -                 memmap[SPIKE_HTIF].base);
> +    htif_mm_init(system_memory, &s->soc[0].harts[0].env,
> +                 serial_hd(0), memmap[SPIKE_HTIF].base);
>  }
>
>  static void spike_machine_instance_init(Object *obj)
> --
> 2.34.1
>
>


^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 04/12] hw/char: riscv_htif: Use conventional 's' for HTIFState
  2022-12-27  6:48 ` [PATCH 04/12] hw/char: riscv_htif: Use conventional 's' for HTIFState Bin Meng
  2022-12-27 17:32   ` Daniel Henrique Barboza
@ 2022-12-28  3:35   ` Alistair Francis
  1 sibling, 0 replies; 35+ messages in thread
From: Alistair Francis @ 2022-12-28  3:35 UTC (permalink / raw)
  To: Bin Meng
  Cc: Alistair Francis, qemu-devel, Daniel Henrique Barboza,
	Marc-André Lureau, Paolo Bonzini

On Tue, Dec 27, 2022 at 4:52 PM Bin Meng <bmeng@tinylab.org> wrote:
>
> QEMU source codes tend to use 's' to represent the hardware state.
> Let's use it for HTIFState.
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>

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

Alistair

> ---
>
>  hw/char/riscv_htif.c | 64 ++++++++++++++++++++++----------------------
>  1 file changed, 32 insertions(+), 32 deletions(-)
>
> diff --git a/hw/char/riscv_htif.c b/hw/char/riscv_htif.c
> index e7e319ca1d..f28976b110 100644
> --- a/hw/char/riscv_htif.c
> +++ b/hw/char/riscv_htif.c
> @@ -85,7 +85,7 @@ static int htif_can_recv(void *opaque)
>   */
>  static void htif_recv(void *opaque, const uint8_t *buf, int size)
>  {
> -    HTIFState *htifstate = opaque;
> +    HTIFState *s = opaque;
>
>      if (size != 1) {
>          return;
> @@ -97,10 +97,10 @@ static void htif_recv(void *opaque, const uint8_t *buf, int size)
>       *        will drop characters
>       */
>
> -    uint64_t val_written = htifstate->pending_read;
> +    uint64_t val_written = s->pending_read;
>      uint64_t resp = 0x100 | *buf;
>
> -    htifstate->env->mfromhost = (val_written >> 48 << 48) | (resp << 16 >> 16);
> +    s->env->mfromhost = (val_written >> 48 << 48) | (resp << 16 >> 16);
>  }
>
>  /*
> @@ -142,7 +142,7 @@ static int htif_be_change(void *opaque)
>   * For RV32, the tohost register is zero-extended, so only device=0 and
>   * command=0 (i.e. HTIF syscalls/exit codes) are supported.
>   */
> -static void htif_handle_tohost_write(HTIFState *htifstate, uint64_t val_written)
> +static void htif_handle_tohost_write(HTIFState *s, uint64_t val_written)
>  {
>      uint8_t device = val_written >> HTIF_DEV_SHIFT;
>      uint8_t cmd = val_written >> HTIF_CMD_SHIFT;
> @@ -174,11 +174,11 @@ static void htif_handle_tohost_write(HTIFState *htifstate, uint64_t val_written)
>          /* HTIF Console */
>          if (cmd == HTIF_CONSOLE_CMD_GETC) {
>              /* this should be a queue, but not yet implemented as such */
> -            htifstate->pending_read = val_written;
> -            htifstate->env->mtohost = 0; /* clear to indicate we read */
> +            s->pending_read = val_written;
> +            s->env->mtohost = 0; /* clear to indicate we read */
>              return;
>          } else if (cmd == HTIF_CONSOLE_CMD_PUTC) {
> -            qemu_chr_fe_write(&htifstate->chr, (uint8_t *)&payload, 1);
> +            qemu_chr_fe_write(&s->chr, (uint8_t *)&payload, 1);
>              resp = 0x100 | (uint8_t)payload;
>          } else {
>              qemu_log("HTIF device %d: unknown command\n", device);
> @@ -194,31 +194,31 @@ static void htif_handle_tohost_write(HTIFState *htifstate, uint64_t val_written)
>       * With this code disabled, qemu works with bbl priv v1.9.1 and v1.10.
>       * HTIF needs protocol documentation and a more complete state machine.
>       *
> -     *  while (!htifstate->fromhost_inprogress &&
> -     *      htifstate->env->mfromhost != 0x0) {
> +     *  while (!s->fromhost_inprogress &&
> +     *      s->env->mfromhost != 0x0) {
>       *  }
>       */
> -    htifstate->env->mfromhost = (val_written >> 48 << 48) | (resp << 16 >> 16);
> -    htifstate->env->mtohost = 0; /* clear to indicate we read */
> +    s->env->mfromhost = (val_written >> 48 << 48) | (resp << 16 >> 16);
> +    s->env->mtohost = 0; /* clear to indicate we read */
>  }
>
> -#define TOHOST_OFFSET1 (htifstate->tohost_offset)
> -#define TOHOST_OFFSET2 (htifstate->tohost_offset + 4)
> -#define FROMHOST_OFFSET1 (htifstate->fromhost_offset)
> -#define FROMHOST_OFFSET2 (htifstate->fromhost_offset + 4)
> +#define TOHOST_OFFSET1      (s->tohost_offset)
> +#define TOHOST_OFFSET2      (s->tohost_offset + 4)
> +#define FROMHOST_OFFSET1    (s->fromhost_offset)
> +#define FROMHOST_OFFSET2    (s->fromhost_offset + 4)
>
>  /* CPU wants to read an HTIF register */
>  static uint64_t htif_mm_read(void *opaque, hwaddr addr, unsigned size)
>  {
> -    HTIFState *htifstate = opaque;
> +    HTIFState *s = opaque;
>      if (addr == TOHOST_OFFSET1) {
> -        return htifstate->env->mtohost & 0xFFFFFFFF;
> +        return s->env->mtohost & 0xFFFFFFFF;
>      } else if (addr == TOHOST_OFFSET2) {
> -        return (htifstate->env->mtohost >> 32) & 0xFFFFFFFF;
> +        return (s->env->mtohost >> 32) & 0xFFFFFFFF;
>      } else if (addr == FROMHOST_OFFSET1) {
> -        return htifstate->env->mfromhost & 0xFFFFFFFF;
> +        return s->env->mfromhost & 0xFFFFFFFF;
>      } else if (addr == FROMHOST_OFFSET2) {
> -        return (htifstate->env->mfromhost >> 32) & 0xFFFFFFFF;
> +        return (s->env->mfromhost >> 32) & 0xFFFFFFFF;
>      } else {
>          qemu_log("Invalid htif read: address %016" PRIx64 "\n",
>              (uint64_t)addr);
> @@ -230,25 +230,25 @@ static uint64_t htif_mm_read(void *opaque, hwaddr addr, unsigned size)
>  static void htif_mm_write(void *opaque, hwaddr addr,
>                            uint64_t value, unsigned size)
>  {
> -    HTIFState *htifstate = opaque;
> +    HTIFState *s = opaque;
>      if (addr == TOHOST_OFFSET1) {
> -        if (htifstate->env->mtohost == 0x0) {
> -            htifstate->allow_tohost = 1;
> -            htifstate->env->mtohost = value & 0xFFFFFFFF;
> +        if (s->env->mtohost == 0x0) {
> +            s->allow_tohost = 1;
> +            s->env->mtohost = value & 0xFFFFFFFF;
>          } else {
> -            htifstate->allow_tohost = 0;
> +            s->allow_tohost = 0;
>          }
>      } else if (addr == TOHOST_OFFSET2) {
> -        if (htifstate->allow_tohost) {
> -            htifstate->env->mtohost |= value << 32;
> -            htif_handle_tohost_write(htifstate, htifstate->env->mtohost);
> +        if (s->allow_tohost) {
> +            s->env->mtohost |= value << 32;
> +            htif_handle_tohost_write(s, s->env->mtohost);
>          }
>      } else if (addr == FROMHOST_OFFSET1) {
> -        htifstate->fromhost_inprogress = 1;
> -        htifstate->env->mfromhost = value & 0xFFFFFFFF;
> +        s->fromhost_inprogress = 1;
> +        s->env->mfromhost = value & 0xFFFFFFFF;
>      } else if (addr == FROMHOST_OFFSET2) {
> -        htifstate->env->mfromhost |= value << 32;
> -        htifstate->fromhost_inprogress = 0;
> +        s->env->mfromhost |= value << 32;
> +        s->fromhost_inprogress = 0;
>      } else {
>          qemu_log("Invalid htif write: address %016" PRIx64 "\n",
>              (uint64_t)addr);
> --
> 2.34.1
>
>


^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 00/12] hw/riscv: Improve Spike HTIF emulation fidelity
  2022-12-27 17:51 ` [PATCH 00/12] hw/riscv: Improve Spike HTIF emulation fidelity Daniel Henrique Barboza
@ 2022-12-28  3:58   ` Bin Meng
  2022-12-28  4:21     ` Alistair Francis
  0 siblings, 1 reply; 35+ messages in thread
From: Bin Meng @ 2022-12-28  3:58 UTC (permalink / raw)
  To: Daniel Henrique Barboza
  Cc: Bin Meng, Alistair Francis, qemu-devel, Anup Patel, Bin Meng,
	Marc-André Lureau, Palmer Dabbelt, Paolo Bonzini,
	qemu-riscv

Hi Daniel,

On Wed, Dec 28, 2022 at 1:52 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
>
>
> On 12/27/22 03:48, Bin Meng wrote:
> > At present the 32-bit OpenSBI generic firmware image does not boot on
> > Spike, only 64-bit image can. This is due to the HTIF emulation does
> > not implement the proxy syscall interface which is required for the
> > 32-bit HTIF console output.
> >
> > An OpenSBI bug fix [1] is also needed when booting the plain binary image.
> >
> > With this series plus the above OpenSBI fix, both 32-bit OpenSBI BIN & ELF
> > images can boot on QEMU 'spike' machine.
> >
> > [1] https://patchwork.ozlabs.org/project/opensbi/patch/20221226033603.1860569-1-bmeng@tinylab.org/
>
> Aside from a nit in patch 12/12, LGTM. I've tested with a patched version of
> Opensbi including [1] and I can get terminal output with riscv32 spike:
>
> $ ./qemu-system-riscv32 -M spike -display none -nographic
> -bios ../../opensbi/build/platform/generic/firmware/fw_payload.bin
>
> OpenSBI v1.1-112-g6ce00f8
>     ____                    _____ ____ _____
>    / __ \                  / ____|  _ \_   _|
>   | |  | |_ __   ___ _ __ | (___ | |_) || |
>   | |  | | '_ \ / _ \ '_ \ \___ \|  _ < | |
>   | |__| | |_) |  __/ | | |____) | |_) || |_
>    \____/| .__/ \___|_| |_|_____/|____/_____|
>          | |
>          |_|
> (.......)
>
>
> Speaking of [1], it seems like the fix went a bit too late for the opensbi 1.2 release.
> Assuming that [1] is accepted, it would be nice if we could bake in this fix on top of the
> 1.2 release when updating the QEMU roms.
>

Thanks for the review and testing!

Regarding whether we can cherry-pick the fix on top of OpenSBI 1.2, I
am not sure if that's allowed by the policy.

Alistair, do you know?

Regards,
Bin


^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 05/12] hw/char: riscv_htif: Move registers from CPUArchState to HTIFState
  2022-12-27  6:48 ` [PATCH 05/12] hw/char: riscv_htif: Move registers from CPUArchState to HTIFState Bin Meng
  2022-12-27 17:33   ` Daniel Henrique Barboza
@ 2022-12-28  4:19   ` Alistair Francis
  1 sibling, 0 replies; 35+ messages in thread
From: Alistair Francis @ 2022-12-28  4:19 UTC (permalink / raw)
  To: Bin Meng
  Cc: Alistair Francis, qemu-devel, Daniel Henrique Barboza, Bin Meng,
	Marc-André Lureau, Palmer Dabbelt, Paolo Bonzini,
	qemu-riscv

On Tue, Dec 27, 2022 at 4:55 PM Bin Meng <bmeng@tinylab.org> wrote:
>
> At present for some unknown reason the HTIF registers (fromhost &
> tohost) are defined in the RISC-V CPUArchState. It should really
> be put in the HTIFState struct as it is only meaningful to HTIF.
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>

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

Alistair

> ---
>
>  include/hw/char/riscv_htif.h |  8 ++++----
>  target/riscv/cpu.h           |  4 ----
>  hw/char/riscv_htif.c         | 35 +++++++++++++++++------------------
>  hw/riscv/spike.c             |  3 +--
>  target/riscv/machine.c       |  6 ++----
>  5 files changed, 24 insertions(+), 32 deletions(-)
>
> diff --git a/include/hw/char/riscv_htif.h b/include/hw/char/riscv_htif.h
> index 6d172ebd6d..55cc352331 100644
> --- a/include/hw/char/riscv_htif.h
> +++ b/include/hw/char/riscv_htif.h
> @@ -23,7 +23,6 @@
>  #include "chardev/char.h"
>  #include "chardev/char-fe.h"
>  #include "exec/memory.h"
> -#include "target/riscv/cpu.h"
>
>  #define TYPE_HTIF_UART "riscv.htif.uart"
>
> @@ -31,11 +30,12 @@ typedef struct HTIFState {
>      int allow_tohost;
>      int fromhost_inprogress;
>
> +    uint64_t tohost;
> +    uint64_t fromhost;
>      hwaddr tohost_offset;
>      hwaddr fromhost_offset;
>      MemoryRegion mmio;
>
> -    CPURISCVState *env;
>      CharBackend chr;
>      uint64_t pending_read;
>  } HTIFState;
> @@ -51,7 +51,7 @@ void htif_symbol_callback(const char *st_name, int st_info, uint64_t st_value,
>  bool htif_uses_elf_symbols(void);
>
>  /* legacy pre qom */
> -HTIFState *htif_mm_init(MemoryRegion *address_space, CPURISCVState *env,
> -                        Chardev *chr, uint64_t nonelf_base);
> +HTIFState *htif_mm_init(MemoryRegion *address_space, Chardev *chr,
> +                        uint64_t nonelf_base);
>
>  #endif
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 443d15a47c..6f04d853dd 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -309,10 +309,6 @@ struct CPUArchState {
>      target_ulong sscratch;
>      target_ulong mscratch;
>
> -    /* temporary htif regs */
> -    uint64_t mfromhost;
> -    uint64_t mtohost;
> -
>      /* Sstc CSRs */
>      uint64_t stimecmp;
>
> diff --git a/hw/char/riscv_htif.c b/hw/char/riscv_htif.c
> index f28976b110..3bb0a37a3e 100644
> --- a/hw/char/riscv_htif.c
> +++ b/hw/char/riscv_htif.c
> @@ -100,7 +100,7 @@ static void htif_recv(void *opaque, const uint8_t *buf, int size)
>      uint64_t val_written = s->pending_read;
>      uint64_t resp = 0x100 | *buf;
>
> -    s->env->mfromhost = (val_written >> 48 << 48) | (resp << 16 >> 16);
> +    s->fromhost = (val_written >> 48 << 48) | (resp << 16 >> 16);
>  }
>
>  /*
> @@ -175,7 +175,7 @@ static void htif_handle_tohost_write(HTIFState *s, uint64_t val_written)
>          if (cmd == HTIF_CONSOLE_CMD_GETC) {
>              /* this should be a queue, but not yet implemented as such */
>              s->pending_read = val_written;
> -            s->env->mtohost = 0; /* clear to indicate we read */
> +            s->tohost = 0; /* clear to indicate we read */
>              return;
>          } else if (cmd == HTIF_CONSOLE_CMD_PUTC) {
>              qemu_chr_fe_write(&s->chr, (uint8_t *)&payload, 1);
> @@ -195,11 +195,11 @@ static void htif_handle_tohost_write(HTIFState *s, uint64_t val_written)
>       * HTIF needs protocol documentation and a more complete state machine.
>       *
>       *  while (!s->fromhost_inprogress &&
> -     *      s->env->mfromhost != 0x0) {
> +     *      s->fromhost != 0x0) {
>       *  }
>       */
> -    s->env->mfromhost = (val_written >> 48 << 48) | (resp << 16 >> 16);
> -    s->env->mtohost = 0; /* clear to indicate we read */
> +    s->fromhost = (val_written >> 48 << 48) | (resp << 16 >> 16);
> +    s->tohost = 0; /* clear to indicate we read */
>  }
>
>  #define TOHOST_OFFSET1      (s->tohost_offset)
> @@ -212,13 +212,13 @@ static uint64_t htif_mm_read(void *opaque, hwaddr addr, unsigned size)
>  {
>      HTIFState *s = opaque;
>      if (addr == TOHOST_OFFSET1) {
> -        return s->env->mtohost & 0xFFFFFFFF;
> +        return s->tohost & 0xFFFFFFFF;
>      } else if (addr == TOHOST_OFFSET2) {
> -        return (s->env->mtohost >> 32) & 0xFFFFFFFF;
> +        return (s->tohost >> 32) & 0xFFFFFFFF;
>      } else if (addr == FROMHOST_OFFSET1) {
> -        return s->env->mfromhost & 0xFFFFFFFF;
> +        return s->fromhost & 0xFFFFFFFF;
>      } else if (addr == FROMHOST_OFFSET2) {
> -        return (s->env->mfromhost >> 32) & 0xFFFFFFFF;
> +        return (s->fromhost >> 32) & 0xFFFFFFFF;
>      } else {
>          qemu_log("Invalid htif read: address %016" PRIx64 "\n",
>              (uint64_t)addr);
> @@ -232,22 +232,22 @@ static void htif_mm_write(void *opaque, hwaddr addr,
>  {
>      HTIFState *s = opaque;
>      if (addr == TOHOST_OFFSET1) {
> -        if (s->env->mtohost == 0x0) {
> +        if (s->tohost == 0x0) {
>              s->allow_tohost = 1;
> -            s->env->mtohost = value & 0xFFFFFFFF;
> +            s->tohost = value & 0xFFFFFFFF;
>          } else {
>              s->allow_tohost = 0;
>          }
>      } else if (addr == TOHOST_OFFSET2) {
>          if (s->allow_tohost) {
> -            s->env->mtohost |= value << 32;
> -            htif_handle_tohost_write(s, s->env->mtohost);
> +            s->tohost |= value << 32;
> +            htif_handle_tohost_write(s, s->tohost);
>          }
>      } else if (addr == FROMHOST_OFFSET1) {
>          s->fromhost_inprogress = 1;
> -        s->env->mfromhost = value & 0xFFFFFFFF;
> +        s->fromhost = value & 0xFFFFFFFF;
>      } else if (addr == FROMHOST_OFFSET2) {
> -        s->env->mfromhost |= value << 32;
> +        s->fromhost |= value << 32;
>          s->fromhost_inprogress = 0;
>      } else {
>          qemu_log("Invalid htif write: address %016" PRIx64 "\n",
> @@ -265,8 +265,8 @@ bool htif_uses_elf_symbols(void)
>      return (address_symbol_set == 3) ? true : false;
>  }
>
> -HTIFState *htif_mm_init(MemoryRegion *address_space, CPURISCVState *env,
> -                        Chardev *chr, uint64_t nonelf_base)
> +HTIFState *htif_mm_init(MemoryRegion *address_space, Chardev *chr,
> +                        uint64_t nonelf_base)
>  {
>      uint64_t base, size, tohost_offset, fromhost_offset;
>
> @@ -281,7 +281,6 @@ HTIFState *htif_mm_init(MemoryRegion *address_space, CPURISCVState *env,
>      fromhost_offset = fromhost_addr - base;
>
>      HTIFState *s = g_new0(HTIFState, 1);
> -    s->env = env;
>      s->tohost_offset = tohost_offset;
>      s->fromhost_offset = fromhost_offset;
>      s->pending_read = 0;
> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> index 82cf41ac27..8606331f61 100644
> --- a/hw/riscv/spike.c
> +++ b/hw/riscv/spike.c
> @@ -317,8 +317,7 @@ static void spike_board_init(MachineState *machine)
>                                fdt_load_addr);
>
>      /* initialize HTIF using symbols found in load_kernel */
> -    htif_mm_init(system_memory, &s->soc[0].harts[0].env,
> -                 serial_hd(0), memmap[SPIKE_HTIF].base);
> +    htif_mm_init(system_memory, serial_hd(0), memmap[SPIKE_HTIF].base);
>  }
>
>  static void spike_machine_instance_init(Object *obj)
> diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> index c2a94a82b3..2e8beef06e 100644
> --- a/target/riscv/machine.c
> +++ b/target/riscv/machine.c
> @@ -298,8 +298,8 @@ static const VMStateDescription vmstate_pmu_ctr_state = {
>
>  const VMStateDescription vmstate_riscv_cpu = {
>      .name = "cpu",
> -    .version_id = 5,
> -    .minimum_version_id = 5,
> +    .version_id = 6,
> +    .minimum_version_id = 6,
>      .post_load = riscv_cpu_post_load,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINTTL_ARRAY(env.gpr, RISCVCPU, 32),
> @@ -349,8 +349,6 @@ const VMStateDescription vmstate_riscv_cpu = {
>          VMSTATE_UINTTL_ARRAY(env.mhpmeventh_val, RISCVCPU, RV_MAX_MHPMEVENTS),
>          VMSTATE_UINTTL(env.sscratch, RISCVCPU),
>          VMSTATE_UINTTL(env.mscratch, RISCVCPU),
> -        VMSTATE_UINT64(env.mfromhost, RISCVCPU),
> -        VMSTATE_UINT64(env.mtohost, RISCVCPU),
>          VMSTATE_UINT64(env.stimecmp, RISCVCPU),
>
>          VMSTATE_END_OF_LIST()
> --
> 2.34.1
>
>


^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 00/12] hw/riscv: Improve Spike HTIF emulation fidelity
  2022-12-28  3:58   ` Bin Meng
@ 2022-12-28  4:21     ` Alistair Francis
  0 siblings, 0 replies; 35+ messages in thread
From: Alistair Francis @ 2022-12-28  4:21 UTC (permalink / raw)
  To: Bin Meng, Anup Patel, Anup Patel
  Cc: Daniel Henrique Barboza, Bin Meng, Alistair Francis, qemu-devel,
	Bin Meng, Marc-André Lureau, Palmer Dabbelt, Paolo Bonzini,
	qemu-riscv

On Wed, Dec 28, 2022 at 1:59 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Daniel,
>
> On Wed, Dec 28, 2022 at 1:52 AM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
> >
> >
> >
> > On 12/27/22 03:48, Bin Meng wrote:
> > > At present the 32-bit OpenSBI generic firmware image does not boot on
> > > Spike, only 64-bit image can. This is due to the HTIF emulation does
> > > not implement the proxy syscall interface which is required for the
> > > 32-bit HTIF console output.
> > >
> > > An OpenSBI bug fix [1] is also needed when booting the plain binary image.
> > >
> > > With this series plus the above OpenSBI fix, both 32-bit OpenSBI BIN & ELF
> > > images can boot on QEMU 'spike' machine.
> > >
> > > [1] https://patchwork.ozlabs.org/project/opensbi/patch/20221226033603.1860569-1-bmeng@tinylab.org/
> >
> > Aside from a nit in patch 12/12, LGTM. I've tested with a patched version of
> > Opensbi including [1] and I can get terminal output with riscv32 spike:
> >
> > $ ./qemu-system-riscv32 -M spike -display none -nographic
> > -bios ../../opensbi/build/platform/generic/firmware/fw_payload.bin
> >
> > OpenSBI v1.1-112-g6ce00f8
> >     ____                    _____ ____ _____
> >    / __ \                  / ____|  _ \_   _|
> >   | |  | |_ __   ___ _ __ | (___ | |_) || |
> >   | |  | | '_ \ / _ \ '_ \ \___ \|  _ < | |
> >   | |__| | |_) |  __/ | | |____) | |_) || |_
> >    \____/| .__/ \___|_| |_|_____/|____/_____|
> >          | |
> >          |_|
> > (.......)
> >
> >
> > Speaking of [1], it seems like the fix went a bit too late for the opensbi 1.2 release.
> > Assuming that [1] is accepted, it would be nice if we could bake in this fix on top of the
> > 1.2 release when updating the QEMU roms.
> >
>
> Thanks for the review and testing!
>
> Regarding whether we can cherry-pick the fix on top of OpenSBI 1.2, I
> am not sure if that's allowed by the policy.

It doesn't seem like a good idea. Maybe it's worth pinging Anup to see
if we can get a 1.2.1 with the fix?

Alistair

>
> Alistair, do you know?
>
> Regards,
> Bin
>


^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 06/12] hw/char: riscv_htif: Remove forward declarations for non-existent variables
  2022-12-27  6:48 ` [PATCH 06/12] hw/char: riscv_htif: Remove forward declarations for non-existent variables Bin Meng
  2022-12-27 17:35   ` Daniel Henrique Barboza
@ 2022-12-28  4:22   ` Alistair Francis
  1 sibling, 0 replies; 35+ messages in thread
From: Alistair Francis @ 2022-12-28  4:22 UTC (permalink / raw)
  To: Bin Meng
  Cc: Alistair Francis, qemu-devel, Daniel Henrique Barboza, Anup Patel

On Tue, Dec 27, 2022 at 4:56 PM Bin Meng <bmeng@tinylab.org> wrote:
>
> There are forward declarations for 'vmstate_htif' and 'htif_io_ops'
> in riscv_htif.h however there are no definitions in the C codes.
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>

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

Alistair

> ---
>
>  include/hw/char/riscv_htif.h | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/include/hw/char/riscv_htif.h b/include/hw/char/riscv_htif.h
> index 55cc352331..9e8ebbe017 100644
> --- a/include/hw/char/riscv_htif.h
> +++ b/include/hw/char/riscv_htif.h
> @@ -40,9 +40,6 @@ typedef struct HTIFState {
>      uint64_t pending_read;
>  } HTIFState;
>
> -extern const VMStateDescription vmstate_htif;
> -extern const MemoryRegionOps htif_io_ops;
> -
>  /* HTIF symbol callback */
>  void htif_symbol_callback(const char *st_name, int st_info, uint64_t st_value,
>      uint64_t st_size);
> --
> 2.34.1
>
>


^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 07/12] hw/char: riscv_htif: Support console output via proxy syscall
  2022-12-27  6:48 ` [PATCH 07/12] hw/char: riscv_htif: Support console output via proxy syscall Bin Meng
  2022-12-27 17:37   ` Daniel Henrique Barboza
@ 2022-12-28  4:30   ` Alistair Francis
  1 sibling, 0 replies; 35+ messages in thread
From: Alistair Francis @ 2022-12-28  4:30 UTC (permalink / raw)
  To: Bin Meng
  Cc: Alistair Francis, qemu-devel, Daniel Henrique Barboza,
	Marc-André Lureau, Paolo Bonzini

On Tue, Dec 27, 2022 at 4:49 PM Bin Meng <bmeng@tinylab.org> wrote:
>
> At present the HTIF proxy syscall is unsupported. On RV32, only
> device 0 is supported so there is no console device for RV32.
> The only way to implement console funtionality on RV32 is to
> support the SYS_WRITE syscall.
>
> With this commit, the Spike machine is able to boot the 32-bit
> OpenSBI generic image.
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>

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

Alistair

> ---
>
>  hw/char/riscv_htif.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/hw/char/riscv_htif.c b/hw/char/riscv_htif.c
> index 3bb0a37a3e..1477fc0090 100644
> --- a/hw/char/riscv_htif.c
> +++ b/hw/char/riscv_htif.c
> @@ -48,6 +48,9 @@
>  #define HTIF_CONSOLE_CMD_GETC   0
>  #define HTIF_CONSOLE_CMD_PUTC   1
>
> +/* PK system call number */
> +#define PK_SYS_WRITE            64
> +
>  static uint64_t fromhost_addr, tohost_addr;
>  static int address_symbol_set;
>
> @@ -165,7 +168,19 @@ static void htif_handle_tohost_write(HTIFState *s, uint64_t val_written)
>                  int exit_code = payload >> 1;
>                  exit(exit_code);
>              } else {
> -                qemu_log_mask(LOG_UNIMP, "pk syscall proxy not supported\n");
> +                uint64_t syscall[8];
> +                cpu_physical_memory_read(payload, syscall, sizeof(syscall));
> +                if (syscall[0] == PK_SYS_WRITE &&
> +                    syscall[1] == HTIF_DEV_CONSOLE &&
> +                    syscall[3] == HTIF_CONSOLE_CMD_PUTC) {
> +                    uint8_t ch;
> +                    cpu_physical_memory_read(syscall[2], &ch, 1);
> +                    qemu_chr_fe_write(&s->chr, &ch, 1);
> +                    resp = 0x100 | (uint8_t)payload;
> +                } else {
> +                    qemu_log_mask(LOG_UNIMP,
> +                                  "pk syscall proxy not supported\n");
> +                }
>              }
>          } else {
>              qemu_log("HTIF device %d: unknown command\n", device);
> --
> 2.34.1
>
>


^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 08/12] hw/riscv: spike: Remove the out-of-date comments
  2022-12-27  6:48 ` [PATCH 08/12] hw/riscv: spike: Remove the out-of-date comments Bin Meng
  2022-12-27 17:37   ` Daniel Henrique Barboza
@ 2022-12-28  4:30   ` Alistair Francis
  1 sibling, 0 replies; 35+ messages in thread
From: Alistair Francis @ 2022-12-28  4:30 UTC (permalink / raw)
  To: Bin Meng
  Cc: Alistair Francis, qemu-devel, Daniel Henrique Barboza, Bin Meng,
	Palmer Dabbelt, qemu-riscv

On Tue, Dec 27, 2022 at 4:54 PM Bin Meng <bmeng@tinylab.org> wrote:
>
> Spike machine now supports OpenSBI plain binary bios image, so the
> comments are no longer valid.
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>

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

Alistair

> ---
>
>  hw/riscv/spike.c | 5 -----
>  1 file changed, 5 deletions(-)
>
> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> index 8606331f61..ab0a945f8b 100644
> --- a/hw/riscv/spike.c
> +++ b/hw/riscv/spike.c
> @@ -256,11 +256,6 @@ static void spike_board_init(MachineState *machine)
>      memory_region_add_subregion(system_memory, memmap[SPIKE_MROM].base,
>                                  mask_rom);
>
> -    /*
> -     * Not like other RISC-V machines that use plain binary bios images,
> -     * 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,
> --
> 2.34.1
>
>


^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 11/12] hw/riscv/boot.c: Introduce riscv_find_firmware()
  2022-12-27  6:48 ` [PATCH 11/12] hw/riscv/boot.c: Introduce riscv_find_firmware() Bin Meng
  2022-12-27 17:40   ` Daniel Henrique Barboza
@ 2022-12-28  4:35   ` Alistair Francis
  1 sibling, 0 replies; 35+ messages in thread
From: Alistair Francis @ 2022-12-28  4:35 UTC (permalink / raw)
  To: Bin Meng
  Cc: Alistair Francis, qemu-devel, Daniel Henrique Barboza, Bin Meng,
	Palmer Dabbelt, qemu-riscv

On Tue, Dec 27, 2022 at 4:55 PM Bin Meng <bmeng@tinylab.org> wrote:
>
> Rename previous riscv_find_firmware() to riscv_find_bios(), and
> introduce a new riscv_find_firmware() to implement the first half
> part of the work done in riscv_find_and_load_firmware().
>
> This new API is helpful for machine that wants to know the final
> chosen firmware file name but does not want to load it.
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>

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

Alistair

> ---
>
>  include/hw/riscv/boot.h |  2 ++
>  hw/riscv/boot.c         | 39 +++++++++++++++++++++++++--------------
>  2 files changed, 27 insertions(+), 14 deletions(-)
>
> diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
> index 60cf320c88..b273ab22f7 100644
> --- a/include/hw/riscv/boot.h
> +++ b/include/hw/riscv/boot.h
> @@ -38,6 +38,8 @@ target_ulong riscv_find_and_load_firmware(MachineState *machine,
>                                            hwaddr firmware_load_addr,
>                                            symbol_fn_t sym_cb);
>  const char *riscv_default_firmware_name(RISCVHartArrayState *harts);
> +char *riscv_find_firmware(const char *firmware_filename,
> +                          const char *default_machine_firmware);
>  target_ulong riscv_load_firmware(const char *firmware_filename,
>                                   hwaddr firmware_load_addr,
>                                   symbol_fn_t sym_cb);
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index e1a544b1d9..98b80af51b 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -84,11 +84,11 @@ const char *riscv_default_firmware_name(RISCVHartArrayState *harts)
>      return RISCV64_BIOS_BIN;
>  }
>
> -static char *riscv_find_firmware(const char *firmware_filename)
> +static char *riscv_find_bios(const char *bios_filename)
>  {
>      char *filename;
>
> -    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, firmware_filename);
> +    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_filename);
>      if (filename == NULL) {
>          if (!qtest_enabled()) {
>              /*
> @@ -97,8 +97,8 @@ static char *riscv_find_firmware(const char *firmware_filename)
>               * 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);
> +            error_report("Unable to find the RISC-V BIOS \"%s\"",
> +                         bios_filename);
>              exit(1);
>          }
>      }
> @@ -106,25 +106,36 @@ static char *riscv_find_firmware(const char *firmware_filename)
>      return filename;
>  }
>
> -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,
> +                          const char *default_machine_firmware)
>  {
> -    char *firmware_filename = NULL;
> -    target_ulong firmware_end_addr = firmware_load_addr;
> +    char *filename = NULL;
>
> -    if ((!machine->firmware) || (!strcmp(machine->firmware, "default"))) {
> +    if ((!firmware_filename) || (!strcmp(firmware_filename, "default"))) {
>          /*
>           * The user didn't specify -bios, or has specified "-bios default".
>           * That means we are going to load the OpenSBI binary included in
>           * the QEMU source.
>           */
> -        firmware_filename = riscv_find_firmware(default_machine_firmware);
> -    } else if (strcmp(machine->firmware, "none")) {
> -        firmware_filename = riscv_find_firmware(machine->firmware);
> +        filename = riscv_find_bios(default_machine_firmware);
> +    } else if (strcmp(firmware_filename, "none")) {
> +        filename = riscv_find_bios(firmware_filename);
>      }
>
> +    return filename;
> +}
> +
> +target_ulong riscv_find_and_load_firmware(MachineState *machine,
> +                                          const char *default_machine_firmware,
> +                                          hwaddr firmware_load_addr,
> +                                          symbol_fn_t sym_cb)
> +{
> +    char *firmware_filename;
> +    target_ulong firmware_end_addr = firmware_load_addr;
> +
> +    firmware_filename = riscv_find_firmware(machine->firmware,
> +                                            default_machine_firmware);
> +
>      if (firmware_filename) {
>          /* If not "none" load the firmware */
>          firmware_end_addr = riscv_load_firmware(firmware_filename,
> --
> 2.34.1
>
>


^ permalink raw reply	[flat|nested] 35+ messages in thread

end of thread, other threads:[~2022-12-28  4:37 UTC | newest]

Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-27  6:48 [PATCH 00/12] hw/riscv: Improve Spike HTIF emulation fidelity Bin Meng
2022-12-27  6:48 ` [PATCH 01/12] hw/char: riscv_htif: Avoid using magic numbers Bin Meng
2022-12-27 17:28   ` Daniel Henrique Barboza
2022-12-28  3:31   ` Alistair Francis
2022-12-27  6:48 ` [PATCH 02/12] hw/char: riscv_htif: Drop {to, from}host_size in HTIFState Bin Meng
2022-12-27 17:29   ` [PATCH 02/12] hw/char: riscv_htif: Drop {to,from}host_size " Daniel Henrique Barboza
2022-12-28  3:32   ` [PATCH 02/12] hw/char: riscv_htif: Drop {to, from}host_size " Alistair Francis
2022-12-27  6:48 ` [PATCH 03/12] hw/char: riscv_htif: Drop useless assignment of memory region Bin Meng
2022-12-27 17:30   ` Daniel Henrique Barboza
2022-12-28  3:33   ` Alistair Francis
2022-12-27  6:48 ` [PATCH 04/12] hw/char: riscv_htif: Use conventional 's' for HTIFState Bin Meng
2022-12-27 17:32   ` Daniel Henrique Barboza
2022-12-28  3:35   ` Alistair Francis
2022-12-27  6:48 ` [PATCH 05/12] hw/char: riscv_htif: Move registers from CPUArchState to HTIFState Bin Meng
2022-12-27 17:33   ` Daniel Henrique Barboza
2022-12-28  4:19   ` Alistair Francis
2022-12-27  6:48 ` [PATCH 06/12] hw/char: riscv_htif: Remove forward declarations for non-existent variables Bin Meng
2022-12-27 17:35   ` Daniel Henrique Barboza
2022-12-28  4:22   ` Alistair Francis
2022-12-27  6:48 ` [PATCH 07/12] hw/char: riscv_htif: Support console output via proxy syscall Bin Meng
2022-12-27 17:37   ` Daniel Henrique Barboza
2022-12-28  4:30   ` Alistair Francis
2022-12-27  6:48 ` [PATCH 08/12] hw/riscv: spike: Remove the out-of-date comments Bin Meng
2022-12-27 17:37   ` Daniel Henrique Barboza
2022-12-28  4:30   ` Alistair Francis
2022-12-27  6:48 ` [PATCH 09/12] hw/riscv/boot.c: make riscv_find_firmware() static Bin Meng
2022-12-27  6:48 ` [PATCH 10/12] hw/riscv/boot.c: introduce riscv_default_firmware_name() Bin Meng
2022-12-27  6:48 ` [PATCH 11/12] hw/riscv/boot.c: Introduce riscv_find_firmware() Bin Meng
2022-12-27 17:40   ` Daniel Henrique Barboza
2022-12-28  4:35   ` Alistair Francis
2022-12-27  6:48 ` [PATCH 12/12] hw/riscv: spike: Decouple create_fdt() dependency to ELF loading Bin Meng
2022-12-27 14:33   ` Daniel Henrique Barboza
2022-12-27 17:51 ` [PATCH 00/12] hw/riscv: Improve Spike HTIF emulation fidelity Daniel Henrique Barboza
2022-12-28  3:58   ` Bin Meng
2022-12-28  4:21     ` Alistair Francis

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.