[v1,04/28] target/riscv: Fix CSR perm checking for HS mode
diff mbox series

Message ID a090bc437bf412c279b1254d05eae5c2d67225db.1566603412.git.alistair.francis@wdc.com
State New
Headers show
Series
  • Add RISC-V Hypervisor Extension v0.4
Related show

Commit Message

Alistair Francis Aug. 23, 2019, 11:38 p.m. UTC
Update the CSR permission checking to work correctly when we are in
HS-mode.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/csr.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Palmer Dabbelt Sept. 10, 2019, 2:48 p.m. UTC | #1
On Fri, 23 Aug 2019 16:38:00 PDT (-0700), Alistair Francis wrote:
> Update the CSR permission checking to work correctly when we are in
> HS-mode.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  target/riscv/csr.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index f767ad24be..471f23a1d0 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -799,9 +799,15 @@ int riscv_csrrw(CPURISCVState *env, int csrno, target_ulong *ret_value,
>
>      /* check privileges and return -1 if check fails */
>  #if !defined(CONFIG_USER_ONLY)
> -    int csr_priv = get_field(csrno, 0x300);
> +    int csr_priv = env->priv;

This isn't really "csr_priv" (ie, the priv needed to access the CSR) any more, 
it's really the effective priv of the machine.  Leaving the variable with the 
same name makes this hard to read, but I think it is correct.

>      int read_only = get_field(csrno, 0xC00) == 3;
> -    if ((write_mask && read_only) || (env->priv < csr_priv)) {
> +
> +    if (riscv_has_ext(env, RVH) && !riscv_cpu_virt_enabled(env)) {
> +        /* Plus 1 as we are in HS mode */

The comment is useless, it doesn't say why we increment it.  Also, I don't 
think this is correct: doesn't it allow U mode to access S CSRs when H is 
present and V is disabled?

Something like

    riscv_effective_priv(CPURISCVState *env)
    {
        if (riscv_has_ext(env, RVH) && env->priv == PRIV_S && !riscv_cpu_virt_enabled(env)) {
            return PRIV_HS;
        }
    
        return env->priv;
    }

would probably be used in a handful of places, and would be a drop in for
env->priv here.

> +        csr_priv++;
> +    }
> +
> +    if ((write_mask && read_only) || (csr_priv < get_field(csrno, 0x300))) {
>          return -1;
>      }
>  #endif

Patch
diff mbox series

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index f767ad24be..471f23a1d0 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -799,9 +799,15 @@  int riscv_csrrw(CPURISCVState *env, int csrno, target_ulong *ret_value,
 
     /* check privileges and return -1 if check fails */
 #if !defined(CONFIG_USER_ONLY)
-    int csr_priv = get_field(csrno, 0x300);
+    int csr_priv = env->priv;
     int read_only = get_field(csrno, 0xC00) == 3;
-    if ((write_mask && read_only) || (env->priv < csr_priv)) {
+
+    if (riscv_has_ext(env, RVH) && !riscv_cpu_virt_enabled(env)) {
+        /* Plus 1 as we are in HS mode */
+        csr_priv++;
+    }
+
+    if ((write_mask && read_only) || (csr_priv < get_field(csrno, 0x300))) {
         return -1;
     }
 #endif