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

[PATCH 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 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      | 302 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 380 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..bbfbe71656 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,288 @@ 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);
+    /* flush translation cache */
+    if (val != env->mmte) {
+        tb_flush(env_cpu(env));
+    }
+    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
+    if (val != env->mpmmask) {
+        tb_flush(env_cpu(env));
+    }
+    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;
+    if (val != env->spmmask) {
+        tb_flush(env_cpu(env));
+    }
+    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;
+    if (val != env->upmmask) {
+        tb_flush(env_cpu(env));
+    }
+    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
+    /* flush translation cache */
+    if (val != env->mpmbase) {
+        tb_flush(env_cpu(env));
+    }
+    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;
+    /* flush translation cache */
+    if (val != env->spmbase) {
+        tb_flush(env_cpu(env));
+    }
+    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;
+    /* flush translation cache */
+    if (val != env->upmbase) {
+        tb_flush(env_cpu(env));
+    }
+    env->upmbase = val;
+    env->mstatus |= MSTATUS_XS | MSTATUS_SD;
+    env->mmte |= PM_EXT_DIRTY;
+    return 0;
+}
 #endif
 
 /*
@@ -1471,6 +1758,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 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 | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index d63031eb08..8f8ee4d29c 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -255,6 +255,15 @@ 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, RVH)) {
+        qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mmte    ", env->mmte);
+        qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "upmbase ", env->upmbase);
+        qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "spmbase ", env->spmbase);
+        qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mpmbase ", env->mpmbase);
+        qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "upmmask ", env->upmmask);
+        qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "spmmask ", env->spmmask);
+        qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mpmmask ", env->mpmmask);
+    }
 #endif
 
     for (i = 0; i < 32; i++) {
-- 
2.20.1

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

[Richard Henderson]

On 10/14/20 10:01 AM, Alexey Baturo wrote:
> +    if (riscv_has_ext(env, RVH)) {

RVJ.

> +        qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mmte    ", env->mmte);
> +        qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "upmbase ", env->upmbase);
> +        qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "spmbase ", env->spmbase);
> +        qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mpmbase ", env->mpmbase);
> +        qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "upmmask ", env->upmmask);
> +        qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "spmmask ", env->spmmask);
> +        qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mpmmask ", env->mpmmask);

Probably you only want to dump the set that's current.


r~

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

[Alexey Baturo]

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

First of all thank you so much for reviewing these patches!

>RVJ.
Thanks, I missed this typo, will fix it.

>Probably you only want to dump the set that's current.
I don't know for sure how anyone would be using this while debugging PM
related code, but I like the idea, so I'll try to do it, thanks!

ср, 14 окт. 2020 г. в 21:41, Richard Henderson <richard.henderson@linaro.org
>:

> On 10/14/20 10:01 AM, Alexey Baturo wrote:
> > +    if (riscv_has_ext(env, RVH)) {
>
> RVJ.
>
> > +        qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mmte    ",
> env->mmte);
> > +        qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "upmbase ",
> env->upmbase);
> > +        qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "spmbase ",
> env->spmbase);
> > +        qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mpmbase ",
> env->mpmbase);
> > +        qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "upmmask ",
> env->upmmask);
> > +        qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "spmmask ",
> env->spmmask);
> > +        qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mpmmask ",
> env->mpmmask);
>
> Probably you only want to dump the set that's current.
>
>
> r~
>

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

[PATCH 4/5] [RISCV_PM] Add 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/translate.c | 65 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 79dca2291b..338a967e0c 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -63,6 +63,10 @@ typedef struct DisasContext {
     uint16_t vlen;
     uint16_t mlen;
     bool vl_eq_vlmax;
+    /* PointerMasking extension */
+    uint8_t pm_enabled;
+    target_ulong pm_mask;
+    target_ulong pm_base;
 } DisasContext;
 
 #ifdef TARGET_RISCV64
@@ -90,6 +94,38 @@ static inline bool has_ext(DisasContext *ctx, uint32_t ext)
     return ctx->misa & ext;
 }
 
