[PATCH 1/2] target/riscv: Fix up masking of vsip/vsie accesses

[PATCH 1/2] target/riscv: Fix up masking of vsip/vsie accesses

[Andrew Bresticker]

The current logic attempts to shift the VS-level bits into their correct
position in mip while leaving the remaining bits in-tact. This is both
pointless and likely incorrect since one would expect that any new, future
VS-level interrupts will get their own position in mip rather than sharing
with their (H)S-level equivalent. Fix this, and make the logic more
readable, by just making off the VS-level bits and shifting them into
position.

This also fixes reads of vsip, which would only ever report vsip.VSSIP
since the non-writable bits got masked off as well.

Fixes: d028ac7512f1 ("arget/riscv: Implement AIA CSRs for 64 local interrupts on RV32")
Signed-off-by: Andrew Bresticker <abrestic@rivosinc.com>
---
 target/riscv/csr.c | 35 +++++++++++------------------------
 1 file changed, 11 insertions(+), 24 deletions(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 5c9a7ee287..984548bf87 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -1975,22 +1975,15 @@ static RISCVException rmw_vsie64(CPURISCVState *env, int csrno,
                                  uint64_t new_val, uint64_t wr_mask)
 {
     RISCVException ret;
-    uint64_t rval, vsbits, mask = env->hideleg & VS_MODE_INTERRUPTS;
+    uint64_t rval, mask = env->hideleg & VS_MODE_INTERRUPTS;
 
     /* Bring VS-level bits to correct position */
-    vsbits = new_val & (VS_MODE_INTERRUPTS >> 1);
-    new_val &= ~(VS_MODE_INTERRUPTS >> 1);
-    new_val |= vsbits << 1;
-    vsbits = wr_mask & (VS_MODE_INTERRUPTS >> 1);
-    wr_mask &= ~(VS_MODE_INTERRUPTS >> 1);
-    wr_mask |= vsbits << 1;
+    new_val = (new_val & (VS_MODE_INTERRUPTS >> 1)) << 1;
+    wr_mask = (wr_mask & (VS_MODE_INTERRUPTS >> 1)) << 1;
 
     ret = rmw_mie64(env, csrno, &rval, new_val, wr_mask & mask);
     if (ret_val) {
-        rval &= mask;
-        vsbits = rval & VS_MODE_INTERRUPTS;
-        rval &= ~VS_MODE_INTERRUPTS;
-        *ret_val = rval | (vsbits >> 1);
+        *ret_val = (rval & mask) >> 1;
     }
 
     return ret;
@@ -2191,22 +2184,16 @@ static RISCVException rmw_vsip64(CPURISCVState *env, int csrno,
                                  uint64_t new_val, uint64_t wr_mask)
 {
     RISCVException ret;
-    uint64_t rval, vsbits, mask = env->hideleg & vsip_writable_mask;
+    uint64_t rval, mask = env->hideleg & VS_MODE_INTERRUPTS;
 
     /* Bring VS-level bits to correct position */
-    vsbits = new_val & (VS_MODE_INTERRUPTS >> 1);
-    new_val &= ~(VS_MODE_INTERRUPTS >> 1);
-    new_val |= vsbits << 1;
-    vsbits = wr_mask & (VS_MODE_INTERRUPTS >> 1);
-    wr_mask &= ~(VS_MODE_INTERRUPTS >> 1);
-    wr_mask |= vsbits << 1;
-
-    ret = rmw_mip64(env, csrno, &rval, new_val, wr_mask & mask);
+    new_val = (new_val & (VS_MODE_INTERRUPTS >> 1)) << 1;
+    wr_mask = (wr_mask & (VS_MODE_INTERRUPTS >> 1)) << 1;
+
+    ret = rmw_mip64(env, csrno, &rval, new_val,
+                    wr_mask & mask & vsip_writable_mask);
     if (ret_val) {
-        rval &= mask;
-        vsbits = rval & VS_MODE_INTERRUPTS;
-        rval &= ~VS_MODE_INTERRUPTS;
-        *ret_val = rval | (vsbits >> 1);
+        *ret_val = (rval & mask) >> 1;
     }
 
     return ret;
-- 
2.25.1

[PATCH 2/2] target/riscv: Trap on writes to stimecmp from VS when hvictl.VTI=1

[Andrew Bresticker]

Per the AIA specification, writes to stimecmp from VS level should
trap when hvictl.VTI is set since the write may cause vsip.STIP to
become unset.

Fixes: 3ec0fe18a31f ("target/riscv: Add vstimecmp support")
Signed-off-by: Andrew Bresticker <abrestic@rivosinc.com>
---
 target/riscv/csr.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 984548bf87..7d9035e7bb 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -935,6 +935,9 @@ static RISCVException write_stimecmp(CPURISCVState *env, int csrno,
     RISCVCPU *cpu = env_archcpu(env);
 
     if (riscv_cpu_virt_enabled(env)) {
+        if (env->hvictl & HVICTL_VTI) {
+            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
+        }
         return write_vstimecmp(env, csrno, val);
     }
 
@@ -955,6 +958,9 @@ static RISCVException write_stimecmph(CPURISCVState *env, int csrno,
     RISCVCPU *cpu = env_archcpu(env);
 
     if (riscv_cpu_virt_enabled(env)) {
+        if (env->hvictl & HVICTL_VTI) {
+            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
+        }
         return write_vstimecmph(env, csrno, val);
     }
 
-- 
2.25.1

Re: [PATCH 1/2] target/riscv: Fix up masking of vsip/vsie accesses

[Alistair Francis]

On Fri, Dec 16, 2022 at 8:46 AM Andrew Bresticker <abrestic@rivosinc.com> wrote:
>
> The current logic attempts to shift the VS-level bits into their correct
> position in mip while leaving the remaining bits in-tact. This is both
> pointless and likely incorrect since one would expect that any new, future
> VS-level interrupts will get their own position in mip rather than sharing
> with their (H)S-level equivalent. Fix this, and make the logic more
> readable, by just making off the VS-level bits and shifting them into
> position.
>
> This also fixes reads of vsip, which would only ever report vsip.VSSIP
> since the non-writable bits got masked off as well.
>
> Fixes: d028ac7512f1 ("arget/riscv: Implement AIA CSRs for 64 local interrupts on RV32")
> Signed-off-by: Andrew Bresticker <abrestic@rivosinc.com>

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

Alistair

> ---
>  target/riscv/csr.c | 35 +++++++++++------------------------
>  1 file changed, 11 insertions(+), 24 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 5c9a7ee287..984548bf87 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -1975,22 +1975,15 @@ static RISCVException rmw_vsie64(CPURISCVState *env, int csrno,
>                                   uint64_t new_val, uint64_t wr_mask)
>  {
>      RISCVException ret;
> -    uint64_t rval, vsbits, mask = env->hideleg & VS_MODE_INTERRUPTS;
> +    uint64_t rval, mask = env->hideleg & VS_MODE_INTERRUPTS;
>
>      /* Bring VS-level bits to correct position */
> -    vsbits = new_val & (VS_MODE_INTERRUPTS >> 1);
> -    new_val &= ~(VS_MODE_INTERRUPTS >> 1);
> -    new_val |= vsbits << 1;
> -    vsbits = wr_mask & (VS_MODE_INTERRUPTS >> 1);
> -    wr_mask &= ~(VS_MODE_INTERRUPTS >> 1);
> -    wr_mask |= vsbits << 1;
> +    new_val = (new_val & (VS_MODE_INTERRUPTS >> 1)) << 1;
> +    wr_mask = (wr_mask & (VS_MODE_INTERRUPTS >> 1)) << 1;
>
>      ret = rmw_mie64(env, csrno, &rval, new_val, wr_mask & mask);
>      if (ret_val) {
> -        rval &= mask;
> -        vsbits = rval & VS_MODE_INTERRUPTS;
> -        rval &= ~VS_MODE_INTERRUPTS;
> -        *ret_val = rval | (vsbits >> 1);
> +        *ret_val = (rval & mask) >> 1;
>      }
>
>      return ret;
> @@ -2191,22 +2184,16 @@ static RISCVException rmw_vsip64(CPURISCVState *env, int csrno,
>                                   uint64_t new_val, uint64_t wr_mask)
>  {
>      RISCVException ret;
> -    uint64_t rval, vsbits, mask = env->hideleg & vsip_writable_mask;
> +    uint64_t rval, mask = env->hideleg & VS_MODE_INTERRUPTS;
>
>      /* Bring VS-level bits to correct position */
> -    vsbits = new_val & (VS_MODE_INTERRUPTS >> 1);
> -    new_val &= ~(VS_MODE_INTERRUPTS >> 1);
> -    new_val |= vsbits << 1;
> -    vsbits = wr_mask & (VS_MODE_INTERRUPTS >> 1);
> -    wr_mask &= ~(VS_MODE_INTERRUPTS >> 1);
> -    wr_mask |= vsbits << 1;
> -
> -    ret = rmw_mip64(env, csrno, &rval, new_val, wr_mask & mask);
> +    new_val = (new_val & (VS_MODE_INTERRUPTS >> 1)) << 1;
> +    wr_mask = (wr_mask & (VS_MODE_INTERRUPTS >> 1)) << 1;
> +
> +    ret = rmw_mip64(env, csrno, &rval, new_val,
> +                    wr_mask & mask & vsip_writable_mask);
>      if (ret_val) {
> -        rval &= mask;
> -        vsbits = rval & VS_MODE_INTERRUPTS;
> -        rval &= ~VS_MODE_INTERRUPTS;
> -        *ret_val = rval | (vsbits >> 1);
> +        *ret_val = (rval & mask) >> 1;
>      }
>
>      return ret;
> --
> 2.25.1
>
>

Re: [PATCH 2/2] target/riscv: Trap on writes to stimecmp from VS when hvictl.VTI=1

[Alistair Francis]

On Fri, Dec 16, 2022 at 8:46 AM Andrew Bresticker <abrestic@rivosinc.com> wrote:
>
> Per the AIA specification, writes to stimecmp from VS level should
> trap when hvictl.VTI is set since the write may cause vsip.STIP to
> become unset.
>
> Fixes: 3ec0fe18a31f ("target/riscv: Add vstimecmp support")
> Signed-off-by: Andrew Bresticker <abrestic@rivosinc.com>

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

Alistair

> ---
>  target/riscv/csr.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 984548bf87..7d9035e7bb 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -935,6 +935,9 @@ static RISCVException write_stimecmp(CPURISCVState *env, int csrno,
>      RISCVCPU *cpu = env_archcpu(env);
>
>      if (riscv_cpu_virt_enabled(env)) {
> +        if (env->hvictl & HVICTL_VTI) {
> +            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> +        }
>          return write_vstimecmp(env, csrno, val);
>      }
>
> @@ -955,6 +958,9 @@ static RISCVException write_stimecmph(CPURISCVState *env, int csrno,
>      RISCVCPU *cpu = env_archcpu(env);
>
>      if (riscv_cpu_virt_enabled(env)) {
> +        if (env->hvictl & HVICTL_VTI) {
> +            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> +        }
>          return write_vstimecmph(env, csrno, val);
>      }
>
> --
> 2.25.1
>
>

Re: [PATCH 2/2] target/riscv: Trap on writes to stimecmp from VS when hvictl.VTI=1

[Alistair Francis]

On Fri, Dec 16, 2022 at 8:46 AM Andrew Bresticker <abrestic@rivosinc.com> wrote:
>
> Per the AIA specification, writes to stimecmp from VS level should
> trap when hvictl.VTI is set since the write may cause vsip.STIP to
> become unset.
>
> Fixes: 3ec0fe18a31f ("target/riscv: Add vstimecmp support")
> Signed-off-by: Andrew Bresticker <abrestic@rivosinc.com>

Thanks!

Applied to riscv-to-apply.next

Alistair

> ---
>  target/riscv/csr.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 984548bf87..7d9035e7bb 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -935,6 +935,9 @@ static RISCVException write_stimecmp(CPURISCVState *env, int csrno,
>      RISCVCPU *cpu = env_archcpu(env);
>
>      if (riscv_cpu_virt_enabled(env)) {
> +        if (env->hvictl & HVICTL_VTI) {
> +            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> +        }
>          return write_vstimecmp(env, csrno, val);
>      }
>
> @@ -955,6 +958,9 @@ static RISCVException write_stimecmph(CPURISCVState *env, int csrno,
>      RISCVCPU *cpu = env_archcpu(env);
>
>      if (riscv_cpu_virt_enabled(env)) {
> +        if (env->hvictl & HVICTL_VTI) {
> +            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> +        }
>          return write_vstimecmph(env, csrno, val);
>      }
>
> --
> 2.25.1
>
>