On riscv32, the time comparator value is split across two CSRs. We write
both when stopping the timer, but realistically the time is just as
unlikely to reach 0xffffffff00000000 as 0xffffffffffffffff, so there is
no need to write the low CSR.
Signed-off-by: Samuel Holland <[email protected]>
---
drivers/clocksource/timer-riscv.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/timer-riscv.c
index e66dcbd66566..eaaf01f3c34b 100644
--- a/drivers/clocksource/timer-riscv.c
+++ b/drivers/clocksource/timer-riscv.c
@@ -35,9 +35,10 @@ static bool riscv_timer_cannot_wake_cpu;
static void riscv_clock_event_stop(void)
{
if (static_branch_likely(&riscv_sstc_available)) {
- csr_write(CSR_STIMECMP, ULONG_MAX);
if (IS_ENABLED(CONFIG_32BIT))
csr_write(CSR_STIMECMPH, ULONG_MAX);
+ else
+ csr_write(CSR_STIMECMP, ULONG_MAX);
} else {
sbi_set_timer(U64_MAX);
}
--
2.43.1
On Wed, Mar 13, 2024 at 1:03 AM Samuel Holland
<[email protected]> wrote:
>
> On riscv32, the time comparator value is split across two CSRs. We write
> both when stopping the timer, but realistically the time is just as
> unlikely to reach 0xffffffff00000000 as 0xffffffffffffffff, so there is
> no need to write the low CSR.
Even though unlikely, there is still a theoretical possibility of
counter reaching value 0xffffffff00000000.
The good thing about value 0xffffffffffffffff is that the counter will
immediately wrap around after reaching it.
Regards,
Anup
>
> Signed-off-by: Samuel Holland <[email protected]>
> ---
>
> drivers/clocksource/timer-riscv.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/timer-riscv.c
> index e66dcbd66566..eaaf01f3c34b 100644
> --- a/drivers/clocksource/timer-riscv.c
> +++ b/drivers/clocksource/timer-riscv.c
> @@ -35,9 +35,10 @@ static bool riscv_timer_cannot_wake_cpu;
> static void riscv_clock_event_stop(void)
> {
> if (static_branch_likely(&riscv_sstc_available)) {
> - csr_write(CSR_STIMECMP, ULONG_MAX);
> if (IS_ENABLED(CONFIG_32BIT))
> csr_write(CSR_STIMECMPH, ULONG_MAX);
> + else
> + csr_write(CSR_STIMECMP, ULONG_MAX);
> } else {
> sbi_set_timer(U64_MAX);
> }
> --
> 2.43.1
>
>
On Wed, 13 Mar 2024 09:56:34 PDT (-0700), [email protected] wrote:
> On Wed, Mar 13, 2024 at 1:03 AM Samuel Holland
> <[email protected]> wrote:
>>
>> On riscv32, the time comparator value is split across two CSRs. We write
>> both when stopping the timer, but realistically the time is just as
>> unlikely to reach 0xffffffff00000000 as 0xffffffffffffffff, so there is
>> no need to write the low CSR.
>
> Even though unlikely, there is still a theoretical possibility of
> counter reaching value 0xffffffff00000000.
I guess that depends on the timebase frequency, but if my math is right
then we've got some timers that will overflow a 32-bit counter in 10
minutes -- take that with a grain of salt as they're all 64-bit systems
(we don't have any 32-bit DTs upstream?), but it still seems plausible
for 32-bit overflows to happen here on real systems.
> The good thing about value 0xffffffffffffffff is that the counter will
> immediately wrap around after reaching it.
I'm not sure how that's good? Luckily we've got ~100,000 years to
figure out, even on these systems with pretty fast timers ;)
> Regards,
> Anup
>
>
>>
>> Signed-off-by: Samuel Holland <[email protected]>
>> ---
>>
>> drivers/clocksource/timer-riscv.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/timer-riscv.c
>> index e66dcbd66566..eaaf01f3c34b 100644
>> --- a/drivers/clocksource/timer-riscv.c
>> +++ b/drivers/clocksource/timer-riscv.c
>> @@ -35,9 +35,10 @@ static bool riscv_timer_cannot_wake_cpu;
>> static void riscv_clock_event_stop(void)
>> {
>> if (static_branch_likely(&riscv_sstc_available)) {
>> - csr_write(CSR_STIMECMP, ULONG_MAX);
>> if (IS_ENABLED(CONFIG_32BIT))
>> csr_write(CSR_STIMECMPH, ULONG_MAX);
>> + else
>> + csr_write(CSR_STIMECMP, ULONG_MAX);
>> } else {
>> sbi_set_timer(U64_MAX);
>> }
>> --
>> 2.43.1
>>
>>
On Wed, Apr 17, 2024 at 7:20 AM Palmer Dabbelt <[email protected]> wrote:
>
> On Wed, 13 Mar 2024 09:56:34 PDT (-0700), [email protected] wrote:
> > On Wed, Mar 13, 2024 at 1:03 AM Samuel Holland
> > <[email protected]> wrote:
> >>
> >> On riscv32, the time comparator value is split across two CSRs. We write
> >> both when stopping the timer, but realistically the time is just as
> >> unlikely to reach 0xffffffff00000000 as 0xffffffffffffffff, so there is
> >> no need to write the low CSR.
> >
> > Even though unlikely, there is still a theoretical possibility of
> > counter reaching value 0xffffffff00000000.
>
> I guess that depends on the timebase frequency, but if my math is right
> then we've got some timers that will overflow a 32-bit counter in 10
> minutes -- take that with a grain of salt as they're all 64-bit systems
> (we don't have any 32-bit DTs upstream?), but it still seems plausible
> for 32-bit overflows to happen here on real systems.
>
> > The good thing about value 0xffffffffffffffff is that the counter will
> > immediately wrap around after reaching it.
>
> I'm not sure how that's good? Luckily we've got ~100,000 years to
> figure out, even on these systems with pretty fast timers ;)
The RISC-V server soc spec mandates timer frequency to be at least
100 MHz so we can certainly have 100MHz as the timer frequency
and based on this time CSR will overflow in roughly 5840 years. Also,
nothing stops a SoC vendor from having a GHz timer frequency.
In addition to timer frequency, changing the initial value of time CSR
can also impact when the time CSR overflows. This particularly applies
to virtualization because KVM RISC-V allows the KVM user space tool
to set the desired initial value of time CSR visible to the Guest/VM.
For example, if the timer frequency is 10MHz and KVM user space
tool sets initial time CSR value to 0xffffffff_00000000 then Guest/VM
time CSR will overflow in roughly 7 mins.
Regards,
Anup
>
> > Regards,
> > Anup
> >
> >
> >>
> >> Signed-off-by: Samuel Holland <[email protected]>
> >> ---
> >>
> >> drivers/clocksource/timer-riscv.c | 3 ++-
> >> 1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/timer-riscv.c
> >> index e66dcbd66566..eaaf01f3c34b 100644
> >> --- a/drivers/clocksource/timer-riscv.c
> >> +++ b/drivers/clocksource/timer-riscv.c
> >> @@ -35,9 +35,10 @@ static bool riscv_timer_cannot_wake_cpu;
> >> static void riscv_clock_event_stop(void)
> >> {
> >> if (static_branch_likely(&riscv_sstc_available)) {
> >> - csr_write(CSR_STIMECMP, ULONG_MAX);
> >> if (IS_ENABLED(CONFIG_32BIT))
> >> csr_write(CSR_STIMECMPH, ULONG_MAX);
> >> + else
> >> + csr_write(CSR_STIMECMP, ULONG_MAX);
> >> } else {
> >> sbi_set_timer(U64_MAX);
> >> }
> >> --
> >> 2.43.1
> >>
> >>
>