+/* Generates address adjustment for PointerMasking */
+static void gen_pm_adjust_address(DisasContext *s,
+                                  TCGv_i64      dst,
+                                  TCGv_i64      src)
+{
+    if (s->pm_enabled == 0) {
+        /* Load unmodified address */
+        tcg_gen_mov_i64(dst, src);
+    } else {
+        TCGv_i64 mask_neg = tcg_const_i64(~s->pm_mask);
+        TCGv_i64 base     = tcg_const_i64(s->pm_base);
+        /* calculate (addr & ~mask) */
+        TCGv res1 = tcg_temp_new();
+        tcg_gen_and_tl(res1, mask_neg, src);
+        /* calculate (1) | (base) */
+        TCGv res2 = tcg_temp_new();
+        tcg_gen_or_tl(res2, res1, base);
+        /* move result to dst */
+        tcg_gen_mov_i64(dst, res2);
+        /* free allocated temps */
+        tcg_temp_free(res1);
+        tcg_temp_free(res2);
+        tcg_temp_free_i64(mask_neg);
+        tcg_temp_free_i64(base);
+    }
+}
+
+static TCGv_i64 apply_pointer_masking(DisasContext *s, TCGv_i64 addr)
+{
+    gen_pm_adjust_address(s, addr, addr);
+    return addr;
+}
 /*
  * RISC-V requires NaN-boxing of narrower width floating point values.
  * This applies when a 32-bit value is assigned to a 64-bit FP register.
@@ -800,8 +836,36 @@ static void riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
     } else {
         ctx->virt_enabled = false;
     }
+    if (riscv_has_ext(env, RVJ)) {
+        switch (env->priv) {
+        case PRV_U:
+            ctx->pm_enabled = get_field(env->mmte, UMTE_U_PM_ENABLE);
+            ctx->pm_mask = env->upmmask;
+            ctx->pm_base = env->upmbase;
+            break;
+        case PRV_S:
+            ctx->pm_enabled = get_field(env->mmte, SMTE_S_PM_ENABLE);
+            ctx->pm_mask = env->spmmask;
+            ctx->pm_base = env->spmbase;
+            break;
+        case PRV_M:
+            ctx->pm_enabled = get_field(env->mmte, MMTE_M_PM_ENABLE);
+            ctx->pm_mask = env->mpmmask;
+            ctx->pm_base = env->mpmbase;
+            break;
+        default:
+            assert(0 && "Unreachable");
+        }
+    } else {
+        ctx->pm_enabled = 0;
+        ctx->pm_mask = 0;
+        ctx->pm_base = 0;
+    }
 #else
     ctx->virt_enabled = false;
+    ctx->pm_enabled = 0;
+    ctx->pm_mask = 0;
+    ctx->pm_base = 0;
 #endif
     ctx->misa = env->misa;
     ctx->frm = -1;  /* unknown rounding mode */
@@ -932,3 +996,4 @@ void riscv_translate_init(void)
     load_val = tcg_global_mem_new(cpu_env, offsetof(CPURISCVState, load_val),
                              "load_val");
 }
+
-- 
2.20.1

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

[Richard Henderson]

On 10/14/20 10:01 AM, Alexey Baturo wrote:
> +static TCGv_i64 apply_pointer_masking(DisasContext *s, TCGv_i64 addr)
> +{
> +    gen_pm_adjust_address(s, addr, addr);
> +    return addr;
> +}

This function is unused in this patch, which means the series as a whole is
non-bisectable.

Rather than merge the two, I suggest adding a stub version of this function to
patch 5, and then swap patch 4 and patch 5.  So you will add uses of
apply_pointer_masking without actually implementing it yet.  Which should be fine.

> @@ -800,8 +836,36 @@ static void riscv_tr_init_disas_context
>      } else {
>          ctx->virt_enabled = false;
>      }
> +    if (riscv_has_ext(env, RVJ)) {
> +        switch (env->priv) {
> +        case PRV_U:
> +            ctx->pm_enabled = get_field(env->mmte, UMTE_U_PM_ENABLE);
> +            ctx->pm_mask = env->upmmask;
> +            ctx->pm_base = env->upmbase;
> +            break;
> +        case PRV_S:
> +            ctx->pm_enabled = get_field(env->mmte, SMTE_S_PM_ENABLE);
> +            ctx->pm_mask = env->spmmask;
> +            ctx->pm_base = env->spmbase;
> +            break;
> +        case PRV_M:
> +            ctx->pm_enabled = get_field(env->mmte, MMTE_M_PM_ENABLE);
> +            ctx->pm_mask = env->mpmmask;

You can't read env like this.

This bakes in values from ENV without adding any way to verify that those
values are still current.

The one thing that you must bake into the generated code is the state of
PM_ENABLE.  Anything else would penalize normal risc-v emulation.

You do that in cpu_get_tb_cpu_state().  Allocate one bit to hold
the current state of the flag.  E.g.

FIELD(TB_FLAGS, PM_ENABLED, 9, 1)

then fill it in from the correct mmte bit for priv (which itself is encoded by
cpu_mmu_index()).

Except for special cases, the mask and base variables cannot be placed into
TB_FLAGS.  For now, I think it's best to ignore the special cases and implement
them all as tcg globals.  Which means that we do *not* bake in a particular
value, but instead read the value from env at runtime.

So, in riscv_translate_init, you create new globals for each of the mask and
base.  In riscv_tr_init_disas_context you examine priv (via mmu_index) and
assign one pair of the globals to DisasContext, so that you don't have to keep
looking them up.

Then you have

static void gen_pm_adjust_address(DisasContext *s,
                                  TCGv_i64 dst,
                                  TCGv_i64 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);
    }
}


r~

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

[Alexey Baturo]

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

> I suggest adding a stub version of this function to patch 5, and then
swap patch 4 and patch 5.
Thanks, will do.

>This bakes in values from ENV without adding any way to verify that those
values are still current.
If I correctly get your idea, you're talking about the situation, when
DisasContext was initialized with some values, which at some point got
changed, so this could lead to incorrect address masking. I tried to handle
this situation by dropping the translation cache in case different values
are written in any of the PM CSRs, which I assumed would lead to refilling
the DIsasContext structure.
This is obviously not the best way to do it, since it may lead to
performance degradation in some cases, so let me process your suggestion
and try to implement it.

Thanks!

ср, 14 окт. 2020 г. в 22:19, Richard Henderson <richard.henderson@linaro.org
>:

> On 10/14/20 10:01 AM, Alexey Baturo wrote:
> > +static TCGv_i64 apply_pointer_masking(DisasContext *s, TCGv_i64 addr)
> > +{
> > +    gen_pm_adjust_address(s, addr, addr);
> > +    return addr;
> > +}
>
> This function is unused in this patch, which means the series as a whole is
> non-bisectable.
>
> Rather than merge the two, I suggest adding a stub version of this
> function to
> patch 5, and then swap patch 4 and patch 5.  So you will add uses of
> apply_pointer_masking without actually implementing it yet.  Which should
> be fine.
>
> > @@ -800,8 +836,36 @@ static void riscv_tr_init_disas_context
> >      } else {
> >          ctx->virt_enabled = false;
> >      }
> > +    if (riscv_has_ext(env, RVJ)) {
> > +        switch (env->priv) {
> > +        case PRV_U:
> > +            ctx->pm_enabled = get_field(env->mmte, UMTE_U_PM_ENABLE);
> > +            ctx->pm_mask = env->upmmask;
> > +            ctx->pm_base = env->upmbase;
> > +            break;
> > +        case PRV_S:
> > +            ctx->pm_enabled = get_field(env->mmte, SMTE_S_PM_ENABLE);
> > +            ctx->pm_mask = env->spmmask;
> > +            ctx->pm_base = env->spmbase;
> > +            break;
> > +        case PRV_M:
> > +            ctx->pm_enabled = get_field(env->mmte, MMTE_M_PM_ENABLE);
> > +            ctx->pm_mask = env->mpmmask;
>
> You can't read env like this.
>
> This bakes in values from ENV without adding any way to verify that those
> values are still current.
>
> The one thing that you must bake into the generated code is the state of
> PM_ENABLE.  Anything else would penalize normal risc-v emulation.
>
> You do that in cpu_get_tb_cpu_state().  Allocate one bit to hold
> the current state of the flag.  E.g.
>
> FIELD(TB_FLAGS, PM_ENABLED, 9, 1)
>
> then fill it in from the correct mmte bit for priv (which itself is
> encoded by
> cpu_mmu_index()).
>
> Except for special cases, the mask and base variables cannot be placed into
> TB_FLAGS.  For now, I think it's best to ignore the special cases and
> implement
> them all as tcg globals.  Which means that we do *not* bake in a particular
> value, but instead read the value from env at runtime.
>
> So, in riscv_translate_init, you create new globals for each of the mask
> and
> base.  In riscv_tr_init_disas_context you examine priv (via mmu_index) and
> assign one pair of the globals to DisasContext, so that you don't have to
> keep
> looking them up.
>
> Then you have
>
> static void gen_pm_adjust_address(DisasContext *s,
>                                   TCGv_i64 dst,
>                                   TCGv_i64 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);
>     }
> }
>
>
> r~
>

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

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

[Alexey Baturo]

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

Hi folks,

I've sent new v2 patches where, I hope, I managed to address all the
comments and suggestions that were provided.
Thanks!

ср, 14 окт. 2020 г. в 23:10, Alexey Baturo <baturo.alexey@gmail.com>:

> > I suggest adding a stub version of this function to patch 5, and then
> swap patch 4 and patch 5.
> Thanks, will do.
>
> >This bakes in values from ENV without adding any way to verify that those
> values are still current.
> If I correctly get your idea, you're talking about the situation, when
> DisasContext was initialized with some values, which at some point got
> changed, so this could lead to incorrect address masking. I tried to handle
> this situation by dropping the translation cache in case different values
> are written in any of the PM CSRs, which I assumed would lead to refilling
> the DIsasContext structure.
> This is obviously not the best way to do it, since it may lead to
> performance degradation in some cases, so let me process your suggestion
> and try to implement it.
>
> Thanks!
>
> ср, 14 окт. 2020 г. в 22:19, Richard Henderson <
> richard.henderson@linaro.org>:
>
>> On 10/14/20 10:01 AM, Alexey Baturo wrote:
>> > +static TCGv_i64 apply_pointer_masking(DisasContext *s, TCGv_i64 addr)
>> > +{
>> > +    gen_pm_adjust_address(s, addr, addr);
>> > +    return addr;
>> > +}
>>
>> This function is unused in this patch, which means the series as a whole
>> is
>> non-bisectable.
>>
>> Rather than merge the two, I suggest adding a stub version of this
>> function to
>> patch 5, and then swap patch 4 and patch 5.  So you will add uses of
>> apply_pointer_masking without actually implementing it yet.  Which should
>> be fine.
>>
>> > @@ -800,8 +836,36 @@ static void riscv_tr_init_disas_context
>> >      } else {
>> >          ctx->virt_enabled = false;
>> >      }
>> > +    if (riscv_has_ext(env, RVJ)) {
>> > +        switch (env->priv) {
>> > +        case PRV_U:
>> > +            ctx->pm_enabled = get_field(env->mmte, UMTE_U_PM_ENABLE);
>> > +            ctx->pm_mask = env->upmmask;
>> > +            ctx->pm_base = env->upmbase;
>> > +            break;
>> > +        case PRV_S:
>> > +            ctx->pm_enabled = get_field(env->mmte, SMTE_S_PM_ENABLE);
>> > +            ctx->pm_mask = env->spmmask;
>> > +            ctx->pm_base = env->spmbase;
>> > +            break;
>> > +        case PRV_M:
>> > +            ctx->pm_enabled = get_field(env->mmte, MMTE_M_PM_ENABLE);
>> > +            ctx->pm_mask = env->mpmmask;
>>
>> You can't read env like this.
>>
>> This bakes in values from ENV without adding any way to verify that those
>> values are still current.
>>
>> The one thing that you must bake into the generated code is the state of
>> PM_ENABLE.  Anything else would penalize normal risc-v emulation.
>>
>> You do that in cpu_get_tb_cpu_state().  Allocate one bit to hold
>> the current state of the flag.  E.g.
>>
>> FIELD(TB_FLAGS, PM_ENABLED, 9, 1)
>>
>> then fill it in from the correct mmte bit for priv (which itself is
>> encoded by
>> cpu_mmu_index()).
>>
>> Except for special cases, the mask and base variables cannot be placed
>> into
>> TB_FLAGS.  For now, I think it's best to ignore the special cases and
>> implement
>> them all as tcg globals.  Which means that we do *not* bake in a
>> particular
>> value, but instead read the value from env at runtime.
>>
>> So, in riscv_translate_init, you create new globals for each of the mask
>> and
>> base.  In riscv_tr_init_disas_context you examine priv (via mmu_index) and
>> assign one pair of the globals to DisasContext, so that you don't have to
>> keep
>> looking them up.
>>
>> Then you have
>>
>> static void gen_pm_adjust_address(DisasContext *s,
>>                                   TCGv_i64 dst,
>>                                   TCGv_i64 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);
>>     }
>> }
>>
>>
>> r~
>>
>

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

