1. Patchew
  2. QEMU
  3. [RFC PATCH v5 0/4] RISC-V Smstateen support
  4. View patch

[RFC PATCH v5 2/4] target/riscv: smstateen check for h/senvcfg

Mayuresh Chitale posted 4 patches 2 years, 4 months ago
Maintainers: Palmer Dabbelt <palmer@dabbelt.com>, Alistair Francis <alistair.francis@wdc.com>, Bin Meng <bin.meng@windriver.com>
There is a newer version of this series
[RFC PATCH v5 2/4] target/riscv: smstateen check for h/senvcfg
Posted by Mayuresh Chitale 2 years, 4 months ago 
Accesses to henvcfg, henvcfgh and senvcfg are allowed
only if corresponding bit in mstateen0/hstateen0 is
enabled. Otherwise an illegal instruction trap is
generated.

Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
---
 target/riscv/csr.c | 84 ++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 78 insertions(+), 6 deletions(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 324fefce59..ae91ae1f7e 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -39,6 +39,37 @@ void riscv_set_csr_ops(int csrno, riscv_csr_operations *ops)
 }
 
 /* Predicates */
+static RISCVException smstateen_acc_ok(CPURISCVState *env, int mode, int bit)
+{
+    CPUState *cs = env_cpu(env);
+    RISCVCPU *cpu = RISCV_CPU(cs);
+    bool virt = riscv_cpu_virt_enabled(env);
+
+    if (!cpu->cfg.ext_smstateen) {
+        return RISCV_EXCP_NONE;
+    }
+
+#if !defined(CONFIG_USER_ONLY)
+    if (!(env->mstateen[0] & 1UL << bit)) {
+        return RISCV_EXCP_ILLEGAL_INST;
+    }
+
+    if (virt) {
+        if (!(env->hstateen[0] & 1UL << bit)) {
+            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
+        }
+    }
+
+    if (mode == PRV_U) {
+        if (!(env->sstateen[0] & 1UL << bit)) {
+            return RISCV_EXCP_ILLEGAL_INST;
+        }
+    }
+#endif
+
+    return RISCV_EXCP_NONE;
+}
+
 static RISCVException fs(CPURISCVState *env, int csrno)
 {
 #if !defined(CONFIG_USER_ONLY)
@@ -1557,6 +1588,13 @@ static RISCVException write_menvcfgh(CPURISCVState *env, int csrno,
 static RISCVException read_senvcfg(CPURISCVState *env, int csrno,
                                  target_ulong *val)
 {
+    RISCVException ret;
+
+    ret = smstateen_acc_ok(env, PRV_S, SMSTATEEN0_HSENVCFG);
+    if (ret != RISCV_EXCP_NONE) {
+        return ret;
+    }
+
     *val = env->senvcfg;
     return RISCV_EXCP_NONE;
 }
@@ -1565,15 +1603,27 @@ static RISCVException write_senvcfg(CPURISCVState *env, int csrno,
                                   target_ulong val)
 {
     uint64_t mask = SENVCFG_FIOM | SENVCFG_CBIE | SENVCFG_CBCFE | SENVCFG_CBZE;
+    RISCVException ret;
 
-    env->senvcfg = (env->senvcfg & ~mask) | (val & mask);
+    ret = smstateen_acc_ok(env, PRV_S, SMSTATEEN0_HSENVCFG);
+    if (ret != RISCV_EXCP_NONE) {
+        return ret;
+    }
 
+    env->senvcfg = (env->senvcfg & ~mask) | (val & mask);
     return RISCV_EXCP_NONE;
 }
 
 static RISCVException read_henvcfg(CPURISCVState *env, int csrno,
                                  target_ulong *val)
 {
+    RISCVException ret;
+
+    ret = smstateen_acc_ok(env, PRV_S, SMSTATEEN0_HSENVCFG);
+    if (ret != RISCV_EXCP_NONE) {
+        return ret;
+    }
+
     *val = env->henvcfg;
     return RISCV_EXCP_NONE;
 }
@@ -1582,6 +1632,12 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
                                   target_ulong val)
 {
     uint64_t mask = HENVCFG_FIOM | HENVCFG_CBIE | HENVCFG_CBCFE | HENVCFG_CBZE;
+    RISCVException ret;
+
+    ret = smstateen_acc_ok(env, PRV_S, SMSTATEEN0_HSENVCFG);
+    if (ret != RISCV_EXCP_NONE) {
+        return ret;
+    }
 
     if (riscv_cpu_mxl(env) == MXL_RV64) {
         mask |= HENVCFG_PBMTE | HENVCFG_STCE;
@@ -1595,6 +1651,13 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
 static RISCVException read_henvcfgh(CPURISCVState *env, int csrno,
                                  target_ulong *val)
 {
+    RISCVException ret;
+
+    ret = smstateen_acc_ok(env, PRV_S, SMSTATEEN0_HSENVCFG);
+    if (ret != RISCV_EXCP_NONE) {
+        return ret;
+    }
+
     *val = env->henvcfg >> 32;
     return RISCV_EXCP_NONE;
 }
@@ -1604,9 +1667,14 @@ static RISCVException write_henvcfgh(CPURISCVState *env, int csrno,
 {
     uint64_t mask = HENVCFG_PBMTE | HENVCFG_STCE;
     uint64_t valh = (uint64_t)val << 32;
+    RISCVException ret;
 
-    env->henvcfg = (env->henvcfg & ~mask) | (valh & mask);
+    ret = smstateen_acc_ok(env, PRV_S, SMSTATEEN0_HSENVCFG);
+    if (ret != RISCV_EXCP_NONE) {
+        return ret;
+    }
 
+    env->henvcfg = (env->henvcfg & ~mask) | (valh & mask);
     return RISCV_EXCP_NONE;
 }
 
@@ -1628,7 +1696,8 @@ static RISCVException write_mstateen(CPURISCVState *env, int csrno,
                                      target_ulong new_val)
 {
     uint64_t *reg;
-    uint64_t wr_mask = 1UL << SMSTATEEN_STATEN;
+    uint64_t wr_mask = (1UL << SMSTATEEN_STATEN) |
+                       (1UL << SMSTATEEN0_HSENVCFG);
 
     reg = &env->mstateen[csrno - CSR_MSTATEEN0];
     write_smstateen(env, reg, wr_mask, new_val);
@@ -1649,7 +1718,8 @@ static RISCVException write_mstateenh(CPURISCVState *env, int csrno,
 {
     uint64_t *reg;
     uint64_t val;
-    uint64_t wr_mask = 1UL << SMSTATEEN_STATEN;
+    uint64_t wr_mask = (1UL << SMSTATEEN_STATEN) |
+                       (1UL << SMSTATEEN0_HSENVCFG);
 
     reg = &env->mstateen[csrno - CSR_MSTATEEN0H];
     val = (uint64_t)new_val << 32;
@@ -1671,7 +1741,8 @@ static RISCVException write_hstateen(CPURISCVState *env, int csrno,
                                      target_ulong new_val)
 {
     uint64_t *reg;
-    uint64_t wr_mask = 1UL << SMSTATEEN_STATEN;
+    uint64_t wr_mask = (1UL << SMSTATEEN_STATEN) |
+                       (1UL << SMSTATEEN0_HSENVCFG);
     int index = csrno - CSR_HSTATEEN0;
 
     reg = &env->hstateen[index];
@@ -1694,8 +1765,9 @@ static RISCVException write_hstateenh(CPURISCVState *env, int csrno,
 {
     uint64_t *reg;
     uint64_t val;
-    uint64_t wr_mask = 1UL << SMSTATEEN_STATEN;
     int index = csrno - CSR_HSTATEEN0H;
+    uint64_t wr_mask = (1UL << SMSTATEEN_STATEN) |
+                       (1UL << SMSTATEEN0_HSENVCFG);
 
     reg = &env->hstateen[index];
     val = (uint64_t)new_val << 32;
-- 
2.25.1
Re:[RFC PATCH v5 2/4] target/riscv: smstateen check for h/senvcfg
Posted by angell1518 2 years, 3 months ago 
At 2022-06-04 00:04:23, "Mayuresh Chitale" <mchitale@ventanamicro.com> wrote:

>Accesses to henvcfg, henvcfgh and senvcfg are allowed
>only if corresponding bit in mstateen0/hstateen0 is
>enabled. Otherwise an illegal instruction trap is
>generated.
>
>Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
>---
> target/riscv/csr.c | 84 ++++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 78 insertions(+), 6 deletions(-)
>
>diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>index 324fefce59..ae91ae1f7e 100644
>--- a/target/riscv/csr.c
>+++ b/target/riscv/csr.ccounteren
>@@ -39,6 +39,37 @@ void riscv_set_csr_ops(int csrno, riscv_csr_operations *ops)
> }
> 
> /* Predicates */
>+static RISCVException smstateen_acc_ok(CPURISCVState *env, int mode, int bit)
>+{
>+    CPUState *cs = env_cpu(env);
>+    RISCVCPU *cpu = RISCV_CPU(cs);
>+    bool virt = riscv_cpu_virt_enabled(env);
>+
>+    if (!cpu->cfg.ext_smstateen) {
>+        return RISCV_EXCP_NONE;
>+    }
>+
>+#if !defined(CONFIG_USER_ONLY)
>+    if (!(env->mstateen[0] & 1UL << bit)) {
>+        return RISCV_EXCP_ILLEGAL_INST;
>+    }
>+
I think mstateen only control access in priv mode under M mode. 

So we should take the current priv node into consideration here.

>+    if (virt) {
>+        if (!(env->hstateen[0] & 1UL << bit)) {
>+            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
>+        }
>+    }
>+
>+    if (mode == PRV_U) {
>+        if (!(env->sstateen[0] & 1UL << bit)) {
>+            return RISCV_EXCP_ILLEGAL_INST;

>+ }
I think there are two problem here:

The first is that we should also take virt mode into consideration here too. As the spec said:

"if executing in VS or VU mode and the circumstances for a virtual instruction
exception apply, raises a virtual instruction exception instead of an illegal instruction exception"

So it will generate RISCV_EXCP_VIRT_INSTRUCTION_FAULT in VU mode.


The second is that sstateen csr works only when 'S' extension is enabled



Regards,
Weiwei Li

>+    }
>+#endif
>+
>+    return RISCV_EXCP_NONE;
>+}
>+
> static RISCVException fs(CPURISCVState *env, int csrno)
> {
> #if !defined(CONFIG_USER_ONLY)
>@@ -1557,6 +1588,13 @@ static RISCVException write_menvcfgh(CPURISCVState *env, int csrno,
> static RISCVException read_senvcfg(CPURISCVState *env, int csrno,
>                                  target_ulong *val)
> {
>+    RISCVException ret;
>+
>+    ret = smstateen_acc_ok(env, PRV_S, SMSTATEEN0_HSENVCFG);
>+    if (ret != RISCV_EXCP_NONE) {
>+        return ret;
>+    }
>+
>     *val = env->senvcfg;
>     return RISCV_EXCP_NONE;
> }
>@@ -1565,15 +1603,27 @@ static RISCVException write_senvcfg(CPURISCVState *env, int csrno,
>                                   target_ulong val)
> {
>     uint64_t mask = SENVCFG_FIOM | SENVCFG_CBIE | SENVCFG_CBCFE | SENVCFG_CBZE;
>+    RISCVException ret;
> 
>-    env->senvcfg = (env->senvcfg & ~mask) | (val & mask);
>+    ret = smstateen_acc_ok(env, PRV_S, SMSTATEEN0_HSENVCFG);
>+    if (ret != RISCV_EXCP_NONE) {
>+        return ret;
>+    }
> 
>+    env->senvcfg = (env->senvcfg & ~mask) | (val & mask);
>     return RISCV_EXCP_NONE;
> }
> 
> static RISCVException read_henvcfg(CPURISCVState *env, int csrno,
>                                  target_ulong *val)
> {
>+    RISCVException ret;
>+
>+    ret = smstateen_acc_ok(env, PRV_S, SMSTATEEN0_HSENVCFG);
>+    if (ret != RISCV_EXCP_NONE) {
>+        return ret;
>+    }
>+
>     *val = env->henvcfg;
>     return RISCV_EXCP_NONE;
> }
>@@ -1582,6 +1632,12 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
>                                   target_ulong val)
> {
>     uint64_t mask = HENVCFG_FIOM | HENVCFG_CBIE | HENVCFG_CBCFE | HENVCFG_CBZE;
>+    RISCVException ret;
>+
>+    ret = smstateen_acc_ok(env, PRV_S, SMSTATEEN0_HSENVCFG);
>+    if (ret != RISCV_EXCP_NONE) {
>+        return ret;
>+    }
> 
>     if (riscv_cpu_mxl(env) == MXL_RV64) {
>         mask |= HENVCFG_PBMTE | HENVCFG_STCE;
>@@ -1595,6 +1651,13 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
> static RISCVException read_henvcfgh(CPURISCVState *env, int csrno,
>                                  target_ulong *val)
> {
>+    RISCVException ret;
>+
>+    ret = smstateen_acc_ok(env, PRV_S, SMSTATEEN0_HSENVCFG);
>+    if (ret != RISCV_EXCP_NONE) {
>+        return ret;
>+    }
>+
>     *val = env->henvcfg >> 32;
>     return RISCV_EXCP_NONE;
> }
>@@ -1604,9 +1667,14 @@ static RISCVException write_henvcfgh(CPURISCVState *env, int csrno,
> {
>     uint64_t mask = HENVCFG_PBMTE | HENVCFG_STCE;
>     uint64_t valh = (uint64_t)val << 32;
>+    RISCVException ret;
> 
>-    env->henvcfg = (env->henvcfg & ~mask) | (valh & mask);
>+    ret = smstateen_acc_ok(env, PRV_S, SMSTATEEN0_HSENVCFG);
>+    if (ret != RISCV_EXCP_NONE) {
>+        return ret;
>+    }
> 
>+    env->henvcfg = (env->henvcfg & ~mask) | (valh & mask);
>     return RISCV_EXCP_NONE;
> }
> 
>@@ -1628,7 +1696,8 @@ static RISCVException write_mstateen(CPURISCVState *env, int csrno,
>                                      target_ulong new_val)
> {
>     uint64_t *reg;
>-    uint64_t wr_mask = 1UL << SMSTATEEN_STATEN;
>+    uint64_t wr_mask = (1UL << SMSTATEEN_STATEN) |
>+                       (1UL << SMSTATEEN0_HSENVCFG);
> 
>     reg = &env->mstateen[csrno - CSR_MSTATEEN0];
>     write_smstateen(env, reg, wr_mask, new_val);
>@@ -1649,7 +1718,8 @@ static RISCVException write_mstateenh(CPURISCVState *env, int csrno,
> {
>     uint64_t *reg;
>     uint64_t val;
>-    uint64_t wr_mask = 1UL << SMSTATEEN_STATEN;
>+    uint64_t wr_mask = (1UL << SMSTATEEN_STATEN) |
>+                       (1UL << SMSTATEEN0_HSENVCFG);
> 
>     reg = &env->mstateen[csrno - CSR_MSTATEEN0H];
>     val = (uint64_t)new_val << 32;
>@@ -1671,7 +1741,8 @@ static RISCVException write_hstateen(CPURISCVState *env, int csrno,
>                                      target_ulong new_val)
> {
>     uint64_t *reg;
>-    uint64_t wr_mask = 1UL << SMSTATEEN_STATEN;
>+    uint64_t wr_mask = (1UL << SMSTATEEN_STATEN) |
>+                       (1UL << SMSTATEEN0_HSENVCFG);
>     int index = csrno - CSR_HSTATEEN0;
> 
>     reg = &env->hstateen[index];
>@@ -1694,8 +1765,9 @@ static RISCVException write_hstateenh(CPURISCVState *env, int csrno,
> {
>     uint64_t *reg;
>     uint64_t val;
>-    uint64_t wr_mask = 1UL << SMSTATEEN_STATEN;
>     int index = csrno - CSR_HSTATEEN0H;
>+    uint64_t wr_mask = (1UL << SMSTATEEN_STATEN) |
>+                       (1UL << SMSTATEEN0_HSENVCFG);
> 
>     reg = &env->hstateen[index];
>     val = (uint64_t)new_val << 32;
>-- 
>2.25.1
>
Re: [RFC PATCH v5 2/4] target/riscv: smstateen check for h/senvcfg
Posted by Mayuresh Chitale 2 years, 3 months ago 
On Sat, 2022-07-02 at 18:33 +0800, angell1518 wrote:
> At 2022-06-04 00:04:23, "Mayuresh Chitale" <mchitale@ventanamicro.com
> > wrote:
> >Accesses to henvcfg, henvcfgh and senvcfg are allowed
> >only if corresponding bit in mstateen0/hstateen0 is
> >enabled. Otherwise an illegal instruction trap is
> >generated.
> >
> >Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> >---
> > target/riscv/csr.c | 84 ++++++++++++++++++++++++++++++++++++++++++-
> ---
> > 1 file changed, 78 insertions(+), 6 deletions(-)
> >
> >diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> >index 324fefce59..ae91ae1f7e 100644
> >--- a/target/riscv/csr.c
> >+++ b/target/riscv/csr.ccounteren
> >@@ -39,6 +39,37 @@ void riscv_set_csr_ops(int csrno,
> riscv_csr_operations *ops)
> > }
> > 
> > /* Predicates */
> >+static RISCVException smstateen_acc_ok(CPURISCVState *env, int
> mode, int bit)
> >+{
> >+    CPUState *cs = env_cpu(env);
> >+    RISCVCPU *cpu = RISCV_CPU(cs);
> >+    bool virt = riscv_cpu_virt_enabled(env);
> >+
> >+    if (!cpu->cfg.ext_smstateen) {
> >+        return RISCV_EXCP_NONE;
> >+    }
> >+
> >+#if !defined(CONFIG_USER_ONLY)
> >+    if (!(env->mstateen[0] & 1UL << bit)) {
> >+        return RISCV_EXCP_ILLEGAL_INST;
> >+    }
> >+
> I think mstateen only control access in priv mode under M mode.  
> So we should take the current priv node into consideration here.

For any curent priv mode if the required bit is not set in mstateen we
can return here itself. For e.g if current priv mode is S then we only
check the required bit in mstateen. If current priv mode is 'U', we
need to check the required bit in mstateen and sstateen

> >+    if (virt) {
> >+        if (!(env->hstateen[0] & 1UL << bit)) {
> >+            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> >+        }
> >+    }
> >+
> >+    if (mode == PRV_U) {
> >+        if (!(env->sstateen[0] & 1UL << bit)) {
> >+            return RISCV_EXCP_ILLEGAL_INST;
> >+ }
> I think there are two problem here:
> The first is that we should also take virt mode into consideration
> here too. 
Actually virt mode is considered above for both priv modes S and U.
> As the spec said: 
> "if executing in VS or VU mode and the circumstances for a virtual
> instruction
> exception apply, raises a virtual instruction exception instead of an
> illegal instruction exception"
> So it will generate RISCV_EXCP_VIRT_INSTRUCTION_FAULT in VU mode.

> 
> The second is that sstateen csr works only when 'S' extension is
> enabled
> 
I will fix it in the next version.

> Regards,
> Weiwei Li
> >+    }
> >+#endif
> >+
> >+    return RISCV_EXCP_NONE;
> >+}
> >+
> > static RISCVException fs(CPURISCVState *env, int csrno)
> > {
> > #if !defined(CONFIG_USER_ONLY)
> >@@ -1557,6 +1588,13 @@ static RISCVException
> write_menvcfgh(CPURISCVState *env, int csrno,
> > static RISCVException read_senvcfg(CPURISCVState *env, int csrno,
> >                                  target_ulong *val)
> > {
> >+    RISCVException ret;
> >+
> >+    ret = smstateen_acc_ok(env, PRV_S, SMSTATEEN0_HSENVCFG);
> >+    if (ret != RISCV_EXCP_NONE) {
> >+        return ret;
> >+    }
> >+
> >     *val = env->senvcfg;
> >     return RISCV_EXCP_NONE;
> > }
> >@@ -1565,15 +1603,27 @@ static RISCVException
> write_senvcfg(CPURISCVState *env, int csrno,
> >                                   target_ulong val)
> > {
> >     uint64_t mask = SENVCFG_FIOM | SENVCFG_CBIE | SENVCFG_CBCFE |
> SENVCFG_CBZE;
> >+    RISCVException ret;
> > 
> >-    env->senvcfg = (env->senvcfg & ~mask) | (val & mask);
> >+    ret = smstateen_acc_ok(env, PRV_S, SMSTATEEN0_HSENVCFG);
> >+    if (ret != RISCV_EXCP_NONE) {
> >+        return ret;
> >+    }
> > 
> >+    env->senvcfg = (env->senvcfg & ~mask) | (val & mask);
> >     return RISCV_EXCP_NONE;
> > }
> > 
> > static RISCVException read_henvcfg(CPURISCVState *env, int csrno,
> >                                  target_ulong *val)
> > {
> >+    RISCVException ret;
> >+
> >+    ret = smstateen_acc_ok(env, PRV_S, SMSTATEEN0_HSENVCFG);
> >+    if (ret != RISCV_EXCP_NONE) {
> >+        return ret;
> >+    }
> >+
> >     *val = env->henvcfg;
> >     return RISCV_EXCP_NONE;
> > }
> >@@ -1582,6 +1632,12 @@ static RISCVException
> write_henvcfg(CPURISCVState *env, int csrno,
> >                                   target_ulong val)
> > {
> >     uint64_t mask = HENVCFG_FIOM | HENVCFG_CBIE | HENVCFG_CBCFE |
> HENVCFG_CBZE;
> >+    RISCVException ret;
> >+
> >+    ret = smstateen_acc_ok(env, PRV_S, SMSTATEEN0_HSENVCFG);
> >+    if (ret != RISCV_EXCP_NONE) {
> >+        return ret;
> >+    }
> > 
> >     if (riscv_cpu_mxl(env) == MXL_RV64) {
> >         mask |= HENVCFG_PBMTE | HENVCFG_STCE;
> >@@ -1595,6 +1651,13 @@ static RISCVException
> write_henvcfg(CPURISCVState *env, int csrno,
> > static RISCVException read_henvcfgh(CPURISCVState *env, int csrno,
> >                                  target_ulong *val)
> > {
> >+    RISCVException ret;
> >+
> >+    ret = smstateen_acc_ok(env, PRV_S, SMSTATEEN0_HSENVCFG);
> >+    if (ret != RISCV_EXCP_NONE) {
> >+        return ret;
> >+    }
> >+
> >     *val = env->henvcfg >> 32;
> >     return RISCV_EXCP_NONE;
> > }
> >@@ -1604,9 +1667,14 @@ static RISCVException
> write_henvcfgh(CPURISCVState *env, int csrno,
> > {
> >     uint64_t mask = HENVCFG_PBMTE | HENVCFG_STCE;
> >     uint64_t valh = (uint64_t)val << 32;
> >+    RISCVException ret;
> > 
> >-    env->henvcfg = (env->henvcfg & ~mask) | (valh & mask);
> >+    ret = smstateen_acc_ok(env, PRV_S, SMSTATEEN0_HSENVCFG);
> >+    if (ret != RISCV_EXCP_NONE) {
> >+        return ret;
> >+    }
> > 
> >+    env->henvcfg = (env->henvcfg & ~mask) | (valh & mask);
> >     return RISCV_EXCP_NONE;
> > }
> > 
> >@@ -1628,7 +1696,8 @@ static RISCVException
> write_mstateen(CPURISCVState *env, int csrno,
> >                                      target_ulong new_val)
> > {
> >     uint64_t *reg;
> >-    uint64_t wr_mask = 1UL << SMSTATEEN_STATEN;
> >+    uint64_t wr_mask = (1UL << SMSTATEEN_STATEN) |
> >+                       (1UL << SMSTATEEN0_HSENVCFG);
> > 
> >     reg = &env->mstateen[csrno - CSR_MSTATEEN0];
> >     write_smstateen(env, reg, wr_mask, new_val);
> >@@ -1649,7 +1718,8 @@ static RISCVException
> write_mstateenh(CPURISCVState *env, int csrno,
> > {
> >     uint64_t *reg;
> >     uint64_t val;
> >-    uint64_t wr_mask = 1UL << SMSTATEEN_STATEN;
> >+    uint64_t wr_mask = (1UL << SMSTATEEN_STATEN) |
> >+                       (1UL << SMSTATEEN0_HSENVCFG);
> > 
> >     reg = &env->mstateen[csrno - CSR_MSTATEEN0H];
> >     val = (uint64_t)new_val << 32;
> >@@ -1671,7 +1741,8 @@ static RISCVException
> write_hstateen(CPURISCVState *env, int csrno,
> >                                      target_ulong new_val)
> > {
> >     uint64_t *reg;
> >-    uint64_t wr_mask = 1UL << SMSTATEEN_STATEN;
> >+    uint64_t wr_mask = (1UL << SMSTATEEN_STATEN) |
> >+                       (1UL << SMSTATEEN0_HSENVCFG);
> >     int index = csrno - CSR_HSTATEEN0;
> > 
> >     reg = &env->hstateen[index];
> >@@ -1694,8 +1765,9 @@ static RISCVException
> write_hstateenh(CPURISCVState *env, int csrno,
> > {
> >     uint64_t *reg;
> >     uint64_t val;
> >-    uint64_t wr_mask = 1UL << SMSTATEEN_STATEN;
> >     int index = csrno - CSR_HSTATEEN0H;
> >+    uint64_t wr_mask = (1UL << SMSTATEEN_STATEN) |
> >+                       (1UL << SMSTATEEN0_HSENVCFG);
> > 
> >     reg = &env->hstateen[index];
> >     val = (uint64_t)new_val << 32;
> >-- 
> >2.25.1
> >
Re: [RFC PATCH v5 2/4] target/riscv: smstateen check for h/senvcfg
Posted by Weiwei Li 2 years, 3 months ago 
在 2022/7/8 上午1:20, Mayuresh Chitale 写道:
> On Sat, 2022-07-02 at 18:33 +0800, angell1518 wrote:
>> At 2022-06-04 00:04:23, "Mayuresh Chitale" <mchitale@ventanamicro.com
>>> wrote:
>>> Accesses to henvcfg, henvcfgh and senvcfg are allowed
>>> only if corresponding bit in mstateen0/hstateen0 is
>>> enabled. Otherwise an illegal instruction trap is
>>> generated.
>>>
>>> Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
>>> ---
>>> target/riscv/csr.c | 84 ++++++++++++++++++++++++++++++++++++++++++-
>> ---
>>> 1 file changed, 78 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>>> index 324fefce59..ae91ae1f7e 100644
>>> --- a/target/riscv/csr.c
>>> +++ b/target/riscv/csr.ccounteren
>>> @@ -39,6 +39,37 @@ void riscv_set_csr_ops(int csrno,
>> riscv_csr_operations *ops)
>>> }
>>>
>>> /* Predicates */
>>> +static RISCVException smstateen_acc_ok(CPURISCVState *env, int
>> mode, int bit)
>>> +{
>>> +    CPUState *cs = env_cpu(env);
>>> +    RISCVCPU *cpu = RISCV_CPU(cs);
>>> +    bool virt = riscv_cpu_virt_enabled(env);
>>> +
>>> +    if (!cpu->cfg.ext_smstateen) {
>>> +        return RISCV_EXCP_NONE;
>>> +    }
>>> +
>>> +#if !defined(CONFIG_USER_ONLY)
>>> +    if (!(env->mstateen[0] & 1UL << bit)) {
>>> +        return RISCV_EXCP_ILLEGAL_INST;
>>> +    }
>>> +
>> I think mstateen only control access in priv mode under M mode.
>> So we should take the current priv node into consideration here.
> For any curent priv mode if the required bit is not set in mstateen we
> can return here itself. For e.g if current priv mode is S then we only
> check the required bit in mstateen. If current priv mode is 'U', we
> need to check the required bit in mstateen and sstateen
>
mstateen only controls the access from less-privilege mode. So do hstateen and sstateen.  we should pass all of the
check, if current priv is PRV_M. like this:
+    if (env->priv == PRV_M) {
+        return RISCV_EXCP_NONE;
+    }

>>> +    if (virt) {
>>> +        if (!(env->hstateen[0] & 1UL << bit)) {
>>> +            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
>>> +        }
>>> +    }
>>> +
>>> +    if (mode == PRV_U) {
>>> +        if (!(env->sstateen[0] & 1UL << bit)) {
>>> +            return RISCV_EXCP_ILLEGAL_INST;
>>> + }
>> I think there are two problem here:
>> The first is that we should also take virt mode into consideration
>> here too.
> Actually virt mode is considered above for both priv modes S and U.

Yeah(we also should pass this check for current priv is PRV_M). However, 
there's still a problem

here: the mode here is the mode for csr not the current priv mode.  
Actually, Sstateen will control the access

for user mode csr(such as fcsr)  from U mode. So taking the following 
question into consideration, the  total check

may be:

+    if (mode == PRV_U &&env->priv == PRV_U) {
+        if (riscv_has_ext(env, RVS)  && !(env->sstateen[0] & 1UL << bit)) {
+            return RISCV_EXCP_ILLEGAL_INST;
+ }

Regards,
Weiwei Li

>> As the spec said:
>> "if executing in VS or VU mode and the circumstances for a virtual
>> instruction
>> exception apply, raises a virtual instruction exception instead of an
>> illegal instruction exception"
>> So it will generate RISCV_EXCP_VIRT_INSTRUCTION_FAULT in VU mode.
>> The second is that sstateen csr works only when 'S' extension is
>> enabled
>>
> I will fix it in the next version.
>
>> Regards,
>> Weiwei Li
>>> +    }
>>> +#endif
>>> +
>>> +    return RISCV_EXCP_NONE;
>>> +}
>>> +
>>> static RISCVException fs(CPURISCVState *env, int csrno)
>>> {
>>> #if !defined(CONFIG_USER_ONLY)
>>> @@ -1557,6 +1588,13 @@ static RISCVException
>> write_menvcfgh(CPURISCVState *env, int csrno,
>>> static RISCVException read_senvcfg(CPURISCVState *env, int csrno,
>>>                                   target_ulong *val)
>>> {
>>> +    RISCVException ret;
>>> +
>>> +    ret = smstateen_acc_ok(env, PRV_S, SMSTATEEN0_HSENVCFG);
>>> +    if (ret != RISCV_EXCP_NONE) {
>>> +        return ret;
>>> +    }
>>> +
>>>      *val = env->senvcfg;
>>>      return RISCV_EXCP_NONE;
>>> }
>>> @@ -1565,15 +1603,27 @@ static RISCVException
>> write_senvcfg(CPURISCVState *env, int csrno,
>>>                                    target_ulong val)
>>> {
>>>      uint64_t mask = SENVCFG_FIOM | SENVCFG_CBIE | SENVCFG_CBCFE |
>> SENVCFG_CBZE;
>>> +    RISCVException ret;
>>>
>>> -    env->senvcfg = (env->senvcfg & ~mask) | (val & mask);
>>> +    ret = smstateen_acc_ok(env, PRV_S, SMSTATEEN0_HSENVCFG);
>>> +    if (ret != RISCV_EXCP_NONE) {
>>> +        return ret;
>>> +    }
>>>
>>> +    env->senvcfg = (env->senvcfg & ~mask) | (val & mask);
>>>      return RISCV_EXCP_NONE;
>>> }
>>>
>>> static RISCVException read_henvcfg(CPURISCVState *env, int csrno,
>>>                                   target_ulong *val)
>>> {
>>> +    RISCVException ret;
>>> +
>>> +    ret = smstateen_acc_ok(env, PRV_S, SMSTATEEN0_HSENVCFG);
>>> +    if (ret != RISCV_EXCP_NONE) {
>>> +        return ret;
>>> +    }
>>> +
>>>      *val = env->henvcfg;
>>>      return RISCV_EXCP_NONE;
>>> }
>>> @@ -1582,6 +1632,12 @@ static RISCVException
>> write_henvcfg(CPURISCVState *env, int csrno,
>>>                                    target_ulong val)
>>> {
>>>      uint64_t mask = HENVCFG_FIOM | HENVCFG_CBIE | HENVCFG_CBCFE |
>> HENVCFG_CBZE;
>>> +    RISCVException ret;
>>> +
>>> +    ret = smstateen_acc_ok(env, PRV_S, SMSTATEEN0_HSENVCFG);
>>> +    if (ret != RISCV_EXCP_NONE) {
>>> +        return ret;
>>> +    }
>>>
>>>      if (riscv_cpu_mxl(env) == MXL_RV64) {
>>>          mask |= HENVCFG_PBMTE | HENVCFG_STCE;
>>> @@ -1595,6 +1651,13 @@ static RISCVException
>> write_henvcfg(CPURISCVState *env, int csrno,
>>> static RISCVException read_henvcfgh(CPURISCVState *env, int csrno,
>>>                                   target_ulong *val)
>>> {
>>> +    RISCVException ret;
>>> +
>>> +    ret = smstateen_acc_ok(env, PRV_S, SMSTATEEN0_HSENVCFG);
>>> +    if (ret != RISCV_EXCP_NONE) {
>>> +        return ret;
>>> +    }
>>> +
>>>      *val = env->henvcfg >> 32;
>>>      return RISCV_EXCP_NONE;
>>> }
>>> @@ -1604,9 +1667,14 @@ static RISCVException
>> write_henvcfgh(CPURISCVState *env, int csrno,
>>> {
>>>      uint64_t mask = HENVCFG_PBMTE | HENVCFG_STCE;
>>>      uint64_t valh = (uint64_t)val << 32;
>>> +    RISCVException ret;
>>>
>>> -    env->henvcfg = (env->henvcfg & ~mask) | (valh & mask);
>>> +    ret = smstateen_acc_ok(env, PRV_S, SMSTATEEN0_HSENVCFG);
>>> +    if (ret != RISCV_EXCP_NONE) {
>>> +        return ret;
>>> +    }
>>>
>>> +    env->henvcfg = (env->henvcfg & ~mask) | (valh & mask);
>>>      return RISCV_EXCP_NONE;
>>> }
>>>
>>> @@ -1628,7 +1696,8 @@ static RISCVException
>> write_mstateen(CPURISCVState *env, int csrno,
>>>                                       target_ulong new_val)
>>> {
>>>      uint64_t *reg;
>>> -    uint64_t wr_mask = 1UL << SMSTATEEN_STATEN;
>>> +    uint64_t wr_mask = (1UL << SMSTATEEN_STATEN) |
>>> +                       (1UL << SMSTATEEN0_HSENVCFG);
>>>
>>>      reg = &env->mstateen[csrno - CSR_MSTATEEN0];
>>>      write_smstateen(env, reg, wr_mask, new_val);
>>> @@ -1649,7 +1718,8 @@ static RISCVException
>> write_mstateenh(CPURISCVState *env, int csrno,
>>> {
>>>      uint64_t *reg;
>>>      uint64_t val;
>>> -    uint64_t wr_mask = 1UL << SMSTATEEN_STATEN;
>>> +    uint64_t wr_mask = (1UL << SMSTATEEN_STATEN) |
>>> +                       (1UL << SMSTATEEN0_HSENVCFG);
>>>
>>>      reg = &env->mstateen[csrno - CSR_MSTATEEN0H];
>>>      val = (uint64_t)new_val << 32;
>>> @@ -1671,7 +1741,8 @@ static RISCVException
>> write_hstateen(CPURISCVState *env, int csrno,
>>>                                       target_ulong new_val)
>>> {
>>>      uint64_t *reg;
>>> -    uint64_t wr_mask = 1UL << SMSTATEEN_STATEN;
>>> +    uint64_t wr_mask = (1UL << SMSTATEEN_STATEN) |
>>> +                       (1UL << SMSTATEEN0_HSENVCFG);
>>>      int index = csrno - CSR_HSTATEEN0;
>>>
>>>      reg = &env->hstateen[index];
>>> @@ -1694,8 +1765,9 @@ static RISCVException
>> write_hstateenh(CPURISCVState *env, int csrno,
>>> {
>>>      uint64_t *reg;
>>>      uint64_t val;
>>> -    uint64_t wr_mask = 1UL << SMSTATEEN_STATEN;
>>>      int index = csrno - CSR_HSTATEEN0H;
>>> +    uint64_t wr_mask = (1UL << SMSTATEEN_STATEN) |
>>> +                       (1UL << SMSTATEEN0_HSENVCFG);
>>>
>>>      reg = &env->hstateen[index];
>>>      val = (uint64_t)new_val << 32;
>>> -- 
>>> 2.25.1
>>>
Re: [RFC PATCH v5 2/4] target/riscv: smstateen check for h/senvcfg
Posted by Alistair Francis 2 years, 4 months ago 
On Sat, Jun 4, 2022 at 2:16 AM Mayuresh Chitale
<mchitale@ventanamicro.com> wrote:
>
> Accesses to henvcfg, henvcfgh and senvcfg are allowed
> only if corresponding bit in mstateen0/hstateen0 is
> enabled. Otherwise an illegal instruction trap is
> generated.
>
> Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>

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

Alistair

> ---
>  target/riscv/csr.c | 84 ++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 78 insertions(+), 6 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 324fefce59..ae91ae1f7e 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -39,6 +39,37 @@ void riscv_set_csr_ops(int csrno, riscv_csr_operations *ops)
>  }
>
>  /* Predicates */
> +static RISCVException smstateen_acc_ok(CPURISCVState *env, int mode, int bit)
> +{
> +    CPUState *cs = env_cpu(env);
> +    RISCVCPU *cpu = RISCV_CPU(cs);
> +    bool virt = riscv_cpu_virt_enabled(env);
> +
> +    if (!cpu->cfg.ext_smstateen) {
> +        return RISCV_EXCP_NONE;
> +    }
> +
> +#if !defined(CONFIG_USER_ONLY)
> +    if (!(env->mstateen[0] & 1UL << bit)) {
> +        return RISCV_EXCP_ILLEGAL_INST;
> +    }
> +
> +    if (virt) {
> +        if (!(env->hstateen[0] & 1UL << bit)) {
> +            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> +        }
> +    }
> +
> +    if (mode == PRV_U) {
> +        if (!(env->sstateen[0] & 1UL << bit)) {
> +            return RISCV_EXCP_ILLEGAL_INST;
> +        }
> +    }
> +#endif
> +
> +    return RISCV_EXCP_NONE;
> +}
> +
>  static RISCVException fs(CPURISCVState *env, int csrno)
>  {
>  #if !defined(CONFIG_USER_ONLY)
> @@ -1557,6 +1588,13 @@ static RISCVException write_menvcfgh(CPURISCVState *env, int csrno,
>  static RISCVException read_senvcfg(CPURISCVState *env, int csrno,
>                                   target_ulong *val)
>  {
> +    RISCVException ret;
> +
> +    ret = smstateen_acc_ok(env, PRV_S, SMSTATEEN0_HSENVCFG);
> +    if (ret != RISCV_EXCP_NONE) {
> +        return ret;
> +    }
> +
>      *val = env->senvcfg;
>      return RISCV_EXCP_NONE;
>  }
> @@ -1565,15 +1603,27 @@ static RISCVException write_senvcfg(CPURISCVState *env, int csrno,
>                                    target_ulong val)
>  {
>      uint64_t mask = SENVCFG_FIOM | SENVCFG_CBIE | SENVCFG_CBCFE | SENVCFG_CBZE;
> +    RISCVException ret;
>
> -    env->senvcfg = (env->senvcfg & ~mask) | (val & mask);
> +    ret = smstateen_acc_ok(env, PRV_S, SMSTATEEN0_HSENVCFG);
> +    if (ret != RISCV_EXCP_NONE) {
> +        return ret;
> +    }
>
> +    env->senvcfg = (env->senvcfg & ~mask) | (val & mask);
>      return RISCV_EXCP_NONE;
>  }
>
>  static RISCVException read_henvcfg(CPURISCVState *env, int csrno,
>                                   target_ulong *val)
>  {
> +    RISCVException ret;
> +
> +    ret = smstateen_acc_ok(env, PRV_S, SMSTATEEN0_HSENVCFG);
> +    if (ret != RISCV_EXCP_NONE) {
> +        return ret;
> +    }
> +
>      *val = env->henvcfg;
>      return RISCV_EXCP_NONE;
>  }
> @@ -1582,6 +1632,12 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
>                                    target_ulong val)
>  {
>      uint64_t mask = HENVCFG_FIOM | HENVCFG_CBIE | HENVCFG_CBCFE | HENVCFG_CBZE;
> +    RISCVException ret;
> +
> +    ret = smstateen_acc_ok(env, PRV_S, SMSTATEEN0_HSENVCFG);
> +    if (ret != RISCV_EXCP_NONE) {
> +        return ret;
> +    }
>
>      if (riscv_cpu_mxl(env) == MXL_RV64) {
>          mask |= HENVCFG_PBMTE | HENVCFG_STCE;
> @@ -1595,6 +1651,13 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
>  static RISCVException read_henvcfgh(CPURISCVState *env, int csrno,
>                                   target_ulong *val)
>  {
> +    RISCVException ret;
> +
> +    ret = smstateen_acc_ok(env, PRV_S, SMSTATEEN0_HSENVCFG);
> +    if (ret != RISCV_EXCP_NONE) {
> +        return ret;
> +    }
> +
>      *val = env->henvcfg >> 32;
>      return RISCV_EXCP_NONE;
>  }
> @@ -1604,9 +1667,14 @@ static RISCVException write_henvcfgh(CPURISCVState *env, int csrno,
>  {
>      uint64_t mask = HENVCFG_PBMTE | HENVCFG_STCE;
>      uint64_t valh = (uint64_t)val << 32;
> +    RISCVException ret;
>
> -    env->henvcfg = (env->henvcfg & ~mask) | (valh & mask);
> +    ret = smstateen_acc_ok(env, PRV_S, SMSTATEEN0_HSENVCFG);
> +    if (ret != RISCV_EXCP_NONE) {
> +        return ret;
> +    }
>
> +    env->henvcfg = (env->henvcfg & ~mask) | (valh & mask);
>      return RISCV_EXCP_NONE;
>  }
>
> @@ -1628,7 +1696,8 @@ static RISCVException write_mstateen(CPURISCVState *env, int csrno,
>                                       target_ulong new_val)
>  {
>      uint64_t *reg;
> -    uint64_t wr_mask = 1UL << SMSTATEEN_STATEN;
> +    uint64_t wr_mask = (1UL << SMSTATEEN_STATEN) |
> +                       (1UL << SMSTATEEN0_HSENVCFG);
>
>      reg = &env->mstateen[csrno - CSR_MSTATEEN0];
>      write_smstateen(env, reg, wr_mask, new_val);
> @@ -1649,7 +1718,8 @@ static RISCVException write_mstateenh(CPURISCVState *env, int csrno,
>  {
>      uint64_t *reg;
>      uint64_t val;
> -    uint64_t wr_mask = 1UL << SMSTATEEN_STATEN;
> +    uint64_t wr_mask = (1UL << SMSTATEEN_STATEN) |
> +                       (1UL << SMSTATEEN0_HSENVCFG);
>
>      reg = &env->mstateen[csrno - CSR_MSTATEEN0H];
>      val = (uint64_t)new_val << 32;
> @@ -1671,7 +1741,8 @@ static RISCVException write_hstateen(CPURISCVState *env, int csrno,
>                                       target_ulong new_val)
>  {
>      uint64_t *reg;
> -    uint64_t wr_mask = 1UL << SMSTATEEN_STATEN;
> +    uint64_t wr_mask = (1UL << SMSTATEEN_STATEN) |
> +                       (1UL << SMSTATEEN0_HSENVCFG);
>      int index = csrno - CSR_HSTATEEN0;
>
>      reg = &env->hstateen[index];
> @@ -1694,8 +1765,9 @@ static RISCVException write_hstateenh(CPURISCVState *env, int csrno,
>  {
>      uint64_t *reg;
>      uint64_t val;
> -    uint64_t wr_mask = 1UL << SMSTATEEN_STATEN;
>      int index = csrno - CSR_HSTATEEN0H;
> +    uint64_t wr_mask = (1UL << SMSTATEEN_STATEN) |
> +                       (1UL << SMSTATEEN0_HSENVCFG);
>
>      reg = &env->hstateen[index];
>      val = (uint64_t)new_val << 32;
> --
> 2.25.1
>
>
Re: [RFC PATCH v5 2/4] target/riscv: smstateen check for h/senvcfg
Posted by Alistair Francis 2 years, 4 months ago 
On Sat, Jun 4, 2022 at 2:16 AM Mayuresh Chitale
<mchitale@ventanamicro.com> wrote:
>
> Accesses to henvcfg, henvcfgh and senvcfg are allowed
> only if corresponding bit in mstateen0/hstateen0 is
> enabled. Otherwise an illegal instruction trap is
> generated.
>
> Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> ---
>  target/riscv/csr.c | 84 ++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 78 insertions(+), 6 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 324fefce59..ae91ae1f7e 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -39,6 +39,37 @@ void riscv_set_csr_ops(int csrno, riscv_csr_operations *ops)
>  }
>
>  /* Predicates */
> +static RISCVException smstateen_acc_ok(CPURISCVState *env, int mode, int bit)
> +{
> +    CPUState *cs = env_cpu(env);
> +    RISCVCPU *cpu = RISCV_CPU(cs);
> +    bool virt = riscv_cpu_virt_enabled(env);
> +
> +    if (!cpu->cfg.ext_smstateen) {
> +        return RISCV_EXCP_NONE;
> +    }
> +
> +#if !defined(CONFIG_USER_ONLY)
> +    if (!(env->mstateen[0] & 1UL << bit)) {
> +        return RISCV_EXCP_ILLEGAL_INST;
> +    }
> +
> +    if (virt) {
> +        if (!(env->hstateen[0] & 1UL << bit)) {
> +            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> +        }
> +    }
> +
> +    if (mode == PRV_U) {
> +        if (!(env->sstateen[0] & 1UL << bit)) {
> +            return RISCV_EXCP_ILLEGAL_INST;
> +        }
> +    }
> +#endif
> +
> +    return RISCV_EXCP_NONE;
> +}
> +
>  static RISCVException fs(CPURISCVState *env, int csrno)
>  {
>  #if !defined(CONFIG_USER_ONLY)
> @@ -1557,6 +1588,13 @@ static RISCVException write_menvcfgh(CPURISCVState *env, int csrno,
>  static RISCVException read_senvcfg(CPURISCVState *env, int csrno,
>                                   target_ulong *val)
>  {
> +    RISCVException ret;
> +
> +    ret = smstateen_acc_ok(env, PRV_S, SMSTATEEN0_HSENVCFG);

Couldn't this be part of the original permission check so we don't
need a second check?

Alistair

> +    if (ret != RISCV_EXCP_NONE) {
> +        return ret;
> +    }
> +
>      *val = env->senvcfg;
>      return RISCV_EXCP_NONE;
>  }
> @@ -1565,15 +1603,27 @@ static RISCVException write_senvcfg(CPURISCVState *env, int csrno,
>                                    target_ulong val)
>  {
>      uint64_t mask = SENVCFG_FIOM | SENVCFG_CBIE | SENVCFG_CBCFE | SENVCFG_CBZE;
> +    RISCVException ret;
>
> -    env->senvcfg = (env->senvcfg & ~mask) | (val & mask);
> +    ret = smstateen_acc_ok(env, PRV_S, SMSTATEEN0_HSENVCFG);
> +    if (ret != RISCV_EXCP_NONE) {
> +        return ret;
> +    }
>
> +    env->senvcfg = (env->senvcfg & ~mask) | (val & mask);
>      return RISCV_EXCP_NONE;
>  }
>
>  static RISCVException read_henvcfg(CPURISCVState *env, int csrno,
>                                   target_ulong *val)
>  {
> +    RISCVException ret;
> +
> +    ret = smstateen_acc_ok(env, PRV_S, SMSTATEEN0_HSENVCFG);
> +    if (ret != RISCV_EXCP_NONE) {
> +        return ret;
> +    }
> +
>      *val = env->henvcfg;
>      return RISCV_EXCP_NONE;
>  }
> @@ -1582,6 +1632,12 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
>                                    target_ulong val)
>  {
>      uint64_t mask = HENVCFG_FIOM | HENVCFG_CBIE | HENVCFG_CBCFE | HENVCFG_CBZE;
> +    RISCVException ret;
> +
> +    ret = smstateen_acc_ok(env, PRV_S, SMSTATEEN0_HSENVCFG);
> +    if (ret != RISCV_EXCP_NONE) {
> +        return ret;
> +    }
>
>      if (riscv_cpu_mxl(env) == MXL_RV64) {
>          mask |= HENVCFG_PBMTE | HENVCFG_STCE;
> @@ -1595,6 +1651,13 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
>  static RISCVException read_henvcfgh(CPURISCVState *env, int csrno,
>                                   target_ulong *val)
>  {
> +    RISCVException ret;
> +
> +    ret = smstateen_acc_ok(env, PRV_S, SMSTATEEN0_HSENVCFG);
> +    if (ret != RISCV_EXCP_NONE) {
> +        return ret;
> +    }
> +
>      *val = env->henvcfg >> 32;
>      return RISCV_EXCP_NONE;
>  }
> @@ -1604,9 +1667,14 @@ static RISCVException write_henvcfgh(CPURISCVState *env, int csrno,
>  {
>      uint64_t mask = HENVCFG_PBMTE | HENVCFG_STCE;
>      uint64_t valh = (uint64_t)val << 32;
> +    RISCVException ret;
>
> -    env->henvcfg = (env->henvcfg & ~mask) | (valh & mask);
> +    ret = smstateen_acc_ok(env, PRV_S, SMSTATEEN0_HSENVCFG);
> +    if (ret != RISCV_EXCP_NONE) {
> +        return ret;
> +    }
>
> +    env->henvcfg = (env->henvcfg & ~mask) | (valh & mask);
>      return RISCV_EXCP_NONE;
>  }
>
> @@ -1628,7 +1696,8 @@ static RISCVException write_mstateen(CPURISCVState *env, int csrno,
>                                       target_ulong new_val)
>  {
>      uint64_t *reg;
> -    uint64_t wr_mask = 1UL << SMSTATEEN_STATEN;
> +    uint64_t wr_mask = (1UL << SMSTATEEN_STATEN) |
> +                       (1UL << SMSTATEEN0_HSENVCFG);
>
>      reg = &env->mstateen[csrno - CSR_MSTATEEN0];
>      write_smstateen(env, reg, wr_mask, new_val);
> @@ -1649,7 +1718,8 @@ static RISCVException write_mstateenh(CPURISCVState *env, int csrno,
>  {
>      uint64_t *reg;
>      uint64_t val;
> -    uint64_t wr_mask = 1UL << SMSTATEEN_STATEN;
> +    uint64_t wr_mask = (1UL << SMSTATEEN_STATEN) |
> +                       (1UL << SMSTATEEN0_HSENVCFG);
>
>      reg = &env->mstateen[csrno - CSR_MSTATEEN0H];
>      val = (uint64_t)new_val << 32;
> @@ -1671,7 +1741,8 @@ static RISCVException write_hstateen(CPURISCVState *env, int csrno,
>                                       target_ulong new_val)
>  {
>      uint64_t *reg;
> -    uint64_t wr_mask = 1UL << SMSTATEEN_STATEN;
> +    uint64_t wr_mask = (1UL << SMSTATEEN_STATEN) |
> +                       (1UL << SMSTATEEN0_HSENVCFG);
>      int index = csrno - CSR_HSTATEEN0;
>
>      reg = &env->hstateen[index];
> @@ -1694,8 +1765,9 @@ static RISCVException write_hstateenh(CPURISCVState *env, int csrno,
>  {
>      uint64_t *reg;
>      uint64_t val;
> -    uint64_t wr_mask = 1UL << SMSTATEEN_STATEN;
>      int index = csrno - CSR_HSTATEEN0H;
> +    uint64_t wr_mask = (1UL << SMSTATEEN_STATEN) |
> +                       (1UL << SMSTATEEN0_HSENVCFG);
>
>      reg = &env->hstateen[index];
>      val = (uint64_t)new_val << 32;
> --
> 2.25.1
>
>
Re: [RFC PATCH v5 2/4] target/riscv: smstateen check for h/senvcfg
Posted by Alistair Francis 2 years, 4 months ago 
On Thu, Jun 16, 2022 at 4:54 PM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Sat, Jun 4, 2022 at 2:16 AM Mayuresh Chitale
> <mchitale@ventanamicro.com> wrote:
> >
> > Accesses to henvcfg, henvcfgh and senvcfg are allowed
> > only if corresponding bit in mstateen0/hstateen0 is
> > enabled. Otherwise an illegal instruction trap is
> > generated.
> >
> > Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> > ---
> >  target/riscv/csr.c | 84 ++++++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 78 insertions(+), 6 deletions(-)
> >
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index 324fefce59..ae91ae1f7e 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -39,6 +39,37 @@ void riscv_set_csr_ops(int csrno, riscv_csr_operations *ops)
> >  }
> >
> >  /* Predicates */
> > +static RISCVException smstateen_acc_ok(CPURISCVState *env, int mode, int bit)
> > +{
> > +    CPUState *cs = env_cpu(env);
> > +    RISCVCPU *cpu = RISCV_CPU(cs);
> > +    bool virt = riscv_cpu_virt_enabled(env);
> > +
> > +    if (!cpu->cfg.ext_smstateen) {
> > +        return RISCV_EXCP_NONE;
> > +    }
> > +
> > +#if !defined(CONFIG_USER_ONLY)
> > +    if (!(env->mstateen[0] & 1UL << bit)) {
> > +        return RISCV_EXCP_ILLEGAL_INST;
> > +    }
> > +
> > +    if (virt) {
> > +        if (!(env->hstateen[0] & 1UL << bit)) {
> > +            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> > +        }
> > +    }
> > +
> > +    if (mode == PRV_U) {
> > +        if (!(env->sstateen[0] & 1UL << bit)) {
> > +            return RISCV_EXCP_ILLEGAL_INST;
> > +        }
> > +    }
> > +#endif
> > +
> > +    return RISCV_EXCP_NONE;
> > +}
> > +
> >  static RISCVException fs(CPURISCVState *env, int csrno)
> >  {
> >  #if !defined(CONFIG_USER_ONLY)
> > @@ -1557,6 +1588,13 @@ static RISCVException write_menvcfgh(CPURISCVState *env, int csrno,
> >  static RISCVException read_senvcfg(CPURISCVState *env, int csrno,
> >                                   target_ulong *val)
> >  {
> > +    RISCVException ret;
> > +
> > +    ret = smstateen_acc_ok(env, PRV_S, SMSTATEEN0_HSENVCFG);
>
> Couldn't this be part of the original permission check so we don't
> need a second check?

Whoops, misread the function. Ignore that

Alistair

>
> Alistair
>
> > +    if (ret != RISCV_EXCP_NONE) {
> > +        return ret;
> > +    }
> > +
> >      *val = env->senvcfg;
> >      return RISCV_EXCP_NONE;
> >  }
> > @@ -1565,15 +1603,27 @@ static RISCVException write_senvcfg(CPURISCVState *env, int csrno,
> >                                    target_ulong val)
> >  {
> >      uint64_t mask = SENVCFG_FIOM | SENVCFG_CBIE | SENVCFG_CBCFE | SENVCFG_CBZE;
> > +    RISCVException ret;
> >
> > -    env->senvcfg = (env->senvcfg & ~mask) | (val & mask);
> > +    ret = smstateen_acc_ok(env, PRV_S, SMSTATEEN0_HSENVCFG);
> > +    if (ret != RISCV_EXCP_NONE) {
> > +        return ret;
> > +    }
> >
> > +    env->senvcfg = (env->senvcfg & ~mask) | (val & mask);
> >      return RISCV_EXCP_NONE;
> >  }
> >
> >  static RISCVException read_henvcfg(CPURISCVState *env, int csrno,
> >                                   target_ulong *val)
> >  {
> > +    RISCVException ret;
> > +
> > +    ret = smstateen_acc_ok(env, PRV_S, SMSTATEEN0_HSENVCFG);
> > +    if (ret != RISCV_EXCP_NONE) {
> > +        return ret;
> > +    }
> > +
> >      *val = env->henvcfg;
> >      return RISCV_EXCP_NONE;
> >  }
> > @@ -1582,6 +1632,12 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
> >                                    target_ulong val)
> >  {
> >      uint64_t mask = HENVCFG_FIOM | HENVCFG_CBIE | HENVCFG_CBCFE | HENVCFG_CBZE;
> > +    RISCVException ret;
> > +
> > +    ret = smstateen_acc_ok(env, PRV_S, SMSTATEEN0_HSENVCFG);
> > +    if (ret != RISCV_EXCP_NONE) {
> > +        return ret;
> > +    }
> >
> >      if (riscv_cpu_mxl(env) == MXL_RV64) {
> >          mask |= HENVCFG_PBMTE | HENVCFG_STCE;
> > @@ -1595,6 +1651,13 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
> >  static RISCVException read_henvcfgh(CPURISCVState *env, int csrno,
> >                                   target_ulong *val)
> >  {
> > +    RISCVException ret;
> > +
> > +    ret = smstateen_acc_ok(env, PRV_S, SMSTATEEN0_HSENVCFG);
> > +    if (ret != RISCV_EXCP_NONE) {
> > +        return ret;
> > +    }
> > +
> >      *val = env->henvcfg >> 32;
> >      return RISCV_EXCP_NONE;
> >  }
> > @@ -1604,9 +1667,14 @@ static RISCVException write_henvcfgh(CPURISCVState *env, int csrno,
> >  {
> >      uint64_t mask = HENVCFG_PBMTE | HENVCFG_STCE;
> >      uint64_t valh = (uint64_t)val << 32;
> > +    RISCVException ret;
> >
> > -    env->henvcfg = (env->henvcfg & ~mask) | (valh & mask);
> > +    ret = smstateen_acc_ok(env, PRV_S, SMSTATEEN0_HSENVCFG);
> > +    if (ret != RISCV_EXCP_NONE) {
> > +        return ret;
> > +    }
> >
> > +    env->henvcfg = (env->henvcfg & ~mask) | (valh & mask);
> >      return RISCV_EXCP_NONE;
> >  }
> >
> > @@ -1628,7 +1696,8 @@ static RISCVException write_mstateen(CPURISCVState *env, int csrno,
> >                                       target_ulong new_val)
> >  {
> >      uint64_t *reg;
> > -    uint64_t wr_mask = 1UL << SMSTATEEN_STATEN;
> > +    uint64_t wr_mask = (1UL << SMSTATEEN_STATEN) |
> > +                       (1UL << SMSTATEEN0_HSENVCFG);
> >
> >      reg = &env->mstateen[csrno - CSR_MSTATEEN0];
> >      write_smstateen(env, reg, wr_mask, new_val);
> > @@ -1649,7 +1718,8 @@ static RISCVException write_mstateenh(CPURISCVState *env, int csrno,
> >  {
> >      uint64_t *reg;
> >      uint64_t val;
> > -    uint64_t wr_mask = 1UL << SMSTATEEN_STATEN;
> > +    uint64_t wr_mask = (1UL << SMSTATEEN_STATEN) |
> > +                       (1UL << SMSTATEEN0_HSENVCFG);
> >
> >      reg = &env->mstateen[csrno - CSR_MSTATEEN0H];
> >      val = (uint64_t)new_val << 32;
> > @@ -1671,7 +1741,8 @@ static RISCVException write_hstateen(CPURISCVState *env, int csrno,
> >                                       target_ulong new_val)
> >  {
> >      uint64_t *reg;
> > -    uint64_t wr_mask = 1UL << SMSTATEEN_STATEN;
> > +    uint64_t wr_mask = (1UL << SMSTATEEN_STATEN) |
> > +                       (1UL << SMSTATEEN0_HSENVCFG);
> >      int index = csrno - CSR_HSTATEEN0;
> >
> >      reg = &env->hstateen[index];
> > @@ -1694,8 +1765,9 @@ static RISCVException write_hstateenh(CPURISCVState *env, int csrno,
> >  {
> >      uint64_t *reg;
> >      uint64_t val;
> > -    uint64_t wr_mask = 1UL << SMSTATEEN_STATEN;
> >      int index = csrno - CSR_HSTATEEN0H;
> > +    uint64_t wr_mask = (1UL << SMSTATEEN_STATEN) |
> > +                       (1UL << SMSTATEEN0_HSENVCFG);
> >
> >      reg = &env->hstateen[index];
> >      val = (uint64_t)new_val << 32;
> > --
> > 2.25.1
> >
> >

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.