| Message ID | a090bc437bf412c279b1254d05eae5c2d67225db.1566603412.git.alistair.francis@wdc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add RISC-V Hypervisor Extension v0.4 | expand |
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
On Tue, Sep 10, 2019 at 7:48 AM Palmer Dabbelt <palmer@sifive.com> wrote: > > 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. I changed the name to effective_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 */ > > 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? Yes, you are correct. I have changed it to check that we are in S mode. > > 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; I don't like this as there is no PRIV_HS. It seems like a bad idea to start using a reserved privilege level, if it is ever used we will then be stuck updating this. I also don't think this is used anywhere else. I have just fixed up the if statement and comment. Alistair > } > > 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
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
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(-)