* [PATCH v6 1/4] target/riscv: Don't force update priv spec version to latest
2022-06-11 8:01 [PATCH v6 0/4] QEMU RISC-V nested virtualization fixes Anup Patel
@ 2022-06-11 8:01 ` Anup Patel
2022-06-11 8:01 ` [PATCH v6 2/4] target/riscv: Add dummy mcountinhibit CSR for priv spec v1.11 or higher Anup Patel
` (3 subsequent siblings)
4 siblings, 0 replies; 12+ messages in thread
From: Anup Patel @ 2022-06-11 8:01 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, Bin Meng
The riscv_cpu_realize() sets priv spec version 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>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
---
target/riscv/cpu.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 05e6521351..8db0f0bd49 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -173,6 +173,8 @@ static void rv64_base_cpu_init(Object *obj)
/* We set this in the realise function */
set_misa(env, MXL_RV64, 0);
register_cpu_props(DEVICE(obj));
+ /* 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 rv128_base_cpu_init(Object *obj)
/* We set this in the realise function */
set_misa(env, MXL_RV128, 0);
register_cpu_props(DEVICE(obj));
+ /* Set latest version of privileged specification */
+ set_priv_version(env, PRIV_VERSION_1_12_0);
}
#else
static void rv32_base_cpu_init(Object *obj)
@@ -212,6 +216,8 @@ static void rv32_base_cpu_init(Object *obj)
/* We set this in the realise function */
set_misa(env, MXL_RV32, 0);
register_cpu_props(DEVICE(obj));
+ /* Set latest version of privileged specification */
+ set_priv_version(env, PRIV_VERSION_1_12_0);
}
static void rv32_sifive_u_cpu_init(Object *obj)
@@ -524,7 +530,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);
@@ -548,10 +554,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] 12+ messages in thread* [PATCH v6 2/4] target/riscv: Add dummy mcountinhibit CSR for priv spec v1.11 or higher
2022-06-11 8:01 [PATCH v6 0/4] QEMU RISC-V nested virtualization fixes Anup Patel
2022-06-11 8:01 ` [PATCH v6 1/4] target/riscv: Don't force update priv spec version to latest Anup Patel
@ 2022-06-11 8:01 ` Anup Patel
2022-06-11 8:01 ` [PATCH v6 3/4] target/riscv: Update [m|h]tinst CSR in riscv_cpu_do_interrupt() Anup Patel
` (2 subsequent siblings)
4 siblings, 0 replies; 12+ messages in thread
From: Anup Patel @ 2022-06-11 8:01 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, Bin Meng
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 is 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>
Reviewed-by: Bin Meng <bmeng.cn@gmail.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 6dbe9b541f..409a209f14 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] 12+ messages in thread* [PATCH v6 3/4] target/riscv: Update [m|h]tinst CSR in riscv_cpu_do_interrupt()
2022-06-11 8:01 [PATCH v6 0/4] QEMU RISC-V nested virtualization fixes Anup Patel
2022-06-11 8:01 ` [PATCH v6 1/4] target/riscv: Don't force update priv spec version to latest Anup Patel
2022-06-11 8:01 ` [PATCH v6 2/4] target/riscv: Add dummy mcountinhibit CSR for priv spec v1.11 or higher Anup Patel
@ 2022-06-11 8:01 ` Anup Patel
2022-06-27 1:00 ` Alistair Francis
2022-06-11 8:01 ` [PATCH v6 4/4] target/riscv: Force disable extensions if priv spec version does not match Anup Patel
2022-06-27 2:56 ` [PATCH v6 0/4] QEMU RISC-V nested virtualization fixes Alistair Francis
4 siblings, 1 reply; 12+ messages in thread
From: Anup Patel @ 2022-06-11 8:01 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.h | 3 +
target/riscv/cpu_helper.c | 214 ++++++++++++++++++++++++++++++++++++--
target/riscv/instmap.h | 45 ++++++++
3 files changed, 256 insertions(+), 6 deletions(-)
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 7d6397acdf..cac9e1876c 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -271,6 +271,9 @@ struct CPUArchState {
/* Signals whether the current exception occurred with two-stage address
translation active. */
bool two_stage_lookup;
+ /* Signals whether the current exception occurred while doing two-stage
+ address translation for the VS-stage page table walk. */
+ bool two_stage_indirect_lookup;
target_ulong scounteren;
target_ulong mcounteren;
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 4a6700c890..3c8ebecf84 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"
@@ -1057,7 +1058,8 @@ restart:
static void raise_mmu_exception(CPURISCVState *env, target_ulong address,
MMUAccessType access_type, bool pmp_violation,
- bool first_stage, bool two_stage)
+ bool first_stage, bool two_stage,
+ bool two_stage_indirect)
{
CPUState *cs = env_cpu(env);
int page_fault_exceptions, vm;
@@ -1107,6 +1109,7 @@ static void raise_mmu_exception(CPURISCVState *env, target_ulong address,
}
env->badaddr = address;
env->two_stage_lookup = two_stage;
+ env->two_stage_indirect_lookup = two_stage_indirect;
}
hwaddr riscv_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
@@ -1152,6 +1155,7 @@ void riscv_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr,
env->badaddr = addr;
env->two_stage_lookup = riscv_cpu_virt_enabled(env) ||
riscv_cpu_two_stage_lookup(mmu_idx);
+ env->two_stage_indirect_lookup = false;
cpu_loop_exit_restore(cs, retaddr);
}
@@ -1177,6 +1181,7 @@ void riscv_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
env->badaddr = addr;
env->two_stage_lookup = riscv_cpu_virt_enabled(env) ||
riscv_cpu_two_stage_lookup(mmu_idx);
+ env->two_stage_indirect_lookup = false;
cpu_loop_exit_restore(cs, retaddr);
}
@@ -1192,6 +1197,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
bool pmp_violation = false;
bool first_stage_error = true;
bool two_stage_lookup = false;
+ bool two_stage_indirect_error = false;
int ret = TRANSLATE_FAIL;
int mode = mmu_idx;
/* default TLB page size */
@@ -1229,6 +1235,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
*/
if (ret == TRANSLATE_G_STAGE_FAIL) {
first_stage_error = false;
+ two_stage_indirect_error = true;
access_type = MMU_DATA_LOAD;
}
@@ -1312,12 +1319,182 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
raise_mmu_exception(env, address, access_type, pmp_violation,
first_stage_error,
riscv_cpu_virt_enabled(env) ||
- riscv_cpu_two_stage_lookup(mmu_idx));
+ riscv_cpu_two_stage_lookup(mmu_idx),
+ two_stage_indirect_error);
cpu_loop_exit_restore(cs, retaddr);
}
return true;
}
+
+static target_ulong riscv_transformed_insn(CPURISCVState *env,
+ target_ulong insn,
+ bool addr_offset_nonzero,
+ target_ulong taddr)
+{
+ target_ulong xinsn = 0, xinsn_access_bits = 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_access_bits = 3;
+ }
+ break;
+ case OPC_RISC_C_FUNC_LW: /* C.LW */
+ xinsn = OPC_RISC_LW;
+ xinsn = SET_RD(xinsn, GET_C_RS2S(insn));
+ xinsn_access_bits = 2;
+ 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_access_bits = 2;
+ } else { /* C.LD (RV64/RV128) */
+ xinsn = OPC_RISC_LD;
+ xinsn = SET_RD(xinsn, GET_C_RS2S(insn));
+ xinsn_access_bits = 3;
+ }
+ 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_access_bits = 3;
+ }
+ break;
+ case OPC_RISC_C_FUNC_SW: /* C.SW */
+ xinsn = OPC_RISC_SW;
+ xinsn = SET_RS2(xinsn, GET_C_RS2S(insn));
+ xinsn_access_bits = 2;
+ 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_access_bits = 2;
+ } else { /* C.SD (RV64/RV128) */
+ xinsn = OPC_RISC_SD;
+ xinsn = SET_RS2(xinsn, GET_C_RS2S(insn));
+ xinsn_access_bits = 3;
+ }
+ 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_access_bits = 3;
+ }
+ break;
+ case OPC_RISC_C_FUNC_LWSP: /* C.LWSP */
+ xinsn = OPC_RISC_LW;
+ xinsn = SET_RD(xinsn, GET_C_RD(insn));
+ xinsn_access_bits = 2;
+ 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_access_bits = 2;
+ } else { /* C.LDSP (RV64/RV128) */
+ xinsn = OPC_RISC_LD;
+ xinsn = SET_RD(xinsn, GET_C_RD(insn));
+ xinsn_access_bits = 3;
+ }
+ 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_access_bits = 3;
+ }
+ break;
+ case OPC_RISC_C_FUNC_SWSP: /* C.SWSP */
+ xinsn = OPC_RISC_SW;
+ xinsn = SET_RS2(xinsn, GET_C_RS2(insn));
+ xinsn_access_bits = 2;
+ 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_access_bits = 2;
+ } else { /* C.SDSP (RV64/RV128) */
+ xinsn = OPC_RISC_SD;
+ xinsn = SET_RS2(xinsn, GET_C_RS2(insn));
+ xinsn_access_bits = 3;
+ }
+ 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 {
+ /* Transform 32bit (or wider) instructions */
+ switch (MASK_OP_MAJOR(insn)) {
+ case OPC_RISC_ATOMIC:
+ xinsn = insn;
+ xinsn_access_bits = GET_FUNCT3(xinsn);
+ break;
+ case OPC_RISC_LOAD:
+ case OPC_RISC_FP_LOAD:
+ xinsn = insn;
+ xinsn_access_bits = GET_FUNCT3(xinsn);
+ xinsn = SET_I_IMM(xinsn, 0);
+ break;
+ case OPC_RISC_STORE:
+ case OPC_RISC_FP_STORE:
+ xinsn = insn;
+ xinsn_access_bits = GET_FUNCT3(xinsn);
+ xinsn = SET_S_IMM(xinsn, 0);
+ break;
+ case OPC_RISC_SYSTEM:
+ if (MASK_OP_SYSTEM(insn) == OPC_RISC_HLVHSV) {
+ xinsn = insn;
+ xinsn_access_bits = 1 << ((GET_FUNCT7(xinsn) >> 1) & 0x3);
+ }
+ break;
+ default:
+ break;
+ }
+ }
+
+ if (addr_offset_nonzero) {
+ xinsn = SET_RS1(xinsn, taddr & ((1 << xinsn_access_bits) - 1));
+ } else {
+ xinsn = SET_RS1(xinsn, 0);
+ }
+
+ return xinsn;
+}
#endif /* !CONFIG_USER_ONLY */
/*
@@ -1342,6 +1519,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;
@@ -1357,18 +1535,39 @@ 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;
+ if (env->two_stage_indirect_lookup) {
+ /*
+ * special pseudoinstruction for G-stage fault taken while
+ * doing VS-stage page table walk.
+ */
+ tinst = (riscv_cpu_xlen(env) == 32) ? 0x00002000 : 0x00003000;
+ } else {
+ /*
+ * The "Addr. Offset" field in transformed instruction is
+ * non-zero only for misaligned load/store traps.
+ */
+ if (cause == RISCV_EXCP_LOAD_ADDR_MIS ||
+ cause == RISCV_EXCP_STORE_AMO_ACCESS_FAULT) {
+ tinst = riscv_transformed_insn(env, env->bins, true, tval);
+ } else {
+ tinst = riscv_transformed_insn(env, env->bins, false, tval);
+ }
+ }
+ 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;
@@ -1450,6 +1649,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);
@@ -1480,6 +1680,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);
@@ -1492,6 +1693,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
*/
env->two_stage_lookup = false;
+ env->two_stage_indirect_lookup = false;
#endif
cs->exception_index = RISCV_EXCP_NONE; /* mark handled to qemu */
}
diff --git a/target/riscv/instmap.h b/target/riscv/instmap.h
index 40b6d2b64d..f877530576 100644
--- a/target/riscv/instmap.h
+++ b/target/riscv/instmap.h
@@ -184,6 +184,8 @@ enum {
OPC_RISC_CSRRWI = OPC_RISC_SYSTEM | (0x5 << 12),
OPC_RISC_CSRRSI = OPC_RISC_SYSTEM | (0x6 << 12),
OPC_RISC_CSRRCI = OPC_RISC_SYSTEM | (0x7 << 12),
+
+ OPC_RISC_HLVHSV = OPC_RISC_SYSTEM | (0x4 << 12),
};
#define MASK_OP_FP_LOAD(op) (MASK_OP_MAJOR(op) | (op & (0x7 << 12)))
@@ -310,12 +312,20 @@ enum {
| (extract32(inst, 12, 8) << 12) \
| (sextract64(inst, 31, 1) << 20))
+#define GET_FUNCT3(inst) extract32(inst, 12, 3)
+#define GET_FUNCT7(inst) extract32(inst, 25, 7)
#define GET_RM(inst) extract32(inst, 12, 3)
#define GET_RS3(inst) extract32(inst, 27, 5)
#define GET_RS1(inst) extract32(inst, 15, 5)
#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 +356,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 +378,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] 12+ messages in thread* Re: [PATCH v6 3/4] target/riscv: Update [m|h]tinst CSR in riscv_cpu_do_interrupt()
2022-06-11 8:01 ` [PATCH v6 3/4] target/riscv: Update [m|h]tinst CSR in riscv_cpu_do_interrupt() Anup Patel
@ 2022-06-27 1:00 ` Alistair Francis
2022-06-27 16:55 ` dramforever
0 siblings, 1 reply; 12+ messages in thread
From: Alistair Francis @ 2022-06-27 1:00 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 Sat, Jun 11, 2022 at 6:06 PM 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.h | 3 +
> target/riscv/cpu_helper.c | 214 ++++++++++++++++++++++++++++++++++++--
> target/riscv/instmap.h | 45 ++++++++
> 3 files changed, 256 insertions(+), 6 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 7d6397acdf..cac9e1876c 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -271,6 +271,9 @@ struct CPUArchState {
> /* Signals whether the current exception occurred with two-stage address
> translation active. */
> bool two_stage_lookup;
> + /* Signals whether the current exception occurred while doing two-stage
> + address translation for the VS-stage page table walk. */
Wrong comment style, otherwise
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
I can fix this up when I apply the series (unless you need to spin a
new version)
Alistair
> + bool two_stage_indirect_lookup;
>
> target_ulong scounteren;
> target_ulong mcounteren;
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 4a6700c890..3c8ebecf84 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"
> @@ -1057,7 +1058,8 @@ restart:
>
> static void raise_mmu_exception(CPURISCVState *env, target_ulong address,
> MMUAccessType access_type, bool pmp_violation,
> - bool first_stage, bool two_stage)
> + bool first_stage, bool two_stage,
> + bool two_stage_indirect)
> {
> CPUState *cs = env_cpu(env);
> int page_fault_exceptions, vm;
> @@ -1107,6 +1109,7 @@ static void raise_mmu_exception(CPURISCVState *env, target_ulong address,
> }
> env->badaddr = address;
> env->two_stage_lookup = two_stage;
> + env->two_stage_indirect_lookup = two_stage_indirect;
> }
>
> hwaddr riscv_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
> @@ -1152,6 +1155,7 @@ void riscv_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr,
> env->badaddr = addr;
> env->two_stage_lookup = riscv_cpu_virt_enabled(env) ||
> riscv_cpu_two_stage_lookup(mmu_idx);
> + env->two_stage_indirect_lookup = false;
> cpu_loop_exit_restore(cs, retaddr);
> }
>
> @@ -1177,6 +1181,7 @@ void riscv_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
> env->badaddr = addr;
> env->two_stage_lookup = riscv_cpu_virt_enabled(env) ||
> riscv_cpu_two_stage_lookup(mmu_idx);
> + env->two_stage_indirect_lookup = false;
> cpu_loop_exit_restore(cs, retaddr);
> }
>
> @@ -1192,6 +1197,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> bool pmp_violation = false;
> bool first_stage_error = true;
> bool two_stage_lookup = false;
> + bool two_stage_indirect_error = false;
> int ret = TRANSLATE_FAIL;
> int mode = mmu_idx;
> /* default TLB page size */
> @@ -1229,6 +1235,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> */
> if (ret == TRANSLATE_G_STAGE_FAIL) {
> first_stage_error = false;
> + two_stage_indirect_error = true;
> access_type = MMU_DATA_LOAD;
> }
>
> @@ -1312,12 +1319,182 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> raise_mmu_exception(env, address, access_type, pmp_violation,
> first_stage_error,
> riscv_cpu_virt_enabled(env) ||
> - riscv_cpu_two_stage_lookup(mmu_idx));
> + riscv_cpu_two_stage_lookup(mmu_idx),
> + two_stage_indirect_error);
> cpu_loop_exit_restore(cs, retaddr);
> }
>
> return true;
> }
> +
> +static target_ulong riscv_transformed_insn(CPURISCVState *env,
> + target_ulong insn,
> + bool addr_offset_nonzero,
> + target_ulong taddr)
> +{
> + target_ulong xinsn = 0, xinsn_access_bits = 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_access_bits = 3;
> + }
> + break;
> + case OPC_RISC_C_FUNC_LW: /* C.LW */
> + xinsn = OPC_RISC_LW;
> + xinsn = SET_RD(xinsn, GET_C_RS2S(insn));
> + xinsn_access_bits = 2;
> + 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_access_bits = 2;
> + } else { /* C.LD (RV64/RV128) */
> + xinsn = OPC_RISC_LD;
> + xinsn = SET_RD(xinsn, GET_C_RS2S(insn));
> + xinsn_access_bits = 3;
> + }
> + 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_access_bits = 3;
> + }
> + break;
> + case OPC_RISC_C_FUNC_SW: /* C.SW */
> + xinsn = OPC_RISC_SW;
> + xinsn = SET_RS2(xinsn, GET_C_RS2S(insn));
> + xinsn_access_bits = 2;
> + 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_access_bits = 2;
> + } else { /* C.SD (RV64/RV128) */
> + xinsn = OPC_RISC_SD;
> + xinsn = SET_RS2(xinsn, GET_C_RS2S(insn));
> + xinsn_access_bits = 3;
> + }
> + 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_access_bits = 3;
> + }
> + break;
> + case OPC_RISC_C_FUNC_LWSP: /* C.LWSP */
> + xinsn = OPC_RISC_LW;
> + xinsn = SET_RD(xinsn, GET_C_RD(insn));
> + xinsn_access_bits = 2;
> + 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_access_bits = 2;
> + } else { /* C.LDSP (RV64/RV128) */
> + xinsn = OPC_RISC_LD;
> + xinsn = SET_RD(xinsn, GET_C_RD(insn));
> + xinsn_access_bits = 3;
> + }
> + 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_access_bits = 3;
> + }
> + break;
> + case OPC_RISC_C_FUNC_SWSP: /* C.SWSP */
> + xinsn = OPC_RISC_SW;
> + xinsn = SET_RS2(xinsn, GET_C_RS2(insn));
> + xinsn_access_bits = 2;
> + 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_access_bits = 2;
> + } else { /* C.SDSP (RV64/RV128) */
> + xinsn = OPC_RISC_SD;
> + xinsn = SET_RS2(xinsn, GET_C_RS2(insn));
> + xinsn_access_bits = 3;
> + }
> + 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 {
> + /* Transform 32bit (or wider) instructions */
> + switch (MASK_OP_MAJOR(insn)) {
> + case OPC_RISC_ATOMIC:
> + xinsn = insn;
> + xinsn_access_bits = GET_FUNCT3(xinsn);
> + break;
> + case OPC_RISC_LOAD:
> + case OPC_RISC_FP_LOAD:
> + xinsn = insn;
> + xinsn_access_bits = GET_FUNCT3(xinsn);
> + xinsn = SET_I_IMM(xinsn, 0);
> + break;
> + case OPC_RISC_STORE:
> + case OPC_RISC_FP_STORE:
> + xinsn = insn;
> + xinsn_access_bits = GET_FUNCT3(xinsn);
> + xinsn = SET_S_IMM(xinsn, 0);
> + break;
> + case OPC_RISC_SYSTEM:
> + if (MASK_OP_SYSTEM(insn) == OPC_RISC_HLVHSV) {
> + xinsn = insn;
> + xinsn_access_bits = 1 << ((GET_FUNCT7(xinsn) >> 1) & 0x3);
> + }
> + break;
> + default:
> + break;
> + }
> + }
> +
> + if (addr_offset_nonzero) {
> + xinsn = SET_RS1(xinsn, taddr & ((1 << xinsn_access_bits) - 1));
> + } else {
> + xinsn = SET_RS1(xinsn, 0);
> + }
> +
> + return xinsn;
> +}
> #endif /* !CONFIG_USER_ONLY */
>
> /*
> @@ -1342,6 +1519,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;
>
> @@ -1357,18 +1535,39 @@ 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;
> + if (env->two_stage_indirect_lookup) {
> + /*
> + * special pseudoinstruction for G-stage fault taken while
> + * doing VS-stage page table walk.
> + */
> + tinst = (riscv_cpu_xlen(env) == 32) ? 0x00002000 : 0x00003000;
> + } else {
> + /*
> + * The "Addr. Offset" field in transformed instruction is
> + * non-zero only for misaligned load/store traps.
> + */
> + if (cause == RISCV_EXCP_LOAD_ADDR_MIS ||
> + cause == RISCV_EXCP_STORE_AMO_ACCESS_FAULT) {
> + tinst = riscv_transformed_insn(env, env->bins, true, tval);
> + } else {
> + tinst = riscv_transformed_insn(env, env->bins, false, tval);
> + }
> + }
> + 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;
> @@ -1450,6 +1649,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);
> @@ -1480,6 +1680,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);
> @@ -1492,6 +1693,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> */
>
> env->two_stage_lookup = false;
> + env->two_stage_indirect_lookup = false;
> #endif
> cs->exception_index = RISCV_EXCP_NONE; /* mark handled to qemu */
> }
> diff --git a/target/riscv/instmap.h b/target/riscv/instmap.h
> index 40b6d2b64d..f877530576 100644
> --- a/target/riscv/instmap.h
> +++ b/target/riscv/instmap.h
> @@ -184,6 +184,8 @@ enum {
> OPC_RISC_CSRRWI = OPC_RISC_SYSTEM | (0x5 << 12),
> OPC_RISC_CSRRSI = OPC_RISC_SYSTEM | (0x6 << 12),
> OPC_RISC_CSRRCI = OPC_RISC_SYSTEM | (0x7 << 12),
> +
> + OPC_RISC_HLVHSV = OPC_RISC_SYSTEM | (0x4 << 12),
> };
>
> #define MASK_OP_FP_LOAD(op) (MASK_OP_MAJOR(op) | (op & (0x7 << 12)))
> @@ -310,12 +312,20 @@ enum {
> | (extract32(inst, 12, 8) << 12) \
> | (sextract64(inst, 31, 1) << 20))
>
> +#define GET_FUNCT3(inst) extract32(inst, 12, 3)
> +#define GET_FUNCT7(inst) extract32(inst, 25, 7)
> #define GET_RM(inst) extract32(inst, 12, 3)
> #define GET_RS3(inst) extract32(inst, 27, 5)
> #define GET_RS1(inst) extract32(inst, 15, 5)
> #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 +356,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 +378,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] 12+ messages in thread* Re: [PATCH v6 3/4] target/riscv: Update [m|h]tinst CSR in riscv_cpu_do_interrupt()
2022-06-27 1:00 ` Alistair Francis
@ 2022-06-27 16:55 ` dramforever
2022-06-28 9:57 ` Anup Patel
0 siblings, 1 reply; 12+ messages in thread
From: dramforever @ 2022-06-27 16:55 UTC (permalink / raw)
To: Alistair Francis, 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 6/27/22 09:00, Alistair Francis wrote:
> On Sat, Jun 11, 2022 at 6:06 PM 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.h | 3 +
>> target/riscv/cpu_helper.c | 214 ++++++++++++++++++++++++++++++++++++--
>> target/riscv/instmap.h | 45 ++++++++
>> 3 files changed, 256 insertions(+), 6 deletions(-)
>>
>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>> index 7d6397acdf..cac9e1876c 100644
>> --- a/target/riscv/cpu.h
>> +++ b/target/riscv/cpu.h
>> @@ -271,6 +271,9 @@ struct CPUArchState {
>> /* Signals whether the current exception occurred with two-stage address
>> translation active. */
>> bool two_stage_lookup;
>> + /* Signals whether the current exception occurred while doing two-stage
>> + address translation for the VS-stage page table walk. */
> Wrong comment style, otherwise
>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>
> I can fix this up when I apply the series (unless you need to spin a
> new version)
>
> Alistair
Hi Anup Patel and Alistair Francis,
I still have some concerns about this patch. Can you please take a look
before this goes to master?
>> + bool two_stage_indirect_lookup;
>>
>> target_ulong scounteren;
>> target_ulong mcounteren;
>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>> index 4a6700c890..3c8ebecf84 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"
>> @@ -1057,7 +1058,8 @@ restart:
>>
>> static void raise_mmu_exception(CPURISCVState *env, target_ulong address,
>> MMUAccessType access_type, bool pmp_violation,
>> - bool first_stage, bool two_stage)
>> + bool first_stage, bool two_stage,
>> + bool two_stage_indirect)
>> {
>> CPUState *cs = env_cpu(env);
>> int page_fault_exceptions, vm;
>> @@ -1107,6 +1109,7 @@ static void raise_mmu_exception(CPURISCVState *env, target_ulong address,
>> }
>> env->badaddr = address;
>> env->two_stage_lookup = two_stage;
>> + env->two_stage_indirect_lookup = two_stage_indirect;
>> }
>>
>> hwaddr riscv_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
>> @@ -1152,6 +1155,7 @@ void riscv_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr,
>> env->badaddr = addr;
>> env->two_stage_lookup = riscv_cpu_virt_enabled(env) ||
>> riscv_cpu_two_stage_lookup(mmu_idx);
>> + env->two_stage_indirect_lookup = false;
>> cpu_loop_exit_restore(cs, retaddr);
>> }
>>
>> @@ -1177,6 +1181,7 @@ void riscv_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
>> env->badaddr = addr;
>> env->two_stage_lookup = riscv_cpu_virt_enabled(env) ||
>> riscv_cpu_two_stage_lookup(mmu_idx);
>> + env->two_stage_indirect_lookup = false;
>> cpu_loop_exit_restore(cs, retaddr);
>> }
>>
>> @@ -1192,6 +1197,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>> bool pmp_violation = false;
>> bool first_stage_error = true;
>> bool two_stage_lookup = false;
>> + bool two_stage_indirect_error = false;
>> int ret = TRANSLATE_FAIL;
>> int mode = mmu_idx;
>> /* default TLB page size */
>> @@ -1229,6 +1235,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>> */
>> if (ret == TRANSLATE_G_STAGE_FAIL) {
>> first_stage_error = false;
>> + two_stage_indirect_error = true;
>> access_type = MMU_DATA_LOAD;
>> }
>>
>> @@ -1312,12 +1319,182 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>> raise_mmu_exception(env, address, access_type, pmp_violation,
>> first_stage_error,
>> riscv_cpu_virt_enabled(env) ||
>> - riscv_cpu_two_stage_lookup(mmu_idx));
>> + riscv_cpu_two_stage_lookup(mmu_idx),
>> + two_stage_indirect_error);
>> cpu_loop_exit_restore(cs, retaddr);
>> }
>>
>> return true;
>> }
>> +
>> +static target_ulong riscv_transformed_insn(CPURISCVState *env,
>> + target_ulong insn,
>> + bool addr_offset_nonzero,
>> + target_ulong taddr)
>> +{
>> + target_ulong xinsn = 0, xinsn_access_bits = 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_access_bits = 3;
>> + }
>> + break;
>> + case OPC_RISC_C_FUNC_LW: /* C.LW */
>> + xinsn = OPC_RISC_LW;
>> + xinsn = SET_RD(xinsn, GET_C_RS2S(insn));
>> + xinsn_access_bits = 2;
>> + 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_access_bits = 2;
>> + } else { /* C.LD (RV64/RV128) */
>> + xinsn = OPC_RISC_LD;
>> + xinsn = SET_RD(xinsn, GET_C_RS2S(insn));
>> + xinsn_access_bits = 3;
>> + }
>> + 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_access_bits = 3;
>> + }
>> + break;
>> + case OPC_RISC_C_FUNC_SW: /* C.SW */
>> + xinsn = OPC_RISC_SW;
>> + xinsn = SET_RS2(xinsn, GET_C_RS2S(insn));
>> + xinsn_access_bits = 2;
>> + 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_access_bits = 2;
>> + } else { /* C.SD (RV64/RV128) */
>> + xinsn = OPC_RISC_SD;
>> + xinsn = SET_RS2(xinsn, GET_C_RS2S(insn));
>> + xinsn_access_bits = 3;
>> + }
>> + 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_access_bits = 3;
>> + }
>> + break;
>> + case OPC_RISC_C_FUNC_LWSP: /* C.LWSP */
>> + xinsn = OPC_RISC_LW;
>> + xinsn = SET_RD(xinsn, GET_C_RD(insn));
>> + xinsn_access_bits = 2;
>> + 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_access_bits = 2;
>> + } else { /* C.LDSP (RV64/RV128) */
>> + xinsn = OPC_RISC_LD;
>> + xinsn = SET_RD(xinsn, GET_C_RD(insn));
>> + xinsn_access_bits = 3;
>> + }
>> + 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_access_bits = 3;
>> + }
>> + break;
>> + case OPC_RISC_C_FUNC_SWSP: /* C.SWSP */
>> + xinsn = OPC_RISC_SW;
>> + xinsn = SET_RS2(xinsn, GET_C_RS2(insn));
>> + xinsn_access_bits = 2;
>> + 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_access_bits = 2;
>> + } else { /* C.SDSP (RV64/RV128) */
>> + xinsn = OPC_RISC_SD;
>> + xinsn = SET_RS2(xinsn, GET_C_RS2(insn));
>> + xinsn_access_bits = 3;
>> + }
>> + 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 {
>> + /* Transform 32bit (or wider) instructions */
>> + switch (MASK_OP_MAJOR(insn)) {
>> + case OPC_RISC_ATOMIC:
>> + xinsn = insn;
>> + xinsn_access_bits = GET_FUNCT3(xinsn);
>> + break;
>> + case OPC_RISC_LOAD:
>> + case OPC_RISC_FP_LOAD:
>> + xinsn = insn;
>> + xinsn_access_bits = GET_FUNCT3(xinsn);
>> + xinsn = SET_I_IMM(xinsn, 0);
>> + break;
>> + case OPC_RISC_STORE:
>> + case OPC_RISC_FP_STORE:
>> + xinsn = insn;
>> + xinsn_access_bits = GET_FUNCT3(xinsn);
>> + xinsn = SET_S_IMM(xinsn, 0);
>> + break;
>> + case OPC_RISC_SYSTEM:
>> + if (MASK_OP_SYSTEM(insn) == OPC_RISC_HLVHSV) {
>> + xinsn = insn;
>> + xinsn_access_bits = 1 << ((GET_FUNCT7(xinsn) >> 1) & 0x3);
>> + }
>> + break;
>> + default:
>> + break;
>> + }
>> + }
>> +
>> + if (addr_offset_nonzero) {
>> + xinsn = SET_RS1(xinsn, taddr & ((1 << xinsn_access_bits) - 1));
>> + } else {
>> + xinsn = SET_RS1(xinsn, 0);
>> + }
>> +
>> + return xinsn;
>> +}
>> #endif /* !CONFIG_USER_ONLY */
>>
>> /*
>> @@ -1342,6 +1519,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;
>>
>> @@ -1357,18 +1535,39 @@ 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;
>> + if (env->two_stage_indirect_lookup) {
>> + /*
>> + * special pseudoinstruction for G-stage fault taken while
>> + * doing VS-stage page table walk.
>> + */
>> + tinst = (riscv_cpu_xlen(env) == 32) ? 0x00002000 : 0x00003000;
>> + } else {
>> + /*
>> + * The "Addr. Offset" field in transformed instruction is
>> + * non-zero only for misaligned load/store traps.
>> + */
>> + if (cause == RISCV_EXCP_LOAD_ADDR_MIS ||
>> + cause == RISCV_EXCP_STORE_AMO_ACCESS_FAULT) {
>> + tinst = riscv_transformed_insn(env, env->bins, true, tval);
>> + } else {
>> + tinst = riscv_transformed_insn(env, env->bins, false, tval);
>> + }
This doesn't match what I expect.
The privileged spec says the Addr. Offset field should be 'the positive
difference between the faulting virtual address (written to mtval or
stval) and the original virtual address,' and that 'this difference can
be nonzero only for a misaligned memory access.'
To me this means that the memory access is misaligned *and* faults in
the middle of the access. Addr. Offset should then be how many bytes
into the access that it fails. For misaligned address *exceptions* the
access always faults right at the start and nothing is performed, so
Addr. Offset should be zero. This patch does something complete unrelated.
I had raised this point in my comment on the v5 patch, with this
example: For example, if an ld instruction reads 8 bytes starting from
0xa00ffe, and the page 0xa00000 to 0xa00fff is mapped, but 0xa01000 to
0xa01fff is not, a load page fault is raised with [m|s]tval = 0xa01000,
while the original virtual address of the access is 0xa00ffe. The 'Addr.
Offset' in this case is 2, i.e. the difference calculated with (0xa01000
- 0xa00ffe).
In any case I think this is all extremely confusing and a clarification
from the spec authors would be better. I had asked on the GitHub
repository about this[1], but did not receive any response.
[1]: https://github.com/riscv/riscv-isa-manual/issues/853
>> + }
>> + 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;
In case of an Instruction Guest-page Fault due to VS-stage address
translation, i.e. env->two_stage_indirect_lookup is true, htinst should
be a pseudoinstruction instead of zero.
This was raised in a comment in v5 too (where I had mistakenly written
two_stage_lookup instead of two_stage_indirect_lookup), and is still not
fixed in v6.
dramforever
>> @@ -1450,6 +1649,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);
>> @@ -1480,6 +1680,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);
>> @@ -1492,6 +1693,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>> */
>>
>> env->two_stage_lookup = false;
>> + env->two_stage_indirect_lookup = false;
>> #endif
>> cs->exception_index = RISCV_EXCP_NONE; /* mark handled to qemu */
>> }
>> diff --git a/target/riscv/instmap.h b/target/riscv/instmap.h
>> index 40b6d2b64d..f877530576 100644
>> --- a/target/riscv/instmap.h
>> +++ b/target/riscv/instmap.h
>> @@ -184,6 +184,8 @@ enum {
>> OPC_RISC_CSRRWI = OPC_RISC_SYSTEM | (0x5 << 12),
>> OPC_RISC_CSRRSI = OPC_RISC_SYSTEM | (0x6 << 12),
>> OPC_RISC_CSRRCI = OPC_RISC_SYSTEM | (0x7 << 12),
>> +
>> + OPC_RISC_HLVHSV = OPC_RISC_SYSTEM | (0x4 << 12),
>> };
>>
>> #define MASK_OP_FP_LOAD(op) (MASK_OP_MAJOR(op) | (op & (0x7 << 12)))
>> @@ -310,12 +312,20 @@ enum {
>> | (extract32(inst, 12, 8) << 12) \
>> | (sextract64(inst, 31, 1) << 20))
>>
>> +#define GET_FUNCT3(inst) extract32(inst, 12, 3)
>> +#define GET_FUNCT7(inst) extract32(inst, 25, 7)
>> #define GET_RM(inst) extract32(inst, 12, 3)
>> #define GET_RS3(inst) extract32(inst, 27, 5)
>> #define GET_RS1(inst) extract32(inst, 15, 5)
>> #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 +356,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 +378,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] 12+ messages in thread* Re: [PATCH v6 3/4] target/riscv: Update [m|h]tinst CSR in riscv_cpu_do_interrupt()
2022-06-27 16:55 ` dramforever
@ 2022-06-28 9:57 ` Anup Patel
0 siblings, 0 replies; 12+ messages in thread
From: Anup Patel @ 2022-06-28 9:57 UTC (permalink / raw)
To: dramforever
Cc: Alistair Francis, Peter Maydell, Palmer Dabbelt,
Alistair Francis, Sagar Karandikar, Atish Patra, Anup Patel,
open list:RISC-V, qemu-devel@nongnu.org Developers
On Mon, Jun 27, 2022 at 10:25 PM dramforever <dramforever@live.com> wrote:
>
> On 6/27/22 09:00, Alistair Francis wrote:
> > On Sat, Jun 11, 2022 at 6:06 PM 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.h | 3 +
> >> target/riscv/cpu_helper.c | 214 ++++++++++++++++++++++++++++++++++++--
> >> target/riscv/instmap.h | 45 ++++++++
> >> 3 files changed, 256 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> >> index 7d6397acdf..cac9e1876c 100644
> >> --- a/target/riscv/cpu.h
> >> +++ b/target/riscv/cpu.h
> >> @@ -271,6 +271,9 @@ struct CPUArchState {
> >> /* Signals whether the current exception occurred with two-stage address
> >> translation active. */
> >> bool two_stage_lookup;
> >> + /* Signals whether the current exception occurred while doing two-stage
> >> + address translation for the VS-stage page table walk. */
> > Wrong comment style, otherwise
> >
> > Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> >
> > I can fix this up when I apply the series (unless you need to spin a
> > new version)
> >
> > Alistair
>
> Hi Anup Patel and Alistair Francis,
>
> I still have some concerns about this patch. Can you please take a look
> before this goes to master?
>
> >> + bool two_stage_indirect_lookup;
> >>
> >> target_ulong scounteren;
> >> target_ulong mcounteren;
> >> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> >> index 4a6700c890..3c8ebecf84 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"
> >> @@ -1057,7 +1058,8 @@ restart:
> >>
> >> static void raise_mmu_exception(CPURISCVState *env, target_ulong address,
> >> MMUAccessType access_type, bool pmp_violation,
> >> - bool first_stage, bool two_stage)
> >> + bool first_stage, bool two_stage,
> >> + bool two_stage_indirect)
> >> {
> >> CPUState *cs = env_cpu(env);
> >> int page_fault_exceptions, vm;
> >> @@ -1107,6 +1109,7 @@ static void raise_mmu_exception(CPURISCVState *env, target_ulong address,
> >> }
> >> env->badaddr = address;
> >> env->two_stage_lookup = two_stage;
> >> + env->two_stage_indirect_lookup = two_stage_indirect;
> >> }
> >>
> >> hwaddr riscv_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
> >> @@ -1152,6 +1155,7 @@ void riscv_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr,
> >> env->badaddr = addr;
> >> env->two_stage_lookup = riscv_cpu_virt_enabled(env) ||
> >> riscv_cpu_two_stage_lookup(mmu_idx);
> >> + env->two_stage_indirect_lookup = false;
> >> cpu_loop_exit_restore(cs, retaddr);
> >> }
> >>
> >> @@ -1177,6 +1181,7 @@ void riscv_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
> >> env->badaddr = addr;
> >> env->two_stage_lookup = riscv_cpu_virt_enabled(env) ||
> >> riscv_cpu_two_stage_lookup(mmu_idx);
> >> + env->two_stage_indirect_lookup = false;
> >> cpu_loop_exit_restore(cs, retaddr);
> >> }
> >>
> >> @@ -1192,6 +1197,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> >> bool pmp_violation = false;
> >> bool first_stage_error = true;
> >> bool two_stage_lookup = false;
> >> + bool two_stage_indirect_error = false;
> >> int ret = TRANSLATE_FAIL;
> >> int mode = mmu_idx;
> >> /* default TLB page size */
> >> @@ -1229,6 +1235,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> >> */
> >> if (ret == TRANSLATE_G_STAGE_FAIL) {
> >> first_stage_error = false;
> >> + two_stage_indirect_error = true;
> >> access_type = MMU_DATA_LOAD;
> >> }
> >>
> >> @@ -1312,12 +1319,182 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> >> raise_mmu_exception(env, address, access_type, pmp_violation,
> >> first_stage_error,
> >> riscv_cpu_virt_enabled(env) ||
> >> - riscv_cpu_two_stage_lookup(mmu_idx));
> >> + riscv_cpu_two_stage_lookup(mmu_idx),
> >> + two_stage_indirect_error);
> >> cpu_loop_exit_restore(cs, retaddr);
> >> }
> >>
> >> return true;
> >> }
> >> +
> >> +static target_ulong riscv_transformed_insn(CPURISCVState *env,
> >> + target_ulong insn,
> >> + bool addr_offset_nonzero,
> >> + target_ulong taddr)
> >> +{
> >> + target_ulong xinsn = 0, xinsn_access_bits = 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_access_bits = 3;
> >> + }
> >> + break;
> >> + case OPC_RISC_C_FUNC_LW: /* C.LW */
> >> + xinsn = OPC_RISC_LW;
> >> + xinsn = SET_RD(xinsn, GET_C_RS2S(insn));
> >> + xinsn_access_bits = 2;
> >> + 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_access_bits = 2;
> >> + } else { /* C.LD (RV64/RV128) */
> >> + xinsn = OPC_RISC_LD;
> >> + xinsn = SET_RD(xinsn, GET_C_RS2S(insn));
> >> + xinsn_access_bits = 3;
> >> + }
> >> + 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_access_bits = 3;
> >> + }
> >> + break;
> >> + case OPC_RISC_C_FUNC_SW: /* C.SW */
> >> + xinsn = OPC_RISC_SW;
> >> + xinsn = SET_RS2(xinsn, GET_C_RS2S(insn));
> >> + xinsn_access_bits = 2;
> >> + 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_access_bits = 2;
> >> + } else { /* C.SD (RV64/RV128) */
> >> + xinsn = OPC_RISC_SD;
> >> + xinsn = SET_RS2(xinsn, GET_C_RS2S(insn));
> >> + xinsn_access_bits = 3;
> >> + }
> >> + 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_access_bits = 3;
> >> + }
> >> + break;
> >> + case OPC_RISC_C_FUNC_LWSP: /* C.LWSP */
> >> + xinsn = OPC_RISC_LW;
> >> + xinsn = SET_RD(xinsn, GET_C_RD(insn));
> >> + xinsn_access_bits = 2;
> >> + 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_access_bits = 2;
> >> + } else { /* C.LDSP (RV64/RV128) */
> >> + xinsn = OPC_RISC_LD;
> >> + xinsn = SET_RD(xinsn, GET_C_RD(insn));
> >> + xinsn_access_bits = 3;
> >> + }
> >> + 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_access_bits = 3;
> >> + }
> >> + break;
> >> + case OPC_RISC_C_FUNC_SWSP: /* C.SWSP */
> >> + xinsn = OPC_RISC_SW;
> >> + xinsn = SET_RS2(xinsn, GET_C_RS2(insn));
> >> + xinsn_access_bits = 2;
> >> + 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_access_bits = 2;
> >> + } else { /* C.SDSP (RV64/RV128) */
> >> + xinsn = OPC_RISC_SD;
> >> + xinsn = SET_RS2(xinsn, GET_C_RS2(insn));
> >> + xinsn_access_bits = 3;
> >> + }
> >> + 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 {
> >> + /* Transform 32bit (or wider) instructions */
> >> + switch (MASK_OP_MAJOR(insn)) {
> >> + case OPC_RISC_ATOMIC:
> >> + xinsn = insn;
> >> + xinsn_access_bits = GET_FUNCT3(xinsn);
> >> + break;
> >> + case OPC_RISC_LOAD:
> >> + case OPC_RISC_FP_LOAD:
> >> + xinsn = insn;
> >> + xinsn_access_bits = GET_FUNCT3(xinsn);
> >> + xinsn = SET_I_IMM(xinsn, 0);
> >> + break;
> >> + case OPC_RISC_STORE:
> >> + case OPC_RISC_FP_STORE:
> >> + xinsn = insn;
> >> + xinsn_access_bits = GET_FUNCT3(xinsn);
> >> + xinsn = SET_S_IMM(xinsn, 0);
> >> + break;
> >> + case OPC_RISC_SYSTEM:
> >> + if (MASK_OP_SYSTEM(insn) == OPC_RISC_HLVHSV) {
> >> + xinsn = insn;
> >> + xinsn_access_bits = 1 << ((GET_FUNCT7(xinsn) >> 1) & 0x3);
> >> + }
> >> + break;
> >> + default:
> >> + break;
> >> + }
> >> + }
> >> +
> >> + if (addr_offset_nonzero) {
> >> + xinsn = SET_RS1(xinsn, taddr & ((1 << xinsn_access_bits) - 1));
> >> + } else {
> >> + xinsn = SET_RS1(xinsn, 0);
> >> + }
> >> +
> >> + return xinsn;
> >> +}
> >> #endif /* !CONFIG_USER_ONLY */
> >>
> >> /*
> >> @@ -1342,6 +1519,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;
> >>
> >> @@ -1357,18 +1535,39 @@ 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;
> >> + if (env->two_stage_indirect_lookup) {
> >> + /*
> >> + * special pseudoinstruction for G-stage fault taken while
> >> + * doing VS-stage page table walk.
> >> + */
> >> + tinst = (riscv_cpu_xlen(env) == 32) ? 0x00002000 : 0x00003000;
> >> + } else {
> >> + /*
> >> + * The "Addr. Offset" field in transformed instruction is
> >> + * non-zero only for misaligned load/store traps.
> >> + */
> >> + if (cause == RISCV_EXCP_LOAD_ADDR_MIS ||
> >> + cause == RISCV_EXCP_STORE_AMO_ACCESS_FAULT) {
> >> + tinst = riscv_transformed_insn(env, env->bins, true, tval);
> >> + } else {
> >> + tinst = riscv_transformed_insn(env, env->bins, false, tval);
> >> + }
>
> This doesn't match what I expect.
>
> The privileged spec says the Addr. Offset field should be 'the positive
> difference between the faulting virtual address (written to mtval or
> stval) and the original virtual address,' and that 'this difference can
> be nonzero only for a misaligned memory access.'
>
> To me this means that the memory access is misaligned *and* faults in
> the middle of the access. Addr. Offset should then be how many bytes
> into the access that it fails. For misaligned address *exceptions* the
> access always faults right at the start and nothing is performed, so
> Addr. Offset should be zero. This patch does something complete unrelated.
>
> I had raised this point in my comment on the v5 patch, with this
> example: For example, if an ld instruction reads 8 bytes starting from
> 0xa00ffe, and the page 0xa00000 to 0xa00fff is mapped, but 0xa01000 to
> 0xa01fff is not, a load page fault is raised with [m|s]tval = 0xa01000,
> while the original virtual address of the access is 0xa00ffe. The 'Addr.
> Offset' in this case is 2, i.e. the difference calculated with (0xa01000
> - 0xa00ffe).
>
> In any case I think this is all extremely confusing and a clarification
> from the spec authors would be better. I had asked on the GitHub
> repository about this[1], but did not receive any response.
Indeed this is very confusing. Your example above is quite useful.
>
> [1]: https://github.com/riscv/riscv-isa-manual/issues/853
>
> >> + }
> >> + 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;
>
> In case of an Instruction Guest-page Fault due to VS-stage address
> translation, i.e. env->two_stage_indirect_lookup is true, htinst should
> be a pseudoinstruction instead of zero.
>
> This was raised in a comment in v5 too (where I had mistakenly written
> two_stage_lookup instead of two_stage_indirect_lookup), and is still not
> fixed in v6.
Ahh, I see.
>
> dramforever
>
> >> @@ -1450,6 +1649,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);
> >> @@ -1480,6 +1680,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);
> >> @@ -1492,6 +1693,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> >> */
> >>
> >> env->two_stage_lookup = false;
> >> + env->two_stage_indirect_lookup = false;
> >> #endif
> >> cs->exception_index = RISCV_EXCP_NONE; /* mark handled to qemu */
> >> }
> >> diff --git a/target/riscv/instmap.h b/target/riscv/instmap.h
> >> index 40b6d2b64d..f877530576 100644
> >> --- a/target/riscv/instmap.h
> >> +++ b/target/riscv/instmap.h
> >> @@ -184,6 +184,8 @@ enum {
> >> OPC_RISC_CSRRWI = OPC_RISC_SYSTEM | (0x5 << 12),
> >> OPC_RISC_CSRRSI = OPC_RISC_SYSTEM | (0x6 << 12),
> >> OPC_RISC_CSRRCI = OPC_RISC_SYSTEM | (0x7 << 12),
> >> +
> >> + OPC_RISC_HLVHSV = OPC_RISC_SYSTEM | (0x4 << 12),
> >> };
> >>
> >> #define MASK_OP_FP_LOAD(op) (MASK_OP_MAJOR(op) | (op & (0x7 << 12)))
> >> @@ -310,12 +312,20 @@ enum {
> >> | (extract32(inst, 12, 8) << 12) \
> >> | (sextract64(inst, 31, 1) << 20))
> >>
> >> +#define GET_FUNCT3(inst) extract32(inst, 12, 3)
> >> +#define GET_FUNCT7(inst) extract32(inst, 25, 7)
> >> #define GET_RM(inst) extract32(inst, 12, 3)
> >> #define GET_RS3(inst) extract32(inst, 27, 5)
> >> #define GET_RS1(inst) extract32(inst, 15, 5)
> >> #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 +356,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 +378,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
> >>
> >>
> >
I will quickly send v8.
Thanks,
Anup
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v6 4/4] target/riscv: Force disable extensions if priv spec version does not match
2022-06-11 8:01 [PATCH v6 0/4] QEMU RISC-V nested virtualization fixes Anup Patel
` (2 preceding siblings ...)
2022-06-11 8:01 ` [PATCH v6 3/4] target/riscv: Update [m|h]tinst CSR in riscv_cpu_do_interrupt() Anup Patel
@ 2022-06-11 8:01 ` Anup Patel
2022-06-27 1:03 ` Alistair Francis
2022-06-27 23:16 ` Alistair Francis
2022-06-27 2:56 ` [PATCH v6 0/4] QEMU RISC-V nested virtualization fixes Alistair Francis
4 siblings, 2 replies; 12+ messages in thread
From: Anup Patel @ 2022-06-11 8:01 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: a775398be2e9 ("target/riscv: Add isa extenstion strings to the device tree")
Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
target/riscv/cpu.c | 144 +++++++++++++++++++++++++++------------------
1 file changed, 88 insertions(+), 56 deletions(-)
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 8db0f0bd49..a17bc98662 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -43,9 +43,82 @@ static const char riscv_single_letter_exts[] = "IEMAFDQCPVH";
struct isa_ext_data {
const char *name;
- bool enabled;
+ bool multi_letter;
+ int min_version;
+ int ext_enable_offset;
};
+#define ISA_EXT_DATA_ENTRY(_name, _m_letter, _min_ver, _prop) \
+{#_name, _m_letter, _min_ver, offsetof(struct RISCVCPUConfig, _prop)}
+
+/**
+ * Here are the ordering rules of extension naming defined by RISC-V
+ * specification :
+ * 1. All extensions should be separated from other multi-letter extensions
+ * by an underscore.
+ * 2. The first letter following the 'Z' conventionally indicates the most
+ * closely related alphabetical extension category, IMAFDQLCBKJTPVH.
+ * If multiple 'Z' extensions are named, they should be ordered first
+ * by category, then alphabetically within a category.
+ * 3. Standard supervisor-level extensions (starts with 'S') should be
+ * listed after standard unprivileged extensions. If multiple
+ * supervisor-level extensions are listed, they should be ordered
+ * alphabetically.
+ * 4. Non-standard extensions (starts with 'X') must be listed after all
+ * standard extensions. They must be separated from other multi-letter
+ * extensions by an underscore.
+ */
+static const struct isa_ext_data isa_edata_arr[] = {
+ ISA_EXT_DATA_ENTRY(h, false, PRIV_VERSION_1_12_0, ext_h),
+ ISA_EXT_DATA_ENTRY(v, false, PRIV_VERSION_1_12_0, ext_v),
+ ISA_EXT_DATA_ENTRY(zicsr, true, PRIV_VERSION_1_10_0, ext_icsr),
+ ISA_EXT_DATA_ENTRY(zifencei, true, PRIV_VERSION_1_10_0, ext_ifencei),
+ ISA_EXT_DATA_ENTRY(zfh, true, PRIV_VERSION_1_12_0, ext_zfh),
+ ISA_EXT_DATA_ENTRY(zfhmin, true, PRIV_VERSION_1_12_0, ext_zfhmin),
+ ISA_EXT_DATA_ENTRY(zfinx, true, PRIV_VERSION_1_12_0, ext_zfinx),
+ ISA_EXT_DATA_ENTRY(zdinx, true, PRIV_VERSION_1_12_0, ext_zdinx),
+ ISA_EXT_DATA_ENTRY(zba, true, PRIV_VERSION_1_12_0, ext_zba),
+ ISA_EXT_DATA_ENTRY(zbb, true, PRIV_VERSION_1_12_0, ext_zbb),
+ ISA_EXT_DATA_ENTRY(zbc, true, PRIV_VERSION_1_12_0, ext_zbc),
+ ISA_EXT_DATA_ENTRY(zbkb, true, PRIV_VERSION_1_12_0, ext_zbkb),
+ ISA_EXT_DATA_ENTRY(zbkc, true, PRIV_VERSION_1_12_0, ext_zbkc),
+ ISA_EXT_DATA_ENTRY(zbkx, true, PRIV_VERSION_1_12_0, ext_zbkx),
+ ISA_EXT_DATA_ENTRY(zbs, true, PRIV_VERSION_1_12_0, ext_zbs),
+ ISA_EXT_DATA_ENTRY(zk, true, PRIV_VERSION_1_12_0, ext_zk),
+ ISA_EXT_DATA_ENTRY(zkn, true, PRIV_VERSION_1_12_0, ext_zkn),
+ ISA_EXT_DATA_ENTRY(zknd, true, PRIV_VERSION_1_12_0, ext_zknd),
+ ISA_EXT_DATA_ENTRY(zkne, true, PRIV_VERSION_1_12_0, ext_zkne),
+ ISA_EXT_DATA_ENTRY(zknh, true, PRIV_VERSION_1_12_0, ext_zknh),
+ ISA_EXT_DATA_ENTRY(zkr, true, PRIV_VERSION_1_12_0, ext_zkr),
+ ISA_EXT_DATA_ENTRY(zks, true, PRIV_VERSION_1_12_0, ext_zks),
+ ISA_EXT_DATA_ENTRY(zksed, true, PRIV_VERSION_1_12_0, ext_zksed),
+ ISA_EXT_DATA_ENTRY(zksh, true, PRIV_VERSION_1_12_0, ext_zksh),
+ ISA_EXT_DATA_ENTRY(zkt, true, PRIV_VERSION_1_12_0, ext_zkt),
+ ISA_EXT_DATA_ENTRY(zve32f, true, PRIV_VERSION_1_12_0, ext_zve32f),
+ ISA_EXT_DATA_ENTRY(zve64f, true, PRIV_VERSION_1_12_0, ext_zve64f),
+ ISA_EXT_DATA_ENTRY(zhinx, true, PRIV_VERSION_1_12_0, ext_zhinx),
+ ISA_EXT_DATA_ENTRY(zhinxmin, true, PRIV_VERSION_1_12_0, ext_zhinxmin),
+ ISA_EXT_DATA_ENTRY(svinval, true, PRIV_VERSION_1_12_0, ext_svinval),
+ ISA_EXT_DATA_ENTRY(svnapot, true, PRIV_VERSION_1_12_0, ext_svnapot),
+ ISA_EXT_DATA_ENTRY(svpbmt, true, PRIV_VERSION_1_12_0, ext_svpbmt),
+};
+
+static bool isa_ext_is_enabled(RISCVCPU *cpu,
+ const struct isa_ext_data *edata)
+{
+ bool *ext_enabled = (void *)&cpu->cfg + edata->ext_enable_offset;
+
+ return *ext_enabled;
+}
+
+static void isa_ext_update_enabled(RISCVCPU *cpu,
+ const struct isa_ext_data *edata, bool en)
+{
+ bool *ext_enabled = (void *)&cpu->cfg + edata->ext_enable_offset;
+
+ *ext_enabled = en;
+}
+
const char * const riscv_int_regnames[] = {
"x0/zero", "x1/ra", "x2/sp", "x3/gp", "x4/tp", "x5/t0", "x6/t1",
"x7/t2", "x8/s0", "x9/s1", "x10/a0", "x11/a1", "x12/a2", "x13/a3",
@@ -530,7 +603,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 = -1;
+ int i, priv_version = -1;
Error *local_err = NULL;
cpu_exec_realizefn(cs, &local_err);
@@ -558,6 +631,17 @@ 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 */
+ for (i = 0; i < ARRAY_SIZE(isa_edata_arr); i++) {
+ if (isa_ext_is_enabled(cpu, &isa_edata_arr[i]) &&
+ (env->priv_ver < isa_edata_arr[i].min_version)) {
+ isa_ext_update_enabled(cpu, &isa_edata_arr[i], false);
+ 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);
+ }
+ }
+
if (cpu->cfg.mmu) {
riscv_set_feature(env, RISCV_FEATURE_MMU);
}
@@ -1050,67 +1134,15 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data)
device_class_set_props(dc, riscv_cpu_properties);
}
-#define ISA_EDATA_ENTRY(name, prop) {#name, cpu->cfg.prop}
-
static void riscv_isa_string_ext(RISCVCPU *cpu, char **isa_str, int max_str_len)
{
char *old = *isa_str;
char *new = *isa_str;
int i;
- /**
- * Here are the ordering rules of extension naming defined by RISC-V
- * specification :
- * 1. All extensions should be separated from other multi-letter extensions
- * by an underscore.
- * 2. The first letter following the 'Z' conventionally indicates the most
- * closely related alphabetical extension category, IMAFDQLCBKJTPVH.
- * If multiple 'Z' extensions are named, they should be ordered first
- * by category, then alphabetically within a category.
- * 3. Standard supervisor-level extensions (starts with 'S') should be
- * listed after standard unprivileged extensions. If multiple
- * supervisor-level extensions are listed, they should be ordered
- * alphabetically.
- * 4. Non-standard extensions (starts with 'X') must be listed after all
- * standard extensions. They must be separated from other multi-letter
- * extensions by an underscore.
- */
- struct isa_ext_data isa_edata_arr[] = {
- ISA_EDATA_ENTRY(zicsr, ext_icsr),
- ISA_EDATA_ENTRY(zifencei, ext_ifencei),
- ISA_EDATA_ENTRY(zmmul, ext_zmmul),
- ISA_EDATA_ENTRY(zfh, ext_zfh),
- ISA_EDATA_ENTRY(zfhmin, ext_zfhmin),
- ISA_EDATA_ENTRY(zfinx, ext_zfinx),
- ISA_EDATA_ENTRY(zdinx, ext_zdinx),
- ISA_EDATA_ENTRY(zba, ext_zba),
- ISA_EDATA_ENTRY(zbb, ext_zbb),
- ISA_EDATA_ENTRY(zbc, ext_zbc),
- ISA_EDATA_ENTRY(zbkb, ext_zbkb),
- ISA_EDATA_ENTRY(zbkc, ext_zbkc),
- ISA_EDATA_ENTRY(zbkx, ext_zbkx),
- ISA_EDATA_ENTRY(zbs, ext_zbs),
- ISA_EDATA_ENTRY(zk, ext_zk),
- ISA_EDATA_ENTRY(zkn, ext_zkn),
- ISA_EDATA_ENTRY(zknd, ext_zknd),
- ISA_EDATA_ENTRY(zkne, ext_zkne),
- ISA_EDATA_ENTRY(zknh, ext_zknh),
- ISA_EDATA_ENTRY(zkr, ext_zkr),
- ISA_EDATA_ENTRY(zks, ext_zks),
- ISA_EDATA_ENTRY(zksed, ext_zksed),
- ISA_EDATA_ENTRY(zksh, ext_zksh),
- ISA_EDATA_ENTRY(zkt, ext_zkt),
- ISA_EDATA_ENTRY(zve32f, ext_zve32f),
- ISA_EDATA_ENTRY(zve64f, ext_zve64f),
- ISA_EDATA_ENTRY(zhinx, ext_zhinx),
- ISA_EDATA_ENTRY(zhinxmin, ext_zhinxmin),
- ISA_EDATA_ENTRY(svinval, ext_svinval),
- ISA_EDATA_ENTRY(svnapot, ext_svnapot),
- ISA_EDATA_ENTRY(svpbmt, ext_svpbmt),
- };
-
for (i = 0; i < ARRAY_SIZE(isa_edata_arr); i++) {
- if (isa_edata_arr[i].enabled) {
+ if (isa_edata_arr[i].multi_letter &&
+ isa_ext_is_enabled(cpu, &isa_edata_arr[i])) {
new = g_strconcat(old, "_", isa_edata_arr[i].name, NULL);
g_free(old);
old = new;
--
2.34.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v6 4/4] target/riscv: Force disable extensions if priv spec version does not match
2022-06-11 8:01 ` [PATCH v6 4/4] target/riscv: Force disable extensions if priv spec version does not match Anup Patel
@ 2022-06-27 1:03 ` Alistair Francis
2022-06-27 23:16 ` Alistair Francis
1 sibling, 0 replies; 12+ messages in thread
From: Alistair Francis @ 2022-06-27 1:03 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 Sat, Jun 11, 2022 at 6:07 PM 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: a775398be2e9 ("target/riscv: Add isa extenstion strings to the device tree")
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> target/riscv/cpu.c | 144 +++++++++++++++++++++++++++------------------
> 1 file changed, 88 insertions(+), 56 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 8db0f0bd49..a17bc98662 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -43,9 +43,82 @@ static const char riscv_single_letter_exts[] = "IEMAFDQCPVH";
>
> struct isa_ext_data {
> const char *name;
> - bool enabled;
> + bool multi_letter;
> + int min_version;
> + int ext_enable_offset;
> };
>
> +#define ISA_EXT_DATA_ENTRY(_name, _m_letter, _min_ver, _prop) \
> +{#_name, _m_letter, _min_ver, offsetof(struct RISCVCPUConfig, _prop)}
> +
> +/**
> + * Here are the ordering rules of extension naming defined by RISC-V
> + * specification :
> + * 1. All extensions should be separated from other multi-letter extensions
> + * by an underscore.
> + * 2. The first letter following the 'Z' conventionally indicates the most
> + * closely related alphabetical extension category, IMAFDQLCBKJTPVH.
> + * If multiple 'Z' extensions are named, they should be ordered first
> + * by category, then alphabetically within a category.
> + * 3. Standard supervisor-level extensions (starts with 'S') should be
> + * listed after standard unprivileged extensions. If multiple
> + * supervisor-level extensions are listed, they should be ordered
> + * alphabetically.
> + * 4. Non-standard extensions (starts with 'X') must be listed after all
> + * standard extensions. They must be separated from other multi-letter
> + * extensions by an underscore.
> + */
> +static const struct isa_ext_data isa_edata_arr[] = {
> + ISA_EXT_DATA_ENTRY(h, false, PRIV_VERSION_1_12_0, ext_h),
> + ISA_EXT_DATA_ENTRY(v, false, PRIV_VERSION_1_12_0, ext_v),
> + ISA_EXT_DATA_ENTRY(zicsr, true, PRIV_VERSION_1_10_0, ext_icsr),
> + ISA_EXT_DATA_ENTRY(zifencei, true, PRIV_VERSION_1_10_0, ext_ifencei),
> + ISA_EXT_DATA_ENTRY(zfh, true, PRIV_VERSION_1_12_0, ext_zfh),
> + ISA_EXT_DATA_ENTRY(zfhmin, true, PRIV_VERSION_1_12_0, ext_zfhmin),
> + ISA_EXT_DATA_ENTRY(zfinx, true, PRIV_VERSION_1_12_0, ext_zfinx),
> + ISA_EXT_DATA_ENTRY(zdinx, true, PRIV_VERSION_1_12_0, ext_zdinx),
> + ISA_EXT_DATA_ENTRY(zba, true, PRIV_VERSION_1_12_0, ext_zba),
> + ISA_EXT_DATA_ENTRY(zbb, true, PRIV_VERSION_1_12_0, ext_zbb),
> + ISA_EXT_DATA_ENTRY(zbc, true, PRIV_VERSION_1_12_0, ext_zbc),
> + ISA_EXT_DATA_ENTRY(zbkb, true, PRIV_VERSION_1_12_0, ext_zbkb),
> + ISA_EXT_DATA_ENTRY(zbkc, true, PRIV_VERSION_1_12_0, ext_zbkc),
> + ISA_EXT_DATA_ENTRY(zbkx, true, PRIV_VERSION_1_12_0, ext_zbkx),
> + ISA_EXT_DATA_ENTRY(zbs, true, PRIV_VERSION_1_12_0, ext_zbs),
> + ISA_EXT_DATA_ENTRY(zk, true, PRIV_VERSION_1_12_0, ext_zk),
> + ISA_EXT_DATA_ENTRY(zkn, true, PRIV_VERSION_1_12_0, ext_zkn),
> + ISA_EXT_DATA_ENTRY(zknd, true, PRIV_VERSION_1_12_0, ext_zknd),
> + ISA_EXT_DATA_ENTRY(zkne, true, PRIV_VERSION_1_12_0, ext_zkne),
> + ISA_EXT_DATA_ENTRY(zknh, true, PRIV_VERSION_1_12_0, ext_zknh),
> + ISA_EXT_DATA_ENTRY(zkr, true, PRIV_VERSION_1_12_0, ext_zkr),
> + ISA_EXT_DATA_ENTRY(zks, true, PRIV_VERSION_1_12_0, ext_zks),
> + ISA_EXT_DATA_ENTRY(zksed, true, PRIV_VERSION_1_12_0, ext_zksed),
> + ISA_EXT_DATA_ENTRY(zksh, true, PRIV_VERSION_1_12_0, ext_zksh),
> + ISA_EXT_DATA_ENTRY(zkt, true, PRIV_VERSION_1_12_0, ext_zkt),
> + ISA_EXT_DATA_ENTRY(zve32f, true, PRIV_VERSION_1_12_0, ext_zve32f),
> + ISA_EXT_DATA_ENTRY(zve64f, true, PRIV_VERSION_1_12_0, ext_zve64f),
> + ISA_EXT_DATA_ENTRY(zhinx, true, PRIV_VERSION_1_12_0, ext_zhinx),
> + ISA_EXT_DATA_ENTRY(zhinxmin, true, PRIV_VERSION_1_12_0, ext_zhinxmin),
> + ISA_EXT_DATA_ENTRY(svinval, true, PRIV_VERSION_1_12_0, ext_svinval),
> + ISA_EXT_DATA_ENTRY(svnapot, true, PRIV_VERSION_1_12_0, ext_svnapot),
> + ISA_EXT_DATA_ENTRY(svpbmt, true, PRIV_VERSION_1_12_0, ext_svpbmt),
> +};
> +
> +static bool isa_ext_is_enabled(RISCVCPU *cpu,
> + const struct isa_ext_data *edata)
> +{
> + bool *ext_enabled = (void *)&cpu->cfg + edata->ext_enable_offset;
> +
> + return *ext_enabled;
> +}
> +
> +static void isa_ext_update_enabled(RISCVCPU *cpu,
> + const struct isa_ext_data *edata, bool en)
> +{
> + bool *ext_enabled = (void *)&cpu->cfg + edata->ext_enable_offset;
> +
> + *ext_enabled = en;
> +}
> +
> const char * const riscv_int_regnames[] = {
> "x0/zero", "x1/ra", "x2/sp", "x3/gp", "x4/tp", "x5/t0", "x6/t1",
> "x7/t2", "x8/s0", "x9/s1", "x10/a0", "x11/a1", "x12/a2", "x13/a3",
> @@ -530,7 +603,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 = -1;
> + int i, priv_version = -1;
> Error *local_err = NULL;
>
> cpu_exec_realizefn(cs, &local_err);
> @@ -558,6 +631,17 @@ 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 */
> + for (i = 0; i < ARRAY_SIZE(isa_edata_arr); i++) {
> + if (isa_ext_is_enabled(cpu, &isa_edata_arr[i]) &&
> + (env->priv_ver < isa_edata_arr[i].min_version)) {
> + isa_ext_update_enabled(cpu, &isa_edata_arr[i], false);
> + 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);
> + }
> + }
> +
> if (cpu->cfg.mmu) {
> riscv_set_feature(env, RISCV_FEATURE_MMU);
> }
> @@ -1050,67 +1134,15 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data)
> device_class_set_props(dc, riscv_cpu_properties);
> }
>
> -#define ISA_EDATA_ENTRY(name, prop) {#name, cpu->cfg.prop}
> -
> static void riscv_isa_string_ext(RISCVCPU *cpu, char **isa_str, int max_str_len)
> {
> char *old = *isa_str;
> char *new = *isa_str;
> int i;
>
> - /**
> - * Here are the ordering rules of extension naming defined by RISC-V
> - * specification :
> - * 1. All extensions should be separated from other multi-letter extensions
> - * by an underscore.
> - * 2. The first letter following the 'Z' conventionally indicates the most
> - * closely related alphabetical extension category, IMAFDQLCBKJTPVH.
> - * If multiple 'Z' extensions are named, they should be ordered first
> - * by category, then alphabetically within a category.
> - * 3. Standard supervisor-level extensions (starts with 'S') should be
> - * listed after standard unprivileged extensions. If multiple
> - * supervisor-level extensions are listed, they should be ordered
> - * alphabetically.
> - * 4. Non-standard extensions (starts with 'X') must be listed after all
> - * standard extensions. They must be separated from other multi-letter
> - * extensions by an underscore.
> - */
> - struct isa_ext_data isa_edata_arr[] = {
> - ISA_EDATA_ENTRY(zicsr, ext_icsr),
> - ISA_EDATA_ENTRY(zifencei, ext_ifencei),
> - ISA_EDATA_ENTRY(zmmul, ext_zmmul),
> - ISA_EDATA_ENTRY(zfh, ext_zfh),
> - ISA_EDATA_ENTRY(zfhmin, ext_zfhmin),
> - ISA_EDATA_ENTRY(zfinx, ext_zfinx),
> - ISA_EDATA_ENTRY(zdinx, ext_zdinx),
> - ISA_EDATA_ENTRY(zba, ext_zba),
> - ISA_EDATA_ENTRY(zbb, ext_zbb),
> - ISA_EDATA_ENTRY(zbc, ext_zbc),
> - ISA_EDATA_ENTRY(zbkb, ext_zbkb),
> - ISA_EDATA_ENTRY(zbkc, ext_zbkc),
> - ISA_EDATA_ENTRY(zbkx, ext_zbkx),
> - ISA_EDATA_ENTRY(zbs, ext_zbs),
> - ISA_EDATA_ENTRY(zk, ext_zk),
> - ISA_EDATA_ENTRY(zkn, ext_zkn),
> - ISA_EDATA_ENTRY(zknd, ext_zknd),
> - ISA_EDATA_ENTRY(zkne, ext_zkne),
> - ISA_EDATA_ENTRY(zknh, ext_zknh),
> - ISA_EDATA_ENTRY(zkr, ext_zkr),
> - ISA_EDATA_ENTRY(zks, ext_zks),
> - ISA_EDATA_ENTRY(zksed, ext_zksed),
> - ISA_EDATA_ENTRY(zksh, ext_zksh),
> - ISA_EDATA_ENTRY(zkt, ext_zkt),
> - ISA_EDATA_ENTRY(zve32f, ext_zve32f),
> - ISA_EDATA_ENTRY(zve64f, ext_zve64f),
> - ISA_EDATA_ENTRY(zhinx, ext_zhinx),
> - ISA_EDATA_ENTRY(zhinxmin, ext_zhinxmin),
> - ISA_EDATA_ENTRY(svinval, ext_svinval),
> - ISA_EDATA_ENTRY(svnapot, ext_svnapot),
> - ISA_EDATA_ENTRY(svpbmt, ext_svpbmt),
> - };
> -
> for (i = 0; i < ARRAY_SIZE(isa_edata_arr); i++) {
> - if (isa_edata_arr[i].enabled) {
> + if (isa_edata_arr[i].multi_letter &&
> + isa_ext_is_enabled(cpu, &isa_edata_arr[i])) {
> new = g_strconcat(old, "_", isa_edata_arr[i].name, NULL);
> g_free(old);
> old = new;
> --
> 2.34.1
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v6 4/4] target/riscv: Force disable extensions if priv spec version does not match
2022-06-11 8:01 ` [PATCH v6 4/4] target/riscv: Force disable extensions if priv spec version does not match Anup Patel
2022-06-27 1:03 ` Alistair Francis
@ 2022-06-27 23:16 ` Alistair Francis
2022-06-28 3:45 ` Anup Patel
1 sibling, 1 reply; 12+ messages in thread
From: Alistair Francis @ 2022-06-27 23:16 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 Sat, Jun 11, 2022 at 6:07 PM 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: a775398be2e9 ("target/riscv: Add isa extenstion strings to the device tree")
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
This fails to build
../target/riscv/cpu.c: In function 'riscv_cpu_realize':
../target/riscv/cpu.c:641:66: error: 'CPURISCVState' {aka 'struct
CPUArchState'} has no member named 'mhartid'
641 | isa_edata_arr[i].name, (unsigned
long)env->mhartid);
| ^~
Alistair
> ---
> target/riscv/cpu.c | 144 +++++++++++++++++++++++++++------------------
> 1 file changed, 88 insertions(+), 56 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 8db0f0bd49..a17bc98662 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -43,9 +43,82 @@ static const char riscv_single_letter_exts[] = "IEMAFDQCPVH";
>
> struct isa_ext_data {
> const char *name;
> - bool enabled;
> + bool multi_letter;
> + int min_version;
> + int ext_enable_offset;
> };
>
> +#define ISA_EXT_DATA_ENTRY(_name, _m_letter, _min_ver, _prop) \
> +{#_name, _m_letter, _min_ver, offsetof(struct RISCVCPUConfig, _prop)}
> +
> +/**
> + * Here are the ordering rules of extension naming defined by RISC-V
> + * specification :
> + * 1. All extensions should be separated from other multi-letter extensions
> + * by an underscore.
> + * 2. The first letter following the 'Z' conventionally indicates the most
> + * closely related alphabetical extension category, IMAFDQLCBKJTPVH.
> + * If multiple 'Z' extensions are named, they should be ordered first
> + * by category, then alphabetically within a category.
> + * 3. Standard supervisor-level extensions (starts with 'S') should be
> + * listed after standard unprivileged extensions. If multiple
> + * supervisor-level extensions are listed, they should be ordered
> + * alphabetically.
> + * 4. Non-standard extensions (starts with 'X') must be listed after all
> + * standard extensions. They must be separated from other multi-letter
> + * extensions by an underscore.
> + */
> +static const struct isa_ext_data isa_edata_arr[] = {
> + ISA_EXT_DATA_ENTRY(h, false, PRIV_VERSION_1_12_0, ext_h),
> + ISA_EXT_DATA_ENTRY(v, false, PRIV_VERSION_1_12_0, ext_v),
> + ISA_EXT_DATA_ENTRY(zicsr, true, PRIV_VERSION_1_10_0, ext_icsr),
> + ISA_EXT_DATA_ENTRY(zifencei, true, PRIV_VERSION_1_10_0, ext_ifencei),
> + ISA_EXT_DATA_ENTRY(zfh, true, PRIV_VERSION_1_12_0, ext_zfh),
> + ISA_EXT_DATA_ENTRY(zfhmin, true, PRIV_VERSION_1_12_0, ext_zfhmin),
> + ISA_EXT_DATA_ENTRY(zfinx, true, PRIV_VERSION_1_12_0, ext_zfinx),
> + ISA_EXT_DATA_ENTRY(zdinx, true, PRIV_VERSION_1_12_0, ext_zdinx),
> + ISA_EXT_DATA_ENTRY(zba, true, PRIV_VERSION_1_12_0, ext_zba),
> + ISA_EXT_DATA_ENTRY(zbb, true, PRIV_VERSION_1_12_0, ext_zbb),
> + ISA_EXT_DATA_ENTRY(zbc, true, PRIV_VERSION_1_12_0, ext_zbc),
> + ISA_EXT_DATA_ENTRY(zbkb, true, PRIV_VERSION_1_12_0, ext_zbkb),
> + ISA_EXT_DATA_ENTRY(zbkc, true, PRIV_VERSION_1_12_0, ext_zbkc),
> + ISA_EXT_DATA_ENTRY(zbkx, true, PRIV_VERSION_1_12_0, ext_zbkx),
> + ISA_EXT_DATA_ENTRY(zbs, true, PRIV_VERSION_1_12_0, ext_zbs),
> + ISA_EXT_DATA_ENTRY(zk, true, PRIV_VERSION_1_12_0, ext_zk),
> + ISA_EXT_DATA_ENTRY(zkn, true, PRIV_VERSION_1_12_0, ext_zkn),
> + ISA_EXT_DATA_ENTRY(zknd, true, PRIV_VERSION_1_12_0, ext_zknd),
> + ISA_EXT_DATA_ENTRY(zkne, true, PRIV_VERSION_1_12_0, ext_zkne),
> + ISA_EXT_DATA_ENTRY(zknh, true, PRIV_VERSION_1_12_0, ext_zknh),
> + ISA_EXT_DATA_ENTRY(zkr, true, PRIV_VERSION_1_12_0, ext_zkr),
> + ISA_EXT_DATA_ENTRY(zks, true, PRIV_VERSION_1_12_0, ext_zks),
> + ISA_EXT_DATA_ENTRY(zksed, true, PRIV_VERSION_1_12_0, ext_zksed),
> + ISA_EXT_DATA_ENTRY(zksh, true, PRIV_VERSION_1_12_0, ext_zksh),
> + ISA_EXT_DATA_ENTRY(zkt, true, PRIV_VERSION_1_12_0, ext_zkt),
> + ISA_EXT_DATA_ENTRY(zve32f, true, PRIV_VERSION_1_12_0, ext_zve32f),
> + ISA_EXT_DATA_ENTRY(zve64f, true, PRIV_VERSION_1_12_0, ext_zve64f),
> + ISA_EXT_DATA_ENTRY(zhinx, true, PRIV_VERSION_1_12_0, ext_zhinx),
> + ISA_EXT_DATA_ENTRY(zhinxmin, true, PRIV_VERSION_1_12_0, ext_zhinxmin),
> + ISA_EXT_DATA_ENTRY(svinval, true, PRIV_VERSION_1_12_0, ext_svinval),
> + ISA_EXT_DATA_ENTRY(svnapot, true, PRIV_VERSION_1_12_0, ext_svnapot),
> + ISA_EXT_DATA_ENTRY(svpbmt, true, PRIV_VERSION_1_12_0, ext_svpbmt),
> +};
> +
> +static bool isa_ext_is_enabled(RISCVCPU *cpu,
> + const struct isa_ext_data *edata)
> +{
> + bool *ext_enabled = (void *)&cpu->cfg + edata->ext_enable_offset;
> +
> + return *ext_enabled;
> +}
> +
> +static void isa_ext_update_enabled(RISCVCPU *cpu,
> + const struct isa_ext_data *edata, bool en)
> +{
> + bool *ext_enabled = (void *)&cpu->cfg + edata->ext_enable_offset;
> +
> + *ext_enabled = en;
> +}
> +
> const char * const riscv_int_regnames[] = {
> "x0/zero", "x1/ra", "x2/sp", "x3/gp", "x4/tp", "x5/t0", "x6/t1",
> "x7/t2", "x8/s0", "x9/s1", "x10/a0", "x11/a1", "x12/a2", "x13/a3",
> @@ -530,7 +603,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 = -1;
> + int i, priv_version = -1;
> Error *local_err = NULL;
>
> cpu_exec_realizefn(cs, &local_err);
> @@ -558,6 +631,17 @@ 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 */
> + for (i = 0; i < ARRAY_SIZE(isa_edata_arr); i++) {
> + if (isa_ext_is_enabled(cpu, &isa_edata_arr[i]) &&
> + (env->priv_ver < isa_edata_arr[i].min_version)) {
> + isa_ext_update_enabled(cpu, &isa_edata_arr[i], false);
> + 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);
> + }
> + }
> +
> if (cpu->cfg.mmu) {
> riscv_set_feature(env, RISCV_FEATURE_MMU);
> }
> @@ -1050,67 +1134,15 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data)
> device_class_set_props(dc, riscv_cpu_properties);
> }
>
> -#define ISA_EDATA_ENTRY(name, prop) {#name, cpu->cfg.prop}
> -
> static void riscv_isa_string_ext(RISCVCPU *cpu, char **isa_str, int max_str_len)
> {
> char *old = *isa_str;
> char *new = *isa_str;
> int i;
>
> - /**
> - * Here are the ordering rules of extension naming defined by RISC-V
> - * specification :
> - * 1. All extensions should be separated from other multi-letter extensions
> - * by an underscore.
> - * 2. The first letter following the 'Z' conventionally indicates the most
> - * closely related alphabetical extension category, IMAFDQLCBKJTPVH.
> - * If multiple 'Z' extensions are named, they should be ordered first
> - * by category, then alphabetically within a category.
> - * 3. Standard supervisor-level extensions (starts with 'S') should be
> - * listed after standard unprivileged extensions. If multiple
> - * supervisor-level extensions are listed, they should be ordered
> - * alphabetically.
> - * 4. Non-standard extensions (starts with 'X') must be listed after all
> - * standard extensions. They must be separated from other multi-letter
> - * extensions by an underscore.
> - */
> - struct isa_ext_data isa_edata_arr[] = {
> - ISA_EDATA_ENTRY(zicsr, ext_icsr),
> - ISA_EDATA_ENTRY(zifencei, ext_ifencei),
> - ISA_EDATA_ENTRY(zmmul, ext_zmmul),
> - ISA_EDATA_ENTRY(zfh, ext_zfh),
> - ISA_EDATA_ENTRY(zfhmin, ext_zfhmin),
> - ISA_EDATA_ENTRY(zfinx, ext_zfinx),
> - ISA_EDATA_ENTRY(zdinx, ext_zdinx),
> - ISA_EDATA_ENTRY(zba, ext_zba),
> - ISA_EDATA_ENTRY(zbb, ext_zbb),
> - ISA_EDATA_ENTRY(zbc, ext_zbc),
> - ISA_EDATA_ENTRY(zbkb, ext_zbkb),
> - ISA_EDATA_ENTRY(zbkc, ext_zbkc),
> - ISA_EDATA_ENTRY(zbkx, ext_zbkx),
> - ISA_EDATA_ENTRY(zbs, ext_zbs),
> - ISA_EDATA_ENTRY(zk, ext_zk),
> - ISA_EDATA_ENTRY(zkn, ext_zkn),
> - ISA_EDATA_ENTRY(zknd, ext_zknd),
> - ISA_EDATA_ENTRY(zkne, ext_zkne),
> - ISA_EDATA_ENTRY(zknh, ext_zknh),
> - ISA_EDATA_ENTRY(zkr, ext_zkr),
> - ISA_EDATA_ENTRY(zks, ext_zks),
> - ISA_EDATA_ENTRY(zksed, ext_zksed),
> - ISA_EDATA_ENTRY(zksh, ext_zksh),
> - ISA_EDATA_ENTRY(zkt, ext_zkt),
> - ISA_EDATA_ENTRY(zve32f, ext_zve32f),
> - ISA_EDATA_ENTRY(zve64f, ext_zve64f),
> - ISA_EDATA_ENTRY(zhinx, ext_zhinx),
> - ISA_EDATA_ENTRY(zhinxmin, ext_zhinxmin),
> - ISA_EDATA_ENTRY(svinval, ext_svinval),
> - ISA_EDATA_ENTRY(svnapot, ext_svnapot),
> - ISA_EDATA_ENTRY(svpbmt, ext_svpbmt),
> - };
> -
> for (i = 0; i < ARRAY_SIZE(isa_edata_arr); i++) {
> - if (isa_edata_arr[i].enabled) {
> + if (isa_edata_arr[i].multi_letter &&
> + isa_ext_is_enabled(cpu, &isa_edata_arr[i])) {
> new = g_strconcat(old, "_", isa_edata_arr[i].name, NULL);
> g_free(old);
> old = new;
> --
> 2.34.1
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v6 4/4] target/riscv: Force disable extensions if priv spec version does not match
2022-06-27 23:16 ` Alistair Francis
@ 2022-06-28 3:45 ` Anup Patel
0 siblings, 0 replies; 12+ messages in thread
From: Anup Patel @ 2022-06-28 3:45 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, Jun 28, 2022 at 4:47 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Sat, Jun 11, 2022 at 6:07 PM 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: a775398be2e9 ("target/riscv: Add isa extenstion strings to the device tree")
> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
>
> This fails to build
>
> ../target/riscv/cpu.c: In function 'riscv_cpu_realize':
> ../target/riscv/cpu.c:641:66: error: 'CPURISCVState' {aka 'struct
> CPUArchState'} has no member named 'mhartid'
> 641 | isa_edata_arr[i].name, (unsigned
> long)env->mhartid);
> | ^~
I missed testing riscv64-linux-user build.
I will fix and quickly send v7.
Regards,
Anup
>
> Alistair
>
> > ---
> > target/riscv/cpu.c | 144 +++++++++++++++++++++++++++------------------
> > 1 file changed, 88 insertions(+), 56 deletions(-)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index 8db0f0bd49..a17bc98662 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -43,9 +43,82 @@ static const char riscv_single_letter_exts[] = "IEMAFDQCPVH";
> >
> > struct isa_ext_data {
> > const char *name;
> > - bool enabled;
> > + bool multi_letter;
> > + int min_version;
> > + int ext_enable_offset;
> > };
> >
> > +#define ISA_EXT_DATA_ENTRY(_name, _m_letter, _min_ver, _prop) \
> > +{#_name, _m_letter, _min_ver, offsetof(struct RISCVCPUConfig, _prop)}
> > +
> > +/**
> > + * Here are the ordering rules of extension naming defined by RISC-V
> > + * specification :
> > + * 1. All extensions should be separated from other multi-letter extensions
> > + * by an underscore.
> > + * 2. The first letter following the 'Z' conventionally indicates the most
> > + * closely related alphabetical extension category, IMAFDQLCBKJTPVH.
> > + * If multiple 'Z' extensions are named, they should be ordered first
> > + * by category, then alphabetically within a category.
> > + * 3. Standard supervisor-level extensions (starts with 'S') should be
> > + * listed after standard unprivileged extensions. If multiple
> > + * supervisor-level extensions are listed, they should be ordered
> > + * alphabetically.
> > + * 4. Non-standard extensions (starts with 'X') must be listed after all
> > + * standard extensions. They must be separated from other multi-letter
> > + * extensions by an underscore.
> > + */
> > +static const struct isa_ext_data isa_edata_arr[] = {
> > + ISA_EXT_DATA_ENTRY(h, false, PRIV_VERSION_1_12_0, ext_h),
> > + ISA_EXT_DATA_ENTRY(v, false, PRIV_VERSION_1_12_0, ext_v),
> > + ISA_EXT_DATA_ENTRY(zicsr, true, PRIV_VERSION_1_10_0, ext_icsr),
> > + ISA_EXT_DATA_ENTRY(zifencei, true, PRIV_VERSION_1_10_0, ext_ifencei),
> > + ISA_EXT_DATA_ENTRY(zfh, true, PRIV_VERSION_1_12_0, ext_zfh),
> > + ISA_EXT_DATA_ENTRY(zfhmin, true, PRIV_VERSION_1_12_0, ext_zfhmin),
> > + ISA_EXT_DATA_ENTRY(zfinx, true, PRIV_VERSION_1_12_0, ext_zfinx),
> > + ISA_EXT_DATA_ENTRY(zdinx, true, PRIV_VERSION_1_12_0, ext_zdinx),
> > + ISA_EXT_DATA_ENTRY(zba, true, PRIV_VERSION_1_12_0, ext_zba),
> > + ISA_EXT_DATA_ENTRY(zbb, true, PRIV_VERSION_1_12_0, ext_zbb),
> > + ISA_EXT_DATA_ENTRY(zbc, true, PRIV_VERSION_1_12_0, ext_zbc),
> > + ISA_EXT_DATA_ENTRY(zbkb, true, PRIV_VERSION_1_12_0, ext_zbkb),
> > + ISA_EXT_DATA_ENTRY(zbkc, true, PRIV_VERSION_1_12_0, ext_zbkc),
> > + ISA_EXT_DATA_ENTRY(zbkx, true, PRIV_VERSION_1_12_0, ext_zbkx),
> > + ISA_EXT_DATA_ENTRY(zbs, true, PRIV_VERSION_1_12_0, ext_zbs),
> > + ISA_EXT_DATA_ENTRY(zk, true, PRIV_VERSION_1_12_0, ext_zk),
> > + ISA_EXT_DATA_ENTRY(zkn, true, PRIV_VERSION_1_12_0, ext_zkn),
> > + ISA_EXT_DATA_ENTRY(zknd, true, PRIV_VERSION_1_12_0, ext_zknd),
> > + ISA_EXT_DATA_ENTRY(zkne, true, PRIV_VERSION_1_12_0, ext_zkne),
> > + ISA_EXT_DATA_ENTRY(zknh, true, PRIV_VERSION_1_12_0, ext_zknh),
> > + ISA_EXT_DATA_ENTRY(zkr, true, PRIV_VERSION_1_12_0, ext_zkr),
> > + ISA_EXT_DATA_ENTRY(zks, true, PRIV_VERSION_1_12_0, ext_zks),
> > + ISA_EXT_DATA_ENTRY(zksed, true, PRIV_VERSION_1_12_0, ext_zksed),
> > + ISA_EXT_DATA_ENTRY(zksh, true, PRIV_VERSION_1_12_0, ext_zksh),
> > + ISA_EXT_DATA_ENTRY(zkt, true, PRIV_VERSION_1_12_0, ext_zkt),
> > + ISA_EXT_DATA_ENTRY(zve32f, true, PRIV_VERSION_1_12_0, ext_zve32f),
> > + ISA_EXT_DATA_ENTRY(zve64f, true, PRIV_VERSION_1_12_0, ext_zve64f),
> > + ISA_EXT_DATA_ENTRY(zhinx, true, PRIV_VERSION_1_12_0, ext_zhinx),
> > + ISA_EXT_DATA_ENTRY(zhinxmin, true, PRIV_VERSION_1_12_0, ext_zhinxmin),
> > + ISA_EXT_DATA_ENTRY(svinval, true, PRIV_VERSION_1_12_0, ext_svinval),
> > + ISA_EXT_DATA_ENTRY(svnapot, true, PRIV_VERSION_1_12_0, ext_svnapot),
> > + ISA_EXT_DATA_ENTRY(svpbmt, true, PRIV_VERSION_1_12_0, ext_svpbmt),
> > +};
> > +
> > +static bool isa_ext_is_enabled(RISCVCPU *cpu,
> > + const struct isa_ext_data *edata)
> > +{
> > + bool *ext_enabled = (void *)&cpu->cfg + edata->ext_enable_offset;
> > +
> > + return *ext_enabled;
> > +}
> > +
> > +static void isa_ext_update_enabled(RISCVCPU *cpu,
> > + const struct isa_ext_data *edata, bool en)
> > +{
> > + bool *ext_enabled = (void *)&cpu->cfg + edata->ext_enable_offset;
> > +
> > + *ext_enabled = en;
> > +}
> > +
> > const char * const riscv_int_regnames[] = {
> > "x0/zero", "x1/ra", "x2/sp", "x3/gp", "x4/tp", "x5/t0", "x6/t1",
> > "x7/t2", "x8/s0", "x9/s1", "x10/a0", "x11/a1", "x12/a2", "x13/a3",
> > @@ -530,7 +603,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 = -1;
> > + int i, priv_version = -1;
> > Error *local_err = NULL;
> >
> > cpu_exec_realizefn(cs, &local_err);
> > @@ -558,6 +631,17 @@ 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 */
> > + for (i = 0; i < ARRAY_SIZE(isa_edata_arr); i++) {
> > + if (isa_ext_is_enabled(cpu, &isa_edata_arr[i]) &&
> > + (env->priv_ver < isa_edata_arr[i].min_version)) {
> > + isa_ext_update_enabled(cpu, &isa_edata_arr[i], false);
> > + 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);
> > + }
> > + }
> > +
> > if (cpu->cfg.mmu) {
> > riscv_set_feature(env, RISCV_FEATURE_MMU);
> > }
> > @@ -1050,67 +1134,15 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data)
> > device_class_set_props(dc, riscv_cpu_properties);
> > }
> >
> > -#define ISA_EDATA_ENTRY(name, prop) {#name, cpu->cfg.prop}
> > -
> > static void riscv_isa_string_ext(RISCVCPU *cpu, char **isa_str, int max_str_len)
> > {
> > char *old = *isa_str;
> > char *new = *isa_str;
> > int i;
> >
> > - /**
> > - * Here are the ordering rules of extension naming defined by RISC-V
> > - * specification :
> > - * 1. All extensions should be separated from other multi-letter extensions
> > - * by an underscore.
> > - * 2. The first letter following the 'Z' conventionally indicates the most
> > - * closely related alphabetical extension category, IMAFDQLCBKJTPVH.
> > - * If multiple 'Z' extensions are named, they should be ordered first
> > - * by category, then alphabetically within a category.
> > - * 3. Standard supervisor-level extensions (starts with 'S') should be
> > - * listed after standard unprivileged extensions. If multiple
> > - * supervisor-level extensions are listed, they should be ordered
> > - * alphabetically.
> > - * 4. Non-standard extensions (starts with 'X') must be listed after all
> > - * standard extensions. They must be separated from other multi-letter
> > - * extensions by an underscore.
> > - */
> > - struct isa_ext_data isa_edata_arr[] = {
> > - ISA_EDATA_ENTRY(zicsr, ext_icsr),
> > - ISA_EDATA_ENTRY(zifencei, ext_ifencei),
> > - ISA_EDATA_ENTRY(zmmul, ext_zmmul),
> > - ISA_EDATA_ENTRY(zfh, ext_zfh),
> > - ISA_EDATA_ENTRY(zfhmin, ext_zfhmin),
> > - ISA_EDATA_ENTRY(zfinx, ext_zfinx),
> > - ISA_EDATA_ENTRY(zdinx, ext_zdinx),
> > - ISA_EDATA_ENTRY(zba, ext_zba),
> > - ISA_EDATA_ENTRY(zbb, ext_zbb),
> > - ISA_EDATA_ENTRY(zbc, ext_zbc),
> > - ISA_EDATA_ENTRY(zbkb, ext_zbkb),
> > - ISA_EDATA_ENTRY(zbkc, ext_zbkc),
> > - ISA_EDATA_ENTRY(zbkx, ext_zbkx),
> > - ISA_EDATA_ENTRY(zbs, ext_zbs),
> > - ISA_EDATA_ENTRY(zk, ext_zk),
> > - ISA_EDATA_ENTRY(zkn, ext_zkn),
> > - ISA_EDATA_ENTRY(zknd, ext_zknd),
> > - ISA_EDATA_ENTRY(zkne, ext_zkne),
> > - ISA_EDATA_ENTRY(zknh, ext_zknh),
> > - ISA_EDATA_ENTRY(zkr, ext_zkr),
> > - ISA_EDATA_ENTRY(zks, ext_zks),
> > - ISA_EDATA_ENTRY(zksed, ext_zksed),
> > - ISA_EDATA_ENTRY(zksh, ext_zksh),
> > - ISA_EDATA_ENTRY(zkt, ext_zkt),
> > - ISA_EDATA_ENTRY(zve32f, ext_zve32f),
> > - ISA_EDATA_ENTRY(zve64f, ext_zve64f),
> > - ISA_EDATA_ENTRY(zhinx, ext_zhinx),
> > - ISA_EDATA_ENTRY(zhinxmin, ext_zhinxmin),
> > - ISA_EDATA_ENTRY(svinval, ext_svinval),
> > - ISA_EDATA_ENTRY(svnapot, ext_svnapot),
> > - ISA_EDATA_ENTRY(svpbmt, ext_svpbmt),
> > - };
> > -
> > for (i = 0; i < ARRAY_SIZE(isa_edata_arr); i++) {
> > - if (isa_edata_arr[i].enabled) {
> > + if (isa_edata_arr[i].multi_letter &&
> > + isa_ext_is_enabled(cpu, &isa_edata_arr[i])) {
> > new = g_strconcat(old, "_", isa_edata_arr[i].name, NULL);
> > g_free(old);
> > old = new;
> > --
> > 2.34.1
> >
> >
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v6 0/4] QEMU RISC-V nested virtualization fixes
2022-06-11 8:01 [PATCH v6 0/4] QEMU RISC-V nested virtualization fixes Anup Patel
` (3 preceding siblings ...)
2022-06-11 8:01 ` [PATCH v6 4/4] target/riscv: Force disable extensions if priv spec version does not match Anup Patel
@ 2022-06-27 2:56 ` Alistair Francis
4 siblings, 0 replies; 12+ messages in thread
From: Alistair Francis @ 2022-06-27 2:56 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 Sat, Jun 11, 2022 at 6:20 PM 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_v6 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 v5:
> - Correctly set "Addr. Offset" for misaligned load/store traps in PATCH3
> - Use offsetof() instead of pointer in PATCH4
>
> Changes since v4:
> - Updated commit description in PATCH1, PATCH2, and PATCH4
> - Use "const" for local array in PATCH5
>
> Changes since v3:
> - Updated PATCH3 to set special pseudoinstructions in htinst for
> guest page faults which result due to VS-stage page table walks
> - Updated warning message in PATCH4
>
> Changes since v2:
> - Dropped the patch which are already in Alistair's next branch
> - Set "Addr. Offset" in the transformed instruction for PATCH3
> - Print warning in riscv_cpu_realize() if we are disabling an
> extension due to privilege spec verions mismatch for PATCH4
>
> 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 (4):
> 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: Update [m|h]tinst CSR in riscv_cpu_do_interrupt()
> target/riscv: Force disable extensions if priv spec version does not
> match
Thanks!
Applied to riscv-to-apply.next
Alistair
>
> target/riscv/cpu.c | 154 ++++++++++++++++-----------
> target/riscv/cpu.h | 3 +
> target/riscv/cpu_bits.h | 3 +
> target/riscv/cpu_helper.c | 214 ++++++++++++++++++++++++++++++++++++--
> target/riscv/csr.c | 2 +
> target/riscv/instmap.h | 45 ++++++++
> 6 files changed, 356 insertions(+), 65 deletions(-)
>
> --
> 2.34.1
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread