[PATCH] riscv: enable TIME CSR in U mode

[PATCH] riscv: enable TIME CSR in U mode

[Gary Guo]

After ad5d112 we let the user mode to use rdtime directly for time
access. This works if the hardware does not implement the TIME CSR
and traps to the firmware. The spec however does allow a hardware
implementation to redirect the CSR access to the memory-mapped MTIME
CSR, and if this is performed it will check {M,S}COUNTEREN CSR to
see if it is allowed.

To prevent unhandled illegal instruction fault on these platforms,
we can request these CSRs to be enabled in U-mode. For platforms
that does not support the TIME CSR to MTIME CSR conversion, they
will hardwire corresponding bit in COUNTEREN to zero and this would
be a no-op.

Signed-off-by: Gary Guo <gary@garyguo.net>
---
 arch/riscv/kernel/head.S | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
index 0a4e81b8dc79..5a7a62190342 100644
--- a/arch/riscv/kernel/head.S
+++ b/arch/riscv/kernel/head.S
@@ -190,6 +190,9 @@ ENTRY(_start_kernel)
 .align 2
 pmp_done:
 
+	/* Allow user-mode to access time CSR */
+	csrw, CSR_MCOUNTEREN, 2
+
 	/*
 	 * The hartid in a0 is expected later on, and we have no firmware
 	 * to hand it to us.
@@ -197,6 +200,9 @@ pmp_done:
 	csrr a0, CSR_MHARTID
 #endif /* CONFIG_RISCV_M_MODE */
 
+	/* Allow user-mode to access time CSR */
+	csrw CSR_SCOUNTEREN, 2
+
 	/* Load the global pointer */
 .option push
 .option norelax
-- 
2.20.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] riscv: enable TIME CSR in U mode

[Nick Kossifidis]

Στις 2021-02-17 00:12, Gary Guo έγραψε:
> After ad5d112 we let the user mode to use rdtime directly for time
> access. This works if the hardware does not implement the TIME CSR
> and traps to the firmware. The spec however does allow a hardware
> implementation to redirect the CSR access to the memory-mapped MTIME
> CSR, and if this is performed it will check {M,S}COUNTEREN CSR to
> see if it is allowed.
> 
> To prevent unhandled illegal instruction fault on these platforms,
> we can request these CSRs to be enabled in U-mode. For platforms
> that does not support the TIME CSR to MTIME CSR conversion, they
> will hardwire corresponding bit in COUNTEREN to zero and this would
> be a no-op.
> 
> Signed-off-by: Gary Guo <gary@garyguo.net>
> ---
>  arch/riscv/kernel/head.S | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
> index 0a4e81b8dc79..5a7a62190342 100644
> --- a/arch/riscv/kernel/head.S
> +++ b/arch/riscv/kernel/head.S
> @@ -190,6 +190,9 @@ ENTRY(_start_kernel)
>  .align 2
>  pmp_done:
> 
> +	/* Allow user-mode to access time CSR */
> +	csrw, CSR_MCOUNTEREN, 2
> +
>  	/*
>  	 * The hartid in a0 is expected later on, and we have no firmware
>  	 * to hand it to us.
> @@ -197,6 +200,9 @@ pmp_done:
>  	csrr a0, CSR_MHARTID
>  #endif /* CONFIG_RISCV_M_MODE */
> 
> +	/* Allow user-mode to access time CSR */
> +	csrw CSR_SCOUNTEREN, 2
> +
>  	/* Load the global pointer */
>  .option push
>  .option norelax

Shouldn't we use csrrs here to preserve current settings from the 
firmware ? Also OpenSBI at this point sets both MCOUNTEREN/SCOUNTEREN to 
-1, does anyone else modify that value ?

Regards,
Nick

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

RE: [PATCH] riscv: enable TIME CSR in U mode

[Gary Guo]

I don't think we should let the firmware to decide which performance counter CSRs can be used by the userspace. No counter should be allowed unless there is a reason to allow it, like the timer CSR. Also, OpenSBI is not the only SBI implementation,; there could be no SBIs at all (i.e. M mode) or have other SBIs that have different defaults.

Best,
Gary

-----Original Message-----
From: Nick Kossifidis <mick@ics.forth.gr> 
Sent: Thursday, March 4, 2021 5:35 PM
To: Gary Guo <gary@garyguo.net>
Cc: linux-riscv@lists.infradead.org
Subject: Re: [PATCH] riscv: enable TIME CSR in U mode

