Received-SPF: pass (google.com: domain of linux-kernel+bounces-147974-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <CAK9=C2Vsq0iN5FW=zC7+2GHe115TPJOEEPcDCLs_40yBGpbVdw@mail.gmail.com>
 <mhng-3a5fcdd0-0bf4-45d3-aa7d-fffddd852288@palmer-ri-x1c9a>
In-Reply-To: <mhng-3a5fcdd0-0bf4-45d3-aa7d-fffddd852288@palmer-ri-x1c9a>
From: Anup Patel <anup@brainfault.org>
Date: Wed, 17 Apr 2024 11:34:02 +0530
Message-ID: <CAAhSdy0YJ7STirgtZpdBGGBT6HB19CdN_Fe0ywhd8SSF2iXJqg@mail.gmail.com>
Subject: Re: [PATCH] clocksource/drivers/timer-riscv: Drop extra CSR write
To: Palmer Dabbelt <palmer@dabbelt.com>
Cc: apatel@ventanamicro.com, samuel.holland@sifive.com, 
	daniel.lezcano@linaro.org, tglx@linutronix.de, aou@eecs.berkeley.edu, 
	Paul Walmsley <paul.walmsley@sifive.com>, linux-kernel@vger.kernel.org, 
	linux-riscv@lists.infradead.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Apr 17, 2024 at 7:20=E2=80=AFAM Palmer Dabbelt <palmer@dabbelt.com>=
 wrote:
>
> On Wed, 13 Mar 2024 09:56:34 PDT (-0700), apatel@ventanamicro.com wrote:
> > On Wed, Mar 13, 2024 at 1:03=E2=80=AFAM Samuel Holland
> > <samuel.holland@sifive.com> wrote:
> >>
> >> On riscv32, the time comparator value is split across two CSRs. We wri=
te
> >> both when stopping the timer, but realistically the time is just as
> >> unlikely to reach 0xffffffff00000000 as 0xffffffffffffffff, so there i=
s
> >> 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 <samuel.holland@sifive.com>
> >> ---
> >>
> >>  drivers/clocksource/timer-riscv.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/t=
imer-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
> >>
> >>
>