From 753ae97abc7459e69d48712355118fb54268f8cb Mon Sep 17 00:00:00 2001 From: Bin Meng Date: Thu, 29 Dec 2022 17:18:17 +0800 Subject: [PATCH 01/37] hw/char: riscv_htif: Avoid using magic numbers 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 Reviewed-by: Daniel Henrique Barboza Reviewed-by: Alistair Francis Message-Id: <20221229091828.1945072-2-bmeng@tinylab.org> Signed-off-by: Alistair Francis --- 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) { From bc9c3b18626fbfe9eb8f37438b5fbb2f901c2460 Mon Sep 17 00:00:00 2001 From: Bin Meng Date: Thu, 29 Dec 2022 17:18:18 +0800 Subject: [PATCH 02/37] hw/char: riscv_htif: Drop {to, from}host_size in HTIFState These are not used anywhere. Drop them. Signed-off-by: Bin Meng Reviewed-by: Daniel Henrique Barboza Reviewed-by: Alistair Francis Message-Id: <20221229091828.1945072-3-bmeng@tinylab.org> Signed-off-by: Alistair Francis --- 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; From dc6882464161a7bf77c8b847cef6d4f2f9066361 Mon Sep 17 00:00:00 2001 From: Bin Meng Date: Thu, 29 Dec 2022 17:18:19 +0800 Subject: [PATCH 03/37] hw/char: riscv_htif: Drop useless assignment of memory region 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 Reviewed-by: Daniel Henrique Barboza Reviewed-by: Alistair Francis Message-Id: <20221229091828.1945072-4-bmeng@tinylab.org> Signed-off-by: Alistair Francis --- hw/char/riscv_htif.c | 7 ++----- hw/riscv/spike.c | 5 ++--- include/hw/char/riscv_htif.h | 7 ++----- 3 files changed, 6 insertions(+), 13 deletions(-) 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 13946acf0d..bc4953cf4a 100644 --- a/hw/riscv/spike.c +++ b/hw/riscv/spike.c @@ -316,9 +316,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) 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 From dadee9e3ce6ee6aad36fe3027eaa0f947358f812 Mon Sep 17 00:00:00 2001 From: Bin Meng Date: Thu, 29 Dec 2022 17:18:20 +0800 Subject: [PATCH 04/37] hw/char: riscv_htif: Use conventional 's' for HTIFState QEMU source codes tend to use 's' to represent the hardware state. Let's use it for HTIFState. Signed-off-by: Bin Meng Reviewed-by: Daniel Henrique Barboza Reviewed-by: Alistair Francis Message-Id: <20221229091828.1945072-5-bmeng@tinylab.org> Signed-off-by: Alistair Francis --- 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); From 1237c2d6942709cf82b999b6f6e8624b86ac495f Mon Sep 17 00:00:00 2001 From: Bin Meng Date: Thu, 29 Dec 2022 17:18:21 +0800 Subject: [PATCH 05/37] hw/char: riscv_htif: Move registers from CPUArchState to HTIFState 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 Reviewed-by: Daniel Henrique Barboza Reviewed-by: Alistair Francis Message-Id: <20221229091828.1945072-6-bmeng@tinylab.org> Signed-off-by: Alistair Francis --- hw/char/riscv_htif.c | 35 +++++++++++++++++------------------ hw/riscv/spike.c | 3 +-- include/hw/char/riscv_htif.h | 8 ++++---- target/riscv/cpu.h | 4 ---- target/riscv/machine.c | 6 ++---- 5 files changed, 24 insertions(+), 32 deletions(-) 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 bc4953cf4a..fb4152c2a2 100644 --- a/hw/riscv/spike.c +++ b/hw/riscv/spike.c @@ -316,8 +316,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/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 f5609b62a2..61a9a40958 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/target/riscv/machine.c b/target/riscv/machine.c index 65a8549ec2..c6ce318cce 100644 --- a/target/riscv/machine.c +++ b/target/riscv/machine.c @@ -333,8 +333,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), @@ -384,8 +384,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() From 03ef1899dd1194d58d51f41491ba24c87f901264 Mon Sep 17 00:00:00 2001 From: Bin Meng Date: Thu, 29 Dec 2022 17:18:22 +0800 Subject: [PATCH 06/37] hw/char: riscv_htif: Remove forward declarations for non-existent variables 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 Reviewed-by: Daniel Henrique Barboza Reviewed-by: Alistair Francis Message-Id: <20221229091828.1945072-7-bmeng@tinylab.org> Signed-off-by: Alistair Francis --- 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); From a6e13e31d5c34d59c28e908f3e51cf87bc82666f Mon Sep 17 00:00:00 2001 From: Bin Meng Date: Thu, 29 Dec 2022 17:18:23 +0800 Subject: [PATCH 07/37] hw/char: riscv_htif: Support console output via proxy syscall 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 Reviewed-by: Daniel Henrique Barboza Reviewed-by: Alistair Francis Message-Id: <20221229091828.1945072-8-bmeng@tinylab.org> Signed-off-by: Alistair Francis --- 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); From a8a7f680d25a6dc52b1a56a597563a6d6be5f8da Mon Sep 17 00:00:00 2001 From: Bin Meng Date: Thu, 29 Dec 2022 17:18:24 +0800 Subject: [PATCH 08/37] hw/riscv: spike: Remove the out-of-date comments Spike machine now supports OpenSBI plain binary bios image, so the comments are no longer valid. Signed-off-by: Bin Meng Reviewed-by: Daniel Henrique Barboza Reviewed-by: Alistair Francis Message-Id: <20221229091828.1945072-9-bmeng@tinylab.org> Signed-off-by: Alistair Francis --- hw/riscv/spike.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c index fb4152c2a2..df9f070707 100644 --- a/hw/riscv/spike.c +++ b/hw/riscv/spike.c @@ -255,11 +255,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, From 808faef7cd38222ac02e5876e5170c7d00982876 Mon Sep 17 00:00:00 2001 From: Daniel Henrique Barboza Date: Thu, 29 Dec 2022 17:18:25 +0800 Subject: [PATCH 09/37] hw/riscv/boot.c: make riscv_find_firmware() static MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The only caller is riscv_find_and_load_firmware(), which is in the same file. Signed-off-by: Daniel Henrique Barboza Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Alistair Francis Reviewed-by: Bin Meng Signed-off-by: Bin Meng Message-Id: <20221221182300.307900-5-dbarboza@ventanamicro.com> Message-Id: <20221229091828.1945072-10-bmeng@tinylab.org> Signed-off-by: Alistair Francis --- hw/riscv/boot.c | 44 ++++++++++++++++++++--------------------- include/hw/riscv/boot.h | 1 - 2 files changed, 22 insertions(+), 23 deletions(-) diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c index ebd351c840..7361d5c0d8 100644 --- a/hw/riscv/boot.c +++ b/hw/riscv/boot.c @@ -75,6 +75,28 @@ target_ulong riscv_calc_kernel_start_addr(RISCVHartArrayState *harts, } } +static char *riscv_find_firmware(const char *firmware_filename) +{ + char *filename; + + filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, firmware_filename); + if (filename == NULL) { + if (!qtest_enabled()) { + /* + * We only ship OpenSBI binary bios images in the QEMU source. + * For machines that use images other than the default bios, + * running QEMU test will complain hence let's suppress the error + * report for QEMU testing. + */ + error_report("Unable to load the RISC-V firmware \"%s\"", + firmware_filename); + exit(1); + } + } + + return filename; +} + target_ulong riscv_find_and_load_firmware(MachineState *machine, const char *default_machine_firmware, hwaddr firmware_load_addr, @@ -104,28 +126,6 @@ target_ulong riscv_find_and_load_firmware(MachineState *machine, return firmware_end_addr; } -char *riscv_find_firmware(const char *firmware_filename) -{ - char *filename; - - filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, firmware_filename); - if (filename == NULL) { - if (!qtest_enabled()) { - /* - * We only ship OpenSBI binary bios images in the QEMU source. - * For machines that use images other than the default bios, - * running QEMU test will complain hence let's suppress the error - * report for QEMU testing. - */ - error_report("Unable to load the RISC-V firmware \"%s\"", - firmware_filename); - exit(1); - } - } - - return filename; -} - target_ulong riscv_load_firmware(const char *firmware_filename, hwaddr firmware_load_addr, symbol_fn_t sym_cb) diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h index 93e5f8760d..c03e4e74c5 100644 --- a/include/hw/riscv/boot.h +++ b/include/hw/riscv/boot.h @@ -37,7 +37,6 @@ target_ulong riscv_find_and_load_firmware(MachineState *machine, const char *default_machine_firmware, hwaddr firmware_load_addr, symbol_fn_t sym_cb); -char *riscv_find_firmware(const char *firmware_filename); target_ulong riscv_load_firmware(const char *firmware_filename, hwaddr firmware_load_addr, symbol_fn_t sym_cb); From 9d3f7108bc43e93ceef7faa27c87eea8295c33ed Mon Sep 17 00:00:00 2001 From: Daniel Henrique Barboza Date: Thu, 29 Dec 2022 17:18:26 +0800 Subject: [PATCH 10/37] hw/riscv/boot.c: introduce riscv_default_firmware_name() 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 Signed-off-by: Daniel Henrique Barboza Reviewed-by: Alistair Francis Reviewed-by: Bin Meng Signed-off-by: Bin Meng Message-Id: <20221221182300.307900-6-dbarboza@ventanamicro.com> Message-Id: <20221229091828.1945072-11-bmeng@tinylab.org> Signed-off-by: Alistair Francis --- hw/riscv/boot.c | 9 +++++++++ hw/riscv/sifive_u.c | 11 ++++------- hw/riscv/spike.c | 14 +++++--------- hw/riscv/virt.c | 10 +++------- include/hw/riscv/boot.h | 1 + 5 files changed, 22 insertions(+), 23 deletions(-) diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c index 7361d5c0d8..e1a544b1d9 100644 --- a/hw/riscv/boot.c +++ b/hw/riscv/boot.c @@ -75,6 +75,15 @@ target_ulong riscv_calc_kernel_start_addr(RISCVHartArrayState *harts, } } +const char *riscv_default_firmware_name(RISCVHartArrayState *harts) +{ + if (riscv_is_32bit(harts)) { + return RISCV32_BIOS_BIN; + } + + return RISCV64_BIOS_BIN; +} + static char *riscv_find_firmware(const char *firmware_filename) { char *filename; diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c index b40a4767e2..a58ddb36ac 100644 --- a/hw/riscv/sifive_u.c +++ b/hw/riscv/sifive_u.c @@ -533,6 +533,7 @@ static void sifive_u_machine_init(MachineState *machine) MemoryRegion *flash0 = g_new(MemoryRegion, 1); target_ulong start_addr = memmap[SIFIVE_U_DEV_DRAM].base; target_ulong firmware_end_addr, kernel_start_addr; + const char *firmware_name; uint32_t start_addr_hi32 = 0x00000000; int i; uint32_t fdt_load_addr; @@ -595,13 +596,9 @@ static void sifive_u_machine_init(MachineState *machine) break; } - if (riscv_is_32bit(&s->soc.u_cpus)) { - firmware_end_addr = riscv_find_and_load_firmware(machine, - RISCV32_BIOS_BIN, start_addr, NULL); - } else { - firmware_end_addr = riscv_find_and_load_firmware(machine, - RISCV64_BIOS_BIN, start_addr, NULL); - } + firmware_name = riscv_default_firmware_name(&s->soc.u_cpus); + firmware_end_addr = riscv_find_and_load_firmware(machine, firmware_name, + start_addr, NULL); if (machine->kernel_filename) { kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc.u_cpus, diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c index df9f070707..3c8a8de673 100644 --- a/hw/riscv/spike.c +++ b/hw/riscv/spike.c @@ -190,6 +190,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; @@ -255,15 +256,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 94ff2a1584..408f7a2256 100644 --- a/hw/riscv/virt.c +++ b/hw/riscv/virt.c @@ -1237,6 +1237,7 @@ static void virt_machine_done(Notifier *notifier, void *data) MachineState *machine = MACHINE(s); target_ulong start_addr = memmap[VIRT_DRAM].base; target_ulong firmware_end_addr, kernel_start_addr; + const char *firmware_name = riscv_default_firmware_name(&s->soc[0]); uint32_t fdt_load_addr; uint64_t kernel_entry; @@ -1256,13 +1257,8 @@ static void virt_machine_done(Notifier *notifier, void *data) } } - if (riscv_is_32bit(&s->soc[0])) { - firmware_end_addr = riscv_find_and_load_firmware(machine, - RISCV32_BIOS_BIN, start_addr, NULL); - } else { - firmware_end_addr = riscv_find_and_load_firmware(machine, - RISCV64_BIOS_BIN, start_addr, NULL); - } + firmware_end_addr = riscv_find_and_load_firmware(machine, firmware_name, + start_addr, NULL); /* * Init fw_cfg. Must be done before riscv_load_fdt, otherwise the device diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h index c03e4e74c5..60cf320c88 100644 --- a/include/hw/riscv/boot.h +++ b/include/hw/riscv/boot.h @@ -37,6 +37,7 @@ target_ulong riscv_find_and_load_firmware(MachineState *machine, const char *default_machine_firmware, hwaddr firmware_load_addr, symbol_fn_t sym_cb); +const char *riscv_default_firmware_name(RISCVHartArrayState *harts); target_ulong riscv_load_firmware(const char *firmware_filename, hwaddr firmware_load_addr, symbol_fn_t sym_cb); From 8f6196266e607a4a014ef1a5ab05b93343f678df Mon Sep 17 00:00:00 2001 From: Bin Meng Date: Thu, 29 Dec 2022 17:18:27 +0800 Subject: [PATCH 11/37] hw/riscv/boot.c: Introduce riscv_find_firmware() 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 Reviewed-by: Daniel Henrique Barboza Reviewed-by: Alistair Francis Message-Id: <20221229091828.1945072-12-bmeng@tinylab.org> Signed-off-by: Alistair Francis --- hw/riscv/boot.c | 41 ++++++++++++++++++++++++++--------------- include/hw/riscv/boot.h | 2 ++ 2 files changed, 28 insertions(+), 15 deletions(-) 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,24 +106,35 @@ static char *riscv_find_firmware(const char *firmware_filename) return filename; } +char *riscv_find_firmware(const char *firmware_filename, + const char *default_machine_firmware) +{ + char *filename = NULL; + + 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. + */ + 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 = NULL; + char *firmware_filename; target_ulong firmware_end_addr = firmware_load_addr; - if ((!machine->firmware) || (!strcmp(machine->firmware, "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); - } + firmware_filename = riscv_find_firmware(machine->firmware, + default_machine_firmware); if (firmware_filename) { /* If not "none" load the firmware */ 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); From 71d68c48be96366fb89f7a2dd9d82dd86bcbe542 Mon Sep 17 00:00:00 2001 From: Bin Meng Date: Thu, 29 Dec 2022 18:31:23 +0800 Subject: [PATCH 12/37] hw/riscv: spike: Decouple create_fdt() dependency to ELF loading At present create_fdt() calls htif_uses_elf_symbols() to determine whether to insert a 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 Reviewed-by: Daniel Henrique Barboza Message-Id: <20221229091828.1945072-13-bmeng@tinylab.org> Signed-off-by: Alistair Francis --- hw/char/riscv_htif.c | 17 +++++----- hw/riscv/spike.c | 61 ++++++++++++++++++++++++++++++------ include/hw/char/riscv_htif.h | 5 +-- 3 files changed, 59 insertions(+), 24 deletions(-) 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 3c8a8de673..1679c325d5 100644 --- a/hw/riscv/spike.c +++ b/hw/riscv/spike.c @@ -49,7 +49,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; @@ -77,7 +78,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); } @@ -183,18 +184,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)) { @@ -256,10 +272,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) { @@ -279,7 +319,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) { @@ -307,7 +347,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) 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 From 44e7372b213bad4e4589d765f011b25c897c8ab1 Mon Sep 17 00:00:00 2001 From: Dongxue Zhang Date: Thu, 15 Dec 2022 16:27:14 +0800 Subject: [PATCH 13/37] target/riscv/cpu.c: Fix elen check The elen check should be cpu->cfg.elen in range [8, 64]. Signed-off-by: Dongxue Zhang Reviewed-by: LIU Zhiwei Reviewed-by: Frank Chang Reviewed-by: Alistair Francis Message-Id: <167236721596.15277.2653405273227256289-0@git.sr.ht> [ Changes by AF: - Tidy up commit message ] Signed-off-by: Alistair Francis --- target/riscv/cpu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c index cc75ca7667..a2e6238bd7 100644 --- a/target/riscv/cpu.c +++ b/target/riscv/cpu.c @@ -882,7 +882,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp) "Vector extension ELEN must be power of 2"); return; } - if (cpu->cfg.elen > 64 || cpu->cfg.vlen < 8) { + if (cpu->cfg.elen > 64 || cpu->cfg.elen < 8) { error_setg(errp, "Vector extension implementation only supports ELEN " "in the range [8, 64]"); From db2b9a59ca633602c4a18474a104182920858060 Mon Sep 17 00:00:00 2001 From: Daniel Henrique Barboza Date: Mon, 2 Jan 2023 08:52:31 -0300 Subject: [PATCH 14/37] tests/avocado: add RISC-V OpenSBI boot test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This test is used to do a quick sanity check to ensure that we're able to run the existing QEMU FW image. 'sifive_u', 'spike' and 'virt' riscv64 machines, and 'sifive_u' and 'virt' 32 bit machines are able to run the default RISCV64_BIOS_BIN | RISCV32_BIOS_BIN firmware with minimal options. The riscv32 'spike' machine isn't bootable at this moment, requiring an OpenSBI fix [1] and QEMU side changes [2]. We could just leave at that or add a 'skip' test to remind us about it. To work as a reminder that we have a riscv32 'spike' test that should be enabled as soon as OpenSBI QEMU rom receives the fix, we're adding a 'skip' test: (06/18) tests/avocado/riscv_opensbi.py:RiscvOpenSBI.test_riscv32_spike: SKIP: requires OpenSBI fix to work [1] https://patchwork.ozlabs.org/project/opensbi/patch/20221226033603.1860569-1-bmeng@tinylab.org/ [2] https://patchwork.ozlabs.org/project/qemu-devel/list/?series=334159 Cc: Cleber Rosa Cc: Philippe Mathieu-Daudé Reviewed-by: Bin Meng Tested-by: Bin Meng Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Daniel Henrique Barboza Acked-by: Alistair Francis Message-Id: <20230102115241.25733-2-dbarboza@ventanamicro.com> Signed-off-by: Alistair Francis --- tests/avocado/riscv_opensbi.py | 65 ++++++++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+) create mode 100644 tests/avocado/riscv_opensbi.py diff --git a/tests/avocado/riscv_opensbi.py b/tests/avocado/riscv_opensbi.py new file mode 100644 index 0000000000..e02f0d404a --- /dev/null +++ b/tests/avocado/riscv_opensbi.py @@ -0,0 +1,65 @@ +# OpenSBI boot test for RISC-V machines +# +# Copyright (c) 2022, Ventana Micro +# +# This work is licensed under the terms of the GNU GPL, version 2 or +# later. See the COPYING file in the top-level directory. + +from avocado_qemu import QemuSystemTest +from avocado import skip +from avocado_qemu import wait_for_console_pattern + +class RiscvOpenSBI(QemuSystemTest): + """ + :avocado: tags=accel:tcg + """ + timeout = 5 + + def boot_opensbi(self): + self.vm.set_console() + self.vm.launch() + wait_for_console_pattern(self, 'Platform Name') + wait_for_console_pattern(self, 'Boot HART MEDELEG') + + @skip("requires OpenSBI fix to work") + def test_riscv32_spike(self): + """ + :avocado: tags=arch:riscv32 + :avocado: tags=machine:spike + """ + self.boot_opensbi() + + def test_riscv64_spike(self): + """ + :avocado: tags=arch:riscv64 + :avocado: tags=machine:spike + """ + self.boot_opensbi() + + def test_riscv32_sifive_u(self): + """ + :avocado: tags=arch:riscv32 + :avocado: tags=machine:sifive_u + """ + self.boot_opensbi() + + def test_riscv64_sifive_u(self): + """ + :avocado: tags=arch:riscv64 + :avocado: tags=machine:sifive_u + """ + self.boot_opensbi() + + def test_riscv32_virt(self): + """ + :avocado: tags=arch:riscv32 + :avocado: tags=machine:virt + """ + self.boot_opensbi() + + def test_riscv64_virt(self): + """ + :avocado: tags=arch:riscv64 + :avocado: tags=machine:virt + """ + self.boot_opensbi() From 3139929da4da015f372c11f9e9e1f2538f9767ed Mon Sep 17 00:00:00 2001 From: Daniel Henrique Barboza Date: Mon, 2 Jan 2023 08:52:32 -0300 Subject: [PATCH 15/37] hw/riscv/spike: use 'fdt' from MachineState MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The MachineState object provides a 'fdt' pointer that is already being used by other RISC-V machines, and it's also used by the 'dumpdtb' QMP command. Remove the 'fdt' pointer from SpikeState and use MachineState::fdt instead. Signed-off-by: Daniel Henrique Barboza Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Alistair Francis Reviewed-by: Bin Meng Message-Id: <20230102115241.25733-3-dbarboza@ventanamicro.com> Signed-off-by: Alistair Francis --- hw/riscv/spike.c | 12 +++++------- include/hw/riscv/spike.h | 2 -- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c index 1679c325d5..25c5420ee6 100644 --- a/hw/riscv/spike.c +++ b/hw/riscv/spike.c @@ -53,6 +53,7 @@ static void create_fdt(SpikeState *s, const MemMapEntry *memmap, bool is_32_bit, bool htif_custom_base) { void *fdt; + int fdt_size; uint64_t addr, size; unsigned long clint_addr; int cpu, socket; @@ -65,7 +66,7 @@ static void create_fdt(SpikeState *s, const MemMapEntry *memmap, "sifive,clint0", "riscv,clint0" }; - fdt = s->fdt = create_device_tree(&s->fdt_size); + fdt = mc->fdt = create_device_tree(&fdt_size); if (!fdt) { error_report("create_device_tree() failed"); exit(1); @@ -327,18 +328,15 @@ static void spike_board_init(MachineState *machine) hwaddr end = riscv_load_initrd(machine->initrd_filename, machine->ram_size, kernel_entry, &start); - qemu_fdt_setprop_cell(s->fdt, "/chosen", + qemu_fdt_setprop_cell(machine->fdt, "/chosen", "linux,initrd-start", start); - qemu_fdt_setprop_cell(s->fdt, "/chosen", "linux,initrd-end", + qemu_fdt_setprop_cell(machine->fdt, "/chosen", "linux,initrd-end", end); } /* Compute the fdt load address in dram */ fdt_load_addr = riscv_load_fdt(memmap[SPIKE_DRAM].base, - machine->ram_size, s->fdt); - - /* Set machine->fdt for 'dumpdtb' QMP/HMP command */ - machine->fdt = s->fdt; + machine->ram_size, machine->fdt); /* load the reset vector */ riscv_setup_rom_reset_vec(machine, &s->soc[0], memmap[SPIKE_DRAM].base, diff --git a/include/hw/riscv/spike.h b/include/hw/riscv/spike.h index 73bf2a9aad..0c2a223763 100644 --- a/include/hw/riscv/spike.h +++ b/include/hw/riscv/spike.h @@ -37,8 +37,6 @@ struct SpikeState { /*< public >*/ RISCVHartArrayState soc[SPIKE_SOCKETS_MAX]; - void *fdt; - int fdt_size; }; enum { From 60c7dfa2a3d7eb3919054367c2d03d4fc1bef3f1 Mon Sep 17 00:00:00 2001 From: Daniel Henrique Barboza Date: Mon, 2 Jan 2023 08:52:33 -0300 Subject: [PATCH 16/37] hw/riscv/sifive_u: use 'fdt' from MachineState MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The MachineState object provides a 'fdt' pointer that is already being used by other RISC-V machines, and it's also used by the 'dumpdtb' QMP command. Remove the 'fdt' pointer from SiFiveUState and use MachineState::fdt instead. Cc: Palmer Dabbelt Signed-off-by: Daniel Henrique Barboza Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Alistair Francis Reviewed-by: Bin Meng Message-Id: <20230102115241.25733-4-dbarboza@ventanamicro.com> Signed-off-by: Alistair Francis --- hw/riscv/sifive_u.c | 15 ++++++--------- include/hw/riscv/sifive_u.h | 3 --- 2 files changed, 6 insertions(+), 12 deletions(-) diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c index a58ddb36ac..ddceb750ea 100644 --- a/hw/riscv/sifive_u.c +++ b/hw/riscv/sifive_u.c @@ -98,7 +98,7 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry *memmap, { MachineState *ms = MACHINE(qdev_get_machine()); void *fdt; - int cpu; + int cpu, fdt_size; uint32_t *cells; char *nodename; uint32_t plic_phandle, prci_phandle, gpio_phandle, phandle = 1; @@ -112,14 +112,14 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry *memmap, }; if (ms->dtb) { - fdt = s->fdt = load_device_tree(ms->dtb, &s->fdt_size); + fdt = ms->fdt = load_device_tree(ms->dtb, &fdt_size); if (!fdt) { error_report("load_device_tree() failed"); exit(1); } goto update_bootargs; } else { - fdt = s->fdt = create_device_tree(&s->fdt_size); + fdt = ms->fdt = create_device_tree(&fdt_size); if (!fdt) { error_report("create_device_tree() failed"); exit(1); @@ -612,9 +612,9 @@ static void sifive_u_machine_init(MachineState *machine) hwaddr end = riscv_load_initrd(machine->initrd_filename, machine->ram_size, kernel_entry, &start); - qemu_fdt_setprop_cell(s->fdt, "/chosen", + qemu_fdt_setprop_cell(machine->fdt, "/chosen", "linux,initrd-start", start); - qemu_fdt_setprop_cell(s->fdt, "/chosen", "linux,initrd-end", + qemu_fdt_setprop_cell(machine->fdt, "/chosen", "linux,initrd-end", end); } } else { @@ -627,14 +627,11 @@ static void sifive_u_machine_init(MachineState *machine) /* Compute the fdt load address in dram */ fdt_load_addr = riscv_load_fdt(memmap[SIFIVE_U_DEV_DRAM].base, - machine->ram_size, s->fdt); + machine->ram_size, machine->fdt); if (!riscv_is_32bit(&s->soc.u_cpus)) { start_addr_hi32 = (uint64_t)start_addr >> 32; } - /* Set machine->fdt for 'dumpdtb' QMP/HMP command */ - machine->fdt = s->fdt; - /* reset vector */ uint32_t reset_vec[12] = { s->msel, /* MSEL pin state */ diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h index a67328f7ad..65af306963 100644 --- a/include/hw/riscv/sifive_u.h +++ b/include/hw/riscv/sifive_u.h @@ -69,9 +69,6 @@ typedef struct SiFiveUState { /*< public >*/ SiFiveUSoCState soc; - void *fdt; - int fdt_size; - bool start_in_flash; uint32_t msel; uint32_t serial; From 1db0c57adeb981c9581f7729e8e8dfb60bdb4e7c Mon Sep 17 00:00:00 2001 From: Daniel Henrique Barboza Date: Mon, 2 Jan 2023 08:52:34 -0300 Subject: [PATCH 17/37] hw/riscv/boot.c: exit early if filename is NULL in load functions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit riscv_load_firmware(), riscv_load_initrd() and riscv_load_kernel() works under the assumption that a 'filename' parameter is always not NULL. This is currently the case since all callers of these functions are checking for NULL before calling them. Add an g_assert() to make sure that a NULL value in these cases are to be considered a bug. Suggested-by: Alex Bennée Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Daniel Henrique Barboza Reviewed-by: Bin Meng Reviewed-by: Alistair Francis Message-Id: <20230102115241.25733-5-dbarboza@ventanamicro.com> Signed-off-by: Alistair Francis --- hw/riscv/boot.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c index 98b80af51b..31aa3385a0 100644 --- a/hw/riscv/boot.c +++ b/hw/riscv/boot.c @@ -153,6 +153,8 @@ target_ulong riscv_load_firmware(const char *firmware_filename, uint64_t firmware_entry, firmware_end; ssize_t firmware_size; + g_assert(firmware_filename != NULL); + if (load_elf_ram_sym(firmware_filename, NULL, NULL, NULL, &firmware_entry, NULL, &firmware_end, NULL, 0, EM_RISCV, 1, 0, NULL, true, sym_cb) > 0) { @@ -177,6 +179,8 @@ target_ulong riscv_load_kernel(const char *kernel_filename, { uint64_t kernel_load_base, kernel_entry; + g_assert(kernel_filename != NULL); + /* * NB: Use low address not ELF entry point to ensure that the fw_dynamic * behaviour when loading an ELF matches the fw_payload, fw_jump and BBL @@ -209,6 +213,8 @@ hwaddr riscv_load_initrd(const char *filename, uint64_t mem_size, { ssize_t size; + g_assert(filename != NULL); + /* * We want to put the initrd far enough into RAM that when the * kernel is uncompressed it will not clobber the initrd. However From c44df400d9fc23d1d135f6aa723cb58ada858ee3 Mon Sep 17 00:00:00 2001 From: Daniel Henrique Barboza Date: Mon, 2 Jan 2023 08:52:35 -0300 Subject: [PATCH 18/37] hw/riscv/spike.c: load initrd right after riscv_load_kernel() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This will make the code more in line with what the other boards are doing. We'll also avoid an extra check to machine->kernel_filename since we already checked that before executing riscv_load_kernel(). Signed-off-by: Daniel Henrique Barboza Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Alistair Francis Reviewed-by: Bin Meng Message-Id: <20230102115241.25733-6-dbarboza@ventanamicro.com> Signed-off-by: Alistair Francis --- hw/riscv/spike.c | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c index 25c5420ee6..004dfb2d5b 100644 --- a/hw/riscv/spike.c +++ b/hw/riscv/spike.c @@ -302,6 +302,10 @@ static void spike_board_init(MachineState *machine) g_free(firmware_name); } + /* Create device tree */ + create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline, + riscv_is_32bit(&s->soc[0]), htif_custom_base); + /* Load kernel */ if (machine->kernel_filename) { kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc[0], @@ -310,6 +314,17 @@ static void spike_board_init(MachineState *machine) kernel_entry = riscv_load_kernel(machine->kernel_filename, kernel_start_addr, htif_symbol_callback); + + if (machine->initrd_filename) { + hwaddr start; + hwaddr end = riscv_load_initrd(machine->initrd_filename, + machine->ram_size, kernel_entry, + &start); + qemu_fdt_setprop_cell(machine->fdt, "/chosen", + "linux,initrd-start", start); + qemu_fdt_setprop_cell(machine->fdt, "/chosen", "linux,initrd-end", + end); + } } else { /* * If dynamic firmware is used, it doesn't know where is the next mode @@ -318,22 +333,6 @@ static void spike_board_init(MachineState *machine) kernel_entry = 0; } - /* Create device tree */ - create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline, - riscv_is_32bit(&s->soc[0]), htif_custom_base); - - /* Load initrd */ - if (machine->kernel_filename && machine->initrd_filename) { - hwaddr start; - hwaddr end = riscv_load_initrd(machine->initrd_filename, - machine->ram_size, kernel_entry, - &start); - qemu_fdt_setprop_cell(machine->fdt, "/chosen", - "linux,initrd-start", start); - qemu_fdt_setprop_cell(machine->fdt, "/chosen", "linux,initrd-end", - end); - } - /* Compute the fdt load address in dram */ fdt_load_addr = riscv_load_fdt(memmap[SPIKE_DRAM].base, machine->ram_size, machine->fdt); From b9a65476cbfc7a47a5c06ffdd58922fd295c5027 Mon Sep 17 00:00:00 2001 From: Daniel Henrique Barboza Date: Mon, 2 Jan 2023 08:52:36 -0300 Subject: [PATCH 19/37] hw/riscv: write initrd 'chosen' FDT inside riscv_load_initrd() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit riscv_load_initrd() returns the initrd end addr while also writing a 'start' var to mark the addr start. These informations are being used just to write the initrd FDT node. Every existing caller of riscv_load_initrd() is writing the FDT in the same manner. We can simplify things by writing the FDT inside riscv_load_initrd(), sparing callers from having to manage start/end addrs to write the FDT themselves. An 'if (fdt)' check is already inserted at the end of the function because we'll end up using it later on with other boards that doesn´t have a FDT. Cc: Palmer Dabbelt Signed-off-by: Daniel Henrique Barboza Reviewed-by: Bin Meng Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Alistair Francis Message-Id: <20230102115241.25733-7-dbarboza@ventanamicro.com> Signed-off-by: Alistair Francis --- hw/riscv/boot.c | 18 ++++++++++++------ hw/riscv/microchip_pfsoc.c | 10 ++-------- hw/riscv/sifive_u.c | 10 ++-------- hw/riscv/spike.c | 10 ++-------- hw/riscv/virt.c | 10 ++-------- include/hw/riscv/boot.h | 4 ++-- 6 files changed, 22 insertions(+), 40 deletions(-) diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c index 31aa3385a0..6b948d1c9e 100644 --- a/hw/riscv/boot.c +++ b/hw/riscv/boot.c @@ -208,9 +208,10 @@ target_ulong riscv_load_kernel(const char *kernel_filename, exit(1); } -hwaddr riscv_load_initrd(const char *filename, uint64_t mem_size, - uint64_t kernel_entry, hwaddr *start) +void riscv_load_initrd(const char *filename, uint64_t mem_size, + uint64_t kernel_entry, void *fdt) { + hwaddr start, end; ssize_t size; g_assert(filename != NULL); @@ -226,18 +227,23 @@ hwaddr riscv_load_initrd(const char *filename, uint64_t mem_size, * halfway into RAM, and for boards with 256MB of RAM or more we put * the initrd at 128MB. */ - *start = kernel_entry + MIN(mem_size / 2, 128 * MiB); + start = kernel_entry + MIN(mem_size / 2, 128 * MiB); - size = load_ramdisk(filename, *start, mem_size - *start); + size = load_ramdisk(filename, start, mem_size - start); if (size == -1) { - size = load_image_targphys(filename, *start, mem_size - *start); + size = load_image_targphys(filename, start, mem_size - start); if (size == -1) { error_report("could not load ramdisk '%s'", filename); exit(1); } } - return *start + size; + /* Some RISC-V machines (e.g. opentitan) don't have a fdt. */ + if (fdt) { + end = start + size; + qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-start", start); + qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-end", end); + } } uint64_t riscv_load_fdt(hwaddr dram_base, uint64_t mem_size, void *fdt) diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c index b10321b564..593a799549 100644 --- a/hw/riscv/microchip_pfsoc.c +++ b/hw/riscv/microchip_pfsoc.c @@ -633,14 +633,8 @@ static void microchip_icicle_kit_machine_init(MachineState *machine) kernel_start_addr, NULL); if (machine->initrd_filename) { - hwaddr start; - hwaddr end = riscv_load_initrd(machine->initrd_filename, - machine->ram_size, kernel_entry, - &start); - qemu_fdt_setprop_cell(machine->fdt, "/chosen", - "linux,initrd-start", start); - qemu_fdt_setprop_cell(machine->fdt, "/chosen", - "linux,initrd-end", end); + riscv_load_initrd(machine->initrd_filename, machine->ram_size, + kernel_entry, machine->fdt); } if (machine->kernel_cmdline && *machine->kernel_cmdline) { diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c index ddceb750ea..37f5087172 100644 --- a/hw/riscv/sifive_u.c +++ b/hw/riscv/sifive_u.c @@ -608,14 +608,8 @@ static void sifive_u_machine_init(MachineState *machine) kernel_start_addr, NULL); if (machine->initrd_filename) { - hwaddr start; - hwaddr end = riscv_load_initrd(machine->initrd_filename, - machine->ram_size, kernel_entry, - &start); - qemu_fdt_setprop_cell(machine->fdt, "/chosen", - "linux,initrd-start", start); - qemu_fdt_setprop_cell(machine->fdt, "/chosen", "linux,initrd-end", - end); + riscv_load_initrd(machine->initrd_filename, machine->ram_size, + kernel_entry, machine->fdt); } } else { /* diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c index 004dfb2d5b..5668fe0694 100644 --- a/hw/riscv/spike.c +++ b/hw/riscv/spike.c @@ -316,14 +316,8 @@ static void spike_board_init(MachineState *machine) htif_symbol_callback); if (machine->initrd_filename) { - hwaddr start; - hwaddr end = riscv_load_initrd(machine->initrd_filename, - machine->ram_size, kernel_entry, - &start); - qemu_fdt_setprop_cell(machine->fdt, "/chosen", - "linux,initrd-start", start); - qemu_fdt_setprop_cell(machine->fdt, "/chosen", "linux,initrd-end", - end); + riscv_load_initrd(machine->initrd_filename, machine->ram_size, + kernel_entry, machine->fdt); } } else { /* diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c index 408f7a2256..5967b136b4 100644 --- a/hw/riscv/virt.c +++ b/hw/riscv/virt.c @@ -1291,14 +1291,8 @@ static void virt_machine_done(Notifier *notifier, void *data) kernel_start_addr, NULL); if (machine->initrd_filename) { - hwaddr start; - hwaddr end = riscv_load_initrd(machine->initrd_filename, - machine->ram_size, kernel_entry, - &start); - qemu_fdt_setprop_cell(machine->fdt, "/chosen", - "linux,initrd-start", start); - qemu_fdt_setprop_cell(machine->fdt, "/chosen", "linux,initrd-end", - end); + riscv_load_initrd(machine->initrd_filename, machine->ram_size, + kernel_entry, machine->fdt); } } else { /* diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h index b273ab22f7..e37e1d1238 100644 --- a/include/hw/riscv/boot.h +++ b/include/hw/riscv/boot.h @@ -46,8 +46,8 @@ target_ulong riscv_load_firmware(const char *firmware_filename, target_ulong riscv_load_kernel(const char *kernel_filename, target_ulong firmware_end_addr, symbol_fn_t sym_cb); -hwaddr riscv_load_initrd(const char *filename, uint64_t mem_size, - uint64_t kernel_entry, hwaddr *start); +void riscv_load_initrd(const char *filename, uint64_t mem_size, + uint64_t kernel_entry, void *fdt); uint64_t riscv_load_fdt(hwaddr dram_start, uint64_t dram_size, void *fdt); void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts, hwaddr saddr, From b1f19f238cae53b5c90085db45e0335af19f5387 Mon Sep 17 00:00:00 2001 From: Daniel Henrique Barboza Date: Mon, 2 Jan 2023 08:52:37 -0300 Subject: [PATCH 20/37] hw/riscv: write bootargs 'chosen' FDT after riscv_load_kernel() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sifive_u, spike and virt machines are writing the 'bootargs' FDT node during their respective create_fdt(). Given that bootargs is written only when '-append' is used, and this option is only allowed with the '-kernel' option, which in turn is already being check before executing riscv_load_kernel(), write 'bootargs' in the same code path as riscv_load_kernel(). Cc: Palmer Dabbelt Signed-off-by: Daniel Henrique Barboza Reviewed-by: Bin Meng Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Alistair Francis Message-Id: <20230102115241.25733-8-dbarboza@ventanamicro.com> Signed-off-by: Alistair Francis --- hw/riscv/sifive_u.c | 11 +++++------ hw/riscv/spike.c | 9 +++++---- hw/riscv/virt.c | 11 +++++------ 3 files changed, 15 insertions(+), 16 deletions(-) diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c index 37f5087172..3e6df87b5b 100644 --- a/hw/riscv/sifive_u.c +++ b/hw/riscv/sifive_u.c @@ -117,7 +117,6 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry *memmap, error_report("load_device_tree() failed"); exit(1); } - goto update_bootargs; } else { fdt = ms->fdt = create_device_tree(&fdt_size); if (!fdt) { @@ -510,11 +509,6 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry *memmap, qemu_fdt_setprop_string(fdt, "/aliases", "serial0", nodename); g_free(nodename); - -update_bootargs: - if (cmdline && *cmdline) { - qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline); - } } static void sifive_u_machine_reset(void *opaque, int n, int level) @@ -611,6 +605,11 @@ static void sifive_u_machine_init(MachineState *machine) riscv_load_initrd(machine->initrd_filename, machine->ram_size, kernel_entry, machine->fdt); } + + if (machine->kernel_cmdline && *machine->kernel_cmdline) { + qemu_fdt_setprop_string(machine->fdt, "/chosen", "bootargs", + machine->kernel_cmdline); + } } else { /* * If dynamic firmware is used, it doesn't know where is the next mode diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c index 5668fe0694..60e2912be5 100644 --- a/hw/riscv/spike.c +++ b/hw/riscv/spike.c @@ -179,10 +179,6 @@ static void create_fdt(SpikeState *s, const MemMapEntry *memmap, qemu_fdt_add_subnode(fdt, "/chosen"); qemu_fdt_setprop_string(fdt, "/chosen", "stdout-path", "/htif"); - - if (cmdline && *cmdline) { - qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline); - } } static bool spike_test_elf_image(char *filename) @@ -319,6 +315,11 @@ static void spike_board_init(MachineState *machine) riscv_load_initrd(machine->initrd_filename, machine->ram_size, kernel_entry, machine->fdt); } + + if (machine->kernel_cmdline && *machine->kernel_cmdline) { + qemu_fdt_setprop_string(machine->fdt, "/chosen", "bootargs", + machine->kernel_cmdline); + } } else { /* * If dynamic firmware is used, it doesn't know where is the next mode diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c index 5967b136b4..6c946b6def 100644 --- a/hw/riscv/virt.c +++ b/hw/riscv/virt.c @@ -1012,7 +1012,6 @@ static void create_fdt(RISCVVirtState *s, const MemMapEntry *memmap, error_report("load_device_tree() failed"); exit(1); } - goto update_bootargs; } else { mc->fdt = create_device_tree(&s->fdt_size); if (!mc->fdt) { @@ -1050,11 +1049,6 @@ static void create_fdt(RISCVVirtState *s, const MemMapEntry *memmap, create_fdt_fw_cfg(s, memmap); create_fdt_pmu(s); -update_bootargs: - if (cmdline && *cmdline) { - qemu_fdt_setprop_string(mc->fdt, "/chosen", "bootargs", cmdline); - } - /* Pass seed to RNG */ qemu_guest_getrandom_nofail(rng_seed, sizeof(rng_seed)); qemu_fdt_setprop(mc->fdt, "/chosen", "rng-seed", rng_seed, sizeof(rng_seed)); @@ -1294,6 +1288,11 @@ static void virt_machine_done(Notifier *notifier, void *data) riscv_load_initrd(machine->initrd_filename, machine->ram_size, kernel_entry, machine->fdt); } + + if (machine->kernel_cmdline && *machine->kernel_cmdline) { + qemu_fdt_setprop_string(machine->fdt, "/chosen", "bootargs", + machine->kernel_cmdline); + } } else { /* * If dynamic firmware is used, it doesn't know where is the next mode From 1f99146103dc49aabfa832f8527804087a4c2651 Mon Sep 17 00:00:00 2001 From: Daniel Henrique Barboza Date: Mon, 2 Jan 2023 08:52:38 -0300 Subject: [PATCH 21/37] hw/riscv/boot.c: use MachineState in riscv_load_initrd() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 'filename', 'mem_size' and 'fdt' from riscv_load_initrd() can all be retrieved by the MachineState object for all callers. Cc: Palmer Dabbelt Signed-off-by: Daniel Henrique Barboza Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Bin Meng Reviewed-by: Alistair Francis Message-Id: <20230102115241.25733-9-dbarboza@ventanamicro.com> Signed-off-by: Alistair Francis --- hw/riscv/boot.c | 6 ++++-- hw/riscv/microchip_pfsoc.c | 3 +-- hw/riscv/sifive_u.c | 3 +-- hw/riscv/spike.c | 3 +-- hw/riscv/virt.c | 3 +-- include/hw/riscv/boot.h | 3 +-- 6 files changed, 9 insertions(+), 12 deletions(-) diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c index 6b948d1c9e..d3e780c3b6 100644 --- a/hw/riscv/boot.c +++ b/hw/riscv/boot.c @@ -208,9 +208,11 @@ target_ulong riscv_load_kernel(const char *kernel_filename, exit(1); } -void riscv_load_initrd(const char *filename, uint64_t mem_size, - uint64_t kernel_entry, void *fdt) +void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry) { + const char *filename = machine->initrd_filename; + uint64_t mem_size = machine->ram_size; + void *fdt = machine->fdt; hwaddr start, end; ssize_t size; diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c index 593a799549..1e9b0a420e 100644 --- a/hw/riscv/microchip_pfsoc.c +++ b/hw/riscv/microchip_pfsoc.c @@ -633,8 +633,7 @@ static void microchip_icicle_kit_machine_init(MachineState *machine) kernel_start_addr, NULL); if (machine->initrd_filename) { - riscv_load_initrd(machine->initrd_filename, machine->ram_size, - kernel_entry, machine->fdt); + riscv_load_initrd(machine, kernel_entry); } if (machine->kernel_cmdline && *machine->kernel_cmdline) { diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c index 3e6df87b5b..c40885ed5c 100644 --- a/hw/riscv/sifive_u.c +++ b/hw/riscv/sifive_u.c @@ -602,8 +602,7 @@ static void sifive_u_machine_init(MachineState *machine) kernel_start_addr, NULL); if (machine->initrd_filename) { - riscv_load_initrd(machine->initrd_filename, machine->ram_size, - kernel_entry, machine->fdt); + riscv_load_initrd(machine, kernel_entry); } if (machine->kernel_cmdline && *machine->kernel_cmdline) { diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c index 60e2912be5..99dec74fe8 100644 --- a/hw/riscv/spike.c +++ b/hw/riscv/spike.c @@ -312,8 +312,7 @@ static void spike_board_init(MachineState *machine) htif_symbol_callback); if (machine->initrd_filename) { - riscv_load_initrd(machine->initrd_filename, machine->ram_size, - kernel_entry, machine->fdt); + riscv_load_initrd(machine, kernel_entry); } if (machine->kernel_cmdline && *machine->kernel_cmdline) { diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c index 6c946b6def..02f1369843 100644 --- a/hw/riscv/virt.c +++ b/hw/riscv/virt.c @@ -1285,8 +1285,7 @@ static void virt_machine_done(Notifier *notifier, void *data) kernel_start_addr, NULL); if (machine->initrd_filename) { - riscv_load_initrd(machine->initrd_filename, machine->ram_size, - kernel_entry, machine->fdt); + riscv_load_initrd(machine, kernel_entry); } if (machine->kernel_cmdline && *machine->kernel_cmdline) { diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h index e37e1d1238..cfd72ecabf 100644 --- a/include/hw/riscv/boot.h +++ b/include/hw/riscv/boot.h @@ -46,8 +46,7 @@ target_ulong riscv_load_firmware(const char *firmware_filename, target_ulong riscv_load_kernel(const char *kernel_filename, target_ulong firmware_end_addr, symbol_fn_t sym_cb); -void riscv_load_initrd(const char *filename, uint64_t mem_size, - uint64_t kernel_entry, void *fdt); +void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry); uint64_t riscv_load_fdt(hwaddr dram_start, uint64_t dram_size, void *fdt); void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts, hwaddr saddr, From 60c1f05e365e08cbdc6a9a64e29a109903a32ee6 Mon Sep 17 00:00:00 2001 From: Daniel Henrique Barboza Date: Mon, 2 Jan 2023 08:52:39 -0300 Subject: [PATCH 22/37] hw/riscv/boot.c: use MachineState in riscv_load_kernel() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All callers are using kernel_filename as machine->kernel_filename. This will also simplify the changes in riscv_load_kernel() that we're going to do next. Cc: Palmer Dabbelt Signed-off-by: Daniel Henrique Barboza Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Bin Meng Reviewed-by: Alistair Francis Message-Id: <20230102115241.25733-10-dbarboza@ventanamicro.com> Signed-off-by: Alistair Francis --- hw/riscv/boot.c | 3 ++- hw/riscv/microchip_pfsoc.c | 3 +-- hw/riscv/opentitan.c | 3 +-- hw/riscv/sifive_e.c | 3 +-- hw/riscv/sifive_u.c | 3 +-- hw/riscv/spike.c | 3 +-- hw/riscv/virt.c | 3 +-- include/hw/riscv/boot.h | 2 +- 8 files changed, 9 insertions(+), 14 deletions(-) diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c index d3e780c3b6..2594276223 100644 --- a/hw/riscv/boot.c +++ b/hw/riscv/boot.c @@ -173,10 +173,11 @@ target_ulong riscv_load_firmware(const char *firmware_filename, exit(1); } -target_ulong riscv_load_kernel(const char *kernel_filename, +target_ulong riscv_load_kernel(MachineState *machine, target_ulong kernel_start_addr, symbol_fn_t sym_cb) { + const char *kernel_filename = machine->kernel_filename; uint64_t kernel_load_base, kernel_entry; g_assert(kernel_filename != NULL); diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c index 1e9b0a420e..82ae5e7023 100644 --- a/hw/riscv/microchip_pfsoc.c +++ b/hw/riscv/microchip_pfsoc.c @@ -629,8 +629,7 @@ static void microchip_icicle_kit_machine_init(MachineState *machine) kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc.u_cpus, firmware_end_addr); - kernel_entry = riscv_load_kernel(machine->kernel_filename, - kernel_start_addr, NULL); + kernel_entry = riscv_load_kernel(machine, kernel_start_addr, NULL); if (machine->initrd_filename) { riscv_load_initrd(machine, kernel_entry); diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c index 85ffdac5be..64d5d435b9 100644 --- a/hw/riscv/opentitan.c +++ b/hw/riscv/opentitan.c @@ -101,8 +101,7 @@ static void opentitan_board_init(MachineState *machine) } if (machine->kernel_filename) { - riscv_load_kernel(machine->kernel_filename, - memmap[IBEX_DEV_RAM].base, NULL); + riscv_load_kernel(machine, memmap[IBEX_DEV_RAM].base, NULL); } } diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c index d65d2fd869..3e3f4b0088 100644 --- a/hw/riscv/sifive_e.c +++ b/hw/riscv/sifive_e.c @@ -114,8 +114,7 @@ static void sifive_e_machine_init(MachineState *machine) memmap[SIFIVE_E_DEV_MROM].base, &address_space_memory); if (machine->kernel_filename) { - riscv_load_kernel(machine->kernel_filename, - memmap[SIFIVE_E_DEV_DTIM].base, NULL); + riscv_load_kernel(machine, memmap[SIFIVE_E_DEV_DTIM].base, NULL); } } diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c index c40885ed5c..bac394c959 100644 --- a/hw/riscv/sifive_u.c +++ b/hw/riscv/sifive_u.c @@ -598,8 +598,7 @@ static void sifive_u_machine_init(MachineState *machine) kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc.u_cpus, firmware_end_addr); - kernel_entry = riscv_load_kernel(machine->kernel_filename, - kernel_start_addr, NULL); + kernel_entry = riscv_load_kernel(machine, kernel_start_addr, NULL); if (machine->initrd_filename) { riscv_load_initrd(machine, kernel_entry); diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c index 99dec74fe8..bff9475686 100644 --- a/hw/riscv/spike.c +++ b/hw/riscv/spike.c @@ -307,8 +307,7 @@ static void spike_board_init(MachineState *machine) kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc[0], firmware_end_addr); - kernel_entry = riscv_load_kernel(machine->kernel_filename, - kernel_start_addr, + kernel_entry = riscv_load_kernel(machine, kernel_start_addr, htif_symbol_callback); if (machine->initrd_filename) { diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c index 02f1369843..c8e35f861e 100644 --- a/hw/riscv/virt.c +++ b/hw/riscv/virt.c @@ -1281,8 +1281,7 @@ static void virt_machine_done(Notifier *notifier, void *data) kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc[0], firmware_end_addr); - kernel_entry = riscv_load_kernel(machine->kernel_filename, - kernel_start_addr, NULL); + kernel_entry = riscv_load_kernel(machine, kernel_start_addr, NULL); if (machine->initrd_filename) { riscv_load_initrd(machine, kernel_entry); diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h index cfd72ecabf..f94653a09b 100644 --- a/include/hw/riscv/boot.h +++ b/include/hw/riscv/boot.h @@ -43,7 +43,7 @@ char *riscv_find_firmware(const char *firmware_filename, target_ulong riscv_load_firmware(const char *firmware_filename, hwaddr firmware_load_addr, symbol_fn_t sym_cb); -target_ulong riscv_load_kernel(const char *kernel_filename, +target_ulong riscv_load_kernel(MachineState *machine, target_ulong firmware_end_addr, symbol_fn_t sym_cb); void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry); From c66ffcd5358ba88e93e1ffb15ae42ca52dab12a8 Mon Sep 17 00:00:00 2001 From: Daniel Henrique Barboza Date: Fri, 13 Jan 2023 14:52:29 -0300 Subject: [PATCH 23/37] target/riscv/cpu: set cpu->cfg in register_cpu_props() There is an informal contract between the cpu_init() functions and riscv_cpu_realize(): if cpu->env.misa_ext is zero, assume that the default settings were loaded via register_cpu_props() and do validations to set env.misa_ext. If it's not zero, skip this whole process and assume that the board somehow did everything. At this moment, all SiFive CPUs are setting a non-zero misa_ext during their cpu_init() and skipping a good chunk of riscv_cpu_realize(). This causes problems when the code being skipped in riscv_cpu_realize() contains fixes or assumptions that affects all CPUs, meaning that SiFive CPUs are missing out. To allow this code to not be skipped anymore, all the cpu->cfg.ext_* attributes needs to be set during cpu_init() time. At this moment this is being done in register_cpu_props(). The SiFive boards are setting their own extensions during cpu_init() though, meaning that they don't want all the defaults from register_cpu_props(). Let's move the contract between *_cpu_init() and riscv_cpu_realize() to register_cpu_props(). Inside this function we'll check if cpu->env.misa_ext was set and, if that's the case, set all relevant cpu->cfg.ext_* attributes, and only that. Leave the 'misa_ext' = 0 case as is today, i.e. loading all the defaults from riscv_cpu_extensions[]. register_cpu_props() can then be called by all the cpu_init() functions, including the SiFive ones. This will make all CPUs behave more in line with what riscv_cpu_realize() expects. This will also make the cpu_init() functions even more alike, but at this moment we would need some design changes in how we're initializing extensions/attributes (e.g. some CPUs are setting cfg options after register_cpu_props(), so we can't simply add the function to a common post_init() hook) to make a common cpu_init() code across all CPUs. Signed-off-by: Daniel Henrique Barboza Reviewed-by: Alistair Francis Message-Id: <20230113175230.473975-2-dbarboza@ventanamicro.com> Signed-off-by: Alistair Francis --- target/riscv/cpu.c | 40 ++++++++++++++++++++++++++++++++++++++++ target/riscv/cpu.h | 4 ++++ 2 files changed, 44 insertions(+) diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c index a2e6238bd7..e682102c2a 100644 --- a/target/riscv/cpu.c +++ b/target/riscv/cpu.c @@ -256,6 +256,7 @@ static void rv64_sifive_u_cpu_init(Object *obj) { CPURISCVState *env = &RISCV_CPU(obj)->env; set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU); + register_cpu_props(DEVICE(obj)); set_priv_version(env, PRIV_VERSION_1_10_0); } @@ -265,6 +266,7 @@ static void rv64_sifive_e_cpu_init(Object *obj) RISCVCPU *cpu = RISCV_CPU(obj); set_misa(env, MXL_RV64, RVI | RVM | RVA | RVC | RVU); + register_cpu_props(DEVICE(obj)); set_priv_version(env, PRIV_VERSION_1_10_0); cpu->cfg.mmu = false; } @@ -299,6 +301,7 @@ static void rv32_sifive_u_cpu_init(Object *obj) { CPURISCVState *env = &RISCV_CPU(obj)->env; set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU); + register_cpu_props(DEVICE(obj)); set_priv_version(env, PRIV_VERSION_1_10_0); } @@ -308,6 +311,7 @@ static void rv32_sifive_e_cpu_init(Object *obj) RISCVCPU *cpu = RISCV_CPU(obj); set_misa(env, MXL_RV32, RVI | RVM | RVA | RVC | RVU); + register_cpu_props(DEVICE(obj)); set_priv_version(env, PRIV_VERSION_1_10_0); cpu->cfg.mmu = false; } @@ -318,6 +322,7 @@ static void rv32_ibex_cpu_init(Object *obj) RISCVCPU *cpu = RISCV_CPU(obj); set_misa(env, MXL_RV32, RVI | RVM | RVC | RVU); + register_cpu_props(DEVICE(obj)); set_priv_version(env, PRIV_VERSION_1_11_0); cpu->cfg.mmu = false; cpu->cfg.epmp = true; @@ -329,6 +334,7 @@ static void rv32_imafcu_nommu_cpu_init(Object *obj) RISCVCPU *cpu = RISCV_CPU(obj); set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVC | RVU); + register_cpu_props(DEVICE(obj)); set_priv_version(env, PRIV_VERSION_1_10_0); cpu->cfg.mmu = false; } @@ -1083,10 +1089,44 @@ static Property riscv_cpu_extensions[] = { DEFINE_PROP_END_OF_LIST(), }; +/* + * Register CPU props based on env.misa_ext. If a non-zero + * value was set, register only the required cpu->cfg.ext_* + * properties and leave. env.misa_ext = 0 means that we want + * all the default properties to be registered. + */ static void register_cpu_props(DeviceState *dev) { + RISCVCPU *cpu = RISCV_CPU(OBJECT(dev)); + uint32_t misa_ext = cpu->env.misa_ext; Property *prop; + /* + * If misa_ext is not zero, set cfg properties now to + * allow them to be read during riscv_cpu_realize() + * later on. + */ + if (cpu->env.misa_ext != 0) { + cpu->cfg.ext_i = misa_ext & RVI; + cpu->cfg.ext_e = misa_ext & RVE; + cpu->cfg.ext_m = misa_ext & RVM; + cpu->cfg.ext_a = misa_ext & RVA; + cpu->cfg.ext_f = misa_ext & RVF; + cpu->cfg.ext_d = misa_ext & RVD; + cpu->cfg.ext_v = misa_ext & RVV; + cpu->cfg.ext_c = misa_ext & RVC; + cpu->cfg.ext_s = misa_ext & RVS; + cpu->cfg.ext_u = misa_ext & RVU; + cpu->cfg.ext_h = misa_ext & RVH; + cpu->cfg.ext_j = misa_ext & RVJ; + + /* + * We don't want to set the default riscv_cpu_extensions + * in this case. + */ + return; + } + for (prop = riscv_cpu_extensions; prop && prop->name; prop++) { qdev_property_add_static(dev, prop); } diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h index 61a9a40958..bcf0826753 100644 --- a/target/riscv/cpu.h +++ b/target/riscv/cpu.h @@ -63,6 +63,10 @@ #define RV(x) ((target_ulong)1 << (x - 'A')) +/* + * Consider updating register_cpu_props() when adding + * new MISA bits here. + */ #define RVI RV('I') #define RVE RV('E') /* E and I are mutually exclusive */ #define RVM RV('M') From 5ab1095213318effd9bb4667f7f52da21f81acc6 Mon Sep 17 00:00:00 2001 From: Daniel Henrique Barboza Date: Fri, 13 Jan 2023 14:52:30 -0300 Subject: [PATCH 24/37] target/riscv/cpu.c: do not skip misa logic in riscv_cpu_realize() All RISCV CPUs are setting cpu->cfg during their cpu_init() functions, meaning that there's no reason to skip all the misa validation and setup if misa_ext was set beforehand - especially since we're setting an updated value in set_misa() in the end. Put this code chunk into a new riscv_cpu_validate_set_extensions() helper and always execute it regardless of what the board set in env->misa_ext. This will put more responsibility in how each board is going to init their attributes and extensions if they're not using the defaults. It'll also allow realize() to do its job looking only at the extensions enabled per se, not corner cases that some CPUs might have, and we won't have to change multiple code paths to fix or change how extensions work. Signed-off-by: Daniel Henrique Barboza Reviewed-by: Alistair Francis Reviewed-by: Bin Meng Message-Id: <20230113175230.473975-3-dbarboza@ventanamicro.com> [ Changes by AF: - Rebase ] Signed-off-by: Alistair Francis --- target/riscv/cpu.c | 399 +++++++++++++++++++++++---------------------- 1 file changed, 205 insertions(+), 194 deletions(-) diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c index e682102c2a..c192d96a94 100644 --- a/target/riscv/cpu.c +++ b/target/riscv/cpu.c @@ -625,6 +625,207 @@ static void riscv_cpu_disas_set_info(CPUState *s, disassemble_info *info) } } +/* + * Check consistency between chosen extensions while setting + * cpu->cfg accordingly, doing a set_misa() in the end. + */ +static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp) +{ + CPURISCVState *env = &cpu->env; + uint32_t ext = 0; + + /* Do some ISA extension error checking */ + if (cpu->cfg.ext_g && !(cpu->cfg.ext_i && cpu->cfg.ext_m && + cpu->cfg.ext_a && cpu->cfg.ext_f && + cpu->cfg.ext_d && + cpu->cfg.ext_icsr && cpu->cfg.ext_ifencei)) { + warn_report("Setting G will also set IMAFD_Zicsr_Zifencei"); + cpu->cfg.ext_i = true; + cpu->cfg.ext_m = true; + cpu->cfg.ext_a = true; + cpu->cfg.ext_f = true; + cpu->cfg.ext_d = true; + cpu->cfg.ext_icsr = true; + cpu->cfg.ext_ifencei = true; + } + + if (cpu->cfg.ext_i && cpu->cfg.ext_e) { + error_setg(errp, + "I and E extensions are incompatible"); + return; + } + + if (!cpu->cfg.ext_i && !cpu->cfg.ext_e) { + error_setg(errp, + "Either I or E extension must be set"); + return; + } + + if (cpu->cfg.ext_s && !cpu->cfg.ext_u) { + error_setg(errp, + "Setting S extension without U extension is illegal"); + return; + } + + if (cpu->cfg.ext_h && !cpu->cfg.ext_i) { + error_setg(errp, + "H depends on an I base integer ISA with 32 x registers"); + return; + } + + if (cpu->cfg.ext_h && !cpu->cfg.ext_s) { + error_setg(errp, "H extension implicitly requires S-mode"); + return; + } + + if (cpu->cfg.ext_f && !cpu->cfg.ext_icsr) { + error_setg(errp, "F extension requires Zicsr"); + return; + } + + if ((cpu->cfg.ext_zawrs) && !cpu->cfg.ext_a) { + error_setg(errp, "Zawrs extension requires A extension"); + return; + } + + if ((cpu->cfg.ext_zfh || cpu->cfg.ext_zfhmin) && !cpu->cfg.ext_f) { + error_setg(errp, "Zfh/Zfhmin extensions require F extension"); + return; + } + + if (cpu->cfg.ext_d && !cpu->cfg.ext_f) { + error_setg(errp, "D extension requires F extension"); + return; + } + + if (cpu->cfg.ext_v && !cpu->cfg.ext_d) { + error_setg(errp, "V extension requires D extension"); + return; + } + + if ((cpu->cfg.ext_zve32f || cpu->cfg.ext_zve64f) && !cpu->cfg.ext_f) { + error_setg(errp, "Zve32f/Zve64f extensions require F extension"); + return; + } + + /* Set the ISA extensions, checks should have happened above */ + if (cpu->cfg.ext_zdinx || cpu->cfg.ext_zhinx || + cpu->cfg.ext_zhinxmin) { + cpu->cfg.ext_zfinx = true; + } + + if (cpu->cfg.ext_zfinx) { + if (!cpu->cfg.ext_icsr) { + error_setg(errp, "Zfinx extension requires Zicsr"); + return; + } + if (cpu->cfg.ext_f) { + error_setg(errp, + "Zfinx cannot be supported together with F extension"); + return; + } + } + + if (cpu->cfg.ext_zk) { + cpu->cfg.ext_zkn = true; + cpu->cfg.ext_zkr = true; + cpu->cfg.ext_zkt = true; + } + + if (cpu->cfg.ext_zkn) { + cpu->cfg.ext_zbkb = true; + cpu->cfg.ext_zbkc = true; + cpu->cfg.ext_zbkx = true; + cpu->cfg.ext_zkne = true; + cpu->cfg.ext_zknd = true; + cpu->cfg.ext_zknh = true; + } + + if (cpu->cfg.ext_zks) { + cpu->cfg.ext_zbkb = true; + cpu->cfg.ext_zbkc = true; + cpu->cfg.ext_zbkx = true; + cpu->cfg.ext_zksed = true; + cpu->cfg.ext_zksh = true; + } + + if (cpu->cfg.ext_i) { + ext |= RVI; + } + if (cpu->cfg.ext_e) { + ext |= RVE; + } + if (cpu->cfg.ext_m) { + ext |= RVM; + } + if (cpu->cfg.ext_a) { + ext |= RVA; + } + if (cpu->cfg.ext_f) { + ext |= RVF; + } + if (cpu->cfg.ext_d) { + ext |= RVD; + } + if (cpu->cfg.ext_c) { + ext |= RVC; + } + if (cpu->cfg.ext_s) { + ext |= RVS; + } + if (cpu->cfg.ext_u) { + ext |= RVU; + } + if (cpu->cfg.ext_h) { + ext |= RVH; + } + if (cpu->cfg.ext_v) { + int vext_version = VEXT_VERSION_1_00_0; + ext |= RVV; + if (!is_power_of_2(cpu->cfg.vlen)) { + error_setg(errp, + "Vector extension VLEN must be power of 2"); + return; + } + if (cpu->cfg.vlen > RV_VLEN_MAX || cpu->cfg.vlen < 128) { + error_setg(errp, + "Vector extension implementation only supports VLEN " + "in the range [128, %d]", RV_VLEN_MAX); + return; + } + if (!is_power_of_2(cpu->cfg.elen)) { + error_setg(errp, + "Vector extension ELEN must be power of 2"); + return; + } + if (cpu->cfg.elen > 64 || cpu->cfg.elen < 8) { + error_setg(errp, + "Vector extension implementation only supports ELEN " + "in the range [8, 64]"); + return; + } + if (cpu->cfg.vext_spec) { + if (!g_strcmp0(cpu->cfg.vext_spec, "v1.0")) { + vext_version = VEXT_VERSION_1_00_0; + } else { + error_setg(errp, + "Unsupported vector spec version '%s'", + cpu->cfg.vext_spec); + return; + } + } else { + qemu_log("vector version is not specified, " + "use the default value v1.0\n"); + } + set_vext_version(env, vext_version); + } + if (cpu->cfg.ext_j) { + ext |= RVJ; + } + + set_misa(env, env->misa_mxl, ext); +} + static void riscv_cpu_realize(DeviceState *dev, Error **errp) { CPUState *cs = CPU(dev); @@ -720,200 +921,10 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp) } assert(env->misa_mxl_max == env->misa_mxl); - /* If only MISA_EXT is unset for misa, then set it from properties */ - if (env->misa_ext == 0) { - uint32_t ext = 0; - - /* Do some ISA extension error checking */ - if (cpu->cfg.ext_g && !(cpu->cfg.ext_i && cpu->cfg.ext_m && - cpu->cfg.ext_a && cpu->cfg.ext_f && - cpu->cfg.ext_d && - cpu->cfg.ext_icsr && cpu->cfg.ext_ifencei)) { - warn_report("Setting G will also set IMAFD_Zicsr_Zifencei"); - cpu->cfg.ext_i = true; - cpu->cfg.ext_m = true; - cpu->cfg.ext_a = true; - cpu->cfg.ext_f = true; - cpu->cfg.ext_d = true; - cpu->cfg.ext_icsr = true; - cpu->cfg.ext_ifencei = true; - } - - if (cpu->cfg.ext_i && cpu->cfg.ext_e) { - error_setg(errp, - "I and E extensions are incompatible"); - return; - } - - if (!cpu->cfg.ext_i && !cpu->cfg.ext_e) { - error_setg(errp, - "Either I or E extension must be set"); - return; - } - - if (cpu->cfg.ext_s && !cpu->cfg.ext_u) { - error_setg(errp, - "Setting S extension without U extension is illegal"); - return; - } - - if (cpu->cfg.ext_h && !cpu->cfg.ext_i) { - error_setg(errp, - "H depends on an I base integer ISA with 32 x registers"); - return; - } - - if (cpu->cfg.ext_h && !cpu->cfg.ext_s) { - error_setg(errp, "H extension implicitly requires S-mode"); - return; - } - - if (cpu->cfg.ext_f && !cpu->cfg.ext_icsr) { - error_setg(errp, "F extension requires Zicsr"); - return; - } - - if ((cpu->cfg.ext_zawrs) && !cpu->cfg.ext_a) { - error_setg(errp, "Zawrs extension requires A extension"); - return; - } - - if ((cpu->cfg.ext_zfh || cpu->cfg.ext_zfhmin) && !cpu->cfg.ext_f) { - error_setg(errp, "Zfh/Zfhmin extensions require F extension"); - return; - } - - if (cpu->cfg.ext_d && !cpu->cfg.ext_f) { - error_setg(errp, "D extension requires F extension"); - return; - } - - if (cpu->cfg.ext_v && !cpu->cfg.ext_d) { - error_setg(errp, "V extension requires D extension"); - return; - } - - if ((cpu->cfg.ext_zve32f || cpu->cfg.ext_zve64f) && !cpu->cfg.ext_f) { - error_setg(errp, "Zve32f/Zve64f extensions require F extension"); - return; - } - - /* Set the ISA extensions, checks should have happened above */ - if (cpu->cfg.ext_zdinx || cpu->cfg.ext_zhinx || - cpu->cfg.ext_zhinxmin) { - cpu->cfg.ext_zfinx = true; - } - - if (cpu->cfg.ext_zfinx) { - if (!cpu->cfg.ext_icsr) { - error_setg(errp, "Zfinx extension requires Zicsr"); - return; - } - if (cpu->cfg.ext_f) { - error_setg(errp, - "Zfinx cannot be supported together with F extension"); - return; - } - } - - if (cpu->cfg.ext_zk) { - cpu->cfg.ext_zkn = true; - cpu->cfg.ext_zkr = true; - cpu->cfg.ext_zkt = true; - } - - if (cpu->cfg.ext_zkn) { - cpu->cfg.ext_zbkb = true; - cpu->cfg.ext_zbkc = true; - cpu->cfg.ext_zbkx = true; - cpu->cfg.ext_zkne = true; - cpu->cfg.ext_zknd = true; - cpu->cfg.ext_zknh = true; - } - - if (cpu->cfg.ext_zks) { - cpu->cfg.ext_zbkb = true; - cpu->cfg.ext_zbkc = true; - cpu->cfg.ext_zbkx = true; - cpu->cfg.ext_zksed = true; - cpu->cfg.ext_zksh = true; - } - - if (cpu->cfg.ext_i) { - ext |= RVI; - } - if (cpu->cfg.ext_e) { - ext |= RVE; - } - if (cpu->cfg.ext_m) { - ext |= RVM; - } - if (cpu->cfg.ext_a) { - ext |= RVA; - } - if (cpu->cfg.ext_f) { - ext |= RVF; - } - if (cpu->cfg.ext_d) { - ext |= RVD; - } - if (cpu->cfg.ext_c) { - ext |= RVC; - } - if (cpu->cfg.ext_s) { - ext |= RVS; - } - if (cpu->cfg.ext_u) { - ext |= RVU; - } - if (cpu->cfg.ext_h) { - ext |= RVH; - } - if (cpu->cfg.ext_v) { - int vext_version = VEXT_VERSION_1_00_0; - ext |= RVV; - if (!is_power_of_2(cpu->cfg.vlen)) { - error_setg(errp, - "Vector extension VLEN must be power of 2"); - return; - } - if (cpu->cfg.vlen > RV_VLEN_MAX || cpu->cfg.vlen < 128) { - error_setg(errp, - "Vector extension implementation only supports VLEN " - "in the range [128, %d]", RV_VLEN_MAX); - return; - } - if (!is_power_of_2(cpu->cfg.elen)) { - error_setg(errp, - "Vector extension ELEN must be power of 2"); - return; - } - if (cpu->cfg.elen > 64 || cpu->cfg.elen < 8) { - error_setg(errp, - "Vector extension implementation only supports ELEN " - "in the range [8, 64]"); - return; - } - if (cpu->cfg.vext_spec) { - if (!g_strcmp0(cpu->cfg.vext_spec, "v1.0")) { - vext_version = VEXT_VERSION_1_00_0; - } else { - error_setg(errp, - "Unsupported vector spec version '%s'", - cpu->cfg.vext_spec); - return; - } - } else { - qemu_log("vector version is not specified, " - "use the default value v1.0\n"); - } - set_vext_version(env, vext_version); - } - if (cpu->cfg.ext_j) { - ext |= RVJ; - } - - set_misa(env, env->misa_mxl, ext); + riscv_cpu_validate_set_extensions(cpu, &local_err); + if (local_err != NULL) { + error_propagate(errp, local_err); + return; } #ifndef CONFIG_USER_ONLY From 877a3a3732dcd45b09b96a6ff9655f6a2e19540f Mon Sep 17 00:00:00 2001 From: Bin Meng Date: Mon, 9 Jan 2023 23:26:55 +0800 Subject: [PATCH 25/37] target/riscv: Use TARGET_FMT_lx for env->mhartid env->mhartid is currently casted to long before printed, which drops the high 32-bit for rv64 on 32-bit host. Use TARGET_FMT_lx instead. Signed-off-by: Bin Meng Reviewed-by: Alistair Francis Message-Id: <20230109152655.340114-1-bmeng@tinylab.org> Signed-off-by: Alistair Francis --- target/riscv/cpu.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c index c192d96a94..14a7027095 100644 --- a/target/riscv/cpu.c +++ b/target/riscv/cpu.c @@ -867,9 +867,9 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp) (env->priv_ver < isa_edata_arr[i].min_version)) { isa_ext_update_enabled(cpu, &isa_edata_arr[i], false); #ifndef CONFIG_USER_ONLY - warn_report("disabling %s extension for hart 0x%lx because " - "privilege spec version does not match", - isa_edata_arr[i].name, (unsigned long)env->mhartid); + warn_report("disabling %s extension for hart 0x" TARGET_FMT_lx + " because privilege spec version does not match", + isa_edata_arr[i].name, env->mhartid); #else warn_report("disabling %s extension because " "privilege spec version does not match", From 5dfe23774de34700337d66fc25b6313b65c34ad7 Mon Sep 17 00:00:00 2001 From: Daniel Henrique Barboza Date: Wed, 11 Jan 2023 14:09:39 -0300 Subject: [PATCH 26/37] hw/riscv/spike.c: simplify create_fdt() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 'mem_size' and 'cmdline' are unused. Signed-off-by: Daniel Henrique Barboza Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Alistair Francis Message-Id: <20230111170948.316276-2-dbarboza@ventanamicro.com> Signed-off-by: Alistair Francis --- hw/riscv/spike.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c index bff9475686..c7550abfc7 100644 --- a/hw/riscv/spike.c +++ b/hw/riscv/spike.c @@ -49,7 +49,6 @@ 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, bool htif_custom_base) { void *fdt; @@ -299,8 +298,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]), htif_custom_base); + create_fdt(s, memmap, riscv_is_32bit(&s->soc[0]), htif_custom_base); /* Load kernel */ if (machine->kernel_filename) { From cdb785683ada2055f8f47ae154d0ce43f97d7a87 Mon Sep 17 00:00:00 2001 From: Daniel Henrique Barboza Date: Wed, 11 Jan 2023 14:09:40 -0300 Subject: [PATCH 27/37] hw/riscv/virt.c: simplify create_fdt() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 'mem_size' and 'cmdline' aren't being used. Remove them. Signed-off-by: Daniel Henrique Barboza Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Alistair Francis Message-Id: <20230111170948.316276-3-dbarboza@ventanamicro.com> Signed-off-by: Alistair Francis --- hw/riscv/virt.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c index c8e35f861e..1921d3caa3 100644 --- a/hw/riscv/virt.c +++ b/hw/riscv/virt.c @@ -999,7 +999,7 @@ static void create_fdt_fw_cfg(RISCVVirtState *s, const MemMapEntry *memmap) } static void create_fdt(RISCVVirtState *s, const MemMapEntry *memmap, - uint64_t mem_size, const char *cmdline, bool is_32_bit) + bool is_32_bit) { MachineState *mc = MACHINE(s); uint32_t phandle = 1, irq_mmio_phandle = 1, msi_pcie_phandle = 1; @@ -1507,8 +1507,7 @@ static void virt_machine_init(MachineState *machine) virt_flash_map(s, system_memory); /* create device tree */ - create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline, - riscv_is_32bit(&s->soc[0])); + create_fdt(s, memmap, riscv_is_32bit(&s->soc[0])); s->machine_done.notify = virt_machine_done; qemu_add_machine_init_done_notifier(&s->machine_done); From f5be2ccb43d3d8466f15f53b82f5fdba1684fc56 Mon Sep 17 00:00:00 2001 From: Daniel Henrique Barboza Date: Wed, 11 Jan 2023 14:09:41 -0300 Subject: [PATCH 28/37] hw/riscv/sifive_u.c: simplify create_fdt() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 'cmdline' isn't being used. Remove it. A MachineState pointer is being retrieved via a MACHINE() macro calling qdev_get_machine(). Use MACHINE(s) instead to avoid calling qdev(). 'mem_size' is being set as machine->ram_size by the caller. Retrieve it via ms->ram_size. Cc: Palmer Dabbelt Signed-off-by: Daniel Henrique Barboza Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Alistair Francis Message-Id: <20230111170948.316276-4-dbarboza@ventanamicro.com> Signed-off-by: Alistair Francis --- hw/riscv/sifive_u.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c index bac394c959..2fb6ee231f 100644 --- a/hw/riscv/sifive_u.c +++ b/hw/riscv/sifive_u.c @@ -94,9 +94,10 @@ static const MemMapEntry sifive_u_memmap[] = { #define GEM_REVISION 0x10070109 static void create_fdt(SiFiveUState *s, const MemMapEntry *memmap, - uint64_t mem_size, const char *cmdline, bool is_32_bit) + bool is_32_bit) { - MachineState *ms = MACHINE(qdev_get_machine()); + MachineState *ms = MACHINE(s); + uint64_t mem_size = ms->ram_size; void *fdt; int cpu, fdt_size; uint32_t *cells; @@ -560,8 +561,7 @@ static void sifive_u_machine_init(MachineState *machine) qemu_allocate_irq(sifive_u_machine_reset, NULL, 0)); /* create device tree */ - create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline, - riscv_is_32bit(&s->soc.u_cpus)); + create_fdt(s, memmap, riscv_is_32bit(&s->soc.u_cpus)); if (s->start_in_flash) { /* From 914c97f968cc70be5275fd230d38f99882896032 Mon Sep 17 00:00:00 2001 From: Daniel Henrique Barboza Date: Wed, 11 Jan 2023 14:09:42 -0300 Subject: [PATCH 29/37] hw/riscv/virt.c: remove 'is_32_bit' param from create_fdt_socket_cpus() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit create_fdt_socket_cpus() writes a different 'mmu-type' value if we're running in 32 or 64 bits. However, the flag is being calculated during virt_machine_init(), and is passed around in create_fdt(), then create_fdt_socket(), and then finally create_fdt_socket_cpus(). None of the intermediate functions are using the flag, which is a bit misleading. Remove 'is_32_bit' flag from create_fdt_socket_cpus() and calculate it using the already available RISCVVirtState pointer. This will also change the signature of create_fdt_socket() and create_fdt(), making it clear that these functions don't do anything special when we're running in 32 bit mode. Signed-off-by: Daniel Henrique Barboza Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Alistair Francis Message-Id: <20230111170948.316276-5-dbarboza@ventanamicro.com> Signed-off-by: Alistair Francis --- hw/riscv/virt.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c index 1921d3caa3..99cb571024 100644 --- a/hw/riscv/virt.c +++ b/hw/riscv/virt.c @@ -223,12 +223,13 @@ static void create_pcie_irq_map(RISCVVirtState *s, void *fdt, char *nodename, static void create_fdt_socket_cpus(RISCVVirtState *s, int socket, char *clust_name, uint32_t *phandle, - bool is_32_bit, uint32_t *intc_phandles) + uint32_t *intc_phandles) { int cpu; uint32_t cpu_phandle; MachineState *mc = MACHINE(s); char *name, *cpu_name, *core_name, *intc_name; + bool is_32_bit = riscv_is_32bit(&s->soc[0]); for (cpu = s->soc[socket].num_harts - 1; cpu >= 0; cpu--) { cpu_phandle = (*phandle)++; @@ -721,7 +722,7 @@ static void create_fdt_pmu(RISCVVirtState *s) } static void create_fdt_sockets(RISCVVirtState *s, const MemMapEntry *memmap, - bool is_32_bit, uint32_t *phandle, + uint32_t *phandle, uint32_t *irq_mmio_phandle, uint32_t *irq_pcie_phandle, uint32_t *irq_virtio_phandle, @@ -750,7 +751,7 @@ static void create_fdt_sockets(RISCVVirtState *s, const MemMapEntry *memmap, qemu_fdt_add_subnode(mc->fdt, clust_name); create_fdt_socket_cpus(s, socket, clust_name, phandle, - is_32_bit, &intc_phandles[phandle_pos]); + &intc_phandles[phandle_pos]); create_fdt_socket_memory(s, memmap, socket); @@ -998,8 +999,7 @@ static void create_fdt_fw_cfg(RISCVVirtState *s, const MemMapEntry *memmap) g_free(nodename); } -static void create_fdt(RISCVVirtState *s, const MemMapEntry *memmap, - bool is_32_bit) +static void create_fdt(RISCVVirtState *s, const MemMapEntry *memmap) { MachineState *mc = MACHINE(s); uint32_t phandle = 1, irq_mmio_phandle = 1, msi_pcie_phandle = 1; @@ -1031,9 +1031,9 @@ static void create_fdt(RISCVVirtState *s, const MemMapEntry *memmap, qemu_fdt_setprop_cell(mc->fdt, "/soc", "#size-cells", 0x2); qemu_fdt_setprop_cell(mc->fdt, "/soc", "#address-cells", 0x2); - create_fdt_sockets(s, memmap, is_32_bit, &phandle, - &irq_mmio_phandle, &irq_pcie_phandle, &irq_virtio_phandle, - &msi_pcie_phandle); + create_fdt_sockets(s, memmap, &phandle, &irq_mmio_phandle, + &irq_pcie_phandle, &irq_virtio_phandle, + &msi_pcie_phandle); create_fdt_virtio(s, memmap, irq_virtio_phandle); @@ -1507,7 +1507,7 @@ static void virt_machine_init(MachineState *machine) virt_flash_map(s, system_memory); /* create device tree */ - create_fdt(s, memmap, riscv_is_32bit(&s->soc[0])); + create_fdt(s, memmap); s->machine_done.notify = virt_machine_done; qemu_add_machine_init_done_notifier(&s->machine_done); From fb60b488cf5027c8134f1ce0c1df9b6bdd3b9276 Mon Sep 17 00:00:00 2001 From: Daniel Henrique Barboza Date: Wed, 11 Jan 2023 14:09:43 -0300 Subject: [PATCH 30/37] hw/riscv: use MachineState::fdt in riscv_socket_fdt_write_id() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There's no need to use a MachineState pointer and a fdt pointer now that all RISC-V machines are using the FDT from the MachineState. Signed-off-by: Daniel Henrique Barboza Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Alistair Francis Message-Id: <20230111170948.316276-6-dbarboza@ventanamicro.com> Signed-off-by: Alistair Francis --- hw/riscv/numa.c | 6 +++--- hw/riscv/spike.c | 6 +++--- hw/riscv/virt.c | 18 +++++++++--------- include/hw/riscv/numa.h | 6 +++--- 4 files changed, 18 insertions(+), 18 deletions(-) diff --git a/hw/riscv/numa.c b/hw/riscv/numa.c index 7fe92d402f..f4343f5cde 100644 --- a/hw/riscv/numa.c +++ b/hw/riscv/numa.c @@ -156,11 +156,11 @@ uint64_t riscv_socket_mem_size(const MachineState *ms, int socket_id) ms->numa_state->nodes[socket_id].node_mem : 0; } -void riscv_socket_fdt_write_id(const MachineState *ms, void *fdt, - const char *node_name, int socket_id) +void riscv_socket_fdt_write_id(const MachineState *ms, const char *node_name, + int socket_id) { if (numa_enabled(ms)) { - qemu_fdt_setprop_cell(fdt, node_name, "numa-node-id", socket_id); + qemu_fdt_setprop_cell(ms->fdt, node_name, "numa-node-id", socket_id); } } diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c index c7550abfc7..5f12d80317 100644 --- a/hw/riscv/spike.c +++ b/hw/riscv/spike.c @@ -121,7 +121,7 @@ static void create_fdt(SpikeState *s, const MemMapEntry *memmap, qemu_fdt_setprop_cell(fdt, cpu_name, "reg", s->soc[socket].hartid_base + cpu); qemu_fdt_setprop_string(fdt, cpu_name, "device_type", "cpu"); - riscv_socket_fdt_write_id(mc, fdt, cpu_name, socket); + riscv_socket_fdt_write_id(mc, cpu_name, socket); qemu_fdt_setprop_cell(fdt, cpu_name, "phandle", cpu_phandle); intc_name = g_strdup_printf("%s/interrupt-controller", cpu_name); @@ -154,7 +154,7 @@ static void create_fdt(SpikeState *s, const MemMapEntry *memmap, qemu_fdt_setprop_cells(fdt, mem_name, "reg", addr >> 32, addr, size >> 32, size); qemu_fdt_setprop_string(fdt, mem_name, "device_type", "memory"); - riscv_socket_fdt_write_id(mc, fdt, mem_name, socket); + riscv_socket_fdt_write_id(mc, mem_name, socket); g_free(mem_name); clint_addr = memmap[SPIKE_CLINT].base + @@ -167,7 +167,7 @@ static void create_fdt(SpikeState *s, const MemMapEntry *memmap, 0x0, clint_addr, 0x0, memmap[SPIKE_CLINT].size); qemu_fdt_setprop(fdt, clint_name, "interrupts-extended", clint_cells, s->soc[socket].num_harts * sizeof(uint32_t) * 4); - riscv_socket_fdt_write_id(mc, fdt, clint_name, socket); + riscv_socket_fdt_write_id(mc, clint_name, socket); g_free(clint_name); g_free(clint_cells); diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c index 99cb571024..6a2422a8cf 100644 --- a/hw/riscv/virt.c +++ b/hw/riscv/virt.c @@ -253,7 +253,7 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket, qemu_fdt_setprop_cell(mc->fdt, cpu_name, "reg", s->soc[socket].hartid_base + cpu); qemu_fdt_setprop_string(mc->fdt, cpu_name, "device_type", "cpu"); - riscv_socket_fdt_write_id(mc, mc->fdt, cpu_name, socket); + riscv_socket_fdt_write_id(mc, cpu_name, socket); qemu_fdt_setprop_cell(mc->fdt, cpu_name, "phandle", cpu_phandle); intc_phandles[cpu] = (*phandle)++; @@ -291,7 +291,7 @@ static void create_fdt_socket_memory(RISCVVirtState *s, qemu_fdt_setprop_cells(mc->fdt, mem_name, "reg", addr >> 32, addr, size >> 32, size); qemu_fdt_setprop_string(mc->fdt, mem_name, "device_type", "memory"); - riscv_socket_fdt_write_id(mc, mc->fdt, mem_name, socket); + riscv_socket_fdt_write_id(mc, mem_name, socket); g_free(mem_name); } @@ -327,7 +327,7 @@ static void create_fdt_socket_clint(RISCVVirtState *s, 0x0, clint_addr, 0x0, memmap[VIRT_CLINT].size); qemu_fdt_setprop(mc->fdt, clint_name, "interrupts-extended", clint_cells, s->soc[socket].num_harts * sizeof(uint32_t) * 4); - riscv_socket_fdt_write_id(mc, mc->fdt, clint_name, socket); + riscv_socket_fdt_write_id(mc, clint_name, socket); g_free(clint_name); g_free(clint_cells); @@ -372,7 +372,7 @@ static void create_fdt_socket_aclint(RISCVVirtState *s, aclint_mswi_cells, aclint_cells_size); qemu_fdt_setprop(mc->fdt, name, "interrupt-controller", NULL, 0); qemu_fdt_setprop_cell(mc->fdt, name, "#interrupt-cells", 0); - riscv_socket_fdt_write_id(mc, mc->fdt, name, socket); + riscv_socket_fdt_write_id(mc, name, socket); g_free(name); } @@ -396,7 +396,7 @@ static void create_fdt_socket_aclint(RISCVVirtState *s, 0x0, RISCV_ACLINT_DEFAULT_MTIME); qemu_fdt_setprop(mc->fdt, name, "interrupts-extended", aclint_mtimer_cells, aclint_cells_size); - riscv_socket_fdt_write_id(mc, mc->fdt, name, socket); + riscv_socket_fdt_write_id(mc, name, socket); g_free(name); if (s->aia_type != VIRT_AIA_TYPE_APLIC_IMSIC) { @@ -412,7 +412,7 @@ static void create_fdt_socket_aclint(RISCVVirtState *s, aclint_sswi_cells, aclint_cells_size); qemu_fdt_setprop(mc->fdt, name, "interrupt-controller", NULL, 0); qemu_fdt_setprop_cell(mc->fdt, name, "#interrupt-cells", 0); - riscv_socket_fdt_write_id(mc, mc->fdt, name, socket); + riscv_socket_fdt_write_id(mc, name, socket); g_free(name); } @@ -471,7 +471,7 @@ static void create_fdt_socket_plic(RISCVVirtState *s, 0x0, plic_addr, 0x0, memmap[VIRT_PLIC].size); qemu_fdt_setprop_cell(mc->fdt, plic_name, "riscv,ndev", VIRT_IRQCHIP_NUM_SOURCES - 1); - riscv_socket_fdt_write_id(mc, mc->fdt, plic_name, socket); + riscv_socket_fdt_write_id(mc, plic_name, socket); qemu_fdt_setprop_cell(mc->fdt, plic_name, "phandle", plic_phandles[socket]); @@ -663,7 +663,7 @@ static void create_fdt_socket_aplic(RISCVVirtState *s, aplic_s_phandle); qemu_fdt_setprop_cells(mc->fdt, aplic_name, "riscv,delegate", aplic_s_phandle, 0x1, VIRT_IRQCHIP_NUM_SOURCES); - riscv_socket_fdt_write_id(mc, mc->fdt, aplic_name, socket); + riscv_socket_fdt_write_id(mc, aplic_name, socket); qemu_fdt_setprop_cell(mc->fdt, aplic_name, "phandle", aplic_m_phandle); g_free(aplic_name); @@ -691,7 +691,7 @@ static void create_fdt_socket_aplic(RISCVVirtState *s, 0x0, aplic_addr, 0x0, memmap[VIRT_APLIC_S].size); qemu_fdt_setprop_cell(mc->fdt, aplic_name, "riscv,num-sources", VIRT_IRQCHIP_NUM_SOURCES); - riscv_socket_fdt_write_id(mc, mc->fdt, aplic_name, socket); + riscv_socket_fdt_write_id(mc, aplic_name, socket); qemu_fdt_setprop_cell(mc->fdt, aplic_name, "phandle", aplic_s_phandle); if (!socket) { diff --git a/include/hw/riscv/numa.h b/include/hw/riscv/numa.h index 1a9cce3344..634df6673f 100644 --- a/include/hw/riscv/numa.h +++ b/include/hw/riscv/numa.h @@ -90,10 +90,10 @@ bool riscv_socket_check_hartids(const MachineState *ms, int socket_id); * @ms: pointer to machine state * @socket_id: socket index * - * Write NUMA node-id FDT property for given FDT node + * Write NUMA node-id FDT property in MachineState->fdt */ -void riscv_socket_fdt_write_id(const MachineState *ms, void *fdt, - const char *node_name, int socket_id); +void riscv_socket_fdt_write_id(const MachineState *ms, const char *node_name, + int socket_id); /** * riscv_socket_fdt_write_distance_matrix: From 9c3ee7e84781909d5a114350c35554f0886491ba Mon Sep 17 00:00:00 2001 From: Daniel Henrique Barboza Date: Wed, 11 Jan 2023 14:09:44 -0300 Subject: [PATCH 31/37] hw/riscv: use ms->fdt in riscv_socket_fdt_write_distance_matrix() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There's no need to use a MachineState pointer and a fdt pointer now that all RISC-V machines are using the FDT from the MachineState. Signed-off-by: Daniel Henrique Barboza Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Alistair Francis Message-Id: <20230111170948.316276-7-dbarboza@ventanamicro.com> Signed-off-by: Alistair Francis --- hw/riscv/numa.c | 8 ++++---- hw/riscv/spike.c | 2 +- hw/riscv/virt.c | 2 +- include/hw/riscv/numa.h | 4 ++-- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/hw/riscv/numa.c b/hw/riscv/numa.c index f4343f5cde..4720102561 100644 --- a/hw/riscv/numa.c +++ b/hw/riscv/numa.c @@ -164,7 +164,7 @@ void riscv_socket_fdt_write_id(const MachineState *ms, const char *node_name, } } -void riscv_socket_fdt_write_distance_matrix(const MachineState *ms, void *fdt) +void riscv_socket_fdt_write_distance_matrix(const MachineState *ms) { int i, j, idx; uint32_t *dist_matrix, dist_matrix_size; @@ -184,10 +184,10 @@ void riscv_socket_fdt_write_distance_matrix(const MachineState *ms, void *fdt) } } - qemu_fdt_add_subnode(fdt, "/distance-map"); - qemu_fdt_setprop_string(fdt, "/distance-map", "compatible", + qemu_fdt_add_subnode(ms->fdt, "/distance-map"); + qemu_fdt_setprop_string(ms->fdt, "/distance-map", "compatible", "numa-distance-map-v1"); - qemu_fdt_setprop(fdt, "/distance-map", "distance-matrix", + qemu_fdt_setprop(ms->fdt, "/distance-map", "distance-matrix", dist_matrix, dist_matrix_size); g_free(dist_matrix); } diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c index 5f12d80317..badc11ec43 100644 --- a/hw/riscv/spike.c +++ b/hw/riscv/spike.c @@ -174,7 +174,7 @@ static void create_fdt(SpikeState *s, const MemMapEntry *memmap, g_free(clust_name); } - riscv_socket_fdt_write_distance_matrix(mc, fdt); + riscv_socket_fdt_write_distance_matrix(mc); qemu_fdt_add_subnode(fdt, "/chosen"); qemu_fdt_setprop_string(fdt, "/chosen", "stdout-path", "/htif"); diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c index 6a2422a8cf..e6d4f06e8d 100644 --- a/hw/riscv/virt.c +++ b/hw/riscv/virt.c @@ -805,7 +805,7 @@ static void create_fdt_sockets(RISCVVirtState *s, const MemMapEntry *memmap, } } - riscv_socket_fdt_write_distance_matrix(mc, mc->fdt); + riscv_socket_fdt_write_distance_matrix(mc); } static void create_fdt_virtio(RISCVVirtState *s, const MemMapEntry *memmap, diff --git a/include/hw/riscv/numa.h b/include/hw/riscv/numa.h index 634df6673f..8f5280211d 100644 --- a/include/hw/riscv/numa.h +++ b/include/hw/riscv/numa.h @@ -100,9 +100,9 @@ void riscv_socket_fdt_write_id(const MachineState *ms, const char *node_name, * @ms: pointer to machine state * @socket_id: socket index * - * Write NUMA distance matrix in FDT for given machine + * Write NUMA distance matrix in MachineState->fdt */ -void riscv_socket_fdt_write_distance_matrix(const MachineState *ms, void *fdt); +void riscv_socket_fdt_write_distance_matrix(const MachineState *ms); CpuInstanceProperties riscv_numa_cpu_index_to_props(MachineState *ms, unsigned cpu_index); From 06d85c24c28f42a57680dc21955e343f58d93089 Mon Sep 17 00:00:00 2001 From: Andrew Bresticker Date: Thu, 15 Dec 2022 17:45:40 -0500 Subject: [PATCH 32/37] target/riscv: Fix up masking of vsip/vsie accesses The current logic attempts to shift the VS-level bits into their correct position in mip while leaving the remaining bits in-tact. This is both pointless and likely incorrect since one would expect that any new, future VS-level interrupts will get their own position in mip rather than sharing with their (H)S-level equivalent. Fix this, and make the logic more readable, by just making off the VS-level bits and shifting them into position. This also fixes reads of vsip, which would only ever report vsip.VSSIP since the non-writable bits got masked off as well. Fixes: d028ac7512f1 ("arget/riscv: Implement AIA CSRs for 64 local interrupts on RV32") Signed-off-by: Andrew Bresticker Reviewed-by: Alistair Francis Message-Id: <20221215224541.1423431-1-abrestic@rivosinc.com> Signed-off-by: Alistair Francis --- target/riscv/csr.c | 33 ++++++++++----------------------- 1 file changed, 10 insertions(+), 23 deletions(-) diff --git a/target/riscv/csr.c b/target/riscv/csr.c index 0db2c233e5..270de7b1a8 100644 --- a/target/riscv/csr.c +++ b/target/riscv/csr.c @@ -2305,22 +2305,15 @@ static RISCVException rmw_vsie64(CPURISCVState *env, int csrno, uint64_t new_val, uint64_t wr_mask) { RISCVException ret; - uint64_t rval, vsbits, mask = env->hideleg & VS_MODE_INTERRUPTS; + uint64_t rval, mask = env->hideleg & VS_MODE_INTERRUPTS; /* Bring VS-level bits to correct position */ - vsbits = new_val & (VS_MODE_INTERRUPTS >> 1); - new_val &= ~(VS_MODE_INTERRUPTS >> 1); - new_val |= vsbits << 1; - vsbits = wr_mask & (VS_MODE_INTERRUPTS >> 1); - wr_mask &= ~(VS_MODE_INTERRUPTS >> 1); - wr_mask |= vsbits << 1; + new_val = (new_val & (VS_MODE_INTERRUPTS >> 1)) << 1; + wr_mask = (wr_mask & (VS_MODE_INTERRUPTS >> 1)) << 1; ret = rmw_mie64(env, csrno, &rval, new_val, wr_mask & mask); if (ret_val) { - rval &= mask; - vsbits = rval & VS_MODE_INTERRUPTS; - rval &= ~VS_MODE_INTERRUPTS; - *ret_val = rval | (vsbits >> 1); + *ret_val = (rval & mask) >> 1; } return ret; @@ -2521,22 +2514,16 @@ static RISCVException rmw_vsip64(CPURISCVState *env, int csrno, uint64_t new_val, uint64_t wr_mask) { RISCVException ret; - uint64_t rval, vsbits, mask = env->hideleg & vsip_writable_mask; + uint64_t rval, mask = env->hideleg & VS_MODE_INTERRUPTS; /* Bring VS-level bits to correct position */ - vsbits = new_val & (VS_MODE_INTERRUPTS >> 1); - new_val &= ~(VS_MODE_INTERRUPTS >> 1); - new_val |= vsbits << 1; - vsbits = wr_mask & (VS_MODE_INTERRUPTS >> 1); - wr_mask &= ~(VS_MODE_INTERRUPTS >> 1); - wr_mask |= vsbits << 1; + new_val = (new_val & (VS_MODE_INTERRUPTS >> 1)) << 1; + wr_mask = (wr_mask & (VS_MODE_INTERRUPTS >> 1)) << 1; - ret = rmw_mip64(env, csrno, &rval, new_val, wr_mask & mask); + ret = rmw_mip64(env, csrno, &rval, new_val, + wr_mask & mask & vsip_writable_mask); if (ret_val) { - rval &= mask; - vsbits = rval & VS_MODE_INTERRUPTS; - rval &= ~VS_MODE_INTERRUPTS; - *ret_val = rval | (vsbits >> 1); + *ret_val = (rval & mask) >> 1; } return ret; From e471a8c9850f1af0c1bc5768ca28285348cdd6c5 Mon Sep 17 00:00:00 2001 From: Andrew Bresticker Date: Thu, 15 Dec 2022 17:45:41 -0500 Subject: [PATCH 33/37] target/riscv: Trap on writes to stimecmp from VS when hvictl.VTI=1 Per the AIA specification, writes to stimecmp from VS level should trap when hvictl.VTI is set since the write may cause vsip.STIP to become unset. Fixes: 3ec0fe18a31f ("target/riscv: Add vstimecmp support") Signed-off-by: Andrew Bresticker Reviewed-by: Alistair Francis Message-Id: <20221215224541.1423431-2-abrestic@rivosinc.com> Signed-off-by: Alistair Francis --- target/riscv/csr.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/target/riscv/csr.c b/target/riscv/csr.c index 270de7b1a8..62e6c4acbd 100644 --- a/target/riscv/csr.c +++ b/target/riscv/csr.c @@ -1037,6 +1037,9 @@ static RISCVException write_stimecmp(CPURISCVState *env, int csrno, RISCVCPU *cpu = env_archcpu(env); if (riscv_cpu_virt_enabled(env)) { + if (env->hvictl & HVICTL_VTI) { + return RISCV_EXCP_VIRT_INSTRUCTION_FAULT; + } return write_vstimecmp(env, csrno, val); } @@ -1057,6 +1060,9 @@ static RISCVException write_stimecmph(CPURISCVState *env, int csrno, RISCVCPU *cpu = env_archcpu(env); if (riscv_cpu_virt_enabled(env)) { + if (env->hvictl & HVICTL_VTI) { + return RISCV_EXCP_VIRT_INSTRUCTION_FAULT; + } return write_vstimecmph(env, csrno, val); } From 9d9db41373a256c5ae011c1428d26d2597a77484 Mon Sep 17 00:00:00 2001 From: Richard Henderson Date: Tue, 17 Jan 2023 13:04:15 -1000 Subject: [PATCH 34/37] tcg/riscv: Use tcg_pcrel_diff in tcg_out_ldst We failed to update this with the w^x split, so misses the fact that true pc-relative offsets are usually small. Signed-off-by: Richard Henderson Reviewed-by: Alistair Francis Message-Id: <20230117230415.354239-1-richard.henderson@linaro.org> Signed-off-by: Alistair Francis --- tcg/riscv/tcg-target.c.inc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tcg/riscv/tcg-target.c.inc b/tcg/riscv/tcg-target.c.inc index fc0edd811f..01cb67ef7b 100644 --- a/tcg/riscv/tcg-target.c.inc +++ b/tcg/riscv/tcg-target.c.inc @@ -599,7 +599,7 @@ static void tcg_out_ldst(TCGContext *s, RISCVInsn opc, TCGReg data, intptr_t imm12 = sextreg(offset, 0, 12); if (offset != imm12) { - intptr_t diff = offset - (uintptr_t)s->code_ptr; + intptr_t diff = tcg_pcrel_diff(s, (void *)offset); if (addr == TCG_REG_ZERO && diff == (int32_t)diff) { imm12 = sextreg(diff, 0, 12); From 3ceeb19a53e51c0c6310d760d26dca08145797c5 Mon Sep 17 00:00:00 2001 From: Richard Henderson Date: Sun, 15 Jan 2023 06:06:56 -1000 Subject: [PATCH 35/37] target/riscv: Introduce helper_set_rounding_mode_chkfrm The new helper always validates the contents of FRM, even if the new rounding mode is not DYN. This is required by the vector unit. Track whether we've validated FRM separately from whether we've updated fp_status with a given rounding mode, so that we can elide calls correctly. This partially reverts d6c4d3f2a69 which attempted the to do the same thing, but with two calls to gen_set_rm(), which is both inefficient and tickles an assertion in decode_save_opc. Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1441 Signed-off-by: Richard Henderson Reviewed-by: Daniel Henrique Barboza Acked-by: Alistair Francis Message-Id: <20230115160657.3169274-2-richard.henderson@linaro.org> Signed-off-by: Alistair Francis --- target/riscv/fpu_helper.c | 37 +++++++++++++++++++++++++ target/riscv/helper.h | 1 + target/riscv/insn_trans/trans_rvv.c.inc | 24 +++------------- target/riscv/translate.c | 19 +++++++++++++ 4 files changed, 61 insertions(+), 20 deletions(-) diff --git a/target/riscv/fpu_helper.c b/target/riscv/fpu_helper.c index 5699c9517f..96817df8ef 100644 --- a/target/riscv/fpu_helper.c +++ b/target/riscv/fpu_helper.c @@ -81,6 +81,43 @@ void helper_set_rounding_mode(CPURISCVState *env, uint32_t rm) set_float_rounding_mode(softrm, &env->fp_status); } +void helper_set_rounding_mode_chkfrm(CPURISCVState *env, uint32_t rm) +{ + int softrm; + + /* Always validate frm, even if rm != DYN. */ + if (unlikely(env->frm >= 5)) { + riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC()); + } + if (rm == RISCV_FRM_DYN) { + rm = env->frm; + } + switch (rm) { + case RISCV_FRM_RNE: + softrm = float_round_nearest_even; + break; + case RISCV_FRM_RTZ: + softrm = float_round_to_zero; + break; + case RISCV_FRM_RDN: + softrm = float_round_down; + break; + case RISCV_FRM_RUP: + softrm = float_round_up; + break; + case RISCV_FRM_RMM: + softrm = float_round_ties_away; + break; + case RISCV_FRM_ROD: + softrm = float_round_to_odd; + break; + default: + g_assert_not_reached(); + } + + set_float_rounding_mode(softrm, &env->fp_status); +} + void helper_set_rod_rounding_mode(CPURISCVState *env) { set_float_rounding_mode(float_round_to_odd, &env->fp_status); diff --git a/target/riscv/helper.h b/target/riscv/helper.h index 227c7122ef..9792ab5086 100644 --- a/target/riscv/helper.h +++ b/target/riscv/helper.h @@ -3,6 +3,7 @@ DEF_HELPER_2(raise_exception, noreturn, env, i32) /* Floating Point - rounding mode */ DEF_HELPER_FLAGS_2(set_rounding_mode, TCG_CALL_NO_WG, void, env, i32) +DEF_HELPER_FLAGS_2(set_rounding_mode_chkfrm, TCG_CALL_NO_WG, void, env, i32) DEF_HELPER_FLAGS_1(set_rod_rounding_mode, TCG_CALL_NO_WG, void, env) /* Floating Point - fused */ diff --git a/target/riscv/insn_trans/trans_rvv.c.inc b/target/riscv/insn_trans/trans_rvv.c.inc index d455acedbf..bbb5c3a7b5 100644 --- a/target/riscv/insn_trans/trans_rvv.c.inc +++ b/target/riscv/insn_trans/trans_rvv.c.inc @@ -2679,13 +2679,9 @@ static bool do_opfv(DisasContext *s, arg_rmr *a, int rm) { if (checkfn(s, a)) { - if (rm != RISCV_FRM_DYN) { - gen_set_rm(s, RISCV_FRM_DYN); - } - uint32_t data = 0; TCGLabel *over = gen_new_label(); - gen_set_rm(s, rm); + gen_set_rm_chkfrm(s, rm); tcg_gen_brcondi_tl(TCG_COND_EQ, cpu_vl, 0, over); tcg_gen_brcond_tl(TCG_COND_GEU, cpu_vstart, cpu_vl, over); @@ -2882,17 +2878,13 @@ static bool opffv_widen_check(DisasContext *s, arg_rmr *a) static bool trans_##NAME(DisasContext *s, arg_rmr *a) \ { \ if (CHECK(s, a)) { \ - if (FRM != RISCV_FRM_DYN) { \ - gen_set_rm(s, RISCV_FRM_DYN); \ - } \ - \ uint32_t data = 0; \ static gen_helper_gvec_3_ptr * const fns[2] = { \ gen_helper_##HELPER##_h, \ gen_helper_##HELPER##_w, \ }; \ TCGLabel *over = gen_new_label(); \ - gen_set_rm(s, FRM); \ + gen_set_rm_chkfrm(s, FRM); \ tcg_gen_brcondi_tl(TCG_COND_EQ, cpu_vl, 0, over); \ tcg_gen_brcond_tl(TCG_COND_GEU, cpu_vstart, cpu_vl, over); \ \ @@ -3005,17 +2997,13 @@ static bool opffv_narrow_check(DisasContext *s, arg_rmr *a) static bool trans_##NAME(DisasContext *s, arg_rmr *a) \ { \ if (CHECK(s, a)) { \ - if (FRM != RISCV_FRM_DYN) { \ - gen_set_rm(s, RISCV_FRM_DYN); \ - } \ - \ uint32_t data = 0; \ static gen_helper_gvec_3_ptr * const fns[2] = { \ gen_helper_##HELPER##_h, \ gen_helper_##HELPER##_w, \ }; \ TCGLabel *over = gen_new_label(); \ - gen_set_rm(s, FRM); \ + gen_set_rm_chkfrm(s, FRM); \ tcg_gen_brcondi_tl(TCG_COND_EQ, cpu_vl, 0, over); \ tcg_gen_brcond_tl(TCG_COND_GEU, cpu_vstart, cpu_vl, over); \ \ @@ -3060,10 +3048,6 @@ static bool opxfv_narrow_check(DisasContext *s, arg_rmr *a) static bool trans_##NAME(DisasContext *s, arg_rmr *a) \ { \ if (opxfv_narrow_check(s, a)) { \ - if (FRM != RISCV_FRM_DYN) { \ - gen_set_rm(s, RISCV_FRM_DYN); \ - } \ - \ uint32_t data = 0; \ static gen_helper_gvec_3_ptr * const fns[3] = { \ gen_helper_##HELPER##_b, \ @@ -3071,7 +3055,7 @@ static bool trans_##NAME(DisasContext *s, arg_rmr *a) \ gen_helper_##HELPER##_w, \ }; \ TCGLabel *over = gen_new_label(); \ - gen_set_rm(s, FRM); \ + gen_set_rm_chkfrm(s, FRM); \ tcg_gen_brcondi_tl(TCG_COND_EQ, cpu_vl, 0, over); \ tcg_gen_brcond_tl(TCG_COND_GEU, cpu_vstart, cpu_vl, over); \ \ diff --git a/target/riscv/translate.c b/target/riscv/translate.c index df38db7553..493c3815e1 100644 --- a/target/riscv/translate.c +++ b/target/riscv/translate.c @@ -114,6 +114,8 @@ typedef struct DisasContext { bool pm_base_enabled; /* Use icount trigger for native debug */ bool itrigger; + /* FRM is known to contain a valid value. */ + bool frm_valid; /* TCG of the current insn_start */ TCGOp *insn_start; } DisasContext; @@ -674,12 +676,29 @@ static void gen_set_rm(DisasContext *ctx, int rm) gen_helper_set_rod_rounding_mode(cpu_env); return; } + if (rm == RISCV_FRM_DYN) { + /* The helper will return only if frm valid. */ + ctx->frm_valid = true; + } /* The helper may raise ILLEGAL_INSN -- record binv for unwind. */ decode_save_opc(ctx); gen_helper_set_rounding_mode(cpu_env, tcg_constant_i32(rm)); } +static void gen_set_rm_chkfrm(DisasContext *ctx, int rm) +{ + if (ctx->frm == rm && ctx->frm_valid) { + return; + } + ctx->frm = rm; + ctx->frm_valid = true; + + /* The helper may raise ILLEGAL_INSN -- record binv for unwind. */ + decode_save_opc(ctx); + gen_helper_set_rounding_mode_chkfrm(cpu_env, tcg_constant_i32(rm)); +} + static int ex_plus_1(DisasContext *ctx, int nf) { return nf + 1; From f251c01a623e0c998a2127f8648d4d02cd04e702 Mon Sep 17 00:00:00 2001 From: Richard Henderson Date: Sun, 15 Jan 2023 06:06:57 -1000 Subject: [PATCH 36/37] target/riscv: Remove helper_set_rod_rounding_mode The only setting of RISCV_FRM_ROD is from the vector unit, and now handled by helper_set_rounding_mode_chkfrm. This helper is now unused. Signed-off-by: Richard Henderson Reviewed-by: Daniel Henrique Barboza Reviewed-by: Alistair Francis Message-Id: <20230115160657.3169274-3-richard.henderson@linaro.org> Signed-off-by: Alistair Francis --- target/riscv/fpu_helper.c | 5 ----- target/riscv/helper.h | 1 - target/riscv/translate.c | 4 ---- 3 files changed, 10 deletions(-) diff --git a/target/riscv/fpu_helper.c b/target/riscv/fpu_helper.c index 96817df8ef..449d236df6 100644 --- a/target/riscv/fpu_helper.c +++ b/target/riscv/fpu_helper.c @@ -118,11 +118,6 @@ void helper_set_rounding_mode_chkfrm(CPURISCVState *env, uint32_t rm) set_float_rounding_mode(softrm, &env->fp_status); } -void helper_set_rod_rounding_mode(CPURISCVState *env) -{ - set_float_rounding_mode(float_round_to_odd, &env->fp_status); -} - static uint64_t do_fmadd_h(CPURISCVState *env, uint64_t rs1, uint64_t rs2, uint64_t rs3, int flags) { diff --git a/target/riscv/helper.h b/target/riscv/helper.h index 9792ab5086..58a30f03d6 100644 --- a/target/riscv/helper.h +++ b/target/riscv/helper.h @@ -4,7 +4,6 @@ DEF_HELPER_2(raise_exception, noreturn, env, i32) /* Floating Point - rounding mode */ DEF_HELPER_FLAGS_2(set_rounding_mode, TCG_CALL_NO_WG, void, env, i32) DEF_HELPER_FLAGS_2(set_rounding_mode_chkfrm, TCG_CALL_NO_WG, void, env, i32) -DEF_HELPER_FLAGS_1(set_rod_rounding_mode, TCG_CALL_NO_WG, void, env) /* Floating Point - fused */ DEF_HELPER_FLAGS_4(fmadd_s, TCG_CALL_NO_RWG, i64, env, i64, i64, i64) diff --git a/target/riscv/translate.c b/target/riscv/translate.c index 493c3815e1..01cc30a365 100644 --- a/target/riscv/translate.c +++ b/target/riscv/translate.c @@ -672,10 +672,6 @@ static void gen_set_rm(DisasContext *ctx, int rm) } ctx->frm = rm; - if (rm == RISCV_FRM_ROD) { - gen_helper_set_rod_rounding_mode(cpu_env); - return; - } if (rm == RISCV_FRM_DYN) { /* The helper will return only if frm valid. */ ctx->frm_valid = true; From b748352c555b42d497fe8ee00ee2e44eb8627660 Mon Sep 17 00:00:00 2001 From: Daniel Henrique Barboza Date: Tue, 17 Jan 2023 10:27:51 -0300 Subject: [PATCH 37/37] hw/riscv/virt.c: move create_fw_cfg() back to virt_machine_init() Commit 1c20d3ff6004 ("hw/riscv: virt: Add a machine done notifier") moved the initialization of fw_cfg to the virt_machine_done() callback. Problem is that the validation of fw_cfg by devices such as ramfb is done before the machine done notifier is called. Moving create_fw_cfg() to machine_done() results in QEMU failing to boot when using a ramfb device: ./qemu-system-riscv64 -machine virt -device ramfb -serial stdio qemu-system-riscv64: -device ramfb: ramfb device requires fw_cfg with DMA The fix is simple: move create_fw_cfg() config back to virt_machine_init(). This happens to be the same way the ARM 'virt' machine deals with fw_cfg (see machvirt_init() and virt_machine_done() in hw/arm/virt.c), so we're keeping consistency with how other machines handle this device. Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1343 Signed-off-by: Daniel Henrique Barboza Reviewed-by: Alistair Francis Message-Id: <20230117132751.229738-2-dbarboza@ventanamicro.com> Signed-off-by: Alistair Francis --- hw/riscv/virt.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c index e6d4f06e8d..4a11b4b010 100644 --- a/hw/riscv/virt.c +++ b/hw/riscv/virt.c @@ -1254,13 +1254,6 @@ static void virt_machine_done(Notifier *notifier, void *data) 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 - * tree cannot be altered and we get FDT_ERR_NOSPACE. - */ - s->fw_cfg = create_fw_cfg(machine); - rom_set_fw(s->fw_cfg); - if (drive_get(IF_PFLASH, 0, 1)) { /* * S-mode FW like EDK2 will be kept in second plash (unit 1). @@ -1468,6 +1461,13 @@ static void virt_machine_init(MachineState *machine) memory_region_add_subregion(system_memory, memmap[VIRT_MROM].base, mask_rom); + /* + * Init fw_cfg. Must be done before riscv_load_fdt, otherwise the + * device tree cannot be altered and we get FDT_ERR_NOSPACE. + */ + s->fw_cfg = create_fw_cfg(machine); + rom_set_fw(s->fw_cfg); + /* SiFive Test MMIO device */ sifive_test_create(memmap[VIRT_TEST].base);