User Space Physical Counter Access by cazb2 · Pull Request #707 · au-ts/sddf

cazb2 · 2026-04-15T13:29:46Z

This commit exposes a common API to interact with physical counters
on arm and x86.

The x86 logic was introduced to improve the performance of the timer driver
see commit #645402620af9bf9a6bf4ef3bd17d5a1077a567b7 on x86.

Using these counters provides significant performance benefits in contrast to
interacting with an external timer driver. At the bare minimum this change will
save thousands of cycles for applications that simply need to track time progression
rather than reading a timestamp and or setting time outs, for the reasons discussed
in #645402620af9bf9a6bf4ef3bd17d5a1077a567b7 as well as avoiding the overhead of invoking
the kernel un-necessarily.

Putting this code in a library like this means that applications that are not worried about
architecture specific details can interact with the generic read_counter and read_freq
functions, avoiding the need for #if defined checks. In addition, applications do not need
to repeat the non trivial boiler plate logic on x86 that is required to calculate the tsc frequency.

Furthermore for x86 in future if we want to calculate the tsc freq when it is otherwise not
available, having a stable API now will avoid having to change more files later.

Will have to deal with risc-v too.

dreamliner787-9 · 2026-04-15T23:48:50Z

Other things:
Your commit message have a typo.

dreamliner787-9 · 2026-04-15T23:49:19Z

You should modify the tsc_hpet driver to use the new common TSC functions that you have extracted into a library.

dreamliner787-9 · 2026-04-15T23:51:50Z

Your changes need to be motivated more, I can intuit why it is needed but I would like to see it in words for future reference by others.

cazb2 · 2026-04-16T00:20:30Z

Hopefully I have addressed your feedback, for the justification was there anything else I should include?

dreamliner787-9 · 2026-04-16T00:25:52Z

+    return get_tsc_frequency();
+}
+
+#elif defined(CONFIG_ARCH_AARCH64)


Since there are both ARM and x86 architectural timer stuff in this file, I don't think it should be called tsc.c, maybe something like arch_counter.c. Or you split it into 2 architecture specific files. Then have the makefile build and link the correct file at build time based on the target architecture.

For now I did your first solution, but this can be "improved" later based on @midnightveil comments about making it more arch generic too

dreamliner787-9 · 2026-04-16T00:26:39Z

Hopefully I have addressed your feedback, for the justification was there anything else I should include?

You should get the CI to pass, so for RISC-V I think you should just not expose the functions, rather than having a hard preprocessor error.

midnightveil

Please see our previous timer design discussions.

This is almost certainly worth doing, but it makes sense to be part of the time client as we don't necessarily always want to give clients access to the current time.

But yes we've wanted to do something like this for a while. Unfortunately this doesn't work on RISC-V, which means we need some kind of equivalent to Linux's gettimeofday where we have the fast architectural access or the slow fallback.

cazb2 · 2026-04-16T00:34:44Z

Please see our previous timer design discussions.

This is almost certainly worth doing, but it makes sense to be part of the time client as we don't necessarily always want to give clients access to the current time.

Is giving clients a counter really the same as giving them time though? Clients would still need to interact with a timer driver / virtualiser to get a timestamp, or through some other means...

But yes we've wanted to do something like this for a while. Unfortunately this doesn't work on RISC-V, which means we need some kind of equivalent to Linux's gettimeofday where we have the fast architectural access or the slow fallback.

Ivan mention a discussion: https://sel4.discourse.group/t/pre-rfc-arm-add-tcb-policy-support-for-controlling-user-level-access-to-certain-system-registers/507

Which is probably the better solution but would require different work, and I think this code would still be required either way.

Does risc-v forbid access to any form of counter in the user level privilege mode?

midnightveil · 2026-04-16T00:39:02Z

Does risc-v forbid access to any form of counter in the user level privilege mode?

It doesn't forbid it, it's just that there is no standardised extension for user-level counters (even if there was there is basically no hardware you can buy that support for the supervisor timestamp lol). Except for maybe rdcycle, but that's cycles, not a fixed-frequency counter.

Which is probably the better solution but would require different work, and I think this code would still be required either way.

Yes, it looks like since the current count is already enabled for everyone by default then this probably makes sense to do. I think there's some modifications we'd do to this to make it more arch-generic but yeah.

cazb2 · 2026-04-16T00:52:33Z

Does risc-v forbid access to any form of counter in the user level privilege mode?

It doesn't forbid it, it's just that there is no standardised extension for user-level counters (even if there was there is basically no hardware you can buy that support for the supervisor timestamp lol). Except for maybe rdcycle, but that's cycles, not a fixed-frequency counter.

That is a bit unfortunate...

Which is probably the better solution but would require different work, and I think this code would still be required either way.

Yes, it looks like since the current count is already enabled for everyone by default then this probably makes sense to do. I think there's some modifications we'd do to this to make it more arch-generic but yeah.

I might need advice on what the accepted way to do that is ...

Ivan-Velickovic · 2026-04-16T01:14:39Z

no hardware you can buy that support for the supervisor timestamp lol

There are multiple platforms, we just don't have any. Most of our platforms our SiFive based which happen to not implement it. In RVA23 Sstc is mandatory.

I've never actually tried to do rdtime from user-space, have we actually checked it is not allowed?

omeh-a · 2026-04-16T01:22:56Z

Is giving clients a counter really the same as giving them time though? Clients would still need to interact with a timer driver / virtualiser to get a timestamp, or through some other means...

Yes, it effectively is. They can track global time themselves using this timer alone. Our current timer driver on x86 will just return the TSC time stamp as the "current time".

Ivan-Velickovic · 2026-04-16T01:26:26Z

I've never actually tried to do rdtime from user-space, have we actually checked it is not allowed?

Worked on QEMU, Star64, and HiFive P550.

cazb2 · 2026-04-16T01:32:02Z

Is giving clients a counter really the same as giving them time though? Clients would still need to interact with a timer driver / virtualiser to get a timestamp, or through some other means...

Yes, it effectively is. They can track global time themselves using this timer alone. Our current timer driver on x86 will just return the TSC time stamp as the "current time".

Hmm my understanding was the TSC is just a "count" since reset.
If unrestricted user access is an issue it should be addressed too, and would likely require kernel changes similar to the pre RFC article linked above.

cazb2 · 2026-04-16T01:36:27Z

I've never actually tried to do rdtime from user-space, have we actually checked it is not allowed?

Worked on QEMU, Star64, and HiFive P550.

https://docs.riscv.org/reference/isa/unpriv/counters.html
is this reliable/what we want

midnightveil · 2026-04-16T01:44:11Z

docs.riscv.org/reference/isa/unpriv/counters.html
is this reliable/what we want

Yes, use the rdtime pseudo-instruction for RISC-V.

omeh-a · 2026-04-16T01:45:22Z

Is giving clients a counter really the same as giving them time though? Clients would still need to interact with a timer driver / virtualiser to get a timestamp, or through some other means...

Yes, it effectively is. They can track global time themselves using this timer alone. Our current timer driver on x86 will just return the TSC time stamp as the "current time".

Hmm my understanding was the TSC is just a "count" since reset. If unrestricted user access is an issue it should be addressed too, and would likely require kernel changes similar to the pre RFC article linked above.

All times on computers are counts since a reset :P ... even the UNIX timestamp is just the count from a certain reset time. Just because a count isn't given a "time of day" format doesn't mean it doesn't give programs the same power. Again, look at the current timer driver design - having this timer exposed effectively gives all clients access to get_time().

Unrestricted access is sort of an issue since it's technically opening a timing channel. Realistically, this isn't a huge problem and systems that are so sensitive can just fully disable it for now. The trouble comes if we make usage of this mandatory for the timer driver - in this case, we should be able to control which clients see the time. It's a philosophical issue basically, not one that is worthy of blocking IMO. @Ivan-Velickovic or @wom-bat may have more thoughts on it.

omeh-a

Generally looks pretty good to me. I am not familiar with the nitty gritty behaviour of these architectural timers however. @KurtWu10 @midnightveil may have some more insight - with another pair of eyes on this we can call it approved and merge :)

omeh-a · 2026-06-09T22:58:27Z

+generated/sddf/generated/board_config.%: $(DTB)
+	mkdir -p $(dir $@)
+	printf '#pragma once\n\n' > $@
+	printf '#define CONFIG_RISCV_RDTIME_FREQ %s\n' "$$($(FDTGET) -t u $(DTB) /cpus timebase-frequency)" >> $@


If we're parsing the DTS to get this, it may be better to support this feature via sdfgen instead of adding new tooling. The new Python-based sdfgen is almost ready and should make this easy... I can help set that up once the new sdfgen is merged.

(sidenote: fine to merge this for now I guess? we should just open an issue about this + a comment in these makefiles)

Will make a note + issue. I imagine a lot of what happens in the makefile could be done in python, maybe your sdfgen PR has already begun that transition so to speak

I think given that we're planning on reworking and not extending this board config header file it surely makes more sense to just do this for tbe timer data and generate a config struct in the build system?

