* [PATCH v2 1/8] target/riscv: Fix csr number based privilege checking
2022-05-11 14:45 [PATCH v2 0/8] QEMU RISC-V nested virtualization fixes Anup Patel
@ 2022-05-11 14:45 ` Anup Patel
2022-05-13 18:19 ` Atish Patra
2022-05-11 14:45 ` [PATCH v2 2/8] target/riscv: Fix hstatus.GVA bit setting for traps taken from HS-mode Anup Patel
` (7 subsequent siblings)
8 siblings, 1 reply; 22+ messages in thread
From: Anup Patel @ 2022-05-11 14:45 UTC (permalink / raw)
To: Peter Maydell, Palmer Dabbelt, Alistair Francis, Sagar Karandikar
Cc: Atish Patra, Anup Patel, qemu-riscv, qemu-devel, Anup Patel,
Alistair Francis, Frank Chang
When hypervisor and VS CSRs are accessed from VS-mode or VU-mode,
the riscv_csrrw_check() function should generate virtual instruction
trap instead illegal instruction trap.
Fixes: 0a42f4c44088 (" target/riscv: Fix CSR perm checking for HS mode")
Signed-off-by: Anup Patel <apatel@ventanamicro.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Frank Chang <frank.chang@sifive.com>
---
target/riscv/csr.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 3500e07f92..2bf0a97196 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -3139,7 +3139,7 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
int read_only = get_field(csrno, 0xC00) == 3;
int csr_min_priv = csr_ops[csrno].min_priv_ver;
#if !defined(CONFIG_USER_ONLY)
- int effective_priv = env->priv;
+ int csr_priv, effective_priv = env->priv;
if (riscv_has_ext(env, RVH) &&
env->priv == PRV_S &&
@@ -3152,7 +3152,11 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
effective_priv++;
}
- if (!env->debugger && (effective_priv < get_field(csrno, 0x300))) {
+ csr_priv = get_field(csrno, 0x300);
+ if (!env->debugger && (effective_priv < csr_priv)) {
+ if (csr_priv == (PRV_S + 1) && riscv_cpu_virt_enabled(env)) {
+ return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
+ }
return RISCV_EXCP_ILLEGAL_INST;
}
#endif
--
2.34.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v2 1/8] target/riscv: Fix csr number based privilege checking
2022-05-11 14:45 ` [PATCH v2 1/8] target/riscv: Fix csr number based privilege checking Anup Patel
@ 2022-05-13 18:19 ` Atish Patra
0 siblings, 0 replies; 22+ messages in thread
From: Atish Patra @ 2022-05-13 18:19 UTC (permalink / raw)
To: Anup Patel
Cc: Peter Maydell, Palmer Dabbelt, Alistair Francis, Sagar Karandikar,
Anup Patel, open list:RISC-V, qemu-devel@nongnu.org Developers,
Frank Chang
On Wed, May 11, 2022 at 7:46 AM Anup Patel <apatel@ventanamicro.com> wrote:
>
> When hypervisor and VS CSRs are accessed from VS-mode or VU-mode,
> the riscv_csrrw_check() function should generate virtual instruction
> trap instead illegal instruction trap.
>
> Fixes: 0a42f4c44088 (" target/riscv: Fix CSR perm checking for HS mode")
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> Reviewed-by: Frank Chang <frank.chang@sifive.com>
> ---
> target/riscv/csr.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 3500e07f92..2bf0a97196 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -3139,7 +3139,7 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
> int read_only = get_field(csrno, 0xC00) == 3;
> int csr_min_priv = csr_ops[csrno].min_priv_ver;
> #if !defined(CONFIG_USER_ONLY)
> - int effective_priv = env->priv;
> + int csr_priv, effective_priv = env->priv;
>
> if (riscv_has_ext(env, RVH) &&
> env->priv == PRV_S &&
> @@ -3152,7 +3152,11 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
> effective_priv++;
> }
>
> - if (!env->debugger && (effective_priv < get_field(csrno, 0x300))) {
> + csr_priv = get_field(csrno, 0x300);
> + if (!env->debugger && (effective_priv < csr_priv)) {
> + if (csr_priv == (PRV_S + 1) && riscv_cpu_virt_enabled(env)) {
> + return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> + }
> return RISCV_EXCP_ILLEGAL_INST;
> }
> #endif
> --
> 2.34.1
>
It seems Alistair has already queued a similar fix
https://www.mail-archive.com/qemu-devel@nongnu.org/msg886861.html
--
Regards,
Atish
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 2/8] target/riscv: Fix hstatus.GVA bit setting for traps taken from HS-mode
2022-05-11 14:45 [PATCH v2 0/8] QEMU RISC-V nested virtualization fixes Anup Patel
2022-05-11 14:45 ` [PATCH v2 1/8] target/riscv: Fix csr number based privilege checking Anup Patel
@ 2022-05-11 14:45 ` Anup Patel
2022-05-16 23:24 ` Alistair Francis
2022-05-11 14:45 ` [PATCH v2 3/8] target/riscv: Set [m|s]tval for both illegal and virtual instruction traps Anup Patel
` (6 subsequent siblings)
8 siblings, 1 reply; 22+ messages in thread
From: Anup Patel @ 2022-05-11 14:45 UTC (permalink / raw)
To: Peter Maydell, Palmer Dabbelt, Alistair Francis, Sagar Karandikar
Cc: Atish Patra, Anup Patel, qemu-riscv, qemu-devel, Anup Patel
Currently, QEMU does not set hstatus.GVA bit for traps taken from
HS-mode into HS-mode which breaks the Xvisor nested MMU test suite
on QEMU. This was working previously.
This patch updates riscv_cpu_do_interrupt() to fix the above issue.
Fixes: 86d0c457396b ("target/riscv: Fixup setting GVA")
Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
target/riscv/cpu_helper.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index e1aa4f2097..b16bfe0182 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -1367,7 +1367,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
case RISCV_EXCP_INST_PAGE_FAULT:
case RISCV_EXCP_LOAD_PAGE_FAULT:
case RISCV_EXCP_STORE_PAGE_FAULT:
- write_gva = true;
+ write_gva = env->two_stage_lookup;
tval = env->badaddr;
break;
case RISCV_EXCP_ILLEGAL_INST:
@@ -1434,7 +1434,6 @@ void riscv_cpu_do_interrupt(CPUState *cs)
/* Trap into HS mode */
env->hstatus = set_field(env->hstatus, HSTATUS_SPV, false);
htval = env->guest_phys_fault_addr;
- write_gva = false;
}
env->hstatus = set_field(env->hstatus, HSTATUS_GVA, write_gva);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v2 2/8] target/riscv: Fix hstatus.GVA bit setting for traps taken from HS-mode
2022-05-11 14:45 ` [PATCH v2 2/8] target/riscv: Fix hstatus.GVA bit setting for traps taken from HS-mode Anup Patel
@ 2022-05-16 23:24 ` Alistair Francis
0 siblings, 0 replies; 22+ messages in thread
From: Alistair Francis @ 2022-05-16 23:24 UTC (permalink / raw)
To: Anup Patel
Cc: Peter Maydell, Palmer Dabbelt, Alistair Francis, Sagar Karandikar,
Atish Patra, Anup Patel, open list:RISC-V,
qemu-devel@nongnu.org Developers
On Thu, May 12, 2022 at 12:49 AM Anup Patel <apatel@ventanamicro.com> wrote:
>
> Currently, QEMU does not set hstatus.GVA bit for traps taken from
> HS-mode into HS-mode which breaks the Xvisor nested MMU test suite
> on QEMU. This was working previously.
>
> This patch updates riscv_cpu_do_interrupt() to fix the above issue.
>
> Fixes: 86d0c457396b ("target/riscv: Fixup setting GVA")
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> target/riscv/cpu_helper.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index e1aa4f2097..b16bfe0182 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -1367,7 +1367,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> case RISCV_EXCP_INST_PAGE_FAULT:
> case RISCV_EXCP_LOAD_PAGE_FAULT:
> case RISCV_EXCP_STORE_PAGE_FAULT:
> - write_gva = true;
> + write_gva = env->two_stage_lookup;
> tval = env->badaddr;
> break;
> case RISCV_EXCP_ILLEGAL_INST:
> @@ -1434,7 +1434,6 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> /* Trap into HS mode */
> env->hstatus = set_field(env->hstatus, HSTATUS_SPV, false);
> htval = env->guest_phys_fault_addr;
> - write_gva = false;
> }
> env->hstatus = set_field(env->hstatus, HSTATUS_GVA, write_gva);
> }
> --
> 2.34.1
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 3/8] target/riscv: Set [m|s]tval for both illegal and virtual instruction traps
2022-05-11 14:45 [PATCH v2 0/8] QEMU RISC-V nested virtualization fixes Anup Patel
2022-05-11 14:45 ` [PATCH v2 1/8] target/riscv: Fix csr number based privilege checking Anup Patel
2022-05-11 14:45 ` [PATCH v2 2/8] target/riscv: Fix hstatus.GVA bit setting for traps taken from HS-mode Anup Patel
@ 2022-05-11 14:45 ` Anup Patel
2022-05-11 14:45 ` [PATCH v2 4/8] target/riscv: Update [m|h]tinst CSR in riscv_cpu_do_interrupt() Anup Patel
` (5 subsequent siblings)
8 siblings, 0 replies; 22+ messages in thread
From: Anup Patel @ 2022-05-11 14:45 UTC (permalink / raw)
To: Peter Maydell, Palmer Dabbelt, Alistair Francis, Sagar Karandikar
Cc: Atish Patra, Anup Patel, qemu-riscv, qemu-devel, Anup Patel,
Frank Chang, Alistair Francis
Currently, the [m|s]tval CSRs are set with trapping instruction encoding
only for illegal instruction traps taken at the time of instruction
decoding.
In RISC-V world, a valid instructions might also trap as illegal or
virtual instruction based to trapping bits in various CSRs (such as
mstatus.TVM or hstatus.VTVM).
We improve setting of [m|s]tval CSRs for all types of illegal and
virtual instruction traps.
Signed-off-by: Anup Patel <apatel@ventanamicro.com>
Reviewed-by: Frank Chang <frank.chang@sifive.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
target/riscv/cpu.c | 2 ++
target/riscv/cpu.h | 8 +++++++-
target/riscv/cpu_helper.c | 1 +
target/riscv/translate.c | 17 +++++++++++++----
4 files changed, 23 insertions(+), 5 deletions(-)
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index ccacdee215..9a5be9732d 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -406,6 +406,7 @@ void restore_state_to_opc(CPURISCVState *env, TranslationBlock *tb,
} else {
env->pc = data[0];
}
+ env->bins = data[1];
}
static void riscv_cpu_reset(DeviceState *dev)
@@ -445,6 +446,7 @@ static void riscv_cpu_reset(DeviceState *dev)
env->mcause = 0;
env->miclaim = MIP_SGEIP;
env->pc = env->resetvec;
+ env->bins = 0;
env->two_stage_lookup = false;
/* Initialized default priorities of local interrupts. */
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index fe6c9a2c92..a55c918274 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -30,6 +30,12 @@
#define TCG_GUEST_DEFAULT_MO 0
+/*
+ * RISC-V-specific extra insn start words:
+ * 1: Original instruction opcode
+ */
+#define TARGET_INSN_START_EXTRA_WORDS 1
+
#define TYPE_RISCV_CPU "riscv-cpu"
#define RISCV_CPU_TYPE_SUFFIX "-" TYPE_RISCV_CPU
@@ -140,7 +146,7 @@ struct CPUArchState {
target_ulong frm;
target_ulong badaddr;
- uint32_t bins;
+ target_ulong bins;
target_ulong guest_phys_fault_addr;
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index b16bfe0182..d99fac9d2d 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -1371,6 +1371,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
tval = env->badaddr;
break;
case RISCV_EXCP_ILLEGAL_INST:
+ case RISCV_EXCP_VIRT_INSTRUCTION_FAULT:
tval = env->bins;
break;
default:
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 0cd1d9ee94..55a4713af2 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -107,6 +107,8 @@ typedef struct DisasContext {
/* PointerMasking extension */
bool pm_mask_enabled;
bool pm_base_enabled;
+ /* TCG of the current insn_start */
+ TCGOp *insn_start;
} DisasContext;
static inline bool has_ext(DisasContext *ctx, uint32_t ext)
@@ -236,9 +238,6 @@ static void generate_exception_mtval(DisasContext *ctx, int excp)
static void gen_exception_illegal(DisasContext *ctx)
{
- tcg_gen_st_i32(tcg_constant_i32(ctx->opcode), cpu_env,
- offsetof(CPURISCVState, bins));
-
generate_exception(ctx, RISCV_EXCP_ILLEGAL_INST);
}
@@ -1017,6 +1016,13 @@ static uint32_t opcode_at(DisasContextBase *dcbase, target_ulong pc)
/* Include decoders for factored-out extensions */
#include "decode-XVentanaCondOps.c.inc"
+static inline void decode_save_opc(DisasContext *ctx, target_ulong opc)
+{
+ assert(ctx->insn_start != NULL);
+ tcg_set_insn_start_param(ctx->insn_start, 1, opc);
+ ctx->insn_start = NULL;
+}
+
static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
{
/*
@@ -1033,6 +1039,7 @@ static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
/* Check for compressed insn */
if (extract16(opcode, 0, 2) != 3) {
+ decode_save_opc(ctx, opcode);
if (!has_ext(ctx, RVC)) {
gen_exception_illegal(ctx);
} else {
@@ -1047,6 +1054,7 @@ static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
opcode32 = deposit32(opcode32, 16, 16,
translator_lduw(env, &ctx->base,
ctx->base.pc_next + 2));
+ decode_save_opc(ctx, opcode32);
ctx->opcode = opcode32;
ctx->pc_succ_insn = ctx->base.pc_next + 4;
@@ -1113,7 +1121,8 @@ static void riscv_tr_insn_start(DisasContextBase *dcbase, CPUState *cpu)
{
DisasContext *ctx = container_of(dcbase, DisasContext, base);
- tcg_gen_insn_start(ctx->base.pc_next);
+ tcg_gen_insn_start(ctx->base.pc_next, 0);
+ ctx->insn_start = tcg_last_op();
}
static void riscv_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
--
2.34.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v2 4/8] target/riscv: Update [m|h]tinst CSR in riscv_cpu_do_interrupt()
2022-05-11 14:45 [PATCH v2 0/8] QEMU RISC-V nested virtualization fixes Anup Patel
` (2 preceding siblings ...)
2022-05-11 14:45 ` [PATCH v2 3/8] target/riscv: Set [m|s]tval for both illegal and virtual instruction traps Anup Patel
@ 2022-05-11 14:45 ` Anup Patel
2022-05-23 21:38 ` Alistair Francis
2022-05-11 14:45 ` [PATCH v2 5/8] target/riscv: Don't force update priv spec version to latest Anup Patel
` (4 subsequent siblings)
8 siblings, 1 reply; 22+ messages in thread
From: Anup Patel @ 2022-05-11 14:45 UTC (permalink / raw)
To: Peter Maydell, Palmer Dabbelt, Alistair Francis, Sagar Karandikar
Cc: Atish Patra, Anup Patel, qemu-riscv, qemu-devel, Anup Patel
We should write transformed instruction encoding of the trapped
instruction in [m|h]tinst CSR at time of taking trap as defined
by the RISC-V privileged specification v1.12.
Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
target/riscv/cpu_helper.c | 168 +++++++++++++++++++++++++++++++++++++-
target/riscv/instmap.h | 41 ++++++++++
2 files changed, 205 insertions(+), 4 deletions(-)
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index d99fac9d2d..b24652eb8d 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -22,6 +22,7 @@
#include "qemu/main-loop.h"
#include "cpu.h"
#include "exec/exec-all.h"
+#include "instmap.h"
#include "tcg/tcg-op.h"
#include "trace.h"
#include "semihosting/common-semi.h"
@@ -1318,6 +1319,158 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
}
#endif /* !CONFIG_USER_ONLY */
+static target_ulong riscv_transformed_insn(CPURISCVState *env,
+ target_ulong insn)
+{
+ target_ulong xinsn = 0;
+
+ /*
+ * Only Quadrant 0 and Quadrant 2 of RVC instruction space need to
+ * be uncompressed. The Quadrant 1 of RVC instruction space need
+ * not be transformed because these instructions won't generate
+ * any load/store trap.
+ */
+
+ if ((insn & 0x3) != 0x3) {
+ /* Transform 16bit instruction into 32bit instruction */
+ switch (GET_C_OP(insn)) {
+ case OPC_RISC_C_OP_QUAD0: /* Quadrant 0 */
+ switch (GET_C_FUNC(insn)) {
+ case OPC_RISC_C_FUNC_FLD_LQ:
+ if (riscv_cpu_xlen(env) != 128) { /* C.FLD (RV32/64) */
+ xinsn = OPC_RISC_FLD;
+ xinsn = SET_RD(xinsn, GET_C_RS2S(insn));
+ xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
+ xinsn = SET_I_IMM(xinsn, GET_C_LD_IMM(insn));
+ }
+ break;
+ case OPC_RISC_C_FUNC_LW: /* C.LW */
+ xinsn = OPC_RISC_LW;
+ xinsn = SET_RD(xinsn, GET_C_RS2S(insn));
+ xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
+ xinsn = SET_I_IMM(xinsn, GET_C_LW_IMM(insn));
+ break;
+ case OPC_RISC_C_FUNC_FLW_LD:
+ if (riscv_cpu_xlen(env) == 32) { /* C.FLW (RV32) */
+ xinsn = OPC_RISC_FLW;
+ xinsn = SET_RD(xinsn, GET_C_RS2S(insn));
+ xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
+ xinsn = SET_I_IMM(xinsn, GET_C_LW_IMM(insn));
+ } else { /* C.LD (RV64/RV128) */
+ xinsn = OPC_RISC_LD;
+ xinsn = SET_RD(xinsn, GET_C_RS2S(insn));
+ xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
+ xinsn = SET_I_IMM(xinsn, GET_C_LD_IMM(insn));
+ }
+ break;
+ case OPC_RISC_C_FUNC_FSD_SQ:
+ if (riscv_cpu_xlen(env) != 128) { /* C.FSD (RV32/64) */
+ xinsn = OPC_RISC_FSD;
+ xinsn = SET_RS2(xinsn, GET_C_RS2S(insn));
+ xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
+ xinsn = SET_S_IMM(xinsn, GET_C_SD_IMM(insn));
+ }
+ break;
+ case OPC_RISC_C_FUNC_SW: /* C.SW */
+ xinsn = OPC_RISC_SW;
+ xinsn = SET_RS2(xinsn, GET_C_RS2S(insn));
+ xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
+ xinsn = SET_S_IMM(xinsn, GET_C_SW_IMM(insn));
+ break;
+ case OPC_RISC_C_FUNC_FSW_SD:
+ if (riscv_cpu_xlen(env) == 32) { /* C.FSW (RV32) */
+ xinsn = OPC_RISC_FSW;
+ xinsn = SET_RS2(xinsn, GET_C_RS2S(insn));
+ xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
+ xinsn = SET_S_IMM(xinsn, GET_C_SW_IMM(insn));
+ } else { /* C.SD (RV64/RV128) */
+ xinsn = OPC_RISC_SD;
+ xinsn = SET_RS2(xinsn, GET_C_RS2S(insn));
+ xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
+ xinsn = SET_S_IMM(xinsn, GET_C_SD_IMM(insn));
+ }
+ break;
+ default:
+ break;
+ }
+ break;
+ case OPC_RISC_C_OP_QUAD2: /* Quadrant 2 */
+ switch (GET_C_FUNC(insn)) {
+ case OPC_RISC_C_FUNC_FLDSP_LQSP:
+ if (riscv_cpu_xlen(env) != 128) { /* C.FLDSP (RV32/64) */
+ xinsn = OPC_RISC_FLD;
+ xinsn = SET_RD(xinsn, GET_C_RD(insn));
+ xinsn = SET_RS1(xinsn, 2);
+ xinsn = SET_I_IMM(xinsn, GET_C_LDSP_IMM(insn));
+ }
+ break;
+ case OPC_RISC_C_FUNC_LWSP: /* C.LWSP */
+ xinsn = OPC_RISC_LW;
+ xinsn = SET_RD(xinsn, GET_C_RD(insn));
+ xinsn = SET_RS1(xinsn, 2);
+ xinsn = SET_I_IMM(xinsn, GET_C_LWSP_IMM(insn));
+ break;
+ case OPC_RISC_C_FUNC_FLWSP_LDSP:
+ if (riscv_cpu_xlen(env) == 32) { /* C.FLWSP (RV32) */
+ xinsn = OPC_RISC_FLW;
+ xinsn = SET_RD(xinsn, GET_C_RD(insn));
+ xinsn = SET_RS1(xinsn, 2);
+ xinsn = SET_I_IMM(xinsn, GET_C_LWSP_IMM(insn));
+ } else { /* C.LDSP (RV64/RV128) */
+ xinsn = OPC_RISC_LD;
+ xinsn = SET_RD(xinsn, GET_C_RD(insn));
+ xinsn = SET_RS1(xinsn, 2);
+ xinsn = SET_I_IMM(xinsn, GET_C_LDSP_IMM(insn));
+ }
+ break;
+ case OPC_RISC_C_FUNC_FSDSP_SQSP:
+ if (riscv_cpu_xlen(env) != 128) { /* C.FSDSP (RV32/64) */
+ xinsn = OPC_RISC_FSD;
+ xinsn = SET_RS2(xinsn, GET_C_RS2(insn));
+ xinsn = SET_RS1(xinsn, 2);
+ xinsn = SET_S_IMM(xinsn, GET_C_SDSP_IMM(insn));
+ }
+ break;
+ case OPC_RISC_C_FUNC_SWSP: /* C.SWSP */
+ xinsn = OPC_RISC_SW;
+ xinsn = SET_RS2(xinsn, GET_C_RS2(insn));
+ xinsn = SET_RS1(xinsn, 2);
+ xinsn = SET_S_IMM(xinsn, GET_C_SWSP_IMM(insn));
+ break;
+ case 7:
+ if (riscv_cpu_xlen(env) == 32) { /* C.FSWSP (RV32) */
+ xinsn = OPC_RISC_FSW;
+ xinsn = SET_RS2(xinsn, GET_C_RS2(insn));
+ xinsn = SET_RS1(xinsn, 2);
+ xinsn = SET_S_IMM(xinsn, GET_C_SWSP_IMM(insn));
+ } else { /* C.SDSP (RV64/RV128) */
+ xinsn = OPC_RISC_SD;
+ xinsn = SET_RS2(xinsn, GET_C_RS2(insn));
+ xinsn = SET_RS1(xinsn, 2);
+ xinsn = SET_S_IMM(xinsn, GET_C_SDSP_IMM(insn));
+ }
+ break;
+ default:
+ break;
+ }
+ break;
+ default:
+ break;
+ }
+
+ /*
+ * Clear Bit1 of transformed instruction to indicate that
+ * original insruction was a 16bit instruction
+ */
+ xinsn &= ~((target_ulong)0x2);
+ } else {
+ /* No need to transform 32bit (or wider) instructions */
+ xinsn = insn;
+ }
+
+ return xinsn;
+}
+
/*
* Handle Traps
*
@@ -1340,6 +1493,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
target_ulong cause = cs->exception_index & RISCV_EXCP_INT_MASK;
uint64_t deleg = async ? env->mideleg : env->medeleg;
target_ulong tval = 0;
+ target_ulong tinst = 0;
target_ulong htval = 0;
target_ulong mtval2 = 0;
@@ -1355,18 +1509,22 @@ void riscv_cpu_do_interrupt(CPUState *cs)
if (!async) {
/* set tval to badaddr for traps with address information */
switch (cause) {
- case RISCV_EXCP_INST_GUEST_PAGE_FAULT:
case RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT:
case RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT:
- case RISCV_EXCP_INST_ADDR_MIS:
- case RISCV_EXCP_INST_ACCESS_FAULT:
case RISCV_EXCP_LOAD_ADDR_MIS:
case RISCV_EXCP_STORE_AMO_ADDR_MIS:
case RISCV_EXCP_LOAD_ACCESS_FAULT:
case RISCV_EXCP_STORE_AMO_ACCESS_FAULT:
- case RISCV_EXCP_INST_PAGE_FAULT:
case RISCV_EXCP_LOAD_PAGE_FAULT:
case RISCV_EXCP_STORE_PAGE_FAULT:
+ write_gva = env->two_stage_lookup;
+ tval = env->badaddr;
+ tinst = riscv_transformed_insn(env, env->bins);
+ break;
+ case RISCV_EXCP_INST_GUEST_PAGE_FAULT:
+ case RISCV_EXCP_INST_ADDR_MIS:
+ case RISCV_EXCP_INST_ACCESS_FAULT:
+ case RISCV_EXCP_INST_PAGE_FAULT:
write_gva = env->two_stage_lookup;
tval = env->badaddr;
break;
@@ -1448,6 +1606,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
env->sepc = env->pc;
env->stval = tval;
env->htval = htval;
+ env->htinst = tinst;
env->pc = (env->stvec >> 2 << 2) +
((async && (env->stvec & 3) == 1) ? cause * 4 : 0);
riscv_cpu_set_mode(env, PRV_S);
@@ -1478,6 +1637,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
env->mepc = env->pc;
env->mtval = tval;
env->mtval2 = mtval2;
+ env->mtinst = tinst;
env->pc = (env->mtvec >> 2 << 2) +
((async && (env->mtvec & 3) == 1) ? cause * 4 : 0);
riscv_cpu_set_mode(env, PRV_M);
diff --git a/target/riscv/instmap.h b/target/riscv/instmap.h
index 40b6d2b64d..f4ee686c78 100644
--- a/target/riscv/instmap.h
+++ b/target/riscv/instmap.h
@@ -316,6 +316,12 @@ enum {
#define GET_RS2(inst) extract32(inst, 20, 5)
#define GET_RD(inst) extract32(inst, 7, 5)
#define GET_IMM(inst) sextract64(inst, 20, 12)
+#define SET_RS1(inst, val) deposit32(inst, 15, 5, val)
+#define SET_RS2(inst, val) deposit32(inst, 20, 5, val)
+#define SET_RD(inst, val) deposit32(inst, 7, 5, val)
+#define SET_I_IMM(inst, val) deposit32(inst, 20, 12, val)
+#define SET_S_IMM(inst, val) \
+ deposit32(deposit32(inst, 7, 5, val), 25, 7, (val) >> 5)
/* RVC decoding macros */
#define GET_C_IMM(inst) (extract32(inst, 2, 5) \
@@ -346,6 +352,8 @@ enum {
| (extract32(inst, 5, 1) << 6))
#define GET_C_LD_IMM(inst) ((extract16(inst, 10, 3) << 3) \
| (extract16(inst, 5, 2) << 6))
+#define GET_C_SW_IMM(inst) GET_C_LW_IMM(inst)
+#define GET_C_SD_IMM(inst) GET_C_LD_IMM(inst)
#define GET_C_J_IMM(inst) ((extract32(inst, 3, 3) << 1) \
| (extract32(inst, 11, 1) << 4) \
| (extract32(inst, 2, 1) << 5) \
@@ -366,4 +374,37 @@ enum {
#define GET_C_RS1S(inst) (8 + extract16(inst, 7, 3))
#define GET_C_RS2S(inst) (8 + extract16(inst, 2, 3))
+#define GET_C_FUNC(inst) extract32(inst, 13, 3)
+#define GET_C_OP(inst) extract32(inst, 0, 2)
+
+enum {
+ /* RVC Quadrants */
+ OPC_RISC_C_OP_QUAD0 = 0x0,
+ OPC_RISC_C_OP_QUAD1 = 0x1,
+ OPC_RISC_C_OP_QUAD2 = 0x2
+};
+
+enum {
+ /* RVC Quadrant 0 */
+ OPC_RISC_C_FUNC_ADDI4SPN = 0x0,
+ OPC_RISC_C_FUNC_FLD_LQ = 0x1,
+ OPC_RISC_C_FUNC_LW = 0x2,
+ OPC_RISC_C_FUNC_FLW_LD = 0x3,
+ OPC_RISC_C_FUNC_FSD_SQ = 0x5,
+ OPC_RISC_C_FUNC_SW = 0x6,
+ OPC_RISC_C_FUNC_FSW_SD = 0x7
+};
+
+enum {
+ /* RVC Quadrant 2 */
+ OPC_RISC_C_FUNC_SLLI_SLLI64 = 0x0,
+ OPC_RISC_C_FUNC_FLDSP_LQSP = 0x1,
+ OPC_RISC_C_FUNC_LWSP = 0x2,
+ OPC_RISC_C_FUNC_FLWSP_LDSP = 0x3,
+ OPC_RISC_C_FUNC_JR_MV_EBREAK_JALR_ADD = 0x4,
+ OPC_RISC_C_FUNC_FSDSP_SQSP = 0x5,
+ OPC_RISC_C_FUNC_SWSP = 0x6,
+ OPC_RISC_C_FUNC_FSWSP_SDSP = 0x7
+};
+
#endif
--
2.34.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v2 4/8] target/riscv: Update [m|h]tinst CSR in riscv_cpu_do_interrupt()
2022-05-11 14:45 ` [PATCH v2 4/8] target/riscv: Update [m|h]tinst CSR in riscv_cpu_do_interrupt() Anup Patel
@ 2022-05-23 21:38 ` Alistair Francis
2022-05-24 12:19 ` Anup Patel
0 siblings, 1 reply; 22+ messages in thread
From: Alistair Francis @ 2022-05-23 21:38 UTC (permalink / raw)
To: Anup Patel
Cc: Peter Maydell, Palmer Dabbelt, Alistair Francis, Sagar Karandikar,
Atish Patra, Anup Patel, open list:RISC-V,
qemu-devel@nongnu.org Developers
On Thu, May 12, 2022 at 12:47 AM Anup Patel <apatel@ventanamicro.com> wrote:
>
> We should write transformed instruction encoding of the trapped
> instruction in [m|h]tinst CSR at time of taking trap as defined
> by the RISC-V privileged specification v1.12.
>
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> ---
> target/riscv/cpu_helper.c | 168 +++++++++++++++++++++++++++++++++++++-
> target/riscv/instmap.h | 41 ++++++++++
> 2 files changed, 205 insertions(+), 4 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index d99fac9d2d..b24652eb8d 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -22,6 +22,7 @@
> #include "qemu/main-loop.h"
> #include "cpu.h"
> #include "exec/exec-all.h"
> +#include "instmap.h"
> #include "tcg/tcg-op.h"
> #include "trace.h"
> #include "semihosting/common-semi.h"
> @@ -1318,6 +1319,158 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> }
> #endif /* !CONFIG_USER_ONLY */
>
> +static target_ulong riscv_transformed_insn(CPURISCVState *env,
> + target_ulong insn)
> +{
> + target_ulong xinsn = 0;
> +
> + /*
> + * Only Quadrant 0 and Quadrant 2 of RVC instruction space need to
> + * be uncompressed. The Quadrant 1 of RVC instruction space need
> + * not be transformed because these instructions won't generate
> + * any load/store trap.
> + */
> +
> + if ((insn & 0x3) != 0x3) {
> + /* Transform 16bit instruction into 32bit instruction */
> + switch (GET_C_OP(insn)) {
> + case OPC_RISC_C_OP_QUAD0: /* Quadrant 0 */
> + switch (GET_C_FUNC(insn)) {
> + case OPC_RISC_C_FUNC_FLD_LQ:
> + if (riscv_cpu_xlen(env) != 128) { /* C.FLD (RV32/64) */
> + xinsn = OPC_RISC_FLD;
> + xinsn = SET_RD(xinsn, GET_C_RS2S(insn));
> + xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
> + xinsn = SET_I_IMM(xinsn, GET_C_LD_IMM(insn));
> + }
> + break;
> + case OPC_RISC_C_FUNC_LW: /* C.LW */
> + xinsn = OPC_RISC_LW;
> + xinsn = SET_RD(xinsn, GET_C_RS2S(insn));
> + xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
> + xinsn = SET_I_IMM(xinsn, GET_C_LW_IMM(insn));
> + break;
> + case OPC_RISC_C_FUNC_FLW_LD:
> + if (riscv_cpu_xlen(env) == 32) { /* C.FLW (RV32) */
> + xinsn = OPC_RISC_FLW;
> + xinsn = SET_RD(xinsn, GET_C_RS2S(insn));
> + xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
> + xinsn = SET_I_IMM(xinsn, GET_C_LW_IMM(insn));
> + } else { /* C.LD (RV64/RV128) */
> + xinsn = OPC_RISC_LD;
> + xinsn = SET_RD(xinsn, GET_C_RS2S(insn));
> + xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
> + xinsn = SET_I_IMM(xinsn, GET_C_LD_IMM(insn));
> + }
> + break;
> + case OPC_RISC_C_FUNC_FSD_SQ:
> + if (riscv_cpu_xlen(env) != 128) { /* C.FSD (RV32/64) */
> + xinsn = OPC_RISC_FSD;
> + xinsn = SET_RS2(xinsn, GET_C_RS2S(insn));
> + xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
> + xinsn = SET_S_IMM(xinsn, GET_C_SD_IMM(insn));
> + }
> + break;
> + case OPC_RISC_C_FUNC_SW: /* C.SW */
> + xinsn = OPC_RISC_SW;
> + xinsn = SET_RS2(xinsn, GET_C_RS2S(insn));
> + xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
> + xinsn = SET_S_IMM(xinsn, GET_C_SW_IMM(insn));
> + break;
> + case OPC_RISC_C_FUNC_FSW_SD:
> + if (riscv_cpu_xlen(env) == 32) { /* C.FSW (RV32) */
> + xinsn = OPC_RISC_FSW;
> + xinsn = SET_RS2(xinsn, GET_C_RS2S(insn));
> + xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
> + xinsn = SET_S_IMM(xinsn, GET_C_SW_IMM(insn));
> + } else { /* C.SD (RV64/RV128) */
> + xinsn = OPC_RISC_SD;
> + xinsn = SET_RS2(xinsn, GET_C_RS2S(insn));
> + xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
> + xinsn = SET_S_IMM(xinsn, GET_C_SD_IMM(insn));
> + }
> + break;
> + default:
> + break;
> + }
> + break;
> + case OPC_RISC_C_OP_QUAD2: /* Quadrant 2 */
> + switch (GET_C_FUNC(insn)) {
> + case OPC_RISC_C_FUNC_FLDSP_LQSP:
> + if (riscv_cpu_xlen(env) != 128) { /* C.FLDSP (RV32/64) */
> + xinsn = OPC_RISC_FLD;
> + xinsn = SET_RD(xinsn, GET_C_RD(insn));
> + xinsn = SET_RS1(xinsn, 2);
Is this right?
Shouldn't this be the offset "between the faulting virtual address
(written to mtval or stval) and the original virtual address"?
> + xinsn = SET_I_IMM(xinsn, GET_C_LDSP_IMM(insn));
> + }
> + break;
> + case OPC_RISC_C_FUNC_LWSP: /* C.LWSP */
> + xinsn = OPC_RISC_LW;
> + xinsn = SET_RD(xinsn, GET_C_RD(insn));
> + xinsn = SET_RS1(xinsn, 2);
> + xinsn = SET_I_IMM(xinsn, GET_C_LWSP_IMM(insn));
> + break;
> + case OPC_RISC_C_FUNC_FLWSP_LDSP:
> + if (riscv_cpu_xlen(env) == 32) { /* C.FLWSP (RV32) */
> + xinsn = OPC_RISC_FLW;
> + xinsn = SET_RD(xinsn, GET_C_RD(insn));
> + xinsn = SET_RS1(xinsn, 2);
> + xinsn = SET_I_IMM(xinsn, GET_C_LWSP_IMM(insn));
> + } else { /* C.LDSP (RV64/RV128) */
> + xinsn = OPC_RISC_LD;
> + xinsn = SET_RD(xinsn, GET_C_RD(insn));
> + xinsn = SET_RS1(xinsn, 2);
> + xinsn = SET_I_IMM(xinsn, GET_C_LDSP_IMM(insn));
> + }
> + break;
> + case OPC_RISC_C_FUNC_FSDSP_SQSP:
> + if (riscv_cpu_xlen(env) != 128) { /* C.FSDSP (RV32/64) */
> + xinsn = OPC_RISC_FSD;
> + xinsn = SET_RS2(xinsn, GET_C_RS2(insn));
> + xinsn = SET_RS1(xinsn, 2);
> + xinsn = SET_S_IMM(xinsn, GET_C_SDSP_IMM(insn));
> + }
> + break;
> + case OPC_RISC_C_FUNC_SWSP: /* C.SWSP */
> + xinsn = OPC_RISC_SW;
> + xinsn = SET_RS2(xinsn, GET_C_RS2(insn));
> + xinsn = SET_RS1(xinsn, 2);
> + xinsn = SET_S_IMM(xinsn, GET_C_SWSP_IMM(insn));
> + break;
> + case 7:
> + if (riscv_cpu_xlen(env) == 32) { /* C.FSWSP (RV32) */
> + xinsn = OPC_RISC_FSW;
> + xinsn = SET_RS2(xinsn, GET_C_RS2(insn));
> + xinsn = SET_RS1(xinsn, 2);
> + xinsn = SET_S_IMM(xinsn, GET_C_SWSP_IMM(insn));
> + } else { /* C.SDSP (RV64/RV128) */
> + xinsn = OPC_RISC_SD;
> + xinsn = SET_RS2(xinsn, GET_C_RS2(insn));
> + xinsn = SET_RS1(xinsn, 2);
> + xinsn = SET_S_IMM(xinsn, GET_C_SDSP_IMM(insn));
> + }
> + break;
> + default:
> + break;
> + }
> + break;
> + default:
> + break;
> + }
> +
> + /*
> + * Clear Bit1 of transformed instruction to indicate that
> + * original insruction was a 16bit instruction
> + */
> + xinsn &= ~((target_ulong)0x2);
> + } else {
> + /* No need to transform 32bit (or wider) instructions */
From what I can tell we still need to transform 32-bit instructions as
well, the "Addr. Offset" for example needs to be changed.
Alistair
> + xinsn = insn;
> + }
> +
> + return xinsn;
> +}
> +
> /*
> * Handle Traps
> *
> @@ -1340,6 +1493,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> target_ulong cause = cs->exception_index & RISCV_EXCP_INT_MASK;
> uint64_t deleg = async ? env->mideleg : env->medeleg;
> target_ulong tval = 0;
> + target_ulong tinst = 0;
> target_ulong htval = 0;
> target_ulong mtval2 = 0;
>
> @@ -1355,18 +1509,22 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> if (!async) {
> /* set tval to badaddr for traps with address information */
> switch (cause) {
> - case RISCV_EXCP_INST_GUEST_PAGE_FAULT:
> case RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT:
> case RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT:
> - case RISCV_EXCP_INST_ADDR_MIS:
> - case RISCV_EXCP_INST_ACCESS_FAULT:
> case RISCV_EXCP_LOAD_ADDR_MIS:
> case RISCV_EXCP_STORE_AMO_ADDR_MIS:
> case RISCV_EXCP_LOAD_ACCESS_FAULT:
> case RISCV_EXCP_STORE_AMO_ACCESS_FAULT:
> - case RISCV_EXCP_INST_PAGE_FAULT:
> case RISCV_EXCP_LOAD_PAGE_FAULT:
> case RISCV_EXCP_STORE_PAGE_FAULT:
> + write_gva = env->two_stage_lookup;
> + tval = env->badaddr;
> + tinst = riscv_transformed_insn(env, env->bins);
> + break;
> + case RISCV_EXCP_INST_GUEST_PAGE_FAULT:
> + case RISCV_EXCP_INST_ADDR_MIS:
> + case RISCV_EXCP_INST_ACCESS_FAULT:
> + case RISCV_EXCP_INST_PAGE_FAULT:
> write_gva = env->two_stage_lookup;
> tval = env->badaddr;
> break;
> @@ -1448,6 +1606,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> env->sepc = env->pc;
> env->stval = tval;
> env->htval = htval;
> + env->htinst = tinst;
> env->pc = (env->stvec >> 2 << 2) +
> ((async && (env->stvec & 3) == 1) ? cause * 4 : 0);
> riscv_cpu_set_mode(env, PRV_S);
> @@ -1478,6 +1637,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> env->mepc = env->pc;
> env->mtval = tval;
> env->mtval2 = mtval2;
> + env->mtinst = tinst;
> env->pc = (env->mtvec >> 2 << 2) +
> ((async && (env->mtvec & 3) == 1) ? cause * 4 : 0);
> riscv_cpu_set_mode(env, PRV_M);
> diff --git a/target/riscv/instmap.h b/target/riscv/instmap.h
> index 40b6d2b64d..f4ee686c78 100644
> --- a/target/riscv/instmap.h
> +++ b/target/riscv/instmap.h
> @@ -316,6 +316,12 @@ enum {
> #define GET_RS2(inst) extract32(inst, 20, 5)
> #define GET_RD(inst) extract32(inst, 7, 5)
> #define GET_IMM(inst) sextract64(inst, 20, 12)
> +#define SET_RS1(inst, val) deposit32(inst, 15, 5, val)
> +#define SET_RS2(inst, val) deposit32(inst, 20, 5, val)
> +#define SET_RD(inst, val) deposit32(inst, 7, 5, val)
> +#define SET_I_IMM(inst, val) deposit32(inst, 20, 12, val)
> +#define SET_S_IMM(inst, val) \
> + deposit32(deposit32(inst, 7, 5, val), 25, 7, (val) >> 5)
>
> /* RVC decoding macros */
> #define GET_C_IMM(inst) (extract32(inst, 2, 5) \
> @@ -346,6 +352,8 @@ enum {
> | (extract32(inst, 5, 1) << 6))
> #define GET_C_LD_IMM(inst) ((extract16(inst, 10, 3) << 3) \
> | (extract16(inst, 5, 2) << 6))
> +#define GET_C_SW_IMM(inst) GET_C_LW_IMM(inst)
> +#define GET_C_SD_IMM(inst) GET_C_LD_IMM(inst)
> #define GET_C_J_IMM(inst) ((extract32(inst, 3, 3) << 1) \
> | (extract32(inst, 11, 1) << 4) \
> | (extract32(inst, 2, 1) << 5) \
> @@ -366,4 +374,37 @@ enum {
> #define GET_C_RS1S(inst) (8 + extract16(inst, 7, 3))
> #define GET_C_RS2S(inst) (8 + extract16(inst, 2, 3))
>
> +#define GET_C_FUNC(inst) extract32(inst, 13, 3)
> +#define GET_C_OP(inst) extract32(inst, 0, 2)
> +
> +enum {
> + /* RVC Quadrants */
> + OPC_RISC_C_OP_QUAD0 = 0x0,
> + OPC_RISC_C_OP_QUAD1 = 0x1,
> + OPC_RISC_C_OP_QUAD2 = 0x2
> +};
> +
> +enum {
> + /* RVC Quadrant 0 */
> + OPC_RISC_C_FUNC_ADDI4SPN = 0x0,
> + OPC_RISC_C_FUNC_FLD_LQ = 0x1,
> + OPC_RISC_C_FUNC_LW = 0x2,
> + OPC_RISC_C_FUNC_FLW_LD = 0x3,
> + OPC_RISC_C_FUNC_FSD_SQ = 0x5,
> + OPC_RISC_C_FUNC_SW = 0x6,
> + OPC_RISC_C_FUNC_FSW_SD = 0x7
> +};
> +
> +enum {
> + /* RVC Quadrant 2 */
> + OPC_RISC_C_FUNC_SLLI_SLLI64 = 0x0,
> + OPC_RISC_C_FUNC_FLDSP_LQSP = 0x1,
> + OPC_RISC_C_FUNC_LWSP = 0x2,
> + OPC_RISC_C_FUNC_FLWSP_LDSP = 0x3,
> + OPC_RISC_C_FUNC_JR_MV_EBREAK_JALR_ADD = 0x4,
> + OPC_RISC_C_FUNC_FSDSP_SQSP = 0x5,
> + OPC_RISC_C_FUNC_SWSP = 0x6,
> + OPC_RISC_C_FUNC_FSWSP_SDSP = 0x7
> +};
> +
> #endif
> --
> 2.34.1
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 4/8] target/riscv: Update [m|h]tinst CSR in riscv_cpu_do_interrupt()
2022-05-23 21:38 ` Alistair Francis
@ 2022-05-24 12:19 ` Anup Patel
0 siblings, 0 replies; 22+ messages in thread
From: Anup Patel @ 2022-05-24 12:19 UTC (permalink / raw)
To: Alistair Francis
Cc: Anup Patel, Peter Maydell, Palmer Dabbelt, Alistair Francis,
Sagar Karandikar, Atish Patra, open list:RISC-V,
qemu-devel@nongnu.org Developers
On Tue, May 24, 2022 at 3:08 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Thu, May 12, 2022 at 12:47 AM Anup Patel <apatel@ventanamicro.com> wrote:
> >
> > We should write transformed instruction encoding of the trapped
> > instruction in [m|h]tinst CSR at time of taking trap as defined
> > by the RISC-V privileged specification v1.12.
> >
> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > ---
> > target/riscv/cpu_helper.c | 168 +++++++++++++++++++++++++++++++++++++-
> > target/riscv/instmap.h | 41 ++++++++++
> > 2 files changed, 205 insertions(+), 4 deletions(-)
> >
> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > index d99fac9d2d..b24652eb8d 100644
> > --- a/target/riscv/cpu_helper.c
> > +++ b/target/riscv/cpu_helper.c
> > @@ -22,6 +22,7 @@
> > #include "qemu/main-loop.h"
> > #include "cpu.h"
> > #include "exec/exec-all.h"
> > +#include "instmap.h"
> > #include "tcg/tcg-op.h"
> > #include "trace.h"
> > #include "semihosting/common-semi.h"
> > @@ -1318,6 +1319,158 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> > }
> > #endif /* !CONFIG_USER_ONLY */
> >
> > +static target_ulong riscv_transformed_insn(CPURISCVState *env,
> > + target_ulong insn)
> > +{
> > + target_ulong xinsn = 0;
> > +
> > + /*
> > + * Only Quadrant 0 and Quadrant 2 of RVC instruction space need to
> > + * be uncompressed. The Quadrant 1 of RVC instruction space need
> > + * not be transformed because these instructions won't generate
> > + * any load/store trap.
> > + */
> > +
> > + if ((insn & 0x3) != 0x3) {
> > + /* Transform 16bit instruction into 32bit instruction */
> > + switch (GET_C_OP(insn)) {
> > + case OPC_RISC_C_OP_QUAD0: /* Quadrant 0 */
> > + switch (GET_C_FUNC(insn)) {
> > + case OPC_RISC_C_FUNC_FLD_LQ:
> > + if (riscv_cpu_xlen(env) != 128) { /* C.FLD (RV32/64) */
> > + xinsn = OPC_RISC_FLD;
> > + xinsn = SET_RD(xinsn, GET_C_RS2S(insn));
> > + xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
> > + xinsn = SET_I_IMM(xinsn, GET_C_LD_IMM(insn));
> > + }
> > + break;
> > + case OPC_RISC_C_FUNC_LW: /* C.LW */
> > + xinsn = OPC_RISC_LW;
> > + xinsn = SET_RD(xinsn, GET_C_RS2S(insn));
> > + xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
> > + xinsn = SET_I_IMM(xinsn, GET_C_LW_IMM(insn));
> > + break;
> > + case OPC_RISC_C_FUNC_FLW_LD:
> > + if (riscv_cpu_xlen(env) == 32) { /* C.FLW (RV32) */
> > + xinsn = OPC_RISC_FLW;
> > + xinsn = SET_RD(xinsn, GET_C_RS2S(insn));
> > + xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
> > + xinsn = SET_I_IMM(xinsn, GET_C_LW_IMM(insn));
> > + } else { /* C.LD (RV64/RV128) */
> > + xinsn = OPC_RISC_LD;
> > + xinsn = SET_RD(xinsn, GET_C_RS2S(insn));
> > + xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
> > + xinsn = SET_I_IMM(xinsn, GET_C_LD_IMM(insn));
> > + }
> > + break;
> > + case OPC_RISC_C_FUNC_FSD_SQ:
> > + if (riscv_cpu_xlen(env) != 128) { /* C.FSD (RV32/64) */
> > + xinsn = OPC_RISC_FSD;
> > + xinsn = SET_RS2(xinsn, GET_C_RS2S(insn));
> > + xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
> > + xinsn = SET_S_IMM(xinsn, GET_C_SD_IMM(insn));
> > + }
> > + break;
> > + case OPC_RISC_C_FUNC_SW: /* C.SW */
> > + xinsn = OPC_RISC_SW;
> > + xinsn = SET_RS2(xinsn, GET_C_RS2S(insn));
> > + xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
> > + xinsn = SET_S_IMM(xinsn, GET_C_SW_IMM(insn));
> > + break;
> > + case OPC_RISC_C_FUNC_FSW_SD:
> > + if (riscv_cpu_xlen(env) == 32) { /* C.FSW (RV32) */
> > + xinsn = OPC_RISC_FSW;
> > + xinsn = SET_RS2(xinsn, GET_C_RS2S(insn));
> > + xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
> > + xinsn = SET_S_IMM(xinsn, GET_C_SW_IMM(insn));
> > + } else { /* C.SD (RV64/RV128) */
> > + xinsn = OPC_RISC_SD;
> > + xinsn = SET_RS2(xinsn, GET_C_RS2S(insn));
> > + xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
> > + xinsn = SET_S_IMM(xinsn, GET_C_SD_IMM(insn));
> > + }
> > + break;
> > + default:
> > + break;
> > + }
> > + break;
> > + case OPC_RISC_C_OP_QUAD2: /* Quadrant 2 */
> > + switch (GET_C_FUNC(insn)) {
> > + case OPC_RISC_C_FUNC_FLDSP_LQSP:
> > + if (riscv_cpu_xlen(env) != 128) { /* C.FLDSP (RV32/64) */
> > + xinsn = OPC_RISC_FLD;
> > + xinsn = SET_RD(xinsn, GET_C_RD(insn));
> > + xinsn = SET_RS1(xinsn, 2);
>
> Is this right?
>
> Shouldn't this be the offset "between the faulting virtual address
> (written to mtval or stval) and the original virtual address"?
Yes, it should be "Addr. Offset". I totally overlooked the "Addr. Offset"
field.
>
> > + xinsn = SET_I_IMM(xinsn, GET_C_LDSP_IMM(insn));
> > + }
> > + break;
> > + case OPC_RISC_C_FUNC_LWSP: /* C.LWSP */
> > + xinsn = OPC_RISC_LW;
> > + xinsn = SET_RD(xinsn, GET_C_RD(insn));
> > + xinsn = SET_RS1(xinsn, 2);
> > + xinsn = SET_I_IMM(xinsn, GET_C_LWSP_IMM(insn));
> > + break;
> > + case OPC_RISC_C_FUNC_FLWSP_LDSP:
> > + if (riscv_cpu_xlen(env) == 32) { /* C.FLWSP (RV32) */
> > + xinsn = OPC_RISC_FLW;
> > + xinsn = SET_RD(xinsn, GET_C_RD(insn));
> > + xinsn = SET_RS1(xinsn, 2);
> > + xinsn = SET_I_IMM(xinsn, GET_C_LWSP_IMM(insn));
> > + } else { /* C.LDSP (RV64/RV128) */
> > + xinsn = OPC_RISC_LD;
> > + xinsn = SET_RD(xinsn, GET_C_RD(insn));
> > + xinsn = SET_RS1(xinsn, 2);
> > + xinsn = SET_I_IMM(xinsn, GET_C_LDSP_IMM(insn));
> > + }
> > + break;
> > + case OPC_RISC_C_FUNC_FSDSP_SQSP:
> > + if (riscv_cpu_xlen(env) != 128) { /* C.FSDSP (RV32/64) */
> > + xinsn = OPC_RISC_FSD;
> > + xinsn = SET_RS2(xinsn, GET_C_RS2(insn));
> > + xinsn = SET_RS1(xinsn, 2);
> > + xinsn = SET_S_IMM(xinsn, GET_C_SDSP_IMM(insn));
> > + }
> > + break;
> > + case OPC_RISC_C_FUNC_SWSP: /* C.SWSP */
> > + xinsn = OPC_RISC_SW;
> > + xinsn = SET_RS2(xinsn, GET_C_RS2(insn));
> > + xinsn = SET_RS1(xinsn, 2);
> > + xinsn = SET_S_IMM(xinsn, GET_C_SWSP_IMM(insn));
> > + break;
> > + case 7:
> > + if (riscv_cpu_xlen(env) == 32) { /* C.FSWSP (RV32) */
> > + xinsn = OPC_RISC_FSW;
> > + xinsn = SET_RS2(xinsn, GET_C_RS2(insn));
> > + xinsn = SET_RS1(xinsn, 2);
> > + xinsn = SET_S_IMM(xinsn, GET_C_SWSP_IMM(insn));
> > + } else { /* C.SDSP (RV64/RV128) */
> > + xinsn = OPC_RISC_SD;
> > + xinsn = SET_RS2(xinsn, GET_C_RS2(insn));
> > + xinsn = SET_RS1(xinsn, 2);
> > + xinsn = SET_S_IMM(xinsn, GET_C_SDSP_IMM(insn));
> > + }
> > + break;
> > + default:
> > + break;
> > + }
> > + break;
> > + default:
> > + break;
> > + }
> > +
> > + /*
> > + * Clear Bit1 of transformed instruction to indicate that
> > + * original insruction was a 16bit instruction
> > + */
> > + xinsn &= ~((target_ulong)0x2);
> > + } else {
> > + /* No need to transform 32bit (or wider) instructions */
>
> From what I can tell we still need to transform 32-bit instructions as
> well, the "Addr. Offset" for example needs to be changed.
The "Addr. Offset" will be non-zero only for misaligned load/store
traps but QEMU RISC-V does not take misaligned load/store traps
so at least for QEMU RISC-V we should always set "Addr. Offset"
to zero.
Regards,
Anup
>
> Alistair
>
> > + xinsn = insn;
> > + }
> > +
> > + return xinsn;
> > +}
> > +
> > /*
> > * Handle Traps
> > *
> > @@ -1340,6 +1493,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> > target_ulong cause = cs->exception_index & RISCV_EXCP_INT_MASK;
> > uint64_t deleg = async ? env->mideleg : env->medeleg;
> > target_ulong tval = 0;
> > + target_ulong tinst = 0;
> > target_ulong htval = 0;
> > target_ulong mtval2 = 0;
> >
> > @@ -1355,18 +1509,22 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> > if (!async) {
> > /* set tval to badaddr for traps with address information */
> > switch (cause) {
> > - case RISCV_EXCP_INST_GUEST_PAGE_FAULT:
> > case RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT:
> > case RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT:
> > - case RISCV_EXCP_INST_ADDR_MIS:
> > - case RISCV_EXCP_INST_ACCESS_FAULT:
> > case RISCV_EXCP_LOAD_ADDR_MIS:
> > case RISCV_EXCP_STORE_AMO_ADDR_MIS:
> > case RISCV_EXCP_LOAD_ACCESS_FAULT:
> > case RISCV_EXCP_STORE_AMO_ACCESS_FAULT:
> > - case RISCV_EXCP_INST_PAGE_FAULT:
> > case RISCV_EXCP_LOAD_PAGE_FAULT:
> > case RISCV_EXCP_STORE_PAGE_FAULT:
> > + write_gva = env->two_stage_lookup;
> > + tval = env->badaddr;
> > + tinst = riscv_transformed_insn(env, env->bins);
> > + break;
> > + case RISCV_EXCP_INST_GUEST_PAGE_FAULT:
> > + case RISCV_EXCP_INST_ADDR_MIS:
> > + case RISCV_EXCP_INST_ACCESS_FAULT:
> > + case RISCV_EXCP_INST_PAGE_FAULT:
> > write_gva = env->two_stage_lookup;
> > tval = env->badaddr;
> > break;
> > @@ -1448,6 +1606,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> > env->sepc = env->pc;
> > env->stval = tval;
> > env->htval = htval;
> > + env->htinst = tinst;
> > env->pc = (env->stvec >> 2 << 2) +
> > ((async && (env->stvec & 3) == 1) ? cause * 4 : 0);
> > riscv_cpu_set_mode(env, PRV_S);
> > @@ -1478,6 +1637,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> > env->mepc = env->pc;
> > env->mtval = tval;
> > env->mtval2 = mtval2;
> > + env->mtinst = tinst;
> > env->pc = (env->mtvec >> 2 << 2) +
> > ((async && (env->mtvec & 3) == 1) ? cause * 4 : 0);
> > riscv_cpu_set_mode(env, PRV_M);
> > diff --git a/target/riscv/instmap.h b/target/riscv/instmap.h
> > index 40b6d2b64d..f4ee686c78 100644
> > --- a/target/riscv/instmap.h
> > +++ b/target/riscv/instmap.h
> > @@ -316,6 +316,12 @@ enum {
> > #define GET_RS2(inst) extract32(inst, 20, 5)
> > #define GET_RD(inst) extract32(inst, 7, 5)
> > #define GET_IMM(inst) sextract64(inst, 20, 12)
> > +#define SET_RS1(inst, val) deposit32(inst, 15, 5, val)
> > +#define SET_RS2(inst, val) deposit32(inst, 20, 5, val)
> > +#define SET_RD(inst, val) deposit32(inst, 7, 5, val)
> > +#define SET_I_IMM(inst, val) deposit32(inst, 20, 12, val)
> > +#define SET_S_IMM(inst, val) \
> > + deposit32(deposit32(inst, 7, 5, val), 25, 7, (val) >> 5)
> >
> > /* RVC decoding macros */
> > #define GET_C_IMM(inst) (extract32(inst, 2, 5) \
> > @@ -346,6 +352,8 @@ enum {
> > | (extract32(inst, 5, 1) << 6))
> > #define GET_C_LD_IMM(inst) ((extract16(inst, 10, 3) << 3) \
> > | (extract16(inst, 5, 2) << 6))
> > +#define GET_C_SW_IMM(inst) GET_C_LW_IMM(inst)
> > +#define GET_C_SD_IMM(inst) GET_C_LD_IMM(inst)
> > #define GET_C_J_IMM(inst) ((extract32(inst, 3, 3) << 1) \
> > | (extract32(inst, 11, 1) << 4) \
> > | (extract32(inst, 2, 1) << 5) \
> > @@ -366,4 +374,37 @@ enum {
> > #define GET_C_RS1S(inst) (8 + extract16(inst, 7, 3))
> > #define GET_C_RS2S(inst) (8 + extract16(inst, 2, 3))
> >
> > +#define GET_C_FUNC(inst) extract32(inst, 13, 3)
> > +#define GET_C_OP(inst) extract32(inst, 0, 2)
> > +
> > +enum {
> > + /* RVC Quadrants */
> > + OPC_RISC_C_OP_QUAD0 = 0x0,
> > + OPC_RISC_C_OP_QUAD1 = 0x1,
> > + OPC_RISC_C_OP_QUAD2 = 0x2
> > +};
> > +
> > +enum {
> > + /* RVC Quadrant 0 */
> > + OPC_RISC_C_FUNC_ADDI4SPN = 0x0,
> > + OPC_RISC_C_FUNC_FLD_LQ = 0x1,
> > + OPC_RISC_C_FUNC_LW = 0x2,
> > + OPC_RISC_C_FUNC_FLW_LD = 0x3,
> > + OPC_RISC_C_FUNC_FSD_SQ = 0x5,
> > + OPC_RISC_C_FUNC_SW = 0x6,
> > + OPC_RISC_C_FUNC_FSW_SD = 0x7
> > +};
> > +
> > +enum {
> > + /* RVC Quadrant 2 */
> > + OPC_RISC_C_FUNC_SLLI_SLLI64 = 0x0,
> > + OPC_RISC_C_FUNC_FLDSP_LQSP = 0x1,
> > + OPC_RISC_C_FUNC_LWSP = 0x2,
> > + OPC_RISC_C_FUNC_FLWSP_LDSP = 0x3,
> > + OPC_RISC_C_FUNC_JR_MV_EBREAK_JALR_ADD = 0x4,
> > + OPC_RISC_C_FUNC_FSDSP_SQSP = 0x5,
> > + OPC_RISC_C_FUNC_SWSP = 0x6,
> > + OPC_RISC_C_FUNC_FSWSP_SDSP = 0x7
> > +};
> > +
> > #endif
> > --
> > 2.34.1
> >
> >
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 5/8] target/riscv: Don't force update priv spec version to latest
2022-05-11 14:45 [PATCH v2 0/8] QEMU RISC-V nested virtualization fixes Anup Patel
` (3 preceding siblings ...)
2022-05-11 14:45 ` [PATCH v2 4/8] target/riscv: Update [m|h]tinst CSR in riscv_cpu_do_interrupt() Anup Patel
@ 2022-05-11 14:45 ` Anup Patel
2022-05-13 18:23 ` Atish Patra
2022-05-11 14:45 ` [PATCH v2 6/8] target/riscv: Add dummy mcountinhibit CSR for priv spec v1.11 or higher Anup Patel
` (3 subsequent siblings)
8 siblings, 1 reply; 22+ messages in thread
From: Anup Patel @ 2022-05-11 14:45 UTC (permalink / raw)
To: Peter Maydell, Palmer Dabbelt, Alistair Francis, Sagar Karandikar
Cc: Atish Patra, Anup Patel, qemu-riscv, qemu-devel, Anup Patel,
Frank Chang, Alistair Francis, Atish Patra
The riscv_cpu_realize() sets priv spec verion to v1.12 when it is
when "env->priv_ver == 0" (i.e. default v1.10) because the enum
value of priv spec v1.10 is zero.
Due to above issue, the sifive_u machine will see priv spec v1.12
instead of priv spec v1.10.
To fix this issue, we set latest priv spec version (i.e. v1.12)
for base rv64/rv32 cpu and riscv_cpu_realize() will override priv
spec version only when "cpu->cfg.priv_spec != NULL".
Fixes: 7100fe6c2441 ("target/riscv: Enable privileged spec version 1.12")
Signed-off-by: Anup Patel <apatel@ventanamicro.com>
Reviewed-by: Frank Chang <frank.chang@sifive.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Atish Patra <atishp@rivosinc.com>
---
target/riscv/cpu.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 9a5be9732d..f3b61dfd63 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -169,6 +169,8 @@ static void rv64_base_cpu_init(Object *obj)
CPURISCVState *env = &RISCV_CPU(obj)->env;
/* We set this in the realise function */
set_misa(env, MXL_RV64, 0);
+ /* Set latest version of privileged specification */
+ set_priv_version(env, PRIV_VERSION_1_12_0);
}
static void rv64_sifive_u_cpu_init(Object *obj)
@@ -204,6 +206,8 @@ static void rv32_base_cpu_init(Object *obj)
CPURISCVState *env = &RISCV_CPU(obj)->env;
/* We set this in the realise function */
set_misa(env, MXL_RV32, 0);
+ /* Set latest version of privileged specification */
+ set_priv_version(env, PRIV_VERSION_1_12_0);
}
static void rv32_sifive_u_cpu_init(Object *obj)
@@ -509,7 +513,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
CPURISCVState *env = &cpu->env;
RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(dev);
CPUClass *cc = CPU_CLASS(mcc);
- int priv_version = 0;
+ int priv_version = -1;
Error *local_err = NULL;
cpu_exec_realizefn(cs, &local_err);
@@ -533,10 +537,8 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
}
}
- if (priv_version) {
+ if (priv_version >= PRIV_VERSION_1_10_0) {
set_priv_version(env, priv_version);
- } else if (!env->priv_ver) {
- set_priv_version(env, PRIV_VERSION_1_12_0);
}
if (cpu->cfg.mmu) {
--
2.34.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v2 5/8] target/riscv: Don't force update priv spec version to latest
2022-05-11 14:45 ` [PATCH v2 5/8] target/riscv: Don't force update priv spec version to latest Anup Patel
@ 2022-05-13 18:23 ` Atish Patra
0 siblings, 0 replies; 22+ messages in thread
From: Atish Patra @ 2022-05-13 18:23 UTC (permalink / raw)
To: Anup Patel
Cc: Peter Maydell, Palmer Dabbelt, Alistair Francis, Sagar Karandikar,
Anup Patel, open list:RISC-V, qemu-devel@nongnu.org Developers,
Frank Chang, Atish Patra
On Wed, May 11, 2022 at 7:46 AM Anup Patel <apatel@ventanamicro.com> wrote:
>
> The riscv_cpu_realize() sets priv spec verion to v1.12 when it is
> when "env->priv_ver == 0" (i.e. default v1.10) because the enum
> value of priv spec v1.10 is zero.
>
> Due to above issue, the sifive_u machine will see priv spec v1.12
> instead of priv spec v1.10.
>
> To fix this issue, we set latest priv spec version (i.e. v1.12)
> for base rv64/rv32 cpu and riscv_cpu_realize() will override priv
> spec version only when "cpu->cfg.priv_spec != NULL".
>
> Fixes: 7100fe6c2441 ("target/riscv: Enable privileged spec version 1.12")
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> Reviewed-by: Frank Chang <frank.chang@sifive.com>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> Reviewed-by: Atish Patra <atishp@rivosinc.com>
> ---
> target/riscv/cpu.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 9a5be9732d..f3b61dfd63 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -169,6 +169,8 @@ static void rv64_base_cpu_init(Object *obj)
> CPURISCVState *env = &RISCV_CPU(obj)->env;
> /* We set this in the realise function */
> set_misa(env, MXL_RV64, 0);
> + /* Set latest version of privileged specification */
> + set_priv_version(env, PRIV_VERSION_1_12_0);
> }
>
> static void rv64_sifive_u_cpu_init(Object *obj)
> @@ -204,6 +206,8 @@ static void rv32_base_cpu_init(Object *obj)
> CPURISCVState *env = &RISCV_CPU(obj)->env;
> /* We set this in the realise function */
> set_misa(env, MXL_RV32, 0);
> + /* Set latest version of privileged specification */
> + set_priv_version(env, PRIV_VERSION_1_12_0);
> }
>
> static void rv32_sifive_u_cpu_init(Object *obj)
> @@ -509,7 +513,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> CPURISCVState *env = &cpu->env;
> RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(dev);
> CPUClass *cc = CPU_CLASS(mcc);
> - int priv_version = 0;
> + int priv_version = -1;
> Error *local_err = NULL;
>
> cpu_exec_realizefn(cs, &local_err);
> @@ -533,10 +537,8 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> }
> }
>
> - if (priv_version) {
> + if (priv_version >= PRIV_VERSION_1_10_0) {
> set_priv_version(env, priv_version);
> - } else if (!env->priv_ver) {
> - set_priv_version(env, PRIV_VERSION_1_12_0);
> }
>
> if (cpu->cfg.mmu) {
> --
> 2.34.1
>
Reviewed-by: Atish Patra <atishp@rivosinc.com>
--
Regards,
Atish
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 6/8] target/riscv: Add dummy mcountinhibit CSR for priv spec v1.11 or higher
2022-05-11 14:45 [PATCH v2 0/8] QEMU RISC-V nested virtualization fixes Anup Patel
` (4 preceding siblings ...)
2022-05-11 14:45 ` [PATCH v2 5/8] target/riscv: Don't force update priv spec version to latest Anup Patel
@ 2022-05-11 14:45 ` Anup Patel
2022-05-13 18:24 ` Atish Patra
2022-05-11 14:45 ` [PATCH v2 7/8] target/riscv: Force disable extensions if priv spec version does not match Anup Patel
` (2 subsequent siblings)
8 siblings, 1 reply; 22+ messages in thread
From: Anup Patel @ 2022-05-11 14:45 UTC (permalink / raw)
To: Peter Maydell, Palmer Dabbelt, Alistair Francis, Sagar Karandikar
Cc: Atish Patra, Anup Patel, qemu-riscv, qemu-devel, Anup Patel,
Frank Chang, Alistair Francis
The mcountinhibit CSR is mandatory for priv spec v1.11 or higher. For
implementation that don't want to implement can simply have a dummy
mcountinhibit which always zero.
Fixes: a4b2fa433125 ("target/riscv: Introduce privilege version field in
the CSR ops.")
Signed-off-by: Anup Patel <apatel@ventanamicro.com>
Reviewed-by: Frank Chang <frank.chang@sifive.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
target/riscv/cpu_bits.h | 3 +++
target/riscv/csr.c | 2 ++
2 files changed, 5 insertions(+)
diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
index 4d04b20d06..4a55c6a709 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -159,6 +159,9 @@
#define CSR_MTVEC 0x305
#define CSR_MCOUNTEREN 0x306
+/* Machine Counter Setup */
+#define CSR_MCOUNTINHIBIT 0x320
+
/* 32-bit only */
#define CSR_MSTATUSH 0x310
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 2bf0a97196..e144ce7135 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -3391,6 +3391,8 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
[CSR_MIE] = { "mie", any, NULL, NULL, rmw_mie },
[CSR_MTVEC] = { "mtvec", any, read_mtvec, write_mtvec },
[CSR_MCOUNTEREN] = { "mcounteren", any, read_mcounteren, write_mcounteren },
+ [CSR_MCOUNTINHIBIT] = { "mcountinhibit", any, read_zero, write_ignore,
+ .min_priv_ver = PRIV_VERSION_1_11_0 },
[CSR_MSTATUSH] = { "mstatush", any32, read_mstatush, write_mstatush },
--
2.34.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v2 6/8] target/riscv: Add dummy mcountinhibit CSR for priv spec v1.11 or higher
2022-05-11 14:45 ` [PATCH v2 6/8] target/riscv: Add dummy mcountinhibit CSR for priv spec v1.11 or higher Anup Patel
@ 2022-05-13 18:24 ` Atish Patra
0 siblings, 0 replies; 22+ messages in thread
From: Atish Patra @ 2022-05-13 18:24 UTC (permalink / raw)
To: Anup Patel
Cc: Peter Maydell, Palmer Dabbelt, Alistair Francis, Sagar Karandikar,
Anup Patel, open list:RISC-V, qemu-devel@nongnu.org Developers,
Frank Chang
On Wed, May 11, 2022 at 7:46 AM Anup Patel <apatel@ventanamicro.com> wrote:
>
> The mcountinhibit CSR is mandatory for priv spec v1.11 or higher. For
> implementation that don't want to implement can simply have a dummy
> mcountinhibit which always zero.
>
> Fixes: a4b2fa433125 ("target/riscv: Introduce privilege version field in
> the CSR ops.")
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> Reviewed-by: Frank Chang <frank.chang@sifive.com>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> target/riscv/cpu_bits.h | 3 +++
> target/riscv/csr.c | 2 ++
> 2 files changed, 5 insertions(+)
>
> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> index 4d04b20d06..4a55c6a709 100644
> --- a/target/riscv/cpu_bits.h
> +++ b/target/riscv/cpu_bits.h
> @@ -159,6 +159,9 @@
> #define CSR_MTVEC 0x305
> #define CSR_MCOUNTEREN 0x306
>
> +/* Machine Counter Setup */
> +#define CSR_MCOUNTINHIBIT 0x320
> +
> /* 32-bit only */
> #define CSR_MSTATUSH 0x310
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 2bf0a97196..e144ce7135 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -3391,6 +3391,8 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
> [CSR_MIE] = { "mie", any, NULL, NULL, rmw_mie },
> [CSR_MTVEC] = { "mtvec", any, read_mtvec, write_mtvec },
> [CSR_MCOUNTEREN] = { "mcounteren", any, read_mcounteren, write_mcounteren },
> + [CSR_MCOUNTINHIBIT] = { "mcountinhibit", any, read_zero, write_ignore,
> + .min_priv_ver = PRIV_VERSION_1_11_0 },
>
> [CSR_MSTATUSH] = { "mstatush", any32, read_mstatush, write_mstatush },
>
> --
> 2.34.1
>
Just to clarify, this is only required if this series is merged before
the PMU series.
FWIW,
Reviewed-by: Atish Patra <atishp@rivosinc.com>
--
Regards,
Atish
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 7/8] target/riscv: Force disable extensions if priv spec version does not match
2022-05-11 14:45 [PATCH v2 0/8] QEMU RISC-V nested virtualization fixes Anup Patel
` (5 preceding siblings ...)
2022-05-11 14:45 ` [PATCH v2 6/8] target/riscv: Add dummy mcountinhibit CSR for priv spec v1.11 or higher Anup Patel
@ 2022-05-11 14:45 ` Anup Patel
2022-05-13 18:45 ` Atish Patra
2022-05-17 0:15 ` Alistair Francis
2022-05-11 14:45 ` [PATCH v2 8/8] hw/riscv: virt: Fix interrupt parent for dynamic platform devices Anup Patel
2022-05-24 22:19 ` [PATCH v2 0/8] QEMU RISC-V nested virtualization fixes Alistair Francis
8 siblings, 2 replies; 22+ messages in thread
From: Anup Patel @ 2022-05-11 14:45 UTC (permalink / raw)
To: Peter Maydell, Palmer Dabbelt, Alistair Francis, Sagar Karandikar
Cc: Atish Patra, Anup Patel, qemu-riscv, qemu-devel, Anup Patel
We should disable extensions in riscv_cpu_realize() if minimum required
priv spec version is not satisfied. This also ensures that machines with
priv spec v1.11 (or lower) cannot enable H, V, and various multi-letter
extensions.
Fixes: a775398be2e ("target/riscv: Add isa extenstion strings to the
device tree")
Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
target/riscv/cpu.c | 34 ++++++++++++++++++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index f3b61dfd63..25a4ba3e22 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -541,6 +541,40 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
set_priv_version(env, priv_version);
}
+ /* Force disable extensions if priv spec version does not match */
+ if (env->priv_ver < PRIV_VERSION_1_12_0) {
+ cpu->cfg.ext_h = false;
+ cpu->cfg.ext_v = false;
+ cpu->cfg.ext_zfh = false;
+ cpu->cfg.ext_zfhmin = false;
+ cpu->cfg.ext_zfinx = false;
+ cpu->cfg.ext_zhinx = false;
+ cpu->cfg.ext_zhinxmin = false;
+ cpu->cfg.ext_zdinx = false;
+ cpu->cfg.ext_zba = false;
+ cpu->cfg.ext_zbb = false;
+ cpu->cfg.ext_zbc = false;
+ cpu->cfg.ext_zbkb = false;
+ cpu->cfg.ext_zbkc = false;
+ cpu->cfg.ext_zbkx = false;
+ cpu->cfg.ext_zbs = false;
+ cpu->cfg.ext_zk = false;
+ cpu->cfg.ext_zkn = false;
+ cpu->cfg.ext_zknd = false;
+ cpu->cfg.ext_zkne = false;
+ cpu->cfg.ext_zknh = false;
+ cpu->cfg.ext_zkr = false;
+ cpu->cfg.ext_zks = false;
+ cpu->cfg.ext_zksed = false;
+ cpu->cfg.ext_zksh = false;
+ cpu->cfg.ext_zkt = false;
+ cpu->cfg.ext_zve32f = false;
+ cpu->cfg.ext_zve64f = false;
+ cpu->cfg.ext_svinval = false;
+ cpu->cfg.ext_svnapot = false;
+ cpu->cfg.ext_svpbmt = false;
+ }
+
if (cpu->cfg.mmu) {
riscv_set_feature(env, RISCV_FEATURE_MMU);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v2 7/8] target/riscv: Force disable extensions if priv spec version does not match
2022-05-11 14:45 ` [PATCH v2 7/8] target/riscv: Force disable extensions if priv spec version does not match Anup Patel
@ 2022-05-13 18:45 ` Atish Patra
2022-05-17 0:15 ` Alistair Francis
1 sibling, 0 replies; 22+ messages in thread
From: Atish Patra @ 2022-05-13 18:45 UTC (permalink / raw)
To: Anup Patel
Cc: Peter Maydell, Palmer Dabbelt, Alistair Francis, Sagar Karandikar,
Anup Patel, open list:RISC-V, qemu-devel@nongnu.org Developers
On Wed, May 11, 2022 at 7:46 AM Anup Patel <apatel@ventanamicro.com> wrote:
>
> We should disable extensions in riscv_cpu_realize() if minimum required
> priv spec version is not satisfied. This also ensures that machines with
> priv spec v1.11 (or lower) cannot enable H, V, and various multi-letter
> extensions.
>
> Fixes: a775398be2e ("target/riscv: Add isa extenstion strings to the
> device tree")
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> ---
> target/riscv/cpu.c | 34 ++++++++++++++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index f3b61dfd63..25a4ba3e22 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -541,6 +541,40 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> set_priv_version(env, priv_version);
> }
>
> + /* Force disable extensions if priv spec version does not match */
> + if (env->priv_ver < PRIV_VERSION_1_12_0) {
> + cpu->cfg.ext_h = false;
> + cpu->cfg.ext_v = false;
> + cpu->cfg.ext_zfh = false;
> + cpu->cfg.ext_zfhmin = false;
> + cpu->cfg.ext_zfinx = false;
> + cpu->cfg.ext_zhinx = false;
> + cpu->cfg.ext_zhinxmin = false;
> + cpu->cfg.ext_zdinx = false;
> + cpu->cfg.ext_zba = false;
> + cpu->cfg.ext_zbb = false;
> + cpu->cfg.ext_zbc = false;
> + cpu->cfg.ext_zbkb = false;
> + cpu->cfg.ext_zbkc = false;
> + cpu->cfg.ext_zbkx = false;
> + cpu->cfg.ext_zbs = false;
> + cpu->cfg.ext_zk = false;
> + cpu->cfg.ext_zkn = false;
> + cpu->cfg.ext_zknd = false;
> + cpu->cfg.ext_zkne = false;
> + cpu->cfg.ext_zknh = false;
> + cpu->cfg.ext_zkr = false;
> + cpu->cfg.ext_zks = false;
> + cpu->cfg.ext_zksed = false;
> + cpu->cfg.ext_zksh = false;
> + cpu->cfg.ext_zkt = false;
> + cpu->cfg.ext_zve32f = false;
> + cpu->cfg.ext_zve64f = false;
> + cpu->cfg.ext_svinval = false;
> + cpu->cfg.ext_svnapot = false;
> + cpu->cfg.ext_svpbmt = false;
> + }
> +
This will not scale in future with new extensions in the next few
years. We probably need to make more intrusive changes to tie up the
min_version for extensions as well.
A brief look at the "Property" data structure and couldn't find an
easy way to do it.
For this patch,
Reviewed-by: Atish Patra <atishp@rivosinc.com>
> if (cpu->cfg.mmu) {
> riscv_set_feature(env, RISCV_FEATURE_MMU);
> }
> --
> 2.34.1
>
--
Regards,
Atish
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 7/8] target/riscv: Force disable extensions if priv spec version does not match
2022-05-11 14:45 ` [PATCH v2 7/8] target/riscv: Force disable extensions if priv spec version does not match Anup Patel
2022-05-13 18:45 ` Atish Patra
@ 2022-05-17 0:15 ` Alistair Francis
2022-05-19 15:07 ` Anup Patel
1 sibling, 1 reply; 22+ messages in thread
From: Alistair Francis @ 2022-05-17 0:15 UTC (permalink / raw)
To: Anup Patel
Cc: Peter Maydell, Palmer Dabbelt, Alistair Francis, Sagar Karandikar,
Atish Patra, Anup Patel, open list:RISC-V,
qemu-devel@nongnu.org Developers
On Thu, May 12, 2022 at 12:52 AM Anup Patel <apatel@ventanamicro.com> wrote:
>
> We should disable extensions in riscv_cpu_realize() if minimum required
> priv spec version is not satisfied. This also ensures that machines with
> priv spec v1.11 (or lower) cannot enable H, V, and various multi-letter
> extensions.
>
> Fixes: a775398be2e ("target/riscv: Add isa extenstion strings to the
> device tree")
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
This will potentially confuse users as we just disable the extension
without telling them.
Could we not just leave this as is and let users specify the
extensions they want? Then it's up to them to specify the correct
combinations
Alistair
> ---
> target/riscv/cpu.c | 34 ++++++++++++++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index f3b61dfd63..25a4ba3e22 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -541,6 +541,40 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> set_priv_version(env, priv_version);
> }
>
> + /* Force disable extensions if priv spec version does not match */
> + if (env->priv_ver < PRIV_VERSION_1_12_0) {
> + cpu->cfg.ext_h = false;
> + cpu->cfg.ext_v = false;
> + cpu->cfg.ext_zfh = false;
> + cpu->cfg.ext_zfhmin = false;
> + cpu->cfg.ext_zfinx = false;
> + cpu->cfg.ext_zhinx = false;
> + cpu->cfg.ext_zhinxmin = false;
> + cpu->cfg.ext_zdinx = false;
> + cpu->cfg.ext_zba = false;
> + cpu->cfg.ext_zbb = false;
> + cpu->cfg.ext_zbc = false;
> + cpu->cfg.ext_zbkb = false;
> + cpu->cfg.ext_zbkc = false;
> + cpu->cfg.ext_zbkx = false;
> + cpu->cfg.ext_zbs = false;
> + cpu->cfg.ext_zk = false;
> + cpu->cfg.ext_zkn = false;
> + cpu->cfg.ext_zknd = false;
> + cpu->cfg.ext_zkne = false;
> + cpu->cfg.ext_zknh = false;
> + cpu->cfg.ext_zkr = false;
> + cpu->cfg.ext_zks = false;
> + cpu->cfg.ext_zksed = false;
> + cpu->cfg.ext_zksh = false;
> + cpu->cfg.ext_zkt = false;
> + cpu->cfg.ext_zve32f = false;
> + cpu->cfg.ext_zve64f = false;
> + cpu->cfg.ext_svinval = false;
> + cpu->cfg.ext_svnapot = false;
> + cpu->cfg.ext_svpbmt = false;
> + }
> +
> if (cpu->cfg.mmu) {
> riscv_set_feature(env, RISCV_FEATURE_MMU);
> }
> --
> 2.34.1
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 7/8] target/riscv: Force disable extensions if priv spec version does not match
2022-05-17 0:15 ` Alistair Francis
@ 2022-05-19 15:07 ` Anup Patel
2022-05-23 21:52 ` Alistair Francis
0 siblings, 1 reply; 22+ messages in thread
From: Anup Patel @ 2022-05-19 15:07 UTC (permalink / raw)
To: Alistair Francis
Cc: Anup Patel, Peter Maydell, Palmer Dabbelt, Alistair Francis,
Sagar Karandikar, Atish Patra, open list:RISC-V,
qemu-devel@nongnu.org Developers
On Tue, May 17, 2022 at 5:46 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Thu, May 12, 2022 at 12:52 AM Anup Patel <apatel@ventanamicro.com> wrote:
> >
> > We should disable extensions in riscv_cpu_realize() if minimum required
> > priv spec version is not satisfied. This also ensures that machines with
> > priv spec v1.11 (or lower) cannot enable H, V, and various multi-letter
> > extensions.
> >
> > Fixes: a775398be2e ("target/riscv: Add isa extenstion strings to the
> > device tree")
> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
>
> This will potentially confuse users as we just disable the extension
> without telling them.
>
> Could we not just leave this as is and let users specify the
> extensions they want? Then it's up to them to specify the correct
> combinations
The ISA extensions are not independent of the Priv spec version.
For example, we have bits for Sstc, Svpbmt, and Zicbo[m|p|z] extensions
in xenvcfg CSRs which are only available for Priv v1.12 spec.
We can't allow users to enable extensions which don't meet
the Priv spec version requirements.
Regards,
Anup
>
> Alistair
>
> > ---
> > target/riscv/cpu.c | 34 ++++++++++++++++++++++++++++++++++
> > 1 file changed, 34 insertions(+)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index f3b61dfd63..25a4ba3e22 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -541,6 +541,40 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> > set_priv_version(env, priv_version);
> > }
> >
> > + /* Force disable extensions if priv spec version does not match */
> > + if (env->priv_ver < PRIV_VERSION_1_12_0) {
> > + cpu->cfg.ext_h = false;
> > + cpu->cfg.ext_v = false;
> > + cpu->cfg.ext_zfh = false;
> > + cpu->cfg.ext_zfhmin = false;
> > + cpu->cfg.ext_zfinx = false;
> > + cpu->cfg.ext_zhinx = false;
> > + cpu->cfg.ext_zhinxmin = false;
> > + cpu->cfg.ext_zdinx = false;
> > + cpu->cfg.ext_zba = false;
> > + cpu->cfg.ext_zbb = false;
> > + cpu->cfg.ext_zbc = false;
> > + cpu->cfg.ext_zbkb = false;
> > + cpu->cfg.ext_zbkc = false;
> > + cpu->cfg.ext_zbkx = false;
> > + cpu->cfg.ext_zbs = false;
> > + cpu->cfg.ext_zk = false;
> > + cpu->cfg.ext_zkn = false;
> > + cpu->cfg.ext_zknd = false;
> > + cpu->cfg.ext_zkne = false;
> > + cpu->cfg.ext_zknh = false;
> > + cpu->cfg.ext_zkr = false;
> > + cpu->cfg.ext_zks = false;
> > + cpu->cfg.ext_zksed = false;
> > + cpu->cfg.ext_zksh = false;
> > + cpu->cfg.ext_zkt = false;
> > + cpu->cfg.ext_zve32f = false;
> > + cpu->cfg.ext_zve64f = false;
> > + cpu->cfg.ext_svinval = false;
> > + cpu->cfg.ext_svnapot = false;
> > + cpu->cfg.ext_svpbmt = false;
> > + }
> > +
> > if (cpu->cfg.mmu) {
> > riscv_set_feature(env, RISCV_FEATURE_MMU);
> > }
> > --
> > 2.34.1
> >
> >
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 7/8] target/riscv: Force disable extensions if priv spec version does not match
2022-05-19 15:07 ` Anup Patel
@ 2022-05-23 21:52 ` Alistair Francis
2022-05-24 12:10 ` Anup Patel
0 siblings, 1 reply; 22+ messages in thread
From: Alistair Francis @ 2022-05-23 21:52 UTC (permalink / raw)
To: Anup Patel
Cc: Anup Patel, Peter Maydell, Palmer Dabbelt, Alistair Francis,
Sagar Karandikar, Atish Patra, open list:RISC-V,
qemu-devel@nongnu.org Developers
On Fri, May 20, 2022 at 1:07 AM Anup Patel <anup@brainfault.org> wrote:
>
> On Tue, May 17, 2022 at 5:46 AM Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Thu, May 12, 2022 at 12:52 AM Anup Patel <apatel@ventanamicro.com> wrote:
> > >
> > > We should disable extensions in riscv_cpu_realize() if minimum required
> > > priv spec version is not satisfied. This also ensures that machines with
> > > priv spec v1.11 (or lower) cannot enable H, V, and various multi-letter
> > > extensions.
> > >
> > > Fixes: a775398be2e ("target/riscv: Add isa extenstion strings to the
> > > device tree")
> > > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> >
> > This will potentially confuse users as we just disable the extension
> > without telling them.
> >
> > Could we not just leave this as is and let users specify the
> > extensions they want? Then it's up to them to specify the correct
> > combinations
>
> The ISA extensions are not independent of the Priv spec version.
>
> For example, we have bits for Sstc, Svpbmt, and Zicbo[m|p|z] extensions
> in xenvcfg CSRs which are only available for Priv v1.12 spec.
>
> We can't allow users to enable extensions which don't meet
> the Priv spec version requirements.
Fair point. Ok we should at least report a warning if any of these are
set though
Alistair
>
> Regards,
> Anup
>
> >
> > Alistair
> >
> > > ---
> > > target/riscv/cpu.c | 34 ++++++++++++++++++++++++++++++++++
> > > 1 file changed, 34 insertions(+)
> > >
> > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > index f3b61dfd63..25a4ba3e22 100644
> > > --- a/target/riscv/cpu.c
> > > +++ b/target/riscv/cpu.c
> > > @@ -541,6 +541,40 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> > > set_priv_version(env, priv_version);
> > > }
> > >
> > > + /* Force disable extensions if priv spec version does not match */
> > > + if (env->priv_ver < PRIV_VERSION_1_12_0) {
> > > + cpu->cfg.ext_h = false;
> > > + cpu->cfg.ext_v = false;
> > > + cpu->cfg.ext_zfh = false;
> > > + cpu->cfg.ext_zfhmin = false;
> > > + cpu->cfg.ext_zfinx = false;
> > > + cpu->cfg.ext_zhinx = false;
> > > + cpu->cfg.ext_zhinxmin = false;
> > > + cpu->cfg.ext_zdinx = false;
> > > + cpu->cfg.ext_zba = false;
> > > + cpu->cfg.ext_zbb = false;
> > > + cpu->cfg.ext_zbc = false;
> > > + cpu->cfg.ext_zbkb = false;
> > > + cpu->cfg.ext_zbkc = false;
> > > + cpu->cfg.ext_zbkx = false;
> > > + cpu->cfg.ext_zbs = false;
> > > + cpu->cfg.ext_zk = false;
> > > + cpu->cfg.ext_zkn = false;
> > > + cpu->cfg.ext_zknd = false;
> > > + cpu->cfg.ext_zkne = false;
> > > + cpu->cfg.ext_zknh = false;
> > > + cpu->cfg.ext_zkr = false;
> > > + cpu->cfg.ext_zks = false;
> > > + cpu->cfg.ext_zksed = false;
> > > + cpu->cfg.ext_zksh = false;
> > > + cpu->cfg.ext_zkt = false;
> > > + cpu->cfg.ext_zve32f = false;
> > > + cpu->cfg.ext_zve64f = false;
> > > + cpu->cfg.ext_svinval = false;
> > > + cpu->cfg.ext_svnapot = false;
> > > + cpu->cfg.ext_svpbmt = false;
> > > + }
> > > +
> > > if (cpu->cfg.mmu) {
> > > riscv_set_feature(env, RISCV_FEATURE_MMU);
> > > }
> > > --
> > > 2.34.1
> > >
> > >
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 7/8] target/riscv: Force disable extensions if priv spec version does not match
2022-05-23 21:52 ` Alistair Francis
@ 2022-05-24 12:10 ` Anup Patel
0 siblings, 0 replies; 22+ messages in thread
From: Anup Patel @ 2022-05-24 12:10 UTC (permalink / raw)
To: Alistair Francis
Cc: Anup Patel, Peter Maydell, Palmer Dabbelt, Alistair Francis,
Sagar Karandikar, Atish Patra, open list:RISC-V,
qemu-devel@nongnu.org Developers
On Tue, May 24, 2022 at 3:22 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Fri, May 20, 2022 at 1:07 AM Anup Patel <anup@brainfault.org> wrote:
> >
> > On Tue, May 17, 2022 at 5:46 AM Alistair Francis <alistair23@gmail.com> wrote:
> > >
> > > On Thu, May 12, 2022 at 12:52 AM Anup Patel <apatel@ventanamicro.com> wrote:
> > > >
> > > > We should disable extensions in riscv_cpu_realize() if minimum required
> > > > priv spec version is not satisfied. This also ensures that machines with
> > > > priv spec v1.11 (or lower) cannot enable H, V, and various multi-letter
> > > > extensions.
> > > >
> > > > Fixes: a775398be2e ("target/riscv: Add isa extenstion strings to the
> > > > device tree")
> > > > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > >
> > > This will potentially confuse users as we just disable the extension
> > > without telling them.
> > >
> > > Could we not just leave this as is and let users specify the
> > > extensions they want? Then it's up to them to specify the correct
> > > combinations
> >
> > The ISA extensions are not independent of the Priv spec version.
> >
> > For example, we have bits for Sstc, Svpbmt, and Zicbo[m|p|z] extensions
> > in xenvcfg CSRs which are only available for Priv v1.12 spec.
> >
> > We can't allow users to enable extensions which don't meet
> > the Priv spec version requirements.
>
> Fair point. Ok we should at least report a warning if any of these are
> set though
Okay, I will update this patch accordingly.
Regards,
Anup
>
> Alistair
>
> >
> > Regards,
> > Anup
> >
> > >
> > > Alistair
> > >
> > > > ---
> > > > target/riscv/cpu.c | 34 ++++++++++++++++++++++++++++++++++
> > > > 1 file changed, 34 insertions(+)
> > > >
> > > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > > index f3b61dfd63..25a4ba3e22 100644
> > > > --- a/target/riscv/cpu.c
> > > > +++ b/target/riscv/cpu.c
> > > > @@ -541,6 +541,40 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> > > > set_priv_version(env, priv_version);
> > > > }
> > > >
> > > > + /* Force disable extensions if priv spec version does not match */
> > > > + if (env->priv_ver < PRIV_VERSION_1_12_0) {
> > > > + cpu->cfg.ext_h = false;
> > > > + cpu->cfg.ext_v = false;
> > > > + cpu->cfg.ext_zfh = false;
> > > > + cpu->cfg.ext_zfhmin = false;
> > > > + cpu->cfg.ext_zfinx = false;
> > > > + cpu->cfg.ext_zhinx = false;
> > > > + cpu->cfg.ext_zhinxmin = false;
> > > > + cpu->cfg.ext_zdinx = false;
> > > > + cpu->cfg.ext_zba = false;
> > > > + cpu->cfg.ext_zbb = false;
> > > > + cpu->cfg.ext_zbc = false;
> > > > + cpu->cfg.ext_zbkb = false;
> > > > + cpu->cfg.ext_zbkc = false;
> > > > + cpu->cfg.ext_zbkx = false;
> > > > + cpu->cfg.ext_zbs = false;
> > > > + cpu->cfg.ext_zk = false;
> > > > + cpu->cfg.ext_zkn = false;
> > > > + cpu->cfg.ext_zknd = false;
> > > > + cpu->cfg.ext_zkne = false;
> > > > + cpu->cfg.ext_zknh = false;
> > > > + cpu->cfg.ext_zkr = false;
> > > > + cpu->cfg.ext_zks = false;
> > > > + cpu->cfg.ext_zksed = false;
> > > > + cpu->cfg.ext_zksh = false;
> > > > + cpu->cfg.ext_zkt = false;
> > > > + cpu->cfg.ext_zve32f = false;
> > > > + cpu->cfg.ext_zve64f = false;
> > > > + cpu->cfg.ext_svinval = false;
> > > > + cpu->cfg.ext_svnapot = false;
> > > > + cpu->cfg.ext_svpbmt = false;
> > > > + }
> > > > +
> > > > if (cpu->cfg.mmu) {
> > > > riscv_set_feature(env, RISCV_FEATURE_MMU);
> > > > }
> > > > --
> > > > 2.34.1
> > > >
> > > >
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 8/8] hw/riscv: virt: Fix interrupt parent for dynamic platform devices
2022-05-11 14:45 [PATCH v2 0/8] QEMU RISC-V nested virtualization fixes Anup Patel
` (6 preceding siblings ...)
2022-05-11 14:45 ` [PATCH v2 7/8] target/riscv: Force disable extensions if priv spec version does not match Anup Patel
@ 2022-05-11 14:45 ` Anup Patel
2022-05-23 21:38 ` Alistair Francis
2022-05-24 22:19 ` [PATCH v2 0/8] QEMU RISC-V nested virtualization fixes Alistair Francis
8 siblings, 1 reply; 22+ messages in thread
From: Anup Patel @ 2022-05-11 14:45 UTC (permalink / raw)
To: Peter Maydell, Palmer Dabbelt, Alistair Francis, Sagar Karandikar
Cc: Atish Patra, Anup Patel, qemu-riscv, qemu-devel, Anup Patel
When both APLIC and IMSIC are present in virt machine, the APLIC should
be used as parent interrupt controller for dynamic platform devices.
In case of multiple sockets, we should prefer interrupt controller of
socket0 for dynamic platform devices.
Fixes: 3029fab64309 ("hw/riscv: virt: Add support for generating
platform FDT entries")
Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
hw/riscv/virt.c | 25 ++++++++++++-------------
1 file changed, 12 insertions(+), 13 deletions(-)
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 3326f4db96..c576173815 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -478,10 +478,12 @@ static void create_fdt_socket_plic(RISCVVirtState *s,
qemu_fdt_setprop_cell(mc->fdt, plic_name, "phandle",
plic_phandles[socket]);
- platform_bus_add_all_fdt_nodes(mc->fdt, plic_name,
- memmap[VIRT_PLATFORM_BUS].base,
- memmap[VIRT_PLATFORM_BUS].size,
- VIRT_PLATFORM_BUS_IRQ);
+ if (!socket) {
+ platform_bus_add_all_fdt_nodes(mc->fdt, plic_name,
+ memmap[VIRT_PLATFORM_BUS].base,
+ memmap[VIRT_PLATFORM_BUS].size,
+ VIRT_PLATFORM_BUS_IRQ);
+ }
g_free(plic_name);
@@ -561,11 +563,6 @@ static void create_fdt_imsic(RISCVVirtState *s, const MemMapEntry *memmap,
}
qemu_fdt_setprop_cell(mc->fdt, imsic_name, "phandle", *msi_m_phandle);
- platform_bus_add_all_fdt_nodes(mc->fdt, imsic_name,
- memmap[VIRT_PLATFORM_BUS].base,
- memmap[VIRT_PLATFORM_BUS].size,
- VIRT_PLATFORM_BUS_IRQ);
-
g_free(imsic_name);
/* S-level IMSIC node */
@@ -704,10 +701,12 @@ static void create_fdt_socket_aplic(RISCVVirtState *s,
riscv_socket_fdt_write_id(mc, mc->fdt, aplic_name, socket);
qemu_fdt_setprop_cell(mc->fdt, aplic_name, "phandle", aplic_s_phandle);
- platform_bus_add_all_fdt_nodes(mc->fdt, aplic_name,
- memmap[VIRT_PLATFORM_BUS].base,
- memmap[VIRT_PLATFORM_BUS].size,
- VIRT_PLATFORM_BUS_IRQ);
+ if (!socket) {
+ platform_bus_add_all_fdt_nodes(mc->fdt, aplic_name,
+ memmap[VIRT_PLATFORM_BUS].base,
+ memmap[VIRT_PLATFORM_BUS].size,
+ VIRT_PLATFORM_BUS_IRQ);
+ }
g_free(aplic_name);
--
2.34.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v2 8/8] hw/riscv: virt: Fix interrupt parent for dynamic platform devices
2022-05-11 14:45 ` [PATCH v2 8/8] hw/riscv: virt: Fix interrupt parent for dynamic platform devices Anup Patel
@ 2022-05-23 21:38 ` Alistair Francis
0 siblings, 0 replies; 22+ messages in thread
From: Alistair Francis @ 2022-05-23 21:38 UTC (permalink / raw)
To: Anup Patel
Cc: Peter Maydell, Palmer Dabbelt, Alistair Francis, Sagar Karandikar,
Atish Patra, Anup Patel, open list:RISC-V,
qemu-devel@nongnu.org Developers
On Thu, May 12, 2022 at 12:53 AM Anup Patel <apatel@ventanamicro.com> wrote:
>
> When both APLIC and IMSIC are present in virt machine, the APLIC should
> be used as parent interrupt controller for dynamic platform devices.
>
> In case of multiple sockets, we should prefer interrupt controller of
> socket0 for dynamic platform devices.
>
> Fixes: 3029fab64309 ("hw/riscv: virt: Add support for generating
> platform FDT entries")
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> hw/riscv/virt.c | 25 ++++++++++++-------------
> 1 file changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 3326f4db96..c576173815 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -478,10 +478,12 @@ static void create_fdt_socket_plic(RISCVVirtState *s,
> qemu_fdt_setprop_cell(mc->fdt, plic_name, "phandle",
> plic_phandles[socket]);
>
> - platform_bus_add_all_fdt_nodes(mc->fdt, plic_name,
> - memmap[VIRT_PLATFORM_BUS].base,
> - memmap[VIRT_PLATFORM_BUS].size,
> - VIRT_PLATFORM_BUS_IRQ);
> + if (!socket) {
> + platform_bus_add_all_fdt_nodes(mc->fdt, plic_name,
> + memmap[VIRT_PLATFORM_BUS].base,
> + memmap[VIRT_PLATFORM_BUS].size,
> + VIRT_PLATFORM_BUS_IRQ);
> + }
>
> g_free(plic_name);
>
> @@ -561,11 +563,6 @@ static void create_fdt_imsic(RISCVVirtState *s, const MemMapEntry *memmap,
> }
> qemu_fdt_setprop_cell(mc->fdt, imsic_name, "phandle", *msi_m_phandle);
>
> - platform_bus_add_all_fdt_nodes(mc->fdt, imsic_name,
> - memmap[VIRT_PLATFORM_BUS].base,
> - memmap[VIRT_PLATFORM_BUS].size,
> - VIRT_PLATFORM_BUS_IRQ);
> -
> g_free(imsic_name);
>
> /* S-level IMSIC node */
> @@ -704,10 +701,12 @@ static void create_fdt_socket_aplic(RISCVVirtState *s,
> riscv_socket_fdt_write_id(mc, mc->fdt, aplic_name, socket);
> qemu_fdt_setprop_cell(mc->fdt, aplic_name, "phandle", aplic_s_phandle);
>
> - platform_bus_add_all_fdt_nodes(mc->fdt, aplic_name,
> - memmap[VIRT_PLATFORM_BUS].base,
> - memmap[VIRT_PLATFORM_BUS].size,
> - VIRT_PLATFORM_BUS_IRQ);
> + if (!socket) {
> + platform_bus_add_all_fdt_nodes(mc->fdt, aplic_name,
> + memmap[VIRT_PLATFORM_BUS].base,
> + memmap[VIRT_PLATFORM_BUS].size,
> + VIRT_PLATFORM_BUS_IRQ);
> + }
>
> g_free(aplic_name);
>
> --
> 2.34.1
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 0/8] QEMU RISC-V nested virtualization fixes
2022-05-11 14:45 [PATCH v2 0/8] QEMU RISC-V nested virtualization fixes Anup Patel
` (7 preceding siblings ...)
2022-05-11 14:45 ` [PATCH v2 8/8] hw/riscv: virt: Fix interrupt parent for dynamic platform devices Anup Patel
@ 2022-05-24 22:19 ` Alistair Francis
8 siblings, 0 replies; 22+ messages in thread
From: Alistair Francis @ 2022-05-24 22:19 UTC (permalink / raw)
To: Anup Patel
Cc: Peter Maydell, Palmer Dabbelt, Alistair Francis, Sagar Karandikar,
Atish Patra, Anup Patel, open list:RISC-V,
qemu-devel@nongnu.org Developers
On Thu, May 12, 2022 at 12:47 AM Anup Patel <apatel@ventanamicro.com> wrote:
>
> This series does fixes and improvements to have nested virtualization
> on QEMU RISC-V.
>
> These patches can also be found in riscv_nested_fixes_v2 branch at:
> https://github.com/avpatel/qemu.git
>
> The RISC-V nested virtualization was tested on QEMU RISC-V using
> Xvisor RISC-V which has required hypervisor support to run another
> hypervisor as Guest/VM.
>
> Changes since v1:
> - Set write_gva to env->two_stage_lookup which ensures that for
> HS-mode to HS-mode trap write_gva is true only for HLV/HSV
> instructions
> - Included "[PATCH 0/3] QEMU RISC-V priv spec version fixes"
> patches in this series for easy review
> - Re-worked PATCH7 to force disable extensions if required
> priv spec version is not staisfied
> - Added new PATCH8 to fix "aia=aplic-imsic" mode of virt machine
>
> Anup Patel (8):
> target/riscv: Fix csr number based privilege checking
> target/riscv: Fix hstatus.GVA bit setting for traps taken from HS-mode
> target/riscv: Set [m|s]tval for both illegal and virtual instruction
> traps
> target/riscv: Update [m|h]tinst CSR in riscv_cpu_do_interrupt()
> target/riscv: Don't force update priv spec version to latest
> target/riscv: Add dummy mcountinhibit CSR for priv spec v1.11 or
> higher
> target/riscv: Force disable extensions if priv spec version does not
> match
> hw/riscv: virt: Fix interrupt parent for dynamic platform devices
Thanks!
I have applied some of these patches to riscv-to-apply.next
Alistair
>
> hw/riscv/virt.c | 25 +++---
> target/riscv/cpu.c | 46 +++++++++-
> target/riscv/cpu.h | 8 +-
> target/riscv/cpu_bits.h | 3 +
> target/riscv/cpu_helper.c | 172 ++++++++++++++++++++++++++++++++++++--
> target/riscv/csr.c | 10 ++-
> target/riscv/instmap.h | 41 +++++++++
> target/riscv/translate.c | 17 +++-
> 8 files changed, 292 insertions(+), 30 deletions(-)
>
> --
> 2.34.1
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread