1. Patchew
  2. QEMU
  3. View series

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

jonathan@fintelia.io posted 1 patch 4 years, 10 months ago
Test docker-clang@ubuntu passed
Test s390x passed
Test asan passed
Test docker-mingw@fedora passed
Test FreeBSD passed
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20190701154617.22908-1-jonathan@fintelia.io
Maintainers: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>, Palmer Dabbelt <palmer@sifive.com>, Alistair Francis <Alistair.Francis@wdc.com>, Sagar Karandikar <sagark@eecs.berkeley.edu>
target/riscv/cpu.h      | 4 ++--
target/riscv/cpu_bits.h | 5 +++++
target/riscv/csr.c      | 2 +-
3 files changed, 8 insertions(+), 3 deletions(-)
[Qemu-devel] [PATCH v2] 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/cpu_bits.h | 5 +++++
 target/riscv/csr.c      | 2 +-
 3 files changed, 8 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/cpu_bits.h b/target/riscv/cpu_bits.h
index 11f971ad5d..0ea1e1caf5 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -532,4 +532,9 @@
 #define SIP_STIP                           MIP_STIP
 #define SIP_SEIP                           MIP_SEIP
 
+/* mcounteren CSR bits */
+#define MCOUNTEREN_CY                      0x1
+#define MCOUNTEREN_TM                      0x2
+#define MCOUNTEREN_IR                      0x4
+
 #endif
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index e0d4586760..8425a6d2bd 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -473,7 +473,7 @@ 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;
+    env->mcounteren = val & ~MCOUNTEREN_TM;
     return 0;
 }
 
-- 
2.22.0
Re: [Qemu-devel] [PATCH v2] target/riscv: Hardwire mcounter.TM and upper bits of [m|s]counteren
Posted by Alistair Francis 4 years, 10 months ago 
On Mon, Jul 1, 2019 at 8:56 AM <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.
>
> Signed-off-by: Jonathan Behrens <jonathan@fintelia.io>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/cpu.h      | 4 ++--
>  target/riscv/cpu_bits.h | 5 +++++
>  target/riscv/csr.c      | 2 +-
>  3 files changed, 8 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/cpu_bits.h b/target/riscv/cpu_bits.h
> index 11f971ad5d..0ea1e1caf5 100644
> --- a/target/riscv/cpu_bits.h
> +++ b/target/riscv/cpu_bits.h
> @@ -532,4 +532,9 @@
>  #define SIP_STIP                           MIP_STIP
>  #define SIP_SEIP                           MIP_SEIP
>
> +/* mcounteren CSR bits */
> +#define MCOUNTEREN_CY                      0x1
> +#define MCOUNTEREN_TM                      0x2
> +#define MCOUNTEREN_IR                      0x4
> +
>  #endif
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index e0d4586760..8425a6d2bd 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -473,7 +473,7 @@ 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;
> +    env->mcounteren = val & ~MCOUNTEREN_TM;
>      return 0;
>  }
>
> --
> 2.22.0
>
Re: [Qemu-devel] [PATCH v2] target/riscv: Hardwire mcounter.TM and upper bits of [m|s]counteren
Posted by Bin Meng 4 years, 10 months ago 
On Tue, Jul 2, 2019 at 8:20 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Mon, Jul 1, 2019 at 8:56 AM <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.
> >
> > Signed-off-by: Jonathan Behrens <jonathan@fintelia.io>
>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>

I am a little bit lost here. I think we agreed to allow directly read
to time CSR when mcounteren.TM is set, no?

Regards,
Bin
Re: [Qemu-devel] [PATCH v2] target/riscv: Hardwire mcounter.TM and upper bits of [m|s]counteren
Posted by Jonathan Behrens 4 years, 9 months ago 
Bin, that proposal proved to be somewhat more controversial than I was
expecting, since it was different than how currently available hardware
worked. This option seemed much more likely to be accepted in the short
term.

Jonathan

On Mon, Jul 1, 2019 at 9:26 PM Bin Meng <bmeng.cn@gmail.com> wrote:

> On Tue, Jul 2, 2019 at 8:20 AM Alistair Francis <alistair23@gmail.com>
> wrote:
> >
> > On Mon, Jul 1, 2019 at 8:56 AM <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.
> > >
> > > Signed-off-by: Jonathan Behrens <jonathan@fintelia.io>
> >
> > Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> >
>
> I am a little bit lost here. I think we agreed to allow directly read
> to time CSR when mcounteren.TM is set, no?
>
> Regards,
> Bin
>
Re: [Qemu-devel] [PATCH v2] target/riscv: Hardwire mcounter.TM and upper bits of [m|s]counteren
Posted by Jonathan Behrens 4 years, 8 months ago 
Ping! What is the status of this patch?

On Wed, Jul 3, 2019 at 2:02 PM Jonathan Behrens <jonathan@fintelia.io>
wrote:

> Bin, that proposal proved to be somewhat more controversial than I was
> expecting, since it was different than how currently available hardware
> worked. This option seemed much more likely to be accepted in the short
> term.
>
> Jonathan
>
> On Mon, Jul 1, 2019 at 9:26 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
>> On Tue, Jul 2, 2019 at 8:20 AM Alistair Francis <alistair23@gmail.com>
>> wrote:
>> >
>> > On Mon, Jul 1, 2019 at 8:56 AM <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.
>> > >
>> > > Signed-off-by: Jonathan Behrens <jonathan@fintelia.io>
>> >
>> > Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>> >
>>
>> I am a little bit lost here. I think we agreed to allow directly read
>> to time CSR when mcounteren.TM is set, no?
>>
>> Regards,
>> Bin
>>
>
Re: [Qemu-devel] [PATCH v2] target/riscv: Hardwire mcounter.TM and upper bits of [m|s]counteren
Posted by Palmer Dabbelt 4 years, 8 months ago 
On Wed, 14 Aug 2019 20:19:39 PDT (-0700), jonathan@fintelia.io wrote:
> Ping! What is the status of this patch?

Sorry, I must have lost track of it.  I've added it to my patch queue.

>
> On Wed, Jul 3, 2019 at 2:02 PM Jonathan Behrens <jonathan@fintelia.io>
> wrote:
>
>> Bin, that proposal proved to be somewhat more controversial than I was
>> expecting, since it was different than how currently available hardware
>> worked. This option seemed much more likely to be accepted in the short
>> term.
>>
>> Jonathan
>>
>> On Mon, Jul 1, 2019 at 9:26 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>>
>>> On Tue, Jul 2, 2019 at 8:20 AM Alistair Francis <alistair23@gmail.com>
>>> wrote:
>>> >
>>> > On Mon, Jul 1, 2019 at 8:56 AM <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.
>>> > >
>>> > > Signed-off-by: Jonathan Behrens <jonathan@fintelia.io>
>>> >
>>> > Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>>> >
>>>
>>> I am a little bit lost here. I think we agreed to allow directly read
>>> to time CSR when mcounteren.TM is set, no?
>>>
>>> Regards,
>>> Bin
>>>
>>
Re: [Qemu-devel] [PATCH v2] target/riscv: Hardwire mcounter.TM and upper bits of [m|s]counteren
Posted by Jonathan Behrens 4 years, 3 months ago 
I was just doubling checking the status of this patch because it conflicts
with the "RISC-V TIME CSR for privileged mode" PR that was just sent out,
and it seems this never got merged? In any case, perhaps these changes
should be rolled into that patch?

On Wed, Aug 21, 2019 at 1:37 PM Palmer Dabbelt <palmer@sifive.com> wrote:

> On Wed, 14 Aug 2019 20:19:39 PDT (-0700), jonathan@fintelia.io wrote:
> > Ping! What is the status of this patch?
>
> Sorry, I must have lost track of it.  I've added it to my patch queue.
>
> >
> > On Wed, Jul 3, 2019 at 2:02 PM Jonathan Behrens <jonathan@fintelia.io>
> > wrote:
> >
> >> Bin, that proposal proved to be somewhat more controversial than I was
> >> expecting, since it was different than how currently available hardware
> >> worked. This option seemed much more likely to be accepted in the short
> >> term.
> >>
> >> Jonathan
> >>
> >> On Mon, Jul 1, 2019 at 9:26 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >>
> >>> On Tue, Jul 2, 2019 at 8:20 AM Alistair Francis <alistair23@gmail.com>
> >>> wrote:
> >>> >
> >>> > On Mon, Jul 1, 2019 at 8:56 AM <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.
> >>> > >
> >>> > > Signed-off-by: Jonathan Behrens <jonathan@fintelia.io>
> >>> >
> >>> > Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> >>> >
> >>>
> >>> I am a little bit lost here. I think we agreed to allow directly read
> >>> to time CSR when mcounteren.TM is set, no?
> >>>
> >>> Regards,
> >>> Bin
> >>>
> >>
>
Re: [Qemu-devel] [PATCH v2] target/riscv: Hardwire mcounter.TM and upper bits of [m|s]counteren
Posted by Alistair Francis 4 years, 3 months ago 
On Tue, Jan 21, 2020 at 11:05 PM Jonathan Behrens <jonathan@fintelia.io> wrote:
>
> I was just doubling checking the status of this patch because it conflicts with the "RISC-V TIME CSR for privileged mode" PR that was just sent out, and it seems this never got merged? In any case, perhaps these changes should be rolled into that patch?

I think this should be merged first. @Palmer Dabbelt  can you merge this?

Alistair

>
> On Wed, Aug 21, 2019 at 1:37 PM Palmer Dabbelt <palmer@sifive.com> wrote:
>>
>> On Wed, 14 Aug 2019 20:19:39 PDT (-0700), jonathan@fintelia.io wrote:
>> > Ping! What is the status of this patch?
>>
>> Sorry, I must have lost track of it.  I've added it to my patch queue.
>>
>> >
>> > On Wed, Jul 3, 2019 at 2:02 PM Jonathan Behrens <jonathan@fintelia.io>
>> > wrote:
>> >
>> >> Bin, that proposal proved to be somewhat more controversial than I was
>> >> expecting, since it was different than how currently available hardware
>> >> worked. This option seemed much more likely to be accepted in the short
>> >> term.
>> >>
>> >> Jonathan
>> >>
>> >> On Mon, Jul 1, 2019 at 9:26 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>> >>
>> >>> On Tue, Jul 2, 2019 at 8:20 AM Alistair Francis <alistair23@gmail.com>
>> >>> wrote:
>> >>> >
>> >>> > On Mon, Jul 1, 2019 at 8:56 AM <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.
>> >>> > >
>> >>> > > Signed-off-by: Jonathan Behrens <jonathan@fintelia.io>
>> >>> >
>> >>> > Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>> >>> >
>> >>>
>> >>> I am a little bit lost here. I think we agreed to allow directly read
>> >>> to time CSR when mcounteren.TM is set, no?
>> >>>
>> >>> Regards,
>> >>> Bin
>> >>>
>> >>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.