| Message ID | 20210216221208.2860-1-gary@garyguo.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | riscv: enable TIME CSR in U mode | expand |
Στις 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
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
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
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
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
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(+)