[PATCH 5/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 |  9 +++++++++
 target/riscv/insn_trans/trans_rvd.c.inc |  6 ++++++
 target/riscv/insn_trans/trans_rvf.c.inc |  6 ++++++
 target/riscv/insn_trans/trans_rvi.c.inc |  6 ++++++
 target/riscv/translate.c                | 12 ++++++++++++
 5 files changed, 39 insertions(+)

diff --git a/target/riscv/insn_trans/trans_rva.c.inc b/target/riscv/insn_trans/trans_rva.c.inc
index be8a9f06dd..3bf2e82013 100644
--- a/target/riscv/insn_trans/trans_rva.c.inc
+++ b/target/riscv/insn_trans/trans_rva.c.inc
@@ -26,6 +26,9 @@ static inline bool gen_lr(DisasContext *ctx, arg_atomic *a, MemOp mop)
     if (a->rl) {
         tcg_gen_mb(TCG_MO_ALL | TCG_BAR_STRL);
     }
+    if (has_ext(ctx, RVJ)) {
+        src1 = apply_pointer_masking(ctx, 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 +49,9 @@ static inline bool gen_sc(DisasContext *ctx, arg_atomic *a, MemOp mop)
     TCGLabel *l2 = gen_new_label();
 
     gen_get_gpr(src1, a->rs1);
+    if (has_ext(ctx, RVJ)) {
+        src1 = apply_pointer_masking(ctx, src1);
+    }
     tcg_gen_brcond_tl(TCG_COND_NE, load_res, src1, l1);
 
     gen_get_gpr(src2, a->rs2);
@@ -91,6 +97,9 @@ static bool gen_amo(DisasContext *ctx, arg_atomic *a,
     gen_get_gpr(src1, a->rs1);
     gen_get_gpr(src2, a->rs2);
 
+    if (has_ext(ctx, RVJ)) {
+        src1 = apply_pointer_masking(ctx, 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..0391bb02be 100644
--- a/target/riscv/insn_trans/trans_rvd.c.inc
+++ b/target/riscv/insn_trans/trans_rvd.c.inc
@@ -25,6 +25,9 @@ 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);
+    if (has_ext(ctx, RVJ)) {
+        t0 = apply_pointer_masking(ctx, t0);
+    }
 
     tcg_gen_qemu_ld_i64(cpu_fpr[a->rd], t0, ctx->mem_idx, MO_TEQ);
 
@@ -40,6 +43,9 @@ 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);
+    if (has_ext(ctx, RVJ)) {
+        t0 = apply_pointer_masking(ctx, 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..176bc992e1 100644
--- a/target/riscv/insn_trans/trans_rvf.c.inc
+++ b/target/riscv/insn_trans/trans_rvf.c.inc
@@ -30,6 +30,9 @@ 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);
+    if (has_ext(ctx, RVJ)) {
+        t0 = apply_pointer_masking(ctx, 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 +50,9 @@ static bool trans_fsw(DisasContext *ctx, arg_fsw *a)
     gen_get_gpr(t0, a->rs1);
 
     tcg_gen_addi_tl(t0, t0, a->imm);
+    if (has_ext(ctx, RVJ)) {
+        t0 = apply_pointer_masking(ctx, 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..3ee2fea271 100644
--- a/target/riscv/insn_trans/trans_rvi.c.inc
+++ b/target/riscv/insn_trans/trans_rvi.c.inc
@@ -141,6 +141,9 @@ 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);
+    if (has_ext(ctx, RVJ)) {
+        t0 = apply_pointer_masking(ctx, t0);
+    }
 
     tcg_gen_qemu_ld_tl(t1, t0, ctx->mem_idx, memop);
     gen_set_gpr(a->rd, t1);
@@ -180,6 +183,9 @@ 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);
+    if (has_ext(ctx, RVJ)) {
+        t0 = apply_pointer_masking(ctx, 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 338a967e0c..0b086753d4 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -416,6 +416,9 @@ 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);
+    if (has_ext(ctx, RVJ)) {
+        t0 = apply_pointer_masking(ctx, t0);
+    }
     int memop = tcg_memop_lookup[(opc >> 12) & 0x7];
 
     if (memop < 0) {
@@ -436,6 +439,9 @@ 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);
+    if (has_ext(ctx, RVJ)) {
+        t0 = apply_pointer_masking(ctx, t0);
+    }
     gen_get_gpr(dat, rs2);
     int memop = tcg_memop_lookup[(opc >> 12) & 0x7];
 
@@ -495,6 +501,9 @@ 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);
+    if (riscv_has_ext(env, RVJ)) {
+        t0 = apply_pointer_masking(ctx, t0);
+    }
 
     switch (opc) {
     case OPC_RISC_FLW:
@@ -534,6 +543,9 @@ 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);
+    if (riscv_has_ext(env, RVJ)) {
+        t0 = apply_pointer_masking(ctx, t0);
+    }
 
     switch (opc) {
     case OPC_RISC_FSW:
-- 
2.20.1

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

[Richard Henderson]

On 10/14/20 10:01 AM, Alexey Baturo wrote:
> +    if (has_ext(ctx, RVJ)) {
> +        src1 = apply_pointer_masking(ctx, src1);
> +    }

The if is redundant, since that will have been done in cpu_get_tb_cpu_state
while assigning pm_enabled.

The test for pm_enabled is in gen_pm_adjust_address.

The final thing is that the API for apply_pointer_masking is misleading.  Here,
it appears as if you are allocating a new temporary and assigning it to src1.
Which is not the case.

I suggest you drop apply_pointer_masking and just use gen_pm_adjust_address.


r~

Re: [PATCH 5/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: 1184 bytes --]

> The if is redundant, since that will have been done in
cpu_get_tb_cpu_state while assigning pm_enabled.
I totally agree here, however I tried to do explicit checks, so this
functionality is available only if a special option is supplied.
But if you think that's too much and I could do just mov(dst, src) in case
PM is disabled, I'd gladly fix that.

> I suggest you drop apply_pointer_masking and just use
gen_pm_adjust_address.
Sure, will do.

Thanks!



ср, 14 окт. 2020 г. в 22:24, Richard Henderson <richard.henderson@linaro.org
>:

> On 10/14/20 10:01 AM, Alexey Baturo wrote:
> > +    if (has_ext(ctx, RVJ)) {
> > +        src1 = apply_pointer_masking(ctx, src1);
> > +    }
>
> The if is redundant, since that will have been done in cpu_get_tb_cpu_state
> while assigning pm_enabled.
>
> The test for pm_enabled is in gen_pm_adjust_address.
>
> The final thing is that the API for apply_pointer_masking is misleading.
> Here,
> it appears as if you are allocating a new temporary and assigning it to
> src1.
> Which is not the case.
>
> I suggest you drop apply_pointer_masking and just use
> gen_pm_adjust_address.
>
>
> r~
>

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