[PATCH v2 0/8] QEMU RISC-V nested virtualization fixes

[PATCH v2 0/8] QEMU RISC-V nested virtualization fixes

[Anup Patel]

This series does fixes and improvements to have nested virtualization
on QEMU RISC-V.

These patches can also be found in riscv_nested_fixes_v2 branch at:
https://github.com/avpatel/qemu.git

The RISC-V nested virtualization was tested on QEMU RISC-V using
Xvisor RISC-V which has required hypervisor support to run another
hypervisor as Guest/VM.

Changes since v1:
 - Set write_gva to env->two_stage_lookup which ensures that for
   HS-mode to HS-mode trap write_gva is true only for HLV/HSV
   instructions
 - Included "[PATCH 0/3] QEMU RISC-V priv spec version fixes"
   patches in this series for easy review
 - Re-worked PATCH7 to force disable extensions if required
   priv spec version is not staisfied
 - Added new PATCH8 to fix "aia=aplic-imsic" mode of virt machine

Anup Patel (8):
  target/riscv: Fix csr number based privilege checking
  target/riscv: Fix hstatus.GVA bit setting for traps taken from HS-mode
  target/riscv: Set [m|s]tval for both illegal and virtual instruction
    traps
  target/riscv: Update [m|h]tinst CSR in riscv_cpu_do_interrupt()
  target/riscv: Don't force update priv spec version to latest
  target/riscv: Add dummy mcountinhibit CSR for priv spec v1.11 or
    higher
  target/riscv: Force disable extensions if priv spec version does not
    match
  hw/riscv: virt: Fix interrupt parent for dynamic platform devices

 hw/riscv/virt.c           |  25 +++---
 target/riscv/cpu.c        |  46 +++++++++-
 target/riscv/cpu.h        |   8 +-
 target/riscv/cpu_bits.h   |   3 +
 target/riscv/cpu_helper.c | 172 ++++++++++++++++++++++++++++++++++++--
 target/riscv/csr.c        |  10 ++-
 target/riscv/instmap.h    |  41 +++++++++
 target/riscv/translate.c  |  17 +++-
 8 files changed, 292 insertions(+), 30 deletions(-)

-- 
2.34.1

[PATCH v2 1/8] target/riscv: Fix csr number based privilege checking

[Anup Patel]

When hypervisor and VS CSRs are accessed from VS-mode or VU-mode,
the riscv_csrrw_check() function should generate virtual instruction
trap instead illegal instruction trap.

Fixes: 0a42f4c44088 (" target/riscv: Fix CSR perm checking for HS mode")
Signed-off-by: Anup Patel <apatel@ventanamicro.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Frank Chang <frank.chang@sifive.com>
---
 target/riscv/csr.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 3500e07f92..2bf0a97196 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -3139,7 +3139,7 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
     int read_only = get_field(csrno, 0xC00) == 3;
     int csr_min_priv = csr_ops[csrno].min_priv_ver;
 #if !defined(CONFIG_USER_ONLY)
-    int effective_priv = env->priv;
+    int csr_priv, effective_priv = env->priv;
 
     if (riscv_has_ext(env, RVH) &&
         env->priv == PRV_S &&
@@ -3152,7 +3152,11 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
         effective_priv++;
     }
 
-    if (!env->debugger && (effective_priv < get_field(csrno, 0x300))) {
+    csr_priv = get_field(csrno, 0x300);
+    if (!env->debugger && (effective_priv < csr_priv)) {
+        if (csr_priv == (PRV_S + 1) && riscv_cpu_virt_enabled(env)) {
+            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
+        }
         return RISCV_EXCP_ILLEGAL_INST;
     }
 #endif
-- 
2.34.1

Re: [PATCH v2 1/8] target/riscv: Fix csr number based privilege checking

[Atish Patra]

On Wed, May 11, 2022 at 7:46 AM Anup Patel <apatel@ventanamicro.com> wrote:
>
> When hypervisor and VS CSRs are accessed from VS-mode or VU-mode,
> the riscv_csrrw_check() function should generate virtual instruction
> trap instead illegal instruction trap.
>
> Fixes: 0a42f4c44088 (" target/riscv: Fix CSR perm checking for HS mode")
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> Reviewed-by: Frank Chang <frank.chang@sifive.com>
> ---
>  target/riscv/csr.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 3500e07f92..2bf0a97196 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -3139,7 +3139,7 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
>      int read_only = get_field(csrno, 0xC00) == 3;
>      int csr_min_priv = csr_ops[csrno].min_priv_ver;
>  #if !defined(CONFIG_USER_ONLY)
> -    int effective_priv = env->priv;
> +    int csr_priv, effective_priv = env->priv;
>
>      if (riscv_has_ext(env, RVH) &&
>          env->priv == PRV_S &&
> @@ -3152,7 +3152,11 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
>          effective_priv++;
>      }
>
> -    if (!env->debugger && (effective_priv < get_field(csrno, 0x300))) {
> +    csr_priv = get_field(csrno, 0x300);
> +    if (!env->debugger && (effective_priv < csr_priv)) {
> +        if (csr_priv == (PRV_S + 1) && riscv_cpu_virt_enabled(env)) {
> +            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> +        }
>          return RISCV_EXCP_ILLEGAL_INST;
>      }
>  #endif
> --
> 2.34.1
>

It seems Alistair has already queued a similar fix
https://www.mail-archive.com/qemu-devel@nongnu.org/msg886861.html

-- 
Regards,
Atish

[PATCH v2 2/8] target/riscv: Fix hstatus.GVA bit setting for traps taken from HS-mode

[Anup Patel]

Currently, QEMU does not set hstatus.GVA bit for traps taken from
HS-mode into HS-mode which breaks the Xvisor nested MMU test suite
on QEMU. This was working previously.

This patch updates riscv_cpu_do_interrupt() to fix the above issue.

Fixes: 86d0c457396b ("target/riscv: Fixup setting GVA")
Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 target/riscv/cpu_helper.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index e1aa4f2097..b16bfe0182 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -1367,7 +1367,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
         case RISCV_EXCP_INST_PAGE_FAULT:
         case RISCV_EXCP_LOAD_PAGE_FAULT:
         case RISCV_EXCP_STORE_PAGE_FAULT:
-            write_gva = true;
+            write_gva = env->two_stage_lookup;
             tval = env->badaddr;
             break;
         case RISCV_EXCP_ILLEGAL_INST:
@@ -1434,7 +1434,6 @@ void riscv_cpu_do_interrupt(CPUState *cs)
                 /* Trap into HS mode */
                 env->hstatus = set_field(env->hstatus, HSTATUS_SPV, false);
                 htval = env->guest_phys_fault_addr;
-                write_gva = false;
             }
             env->hstatus = set_field(env->hstatus, HSTATUS_GVA, write_gva);
         }
-- 
2.34.1

Re: [PATCH v2 2/8] target/riscv: Fix hstatus.GVA bit setting for traps taken from HS-mode

[Alistair Francis]

On Thu, May 12, 2022 at 12:49 AM Anup Patel <apatel@ventanamicro.com> wrote:
>
> Currently, QEMU does not set hstatus.GVA bit for traps taken from
> HS-mode into HS-mode which breaks the Xvisor nested MMU test suite
> on QEMU. This was working previously.
>
> This patch updates riscv_cpu_do_interrupt() to fix the above issue.
>
> Fixes: 86d0c457396b ("target/riscv: Fixup setting GVA")
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>

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

Alistair

> ---
>  target/riscv/cpu_helper.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index e1aa4f2097..b16bfe0182 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -1367,7 +1367,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>          case RISCV_EXCP_INST_PAGE_FAULT:
>          case RISCV_EXCP_LOAD_PAGE_FAULT:
>          case RISCV_EXCP_STORE_PAGE_FAULT:
> -            write_gva = true;
> +            write_gva = env->two_stage_lookup;
>              tval = env->badaddr;
>              break;
>          case RISCV_EXCP_ILLEGAL_INST:
> @@ -1434,7 +1434,6 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>                  /* Trap into HS mode */
>                  env->hstatus = set_field(env->hstatus, HSTATUS_SPV, false);
>                  htval = env->guest_phys_fault_addr;
> -                write_gva = false;
>              }
>              env->hstatus = set_field(env->hstatus, HSTATUS_GVA, write_gva);
>          }
> --
> 2.34.1
>
>

[PATCH v2 3/8] target/riscv: Set [m|s]tval for both illegal and virtual instruction traps

[Anup Patel]

Currently, the [m|s]tval CSRs are set with trapping instruction encoding
only for illegal instruction traps taken at the time of instruction
decoding.

In RISC-V world, a valid instructions might also trap as illegal or
virtual instruction based to trapping bits in various CSRs (such as
mstatus.TVM or hstatus.VTVM).

We improve setting of [m|s]tval CSRs for all types of illegal and
virtual instruction traps.

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
Reviewed-by: Frank Chang <frank.chang@sifive.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/cpu.c        |  2 ++
 target/riscv/cpu.h        |  8 +++++++-
 target/riscv/cpu_helper.c |  1 +
 target/riscv/translate.c  | 17 +++++++++++++----
 4 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index ccacdee215..9a5be9732d 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -406,6 +406,7 @@ void restore_state_to_opc(CPURISCVState *env, TranslationBlock *tb,
     } else {
         env->pc = data[0];
     }
+    env->bins = data[1];
 }
 
 static void riscv_cpu_reset(DeviceState *dev)
@@ -445,6 +446,7 @@ static void riscv_cpu_reset(DeviceState *dev)
     env->mcause = 0;
     env->miclaim = MIP_SGEIP;
     env->pc = env->resetvec;
+    env->bins = 0;
     env->two_stage_lookup = false;
 
     /* Initialized default priorities of local interrupts. */
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index fe6c9a2c92..a55c918274 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -30,6 +30,12 @@
 
 #define TCG_GUEST_DEFAULT_MO 0
 
+/*
+ * RISC-V-specific extra insn start words:
+ * 1: Original instruction opcode
+ */
+#define TARGET_INSN_START_EXTRA_WORDS 1
+
 #define TYPE_RISCV_CPU "riscv-cpu"
 
 #define RISCV_CPU_TYPE_SUFFIX "-" TYPE_RISCV_CPU
@@ -140,7 +146,7 @@ struct CPUArchState {
     target_ulong frm;
 
     target_ulong badaddr;
-    uint32_t bins;
+    target_ulong bins;
 
     target_ulong guest_phys_fault_addr;
 
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index b16bfe0182..d99fac9d2d 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -1371,6 +1371,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
             tval = env->badaddr;
             break;
         case RISCV_EXCP_ILLEGAL_INST:
+        case RISCV_EXCP_VIRT_INSTRUCTION_FAULT:
             tval = env->bins;
             break;
         default:
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 0cd1d9ee94..55a4713af2 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -107,6 +107,8 @@ typedef struct DisasContext {
     /* PointerMasking extension */
     bool pm_mask_enabled;
     bool pm_base_enabled;
+    /* TCG of the current insn_start */
+    TCGOp *insn_start;
 } DisasContext;
 
 static inline bool has_ext(DisasContext *ctx, uint32_t ext)
@@ -236,9 +238,6 @@ static void generate_exception_mtval(DisasContext *ctx, int excp)
 
 static void gen_exception_illegal(DisasContext *ctx)
 {
-    tcg_gen_st_i32(tcg_constant_i32(ctx->opcode), cpu_env,
-                   offsetof(CPURISCVState, bins));
-
     generate_exception(ctx, RISCV_EXCP_ILLEGAL_INST);
 }
 
@@ -1017,6 +1016,13 @@ static uint32_t opcode_at(DisasContextBase *dcbase, target_ulong pc)
 /* Include decoders for factored-out extensions */
 #include "decode-XVentanaCondOps.c.inc"
 
+static inline void decode_save_opc(DisasContext *ctx, target_ulong opc)
+{
+    assert(ctx->insn_start != NULL);
+    tcg_set_insn_start_param(ctx->insn_start, 1, opc);
+    ctx->insn_start = NULL;
+}
+
 static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
 {
     /*
@@ -1033,6 +1039,7 @@ static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
 
     /* Check for compressed insn */
     if (extract16(opcode, 0, 2) != 3) {
+        decode_save_opc(ctx, opcode);
         if (!has_ext(ctx, RVC)) {
             gen_exception_illegal(ctx);
         } else {
@@ -1047,6 +1054,7 @@ static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
         opcode32 = deposit32(opcode32, 16, 16,
                              translator_lduw(env, &ctx->base,
                                              ctx->base.pc_next + 2));
+        decode_save_opc(ctx, opcode32);
         ctx->opcode = opcode32;
         ctx->pc_succ_insn = ctx->base.pc_next + 4;
 
@@ -1113,7 +1121,8 @@ static void riscv_tr_insn_start(DisasContextBase *dcbase, CPUState *cpu)
 {
     DisasContext *ctx = container_of(dcbase, DisasContext, base);
 
-    tcg_gen_insn_start(ctx->base.pc_next);
+    tcg_gen_insn_start(ctx->base.pc_next, 0);
+    ctx->insn_start = tcg_last_op();
 }
 
 static void riscv_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
-- 
2.34.1

[PATCH v2 4/8] target/riscv: Update [m|h]tinst CSR in riscv_cpu_do_interrupt()

[Anup Patel]

We should write transformed instruction encoding of the trapped
instruction in [m|h]tinst CSR at time of taking trap as defined
by the RISC-V privileged specification v1.12.

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 target/riscv/cpu_helper.c | 168 +++++++++++++++++++++++++++++++++++++-
 target/riscv/instmap.h    |  41 ++++++++++
 2 files changed, 205 insertions(+), 4 deletions(-)

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index d99fac9d2d..b24652eb8d 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -22,6 +22,7 @@
 #include "qemu/main-loop.h"
 #include "cpu.h"
 #include "exec/exec-all.h"
+#include "instmap.h"
 #include "tcg/tcg-op.h"
 #include "trace.h"
 #include "semihosting/common-semi.h"
@@ -1318,6 +1319,158 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
 }
 #endif /* !CONFIG_USER_ONLY */
 
+static target_ulong riscv_transformed_insn(CPURISCVState *env,
+                                           target_ulong insn)
+{
+    target_ulong xinsn = 0;
+
+    /*
+     * Only Quadrant 0 and Quadrant 2 of RVC instruction space need to
+     * be uncompressed. The Quadrant 1 of RVC instruction space need
+     * not be transformed because these instructions won't generate
+     * any load/store trap.
+     */
+
+    if ((insn & 0x3) != 0x3) {
+        /* Transform 16bit instruction into 32bit instruction */
+        switch (GET_C_OP(insn)) {
+        case OPC_RISC_C_OP_QUAD0: /* Quadrant 0 */
+            switch (GET_C_FUNC(insn)) {
+            case OPC_RISC_C_FUNC_FLD_LQ:
+                if (riscv_cpu_xlen(env) != 128) { /* C.FLD (RV32/64) */
+                    xinsn = OPC_RISC_FLD;
+                    xinsn = SET_RD(xinsn, GET_C_RS2S(insn));
+                    xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
+                    xinsn = SET_I_IMM(xinsn, GET_C_LD_IMM(insn));
+                }
+                break;
+            case OPC_RISC_C_FUNC_LW: /* C.LW */
+                xinsn = OPC_RISC_LW;
+                xinsn = SET_RD(xinsn, GET_C_RS2S(insn));
+                xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
+                xinsn = SET_I_IMM(xinsn, GET_C_LW_IMM(insn));
+                break;
+            case OPC_RISC_C_FUNC_FLW_LD:
+                if (riscv_cpu_xlen(env) == 32) { /* C.FLW (RV32) */
+                    xinsn = OPC_RISC_FLW;
+                    xinsn = SET_RD(xinsn, GET_C_RS2S(insn));
+                    xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
+                    xinsn = SET_I_IMM(xinsn, GET_C_LW_IMM(insn));
+                } else { /* C.LD (RV64/RV128) */
+                    xinsn = OPC_RISC_LD;
+                    xinsn = SET_RD(xinsn, GET_C_RS2S(insn));
+                    xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
+                    xinsn = SET_I_IMM(xinsn, GET_C_LD_IMM(insn));
+                }
+                break;
+            case OPC_RISC_C_FUNC_FSD_SQ:
+                if (riscv_cpu_xlen(env) != 128) { /* C.FSD (RV32/64) */
+                    xinsn = OPC_RISC_FSD;
+                    xinsn = SET_RS2(xinsn, GET_C_RS2S(insn));
+                    xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
+                    xinsn = SET_S_IMM(xinsn, GET_C_SD_IMM(insn));
+                }
+                break;
+            case OPC_RISC_C_FUNC_SW: /* C.SW */
+                xinsn = OPC_RISC_SW;
+                xinsn = SET_RS2(xinsn, GET_C_RS2S(insn));
+                xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
+                xinsn = SET_S_IMM(xinsn, GET_C_SW_IMM(insn));
+                break;
+            case OPC_RISC_C_FUNC_FSW_SD:
+                if (riscv_cpu_xlen(env) == 32) { /* C.FSW (RV32) */
+                    xinsn = OPC_RISC_FSW;
+                    xinsn = SET_RS2(xinsn, GET_C_RS2S(insn));
+                    xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
+                    xinsn = SET_S_IMM(xinsn, GET_C_SW_IMM(insn));
+                } else { /* C.SD (RV64/RV128) */
+                    xinsn = OPC_RISC_SD;
+                    xinsn = SET_RS2(xinsn, GET_C_RS2S(insn));
+                    xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
+                    xinsn = SET_S_IMM(xinsn, GET_C_SD_IMM(insn));
+                }
+                break;
+            default:
+                break;
+            }
+            break;
+        case OPC_RISC_C_OP_QUAD2: /* Quadrant 2 */
+            switch (GET_C_FUNC(insn)) {
+            case OPC_RISC_C_FUNC_FLDSP_LQSP:
+                if (riscv_cpu_xlen(env) != 128) { /* C.FLDSP (RV32/64) */
+                    xinsn = OPC_RISC_FLD;
+                    xinsn = SET_RD(xinsn, GET_C_RD(insn));
+                    xinsn = SET_RS1(xinsn, 2);
+                    xinsn = SET_I_IMM(xinsn, GET_C_LDSP_IMM(insn));
+                }
+                break;
+            case OPC_RISC_C_FUNC_LWSP: /* C.LWSP */
+                xinsn = OPC_RISC_LW;
+                xinsn = SET_RD(xinsn, GET_C_RD(insn));
+                xinsn = SET_RS1(xinsn, 2);
+                xinsn = SET_I_IMM(xinsn, GET_C_LWSP_IMM(insn));
+                break;
+            case OPC_RISC_C_FUNC_FLWSP_LDSP:
+                if (riscv_cpu_xlen(env) == 32) { /* C.FLWSP (RV32) */
+                    xinsn = OPC_RISC_FLW;
+                    xinsn = SET_RD(xinsn, GET_C_RD(insn));
+                    xinsn = SET_RS1(xinsn, 2);
+                    xinsn = SET_I_IMM(xinsn, GET_C_LWSP_IMM(insn));
+                } else { /* C.LDSP (RV64/RV128) */
+                    xinsn = OPC_RISC_LD;
+                    xinsn = SET_RD(xinsn, GET_C_RD(insn));
+                    xinsn = SET_RS1(xinsn, 2);
+                    xinsn = SET_I_IMM(xinsn, GET_C_LDSP_IMM(insn));
+                }
+                break;
+            case OPC_RISC_C_FUNC_FSDSP_SQSP:
+                if (riscv_cpu_xlen(env) != 128) { /* C.FSDSP (RV32/64) */
+                    xinsn = OPC_RISC_FSD;
+                    xinsn = SET_RS2(xinsn, GET_C_RS2(insn));
+                    xinsn = SET_RS1(xinsn, 2);
+                    xinsn = SET_S_IMM(xinsn, GET_C_SDSP_IMM(insn));
+                }
+                break;
+            case OPC_RISC_C_FUNC_SWSP: /* C.SWSP */
+                xinsn = OPC_RISC_SW;
+                xinsn = SET_RS2(xinsn, GET_C_RS2(insn));
+                xinsn = SET_RS1(xinsn, 2);
+                xinsn = SET_S_IMM(xinsn, GET_C_SWSP_IMM(insn));
+                break;
+            case 7:
+                if (riscv_cpu_xlen(env) == 32) { /* C.FSWSP (RV32) */
+                    xinsn = OPC_RISC_FSW;
+                    xinsn = SET_RS2(xinsn, GET_C_RS2(insn));
+                    xinsn = SET_RS1(xinsn, 2);
+                    xinsn = SET_S_IMM(xinsn, GET_C_SWSP_IMM(insn));
+                } else { /* C.SDSP (RV64/RV128) */
+                    xinsn = OPC_RISC_SD;
+                    xinsn = SET_RS2(xinsn, GET_C_RS2(insn));
+                    xinsn = SET_RS1(xinsn, 2);
+                    xinsn = SET_S_IMM(xinsn, GET_C_SDSP_IMM(insn));
+                }
+                break;
+            default:
+                break;
+            }
+            break;
+        default:
+            break;
+        }
+
+        /*
+         * Clear Bit1 of transformed instruction to indicate that
+         * original insruction was a 16bit instruction
+         */
+        xinsn &= ~((target_ulong)0x2);
+    } else {
+        /* No need to transform 32bit (or wider) instructions */
+        xinsn = insn;
+    }
+
+    return xinsn;
+}
+
 /*
  * Handle Traps
  *
@@ -1340,6 +1493,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
     target_ulong cause = cs->exception_index & RISCV_EXCP_INT_MASK;
     uint64_t deleg = async ? env->mideleg : env->medeleg;
     target_ulong tval = 0;
+    target_ulong tinst = 0;
     target_ulong htval = 0;
     target_ulong mtval2 = 0;
 
@@ -1355,18 +1509,22 @@ void riscv_cpu_do_interrupt(CPUState *cs)
     if (!async) {
         /* set tval to badaddr for traps with address information */
         switch (cause) {
-        case RISCV_EXCP_INST_GUEST_PAGE_FAULT:
         case RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT:
         case RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT:
-        case RISCV_EXCP_INST_ADDR_MIS:
-        case RISCV_EXCP_INST_ACCESS_FAULT:
         case RISCV_EXCP_LOAD_ADDR_MIS:
         case RISCV_EXCP_STORE_AMO_ADDR_MIS:
         case RISCV_EXCP_LOAD_ACCESS_FAULT:
         case RISCV_EXCP_STORE_AMO_ACCESS_FAULT:
-        case RISCV_EXCP_INST_PAGE_FAULT:
         case RISCV_EXCP_LOAD_PAGE_FAULT:
         case RISCV_EXCP_STORE_PAGE_FAULT:
+            write_gva = env->two_stage_lookup;
+            tval = env->badaddr;
+            tinst = riscv_transformed_insn(env, env->bins);
+            break;
+        case RISCV_EXCP_INST_GUEST_PAGE_FAULT:
+        case RISCV_EXCP_INST_ADDR_MIS:
+        case RISCV_EXCP_INST_ACCESS_FAULT:
+        case RISCV_EXCP_INST_PAGE_FAULT:
             write_gva = env->two_stage_lookup;
             tval = env->badaddr;
             break;
@@ -1448,6 +1606,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
         env->sepc = env->pc;
         env->stval = tval;
         env->htval = htval;
+        env->htinst = tinst;
         env->pc = (env->stvec >> 2 << 2) +
             ((async && (env->stvec & 3) == 1) ? cause * 4 : 0);
         riscv_cpu_set_mode(env, PRV_S);
@@ -1478,6 +1637,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
         env->mepc = env->pc;
         env->mtval = tval;
         env->mtval2 = mtval2;
+        env->mtinst = tinst;
         env->pc = (env->mtvec >> 2 << 2) +
             ((async && (env->mtvec & 3) == 1) ? cause * 4 : 0);
         riscv_cpu_set_mode(env, PRV_M);
diff --git a/target/riscv/instmap.h b/target/riscv/instmap.h
index 40b6d2b64d..f4ee686c78 100644
--- a/target/riscv/instmap.h
+++ b/target/riscv/instmap.h
@@ -316,6 +316,12 @@ enum {
 #define GET_RS2(inst)  extract32(inst, 20, 5)
 #define GET_RD(inst)   extract32(inst, 7, 5)
 #define GET_IMM(inst)  sextract64(inst, 20, 12)
+#define SET_RS1(inst, val)  deposit32(inst, 15, 5, val)
+#define SET_RS2(inst, val)  deposit32(inst, 20, 5, val)
+#define SET_RD(inst, val)   deposit32(inst, 7, 5, val)
+#define SET_I_IMM(inst, val)  deposit32(inst, 20, 12, val)
+#define SET_S_IMM(inst, val)  \
+    deposit32(deposit32(inst, 7, 5, val), 25, 7, (val) >> 5)
 
 /* RVC decoding macros */
 #define GET_C_IMM(inst)             (extract32(inst, 2, 5) \
@@ -346,6 +352,8 @@ enum {
                                     | (extract32(inst, 5, 1) << 6))
 #define GET_C_LD_IMM(inst)          ((extract16(inst, 10, 3) << 3) \
                                     | (extract16(inst, 5, 2) << 6))
+#define GET_C_SW_IMM(inst)          GET_C_LW_IMM(inst)
+#define GET_C_SD_IMM(inst)          GET_C_LD_IMM(inst)
 #define GET_C_J_IMM(inst)           ((extract32(inst, 3, 3) << 1) \
                                     | (extract32(inst, 11, 1) << 4) \
                                     | (extract32(inst, 2, 1) << 5) \
@@ -366,4 +374,37 @@ enum {
 #define GET_C_RS1S(inst)            (8 + extract16(inst, 7, 3))
 #define GET_C_RS2S(inst)            (8 + extract16(inst, 2, 3))
 
+#define GET_C_FUNC(inst)           extract32(inst, 13, 3)
+#define GET_C_OP(inst)             extract32(inst, 0, 2)
+
+enum {
+    /* RVC Quadrants */
+    OPC_RISC_C_OP_QUAD0 = 0x0,
+    OPC_RISC_C_OP_QUAD1 = 0x1,
+    OPC_RISC_C_OP_QUAD2 = 0x2
+};
+
+enum {
+    /* RVC Quadrant 0 */
+    OPC_RISC_C_FUNC_ADDI4SPN = 0x0,
+    OPC_RISC_C_FUNC_FLD_LQ = 0x1,
+    OPC_RISC_C_FUNC_LW = 0x2,
+    OPC_RISC_C_FUNC_FLW_LD = 0x3,
+    OPC_RISC_C_FUNC_FSD_SQ = 0x5,
+    OPC_RISC_C_FUNC_SW = 0x6,
+    OPC_RISC_C_FUNC_FSW_SD = 0x7
+};
+
+enum {
+    /* RVC Quadrant 2 */
+    OPC_RISC_C_FUNC_SLLI_SLLI64 = 0x0,
+    OPC_RISC_C_FUNC_FLDSP_LQSP = 0x1,
+    OPC_RISC_C_FUNC_LWSP = 0x2,
+    OPC_RISC_C_FUNC_FLWSP_LDSP = 0x3,
+    OPC_RISC_C_FUNC_JR_MV_EBREAK_JALR_ADD = 0x4,
+    OPC_RISC_C_FUNC_FSDSP_SQSP = 0x5,
+    OPC_RISC_C_FUNC_SWSP = 0x6,
+    OPC_RISC_C_FUNC_FSWSP_SDSP = 0x7
+};
+
 #endif
-- 
2.34.1

Re: [PATCH v2 4/8] target/riscv: Update [m|h]tinst CSR in riscv_cpu_do_interrupt()

[Alistair Francis]

On Thu, May 12, 2022 at 12:47 AM Anup Patel <apatel@ventanamicro.com> wrote:
>
> We should write transformed instruction encoding of the trapped
> instruction in [m|h]tinst CSR at time of taking trap as defined
> by the RISC-V privileged specification v1.12.
>
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> ---
>  target/riscv/cpu_helper.c | 168 +++++++++++++++++++++++++++++++++++++-
>  target/riscv/instmap.h    |  41 ++++++++++
>  2 files changed, 205 insertions(+), 4 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index d99fac9d2d..b24652eb8d 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -22,6 +22,7 @@
>  #include "qemu/main-loop.h"
>  #include "cpu.h"
>  #include "exec/exec-all.h"
> +#include "instmap.h"
>  #include "tcg/tcg-op.h"
>  #include "trace.h"
>  #include "semihosting/common-semi.h"
> @@ -1318,6 +1319,158 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>  }
>  #endif /* !CONFIG_USER_ONLY */
>
> +static target_ulong riscv_transformed_insn(CPURISCVState *env,
> +                                           target_ulong insn)
> +{
> +    target_ulong xinsn = 0;
> +
> +    /*
> +     * Only Quadrant 0 and Quadrant 2 of RVC instruction space need to
> +     * be uncompressed. The Quadrant 1 of RVC instruction space need
> +     * not be transformed because these instructions won't generate
> +     * any load/store trap.
> +     */
> +
> +    if ((insn & 0x3) != 0x3) {
> +        /* Transform 16bit instruction into 32bit instruction */
> +        switch (GET_C_OP(insn)) {
> +        case OPC_RISC_C_OP_QUAD0: /* Quadrant 0 */
> +            switch (GET_C_FUNC(insn)) {
> +            case OPC_RISC_C_FUNC_FLD_LQ:
> +                if (riscv_cpu_xlen(env) != 128) { /* C.FLD (RV32/64) */
> +                    xinsn = OPC_RISC_FLD;
> +                    xinsn = SET_RD(xinsn, GET_C_RS2S(insn));
> +                    xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
> +                    xinsn = SET_I_IMM(xinsn, GET_C_LD_IMM(insn));
> +                }
> +                break;
> +            case OPC_RISC_C_FUNC_LW: /* C.LW */
> +                xinsn = OPC_RISC_LW;
> +                xinsn = SET_RD(xinsn, GET_C_RS2S(insn));
> +                xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
> +                xinsn = SET_I_IMM(xinsn, GET_C_LW_IMM(insn));
> +                break;
> +            case OPC_RISC_C_FUNC_FLW_LD:
> +                if (riscv_cpu_xlen(env) == 32) { /* C.FLW (RV32) */
> +                    xinsn = OPC_RISC_FLW;
> +                    xinsn = SET_RD(xinsn, GET_C_RS2S(insn));
> +                    xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
> +                    xinsn = SET_I_IMM(xinsn, GET_C_LW_IMM(insn));
> +                } else { /* C.LD (RV64/RV128) */
> +                    xinsn = OPC_RISC_LD;
> +                    xinsn = SET_RD(xinsn, GET_C_RS2S(insn));
> +                    xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
> +                    xinsn = SET_I_IMM(xinsn, GET_C_LD_IMM(insn));
> +                }
> +                break;
> +            case OPC_RISC_C_FUNC_FSD_SQ:
> +                if (riscv_cpu_xlen(env) != 128) { /* C.FSD (RV32/64) */
> +                    xinsn = OPC_RISC_FSD;
> +                    xinsn = SET_RS2(xinsn, GET_C_RS2S(insn));
> +                    xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
> +                    xinsn = SET_S_IMM(xinsn, GET_C_SD_IMM(insn));
> +                }
> +                break;
> +            case OPC_RISC_C_FUNC_SW: /* C.SW */
> +                xinsn = OPC_RISC_SW;
> +                xinsn = SET_RS2(xinsn, GET_C_RS2S(insn));
> +                xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
> +                xinsn = SET_S_IMM(xinsn, GET_C_SW_IMM(insn));
> +                break;
> +            case OPC_RISC_C_FUNC_FSW_SD:
> +                if (riscv_cpu_xlen(env) == 32) { /* C.FSW (RV32) */
> +                    xinsn = OPC_RISC_FSW;
> +                    xinsn = SET_RS2(xinsn, GET_C_RS2S(insn));
> +                    xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
> +                    xinsn = SET_S_IMM(xinsn, GET_C_SW_IMM(insn));
> +                } else { /* C.SD (RV64/RV128) */
> +                    xinsn = OPC_RISC_SD;
> +                    xinsn = SET_RS2(xinsn, GET_C_RS2S(insn));
> +                    xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
> +                    xinsn = SET_S_IMM(xinsn, GET_C_SD_IMM(insn));
> +                }
> +                break;
> +            default:
> +                break;
> +            }
> +            break;
> +        case OPC_RISC_C_OP_QUAD2: /* Quadrant 2 */
> +            switch (GET_C_FUNC(insn)) {
> +            case OPC_RISC_C_FUNC_FLDSP_LQSP:
> +                if (riscv_cpu_xlen(env) != 128) { /* C.FLDSP (RV32/64) */
> +                    xinsn = OPC_RISC_FLD;
> +                    xinsn = SET_RD(xinsn, GET_C_RD(insn));
> +                    xinsn = SET_RS1(xinsn, 2);

Is this right?

Shouldn't this be the offset "between the faulting virtual address
(written to mtval or stval) and the original virtual address"?

> +                    xinsn = SET_I_IMM(xinsn, GET_C_LDSP_IMM(insn));
> +                }
> +                break;
> +            case OPC_RISC_C_FUNC_LWSP: /* C.LWSP */
> +                xinsn = OPC_RISC_LW;
> +                xinsn = SET_RD(xinsn, GET_C_RD(insn));
> +                xinsn = SET_RS1(xinsn, 2);
> +                xinsn = SET_I_IMM(xinsn, GET_C_LWSP_IMM(insn));
> +                break;
> +            case OPC_RISC_C_FUNC_FLWSP_LDSP:
> +                if (riscv_cpu_xlen(env) == 32) { /* C.FLWSP (RV32) */
> +                    xinsn = OPC_RISC_FLW;
> +                    xinsn = SET_RD(xinsn, GET_C_RD(insn));
> +                    xinsn = SET_RS1(xinsn, 2);
> +                    xinsn = SET_I_IMM(xinsn, GET_C_LWSP_IMM(insn));
> +                } else { /* C.LDSP (RV64/RV128) */
> +                    xinsn = OPC_RISC_LD;
> +                    xinsn = SET_RD(xinsn, GET_C_RD(insn));
> +                    xinsn = SET_RS1(xinsn, 2);
> +                    xinsn = SET_I_IMM(xinsn, GET_C_LDSP_IMM(insn));
> +                }
> +                break;
> +            case OPC_RISC_C_FUNC_FSDSP_SQSP:
> +                if (riscv_cpu_xlen(env) != 128) { /* C.FSDSP (RV32/64) */
> +                    xinsn = OPC_RISC_FSD;
> +                    xinsn = SET_RS2(xinsn, GET_C_RS2(insn));
> +                    xinsn = SET_RS1(xinsn, 2);
> +                    xinsn = SET_S_IMM(xinsn, GET_C_SDSP_IMM(insn));
> +                }
> +                break;
> +            case OPC_RISC_C_FUNC_SWSP: /* C.SWSP */
> +                xinsn = OPC_RISC_SW;
> +                xinsn = SET_RS2(xinsn, GET_C_RS2(insn));
> +                xinsn = SET_RS1(xinsn, 2);
> +                xinsn = SET_S_IMM(xinsn, GET_C_SWSP_IMM(insn));
> +                break;
> +            case 7:
> +                if (riscv_cpu_xlen(env) == 32) { /* C.FSWSP (RV32) */
> +                    xinsn = OPC_RISC_FSW;
> +                    xinsn = SET_RS2(xinsn, GET_C_RS2(insn));
> +                    xinsn = SET_RS1(xinsn, 2);
> +                    xinsn = SET_S_IMM(xinsn, GET_C_SWSP_IMM(insn));
> +                } else { /* C.SDSP (RV64/RV128) */
> +                    xinsn = OPC_RISC_SD;
> +                    xinsn = SET_RS2(xinsn, GET_C_RS2(insn));
> +                    xinsn = SET_RS1(xinsn, 2);
> +                    xinsn = SET_S_IMM(xinsn, GET_C_SDSP_IMM(insn));
> +                }
> +                break;
> +            default:
> +                break;
> +            }
> +            break;
> +        default:
> +            break;
> +        }
> +
> +        /*
> +         * Clear Bit1 of transformed instruction to indicate that
> +         * original insruction was a 16bit instruction
> +         */
> +        xinsn &= ~((target_ulong)0x2);
> +    } else {
> +        /* No need to transform 32bit (or wider) instructions */

From what I can tell we still need to transform 32-bit instructions as
well, the "Addr. Offset" for example needs to be changed.

Alistair

> +        xinsn = insn;
> +    }
> +
> +    return xinsn;
> +}
> +
>  /*
>   * Handle Traps
>   *
> @@ -1340,6 +1493,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>      target_ulong cause = cs->exception_index & RISCV_EXCP_INT_MASK;
>      uint64_t deleg = async ? env->mideleg : env->medeleg;
>      target_ulong tval = 0;
> +    target_ulong tinst = 0;
>      target_ulong htval = 0;
>      target_ulong mtval2 = 0;
>
> @@ -1355,18 +1509,22 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>      if (!async) {
>          /* set tval to badaddr for traps with address information */
>          switch (cause) {
> -        case RISCV_EXCP_INST_GUEST_PAGE_FAULT:
>          case RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT:
>          case RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT:
> -        case RISCV_EXCP_INST_ADDR_MIS:
> -        case RISCV_EXCP_INST_ACCESS_FAULT:
>          case RISCV_EXCP_LOAD_ADDR_MIS:
>          case RISCV_EXCP_STORE_AMO_ADDR_MIS:
>          case RISCV_EXCP_LOAD_ACCESS_FAULT:
>          case RISCV_EXCP_STORE_AMO_ACCESS_FAULT:
> -        case RISCV_EXCP_INST_PAGE_FAULT:
>          case RISCV_EXCP_LOAD_PAGE_FAULT:
>          case RISCV_EXCP_STORE_PAGE_FAULT:
> +            write_gva = env->two_stage_lookup;
> +            tval = env->badaddr;
> +            tinst = riscv_transformed_insn(env, env->bins);
> +            break;
> +        case RISCV_EXCP_INST_GUEST_PAGE_FAULT:
> +        case RISCV_EXCP_INST_ADDR_MIS:
> +        case RISCV_EXCP_INST_ACCESS_FAULT:
> +        case RISCV_EXCP_INST_PAGE_FAULT:
>              write_gva = env->two_stage_lookup;
>              tval = env->badaddr;
>              break;
> @@ -1448,6 +1606,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>          env->sepc = env->pc;
>          env->stval = tval;
>          env->htval = htval;
> +        env->htinst = tinst;
>          env->pc = (env->stvec >> 2 << 2) +
>              ((async && (env->stvec & 3) == 1) ? cause * 4 : 0);
>          riscv_cpu_set_mode(env, PRV_S);
> @@ -1478,6 +1637,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>          env->mepc = env->pc;
>          env->mtval = tval;
>          env->mtval2 = mtval2;
> +        env->mtinst = tinst;
>          env->pc = (env->mtvec >> 2 << 2) +
>              ((async && (env->mtvec & 3) == 1) ? cause * 4 : 0);
>          riscv_cpu_set_mode(env, PRV_M);
> diff --git a/target/riscv/instmap.h b/target/riscv/instmap.h
> index 40b6d2b64d..f4ee686c78 100644
> --- a/target/riscv/instmap.h
> +++ b/target/riscv/instmap.h
> @@ -316,6 +316,12 @@ enum {
>  #define GET_RS2(inst)  extract32(inst, 20, 5)
>  #define GET_RD(inst)   extract32(inst, 7, 5)
>  #define GET_IMM(inst)  sextract64(inst, 20, 12)
> +#define SET_RS1(inst, val)  deposit32(inst, 15, 5, val)
> +#define SET_RS2(inst, val)  deposit32(inst, 20, 5, val)
> +#define SET_RD(inst, val)   deposit32(inst, 7, 5, val)
> +#define SET_I_IMM(inst, val)  deposit32(inst, 20, 12, val)
> +#define SET_S_IMM(inst, val)  \
> +    deposit32(deposit32(inst, 7, 5, val), 25, 7, (val) >> 5)
>
>  /* RVC decoding macros */
>  #define GET_C_IMM(inst)             (extract32(inst, 2, 5) \
> @@ -346,6 +352,8 @@ enum {
>                                      | (extract32(inst, 5, 1) << 6))
>  #define GET_C_LD_IMM(inst)          ((extract16(inst, 10, 3) << 3) \
>                                      | (extract16(inst, 5, 2) << 6))
> +#define GET_C_SW_IMM(inst)          GET_C_LW_IMM(inst)
> +#define GET_C_SD_IMM(inst)          GET_C_LD_IMM(inst)
>  #define GET_C_J_IMM(inst)           ((extract32(inst, 3, 3) << 1) \
>                                      | (extract32(inst, 11, 1) << 4) \
>                                      | (extract32(inst, 2, 1) << 5) \
> @@ -366,4 +374,37 @@ enum {
>  #define GET_C_RS1S(inst)            (8 + extract16(inst, 7, 3))
>  #define GET_C_RS2S(inst)            (8 + extract16(inst, 2, 3))
>
> +#define GET_C_FUNC(inst)           extract32(inst, 13, 3)
> +#define GET_C_OP(inst)             extract32(inst, 0, 2)
> +
> +enum {
> +    /* RVC Quadrants */
> +    OPC_RISC_C_OP_QUAD0 = 0x0,
> +    OPC_RISC_C_OP_QUAD1 = 0x1,
> +    OPC_RISC_C_OP_QUAD2 = 0x2
> +};
> +
> +enum {
> +    /* RVC Quadrant 0 */
> +    OPC_RISC_C_FUNC_ADDI4SPN = 0x0,
> +    OPC_RISC_C_FUNC_FLD_LQ = 0x1,
> +    OPC_RISC_C_FUNC_LW = 0x2,
> +    OPC_RISC_C_FUNC_FLW_LD = 0x3,
> +    OPC_RISC_C_FUNC_FSD_SQ = 0x5,
> +    OPC_RISC_C_FUNC_SW = 0x6,
> +    OPC_RISC_C_FUNC_FSW_SD = 0x7
> +};
> +
> +enum {
> +    /* RVC Quadrant 2 */
> +    OPC_RISC_C_FUNC_SLLI_SLLI64 = 0x0,
> +    OPC_RISC_C_FUNC_FLDSP_LQSP = 0x1,
> +    OPC_RISC_C_FUNC_LWSP = 0x2,
> +    OPC_RISC_C_FUNC_FLWSP_LDSP = 0x3,
> +    OPC_RISC_C_FUNC_JR_MV_EBREAK_JALR_ADD = 0x4,
> +    OPC_RISC_C_FUNC_FSDSP_SQSP = 0x5,
> +    OPC_RISC_C_FUNC_SWSP = 0x6,
> +    OPC_RISC_C_FUNC_FSWSP_SDSP = 0x7
> +};
> +
>  #endif
> --
> 2.34.1
>
>

Re: [PATCH v2 4/8] target/riscv: Update [m|h]tinst CSR in riscv_cpu_do_interrupt()

[Anup Patel]

On Tue, May 24, 2022 at 3:08 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Thu, May 12, 2022 at 12:47 AM Anup Patel <apatel@ventanamicro.com> wrote:
> >
> > We should write transformed instruction encoding of the trapped
> > instruction in [m|h]tinst CSR at time of taking trap as defined
> > by the RISC-V privileged specification v1.12.
> >
> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > ---
> >  target/riscv/cpu_helper.c | 168 +++++++++++++++++++++++++++++++++++++-
> >  target/riscv/instmap.h    |  41 ++++++++++
> >  2 files changed, 205 insertions(+), 4 deletions(-)
> >
> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > index d99fac9d2d..b24652eb8d 100644
> > --- a/target/riscv/cpu_helper.c
> > +++ b/target/riscv/cpu_helper.c
> > @@ -22,6 +22,7 @@
> >  #include "qemu/main-loop.h"
> >  #include "cpu.h"
> >  #include "exec/exec-all.h"
> > +#include "instmap.h"
> >  #include "tcg/tcg-op.h"
> >  #include "trace.h"
> >  #include "semihosting/common-semi.h"
> > @@ -1318,6 +1319,158 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> >  }
> >  #endif /* !CONFIG_USER_ONLY */
> >
> > +static target_ulong riscv_transformed_insn(CPURISCVState *env,
> > +                                           target_ulong insn)
> > +{
> > +    target_ulong xinsn = 0;
> > +
> > +    /*
> > +     * Only Quadrant 0 and Quadrant 2 of RVC instruction space need to
> > +     * be uncompressed. The Quadrant 1 of RVC instruction space need
> > +     * not be transformed because these instructions won't generate
> > +     * any load/store trap.
> > +     */
> > +
> > +    if ((insn & 0x3) != 0x3) {
> > +        /* Transform 16bit instruction into 32bit instruction */
> > +        switch (GET_C_OP(insn)) {
> > +        case OPC_RISC_C_OP_QUAD0: /* Quadrant 0 */
> > +            switch (GET_C_FUNC(insn)) {
> > +            case OPC_RISC_C_FUNC_FLD_LQ:
> > +                if (riscv_cpu_xlen(env) != 128) { /* C.FLD (RV32/64) */
> > +                    xinsn = OPC_RISC_FLD;
> > +                    xinsn = SET_RD(xinsn, GET_C_RS2S(insn));
> > +                    xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
> > +                    xinsn = SET_I_IMM(xinsn, GET_C_LD_IMM(insn));
> > +                }
> > +                break;
> > +            case OPC_RISC_C_FUNC_LW: /* C.LW */
> > +                xinsn = OPC_RISC_LW;
> > +                xinsn = SET_RD(xinsn, GET_C_RS2S(insn));
> > +                xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
> > +                xinsn = SET_I_IMM(xinsn, GET_C_LW_IMM(insn));
> > +                break;
> > +            case OPC_RISC_C_FUNC_FLW_LD:
> > +                if (riscv_cpu_xlen(env) == 32) { /* C.FLW (RV32) */
> > +                    xinsn = OPC_RISC_FLW;
> > +                    xinsn = SET_RD(xinsn, GET_C_RS2S(insn));
> > +                    xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
> > +                    xinsn = SET_I_IMM(xinsn, GET_C_LW_IMM(insn));
> > +                } else { /* C.LD (RV64/RV128) */
> > +                    xinsn = OPC_RISC_LD;
> > +                    xinsn = SET_RD(xinsn, GET_C_RS2S(insn));
> > +                    xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
> > +                    xinsn = SET_I_IMM(xinsn, GET_C_LD_IMM(insn));
> > +                }
> > +                break;
> > +            case OPC_RISC_C_FUNC_FSD_SQ:
> > +                if (riscv_cpu_xlen(env) != 128) { /* C.FSD (RV32/64) */
> > +                    xinsn = OPC_RISC_FSD;
> > +                    xinsn = SET_RS2(xinsn, GET_C_RS2S(insn));
> > +                    xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
> > +                    xinsn = SET_S_IMM(xinsn, GET_C_SD_IMM(insn));
> > +                }
> > +                break;
> > +            case OPC_RISC_C_FUNC_SW: /* C.SW */
> > +                xinsn = OPC_RISC_SW;
> > +                xinsn = SET_RS2(xinsn, GET_C_RS2S(insn));
> > +                xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
> > +                xinsn = SET_S_IMM(xinsn, GET_C_SW_IMM(insn));
> > +                break;
> > +            case OPC_RISC_C_FUNC_FSW_SD:
> > +                if (riscv_cpu_xlen(env) == 32) { /* C.FSW (RV32) */
> > +                    xinsn = OPC_RISC_FSW;
> > +                    xinsn = SET_RS2(xinsn, GET_C_RS2S(insn));
> > +                    xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
> > +                    xinsn = SET_S_IMM(xinsn, GET_C_SW_IMM(insn));
> > +                } else { /* C.SD (RV64/RV128) */
> > +                    xinsn = OPC_RISC_SD;
> > +                    xinsn = SET_RS2(xinsn, GET_C_RS2S(insn));
> > +                    xinsn = SET_RS1(xinsn, GET_C_RS1S(insn));
> > +                    xinsn = SET_S_IMM(xinsn, GET_C_SD_IMM(insn));
> > +                }
> > +                break;
> > +            default:
> > +                break;
> > +            }
> > +            break;
> > +        case OPC_RISC_C_OP_QUAD2: /* Quadrant 2 */
> > +            switch (GET_C_FUNC(insn)) {
> > +            case OPC_RISC_C_FUNC_FLDSP_LQSP:
> > +                if (riscv_cpu_xlen(env) != 128) { /* C.FLDSP (RV32/64) */
> > +                    xinsn = OPC_RISC_FLD;
> > +                    xinsn = SET_RD(xinsn, GET_C_RD(insn));
> > +                    xinsn = SET_RS1(xinsn, 2);
>
> Is this right?
>
> Shouldn't this be the offset "between the faulting virtual address
> (written to mtval or stval) and the original virtual address"?

Yes, it should be "Addr. Offset". I totally overlooked the "Addr. Offset"
field.

>
> > +                    xinsn = SET_I_IMM(xinsn, GET_C_LDSP_IMM(insn));
> > +                }
> > +                break;
> > +            case OPC_RISC_C_FUNC_LWSP: /* C.LWSP */
> > +                xinsn = OPC_RISC_LW;
> > +                xinsn = SET_RD(xinsn, GET_C_RD(insn));
> > +                xinsn = SET_RS1(xinsn, 2);
> > +                xinsn = SET_I_IMM(xinsn, GET_C_LWSP_IMM(insn));
> > +                break;
> > +            case OPC_RISC_C_FUNC_FLWSP_LDSP:
> > +                if (riscv_cpu_xlen(env) == 32) { /* C.FLWSP (RV32) */
> > +                    xinsn = OPC_RISC_FLW;
> > +                    xinsn = SET_RD(xinsn, GET_C_RD(insn));
> > +                    xinsn = SET_RS1(xinsn, 2);
> > +                    xinsn = SET_I_IMM(xinsn, GET_C_LWSP_IMM(insn));
> > +                } else { /* C.LDSP (RV64/RV128) */
> > +                    xinsn = OPC_RISC_LD;
> > +                    xinsn = SET_RD(xinsn, GET_C_RD(insn));
> > +                    xinsn = SET_RS1(xinsn, 2);
> > +                    xinsn = SET_I_IMM(xinsn, GET_C_LDSP_IMM(insn));
> > +                }
> > +                break;
> > +            case OPC_RISC_C_FUNC_FSDSP_SQSP:
> > +                if (riscv_cpu_xlen(env) != 128) { /* C.FSDSP (RV32/64) */
> > +                    xinsn = OPC_RISC_FSD;
> > +                    xinsn = SET_RS2(xinsn, GET_C_RS2(insn));
> > +                    xinsn = SET_RS1(xinsn, 2);
> > +                    xinsn = SET_S_IMM(xinsn, GET_C_SDSP_IMM(insn));
> > +                }
> > +                break;
> > +            case OPC_RISC_C_FUNC_SWSP: /* C.SWSP */
> > +                xinsn = OPC_RISC_SW;
> > +                xinsn = SET_RS2(xinsn, GET_C_RS2(insn));
> > +                xinsn = SET_RS1(xinsn, 2);
> > +                xinsn = SET_S_IMM(xinsn, GET_C_SWSP_IMM(insn));
> > +                break;
> > +            case 7:
> > +                if (riscv_cpu_xlen(env) == 32) { /* C.FSWSP (RV32) */
> > +                    xinsn = OPC_RISC_FSW;
> > +                    xinsn = SET_RS2(xinsn, GET_C_RS2(insn));
> > +                    xinsn = SET_RS1(xinsn, 2);
> > +                    xinsn = SET_S_IMM(xinsn, GET_C_SWSP_IMM(insn));
> > +                } else { /* C.SDSP (RV64/RV128) */
> > +                    xinsn = OPC_RISC_SD;
> > +                    xinsn = SET_RS2(xinsn, GET_C_RS2(insn));
> > +                    xinsn = SET_RS1(xinsn, 2);
> > +                    xinsn = SET_S_IMM(xinsn, GET_C_SDSP_IMM(insn));
> > +                }
> > +                break;
> > +            default:
> > +                break;
> > +            }
> > +            break;
> > +        default:
> > +            break;
> > +        }
> > +
> > +        /*
> > +         * Clear Bit1 of transformed instruction to indicate that
> > +         * original insruction was a 16bit instruction
> > +         */
> > +        xinsn &= ~((target_ulong)0x2);
> > +    } else {
> > +        /* No need to transform 32bit (or wider) instructions */
>
> From what I can tell we still need to transform 32-bit instructions as
> well, the "Addr. Offset" for example needs to be changed.

The "Addr. Offset" will be non-zero only for misaligned load/store
traps but QEMU RISC-V does not take misaligned load/store traps
so at least for QEMU RISC-V we should always set "Addr. Offset"
to zero.

Regards,
Anup

>
> Alistair
>
> > +        xinsn = insn;
> > +    }
> > +
> > +    return xinsn;
> > +}
> > +
> >  /*
> >   * Handle Traps
> >   *
> > @@ -1340,6 +1493,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> >      target_ulong cause = cs->exception_index & RISCV_EXCP_INT_MASK;
> >      uint64_t deleg = async ? env->mideleg : env->medeleg;
> >      target_ulong tval = 0;
> > +    target_ulong tinst = 0;
> >      target_ulong htval = 0;
> >      target_ulong mtval2 = 0;
> >
> > @@ -1355,18 +1509,22 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> >      if (!async) {
> >          /* set tval to badaddr for traps with address information */
> >          switch (cause) {
> > -        case RISCV_EXCP_INST_GUEST_PAGE_FAULT:
> >          case RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT:
> >          case RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT:
> > -        case RISCV_EXCP_INST_ADDR_MIS:
> > -        case RISCV_EXCP_INST_ACCESS_FAULT:
> >          case RISCV_EXCP_LOAD_ADDR_MIS:
> >          case RISCV_EXCP_STORE_AMO_ADDR_MIS:
> >          case RISCV_EXCP_LOAD_ACCESS_FAULT:
> >          case RISCV_EXCP_STORE_AMO_ACCESS_FAULT:
> > -        case RISCV_EXCP_INST_PAGE_FAULT:
> >          case RISCV_EXCP_LOAD_PAGE_FAULT:
> >          case RISCV_EXCP_STORE_PAGE_FAULT:
> > +            write_gva = env->two_stage_lookup;
> > +            tval = env->badaddr;
> > +            tinst = riscv_transformed_insn(env, env->bins);
> > +            break;
> > +        case RISCV_EXCP_INST_GUEST_PAGE_FAULT:
> > +        case RISCV_EXCP_INST_ADDR_MIS:
> > +        case RISCV_EXCP_INST_ACCESS_FAULT:
> > +        case RISCV_EXCP_INST_PAGE_FAULT:
> >              write_gva = env->two_stage_lookup;
> >              tval = env->badaddr;
> >              break;
> > @@ -1448,6 +1606,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> >          env->sepc = env->pc;
> >          env->stval = tval;
> >          env->htval = htval;
> > +        env->htinst = tinst;
> >          env->pc = (env->stvec >> 2 << 2) +
> >              ((async && (env->stvec & 3) == 1) ? cause * 4 : 0);
> >          riscv_cpu_set_mode(env, PRV_S);
> > @@ -1478,6 +1637,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> >          env->mepc = env->pc;
> >          env->mtval = tval;
> >          env->mtval2 = mtval2;
> > +        env->mtinst = tinst;
> >          env->pc = (env->mtvec >> 2 << 2) +
> >              ((async && (env->mtvec & 3) == 1) ? cause * 4 : 0);
> >          riscv_cpu_set_mode(env, PRV_M);
> > diff --git a/target/riscv/instmap.h b/target/riscv/instmap.h
> > index 40b6d2b64d..f4ee686c78 100644
> > --- a/target/riscv/instmap.h
> > +++ b/target/riscv/instmap.h
> > @@ -316,6 +316,12 @@ enum {
> >  #define GET_RS2(inst)  extract32(inst, 20, 5)
> >  #define GET_RD(inst)   extract32(inst, 7, 5)
> >  #define GET_IMM(inst)  sextract64(inst, 20, 12)
> > +#define SET_RS1(inst, val)  deposit32(inst, 15, 5, val)
> > +#define SET_RS2(inst, val)  deposit32(inst, 20, 5, val)
> > +#define SET_RD(inst, val)   deposit32(inst, 7, 5, val)
> > +#define SET_I_IMM(inst, val)  deposit32(inst, 20, 12, val)
> > +#define SET_S_IMM(inst, val)  \
> > +    deposit32(deposit32(inst, 7, 5, val), 25, 7, (val) >> 5)
> >
> >  /* RVC decoding macros */
> >  #define GET_C_IMM(inst)             (extract32(inst, 2, 5) \
> > @@ -346,6 +352,8 @@ enum {
> >                                      | (extract32(inst, 5, 1) << 6))
> >  #define GET_C_LD_IMM(inst)          ((extract16(inst, 10, 3) << 3) \
> >                                      | (extract16(inst, 5, 2) << 6))
> > +#define GET_C_SW_IMM(inst)          GET_C_LW_IMM(inst)
> > +#define GET_C_SD_IMM(inst)          GET_C_LD_IMM(inst)
> >  #define GET_C_J_IMM(inst)           ((extract32(inst, 3, 3) << 1) \
> >                                      | (extract32(inst, 11, 1) << 4) \
> >                                      | (extract32(inst, 2, 1) << 5) \
> > @@ -366,4 +374,37 @@ enum {
> >  #define GET_C_RS1S(inst)            (8 + extract16(inst, 7, 3))
> >  #define GET_C_RS2S(inst)            (8 + extract16(inst, 2, 3))
> >
> > +#define GET_C_FUNC(inst)           extract32(inst, 13, 3)
> > +#define GET_C_OP(inst)             extract32(inst, 0, 2)
> > +
> > +enum {
> > +    /* RVC Quadrants */
> > +    OPC_RISC_C_OP_QUAD0 = 0x0,
> > +    OPC_RISC_C_OP_QUAD1 = 0x1,
> > +    OPC_RISC_C_OP_QUAD2 = 0x2
> > +};
> > +
> > +enum {
> > +    /* RVC Quadrant 0 */
> > +    OPC_RISC_C_FUNC_ADDI4SPN = 0x0,
> > +    OPC_RISC_C_FUNC_FLD_LQ = 0x1,
> > +    OPC_RISC_C_FUNC_LW = 0x2,
> > +    OPC_RISC_C_FUNC_FLW_LD = 0x3,
> > +    OPC_RISC_C_FUNC_FSD_SQ = 0x5,
> > +    OPC_RISC_C_FUNC_SW = 0x6,
> > +    OPC_RISC_C_FUNC_FSW_SD = 0x7
> > +};
> > +
> > +enum {
> > +    /* RVC Quadrant 2 */
> > +    OPC_RISC_C_FUNC_SLLI_SLLI64 = 0x0,
> > +    OPC_RISC_C_FUNC_FLDSP_LQSP = 0x1,
> > +    OPC_RISC_C_FUNC_LWSP = 0x2,
> > +    OPC_RISC_C_FUNC_FLWSP_LDSP = 0x3,
> > +    OPC_RISC_C_FUNC_JR_MV_EBREAK_JALR_ADD = 0x4,
> > +    OPC_RISC_C_FUNC_FSDSP_SQSP = 0x5,
> > +    OPC_RISC_C_FUNC_SWSP = 0x6,
> > +    OPC_RISC_C_FUNC_FSWSP_SDSP = 0x7
> > +};
> > +
> >  #endif
> > --
> > 2.34.1
> >
> >

[PATCH v2 5/8] target/riscv: Don't force update priv spec version to latest

[Anup Patel]

The riscv_cpu_realize() sets priv spec verion to v1.12 when it is
when "env->priv_ver == 0" (i.e. default v1.10) because the enum
value of priv spec v1.10 is zero.

Due to above issue, the sifive_u machine will see priv spec v1.12
instead of priv spec v1.10.

To fix this issue, we set latest priv spec version (i.e. v1.12)
for base rv64/rv32 cpu and riscv_cpu_realize() will override priv
spec version only when "cpu->cfg.priv_spec != NULL".

Fixes: 7100fe6c2441 ("target/riscv: Enable privileged spec version 1.12")
Signed-off-by: Anup Patel <apatel@ventanamicro.com>
Reviewed-by: Frank Chang <frank.chang@sifive.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Atish Patra <atishp@rivosinc.com>
---
 target/riscv/cpu.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 9a5be9732d..f3b61dfd63 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -169,6 +169,8 @@ static void rv64_base_cpu_init(Object *obj)
     CPURISCVState *env = &RISCV_CPU(obj)->env;
     /* We set this in the realise function */
     set_misa(env, MXL_RV64, 0);
+    /* Set latest version of privileged specification */
+    set_priv_version(env, PRIV_VERSION_1_12_0);
 }
 
 static void rv64_sifive_u_cpu_init(Object *obj)
@@ -204,6 +206,8 @@ static void rv32_base_cpu_init(Object *obj)
     CPURISCVState *env = &RISCV_CPU(obj)->env;
     /* We set this in the realise function */
     set_misa(env, MXL_RV32, 0);
+    /* Set latest version of privileged specification */
+    set_priv_version(env, PRIV_VERSION_1_12_0);
 }
 
 static void rv32_sifive_u_cpu_init(Object *obj)
@@ -509,7 +513,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
     CPURISCVState *env = &cpu->env;
     RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(dev);
     CPUClass *cc = CPU_CLASS(mcc);
-    int priv_version = 0;
+    int priv_version = -1;
     Error *local_err = NULL;
 
     cpu_exec_realizefn(cs, &local_err);
@@ -533,10 +537,8 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
         }
     }
 
-    if (priv_version) {
+    if (priv_version >= PRIV_VERSION_1_10_0) {
         set_priv_version(env, priv_version);
-    } else if (!env->priv_ver) {
-        set_priv_version(env, PRIV_VERSION_1_12_0);
     }
 
     if (cpu->cfg.mmu) {
-- 
2.34.1

Re: [PATCH v2 5/8] target/riscv: Don't force update priv spec version to latest

[Atish Patra]

On Wed, May 11, 2022 at 7:46 AM Anup Patel <apatel@ventanamicro.com> wrote:
>
> The riscv_cpu_realize() sets priv spec verion to v1.12 when it is
> when "env->priv_ver == 0" (i.e. default v1.10) because the enum
> value of priv spec v1.10 is zero.
>
> Due to above issue, the sifive_u machine will see priv spec v1.12
> instead of priv spec v1.10.
>
> To fix this issue, we set latest priv spec version (i.e. v1.12)
> for base rv64/rv32 cpu and riscv_cpu_realize() will override priv
> spec version only when "cpu->cfg.priv_spec != NULL".
>
> Fixes: 7100fe6c2441 ("target/riscv: Enable privileged spec version 1.12")
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> Reviewed-by: Frank Chang <frank.chang@sifive.com>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> Reviewed-by: Atish Patra <atishp@rivosinc.com>
> ---
>  target/riscv/cpu.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 9a5be9732d..f3b61dfd63 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -169,6 +169,8 @@ static void rv64_base_cpu_init(Object *obj)
>      CPURISCVState *env = &RISCV_CPU(obj)->env;
>      /* We set this in the realise function */
>      set_misa(env, MXL_RV64, 0);
> +    /* Set latest version of privileged specification */
> +    set_priv_version(env, PRIV_VERSION_1_12_0);
>  }
>
>  static void rv64_sifive_u_cpu_init(Object *obj)
> @@ -204,6 +206,8 @@ static void rv32_base_cpu_init(Object *obj)
>      CPURISCVState *env = &RISCV_CPU(obj)->env;
>      /* We set this in the realise function */
>      set_misa(env, MXL_RV32, 0);
> +    /* Set latest version of privileged specification */
> +    set_priv_version(env, PRIV_VERSION_1_12_0);
>  }
>
>  static void rv32_sifive_u_cpu_init(Object *obj)
> @@ -509,7 +513,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
>      CPURISCVState *env = &cpu->env;
>      RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(dev);
>      CPUClass *cc = CPU_CLASS(mcc);
> -    int priv_version = 0;
> +    int priv_version = -1;
>      Error *local_err = NULL;
>
>      cpu_exec_realizefn(cs, &local_err);
> @@ -533,10 +537,8 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
>          }
>      }
>
> -    if (priv_version) {
> +    if (priv_version >= PRIV_VERSION_1_10_0) {
>          set_priv_version(env, priv_version);
> -    } else if (!env->priv_ver) {
> -        set_priv_version(env, PRIV_VERSION_1_12_0);
>      }
>
>      if (cpu->cfg.mmu) {
> --
> 2.34.1
>


Reviewed-by: Atish Patra <atishp@rivosinc.com>
-- 
Regards,
Atish

[PATCH v2 6/8] target/riscv: Add dummy mcountinhibit CSR for priv spec v1.11 or higher

[Anup Patel]

The mcountinhibit CSR is mandatory for priv spec v1.11 or higher. For
implementation that don't want to implement can simply have a dummy
mcountinhibit which always zero.

Fixes: a4b2fa433125 ("target/riscv: Introduce privilege version field in
the CSR ops.")
Signed-off-by: Anup Patel <apatel@ventanamicro.com>
Reviewed-by: Frank Chang <frank.chang@sifive.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/cpu_bits.h | 3 +++
 target/riscv/csr.c      | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
index 4d04b20d06..4a55c6a709 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -159,6 +159,9 @@
 #define CSR_MTVEC           0x305
 #define CSR_MCOUNTEREN      0x306
 
+/* Machine Counter Setup */
+#define CSR_MCOUNTINHIBIT   0x320
+
 /* 32-bit only */
 #define CSR_MSTATUSH        0x310
 
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 2bf0a97196..e144ce7135 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -3391,6 +3391,8 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
     [CSR_MIE]         = { "mie",        any,   NULL,    NULL,    rmw_mie           },
     [CSR_MTVEC]       = { "mtvec",      any,   read_mtvec,       write_mtvec       },
     [CSR_MCOUNTEREN]  = { "mcounteren", any,   read_mcounteren,  write_mcounteren  },
+    [CSR_MCOUNTINHIBIT] = { "mcountinhibit", any, read_zero, write_ignore,
+                                             .min_priv_ver = PRIV_VERSION_1_11_0 },
 
     [CSR_MSTATUSH]    = { "mstatush",   any32, read_mstatush,    write_mstatush    },
 
-- 
2.34.1

Re: [PATCH v2 6/8] target/riscv: Add dummy mcountinhibit CSR for priv spec v1.11 or higher

[Atish Patra]

On Wed, May 11, 2022 at 7:46 AM Anup Patel <apatel@ventanamicro.com> wrote:
>
> The mcountinhibit CSR is mandatory for priv spec v1.11 or higher. For
> implementation that don't want to implement can simply have a dummy
> mcountinhibit which always zero.
>
> Fixes: a4b2fa433125 ("target/riscv: Introduce privilege version field in
> the CSR ops.")
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> Reviewed-by: Frank Chang <frank.chang@sifive.com>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  target/riscv/cpu_bits.h | 3 +++
>  target/riscv/csr.c      | 2 ++
>  2 files changed, 5 insertions(+)
>
> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> index 4d04b20d06..4a55c6a709 100644
> --- a/target/riscv/cpu_bits.h
> +++ b/target/riscv/cpu_bits.h
> @@ -159,6 +159,9 @@
>  #define CSR_MTVEC           0x305
>  #define CSR_MCOUNTEREN      0x306
>
> +/* Machine Counter Setup */
> +#define CSR_MCOUNTINHIBIT   0x320
> +
>  /* 32-bit only */
>  #define CSR_MSTATUSH        0x310
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 2bf0a97196..e144ce7135 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -3391,6 +3391,8 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
>      [CSR_MIE]         = { "mie",        any,   NULL,    NULL,    rmw_mie           },
>      [CSR_MTVEC]       = { "mtvec",      any,   read_mtvec,       write_mtvec       },
>      [CSR_MCOUNTEREN]  = { "mcounteren", any,   read_mcounteren,  write_mcounteren  },
> +    [CSR_MCOUNTINHIBIT] = { "mcountinhibit", any, read_zero, write_ignore,
> +                                             .min_priv_ver = PRIV_VERSION_1_11_0 },
>
>      [CSR_MSTATUSH]    = { "mstatush",   any32, read_mstatush,    write_mstatush    },
>
> --
> 2.34.1
>

Just to clarify, this is only required if this series is merged before
the PMU series.
FWIW,

Reviewed-by: Atish Patra <atishp@rivosinc.com>

-- 
Regards,
Atish

[PATCH v2 7/8] target/riscv: Force disable extensions if priv spec version does not match

[Anup Patel]

We should disable extensions in riscv_cpu_realize() if minimum required
priv spec version is not satisfied. This also ensures that machines with
priv spec v1.11 (or lower) cannot enable H, V, and various multi-letter
extensions.

Fixes: a775398be2e ("target/riscv: Add isa extenstion strings to the
device tree")
Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 target/riscv/cpu.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index f3b61dfd63..25a4ba3e22 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -541,6 +541,40 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
         set_priv_version(env, priv_version);
     }
 
+    /* Force disable extensions if priv spec version does not match */
+    if (env->priv_ver < PRIV_VERSION_1_12_0) {
+        cpu->cfg.ext_h = false;
+        cpu->cfg.ext_v = false;
+        cpu->cfg.ext_zfh = false;
+        cpu->cfg.ext_zfhmin = false;
+        cpu->cfg.ext_zfinx = false;
+        cpu->cfg.ext_zhinx = false;
+        cpu->cfg.ext_zhinxmin = false;
+        cpu->cfg.ext_zdinx = false;
+        cpu->cfg.ext_zba = false;
+        cpu->cfg.ext_zbb = false;
+        cpu->cfg.ext_zbc = false;
+        cpu->cfg.ext_zbkb = false;
+        cpu->cfg.ext_zbkc = false;
+        cpu->cfg.ext_zbkx = false;
+        cpu->cfg.ext_zbs = false;
+        cpu->cfg.ext_zk = false;
+        cpu->cfg.ext_zkn = false;
+        cpu->cfg.ext_zknd = false;
+        cpu->cfg.ext_zkne = false;
+        cpu->cfg.ext_zknh = false;
+        cpu->cfg.ext_zkr = false;
+        cpu->cfg.ext_zks = false;
+        cpu->cfg.ext_zksed = false;
+        cpu->cfg.ext_zksh = false;
+        cpu->cfg.ext_zkt = false;
+        cpu->cfg.ext_zve32f = false;
+        cpu->cfg.ext_zve64f = false;
+        cpu->cfg.ext_svinval = false;
+        cpu->cfg.ext_svnapot = false;
+        cpu->cfg.ext_svpbmt = false;
+    }
+
     if (cpu->cfg.mmu) {
         riscv_set_feature(env, RISCV_FEATURE_MMU);
     }
-- 
2.34.1

Re: [PATCH v2 7/8] target/riscv: Force disable extensions if priv spec version does not match

[Atish Patra]

On Wed, May 11, 2022 at 7:46 AM Anup Patel <apatel@ventanamicro.com> wrote:
>
> We should disable extensions in riscv_cpu_realize() if minimum required
> priv spec version is not satisfied. This also ensures that machines with
> priv spec v1.11 (or lower) cannot enable H, V, and various multi-letter
> extensions.
>
> Fixes: a775398be2e ("target/riscv: Add isa extenstion strings to the
> device tree")
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> ---
>  target/riscv/cpu.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index f3b61dfd63..25a4ba3e22 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -541,6 +541,40 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
>          set_priv_version(env, priv_version);
>      }
>
> +    /* Force disable extensions if priv spec version does not match */
> +    if (env->priv_ver < PRIV_VERSION_1_12_0) {
> +        cpu->cfg.ext_h = false;
> +        cpu->cfg.ext_v = false;
> +        cpu->cfg.ext_zfh = false;
> +        cpu->cfg.ext_zfhmin = false;
> +        cpu->cfg.ext_zfinx = false;
> +        cpu->cfg.ext_zhinx = false;
> +        cpu->cfg.ext_zhinxmin = false;
> +        cpu->cfg.ext_zdinx = false;
> +        cpu->cfg.ext_zba = false;
> +        cpu->cfg.ext_zbb = false;
> +        cpu->cfg.ext_zbc = false;
> +        cpu->cfg.ext_zbkb = false;
> +        cpu->cfg.ext_zbkc = false;
> +        cpu->cfg.ext_zbkx = false;
> +        cpu->cfg.ext_zbs = false;
> +        cpu->cfg.ext_zk = false;
> +        cpu->cfg.ext_zkn = false;
> +        cpu->cfg.ext_zknd = false;
> +        cpu->cfg.ext_zkne = false;
> +        cpu->cfg.ext_zknh = false;
> +        cpu->cfg.ext_zkr = false;
> +        cpu->cfg.ext_zks = false;
> +        cpu->cfg.ext_zksed = false;
> +        cpu->cfg.ext_zksh = false;
> +        cpu->cfg.ext_zkt = false;
> +        cpu->cfg.ext_zve32f = false;
> +        cpu->cfg.ext_zve64f = false;
> +        cpu->cfg.ext_svinval = false;
> +        cpu->cfg.ext_svnapot = false;
> +        cpu->cfg.ext_svpbmt = false;
> +    }
> +

This will not scale in future with new extensions in the next few
years. We probably need to make more intrusive changes to tie up the
min_version for extensions as well.
A brief look at the "Property" data structure and couldn't find an
easy way to do it.

For this patch,
Reviewed-by: Atish Patra <atishp@rivosinc.com>

>      if (cpu->cfg.mmu) {
>          riscv_set_feature(env, RISCV_FEATURE_MMU);
>      }
> --
> 2.34.1
>


-- 
Regards,
Atish

Re: [PATCH v2 7/8] target/riscv: Force disable extensions if priv spec version does not match

[Alistair Francis]

On Thu, May 12, 2022 at 12:52 AM Anup Patel <apatel@ventanamicro.com> wrote:
>
> We should disable extensions in riscv_cpu_realize() if minimum required
> priv spec version is not satisfied. This also ensures that machines with
> priv spec v1.11 (or lower) cannot enable H, V, and various multi-letter
> extensions.
>
> Fixes: a775398be2e ("target/riscv: Add isa extenstion strings to the
> device tree")
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>

This will potentially confuse users as we just disable the extension
without telling them.

Could we not just leave this as is and let users specify the
extensions they want? Then it's up to them to specify the correct
combinations

Alistair

> ---
>  target/riscv/cpu.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index f3b61dfd63..25a4ba3e22 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -541,6 +541,40 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
>          set_priv_version(env, priv_version);
>      }
>
> +    /* Force disable extensions if priv spec version does not match */
> +    if (env->priv_ver < PRIV_VERSION_1_12_0) {
> +        cpu->cfg.ext_h = false;
> +        cpu->cfg.ext_v = false;
> +        cpu->cfg.ext_zfh = false;
> +        cpu->cfg.ext_zfhmin = false;
> +        cpu->cfg.ext_zfinx = false;
> +        cpu->cfg.ext_zhinx = false;
> +        cpu->cfg.ext_zhinxmin = false;
> +        cpu->cfg.ext_zdinx = false;
> +        cpu->cfg.ext_zba = false;
> +        cpu->cfg.ext_zbb = false;
> +        cpu->cfg.ext_zbc = false;
> +        cpu->cfg.ext_zbkb = false;
> +        cpu->cfg.ext_zbkc = false;
> +        cpu->cfg.ext_zbkx = false;
> +        cpu->cfg.ext_zbs = false;
> +        cpu->cfg.ext_zk = false;
> +        cpu->cfg.ext_zkn = false;
> +        cpu->cfg.ext_zknd = false;
> +        cpu->cfg.ext_zkne = false;
> +        cpu->cfg.ext_zknh = false;
> +        cpu->cfg.ext_zkr = false;
> +        cpu->cfg.ext_zks = false;
> +        cpu->cfg.ext_zksed = false;
> +        cpu->cfg.ext_zksh = false;
> +        cpu->cfg.ext_zkt = false;
> +        cpu->cfg.ext_zve32f = false;
> +        cpu->cfg.ext_zve64f = false;
> +        cpu->cfg.ext_svinval = false;
> +        cpu->cfg.ext_svnapot = false;
> +        cpu->cfg.ext_svpbmt = false;
> +    }
> +
>      if (cpu->cfg.mmu) {
>          riscv_set_feature(env, RISCV_FEATURE_MMU);
>      }
> --
> 2.34.1
>
>

Re: [PATCH v2 7/8] target/riscv: Force disable extensions if priv spec version does not match

[Anup Patel]

On Tue, May 17, 2022 at 5:46 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Thu, May 12, 2022 at 12:52 AM Anup Patel <apatel@ventanamicro.com> wrote:
> >
> > We should disable extensions in riscv_cpu_realize() if minimum required
> > priv spec version is not satisfied. This also ensures that machines with
> > priv spec v1.11 (or lower) cannot enable H, V, and various multi-letter
> > extensions.
> >
> > Fixes: a775398be2e ("target/riscv: Add isa extenstion strings to the
> > device tree")
> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
>
> This will potentially confuse users as we just disable the extension
> without telling them.
>
> Could we not just leave this as is and let users specify the
> extensions they want? Then it's up to them to specify the correct
> combinations

The ISA extensions are not independent of the Priv spec version.

For example, we have bits for Sstc, Svpbmt, and Zicbo[m|p|z] extensions
in xenvcfg CSRs which are only available for Priv v1.12 spec.

We can't allow users to enable extensions which don't meet
the Priv spec version requirements.

Regards,
Anup

>
> Alistair
>
> > ---
> >  target/riscv/cpu.c | 34 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index f3b61dfd63..25a4ba3e22 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -541,6 +541,40 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> >          set_priv_version(env, priv_version);
> >      }
> >
> > +    /* Force disable extensions if priv spec version does not match */
> > +    if (env->priv_ver < PRIV_VERSION_1_12_0) {
> > +        cpu->cfg.ext_h = false;
> > +        cpu->cfg.ext_v = false;
> > +        cpu->cfg.ext_zfh = false;
> > +        cpu->cfg.ext_zfhmin = false;
> > +        cpu->cfg.ext_zfinx = false;
> > +        cpu->cfg.ext_zhinx = false;
> > +        cpu->cfg.ext_zhinxmin = false;
> > +        cpu->cfg.ext_zdinx = false;
> > +        cpu->cfg.ext_zba = false;
> > +        cpu->cfg.ext_zbb = false;
> > +        cpu->cfg.ext_zbc = false;
> > +        cpu->cfg.ext_zbkb = false;
> > +        cpu->cfg.ext_zbkc = false;
> > +        cpu->cfg.ext_zbkx = false;
> > +        cpu->cfg.ext_zbs = false;
> > +        cpu->cfg.ext_zk = false;
> > +        cpu->cfg.ext_zkn = false;
> > +        cpu->cfg.ext_zknd = false;
> > +        cpu->cfg.ext_zkne = false;
> > +        cpu->cfg.ext_zknh = false;
> > +        cpu->cfg.ext_zkr = false;
> > +        cpu->cfg.ext_zks = false;
> > +        cpu->cfg.ext_zksed = false;
> > +        cpu->cfg.ext_zksh = false;
> > +        cpu->cfg.ext_zkt = false;
> > +        cpu->cfg.ext_zve32f = false;
> > +        cpu->cfg.ext_zve64f = false;
> > +        cpu->cfg.ext_svinval = false;
> > +        cpu->cfg.ext_svnapot = false;
> > +        cpu->cfg.ext_svpbmt = false;
> > +    }
> > +
> >      if (cpu->cfg.mmu) {
> >          riscv_set_feature(env, RISCV_FEATURE_MMU);
> >      }
> > --
> > 2.34.1
> >
> >

Re: [PATCH v2 7/8] target/riscv: Force disable extensions if priv spec version does not match

[Alistair Francis]

On Fri, May 20, 2022 at 1:07 AM Anup Patel <anup@brainfault.org> wrote:
>
> On Tue, May 17, 2022 at 5:46 AM Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Thu, May 12, 2022 at 12:52 AM Anup Patel <apatel@ventanamicro.com> wrote:
> > >
> > > We should disable extensions in riscv_cpu_realize() if minimum required
> > > priv spec version is not satisfied. This also ensures that machines with
> > > priv spec v1.11 (or lower) cannot enable H, V, and various multi-letter
> > > extensions.
> > >
> > > Fixes: a775398be2e ("target/riscv: Add isa extenstion strings to the
> > > device tree")
> > > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> >
> > This will potentially confuse users as we just disable the extension
> > without telling them.
> >
> > Could we not just leave this as is and let users specify the
> > extensions they want? Then it's up to them to specify the correct
> > combinations
>
> The ISA extensions are not independent of the Priv spec version.
>
> For example, we have bits for Sstc, Svpbmt, and Zicbo[m|p|z] extensions
> in xenvcfg CSRs which are only available for Priv v1.12 spec.
>
> We can't allow users to enable extensions which don't meet
> the Priv spec version requirements.

Fair point. Ok we should at least report a warning if any of these are
set though

Alistair

>
> Regards,
> Anup
>
> >
> > Alistair
> >
> > > ---
> > >  target/riscv/cpu.c | 34 ++++++++++++++++++++++++++++++++++
> > >  1 file changed, 34 insertions(+)
> > >
> > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > index f3b61dfd63..25a4ba3e22 100644
> > > --- a/target/riscv/cpu.c
> > > +++ b/target/riscv/cpu.c
> > > @@ -541,6 +541,40 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> > >          set_priv_version(env, priv_version);
> > >      }
> > >
> > > +    /* Force disable extensions if priv spec version does not match */
> > > +    if (env->priv_ver < PRIV_VERSION_1_12_0) {
> > > +        cpu->cfg.ext_h = false;
> > > +        cpu->cfg.ext_v = false;
> > > +        cpu->cfg.ext_zfh = false;
> > > +        cpu->cfg.ext_zfhmin = false;
> > > +        cpu->cfg.ext_zfinx = false;
> > > +        cpu->cfg.ext_zhinx = false;
> > > +        cpu->cfg.ext_zhinxmin = false;
> > > +        cpu->cfg.ext_zdinx = false;
> > > +        cpu->cfg.ext_zba = false;
> > > +        cpu->cfg.ext_zbb = false;
> > > +        cpu->cfg.ext_zbc = false;
> > > +        cpu->cfg.ext_zbkb = false;
> > > +        cpu->cfg.ext_zbkc = false;
> > > +        cpu->cfg.ext_zbkx = false;
> > > +        cpu->cfg.ext_zbs = false;
> > > +        cpu->cfg.ext_zk = false;
> > > +        cpu->cfg.ext_zkn = false;
> > > +        cpu->cfg.ext_zknd = false;
> > > +        cpu->cfg.ext_zkne = false;
> > > +        cpu->cfg.ext_zknh = false;
> > > +        cpu->cfg.ext_zkr = false;
> > > +        cpu->cfg.ext_zks = false;
> > > +        cpu->cfg.ext_zksed = false;
> > > +        cpu->cfg.ext_zksh = false;
> > > +        cpu->cfg.ext_zkt = false;
> > > +        cpu->cfg.ext_zve32f = false;
> > > +        cpu->cfg.ext_zve64f = false;
> > > +        cpu->cfg.ext_svinval = false;
> > > +        cpu->cfg.ext_svnapot = false;
> > > +        cpu->cfg.ext_svpbmt = false;
> > > +    }
> > > +
> > >      if (cpu->cfg.mmu) {
> > >          riscv_set_feature(env, RISCV_FEATURE_MMU);
> > >      }
> > > --
> > > 2.34.1
> > >
> > >

Re: [PATCH v2 7/8] target/riscv: Force disable extensions if priv spec version does not match

[Anup Patel]

On Tue, May 24, 2022 at 3:22 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Fri, May 20, 2022 at 1:07 AM Anup Patel <anup@brainfault.org> wrote:
> >
> > On Tue, May 17, 2022 at 5:46 AM Alistair Francis <alistair23@gmail.com> wrote:
> > >
> > > On Thu, May 12, 2022 at 12:52 AM Anup Patel <apatel@ventanamicro.com> wrote:
> > > >
> > > > We should disable extensions in riscv_cpu_realize() if minimum required
> > > > priv spec version is not satisfied. This also ensures that machines with
> > > > priv spec v1.11 (or lower) cannot enable H, V, and various multi-letter
> > > > extensions.
> > > >
> > > > Fixes: a775398be2e ("target/riscv: Add isa extenstion strings to the
> > > > device tree")
> > > > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > >
> > > This will potentially confuse users as we just disable the extension
> > > without telling them.
> > >
> > > Could we not just leave this as is and let users specify the
> > > extensions they want? Then it's up to them to specify the correct
> > > combinations
> >
> > The ISA extensions are not independent of the Priv spec version.
> >
> > For example, we have bits for Sstc, Svpbmt, and Zicbo[m|p|z] extensions
> > in xenvcfg CSRs which are only available for Priv v1.12 spec.
> >
> > We can't allow users to enable extensions which don't meet
> > the Priv spec version requirements.
>
> Fair point. Ok we should at least report a warning if any of these are
> set though

Okay, I will update this patch accordingly.

Regards,
Anup

>
> Alistair
>
> >
> > Regards,
> > Anup
> >
> > >
> > > Alistair
> > >
> > > > ---
> > > >  target/riscv/cpu.c | 34 ++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 34 insertions(+)
> > > >
> > > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > > index f3b61dfd63..25a4ba3e22 100644
> > > > --- a/target/riscv/cpu.c
> > > > +++ b/target/riscv/cpu.c
> > > > @@ -541,6 +541,40 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> > > >          set_priv_version(env, priv_version);
> > > >      }
> > > >
> > > > +    /* Force disable extensions if priv spec version does not match */
> > > > +    if (env->priv_ver < PRIV_VERSION_1_12_0) {
> > > > +        cpu->cfg.ext_h = false;
> > > > +        cpu->cfg.ext_v = false;
> > > > +        cpu->cfg.ext_zfh = false;
> > > > +        cpu->cfg.ext_zfhmin = false;
> > > > +        cpu->cfg.ext_zfinx = false;
> > > > +        cpu->cfg.ext_zhinx = false;
> > > > +        cpu->cfg.ext_zhinxmin = false;
> > > > +        cpu->cfg.ext_zdinx = false;
> > > > +        cpu->cfg.ext_zba = false;
> > > > +        cpu->cfg.ext_zbb = false;
> > > > +        cpu->cfg.ext_zbc = false;
> > > > +        cpu->cfg.ext_zbkb = false;
> > > > +        cpu->cfg.ext_zbkc = false;
> > > > +        cpu->cfg.ext_zbkx = false;
> > > > +        cpu->cfg.ext_zbs = false;
> > > > +        cpu->cfg.ext_zk = false;
> > > > +        cpu->cfg.ext_zkn = false;
> > > > +        cpu->cfg.ext_zknd = false;
> > > > +        cpu->cfg.ext_zkne = false;
> > > > +        cpu->cfg.ext_zknh = false;
> > > > +        cpu->cfg.ext_zkr = false;
> > > > +        cpu->cfg.ext_zks = false;
> > > > +        cpu->cfg.ext_zksed = false;
> > > > +        cpu->cfg.ext_zksh = false;
> > > > +        cpu->cfg.ext_zkt = false;
> > > > +        cpu->cfg.ext_zve32f = false;
> > > > +        cpu->cfg.ext_zve64f = false;
> > > > +        cpu->cfg.ext_svinval = false;
> > > > +        cpu->cfg.ext_svnapot = false;
> > > > +        cpu->cfg.ext_svpbmt = false;
> > > > +    }
> > > > +
> > > >      if (cpu->cfg.mmu) {
> > > >          riscv_set_feature(env, RISCV_FEATURE_MMU);
> > > >      }
> > > > --
> > > > 2.34.1
> > > >
> > > >

[PATCH v2 8/8] hw/riscv: virt: Fix interrupt parent for dynamic platform devices

[Anup Patel]

When both APLIC and IMSIC are present in virt machine, the APLIC should
be used as parent interrupt controller for dynamic platform devices.

In case of  multiple sockets, we should prefer interrupt controller of
socket0 for dynamic platform devices.

Fixes: 3029fab64309 ("hw/riscv: virt: Add support for generating
platform FDT entries")
Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 hw/riscv/virt.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 3326f4db96..c576173815 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -478,10 +478,12 @@ static void create_fdt_socket_plic(RISCVVirtState *s,
     qemu_fdt_setprop_cell(mc->fdt, plic_name, "phandle",
         plic_phandles[socket]);
 
-    platform_bus_add_all_fdt_nodes(mc->fdt, plic_name,
-                                   memmap[VIRT_PLATFORM_BUS].base,
-                                   memmap[VIRT_PLATFORM_BUS].size,
-                                   VIRT_PLATFORM_BUS_IRQ);
+    if (!socket) {
+        platform_bus_add_all_fdt_nodes(mc->fdt, plic_name,
+                                       memmap[VIRT_PLATFORM_BUS].base,
+                                       memmap[VIRT_PLATFORM_BUS].size,
+                                       VIRT_PLATFORM_BUS_IRQ);
+    }
 
     g_free(plic_name);
 
@@ -561,11 +563,6 @@ static void create_fdt_imsic(RISCVVirtState *s, const MemMapEntry *memmap,
     }
     qemu_fdt_setprop_cell(mc->fdt, imsic_name, "phandle", *msi_m_phandle);
 
-    platform_bus_add_all_fdt_nodes(mc->fdt, imsic_name,
-                                   memmap[VIRT_PLATFORM_BUS].base,
-                                   memmap[VIRT_PLATFORM_BUS].size,
-                                   VIRT_PLATFORM_BUS_IRQ);
-
     g_free(imsic_name);
 
     /* S-level IMSIC node */
@@ -704,10 +701,12 @@ static void create_fdt_socket_aplic(RISCVVirtState *s,
     riscv_socket_fdt_write_id(mc, mc->fdt, aplic_name, socket);
     qemu_fdt_setprop_cell(mc->fdt, aplic_name, "phandle", aplic_s_phandle);
 
-    platform_bus_add_all_fdt_nodes(mc->fdt, aplic_name,
-                                   memmap[VIRT_PLATFORM_BUS].base,
-                                   memmap[VIRT_PLATFORM_BUS].size,
-                                   VIRT_PLATFORM_BUS_IRQ);
+    if (!socket) {
+        platform_bus_add_all_fdt_nodes(mc->fdt, aplic_name,
+                                       memmap[VIRT_PLATFORM_BUS].base,
+                                       memmap[VIRT_PLATFORM_BUS].size,
+                                       VIRT_PLATFORM_BUS_IRQ);
+    }
 
     g_free(aplic_name);
 
-- 
2.34.1

Re: [PATCH v2 8/8] hw/riscv: virt: Fix interrupt parent for dynamic platform devices

[Alistair Francis]

On Thu, May 12, 2022 at 12:53 AM Anup Patel <apatel@ventanamicro.com> wrote:
>
> When both APLIC and IMSIC are present in virt machine, the APLIC should
> be used as parent interrupt controller for dynamic platform devices.
>
> In case of  multiple sockets, we should prefer interrupt controller of
> socket0 for dynamic platform devices.
>
> Fixes: 3029fab64309 ("hw/riscv: virt: Add support for generating
> platform FDT entries")
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>

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

Alistair

> ---
>  hw/riscv/virt.c | 25 ++++++++++++-------------
>  1 file changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 3326f4db96..c576173815 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -478,10 +478,12 @@ static void create_fdt_socket_plic(RISCVVirtState *s,
>      qemu_fdt_setprop_cell(mc->fdt, plic_name, "phandle",
>          plic_phandles[socket]);
>
> -    platform_bus_add_all_fdt_nodes(mc->fdt, plic_name,
> -                                   memmap[VIRT_PLATFORM_BUS].base,
> -                                   memmap[VIRT_PLATFORM_BUS].size,
> -                                   VIRT_PLATFORM_BUS_IRQ);
> +    if (!socket) {
> +        platform_bus_add_all_fdt_nodes(mc->fdt, plic_name,
> +                                       memmap[VIRT_PLATFORM_BUS].base,
> +                                       memmap[VIRT_PLATFORM_BUS].size,
> +                                       VIRT_PLATFORM_BUS_IRQ);
> +    }
>
>      g_free(plic_name);
>
> @@ -561,11 +563,6 @@ static void create_fdt_imsic(RISCVVirtState *s, const MemMapEntry *memmap,
>      }
>      qemu_fdt_setprop_cell(mc->fdt, imsic_name, "phandle", *msi_m_phandle);
>
> -    platform_bus_add_all_fdt_nodes(mc->fdt, imsic_name,
> -                                   memmap[VIRT_PLATFORM_BUS].base,
> -                                   memmap[VIRT_PLATFORM_BUS].size,
> -                                   VIRT_PLATFORM_BUS_IRQ);
> -
>      g_free(imsic_name);
>
>      /* S-level IMSIC node */
> @@ -704,10 +701,12 @@ static void create_fdt_socket_aplic(RISCVVirtState *s,
>      riscv_socket_fdt_write_id(mc, mc->fdt, aplic_name, socket);
>      qemu_fdt_setprop_cell(mc->fdt, aplic_name, "phandle", aplic_s_phandle);
>
> -    platform_bus_add_all_fdt_nodes(mc->fdt, aplic_name,
> -                                   memmap[VIRT_PLATFORM_BUS].base,
> -                                   memmap[VIRT_PLATFORM_BUS].size,
> -                                   VIRT_PLATFORM_BUS_IRQ);
> +    if (!socket) {
> +        platform_bus_add_all_fdt_nodes(mc->fdt, aplic_name,
> +                                       memmap[VIRT_PLATFORM_BUS].base,
> +                                       memmap[VIRT_PLATFORM_BUS].size,
> +                                       VIRT_PLATFORM_BUS_IRQ);
> +    }
>
>      g_free(aplic_name);
>
> --
> 2.34.1
>
>

Re: [PATCH v2 0/8] QEMU RISC-V nested virtualization fixes

[Alistair Francis]

On Thu, May 12, 2022 at 12:47 AM Anup Patel <apatel@ventanamicro.com> wrote:
>
> This series does fixes and improvements to have nested virtualization
> on QEMU RISC-V.
>
> These patches can also be found in riscv_nested_fixes_v2 branch at:
> https://github.com/avpatel/qemu.git
>
> The RISC-V nested virtualization was tested on QEMU RISC-V using
> Xvisor RISC-V which has required hypervisor support to run another
> hypervisor as Guest/VM.
>
> Changes since v1:
>  - Set write_gva to env->two_stage_lookup which ensures that for
>    HS-mode to HS-mode trap write_gva is true only for HLV/HSV
>    instructions
>  - Included "[PATCH 0/3] QEMU RISC-V priv spec version fixes"
>    patches in this series for easy review
>  - Re-worked PATCH7 to force disable extensions if required
>    priv spec version is not staisfied
>  - Added new PATCH8 to fix "aia=aplic-imsic" mode of virt machine
>
> Anup Patel (8):
>   target/riscv: Fix csr number based privilege checking
>   target/riscv: Fix hstatus.GVA bit setting for traps taken from HS-mode
>   target/riscv: Set [m|s]tval for both illegal and virtual instruction
>     traps
>   target/riscv: Update [m|h]tinst CSR in riscv_cpu_do_interrupt()
>   target/riscv: Don't force update priv spec version to latest
>   target/riscv: Add dummy mcountinhibit CSR for priv spec v1.11 or
>     higher
>   target/riscv: Force disable extensions if priv spec version does not
>     match
>   hw/riscv: virt: Fix interrupt parent for dynamic platform devices

Thanks!

I have applied some of these patches to riscv-to-apply.next

Alistair

>
>  hw/riscv/virt.c           |  25 +++---
>  target/riscv/cpu.c        |  46 +++++++++-
>  target/riscv/cpu.h        |   8 +-
>  target/riscv/cpu_bits.h   |   3 +
>  target/riscv/cpu_helper.c | 172 ++++++++++++++++++++++++++++++++++++--
>  target/riscv/csr.c        |  10 ++-
>  target/riscv/instmap.h    |  41 +++++++++
>  target/riscv/translate.c  |  17 +++-
>  8 files changed, 292 insertions(+), 30 deletions(-)
>
> --
> 2.34.1
>
>