* [PATCH v2 00/12] hw/riscv: Improve Spike HTIF emulation fidelity
@ 2022-12-29 9:18 Bin Meng
2022-12-29 9:18 ` [PATCH v2 01/12] hw/char: riscv_htif: Avoid using magic numbers Bin Meng
` (12 more replies)
0 siblings, 13 replies; 18+ messages in thread
From: Bin Meng @ 2022-12-29 9:18 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/
Changes in v2:
- fix 2 typos in the commit message
- initialize firmware_end_addr to memmap[SPIKE_DRAM].base
- rework the htif_custom_base detection logic
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 | 64 +++++++++----
hw/riscv/virt.c | 10 +-
target/riscv/machine.c | 6 +-
9 files changed, 217 insertions(+), 149 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 01/12] hw/char: riscv_htif: Avoid using magic numbers
2022-12-29 9:18 [PATCH v2 00/12] hw/riscv: Improve Spike HTIF emulation fidelity Bin Meng
@ 2022-12-29 9:18 ` Bin Meng
2022-12-29 9:18 ` [PATCH v2 02/12] hw/char: riscv_htif: Drop {to, from}host_size in HTIFState Bin Meng
` (11 subsequent siblings)
12 siblings, 0 replies; 18+ messages in thread
From: Bin Meng @ 2022-12-29 9:18 UTC (permalink / raw)
To: Alistair Francis, qemu-devel
Cc: Daniel Henrique Barboza, Alistair Francis,
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 instead of
magic numbers in the codes.
While we are here, correct 2 multi-line comment blocks that have
wrong format.
Link: https://github.com/riscv-software-src/riscv-isa-sim/issues/364#issuecomment-607657754 [1]
Signed-off-by: Bin Meng <bmeng@tinylab.org>
Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
Changes in v2:
- fix 2 typos in the commit message
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] 18+ messages in thread
* [PATCH v2 02/12] hw/char: riscv_htif: Drop {to, from}host_size in HTIFState
2022-12-29 9:18 [PATCH v2 00/12] hw/riscv: Improve Spike HTIF emulation fidelity Bin Meng
2022-12-29 9:18 ` [PATCH v2 01/12] hw/char: riscv_htif: Avoid using magic numbers Bin Meng
@ 2022-12-29 9:18 ` Bin Meng
2022-12-29 9:18 ` [PATCH v2 03/12] hw/char: riscv_htif: Drop useless assignment of memory region Bin Meng
` (10 subsequent siblings)
12 siblings, 0 replies; 18+ messages in thread
From: Bin Meng @ 2022-12-29 9:18 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>
Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
(no changes since v1)
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] 18+ messages in thread
* [PATCH v2 03/12] hw/char: riscv_htif: Drop useless assignment of memory region
2022-12-29 9:18 [PATCH v2 00/12] hw/riscv: Improve Spike HTIF emulation fidelity Bin Meng
2022-12-29 9:18 ` [PATCH v2 01/12] hw/char: riscv_htif: Avoid using magic numbers Bin Meng
2022-12-29 9:18 ` [PATCH v2 02/12] hw/char: riscv_htif: Drop {to, from}host_size in HTIFState Bin Meng
@ 2022-12-29 9:18 ` Bin Meng
2022-12-29 9:18 ` [PATCH v2 04/12] hw/char: riscv_htif: Use conventional 's' for HTIFState Bin Meng
` (9 subsequent siblings)
12 siblings, 0 replies; 18+ messages in thread
From: Bin Meng @ 2022-12-29 9:18 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>
Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
(no changes since v1)
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] 18+ messages in thread
* [PATCH v2 04/12] hw/char: riscv_htif: Use conventional 's' for HTIFState
2022-12-29 9:18 [PATCH v2 00/12] hw/riscv: Improve Spike HTIF emulation fidelity Bin Meng
` (2 preceding siblings ...)
2022-12-29 9:18 ` [PATCH v2 03/12] hw/char: riscv_htif: Drop useless assignment of memory region Bin Meng
@ 2022-12-29 9:18 ` Bin Meng
2022-12-29 9:18 ` [PATCH v2 05/12] hw/char: riscv_htif: Move registers from CPUArchState to HTIFState Bin Meng
` (8 subsequent siblings)
12 siblings, 0 replies; 18+ messages in thread
From: Bin Meng @ 2022-12-29 9:18 UTC (permalink / raw)
To: Alistair Francis, qemu-devel
Cc: Daniel Henrique Barboza, Alistair Francis,
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>
Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
(no changes since v1)
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] 18+ messages in thread
* [PATCH v2 05/12] hw/char: riscv_htif: Move registers from CPUArchState to HTIFState
2022-12-29 9:18 [PATCH v2 00/12] hw/riscv: Improve Spike HTIF emulation fidelity Bin Meng
` (3 preceding siblings ...)
2022-12-29 9:18 ` [PATCH v2 04/12] hw/char: riscv_htif: Use conventional 's' for HTIFState Bin Meng
@ 2022-12-29 9:18 ` Bin Meng
2022-12-29 9:18 ` [PATCH v2 06/12] hw/char: riscv_htif: Remove forward declarations for non-existent variables Bin Meng
` (7 subsequent siblings)
12 siblings, 0 replies; 18+ messages in thread
From: Bin Meng @ 2022-12-29 9:18 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>
Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
(no changes since v1)
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] 18+ messages in thread
* [PATCH v2 06/12] hw/char: riscv_htif: Remove forward declarations for non-existent variables
2022-12-29 9:18 [PATCH v2 00/12] hw/riscv: Improve Spike HTIF emulation fidelity Bin Meng
` (4 preceding siblings ...)
2022-12-29 9:18 ` [PATCH v2 05/12] hw/char: riscv_htif: Move registers from CPUArchState to HTIFState Bin Meng
@ 2022-12-29 9:18 ` Bin Meng
2022-12-29 9:18 ` [PATCH v2 07/12] hw/char: riscv_htif: Support console output via proxy syscall Bin Meng
` (6 subsequent siblings)
12 siblings, 0 replies; 18+ messages in thread
From: Bin Meng @ 2022-12-29 9:18 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>
Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
(no changes since v1)
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] 18+ messages in thread
* [PATCH v2 07/12] hw/char: riscv_htif: Support console output via proxy syscall
2022-12-29 9:18 [PATCH v2 00/12] hw/riscv: Improve Spike HTIF emulation fidelity Bin Meng
` (5 preceding siblings ...)
2022-12-29 9:18 ` [PATCH v2 06/12] hw/char: riscv_htif: Remove forward declarations for non-existent variables Bin Meng
@ 2022-12-29 9:18 ` Bin Meng
2022-12-29 9:18 ` [PATCH v2 08/12] hw/riscv: spike: Remove the out-of-date comments Bin Meng
` (5 subsequent siblings)
12 siblings, 0 replies; 18+ messages in thread
From: Bin Meng @ 2022-12-29 9:18 UTC (permalink / raw)
To: Alistair Francis, qemu-devel
Cc: Daniel Henrique Barboza, Alistair Francis,
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>
Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
(no changes since v1)
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] 18+ messages in thread
* [PATCH v2 08/12] hw/riscv: spike: Remove the out-of-date comments
2022-12-29 9:18 [PATCH v2 00/12] hw/riscv: Improve Spike HTIF emulation fidelity Bin Meng
` (6 preceding siblings ...)
2022-12-29 9:18 ` [PATCH v2 07/12] hw/char: riscv_htif: Support console output via proxy syscall Bin Meng
@ 2022-12-29 9:18 ` Bin Meng
2022-12-29 9:18 ` [PATCH v2 09/12] hw/riscv/boot.c: make riscv_find_firmware() static Bin Meng
` (4 subsequent siblings)
12 siblings, 0 replies; 18+ messages in thread
From: Bin Meng @ 2022-12-29 9:18 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>
Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
(no changes since v1)
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] 18+ messages in thread
* [PATCH v2 09/12] hw/riscv/boot.c: make riscv_find_firmware() static
2022-12-29 9:18 [PATCH v2 00/12] hw/riscv: Improve Spike HTIF emulation fidelity Bin Meng
` (7 preceding siblings ...)
2022-12-29 9:18 ` [PATCH v2 08/12] hw/riscv: spike: Remove the out-of-date comments Bin Meng
@ 2022-12-29 9:18 ` Bin Meng
2022-12-29 9:18 ` [PATCH v2 10/12] hw/riscv/boot.c: introduce riscv_default_firmware_name() Bin Meng
` (3 subsequent siblings)
12 siblings, 0 replies; 18+ messages in thread
From: Bin Meng @ 2022-12-29 9:18 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>
---
(no changes since v1)
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] 18+ messages in thread
* [PATCH v2 10/12] hw/riscv/boot.c: introduce riscv_default_firmware_name()
2022-12-29 9:18 [PATCH v2 00/12] hw/riscv: Improve Spike HTIF emulation fidelity Bin Meng
` (8 preceding siblings ...)
2022-12-29 9:18 ` [PATCH v2 09/12] hw/riscv/boot.c: make riscv_find_firmware() static Bin Meng
@ 2022-12-29 9:18 ` Bin Meng
2022-12-29 9:18 ` [PATCH v2 11/12] hw/riscv/boot.c: Introduce riscv_find_firmware() Bin Meng
` (2 subsequent siblings)
12 siblings, 0 replies; 18+ messages in thread
From: Bin Meng @ 2022-12-29 9:18 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>
---
(no changes since v1)
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] 18+ messages in thread
* [PATCH v2 11/12] hw/riscv/boot.c: Introduce riscv_find_firmware()
2022-12-29 9:18 [PATCH v2 00/12] hw/riscv: Improve Spike HTIF emulation fidelity Bin Meng
` (9 preceding siblings ...)
2022-12-29 9:18 ` [PATCH v2 10/12] hw/riscv/boot.c: introduce riscv_default_firmware_name() Bin Meng
@ 2022-12-29 9:18 ` Bin Meng
2022-12-29 10:24 ` [PATCH v2 00/12] hw/riscv: Improve Spike HTIF emulation fidelity Daniel Henrique Barboza
2022-12-29 10:31 ` [PATCH v2 12/12] hw/riscv: spike: Decouple create_fdt() dependency to ELF loading Bin Meng
12 siblings, 0 replies; 18+ messages in thread
From: Bin Meng @ 2022-12-29 9:18 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>
Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
(no changes since v1)
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] 18+ messages in thread
* Re: [PATCH v2 00/12] hw/riscv: Improve Spike HTIF emulation fidelity
2022-12-29 9:18 [PATCH v2 00/12] hw/riscv: Improve Spike HTIF emulation fidelity Bin Meng
` (10 preceding siblings ...)
2022-12-29 9:18 ` [PATCH v2 11/12] hw/riscv/boot.c: Introduce riscv_find_firmware() Bin Meng
@ 2022-12-29 10:24 ` Daniel Henrique Barboza
2022-12-29 10:38 ` Bin Meng
2022-12-29 10:31 ` [PATCH v2 12/12] hw/riscv: spike: Decouple create_fdt() dependency to ELF loading Bin Meng
12 siblings, 1 reply; 18+ messages in thread
From: Daniel Henrique Barboza @ 2022-12-29 10:24 UTC (permalink / raw)
To: Bin Meng, Alistair Francis, qemu-devel
Cc: Anup Patel, Bin Meng, Marc-André Lureau, Palmer Dabbelt,
Paolo Bonzini, qemu-riscv
[-- Attachment #1: Type: text/plain, Size: 2415 bytes --]
Bin,
Not sure if it's a problem on my side but I can't find patch 12/12. I didn't
received in my mailbox. I tried patchwork but didn't find in there:
https://patchwork.ozlabs.org/project/qemu-devel/list/?series=334352
And it's not in the ML archives as well:
https://mail.gnu.org/archive/html/qemu-devel/2022-12/msg04581.html
Thanks,
Daniel
On 12/29/22 06:18, 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/
>
> Changes in v2:
> - fix 2 typos in the commit message
> - initialize firmware_end_addr to memmap[SPIKE_DRAM].base
> - rework the htif_custom_base detection logic
>
> 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 | 64 +++++++++----
> hw/riscv/virt.c | 10 +-
> target/riscv/machine.c | 6 +-
> 9 files changed, 217 insertions(+), 149 deletions(-)
>
[-- Attachment #2: Type: text/html, Size: 3228 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 12/12] hw/riscv: spike: Decouple create_fdt() dependency to ELF loading
2022-12-29 9:18 [PATCH v2 00/12] hw/riscv: Improve Spike HTIF emulation fidelity Bin Meng
` (11 preceding siblings ...)
2022-12-29 10:24 ` [PATCH v2 00/12] hw/riscv: Improve Spike HTIF emulation fidelity Daniel Henrique Barboza
@ 2022-12-29 10:31 ` Bin Meng
2022-12-29 13:58 ` Daniel Henrique Barboza
2023-01-09 22:55 ` Alistair Francis
12 siblings, 2 replies; 18+ messages in thread
From: Bin Meng @ 2022-12-29 10:31 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>
---
Changes in v2:
- initialize firmware_end_addr to memmap[SPIKE_DRAM].base
- rework the htif_custom_base detection logic
include/hw/char/riscv_htif.h | 5 +--
hw/char/riscv_htif.c | 17 +++++-----
hw/riscv/spike.c | 61 ++++++++++++++++++++++++++++++------
3 files changed, 59 insertions(+), 24 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..dd5f912e3d 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,18 +185,33 @@ static void create_fdt(SpikeState *s, const MemMapEntry *memmap,
}
}
+static bool spike_test_elf_image(char *filename)
+{
+ Error *err = NULL;
+
+ load_elf_hdr(filename, NULL, NULL, &err);
+ if (err) {
+ error_free(err);
+ return false;
+ } else {
+ return true;
+ }
+}
+
static void spike_board_init(MachineState *machine)
{
const MemMapEntry *memmap = spike_memmap;
SpikeState *s = SPIKE_MACHINE(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;
+ target_ulong firmware_end_addr = memmap[SPIKE_DRAM].base;
+ target_ulong kernel_start_addr;
+ 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 = false;
/* Check socket count limit */
if (SPIKE_SOCKETS_MAX < riscv_socket_count(machine)) {
@@ -257,10 +273,34 @@ 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.
+ */
+ if (!firmware_name && !machine->kernel_filename) {
+ htif_custom_base = true;
+ } else {
+ if (firmware_name) {
+ htif_custom_base = !spike_test_elf_image(firmware_name);
+ }
+ if (!htif_custom_base && machine->kernel_filename) {
+ 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 +320,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 +348,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] 18+ messages in thread
* Re: [PATCH v2 00/12] hw/riscv: Improve Spike HTIF emulation fidelity
2022-12-29 10:24 ` [PATCH v2 00/12] hw/riscv: Improve Spike HTIF emulation fidelity Daniel Henrique Barboza
@ 2022-12-29 10:38 ` Bin Meng
2022-12-29 10:58 ` Daniel Henrique Barboza
0 siblings, 1 reply; 18+ messages in thread
From: Bin Meng @ 2022-12-29 10:38 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 Thu, Dec 29, 2022 at 6:25 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Bin,
>
> Not sure if it's a problem on my side but I can't find patch 12/12. I didn't
> received in my mailbox. I tried patchwork but didn't find in there:
>
> https://patchwork.ozlabs.org/project/qemu-devel/list/?series=334352
>
>
> And it's not in the ML archives as well:
>
> https://mail.gnu.org/archive/html/qemu-devel/2022-12/msg04581.html
>
Thanks for reporting this!
I just examined the "git send-email" log, and indeed the "git
send-email" aborted at the last patch for some unknown reason.
I just resent the 12th patch using "git send-email" using the correct
Message-Id, and hopefully it will be correctly chained into the v2
patch thread.
Regards,
Bin
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 00/12] hw/riscv: Improve Spike HTIF emulation fidelity
2022-12-29 10:38 ` Bin Meng
@ 2022-12-29 10:58 ` Daniel Henrique Barboza
0 siblings, 0 replies; 18+ messages in thread
From: Daniel Henrique Barboza @ 2022-12-29 10:58 UTC (permalink / raw)
To: Bin Meng
Cc: Bin Meng, Alistair Francis, qemu-devel, Anup Patel, Bin Meng,
Marc-André Lureau, Palmer Dabbelt, Paolo Bonzini,
qemu-riscv
On 12/29/22 07:38, Bin Meng wrote:
> Hi Daniel,
>
> On Thu, Dec 29, 2022 at 6:25 PM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
>> Bin,
>>
>> Not sure if it's a problem on my side but I can't find patch 12/12. I didn't
>> received in my mailbox. I tried patchwork but didn't find in there:
>>
>> https://patchwork.ozlabs.org/project/qemu-devel/list/?series=334352
>>
>>
>> And it's not in the ML archives as well:
>>
>> https://mail.gnu.org/archive/html/qemu-devel/2022-12/msg04581.html
>>
> Thanks for reporting this!
>
> I just examined the "git send-email" log, and indeed the "git
> send-email" aborted at the last patch for some unknown reason.
>
> I just resent the 12th patch using "git send-email" using the correct
> Message-Id, and hopefully it will be correctly chained into the v2
> patch thread.
That worked for me!
Good to know that there's a workaround to send missing patches in a
series - historically I would just resend the series all over again when
that happens,
Daniel
>
> Regards,
> Bin
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 12/12] hw/riscv: spike: Decouple create_fdt() dependency to ELF loading
2022-12-29 10:31 ` [PATCH v2 12/12] hw/riscv: spike: Decouple create_fdt() dependency to ELF loading Bin Meng
@ 2022-12-29 13:58 ` Daniel Henrique Barboza
2023-01-09 22:55 ` Alistair Francis
1 sibling, 0 replies; 18+ messages in thread
From: Daniel Henrique Barboza @ 2022-12-29 13:58 UTC (permalink / raw)
To: Bin Meng, Alistair Francis, qemu-devel
Cc: Bin Meng, Marc-André Lureau, Palmer Dabbelt, Paolo Bonzini,
qemu-riscv
On 12/29/22 07:31, 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>
>
> ---
Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>
> Changes in v2:
> - initialize firmware_end_addr to memmap[SPIKE_DRAM].base
> - rework the htif_custom_base detection logic
>
> include/hw/char/riscv_htif.h | 5 +--
> hw/char/riscv_htif.c | 17 +++++-----
> hw/riscv/spike.c | 61 ++++++++++++++++++++++++++++++------
> 3 files changed, 59 insertions(+), 24 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..dd5f912e3d 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,18 +185,33 @@ static void create_fdt(SpikeState *s, const MemMapEntry *memmap,
> }
> }
>
> +static bool spike_test_elf_image(char *filename)
> +{
> + Error *err = NULL;
> +
> + load_elf_hdr(filename, NULL, NULL, &err);
> + if (err) {
> + error_free(err);
> + return false;
> + } else {
> + return true;
> + }
> +}
> +
> static void spike_board_init(MachineState *machine)
> {
> const MemMapEntry *memmap = spike_memmap;
> SpikeState *s = SPIKE_MACHINE(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;
> + target_ulong firmware_end_addr = memmap[SPIKE_DRAM].base;
> + target_ulong kernel_start_addr;
> + 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 = false;
>
> /* Check socket count limit */
> if (SPIKE_SOCKETS_MAX < riscv_socket_count(machine)) {
> @@ -257,10 +273,34 @@ 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.
> + */
> + if (!firmware_name && !machine->kernel_filename) {
> + htif_custom_base = true;
> + } else {
> + if (firmware_name) {
> + htif_custom_base = !spike_test_elf_image(firmware_name);
> + }
> + if (!htif_custom_base && machine->kernel_filename) {
> + 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 +320,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 +348,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] 18+ messages in thread
* Re: [PATCH v2 12/12] hw/riscv: spike: Decouple create_fdt() dependency to ELF loading
2022-12-29 10:31 ` [PATCH v2 12/12] hw/riscv: spike: Decouple create_fdt() dependency to ELF loading Bin Meng
2022-12-29 13:58 ` Daniel Henrique Barboza
@ 2023-01-09 22:55 ` Alistair Francis
1 sibling, 0 replies; 18+ messages in thread
From: Alistair Francis @ 2023-01-09 22:55 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 Thu, Dec 29, 2022 at 8:33 PM Bin Meng <bmeng@tinylab.org> 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>
Thanks!
Applied to riscv-to-apply.next
Alistair
>
> ---
>
> Changes in v2:
> - initialize firmware_end_addr to memmap[SPIKE_DRAM].base
> - rework the htif_custom_base detection logic
>
> include/hw/char/riscv_htif.h | 5 +--
> hw/char/riscv_htif.c | 17 +++++-----
> hw/riscv/spike.c | 61 ++++++++++++++++++++++++++++++------
> 3 files changed, 59 insertions(+), 24 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..dd5f912e3d 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,18 +185,33 @@ static void create_fdt(SpikeState *s, const MemMapEntry *memmap,
> }
> }
>
> +static bool spike_test_elf_image(char *filename)
> +{
> + Error *err = NULL;
> +
> + load_elf_hdr(filename, NULL, NULL, &err);
> + if (err) {
> + error_free(err);
> + return false;
> + } else {
> + return true;
> + }
> +}
> +
> static void spike_board_init(MachineState *machine)
> {
> const MemMapEntry *memmap = spike_memmap;
> SpikeState *s = SPIKE_MACHINE(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;
> + target_ulong firmware_end_addr = memmap[SPIKE_DRAM].base;
> + target_ulong kernel_start_addr;
> + 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 = false;
>
> /* Check socket count limit */
> if (SPIKE_SOCKETS_MAX < riscv_socket_count(machine)) {
> @@ -257,10 +273,34 @@ 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.
> + */
> + if (!firmware_name && !machine->kernel_filename) {
> + htif_custom_base = true;
> + } else {
> + if (firmware_name) {
> + htif_custom_base = !spike_test_elf_image(firmware_name);
> + }
> + if (!htif_custom_base && machine->kernel_filename) {
> + 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 +320,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 +348,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 [flat|nested] 18+ messages in thread
end of thread, other threads:[~2023-01-09 22:56 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-29 9:18 [PATCH v2 00/12] hw/riscv: Improve Spike HTIF emulation fidelity Bin Meng
2022-12-29 9:18 ` [PATCH v2 01/12] hw/char: riscv_htif: Avoid using magic numbers Bin Meng
2022-12-29 9:18 ` [PATCH v2 02/12] hw/char: riscv_htif: Drop {to, from}host_size in HTIFState Bin Meng
2022-12-29 9:18 ` [PATCH v2 03/12] hw/char: riscv_htif: Drop useless assignment of memory region Bin Meng
2022-12-29 9:18 ` [PATCH v2 04/12] hw/char: riscv_htif: Use conventional 's' for HTIFState Bin Meng
2022-12-29 9:18 ` [PATCH v2 05/12] hw/char: riscv_htif: Move registers from CPUArchState to HTIFState Bin Meng
2022-12-29 9:18 ` [PATCH v2 06/12] hw/char: riscv_htif: Remove forward declarations for non-existent variables Bin Meng
2022-12-29 9:18 ` [PATCH v2 07/12] hw/char: riscv_htif: Support console output via proxy syscall Bin Meng
2022-12-29 9:18 ` [PATCH v2 08/12] hw/riscv: spike: Remove the out-of-date comments Bin Meng
2022-12-29 9:18 ` [PATCH v2 09/12] hw/riscv/boot.c: make riscv_find_firmware() static Bin Meng
2022-12-29 9:18 ` [PATCH v2 10/12] hw/riscv/boot.c: introduce riscv_default_firmware_name() Bin Meng
2022-12-29 9:18 ` [PATCH v2 11/12] hw/riscv/boot.c: Introduce riscv_find_firmware() Bin Meng
2022-12-29 10:24 ` [PATCH v2 00/12] hw/riscv: Improve Spike HTIF emulation fidelity Daniel Henrique Barboza
2022-12-29 10:38 ` Bin Meng
2022-12-29 10:58 ` Daniel Henrique Barboza
2022-12-29 10:31 ` [PATCH v2 12/12] hw/riscv: spike: Decouple create_fdt() dependency to ELF loading Bin Meng
2022-12-29 13:58 ` Daniel Henrique Barboza
2023-01-09 22:55 ` 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.