[v1] target/riscv: Hardwire mcounter.TM and upper bits of [m|s]counteren

[Qemu-devel] [PATCH] target/riscv: Hardwire mcounter.TM and upper bits of [m|s]counteren

Posted by jonathan@fintelia.io 4 years, 10 months ago

From: Jonathan Behrens <jonathan@fintelia.io>

QEMU currently always triggers an illegal instruction exception when code
attempts to read the time CSR. This is valid behavor, but only if the TM bit in
mcounteren is hardwired to zero. This change also corrects mcounteren and scounteren CSRs to be 32-bits on both
32-bit and 64-bit targets.

Signed-off-by: Jonathan Behrens <jonathan@fintelia.io>
---
 target/riscv/cpu.h | 4 ++--
 target/riscv/csr.c | 3 ++-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 0adb307f32..2d0cbe9c78 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -151,8 +151,8 @@ struct CPURISCVState {
     target_ulong mcause;
     target_ulong mtval;  /* since: priv-1.10.0 */
 
-    target_ulong scounteren;
-    target_ulong mcounteren;
+    uint32_t scounteren;
+    uint32_t mcounteren;
 
     target_ulong sscratch;
     target_ulong mscratch;
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index e0d4586760..89cf9734c3 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -473,7 +473,8 @@ static int write_mcounteren(CPURISCVState *env, int csrno, target_ulong val)
     if (env->priv_ver < PRIV_VERSION_1_10_0) {
         return -1;
     }
-    env->mcounteren = val;
+    /* mcounteren.TM is hardwired to zero, all other bits are writable */
+    env->mcounteren = val & ~(1 << (CSR_TIME & 31));
     return 0;
 }
 
-- 
2.22.0

Re: [Qemu-devel] [PATCH] target/riscv: Hardwire mcounter.TM and upper bits of [m|s]counteren

Posted by Alistair Francis 4 years, 10 months ago

On Fri, Jun 28, 2019 at 1:12 PM <jonathan@fintelia.io> wrote:
>
> From: Jonathan Behrens <jonathan@fintelia.io>
>
> QEMU currently always triggers an illegal instruction exception when code
> attempts to read the time CSR. This is valid behavor, but only if the TM bit in
> mcounteren is hardwired to zero. This change also corrects mcounteren and scounteren CSRs to be 32-bits on both
> 32-bit and 64-bit targets.

Thanks for the patch.

Can you wrap your commit message at ~70 lines?

>
> Signed-off-by: Jonathan Behrens <jonathan@fintelia.io>
> ---
>  target/riscv/cpu.h | 4 ++--
>  target/riscv/csr.c | 3 ++-
>  2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 0adb307f32..2d0cbe9c78 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -151,8 +151,8 @@ struct CPURISCVState {
>      target_ulong mcause;
>      target_ulong mtval;  /* since: priv-1.10.0 */
>
> -    target_ulong scounteren;
> -    target_ulong mcounteren;
> +    uint32_t scounteren;
> +    uint32_t mcounteren;
>
>      target_ulong sscratch;
>      target_ulong mscratch;
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index e0d4586760..89cf9734c3 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -473,7 +473,8 @@ static int write_mcounteren(CPURISCVState *env, int csrno, target_ulong val)
>      if (env->priv_ver < PRIV_VERSION_1_10_0) {
>          return -1;
>      }
> -    env->mcounteren = val;
> +    /* mcounteren.TM is hardwired to zero, all other bits are writable */
> +    env->mcounteren = val & ~(1 << (CSR_TIME & 31));

Isn't CSR_TIME & 31 just 0? Can this just be changed 1 << 1 or even
better add a macro?

Otherwise this patch looks good :)

Alistair

>      return 0;
>  }
>
> --
> 2.22.0
>

Re: [Qemu-devel] [PATCH] target/riscv: Hardwire mcounter.TM and upper bits of [m|s]counteren

Posted by Jonathan Behrens 4 years, 10 months ago

> Can you wrap your commit message at ~70 lines?

Sure.

> Isn't CSR_TIME & 31 just 0? Can this just be changed 1 << 1 or even
better add a macro?

Any of those options work. Unless anyone feels strongly otherwise, I'll add
macros for the bits associated with the three named counters but not the
remaining 29 unnamed "high performance counters".

On Fri, Jun 28, 2019 at 5:03 PM Alistair Francis <alistair23@gmail.com>
wrote:

