Re: [PATCH v1 04/28] target/riscv: Fix CSR perm checking for HS mode

From: Alistair Francis <alistair23@gmail.com>
To: Palmer Dabbelt <palmer@sifive.com>
Cc: "open list:RISC-V" <qemu-riscv@nongnu.org>,
	Anup Patel <Anup.Patel@wdc.com>,
	Alistair Francis <Alistair.Francis@wdc.com>,
	"qemu-devel@nongnu.org Developers" <qemu-devel@nongnu.org>,
	Atish Patra <Atish.Patra@wdc.com>
Subject: Re: [PATCH v1 04/28] target/riscv: Fix CSR perm checking for HS mode
Date: Wed, 16 Oct 2019 13:56:04 -0700	[thread overview]
Message-ID: <CAKmqyKMf1bPSpM7RP1ULmOK7sKcUFA5Rbb_TkD7f0vTtu5fa1Q@mail.gmail.com> (raw)
In-Reply-To: <mhng-afa69c9b-17b7-4043-b204-512c92cf618a@palmer-si-x1e>

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