Στις 2021-02-17 00:12, Gary Guo έγραψε:
> After ad5d112 we let the user mode to use rdtime directly for time 
> access. This works if the hardware does not implement the TIME CSR and 
> traps to the firmware. The spec however does allow a hardware 
> implementation to redirect the CSR access to the memory-mapped MTIME 
> CSR, and if this is performed it will check {M,S}COUNTEREN CSR to see 
> if it is allowed.
> 
> To prevent unhandled illegal instruction fault on these platforms, we 
> can request these CSRs to be enabled in U-mode. For platforms that 
> does not support the TIME CSR to MTIME CSR conversion, they will 
> hardwire corresponding bit in COUNTEREN to zero and this would be a 
> no-op.
> 
> Signed-off-by: Gary Guo <gary@garyguo.net>
> ---
>  arch/riscv/kernel/head.S | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S index 
> 0a4e81b8dc79..5a7a62190342 100644
> --- a/arch/riscv/kernel/head.S
> +++ b/arch/riscv/kernel/head.S
> @@ -190,6 +190,9 @@ ENTRY(_start_kernel)  .align 2
>  pmp_done:
> 
> +	/* Allow user-mode to access time CSR */
> +	csrw, CSR_MCOUNTEREN, 2
> +
>  	/*
>  	 * The hartid in a0 is expected later on, and we have no firmware
>  	 * to hand it to us.
> @@ -197,6 +200,9 @@ pmp_done:
>  	csrr a0, CSR_MHARTID
>  #endif /* CONFIG_RISCV_M_MODE */
> 
> +	/* Allow user-mode to access time CSR */
> +	csrw CSR_SCOUNTEREN, 2
> +
>  	/* Load the global pointer */
>  .option push
>  .option norelax

Shouldn't we use csrrs here to preserve current settings from the firmware ? Also OpenSBI at this point sets both MCOUNTEREN/SCOUNTEREN to -1, does anyone else modify that value ?

Regards,
Nick
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] riscv: enable TIME CSR in U mode

[Nick Kossifidis]

Then we also need to modify the current perf code that assumes e.g. that 
instret is enabled. I'd suggest that until we have an SBI interface 
(since we also need to make sure that the enabled counters are also set 
on mcounteren) for enabling / disabling counters, we leave this to the 
firmware.

Regards,
Nick

Στις 2021-03-16 08:24, Gary Guo έγραψε:
> I don't think we should let the firmware to decide which performance
> counter CSRs can be used by the userspace. No counter should be
> allowed unless there is a reason to allow it, like the timer CSR.
> Also, OpenSBI is not the only SBI implementation,; there could be no
> SBIs at all (i.e. M mode) or have other SBIs that have different
> defaults.
> 
> Best,
> Gary
> 
> -----Original Message-----
> From: Nick Kossifidis <mick@ics.forth.gr>
> Sent: Thursday, March 4, 2021 5:35 PM
> To: Gary Guo <gary@garyguo.net>
> Cc: linux-riscv@lists.infradead.org
> Subject: Re: [PATCH] riscv: enable TIME CSR in U mode
> 
> Στις 2021-02-17 00:12, Gary Guo έγραψε:
>> After ad5d112 we let the user mode to use rdtime directly for time
>> access. This works if the hardware does not implement the TIME CSR and
>> traps to the firmware. The spec however does allow a hardware
>> implementation to redirect the CSR access to the memory-mapped MTIME
>> CSR, and if this is performed it will check {M,S}COUNTEREN CSR to see
>> if it is allowed.
>> 
>> To prevent unhandled illegal instruction fault on these platforms, we
>> can request these CSRs to be enabled in U-mode. For platforms that
>> does not support the TIME CSR to MTIME CSR conversion, they will
>> hardwire corresponding bit in COUNTEREN to zero and this would be a
>> no-op.
>> 
>> Signed-off-by: Gary Guo <gary@garyguo.net>
>> ---
>>  arch/riscv/kernel/head.S | 6 ++++++
>>  1 file changed, 6 insertions(+)
>> 
>> diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S index
>> 0a4e81b8dc79..5a7a62190342 100644
>> --- a/arch/riscv/kernel/head.S
>> +++ b/arch/riscv/kernel/head.S
>> @@ -190,6 +190,9 @@ ENTRY(_start_kernel)  .align 2
>>  pmp_done:
>> 
>> +	/* Allow user-mode to access time CSR */
>> +	csrw, CSR_MCOUNTEREN, 2
>> +
>>  	/*
>>  	 * The hartid in a0 is expected later on, and we have no firmware
>>  	 * to hand it to us.
>> @@ -197,6 +200,9 @@ pmp_done:
>>  	csrr a0, CSR_MHARTID
>>  #endif /* CONFIG_RISCV_M_MODE */
>> 
>> +	/* Allow user-mode to access time CSR */
>> +	csrw CSR_SCOUNTEREN, 2
>> +
>>  	/* Load the global pointer */
>>  .option push
>>  .option norelax
> 
> Shouldn't we use csrrs here to preserve current settings from the
> firmware ? Also OpenSBI at this point sets both MCOUNTEREN/SCOUNTEREN
> to -1, does anyone else modify that value ?
> 
> Regards,
> Nick

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] riscv: enable TIME CSR in U mode