given (a) this is a bad idea if we have one timer driver shared by multiple boards, since it breaks the compile once model and (b) we're gonna change it anyway and making generic stuff is more pain when it doesn't need to be generic.

I'm not 100% confident I'm following you here, but if the jist is that we can embed the frequency as a config data blob rather than forcing recompilation that makes sense to me. In terms of implementing that since this stuff can be used by other PD's i.e. non timer driver PDs where should the config blob be generated (makefile? a meta.py?)

ideally meta.py.

I implemented the library side of this. The client will need to perform the necessary symbol update using their meta.py.

KurtWu10 · 2026-06-12T04:03:30Z

The inline assembly looks good to me.

midnightveil · 2026-06-13T01:52:28Z

+}
+
+static bool cached_frequency = false;
+static uint64_t get_tsc_frequency(void)


seL4 exposes the x86 tsc frequency (if there is one) in its boot info. We should just use that.

wouldn't that require run time sharing of that information?

omeh-a

Looks good to me, we can merge if @midnightveil is satisfied with feedback.

midnightveil

Can this not be integrated into PDs, or into our existing timer subsystem? Adding this code and never using it seems a bit weird...?

(Please see my previous review: can this not be integrated into a time subsystem...?.

This feels like an internal implementation detail of our time functions, not something that we "need" to expose to people).

Also, I don't think arch_counter.h should be exposing arch-specific definitions: it is quite annoying when things only exist sometimes.

midnightveil · 2026-06-15T01:16:14Z

 #
 # SPDX-License-Identifier: BSD-2-Clause
 #
+PLATFORM ?= riscv


not needed anymore?

I will remove, is it intentionally not there because all the other make files seem to define the platform?

midnightveil · 2026-06-15T01:16:19Z

@@ -0,0 +1,38 @@
+/*
+ * Copyright 2022, UNSW


midnightveil · 2026-06-15T01:16:52Z

+ * Each implementation aims to use the required serialisation instructions to ensure
+ * an accurate counter read is returned. The ARM barriers try to reflect the x86 synchronisation
+ * and the risc-v is the mapping of the ARM barriers from https://docs.riscv.org/reference/isa/unpriv/mm-eplan.html#armmappings
+ */


What does this mean?

"reflect the x86 synchronisation"

I was trying to say that the ARM tries to match the x86 in terms of the synchronisation semantics but its not that important so I will remove

midnightveil · 2026-06-15T01:17:25Z

+
+#if defined(CONFIG_ARCH_X86)
+bool is_intel_cpu(void);
+/* On intel x86 if the TSC is not invariant then if the processor is put into sleep states or is overclocked then the


Please keep in mind line length with the comments. Also, this sentence is a bit of a run on sentence.

midnightveil · 2026-06-15T01:17:58Z

+
+#if defined(CONFIG_ARCH_RISCV)
+#define COUNTER_UTIL_MAGIC_LEN 5
+static char COUNTER_UTIL_MAGIC[COUNTER_UTIL_MAGIC_LEN] = { 's', 'D', 'D', 'F', 0x4 };


const static char?

midnightveil · 2026-06-15T01:20:40Z

+{
+    uint64_t v;
+    __asm__ volatile("isb" ::: "memory");
+    __asm__ volatile("mrs %0, cntpct_el0" : "=r"(v)::"memory");


sapa

Suggested change

__asm__ volatile("mrs %0, cntpct_el0" : "=r"(v)::"memory");

asm volatile("mrs %0, cntpct_el0" : "=r"(v) :: "memory");

midnightveil · 2026-06-15T01:21:04Z

+}
+
+#elif defined(CONFIG_ARCH_RISCV)
+__attribute__((__section__(".arch_counter_config"))) arch_counter_config_t arch_counter_config;


This could do with an extra space between this and the following function.

midnightveil · 2026-06-15T01:21:17Z

+static inline bool counter_config_check_magic(void *config)
+{
+    char *magic = (char *)config;
+    for (int i = 0; i < COUNTER_UTIL_MAGIC_LEN; i++) {


Do we not have memcmp?

midnightveil · 2026-06-15T01:22:00Z

+    if (likely(checked_config)) {
+        return arch_counter_config.frequency;
+    }
+
+    if (!counter_config_check_magic(&arch_counter_config)) {
+        LOG_ARCH_COUNTER_ERR("RISC-V requires an arch_counter_config struct\n");
+        return 0;
+    }


if -> else if then a spacing

midnightveil · 2026-06-15T01:23:17Z

+uint64_t read_counter(void);
+/* This may return 0 if frequency was not available */
+uint64_t read_freq(void);