> On Fri, Jun 28, 2019 at 1:12 PM <jonathan@fintelia.io> wrote:
> >
> > From: Jonathan Behrens <jonathan@fintelia.io>
> >
> > QEMU currently always triggers an illegal instruction exception when code
> > attempts to read the time CSR. This is valid behavor, but only if the TM
> bit in
> > mcounteren is hardwired to zero. This change also corrects mcounteren
> and scounteren CSRs to be 32-bits on both
> > 32-bit and 64-bit targets.
>
> Thanks for the patch.
>
> Can you wrap your commit message at ~70 lines?
>
> >
> > Signed-off-by: Jonathan Behrens <jonathan@fintelia.io>
> > ---
> >  target/riscv/cpu.h | 4 ++--
> >  target/riscv/csr.c | 3 ++-
> >  2 files changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index 0adb307f32..2d0cbe9c78 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -151,8 +151,8 @@ struct CPURISCVState {
> >      target_ulong mcause;
> >      target_ulong mtval;  /* since: priv-1.10.0 */
> >
> > -    target_ulong scounteren;
> > -    target_ulong mcounteren;
> > +    uint32_t scounteren;
> > +    uint32_t mcounteren;
> >
> >      target_ulong sscratch;
> >      target_ulong mscratch;
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index e0d4586760..89cf9734c3 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -473,7 +473,8 @@ static int write_mcounteren(CPURISCVState *env, int
> csrno, target_ulong val)
> >      if (env->priv_ver < PRIV_VERSION_1_10_0) {
> >          return -1;
> >      }
> > -    env->mcounteren = val;
> > +    /* mcounteren.TM is hardwired to zero, all other bits are writable
> */
> > +    env->mcounteren = val & ~(1 << (CSR_TIME & 31));
>
> Isn't CSR_TIME & 31 just 0? Can this just be changed 1 << 1 or even
> better add a macro?
>
> Otherwise this patch looks good :)
>
> Alistair
>
> >      return 0;
> >  }
> >
> > --
> > 2.22.0
> >
>

Re: [Qemu-devel] [PATCH] target/riscv: Hardwire mcounter.TM and upper bits of [m|s]counteren

Posted by Alistair Francis 4 years, 10 months ago

On Fri, Jun 28, 2019 at 2:20 PM Jonathan Behrens <jonathan@fintelia.io> wrote:
>
> > Can you wrap your commit message at ~70 lines?
>
> Sure.
>
> > Isn't CSR_TIME & 31 just 0? Can this just be changed 1 << 1 or even better add a macro?
>
> Any of those options work. Unless anyone feels strongly otherwise, I'll add macros for the bits associated with the three named counters but not the remaining 29 unnamed "high performance counters".

Yep, sounds good.

Alistair

>
> On Fri, Jun 28, 2019 at 5:03 PM Alistair Francis <alistair23@gmail.com> wrote:
>>
>> On Fri, Jun 28, 2019 at 1:12 PM <jonathan@fintelia.io> wrote:
>> >
>> > From: Jonathan Behrens <jonathan@fintelia.io>
>> >
>> > QEMU currently always triggers an illegal instruction exception when code
>> > attempts to read the time CSR. This is valid behavor, but only if the TM bit in
>> > mcounteren is hardwired to zero. This change also corrects mcounteren and scounteren CSRs to be 32-bits on both
>> > 32-bit and 64-bit targets.
>>
>> Thanks for the patch.
>>
>> Can you wrap your commit message at ~70 lines?
>>
>> >
>> > Signed-off-by: Jonathan Behrens <jonathan@fintelia.io>
>> > ---
>> >  target/riscv/cpu.h | 4 ++--
>> >  target/riscv/csr.c | 3 ++-
>> >  2 files changed, 4 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>> > index 0adb307f32..2d0cbe9c78 100644
>> > --- a/target/riscv/cpu.h
>> > +++ b/target/riscv/cpu.h
>> > @@ -151,8 +151,8 @@ struct CPURISCVState {
>> >      target_ulong mcause;
>> >      target_ulong mtval;  /* since: priv-1.10.0 */
>> >
>> > -    target_ulong scounteren;
>> > -    target_ulong mcounteren;
>> > +    uint32_t scounteren;
>> > +    uint32_t mcounteren;
>> >
>> >      target_ulong sscratch;
>> >      target_ulong mscratch;
>> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>> > index e0d4586760..89cf9734c3 100644
>> > --- a/target/riscv/csr.c
>> > +++ b/target/riscv/csr.c
>> > @@ -473,7 +473,8 @@ static int write_mcounteren(CPURISCVState *env, int csrno, target_ulong val)
>> >      if (env->priv_ver < PRIV_VERSION_1_10_0) {
>> >          return -1;
>> >      }
>> > -    env->mcounteren = val;
>> > +    /* mcounteren.TM is hardwired to zero, all other bits are writable */
>> > +    env->mcounteren = val & ~(1 << (CSR_TIME & 31));
>>
>> Isn't CSR_TIME & 31 just 0? Can this just be changed 1 << 1 or even
>> better add a macro?
>>
>> Otherwise this patch looks good :)
>>
>> Alistair
>>
>> >      return 0;
>> >  }
>> >
>> > --
>> > 2.22.0
>> >