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

[Alistair Francis]

On Fri, Jun 28, 2019 at 2:20 PM Jonathan Behrens <address@hidden> 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 <address@hidden> wrote:
>>
>> On Fri, Jun 28, 2019 at 1:12 PM <address@hidden> wrote:
>> >
>> > From: Jonathan Behrens <address@hidden>
>> >
>> > 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 <address@hidden>
>> > ---
>> >  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
>> >