Counter is quite vague: This could be a counter of many things, same with function name: "arch_counter" is vague.

This is a "timestamp counter" .

Also, should we mention which counter this uses on each platform?

Is there a reason why this complete ignores our time functions..?

What time functions are you referring to?

The top-level comment I made as part of the review, i.e. sddf_timer_time_now.

(Is this intended to be exposed as part of the sDDF, or an internal API?)

An internal API.

omeh-a · 2026-06-15T02:41:32Z

(Please see my previous review: can this not be integrated into a time subsystem...?.

Fwiw, this is not strictly needed for right now in my opinion. We have a pending rewrite of our timer subsystem and this would be a necessary component, so I personally am not that bothered by getting this merged as a standalone thing for now since we would be adding it to the timer subsystem later anyway

This commit exposes a common API to interact with physical counters on arm and x86. The x86 logic was introduced to improve the performance of the timer driver see commit #645402620af9bf9a6bf4ef3bd17d5a1077a567b7 on x86. Using these counters provides significant performance benefits in contrast to interacting with an external timer driver. At the bare minimum this change will save thousands of cycles for applications that simply need to track time progression rather than reading a timestamp and or setting time outs, for the reasons discussed in #645402620af9bf9a6bf4ef3bd17d5a1077a567b7 as well as avoiding the overhead of invoking the kernel un-necessarily. Putting this code in a library like this means that applications that are not worried about architecture specific details can interact with the generic read_counter and read_freq functions, avoiding the need for #if defined checks. In addition, applications do not need to repeat the non trivial boiler plate logic on x86 that is required to calculate the tsc frequency. Furthermore for x86 in future if we want to calculate the tsc freq when it is otherwise not available, having a stable API now will avoid having to change more files later. For RISC-V the frequency must be supplied by the user of the library by patching a valid config struct into the elfs that use the library. Failure to do this will likely fault but to ensure this error is caught we check for magic bytes. Signed-off-by: Callum <c.berry@student.unsw.edu.au>

cazb2 requested review from Ivan-Velickovic and dreamliner787-9 April 15, 2026 13:35

cazb2 changed the title ~~User Space Physical Timer Access:~~ User Space Physical Counter Access Apr 15, 2026

cazb2 force-pushed the callumb/user_cnt branch from 0f05c39 to a7d2364 Compare April 15, 2026 20:53

dreamliner787-9 reviewed Apr 15, 2026

View reviewed changes

Comment thread util/tsc.c Outdated

dreamliner787-9 reviewed Apr 15, 2026

View reviewed changes

Comment thread util/tsc.c Outdated

cazb2 force-pushed the callumb/user_cnt branch from a7d2364 to 87a3f2a Compare April 15, 2026 23:47

cazb2 force-pushed the callumb/user_cnt branch from 87a3f2a to 9bb7f0a Compare April 16, 2026 00:16

dreamliner787-9 reviewed Apr 16, 2026

View reviewed changes

midnightveil reviewed Apr 16, 2026

View reviewed changes

Comment thread util/tsc.c Outdated

midnightveil reviewed Apr 16, 2026

View reviewed changes

cazb2 force-pushed the callumb/user_cnt branch from 9bb7f0a to 68e1fb9 Compare April 16, 2026 00:36

cazb2 force-pushed the callumb/user_cnt branch from 68e1fb9 to e4dfa34 Compare April 16, 2026 00:50

cazb2 force-pushed the callumb/user_cnt branch from e4dfa34 to b5eb0db Compare April 16, 2026 01:11

cazb2 force-pushed the callumb/user_cnt branch from b5eb0db to 7fad251 Compare April 16, 2026 01:21

cazb2 force-pushed the callumb/user_cnt branch from 7fad251 to 3135b2b Compare April 16, 2026 02:06

cazb2 force-pushed the callumb/user_cnt branch 4 times, most recently from 9a171f3 to 147542f Compare June 8, 2026 03:03

cazb2 marked this pull request as ready for review June 8, 2026 03:06

KurtWu10 reviewed Jun 8, 2026

View reviewed changes

Comment thread util/arch_counter.c Outdated

cazb2 force-pushed the callumb/user_cnt branch from 147542f to b6b273c Compare June 8, 2026 08:56

KurtWu10 reviewed Jun 8, 2026

View reviewed changes

Comment thread util/tsc.c Outdated

Comment thread util/arch_counter.c Outdated

cazb2 force-pushed the callumb/user_cnt branch 2 times, most recently from 3ba54ac to e3ce899 Compare June 8, 2026 09:40

omeh-a reviewed Jun 12, 2026

View reviewed changes