[Atish Patra]

On Tue, Mar 16, 2021 at 1:53 PM Nick Kossifidis <mick@ics.forth.gr> wrote:
>
> Then we also need to modify the current perf code that assumes e.g. that
> instret is enabled. I'd suggest that until we have an SBI interface
> (since we also need to make sure that the enabled counters are also set
> on mcounteren) for enabling / disabling counters, we leave this to the
> firmware.
>

Just FYI:
I just sent the revised version of SBI PMU extension patch to the unix
platform mailing list.
https://lists.riscv.org/g/tech-unixplatformspec/message/598

I am mostly done with OpenSBI/Linux kernel implementation as well and
will be sending patches
sometime this week.

> Regards,
> Nick
>
> Στις 2021-03-16 08:24, Gary Guo έγραψε:
> > I don't think we should let the firmware to decide which performance
> > counter CSRs can be used by the userspace. No counter should be
> > allowed unless there is a reason to allow it, like the timer CSR.
> > Also, OpenSBI is not the only SBI implementation,; there could be no
> > SBIs at all (i.e. M mode) or have other SBIs that have different
> > defaults.
> >
> > Best,
> > Gary
> >
> > -----Original Message-----
> > From: Nick Kossifidis <mick@ics.forth.gr>
> > Sent: Thursday, March 4, 2021 5:35 PM
> > To: Gary Guo <gary@garyguo.net>
> > Cc: linux-riscv@lists.infradead.org
> > Subject: Re: [PATCH] riscv: enable TIME CSR in U mode
> >
> > Στις 2021-02-17 00:12, Gary Guo έγραψε:
> >> After ad5d112 we let the user mode to use rdtime directly for time
> >> access. This works if the hardware does not implement the TIME CSR and
> >> traps to the firmware. The spec however does allow a hardware
> >> implementation to redirect the CSR access to the memory-mapped MTIME
> >> CSR, and if this is performed it will check {M,S}COUNTEREN CSR to see
> >> if it is allowed.
> >>
> >> To prevent unhandled illegal instruction fault on these platforms, we
> >> can request these CSRs to be enabled in U-mode. For platforms that
> >> does not support the TIME CSR to MTIME CSR conversion, they will
> >> hardwire corresponding bit in COUNTEREN to zero and this would be a
> >> no-op.
> >>
> >> Signed-off-by: Gary Guo <gary@garyguo.net>
> >> ---
> >>  arch/riscv/kernel/head.S | 6 ++++++
> >>  1 file changed, 6 insertions(+)
> >>
> >> diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S index
> >> 0a4e81b8dc79..5a7a62190342 100644
> >> --- a/arch/riscv/kernel/head.S
> >> +++ b/arch/riscv/kernel/head.S
> >> @@ -190,6 +190,9 @@ ENTRY(_start_kernel)  .align 2
> >>  pmp_done:
> >>
> >> +    /* Allow user-mode to access time CSR */
> >> +    csrw, CSR_MCOUNTEREN, 2
> >> +
> >>      /*
> >>       * The hartid in a0 is expected later on, and we have no firmware
> >>       * to hand it to us.
> >> @@ -197,6 +200,9 @@ pmp_done:
> >>      csrr a0, CSR_MHARTID
> >>  #endif /* CONFIG_RISCV_M_MODE */
> >>
> >> +    /* Allow user-mode to access time CSR */
> >> +    csrw CSR_SCOUNTEREN, 2
> >> +
> >>      /* Load the global pointer */
> >>  .option push
> >>  .option norelax
> >
> > Shouldn't we use csrrs here to preserve current settings from the
> > firmware ? Also OpenSBI at this point sets both MCOUNTEREN/SCOUNTEREN
> > to -1, does anyone else modify that value ?
> >
> > Regards,
> > Nick
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv



-- 
Regards,
Atish

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv