1. Patchew
  2. QEMU
  3. View series

[PATCH] target/riscv: Fix read and write accesses to vsip and vsie

Georg Kotheimer posted 1 patch 3 years, 1 month ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20210311094738.1376795-1-georg.kotheimer@kernkonzept.com
Maintainers: Sagar Karandikar <sagark@eecs.berkeley.edu>, Alistair Francis <Alistair.Francis@wdc.com>, Bastian Koppelmann <kbastian@mail.uni-paderborn.de>, Palmer Dabbelt <palmer@dabbelt.com>
target/riscv/csr.c | 68 +++++++++++++++++++++++-----------------------
1 file changed, 34 insertions(+), 34 deletions(-)
[PATCH] target/riscv: Fix read and write accesses to vsip and vsie
Posted by Georg Kotheimer 3 years, 1 month ago 
The previous implementation was broken in many ways:
 - Used mideleg instead of hideleg to mask accesses
 - Used MIP_VSSIP instead of VS_MODE_INTERRUPTS to mask writes to vsie
 - Did not shift between S bits and VS bits (VSEIP <-> SEIP, ...)

Signed-off-by: Georg Kotheimer <georg.kotheimer@kernkonzept.com>
---
 target/riscv/csr.c | 68 +++++++++++++++++++++++-----------------------
 1 file changed, 34 insertions(+), 34 deletions(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index fd2e6363f3..4a5b362ec1 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -748,30 +748,42 @@ static int write_sstatus(CPURISCVState *env, int csrno, target_ulong val)
     return write_mstatus(env, CSR_MSTATUS, newval);
 }
 
+static int read_vsie(CPURISCVState *env, int csrno, target_ulong *val)
+{
+    /* Shift the VS bits to their S bit location in vsie */
+    *val = (env->mie & env->hideleg & VS_MODE_INTERRUPTS) >> 1;
+    return 0;
+}
+
 static int read_sie(CPURISCVState *env, int csrno, target_ulong *val)
 {
     if (riscv_cpu_virt_enabled(env)) {
-        /* Tell the guest the VS bits, shifted to the S bit locations */
-        *val = (env->mie & env->mideleg & VS_MODE_INTERRUPTS) >> 1;
+        read_vsie(env, CSR_VSIE, val);
     } else {
         *val = env->mie & env->mideleg;
     }
     return 0;
 }
 
-static int write_sie(CPURISCVState *env, int csrno, target_ulong val)
+static int write_vsie(CPURISCVState *env, int csrno, target_ulong val)
 {
-    target_ulong newval;
+    /* Shift the S bits to their VS bit location in mie */
+    target_ulong newval = (env->mie & ~VS_MODE_INTERRUPTS) |
+                          ((val << 1) & env->hideleg & VS_MODE_INTERRUPTS);
+    return write_mie(env, CSR_MIE, newval);
+}
 
+static int write_sie(CPURISCVState *env, int csrno, target_ulong val)
+{
     if (riscv_cpu_virt_enabled(env)) {
-        /* Shift the guests S bits to VS */
-        newval = (env->mie & ~VS_MODE_INTERRUPTS) |
-                 ((val << 1) & VS_MODE_INTERRUPTS);
+        write_vsie(env, CSR_VSIE, val);
     } else {
-        newval = (env->mie & ~S_MODE_INTERRUPTS) | (val & S_MODE_INTERRUPTS);
+        target_ulong newval = (env->mie & ~S_MODE_INTERRUPTS) |
+                              (val & S_MODE_INTERRUPTS);
+        write_mie(env, CSR_MIE, newval);
     }
 
-    return write_mie(env, CSR_MIE, newval);
+    return 0;
 }
 
 static int read_stvec(CPURISCVState *env, int csrno, target_ulong *val)
@@ -852,17 +864,25 @@ static int write_sbadaddr(CPURISCVState *env, int csrno, target_ulong val)
     return 0;
 }
 
+static int rmw_vsip(CPURISCVState *env, int csrno, target_ulong *ret_value,
+                    target_ulong new_value, target_ulong write_mask)
+{
+    /* Shift the S bits to their VS bit location in mip */
+    int ret = rmw_mip(env, 0, ret_value, new_value << 1,
+                      (write_mask << 1) & vsip_writable_mask & env->hideleg);
+    *ret_value &= VS_MODE_INTERRUPTS;
+    /* Shift the VS bits to their S bit location in vsip */
+    *ret_value >>= 1;
+    return ret;
+}
+
 static int rmw_sip(CPURISCVState *env, int csrno, target_ulong *ret_value,
                    target_ulong new_value, target_ulong write_mask)
 {
     int ret;
 
     if (riscv_cpu_virt_enabled(env)) {
-        /* Shift the new values to line up with the VS bits */
-        ret = rmw_mip(env, CSR_MSTATUS, ret_value, new_value << 1,
-                      (write_mask & sip_writable_mask) << 1 & env->mideleg);
-        ret &= vsip_writable_mask;
-        ret >>= 1;
+        ret = rmw_vsip(env, CSR_VSIP, ret_value, new_value, write_mask);
     } else {
         ret = rmw_mip(env, CSR_MSTATUS, ret_value, new_value,
                       write_mask & env->mideleg & sip_writable_mask);
@@ -1121,26 +1141,6 @@ static int write_vsstatus(CPURISCVState *env, int csrno, target_ulong val)
     return 0;
 }
 
-static int rmw_vsip(CPURISCVState *env, int csrno, target_ulong *ret_value,
-                    target_ulong new_value, target_ulong write_mask)
-{
-    int ret = rmw_mip(env, 0, ret_value, new_value,
-                      write_mask & env->mideleg & vsip_writable_mask);
-    return ret;
-}
-
-static int read_vsie(CPURISCVState *env, int csrno, target_ulong *val)
-{
-    *val = env->mie & env->mideleg & VS_MODE_INTERRUPTS;
-    return 0;
-}
-
-static int write_vsie(CPURISCVState *env, int csrno, target_ulong val)
-{
-    target_ulong newval = (env->mie & ~env->mideleg) | (val & env->mideleg & MIP_VSSIP);
-    return write_mie(env, CSR_MIE, newval);
-}
-
 static int read_vstvec(CPURISCVState *env, int csrno, target_ulong *val)
 {
     *val = env->vstvec;
-- 
2.30.1
Re: [PATCH] target/riscv: Fix read and write accesses to vsip and vsie
Posted by Alistair Francis 3 years, 1 month ago 
On Thu, Mar 11, 2021 at 4:49 AM Georg Kotheimer
<georg.kotheimer@kernkonzept.com> wrote:
>
> The previous implementation was broken in many ways:
>  - Used mideleg instead of hideleg to mask accesses
>  - Used MIP_VSSIP instead of VS_MODE_INTERRUPTS to mask writes to vsie
>  - Did not shift between S bits and VS bits (VSEIP <-> SEIP, ...)
>
> Signed-off-by: Georg Kotheimer <georg.kotheimer@kernkonzept.com>

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

Alistair

> ---
>  target/riscv/csr.c | 68 +++++++++++++++++++++++-----------------------
>  1 file changed, 34 insertions(+), 34 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index fd2e6363f3..4a5b362ec1 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -748,30 +748,42 @@ static int write_sstatus(CPURISCVState *env, int csrno, target_ulong val)
>      return write_mstatus(env, CSR_MSTATUS, newval);
>  }
>
> +static int read_vsie(CPURISCVState *env, int csrno, target_ulong *val)
> +{
> +    /* Shift the VS bits to their S bit location in vsie */
> +    *val = (env->mie & env->hideleg & VS_MODE_INTERRUPTS) >> 1;
> +    return 0;
> +}
> +
>  static int read_sie(CPURISCVState *env, int csrno, target_ulong *val)
>  {
>      if (riscv_cpu_virt_enabled(env)) {
> -        /* Tell the guest the VS bits, shifted to the S bit locations */
> -        *val = (env->mie & env->mideleg & VS_MODE_INTERRUPTS) >> 1;
> +        read_vsie(env, CSR_VSIE, val);
>      } else {
>          *val = env->mie & env->mideleg;
>      }
>      return 0;
>  }
>
> -static int write_sie(CPURISCVState *env, int csrno, target_ulong val)
> +static int write_vsie(CPURISCVState *env, int csrno, target_ulong val)
>  {
> -    target_ulong newval;
> +    /* Shift the S bits to their VS bit location in mie */
> +    target_ulong newval = (env->mie & ~VS_MODE_INTERRUPTS) |
> +                          ((val << 1) & env->hideleg & VS_MODE_INTERRUPTS);
> +    return write_mie(env, CSR_MIE, newval);
> +}
>
> +static int write_sie(CPURISCVState *env, int csrno, target_ulong val)
> +{
>      if (riscv_cpu_virt_enabled(env)) {
> -        /* Shift the guests S bits to VS */
> -        newval = (env->mie & ~VS_MODE_INTERRUPTS) |
> -                 ((val << 1) & VS_MODE_INTERRUPTS);
> +        write_vsie(env, CSR_VSIE, val);
>      } else {
> -        newval = (env->mie & ~S_MODE_INTERRUPTS) | (val & S_MODE_INTERRUPTS);
> +        target_ulong newval = (env->mie & ~S_MODE_INTERRUPTS) |
> +                              (val & S_MODE_INTERRUPTS);
> +        write_mie(env, CSR_MIE, newval);
>      }
>
> -    return write_mie(env, CSR_MIE, newval);
> +    return 0;
>  }
>
>  static int read_stvec(CPURISCVState *env, int csrno, target_ulong *val)
> @@ -852,17 +864,25 @@ static int write_sbadaddr(CPURISCVState *env, int csrno, target_ulong val)
>      return 0;
>  }
>
> +static int rmw_vsip(CPURISCVState *env, int csrno, target_ulong *ret_value,
> +                    target_ulong new_value, target_ulong write_mask)
> +{
> +    /* Shift the S bits to their VS bit location in mip */
> +    int ret = rmw_mip(env, 0, ret_value, new_value << 1,
> +                      (write_mask << 1) & vsip_writable_mask & env->hideleg);
> +    *ret_value &= VS_MODE_INTERRUPTS;
> +    /* Shift the VS bits to their S bit location in vsip */
> +    *ret_value >>= 1;
> +    return ret;
> +}
> +
>  static int rmw_sip(CPURISCVState *env, int csrno, target_ulong *ret_value,
>                     target_ulong new_value, target_ulong write_mask)
>  {
>      int ret;
>
>      if (riscv_cpu_virt_enabled(env)) {
> -        /* Shift the new values to line up with the VS bits */
> -        ret = rmw_mip(env, CSR_MSTATUS, ret_value, new_value << 1,
> -                      (write_mask & sip_writable_mask) << 1 & env->mideleg);
> -        ret &= vsip_writable_mask;
> -        ret >>= 1;
> +        ret = rmw_vsip(env, CSR_VSIP, ret_value, new_value, write_mask);
>      } else {
>          ret = rmw_mip(env, CSR_MSTATUS, ret_value, new_value,
>                        write_mask & env->mideleg & sip_writable_mask);
> @@ -1121,26 +1141,6 @@ static int write_vsstatus(CPURISCVState *env, int csrno, target_ulong val)
>      return 0;
>  }
>
> -static int rmw_vsip(CPURISCVState *env, int csrno, target_ulong *ret_value,
> -                    target_ulong new_value, target_ulong write_mask)
> -{
> -    int ret = rmw_mip(env, 0, ret_value, new_value,
> -                      write_mask & env->mideleg & vsip_writable_mask);
> -    return ret;
> -}
> -
> -static int read_vsie(CPURISCVState *env, int csrno, target_ulong *val)
> -{
> -    *val = env->mie & env->mideleg & VS_MODE_INTERRUPTS;
> -    return 0;
> -}
> -
> -static int write_vsie(CPURISCVState *env, int csrno, target_ulong val)
> -{
> -    target_ulong newval = (env->mie & ~env->mideleg) | (val & env->mideleg & MIP_VSSIP);
> -    return write_mie(env, CSR_MIE, newval);
> -}
> -
>  static int read_vstvec(CPURISCVState *env, int csrno, target_ulong *val)
>  {
>      *val = env->vstvec;
> --
> 2.30.1
>
>
Re: [PATCH] target/riscv: Fix read and write accesses to vsip and vsie
Posted by Alistair Francis 3 years, 1 month ago 
On Thu, Mar 11, 2021 at 4:49 AM Georg Kotheimer
<georg.kotheimer@kernkonzept.com> wrote:
>
> The previous implementation was broken in many ways:
>  - Used mideleg instead of hideleg to mask accesses
>  - Used MIP_VSSIP instead of VS_MODE_INTERRUPTS to mask writes to vsie
>  - Did not shift between S bits and VS bits (VSEIP <-> SEIP, ...)
>
> Signed-off-by: Georg Kotheimer <georg.kotheimer@kernkonzept.com>

Thanks!

Applied to riscv-to-apply.next

Alistair

> ---
>  target/riscv/csr.c | 68 +++++++++++++++++++++++-----------------------
>  1 file changed, 34 insertions(+), 34 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index fd2e6363f3..4a5b362ec1 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -748,30 +748,42 @@ static int write_sstatus(CPURISCVState *env, int csrno, target_ulong val)
>      return write_mstatus(env, CSR_MSTATUS, newval);
>  }
>
> +static int read_vsie(CPURISCVState *env, int csrno, target_ulong *val)
> +{
> +    /* Shift the VS bits to their S bit location in vsie */
> +    *val = (env->mie & env->hideleg & VS_MODE_INTERRUPTS) >> 1;
> +    return 0;
> +}
> +
>  static int read_sie(CPURISCVState *env, int csrno, target_ulong *val)
>  {
>      if (riscv_cpu_virt_enabled(env)) {
> -        /* Tell the guest the VS bits, shifted to the S bit locations */
> -        *val = (env->mie & env->mideleg & VS_MODE_INTERRUPTS) >> 1;
> +        read_vsie(env, CSR_VSIE, val);
>      } else {
>          *val = env->mie & env->mideleg;
>      }
>      return 0;
>  }
>
> -static int write_sie(CPURISCVState *env, int csrno, target_ulong val)
> +static int write_vsie(CPURISCVState *env, int csrno, target_ulong val)
>  {
> -    target_ulong newval;
> +    /* Shift the S bits to their VS bit location in mie */
> +    target_ulong newval = (env->mie & ~VS_MODE_INTERRUPTS) |
> +                          ((val << 1) & env->hideleg & VS_MODE_INTERRUPTS);
> +    return write_mie(env, CSR_MIE, newval);
> +}
>
> +static int write_sie(CPURISCVState *env, int csrno, target_ulong val)
> +{
>      if (riscv_cpu_virt_enabled(env)) {
> -        /* Shift the guests S bits to VS */
> -        newval = (env->mie & ~VS_MODE_INTERRUPTS) |
> -                 ((val << 1) & VS_MODE_INTERRUPTS);
> +        write_vsie(env, CSR_VSIE, val);
>      } else {
> -        newval = (env->mie & ~S_MODE_INTERRUPTS) | (val & S_MODE_INTERRUPTS);
> +        target_ulong newval = (env->mie & ~S_MODE_INTERRUPTS) |
> +                              (val & S_MODE_INTERRUPTS);
> +        write_mie(env, CSR_MIE, newval);
>      }
>
> -    return write_mie(env, CSR_MIE, newval);
> +    return 0;
>  }
>
>  static int read_stvec(CPURISCVState *env, int csrno, target_ulong *val)
> @@ -852,17 +864,25 @@ static int write_sbadaddr(CPURISCVState *env, int csrno, target_ulong val)
>      return 0;
>  }
>
> +static int rmw_vsip(CPURISCVState *env, int csrno, target_ulong *ret_value,
> +                    target_ulong new_value, target_ulong write_mask)
> +{
> +    /* Shift the S bits to their VS bit location in mip */
> +    int ret = rmw_mip(env, 0, ret_value, new_value << 1,
> +                      (write_mask << 1) & vsip_writable_mask & env->hideleg);
> +    *ret_value &= VS_MODE_INTERRUPTS;
> +    /* Shift the VS bits to their S bit location in vsip */
> +    *ret_value >>= 1;
> +    return ret;
> +}
> +
>  static int rmw_sip(CPURISCVState *env, int csrno, target_ulong *ret_value,
>                     target_ulong new_value, target_ulong write_mask)
>  {
>      int ret;
>
>      if (riscv_cpu_virt_enabled(env)) {
> -        /* Shift the new values to line up with the VS bits */
> -        ret = rmw_mip(env, CSR_MSTATUS, ret_value, new_value << 1,
> -                      (write_mask & sip_writable_mask) << 1 & env->mideleg);
> -        ret &= vsip_writable_mask;
> -        ret >>= 1;
> +        ret = rmw_vsip(env, CSR_VSIP, ret_value, new_value, write_mask);
>      } else {
>          ret = rmw_mip(env, CSR_MSTATUS, ret_value, new_value,
>                        write_mask & env->mideleg & sip_writable_mask);
> @@ -1121,26 +1141,6 @@ static int write_vsstatus(CPURISCVState *env, int csrno, target_ulong val)
>      return 0;
>  }
>
> -static int rmw_vsip(CPURISCVState *env, int csrno, target_ulong *ret_value,
> -                    target_ulong new_value, target_ulong write_mask)
> -{
> -    int ret = rmw_mip(env, 0, ret_value, new_value,
> -                      write_mask & env->mideleg & vsip_writable_mask);
> -    return ret;
> -}
> -
> -static int read_vsie(CPURISCVState *env, int csrno, target_ulong *val)
> -{
> -    *val = env->mie & env->mideleg & VS_MODE_INTERRUPTS;
> -    return 0;
> -}
> -
> -static int write_vsie(CPURISCVState *env, int csrno, target_ulong val)
> -{
> -    target_ulong newval = (env->mie & ~env->mideleg) | (val & env->mideleg & MIP_VSSIP);
> -    return write_mie(env, CSR_MIE, newval);
> -}
> -
>  static int read_vstvec(CPURISCVState *env, int csrno, target_ulong *val)
>  {
>      *val = env->vstvec;
> --
> 2.30.1
>
>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.