[PATCH v2 1/5] [RISCV_PM] Add J-extension into RISC-V

[PATCH v2 1/5] [RISCV_PM] Add J-extension into RISC-V

[Alexey Baturo]

Signed-off-by: Alexey Baturo <space.monkey.delivers@gmail.com>
---
 target/riscv/cpu.c | 4 ++++
 target/riscv/cpu.h | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 0bbfd7f457..fe6bab4a52 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -438,6 +438,9 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
         if (cpu->cfg.ext_h) {
             target_misa |= RVH;
         }
+        if (cpu->cfg.ext_j) {
+            target_misa |= RVJ;
+        }
         if (cpu->cfg.ext_v) {
             target_misa |= RVV;
             if (!is_power_of_2(cpu->cfg.vlen)) {
@@ -516,6 +519,7 @@ static Property riscv_cpu_properties[] = {
     DEFINE_PROP_BOOL("u", RISCVCPU, cfg.ext_u, true),
     /* This is experimental so mark with 'x-' */
     DEFINE_PROP_BOOL("x-h", RISCVCPU, cfg.ext_h, false),
+    DEFINE_PROP_BOOL("x-j", RISCVCPU, cfg.ext_j, false),
     DEFINE_PROP_BOOL("x-v", RISCVCPU, cfg.ext_v, false),
     DEFINE_PROP_BOOL("Counters", RISCVCPU, cfg.ext_counters, true),
     DEFINE_PROP_BOOL("Zifencei", RISCVCPU, cfg.ext_ifencei, true),
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index de275782e6..eca611a367 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -66,6 +66,7 @@
 #define RVS RV('S')
 #define RVU RV('U')
 #define RVH RV('H')
+#define RVJ RV('J')
 
 /* S extension denotes that Supervisor mode exists, however it is possible
    to have a core that support S mode but does not have an MMU and there
@@ -277,6 +278,7 @@ struct RISCVCPU {
         bool ext_s;
         bool ext_u;
         bool ext_h;
+        bool ext_j;
         bool ext_v;
         bool ext_counters;
         bool ext_ifencei;
-- 
2.20.1

[PATCH v2 2/5] [RISCV_PM] Support CSRs required for RISC-V PM extension except for ones in hypervisor mode

[Alexey Baturo]

Signed-off-by: Alexey Baturo <space.monkey.delivers@gmail.com>
---
 target/riscv/cpu.c      |   1 +
 target/riscv/cpu.h      |  11 ++
 target/riscv/cpu_bits.h |  66 ++++++++++
 target/riscv/csr.c      | 277 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 355 insertions(+)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index fe6bab4a52..d63031eb08 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -440,6 +440,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
         }
         if (cpu->cfg.ext_j) {
             target_misa |= RVJ;
+            env->mmte |= PM_EXT_INITIAL;
         }
         if (cpu->cfg.ext_v) {
             target_misa |= RVV;
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index eca611a367..21e47b8283 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -226,6 +226,17 @@ struct CPURISCVState {
 
     /* True if in debugger mode.  */
     bool debugger;
+
+    /* CSRs for PM
+     * TODO: move these csr to appropriate groups
+     */
+    target_ulong mmte;
+    target_ulong mpmmask;
+    target_ulong mpmbase;
+    target_ulong spmmask;
+    target_ulong spmbase;
+    target_ulong upmmask;
+    target_ulong upmbase;
 #endif
 
     float_status fp_status;
diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
index bd36062877..84c93c77ae 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -354,6 +354,21 @@
 #define CSR_MHPMCOUNTER30H  0xb9e
 #define CSR_MHPMCOUNTER31H  0xb9f
 
+/* Custom user register */
+#define CSR_UMTE            0x8c0
+#define CSR_UPMMASK         0x8c1
+#define CSR_UPMBASE         0x8c2
+
+/* Custom machine register */
+#define CSR_MMTE            0x7c0
+#define CSR_MPMMASK         0x7c1
+#define CSR_MPMBASE         0x7c2
+
+/* Custom supervisor register */
+#define CSR_SMTE            0x9c0
+#define CSR_SPMMASK         0x9c1
+#define CSR_SPMBASE         0x9c2
+
 /* Legacy Machine Protection and Translation (priv v1.9.1) */
 #define CSR_MBASE           0x380
 #define CSR_MBOUND          0x381
@@ -604,4 +619,55 @@
 #define MIE_UTIE                           (1 << IRQ_U_TIMER)
 #define MIE_SSIE                           (1 << IRQ_S_SOFT)
 #define MIE_USIE                           (1 << IRQ_U_SOFT)
+
+/* general mte CSR bits*/
+#define PM_ENABLE       0x00000001ULL
+#define PM_CURRENT      0x00000002ULL
+#define PM_XS_MASK      0x00000003ULL
+
+/* PM XS bits values */
+#define PM_EXT_DISABLE  0x00000000ULL
+#define PM_EXT_INITIAL  0x00000001ULL
+#define PM_EXT_CLEAN    0x00000002ULL
+#define PM_EXT_DIRTY    0x00000003ULL
+
+/* offsets for every pair of control bits per each priv level */
+#define XS_OFFSET    0ULL
+#define U_OFFSET     2ULL
+#define S_OFFSET     4ULL
+#define M_OFFSET     6ULL
+
+#define PM_XS_BITS   (PM_XS_MASK << XS_OFFSET)
+#define U_PM_ENABLE  (PM_ENABLE  << U_OFFSET)
+#define U_PM_CURRENT (PM_CURRENT << U_OFFSET)
+#define S_PM_ENABLE  (PM_ENABLE  << S_OFFSET)
+#define S_PM_CURRENT (PM_CURRENT << S_OFFSET)
+#define M_PM_ENABLE  (PM_ENABLE  << M_OFFSET)
+
+/* mmte CSR bits */
+#define MMTE_PM_XS_BITS     PM_XS_BITS
+#define MMTE_U_PM_ENABLE    U_PM_ENABLE
+#define MMTE_U_PM_CURRENT   U_PM_CURRENT
+#define MMTE_S_PM_ENABLE    S_PM_ENABLE
+#define MMTE_S_PM_CURRENT   S_PM_CURRENT
+#define MMTE_M_PM_ENABLE    M_PM_ENABLE
+#define MMTE_MASK           (MMTE_U_PM_ENABLE | MMTE_U_PM_CURRENT | \
+                             MMTE_S_PM_ENABLE | MMTE_S_PM_CURRENT | \
+                             MMTE_M_PM_ENABLE | MMTE_PM_XS_BITS)
+
+/* smte CSR bits */
+#define SMTE_PM_XS_BITS     PM_XS_BITS
+#define SMTE_U_PM_ENABLE    U_PM_ENABLE
+#define SMTE_U_PM_CURRENT   U_PM_CURRENT
+#define SMTE_S_PM_ENABLE    S_PM_ENABLE
+#define SMTE_S_PM_CURRENT   S_PM_CURRENT
+#define SMTE_MASK           (SMTE_U_PM_ENABLE | SMTE_U_PM_CURRENT | \
+                             SMTE_S_PM_ENABLE | SMTE_S_PM_CURRENT | \
+                             SMTE_PM_XS_BITS)
+
+/* umte CSR bits */
+#define UMTE_U_PM_ENABLE    U_PM_ENABLE
+#define UMTE_U_PM_CURRENT   U_PM_CURRENT
+#define UMTE_MASK           (UMTE_U_PM_ENABLE | MMTE_U_PM_CURRENT)
+
 #endif
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index aaef6c6f20..f48f4c4070 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -140,6 +140,11 @@ static int any(CPURISCVState *env, int csrno)
     return 0;
 }
 
+static int umode(CPURISCVState *env, int csrno)
+{
+    return -!riscv_has_ext(env, RVU);
+}
+
 static int smode(CPURISCVState *env, int csrno)
 {
     return -!riscv_has_ext(env, RVS);
@@ -1250,6 +1255,263 @@ static int write_pmpaddr(CPURISCVState *env, int csrno, target_ulong val)
     return 0;
 }
 
+/* Functions to access Pointer Masking feature registers 
+ * We have to check if current priv lvl could modify
+ * csr in given mode
+ */
+static int check_pm_current_disabled(CPURISCVState *env, int csrno)
+{
+    /* m-mode is always allowed to modify registers, so allow */
+    if (env->priv == PRV_M) {
+        return 0;
+    }
+    int csr_priv = get_field(csrno, 0xC00);
+    /* If priv lvls differ that means we're accessing csr from higher priv lvl, so allow */
+    if (env->priv != csr_priv) {
+        return 0;
+    }
+    int cur_bit_pos = (env->priv == PRV_U) ? U_PM_CURRENT :
+                      (env->priv == PRV_S) ? S_PM_CURRENT : -1;
+    assert(cur_bit_pos != -1);
+    int pm_current = get_field(env->mmte, cur_bit_pos);
+    /* We're in same priv lvl, so we allow to modify csr only if pm_current==1 */
+    return !pm_current;
+}
+
+static int read_mmte(CPURISCVState *env, int csrno, target_ulong *val)
+{
+#if !defined(CONFIG_USER_ONLY)
+    if (!riscv_has_ext(env, RVJ)) {
+        *val = 0;
+        return 0;
+    }
+#endif
+    *val = env->mmte & MMTE_MASK;
+    return 0;
+}
+
+static int write_mmte(CPURISCVState *env, int csrno, target_ulong val)
+{
+#if !defined(CONFIG_USER_ONLY)
+    if (!riscv_has_ext(env, RVJ)) {
+        return -RISCV_EXCP_ILLEGAL_INST;
+    }
+#endif
+    target_ulong wpri_val = val & MMTE_MASK;
+    assert(val == wpri_val);
+    env->mmte = val;
+    env->mstatus |= MSTATUS_XS | MSTATUS_SD;
+    env->mmte |= PM_EXT_DIRTY;
+    return 0;
+}
+
+static int read_smte(CPURISCVState *env, int csrno, target_ulong *val)
+{
+#if !defined(CONFIG_USER_ONLY)
+    if (!riscv_has_ext(env, RVJ)) {
+        *val = 0;
+        return 0;
+    }
+#endif
+    *val = env->mmte & SMTE_MASK;
+    return 0;
+}
+
+static int write_smte(CPURISCVState *env, int csrno, target_ulong val)
+{
+#if !defined(CONFIG_USER_ONLY)
+    if (!riscv_has_ext(env, RVJ)) {
+        return -RISCV_EXCP_ILLEGAL_INST;
+    }
+#endif
+    target_ulong wpri_val = val & SMTE_MASK;
+    assert(val == wpri_val);
+    if (check_pm_current_disabled(env, csrno))
+        return 0;
+    target_ulong new_val = val | (env->mmte & ~SMTE_MASK);
+    write_mmte(env, csrno, new_val);
+    return 0;
+}
+
+static int read_umte(CPURISCVState *env, int csrno, target_ulong *val)
+{
+#if !defined(CONFIG_USER_ONLY)
+    if (!riscv_has_ext(env, RVJ)) {
+        *val = 0;
+        return 0;
+    }
+#endif
+    *val = env->mmte & UMTE_MASK;
+    return 0;
+}
+
+static int write_umte(CPURISCVState *env, int csrno, target_ulong val)
+{
+#if !defined(CONFIG_USER_ONLY)
+    if (!riscv_has_ext(env, RVJ)) {
+        return -RISCV_EXCP_ILLEGAL_INST;
+    }
+#endif
+    target_ulong wpri_val = val & UMTE_MASK;
+    assert(val == wpri_val);
+    if (check_pm_current_disabled(env, csrno))
+        return 0;
+    target_ulong new_val = val | (env->mmte & ~UMTE_MASK);
+    write_mmte(env, csrno, new_val);
+    return 0;
+}
+
+static int read_mpmmask(CPURISCVState *env, int csrno, target_ulong *val)
+{
+#if !defined(CONFIG_USER_ONLY)
+    if (!riscv_has_ext(env, RVJ)) {
+        return -RISCV_EXCP_ILLEGAL_INST;
+    }
+#endif
+    *val = env->mpmmask;
+    return 0;
+}
+
+static int write_mpmmask(CPURISCVState *env, int csrno, target_ulong val)
+{
+#if !defined(CONFIG_USER_ONLY)
+    if (!riscv_has_ext(env, RVJ)) {
+        return -RISCV_EXCP_ILLEGAL_INST;
+    }
+#endif
+    env->mpmmask = val;
+    env->mstatus |= MSTATUS_XS | MSTATUS_SD;
+    env->mmte |= PM_EXT_DIRTY;
+    return 0;
+}
+
+static int read_spmmask(CPURISCVState *env, int csrno, target_ulong *val)
+{
+#if !defined(CONFIG_USER_ONLY)
+    if (!riscv_has_ext(env, RVJ)) {
+        return -RISCV_EXCP_ILLEGAL_INST;
+    }
+#endif
+    *val = env->spmmask;
+    return 0;
+}
+
+static int write_spmmask(CPURISCVState *env, int csrno, target_ulong val)
+{
+#if !defined(CONFIG_USER_ONLY)
+    if (!riscv_has_ext(env, RVJ)) {
+        return -RISCV_EXCP_ILLEGAL_INST;
+    }
+#endif
+    if (check_pm_current_disabled(env, csrno))
+        return 0;
+    env->spmmask = val;
+    env->mstatus |= MSTATUS_XS | MSTATUS_SD;
+    env->mmte |= PM_EXT_DIRTY;
+    return 0;
+}
+
+static int read_upmmask(CPURISCVState *env, int csrno, target_ulong *val)
+{
+#if !defined(CONFIG_USER_ONLY)
+    if (!riscv_has_ext(env, RVJ)) {
+        return -RISCV_EXCP_ILLEGAL_INST;
+    }
+#endif
+    *val = env->upmmask;
+    return 0;
+}
+
+static int write_upmmask(CPURISCVState *env, int csrno, target_ulong val)
+{
+#if !defined(CONFIG_USER_ONLY)
+    if (!riscv_has_ext(env, RVJ)) {
+        return -RISCV_EXCP_ILLEGAL_INST;
+    }
+#endif
+    if (check_pm_current_disabled(env, csrno))
+        return 0;
+    env->upmmask = val;
+    env->mstatus |= MSTATUS_XS | MSTATUS_SD;
+    env->mmte |= PM_EXT_DIRTY;
+    return 0;
+}
+
+static int read_mpmbase(CPURISCVState *env, int csrno, target_ulong *val)
+{
+#if !defined(CONFIG_USER_ONLY)
+    if (!riscv_has_ext(env, RVJ)) {
+        return -RISCV_EXCP_ILLEGAL_INST;
+    }
+#endif
+    *val = env->mpmbase;
+    return 0;
+}
+
+static int write_mpmbase(CPURISCVState *env, int csrno, target_ulong val)
+{
+#if !defined(CONFIG_USER_ONLY)
+    if (!riscv_has_ext(env, RVJ)) {
+        return -RISCV_EXCP_ILLEGAL_INST;
+    }
+#endif
+    env->mpmbase = val;
+    env->mstatus |= MSTATUS_XS | MSTATUS_SD;
+    env->mmte |= PM_EXT_DIRTY;
+    return 0;
+}
+
+static int read_spmbase(CPURISCVState *env, int csrno, target_ulong *val)
+{
+#if !defined(CONFIG_USER_ONLY)
+    if (!riscv_has_ext(env, RVJ)) {
+        return -RISCV_EXCP_ILLEGAL_INST;
+    }
+#endif
+    *val = env->spmbase;
+    return 0;
+}
+
+static int write_spmbase(CPURISCVState *env, int csrno, target_ulong val)
+{
+#if !defined(CONFIG_USER_ONLY)
+    if (!riscv_has_ext(env, RVJ)) {
+        return -RISCV_EXCP_ILLEGAL_INST;
+    }
+#endif
+    if (check_pm_current_disabled(env, csrno))
+        return 0;
+    env->spmbase = val;
+    env->mstatus |= MSTATUS_XS | MSTATUS_SD;
+    env->mmte |= PM_EXT_DIRTY;
+    return 0;
+}
+
+static int read_upmbase(CPURISCVState *env, int csrno, target_ulong *val)
+{
+#if !defined(CONFIG_USER_ONLY)
+    if (!riscv_has_ext(env, RVJ)) {
+        return -RISCV_EXCP_ILLEGAL_INST;
+    }
+#endif
+    *val = env->upmbase;
+    return 0;
+}
+
+static int write_upmbase(CPURISCVState *env, int csrno, target_ulong val)
+{
+#if !defined(CONFIG_USER_ONLY)
+    if (!riscv_has_ext(env, RVJ)) {
+        return -RISCV_EXCP_ILLEGAL_INST;
+    }
+#endif
+    if (check_pm_current_disabled(env, csrno))
+        return 0;
+    env->upmbase = val;
+    env->mstatus |= MSTATUS_XS | MSTATUS_SD;
+    env->mmte |= PM_EXT_DIRTY;
+    return 0;
+}
 #endif
 
 /*
@@ -1471,6 +1733,21 @@ static riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
     [CSR_PMPCFG0  ... CSR_PMPCFG3]   = { pmp,   read_pmpcfg,  write_pmpcfg   },
     [CSR_PMPADDR0 ... CSR_PMPADDR15] = { pmp,   read_pmpaddr, write_pmpaddr  },
 
+    /* User Pointer Masking */
+    [CSR_UMTE] =                { umode,  read_umte,        write_umte        },
+    [CSR_UPMMASK] =             { umode,  read_upmmask,     write_upmmask     },
+    [CSR_UPMBASE] =             { umode,  read_upmbase,     write_upmbase     },
+
+    /* Machine Pointer Masking */
+    [CSR_MMTE] =                { any,  read_mmte,        write_mmte        },
+    [CSR_MPMMASK] =             { any,  read_mpmmask,     write_mpmmask     },
+    [CSR_MPMBASE] =             { any,  read_mpmbase,     write_mpmbase     },
+
+    /* Supervisor Pointer Masking */
+    [CSR_SMTE] =                { smode, read_smte,        write_smte        },
+    [CSR_SPMMASK] =             { smode, read_spmmask,     write_spmmask     },
+    [CSR_SPMBASE] =             { smode, read_spmbase,     write_spmbase     },
+
     /* Performance Counters */
     [CSR_HPMCOUNTER3   ... CSR_HPMCOUNTER31] =    { ctr,  read_zero          },
     [CSR_MHPMCOUNTER3  ... CSR_MHPMCOUNTER31] =   { any,  read_zero          },
-- 
2.20.1

[PATCH v2 3/5] [RISCV_PM] Print new PM CSRs in QEMU logs

[Alexey Baturo]

Signed-off-by: Alexey Baturo <space.monkey.delivers@gmail.com>
---
 target/riscv/cpu.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index d63031eb08..6ba3e98508 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -255,6 +255,25 @@ static void riscv_cpu_dump_state(CPUState *cs, FILE *f, int flags)
         qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "htval ", env->htval);
         qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mtval2 ", env->mtval2);
     }
+    if (riscv_has_ext(env, RVJ)) {
+        qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mmte    ", env->mmte);
+        switch (env->priv) {
+        case PRV_U:
+            qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "upmbase ", env->upmbase);
+            qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "upmmask ", env->upmmask);
+            break;
+        case PRV_S:
+            qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "spmbase ", env->spmbase);
+            qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "spmmask ", env->spmmask);
+            break;
+        case PRV_M:
+            qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mpmbase ", env->mpmbase);
+            qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mpmmask ", env->mpmmask);
+            break;
+        default:
+            assert(0 && "Unreachable");
+        }
+    }
 #endif
 
     for (i = 0; i < 32; i++) {
-- 
2.20.1

[PATCH v2 4/5] [RISCV_PM] Support pointer masking for RISC-V for i/c/f/d/a types of instructions

[Alexey Baturo]

Signed-off-by: Alexey Baturo <space.monkey.delivers@gmail.com>
---
 target/riscv/insn_trans/trans_rva.c.inc |  3 +++
 target/riscv/insn_trans/trans_rvd.c.inc |  2 ++
 target/riscv/insn_trans/trans_rvf.c.inc |  2 ++
 target/riscv/insn_trans/trans_rvi.c.inc |  2 ++
 target/riscv/translate.c                | 14 ++++++++++++++
 5 files changed, 23 insertions(+)

diff --git a/target/riscv/insn_trans/trans_rva.c.inc b/target/riscv/insn_trans/trans_rva.c.inc
index be8a9f06dd..5559e347ba 100644
--- a/target/riscv/insn_trans/trans_rva.c.inc
+++ b/target/riscv/insn_trans/trans_rva.c.inc
@@ -26,6 +26,7 @@ static inline bool gen_lr(DisasContext *ctx, arg_atomic *a, MemOp mop)
     if (a->rl) {
         tcg_gen_mb(TCG_MO_ALL | TCG_BAR_STRL);
     }
+    gen_pm_adjust_address(ctx, src1, src1);
     tcg_gen_qemu_ld_tl(load_val, src1, ctx->mem_idx, mop);
     if (a->aq) {
         tcg_gen_mb(TCG_MO_ALL | TCG_BAR_LDAQ);
@@ -46,6 +47,7 @@ static inline bool gen_sc(DisasContext *ctx, arg_atomic *a, MemOp mop)
     TCGLabel *l2 = gen_new_label();
 
     gen_get_gpr(src1, a->rs1);
+    gen_pm_adjust_address(ctx, src1, src1);
     tcg_gen_brcond_tl(TCG_COND_NE, load_res, src1, l1);
 
     gen_get_gpr(src2, a->rs2);
@@ -91,6 +93,7 @@ static bool gen_amo(DisasContext *ctx, arg_atomic *a,
     gen_get_gpr(src1, a->rs1);
     gen_get_gpr(src2, a->rs2);
 
+    gen_pm_adjust_address(ctx, src1, src1);
     (*func)(src2, src1, src2, ctx->mem_idx, mop);
 
     gen_set_gpr(a->rd, src2);
diff --git a/target/riscv/insn_trans/trans_rvd.c.inc b/target/riscv/insn_trans/trans_rvd.c.inc
index 4f832637fa..935342f66d 100644
--- a/target/riscv/insn_trans/trans_rvd.c.inc
+++ b/target/riscv/insn_trans/trans_rvd.c.inc
@@ -25,6 +25,7 @@ static bool trans_fld(DisasContext *ctx, arg_fld *a)
     TCGv t0 = tcg_temp_new();
     gen_get_gpr(t0, a->rs1);
     tcg_gen_addi_tl(t0, t0, a->imm);
+    gen_pm_adjust_address(ctx, t0, t0);
 
     tcg_gen_qemu_ld_i64(cpu_fpr[a->rd], t0, ctx->mem_idx, MO_TEQ);
 
@@ -40,6 +41,7 @@ static bool trans_fsd(DisasContext *ctx, arg_fsd *a)
     TCGv t0 = tcg_temp_new();
     gen_get_gpr(t0, a->rs1);
     tcg_gen_addi_tl(t0, t0, a->imm);
+    gen_pm_adjust_address(ctx, t0, t0);
 
     tcg_gen_qemu_st_i64(cpu_fpr[a->rs2], t0, ctx->mem_idx, MO_TEQ);
 
diff --git a/target/riscv/insn_trans/trans_rvf.c.inc b/target/riscv/insn_trans/trans_rvf.c.inc
index 3dfec8211d..04b3c3eb3d 100644
--- a/target/riscv/insn_trans/trans_rvf.c.inc
+++ b/target/riscv/insn_trans/trans_rvf.c.inc
@@ -30,6 +30,7 @@ static bool trans_flw(DisasContext *ctx, arg_flw *a)
     TCGv t0 = tcg_temp_new();
     gen_get_gpr(t0, a->rs1);
     tcg_gen_addi_tl(t0, t0, a->imm);
+    gen_pm_adjust_address(ctx, t0, t0);
 
     tcg_gen_qemu_ld_i64(cpu_fpr[a->rd], t0, ctx->mem_idx, MO_TEUL);
     gen_nanbox_s(cpu_fpr[a->rd], cpu_fpr[a->rd]);
@@ -47,6 +48,7 @@ static bool trans_fsw(DisasContext *ctx, arg_fsw *a)
     gen_get_gpr(t0, a->rs1);
 
     tcg_gen_addi_tl(t0, t0, a->imm);
+    gen_pm_adjust_address(ctx, t0, t0);
 
     tcg_gen_qemu_st_i64(cpu_fpr[a->rs2], t0, ctx->mem_idx, MO_TEUL);
 
diff --git a/target/riscv/insn_trans/trans_rvi.c.inc b/target/riscv/insn_trans/trans_rvi.c.inc
index d04ca0394c..bee7f6be46 100644
--- a/target/riscv/insn_trans/trans_rvi.c.inc
+++ b/target/riscv/insn_trans/trans_rvi.c.inc
@@ -141,6 +141,7 @@ static bool gen_load(DisasContext *ctx, arg_lb *a, MemOp memop)
     TCGv t1 = tcg_temp_new();
     gen_get_gpr(t0, a->rs1);
     tcg_gen_addi_tl(t0, t0, a->imm);
+    gen_pm_adjust_address(ctx, t0, t0);
 
     tcg_gen_qemu_ld_tl(t1, t0, ctx->mem_idx, memop);
     gen_set_gpr(a->rd, t1);
@@ -180,6 +181,7 @@ static bool gen_store(DisasContext *ctx, arg_sb *a, MemOp memop)
     TCGv dat = tcg_temp_new();
     gen_get_gpr(t0, a->rs1);
     tcg_gen_addi_tl(t0, t0, a->imm);
+    gen_pm_adjust_address(ctx, t0, t0);
     gen_get_gpr(dat, a->rs2);
 
     tcg_gen_qemu_st_tl(dat, t0, ctx->mem_idx, memop);
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 79dca2291b..a7cbf909f3 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -101,6 +101,16 @@ static void gen_nanbox_s(TCGv_i64 out, TCGv_i64 in)
     tcg_gen_ori_i64(out, in, MAKE_64BIT_MASK(32, 32));
 }
 
+/*
+ * Temp stub: generates address adjustment for PointerMasking
+ */
+static void gen_pm_adjust_address(DisasContext *s,
+                                  TCGv_i64      dst,
+                                  TCGv_i64      src)
+{
+    tcg_gen_mov_i64(dst, src);
+}
+
 /*
  * A narrow n-bit operation, where n < FLEN, checks that input operands
  * are correctly Nan-boxed, i.e., all upper FLEN - n bits are 1.
@@ -380,6 +390,7 @@ static void gen_load_c(DisasContext *ctx, uint32_t opc, int rd, int rs1,
     TCGv t1 = tcg_temp_new();
     gen_get_gpr(t0, rs1);
     tcg_gen_addi_tl(t0, t0, imm);
+    gen_pm_adjust_address(ctx, t0, t0);
     int memop = tcg_memop_lookup[(opc >> 12) & 0x7];
 
     if (memop < 0) {
@@ -400,6 +411,7 @@ static void gen_store_c(DisasContext *ctx, uint32_t opc, int rs1, int rs2,
     TCGv dat = tcg_temp_new();
     gen_get_gpr(t0, rs1);
     tcg_gen_addi_tl(t0, t0, imm);
+    gen_pm_adjust_address(ctx, t0, t0);
     gen_get_gpr(dat, rs2);
     int memop = tcg_memop_lookup[(opc >> 12) & 0x7];
 
@@ -459,6 +471,7 @@ static void gen_fp_load(DisasContext *ctx, uint32_t opc, int rd,
     t0 = tcg_temp_new();
     gen_get_gpr(t0, rs1);
     tcg_gen_addi_tl(t0, t0, imm);
+    gen_pm_adjust_address(ctx, t0, t0);
 
     switch (opc) {
     case OPC_RISC_FLW:
@@ -498,6 +511,7 @@ static void gen_fp_store(DisasContext *ctx, uint32_t opc, int rs1,
     t0 = tcg_temp_new();
     gen_get_gpr(t0, rs1);
     tcg_gen_addi_tl(t0, t0, imm);
+    gen_pm_adjust_address(ctx, t0, t0);
 
     switch (opc) {
     case OPC_RISC_FSW:
-- 
2.20.1

[PATCH v2 5/5] [RISCV_PM] Implement address masking functions required for RISC-V Pointer Masking extension

[Alexey Baturo]

From: Anatoly Parshintsev <kupokupokupopo@gmail.com>

Signed-off-by: Anatoly Parshintsev <kupokupokupopo@gmail.com>
---
 target/riscv/cpu.h       | 19 +++++++++++++++++++
 target/riscv/translate.c | 39 +++++++++++++++++++++++++++++++++++++--
 2 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 21e47b8283..6c301b7ab1 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -385,6 +385,7 @@ FIELD(TB_FLAGS, VL_EQ_VLMAX, 2, 1)
 FIELD(TB_FLAGS, LMUL, 3, 2)
 FIELD(TB_FLAGS, SEW, 5, 3)
 FIELD(TB_FLAGS, VILL, 8, 1)
+FIELD(TB_FLAGS, PM_ENABLED, 9, 1)
 
 /*
  * A simplification for VLMAX
@@ -431,6 +432,24 @@ static inline void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc,
     if (riscv_cpu_fp_enabled(env)) {
         flags |= env->mstatus & MSTATUS_FS;
     }
+    if (riscv_has_ext(env, RVJ)) {
+        int priv = cpu_mmu_index(env, false);
+        bool pm_enabled = false;
+        switch (priv) {
+        case PRV_U:
+            pm_enabled = env->mmte & U_PM_ENABLE;
+            break;
+        case PRV_S:
+            pm_enabled = env->mmte & S_PM_ENABLE;
+            break;
+        case PRV_M:
+            pm_enabled = env->mmte & M_PM_ENABLE;
+            break;
+        default:
+            assert(0 && "Unreachable");
+        }
+        flags = FIELD_DP32(flags, TB_FLAGS, PM_ENABLED, pm_enabled);
+    }
 #endif
     *pflags = flags;
 }
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index a7cbf909f3..58b05ee2c7 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -36,6 +36,9 @@ static TCGv cpu_gpr[32], cpu_pc, cpu_vl;
 static TCGv_i64 cpu_fpr[32]; /* assume F and D extensions */
 static TCGv load_res;
 static TCGv load_val;
+/* globals for PM CSRs */
+static TCGv pm_mask[4];
+static TCGv pm_base[4];
 
 #include "exec/gen-icount.h"
 
@@ -63,6 +66,10 @@ typedef struct DisasContext {
     uint16_t vlen;
     uint16_t mlen;
     bool vl_eq_vlmax;
+    /* PointerMasking extension */
+    uint8_t pm_enabled;
+    TCGv pm_mask;
+    TCGv pm_base;
 } DisasContext;
 
 #ifdef TARGET_RISCV64
@@ -102,13 +109,19 @@ static void gen_nanbox_s(TCGv_i64 out, TCGv_i64 in)
 }
 
 /*
- * Temp stub: generates address adjustment for PointerMasking
+ * Generates address adjustment for PointerMasking
  */
 static void gen_pm_adjust_address(DisasContext *s,
                                   TCGv_i64      dst,
                                   TCGv_i64      src)
 {
-    tcg_gen_mov_i64(dst, src);
+    if (s->pm_enabled == 0) {
+        /* Load unmodified address */
+        tcg_gen_mov_i64(dst, src);
+    } else {
+        tcg_gen_andc_i64(dst, src, s->pm_mask);
+        tcg_gen_or_i64(dst, dst, s->pm_base);
+    }
 }
 
 /*
@@ -814,8 +827,17 @@ static void riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
     } else {
         ctx->virt_enabled = false;
     }
+    if (riscv_has_ext(env, RVJ)) {
+        ctx->pm_enabled = FIELD_EX32(tb_flags, TB_FLAGS, PM_ENABLED);
+        int priv = cpu_mmu_index(env, false);
+        ctx->pm_mask = pm_mask[priv];
+        ctx->pm_base = pm_base[priv];
+    } else {
+        ctx->pm_enabled = 0;
+    }
 #else
     ctx->virt_enabled = false;
+    ctx->pm_enabled = 0;
 #endif
     ctx->misa = env->misa;
     ctx->frm = -1;  /* unknown rounding mode */
@@ -945,4 +967,17 @@ void riscv_translate_init(void)
                              "load_res");
     load_val = tcg_global_mem_new(cpu_env, offsetof(CPURISCVState, load_val),
                              "load_val");
+    /* Assign PM CSRs to tcg globals */
+    pm_mask[PRV_U] = tcg_global_mem_new(cpu_env, offsetof(CPURISCVState, upmmask),
+                                        "upmmask");
+    pm_base[PRV_U] = tcg_global_mem_new(cpu_env, offsetof(CPURISCVState, upmbase),
+                                        "upmbase");
+    pm_mask[PRV_S] = tcg_global_mem_new(cpu_env, offsetof(CPURISCVState, spmmask),
+                                        "spmmask");
+    pm_base[PRV_S] = tcg_global_mem_new(cpu_env, offsetof(CPURISCVState, spmbase),
+                                        "spmbase");
+    pm_mask[PRV_M] = tcg_global_mem_new(cpu_env, offsetof(CPURISCVState, mpmmask),
+                                        "mpmmask");
+    pm_base[PRV_M] = tcg_global_mem_new(cpu_env, offsetof(CPURISCVState, mpmbase),
+                                        "mpmbase");
 }
-- 
2.20.1

Re: [PATCH v2 2/5] [RISCV_PM] Support CSRs required for RISC-V PM extension except for ones in hypervisor mode

[Richard Henderson]

On 10/15/20 8:21 AM, Alexey Baturo wrote:
> +/* Functions to access Pointer Masking feature registers 
> + * We have to check if current priv lvl could modify
> + * csr in given mode
> + */
> +static int check_pm_current_disabled(CPURISCVState *env, int csrno)
> +{
> +    /* m-mode is always allowed to modify registers, so allow */
> +    if (env->priv == PRV_M) {
> +        return 0;
> +    }
> +    int csr_priv = get_field(csrno, 0xC00);
> +    /* If priv lvls differ that means we're accessing csr from higher priv lvl, so allow */
> +    if (env->priv != csr_priv) {
> +        return 0;
> +    }
> +    int cur_bit_pos = (env->priv == PRV_U) ? U_PM_CURRENT :
> +                      (env->priv == PRV_S) ? S_PM_CURRENT : -1;
> +    assert(cur_bit_pos != -1);

This is a bit clumsy.  I suggest

    int cur_bit_pos;
    switch (env->priv) {
    case PRV_M:
        return 0;
    case PRV_S:
        cur_bit_pos = S_PM_CURRENT;
        break;
    case PRV_U:
        cur_bit_pos = U_PM_CURRENT;
        break;
    default:
        g_assert_not_reached();
    }

> +static int read_mmte(CPURISCVState *env, int csrno, target_ulong *val)
> +{
> +#if !defined(CONFIG_USER_ONLY)
> +    if (!riscv_has_ext(env, RVJ)) {
> +        *val = 0;
> +        return 0;
> +    }
> +#endif

This ifdef is wrong.  CONFIG_USER_ONLY doesn't have anything to do with what
features the cpu supports.  Nor can it be correct.  If you try to read this on
current hardware, without J, then you get an exception not zero.

I would have expected this check would actually go into the csr predicate function.

> +    *val = env->mmte & MMTE_MASK;

Surely you don't need MMTE_MASK here because you used it when writing.

That said, don't you also need to mask vs the current priv level?  There's
language in your doc that says "XS bits are available in..." and "PM bits are
available in...".

I'll also note that it says "by default only higher priv code can set the value
for PM bits", while surely it's a security bug for lower priv code to read
anything related to a higher priv.


> +    target_ulong wpri_val = val & SMTE_MASK;
> +    assert(val == wpri_val);

Incorrect assert.  This value is under guest control.  Do not crash qemu
because the guest is doing silly things.  Raise an exception if you like, and
perhaps qemu_log_mask with LOG_GUEST_ERROR.

> +    if (check_pm_current_disabled(env, csrno))
> +        return 0;

Missing braces.


r~

Re: [PATCH v2 4/5] [RISCV_PM] Support pointer masking for RISC-V for i/c/f/d/a types of instructions

[Richard Henderson]

On 10/15/20 8:21 AM, Alexey Baturo wrote:
> Signed-off-by: Alexey Baturo <space.monkey.delivers@gmail.com>
> ---
>  target/riscv/insn_trans/trans_rva.c.inc |  3 +++
>  target/riscv/insn_trans/trans_rvd.c.inc |  2 ++
>  target/riscv/insn_trans/trans_rvf.c.inc |  2 ++
>  target/riscv/insn_trans/trans_rvi.c.inc |  2 ++
>  target/riscv/translate.c                | 14 ++++++++++++++
>  5 files changed, 23 insertions(+)

Looks ok.

It does occur to me to wonder how this is intended to work with unaligned
addresses, or large memory operations such as with RVV.

Without changes in the generic tcg code, an unaligned memory op that crosses
the mask will not wrap the second half.  E.g.

  upmbase = 0
  upmmask = 0xffff
  address = 0xfffe
  size    = 8

will read [0x10005 : 0xfffe] and not
[0x0005 : 0x0000] | [0xffff : 0xfffe] as a true wrapping would lead you do believe.


r~

Re: [PATCH v2 5/5] [RISCV_PM] Implement address masking functions required for RISC-V Pointer Masking extension

[Richard Henderson]

On 10/15/20 8:21 AM, Alexey Baturo wrote:
> +        switch (priv) {
> +        case PRV_U:
> +            pm_enabled = env->mmte & U_PM_ENABLE;
> +            break;
> +        case PRV_S:
> +            pm_enabled = env->mmte & S_PM_ENABLE;
> +            break;
> +        case PRV_M:
> +            pm_enabled = env->mmte & M_PM_ENABLE;
> +            break;
> +        default:
> +            assert(0 && "Unreachable");

g_assert_not_reached();

> +    /* PointerMasking extension */
> +    uint8_t pm_enabled;

bool

> +    if (s->pm_enabled == 0) {

!s->pm_enabled

> +    if (riscv_has_ext(env, RVJ)) {
> +        ctx->pm_enabled = FIELD_EX32(tb_flags, TB_FLAGS, PM_ENABLED);
> +        int priv = cpu_mmu_index(env, false);
> +        ctx->pm_mask = pm_mask[priv];
> +        ctx->pm_base = pm_base[priv];
> +    } else {
> +        ctx->pm_enabled = 0;
> +    }

Don't need the if.  And should it in fact be placed outside the ifdef?  This
shouldn't be related to !CONFIG_USER_ONLY here and nowhere else.


r~

Re: [PATCH v2 2/5] [RISCV_PM] Support CSRs required for RISC-V PM extension except for ones in hypervisor mode

[Alexey Baturo]

[-- Attachment #1: Type: text/plain, Size: 4517 bytes --]

Richard, again thanks for the review!

> This is a bit clumsy.  I suggest
Sure, will fix.

> If you try to read this on current hardware, without J, then you get an
exception not zero.
If I get the spec right, the idea is to read 0 from PM CSRs even if this
extension is not present.

>I would have expected this check would actually go into the csr predicate
function.
If I understand your proposal correctly, in this case I'd have to introduce
3 new functions for S/M/U PM CSRs as a predicate. I'm okay with that if you
suggest so

>Surely you don't need MMTE_MASK here because you used it when writing.
>That said, don't you also need to mask vs the current priv level?
I suppose that applying these masks helps to prepare the correct value for
the given priv level. So if I understand correctly if you're in *M-mode*
and try to read *UMTE*, you'll get only *U.Enable* and *U.Current*, while
if you'd try to read *SMTE*, you'll also get *XS* bits, *S.Enable* and
*S.Current*.

> it's a security bug for lower priv code to read anything related to a
higher priv.
Could you please elaborate a bit more here? I don't see how this scenario
is valid: in *U-mode *you're supposed to be able to read only *U* *registers,
while *M*/*S *mode could allow *U-mode *to write to its *U** registers.
As for *S-mode*, I believe it's allowed to write to any *U** registers and
it's available to write to *S** registers if *S.Current* is set.

> Do not crash qemu because the guest is doing silly things
Sure, you're right. Will fix.

>Raise an exception if you like, and perhaps qemu_log_mask with
LOG_GUEST_ERROR
I think raising exception here might be too much, but logging the error is
a great idea, thanks!

Thanks!

чт, 15 окт. 2020 г. в 19:48, Richard Henderson <richard.henderson@linaro.org
>:

> On 10/15/20 8:21 AM, Alexey Baturo wrote:
> > +/* Functions to access Pointer Masking feature registers
> > + * We have to check if current priv lvl could modify
> > + * csr in given mode
> > + */
> > +static int check_pm_current_disabled(CPURISCVState *env, int csrno)
> > +{
> > +    /* m-mode is always allowed to modify registers, so allow */
> > +    if (env->priv == PRV_M) {
> > +        return 0;
> > +    }
> > +    int csr_priv = get_field(csrno, 0xC00);
> > +    /* If priv lvls differ that means we're accessing csr from higher
> priv lvl, so allow */
> > +    if (env->priv != csr_priv) {
> > +        return 0;
> > +    }
> > +    int cur_bit_pos = (env->priv == PRV_U) ? U_PM_CURRENT :
> > +                      (env->priv == PRV_S) ? S_PM_CURRENT : -1;
> > +    assert(cur_bit_pos != -1);
>
> This is a bit clumsy.  I suggest
>
>     int cur_bit_pos;
>     switch (env->priv) {
>     case PRV_M:
>         return 0;
>     case PRV_S:
>         cur_bit_pos = S_PM_CURRENT;
>         break;
>     case PRV_U:
>         cur_bit_pos = U_PM_CURRENT;
>         break;
>     default:
>         g_assert_not_reached();
>     }
>
> > +static int read_mmte(CPURISCVState *env, int csrno, target_ulong *val)
> > +{
> > +#if !defined(CONFIG_USER_ONLY)
> > +    if (!riscv_has_ext(env, RVJ)) {
> > +        *val = 0;
> > +        return 0;
> > +    }
> > +#endif
>
> This ifdef is wrong.  CONFIG_USER_ONLY doesn't have anything to do with
> what
> features the cpu supports.  Nor can it be correct.  If you try to read
> this on
> current hardware, without J, then you get an exception not zero.
>
> I would have expected this check would actually go into the csr predicate
> function.
>
> > +    *val = env->mmte & MMTE_MASK;
>
> Surely you don't need MMTE_MASK here because you used it when writing.
>
> That said, don't you also need to mask vs the current priv level?  There's
> language in your doc that says "XS bits are available in..." and "PM bits
> are
> available in...".
>
> I'll also note that it says "by default only higher priv code can set the
> value
> for PM bits", while surely it's a security bug for lower priv code to read
> anything related to a higher priv.
>
>
> > +    target_ulong wpri_val = val & SMTE_MASK;
> > +    assert(val == wpri_val);
>
> Incorrect assert.  This value is under guest control.  Do not crash qemu
> because the guest is doing silly things.  Raise an exception if you like,
> and
> perhaps qemu_log_mask with LOG_GUEST_ERROR.
>
> > +    if (check_pm_current_disabled(env, csrno))
> > +        return 0;
>
> Missing braces.
>
>
> r~
>

[-- Attachment #2: Type: text/html, Size: 5650 bytes --]

Re: [PATCH v2 4/5] [RISCV_PM] Support pointer masking for RISC-V for i/c/f/d/a types of instructions

[Alexey Baturo]

[-- Attachment #1: Type: text/plain, Size: 1266 bytes --]

That's a great question, but unfortunately, I don't have an answer for it
now.
Let me ask it on J WG meeting that would happen next Monday along with
extension naming and CSR numbers(hopefuly).

Thanks!

чт, 15 окт. 2020 г. в 20:00, Richard Henderson <richard.henderson@linaro.org
>:

> On 10/15/20 8:21 AM, Alexey Baturo wrote:
> > Signed-off-by: Alexey Baturo <space.monkey.delivers@gmail.com>
> > ---
> >  target/riscv/insn_trans/trans_rva.c.inc |  3 +++
> >  target/riscv/insn_trans/trans_rvd.c.inc |  2 ++
> >  target/riscv/insn_trans/trans_rvf.c.inc |  2 ++
> >  target/riscv/insn_trans/trans_rvi.c.inc |  2 ++
> >  target/riscv/translate.c                | 14 ++++++++++++++
> >  5 files changed, 23 insertions(+)
>
> Looks ok.
>
> It does occur to me to wonder how this is intended to work with unaligned
> addresses, or large memory operations such as with RVV.
>
> Without changes in the generic tcg code, an unaligned memory op that
> crosses
> the mask will not wrap the second half.  E.g.
>
>   upmbase = 0
>   upmmask = 0xffff
>   address = 0xfffe
>   size    = 8
>
> will read [0x10005 : 0xfffe] and not
> [0x0005 : 0x0000] | [0xffff : 0xfffe] as a true wrapping would lead you do
> believe.
>
>
> r~
>

[-- Attachment #2: Type: text/html, Size: 1764 bytes --]

Re: [PATCH v2 5/5] [RISCV_PM] Implement address masking functions required for RISC-V Pointer Masking extension

[Alexey Baturo]

[-- Attachment #1: Type: text/plain, Size: 1518 bytes --]

> g_assert_not_reached();
Would fix, thanks.

> bool
Would fix.

>!s->pm_enabled
Same.

> Don't need the if.
Would remove, thanks.

> And should it in fact be placed outside the ifdef?
Sure, you're right.

Richard, thank you for your time and effort reviewing these changes!

чт, 15 окт. 2020 г. в 20:07, Richard Henderson <richard.henderson@linaro.org
>:

> On 10/15/20 8:21 AM, Alexey Baturo wrote:
> > +        switch (priv) {
> > +        case PRV_U:
> > +            pm_enabled = env->mmte & U_PM_ENABLE;
> > +            break;
> > +        case PRV_S:
> > +            pm_enabled = env->mmte & S_PM_ENABLE;
> > +            break;
> > +        case PRV_M:
> > +            pm_enabled = env->mmte & M_PM_ENABLE;
> > +            break;
> > +        default:
> > +            assert(0 && "Unreachable");
>
> g_assert_not_reached();
>
> > +    /* PointerMasking extension */
> > +    uint8_t pm_enabled;
>
> bool
>
> > +    if (s->pm_enabled == 0) {
>
> !s->pm_enabled
>
> > +    if (riscv_has_ext(env, RVJ)) {
> > +        ctx->pm_enabled = FIELD_EX32(tb_flags, TB_FLAGS, PM_ENABLED);
> > +        int priv = cpu_mmu_index(env, false);
> > +        ctx->pm_mask = pm_mask[priv];
> > +        ctx->pm_base = pm_base[priv];
> > +    } else {
> > +        ctx->pm_enabled = 0;
> > +    }
>
> Don't need the if.  And should it in fact be placed outside the ifdef?
> This
> shouldn't be related to !CONFIG_USER_ONLY here and nowhere else.
>
>
> r~
>

[-- Attachment #2: Type: text/html, Size: 2297 bytes --]

Re: [PATCH v2 2/5] [RISCV_PM] Support CSRs required for RISC-V PM extension except for ones in hypervisor mode

[Alexey Baturo]

[-- Attachment #1: Type: text/plain, Size: 5455 bytes --]

> Surely you don't need MMTE_MASK here because you used it when writing.
> That said, don't you also need to mask vs the current priv level?
> while surely it's a security bug for lower priv code to read anything
related to a higher priv
Now I think I get what you've meant:
The spec states that for lower priv level you get WPRI fields that contain
bits for higher priv levels.
Applying those masks while reading CSRs on the one hand solves this
security breach, but on the other - violates the spec. Let me raise this
question at the upcoming J WG meeting.
Meanwhile, do you think applying **MTE *masks while reading CSR values is a
good solution for now?

Thanks again!

чт, 15 окт. 2020 г. в 20:28, Alexey Baturo <baturo.alexey@gmail.com>:

> Richard, again thanks for the review!
>
> > This is a bit clumsy.  I suggest
> Sure, will fix.
>
> > If you try to read this on current hardware, without J, then you get an
> exception not zero.
> If I get the spec right, the idea is to read 0 from PM CSRs even if this
> extension is not present.
>
> >I would have expected this check would actually go into the csr predicate
> function.
> If I understand your proposal correctly, in this case I'd have to
> introduce 3 new functions for S/M/U PM CSRs as a predicate. I'm okay with
> that if you suggest so
>
> >Surely you don't need MMTE_MASK here because you used it when writing.
> >That said, don't you also need to mask vs the current priv level?
> I suppose that applying these masks helps to prepare the correct value for
> the given priv level. So if I understand correctly if you're in *M-mode*
> and try to read *UMTE*, you'll get only *U.Enable* and *U.Current*, while
> if you'd try to read *SMTE*, you'll also get *XS* bits, *S.Enable* and
> *S.Current*.
>
> > it's a security bug for lower priv code to read anything related to a
> higher priv.
> Could you please elaborate a bit more here? I don't see how this scenario
> is valid: in *U-mode *you're supposed to be able to read only *U* *registers,
> while *M*/*S *mode could allow *U-mode *to write to its *U** registers.
> As for *S-mode*, I believe it's allowed to write to any *U** registers
> and it's available to write to *S** registers if *S.Current* is set.
>
> > Do not crash qemu because the guest is doing silly things
> Sure, you're right. Will fix.
>
> >Raise an exception if you like, and perhaps qemu_log_mask with
> LOG_GUEST_ERROR
> I think raising exception here might be too much, but logging the error is
> a great idea, thanks!
>
> Thanks!
>
> чт, 15 окт. 2020 г. в 19:48, Richard Henderson <
> richard.henderson@linaro.org>:
>
>> On 10/15/20 8:21 AM, Alexey Baturo wrote:
>> > +/* Functions to access Pointer Masking feature registers
>> > + * We have to check if current priv lvl could modify
>> > + * csr in given mode
>> > + */
>> > +static int check_pm_current_disabled(CPURISCVState *env, int csrno)
>> > +{
>> > +    /* m-mode is always allowed to modify registers, so allow */
>> > +    if (env->priv == PRV_M) {
>> > +        return 0;
>> > +    }
>> > +    int csr_priv = get_field(csrno, 0xC00);
>> > +    /* If priv lvls differ that means we're accessing csr from higher
>> priv lvl, so allow */
>> > +    if (env->priv != csr_priv) {
>> > +        return 0;
>> > +    }
>> > +    int cur_bit_pos = (env->priv == PRV_U) ? U_PM_CURRENT :
>> > +                      (env->priv == PRV_S) ? S_PM_CURRENT : -1;
>> > +    assert(cur_bit_pos != -1);
>>
>> This is a bit clumsy.  I suggest
>>
>>     int cur_bit_pos;
>>     switch (env->priv) {
>>     case PRV_M:
>>         return 0;
>>     case PRV_S:
>>         cur_bit_pos = S_PM_CURRENT;
>>         break;
>>     case PRV_U:
>>         cur_bit_pos = U_PM_CURRENT;
>>         break;
>>     default:
>>         g_assert_not_reached();
>>     }
>>
>> > +static int read_mmte(CPURISCVState *env, int csrno, target_ulong *val)
>> > +{
>> > +#if !defined(CONFIG_USER_ONLY)
>> > +    if (!riscv_has_ext(env, RVJ)) {
>> > +        *val = 0;
>> > +        return 0;
>> > +    }
>> > +#endif
>>
>> This ifdef is wrong.  CONFIG_USER_ONLY doesn't have anything to do with
>> what
>> features the cpu supports.  Nor can it be correct.  If you try to read
>> this on
>> current hardware, without J, then you get an exception not zero.
>>
>> I would have expected this check would actually go into the csr predicate
>> function.
>>
>> > +    *val = env->mmte & MMTE_MASK;
>>
>> Surely you don't need MMTE_MASK here because you used it when writing.
>>
>> That said, don't you also need to mask vs the current priv level?  There's
>> language in your doc that says "XS bits are available in..." and "PM bits
>> are
>> available in...".
>>
>> I'll also note that it says "by default only higher priv code can set the
>> value
>> for PM bits", while surely it's a security bug for lower priv code to read
>> anything related to a higher priv.
>>
>>
>> > +    target_ulong wpri_val = val & SMTE_MASK;
>> > +    assert(val == wpri_val);
>>
>> Incorrect assert.  This value is under guest control.  Do not crash qemu
>> because the guest is doing silly things.  Raise an exception if you like,
>> and
>> perhaps qemu_log_mask with LOG_GUEST_ERROR.
>>
>> > +    if (check_pm_current_disabled(env, csrno))
>> > +        return 0;
>>
>> Missing braces.
>>
>>
>> r~
>>
>

[-- Attachment #2: Type: text/html, Size: 6818 bytes --]

Re: [PATCH v2 2/5] [RISCV_PM] Support CSRs required for RISC-V PM extension except for ones in hypervisor mode

[Richard Henderson]

On 10/15/20 11:05 AM, Alexey Baturo wrote:
> Meanwhile, do you think applying **MTE *masks while reading CSR values is a
> good solution for now?

Yes.


r~