On Fri, Jun 28, 2019 at 2:20 PM Jonathan Behrens <jonat...@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 <alistai...@gmail.com> wrote:
>>
>> On Fri, Jun 28, 2019 at 1:12 PM <jonat...@fintelia.io> wrote:
>> >
>> > From: Jonathan Behrens <jonat...@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 <jonat...@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
>> >
