arch/arm64/kvm/hyp/include/nvhe/clock.h | 8 ++++---- arch/arm64/kvm/hyp/nvhe/clock.c | 4 ++-- arch/arm64/kvm/hyp/nvhe/trace.c | 4 ++-- 3 files changed, 8 insertions(+), 8 deletions(-)
When the hypervisor tracing (CONFIG_NVHE_EL2_TRACING) is disabled, it
defines a static inline stub for trace_clock().
However, trace_clock() is already declared as an extern function in
linux/trace_clock.h which is pulled in EL2 compilation.
If the file <nvhe/clock.h> is included when CONFIG_NVHE_EL2_TRACING
is disabled (by including it manually in setup.c) it will cause:
In file included from arch/arm64/kvm/hyp/nvhe/setup.c:22:
./arch/arm64/kvm/hyp/include/nvhe/clock.h:14:19: error: static declaration of ‘trace_clock’ follows non-static declaration
14 | static inline u64 trace_clock(void) { return 0; }
| ^~~~~~~~~~~
on GCC and a linker error on LLVM (it seems to change the linkage to
global)
Although that is not a problem at the moment, as no other files
include <nvhe/clock.h>. That does not seem to be the intent of
this code and that will cause issues with more users as the SMMUv3
driver.
Signed-off-by: Mostafa Saleh <smostafa@google.com>
--
I did not add Fixes tag as this is currently dormant and not breaking
anything.
---
arch/arm64/kvm/hyp/include/nvhe/clock.h | 8 ++++----
arch/arm64/kvm/hyp/nvhe/clock.c | 4 ++--
arch/arm64/kvm/hyp/nvhe/trace.c | 4 ++--
3 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/arch/arm64/kvm/hyp/include/nvhe/clock.h b/arch/arm64/kvm/hyp/include/nvhe/clock.h
index 9f429f5c0664..c2ccd0e8bf22 100644
--- a/arch/arm64/kvm/hyp/include/nvhe/clock.h
+++ b/arch/arm64/kvm/hyp/include/nvhe/clock.h
@@ -6,11 +6,11 @@
#include <asm/kvm_hyp.h>
#ifdef CONFIG_NVHE_EL2_TRACING
-void trace_clock_update(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc);
-u64 trace_clock(void);
+void hyp_trace_clock_update(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc);
+u64 hyp_trace_clock(void);
#else
static inline void
-trace_clock_update(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc) { }
-static inline u64 trace_clock(void) { return 0; }
+hyp_trace_clock_update(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc) { }
+static inline u64 hyp_trace_clock(void) { return 0; }
#endif
#endif
diff --git a/arch/arm64/kvm/hyp/nvhe/clock.c b/arch/arm64/kvm/hyp/nvhe/clock.c
index a7fc61976fd0..8adefb0b696c 100644
--- a/arch/arm64/kvm/hyp/nvhe/clock.c
+++ b/arch/arm64/kvm/hyp/nvhe/clock.c
@@ -30,7 +30,7 @@ static u64 __clock_mult_uint128(u64 cyc, u32 mult, u32 shift)
}
/* Does not guarantee no reader on the modified bank. */
-void trace_clock_update(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc)
+void hyp_trace_clock_update(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc)
{
struct clock_data *clock = &trace_clock_data;
u64 bank = clock->cur ^ 1;
@@ -48,7 +48,7 @@ void trace_clock_update(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc)
}
/* Use untrusted host data */
-u64 trace_clock(void)
+u64 hyp_trace_clock(void)
{
struct clock_data *clock = &trace_clock_data;
u64 bank = smp_load_acquire(&clock->cur);
diff --git a/arch/arm64/kvm/hyp/nvhe/trace.c b/arch/arm64/kvm/hyp/nvhe/trace.c
index e7e150ab265f..f2a32463c6c2 100644
--- a/arch/arm64/kvm/hyp/nvhe/trace.c
+++ b/arch/arm64/kvm/hyp/nvhe/trace.c
@@ -35,7 +35,7 @@ static bool hyp_trace_buffer_loaded(struct hyp_trace_buffer *trace_buffer)
void *tracing_reserve_entry(unsigned long length)
{
return simple_ring_buffer_reserve(this_cpu_ptr(trace_buffer.simple_rbs), length,
- trace_clock());
+ hyp_trace_clock());
}
void tracing_commit_entry(void)
@@ -290,7 +290,7 @@ void __tracing_update_clock(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc)
}
/* ...we can now override the old one and swap. */
- trace_clock_update(mult, shift, epoch_ns, epoch_cyc);
+ hyp_trace_clock_update(mult, shift, epoch_ns, epoch_cyc);
}
int __tracing_reset(unsigned int cpu)
--
2.55.0.795.g602f6c329a-googOn Sun, Jul 12, 2026 at 03:38:35PM +0000, Mostafa Saleh wrote:
> When the hypervisor tracing (CONFIG_NVHE_EL2_TRACING) is disabled, it
> defines a static inline stub for trace_clock().
>
> However, trace_clock() is already declared as an extern function in
> linux/trace_clock.h which is pulled in EL2 compilation.
I am completely unable to reproduce that. CONFIG_NVHE_EL2_TRACING, isn't a
selectable option, it depends on NVHE_EL2_DEBUG.
How does your .config look like?
>
> If the file <nvhe/clock.h> is included when CONFIG_NVHE_EL2_TRACING
> is disabled (by including it manually in setup.c) it will cause:
> In file included from arch/arm64/kvm/hyp/nvhe/setup.c:22:
nvhe/clock.h seems already included in setup.c
>
> ./arch/arm64/kvm/hyp/include/nvhe/clock.h:14:19: error: static declaration of ‘trace_clock’ follows non-static declaration
>
> 14 | static inline u64 trace_clock(void) { return 0; }
>
> | ^~~~~~~~~~~
>
> on GCC and a linker error on LLVM (it seems to change the linkage to
> global)
>
> Although that is not a problem at the moment, as no other files
> include <nvhe/clock.h>. That does not seem to be the intent of
> this code and that will cause issues with more users as the SMMUv3
> driver.
>
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
>
> --
> I did not add Fixes tag as this is currently dormant and not breaking
> anything.
> ---
> arch/arm64/kvm/hyp/include/nvhe/clock.h | 8 ++++----
> arch/arm64/kvm/hyp/nvhe/clock.c | 4 ++--
> arch/arm64/kvm/hyp/nvhe/trace.c | 4 ++--
> 3 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm64/kvm/hyp/include/nvhe/clock.h b/arch/arm64/kvm/hyp/include/nvhe/clock.h
> index 9f429f5c0664..c2ccd0e8bf22 100644
> --- a/arch/arm64/kvm/hyp/include/nvhe/clock.h
> +++ b/arch/arm64/kvm/hyp/include/nvhe/clock.h
> @@ -6,11 +6,11 @@
> #include <asm/kvm_hyp.h>
>
> #ifdef CONFIG_NVHE_EL2_TRACING
> -void trace_clock_update(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc);
> -u64 trace_clock(void);
> +void hyp_trace_clock_update(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc);
> +u64 hyp_trace_clock(void);
> #else
> static inline void
> -trace_clock_update(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc) { }
> -static inline u64 trace_clock(void) { return 0; }
> +hyp_trace_clock_update(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc) { }
> +static inline u64 hyp_trace_clock(void) { return 0; }
> #endif
> #endif
> diff --git a/arch/arm64/kvm/hyp/nvhe/clock.c b/arch/arm64/kvm/hyp/nvhe/clock.c
> index a7fc61976fd0..8adefb0b696c 100644
> --- a/arch/arm64/kvm/hyp/nvhe/clock.c
> +++ b/arch/arm64/kvm/hyp/nvhe/clock.c
> @@ -30,7 +30,7 @@ static u64 __clock_mult_uint128(u64 cyc, u32 mult, u32 shift)
> }
>
> /* Does not guarantee no reader on the modified bank. */
> -void trace_clock_update(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc)
> +void hyp_trace_clock_update(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc)
> {
> struct clock_data *clock = &trace_clock_data;
> u64 bank = clock->cur ^ 1;
> @@ -48,7 +48,7 @@ void trace_clock_update(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc)
> }
>
> /* Use untrusted host data */
> -u64 trace_clock(void)
> +u64 hyp_trace_clock(void)
> {
> struct clock_data *clock = &trace_clock_data;
> u64 bank = smp_load_acquire(&clock->cur);
> diff --git a/arch/arm64/kvm/hyp/nvhe/trace.c b/arch/arm64/kvm/hyp/nvhe/trace.c
> index e7e150ab265f..f2a32463c6c2 100644
> --- a/arch/arm64/kvm/hyp/nvhe/trace.c
> +++ b/arch/arm64/kvm/hyp/nvhe/trace.c
> @@ -35,7 +35,7 @@ static bool hyp_trace_buffer_loaded(struct hyp_trace_buffer *trace_buffer)
> void *tracing_reserve_entry(unsigned long length)
> {
> return simple_ring_buffer_reserve(this_cpu_ptr(trace_buffer.simple_rbs), length,
> - trace_clock());
> + hyp_trace_clock());
> }
>
> void tracing_commit_entry(void)
> @@ -290,7 +290,7 @@ void __tracing_update_clock(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc)
> }
>
> /* ...we can now override the old one and swap. */
> - trace_clock_update(mult, shift, epoch_ns, epoch_cyc);
> + hyp_trace_clock_update(mult, shift, epoch_ns, epoch_cyc);
> }
>
> int __tracing_reset(unsigned int cpu)
> --
> 2.55.0.795.g602f6c329a-goog
>
On Mon, Jul 13, 2026 at 09:02:48AM +0100, Vincent Donnefort wrote:
> On Sun, Jul 12, 2026 at 03:38:35PM +0000, Mostafa Saleh wrote:
> > When the hypervisor tracing (CONFIG_NVHE_EL2_TRACING) is disabled, it
> > defines a static inline stub for trace_clock().
> >
> > However, trace_clock() is already declared as an extern function in
> > linux/trace_clock.h which is pulled in EL2 compilation.
>
> I am completely unable to reproduce that. CONFIG_NVHE_EL2_TRACING, isn't a
> selectable option, it depends on NVHE_EL2_DEBUG.
>
> How does your .config look like?
>
> >
> > If the file <nvhe/clock.h> is included when CONFIG_NVHE_EL2_TRACING
> > is disabled (by including it manually in setup.c) it will cause:
> > In file included from arch/arm64/kvm/hyp/nvhe/setup.c:22:
>
> nvhe/clock.h seems already included in setup.c
Ha no appologies, I was looking at the wrong branch.
However I am still enable to reproduce this issue. So I am still interested in
knowing your .defconfig
>
> >
> > ./arch/arm64/kvm/hyp/include/nvhe/clock.h:14:19: error: static declaration of ‘trace_clock’ follows non-static declaration
> >
> > 14 | static inline u64 trace_clock(void) { return 0; }
> >
> > | ^~~~~~~~~~~
> >
> > on GCC and a linker error on LLVM (it seems to change the linkage to
> > global)
> >
> > Although that is not a problem at the moment, as no other files
> > include <nvhe/clock.h>. That does not seem to be the intent of
> > this code and that will cause issues with more users as the SMMUv3
> > driver.
> >
> > Signed-off-by: Mostafa Saleh <smostafa@google.com>
> >
> > --
[...]
On Mon, Jul 13, 2026 at 09:05:55AM +0100, Vincent Donnefort wrote: > On Mon, Jul 13, 2026 at 09:02:48AM +0100, Vincent Donnefort wrote: > > On Sun, Jul 12, 2026 at 03:38:35PM +0000, Mostafa Saleh wrote: > > > When the hypervisor tracing (CONFIG_NVHE_EL2_TRACING) is disabled, it > > > defines a static inline stub for trace_clock(). > > > > > > However, trace_clock() is already declared as an extern function in > > > linux/trace_clock.h which is pulled in EL2 compilation. > > > > I am completely unable to reproduce that. CONFIG_NVHE_EL2_TRACING, isn't a > > selectable option, it depends on NVHE_EL2_DEBUG. > > > > How does your .config look like? > > > > > > > > If the file <nvhe/clock.h> is included when CONFIG_NVHE_EL2_TRACING > > > is disabled (by including it manually in setup.c) it will cause: > > > In file included from arch/arm64/kvm/hyp/nvhe/setup.c:22: > > > > nvhe/clock.h seems already included in setup.c > > Ha no appologies, I was looking at the wrong branch. > > However I am still enable to reproduce this issue. So I am still interested in > knowing your .defconfig It is just defconfig (CONFIG_NVHE_EL2_TRACING is already disabled). If you attempt to include <nvhe/clock.h> in another file: diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c index d3c69de698f4..ca91e453fb10 100644 --- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c +++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c @@ -15,6 +15,7 @@ #include <asm/kvm_hypevents.h> #include <asm/kvm_mmu.h> +#include <nvhe/clock.h> #include <nvhe/ffa.h> #include <nvhe/mem_protect.h> #include <nvhe/mm.h> With GCC: make ARCH=arm64 CROSS_COMPILE="aarch64-linux-gnu-" -j`nproc` It will show the above error. With LLVM, you have to include it in more than one file, which results in: >>> defined at clock.h:14 (./arch/arm64/kvm/hyp/include/nvhe/clock.h:14) >>> arch/arm64/kvm/hyp/nvhe/hyp-main.nvhe.o:(trace_clock) >>> defined at clock.h:14 (./arch/arm64/kvm/hyp/include/nvhe/clock.h:14) >>> arch/arm64/kvm/hyp/nvhe/mem_protect.nvhe.o:(.text+0x0) >>> As the compiler would convert the function global. Thanks, Mostafa
Hi Mostafa,
On Sun, 12 Jul 2026 at 16:38, Mostafa Saleh <smostafa@google.com> wrote:
>
> When the hypervisor tracing (CONFIG_NVHE_EL2_TRACING) is disabled, it
> defines a static inline stub for trace_clock().
>
> However, trace_clock() is already declared as an extern function in
> linux/trace_clock.h which is pulled in EL2 compilation.
The rename itself looks correct and complete: all three sites (the
declarations and stubs in nvhe/clock.h, the definitions in clock.c,
and the two callers in trace.c) are updated, and no other reference to
the old names remains under arch/arm64/kvm.
Two small things...
>
> If the file <nvhe/clock.h> is included when CONFIG_NVHE_EL2_TRACING
> is disabled (by including it manually in setup.c) it will cause:
> In file included from arch/arm64/kvm/hyp/nvhe/setup.c:22:
>
> ./arch/arm64/kvm/hyp/include/nvhe/clock.h:14:19: error: static declaration of ‘trace_clock’ follows non-static declaration
>
> 14 | static inline u64 trace_clock(void) { return 0; }
>
> | ^~~~~~~~~~~
>
> on GCC and a linker error on LLVM (it seems to change the linkage to
> global)
>
> Although that is not a problem at the moment, as no other files
> include <nvhe/clock.h>. That does not seem to be the intent of
> this code and that will cause issues with more users as the SMMUv3
> driver.
>
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
>
> --
git am only treats a three-dash "---" line as the scissors; the "--"
here is two dashes, so mailinfo keeps everything above the diffstat.
Worth a "---" on the repost so the note stays out of the log.
> I did not add Fixes tag as this is currently dormant and not breaking
> anything.
> ---
> arch/arm64/kvm/hyp/include/nvhe/clock.h | 8 ++++----
> arch/arm64/kvm/hyp/nvhe/clock.c | 4 ++--
> arch/arm64/kvm/hyp/nvhe/trace.c | 4 ++--
> 3 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm64/kvm/hyp/include/nvhe/clock.h b/arch/arm64/kvm/hyp/include/nvhe/clock.h
> index 9f429f5c0664..c2ccd0e8bf22 100644
> --- a/arch/arm64/kvm/hyp/include/nvhe/clock.h
> +++ b/arch/arm64/kvm/hyp/include/nvhe/clock.h
> @@ -6,11 +6,11 @@
> #include <asm/kvm_hyp.h>
>
> #ifdef CONFIG_NVHE_EL2_TRACING
> -void trace_clock_update(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc);
> -u64 trace_clock(void);
> +void hyp_trace_clock_update(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc);
> +u64 hyp_trace_clock(void);
hyp_trace_clock overlaps the host side: arch/arm64/kvm/hyp_trace.c
already has a struct hyp_trace_clock and static helpers
hyp_trace_clock_enable() / hyp_trace_clock_show() for the debugfs view
of the same clock. No actual collision, so this is only a readability
point, but a reader grepping hyp_trace_clock now gets two unrelated
things. Maybe you'd want to consider a different name, but naming is
hard :)
Reviewed-by: Fuad Tabba <fuad.tabba@linux.dev>
Tested-by: Fuad Tabba < fuad.tabba@linux.dev>
Test: builds fine with the different config enables.
Cheers,
/fuad
> #else
> static inline void
> -trace_clock_update(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc) { }
> -static inline u64 trace_clock(void) { return 0; }
> +hyp_trace_clock_update(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc) { }
> +static inline u64 hyp_trace_clock(void) { return 0; }
> #endif
> #endif
> diff --git a/arch/arm64/kvm/hyp/nvhe/clock.c b/arch/arm64/kvm/hyp/nvhe/clock.c
> index a7fc61976fd0..8adefb0b696c 100644
> --- a/arch/arm64/kvm/hyp/nvhe/clock.c
> +++ b/arch/arm64/kvm/hyp/nvhe/clock.c
> @@ -30,7 +30,7 @@ static u64 __clock_mult_uint128(u64 cyc, u32 mult, u32 shift)
> }
>
> /* Does not guarantee no reader on the modified bank. */
> -void trace_clock_update(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc)
> +void hyp_trace_clock_update(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc)
> {
> struct clock_data *clock = &trace_clock_data;
> u64 bank = clock->cur ^ 1;
> @@ -48,7 +48,7 @@ void trace_clock_update(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc)
> }
>
> /* Use untrusted host data */
> -u64 trace_clock(void)
> +u64 hyp_trace_clock(void)
> {
> struct clock_data *clock = &trace_clock_data;
> u64 bank = smp_load_acquire(&clock->cur);
> diff --git a/arch/arm64/kvm/hyp/nvhe/trace.c b/arch/arm64/kvm/hyp/nvhe/trace.c
> index e7e150ab265f..f2a32463c6c2 100644
> --- a/arch/arm64/kvm/hyp/nvhe/trace.c
> +++ b/arch/arm64/kvm/hyp/nvhe/trace.c
> @@ -35,7 +35,7 @@ static bool hyp_trace_buffer_loaded(struct hyp_trace_buffer *trace_buffer)
> void *tracing_reserve_entry(unsigned long length)
> {
> return simple_ring_buffer_reserve(this_cpu_ptr(trace_buffer.simple_rbs), length,
> - trace_clock());
> + hyp_trace_clock());
> }
>
> void tracing_commit_entry(void)
> @@ -290,7 +290,7 @@ void __tracing_update_clock(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc)
> }
>
> /* ...we can now override the old one and swap. */
> - trace_clock_update(mult, shift, epoch_ns, epoch_cyc);
> + hyp_trace_clock_update(mult, shift, epoch_ns, epoch_cyc);
> }
>
> int __tracing_reset(unsigned int cpu)
> --
> 2.55.0.795.g602f6c329a-goog
>
Hi Fuad,
On Sun, Jul 12, 2026 at 05:19:22PM +0100, Fuad Tabba wrote:
> Hi Mostafa,
>
> On Sun, 12 Jul 2026 at 16:38, Mostafa Saleh <smostafa@google.com> wrote:
> >
> > When the hypervisor tracing (CONFIG_NVHE_EL2_TRACING) is disabled, it
> > defines a static inline stub for trace_clock().
> >
> > However, trace_clock() is already declared as an extern function in
> > linux/trace_clock.h which is pulled in EL2 compilation.
>
> The rename itself looks correct and complete: all three sites (the
> declarations and stubs in nvhe/clock.h, the definitions in clock.c,
> and the two callers in trace.c) are updated, and no other reference to
> the old names remains under arch/arm64/kvm.
>
> Two small things...
>
> >
> > If the file <nvhe/clock.h> is included when CONFIG_NVHE_EL2_TRACING
> > is disabled (by including it manually in setup.c) it will cause:
> > In file included from arch/arm64/kvm/hyp/nvhe/setup.c:22:
> >
> > ./arch/arm64/kvm/hyp/include/nvhe/clock.h:14:19: error: static declaration of ‘trace_clock’ follows non-static declaration
> >
> > 14 | static inline u64 trace_clock(void) { return 0; }
> >
> > | ^~~~~~~~~~~
> >
> > on GCC and a linker error on LLVM (it seems to change the linkage to
> > global)
> >
> > Although that is not a problem at the moment, as no other files
> > include <nvhe/clock.h>. That does not seem to be the intent of
> > this code and that will cause issues with more users as the SMMUv3
> > driver.
> >
> > Signed-off-by: Mostafa Saleh <smostafa@google.com>
> >
> > --
>
> git am only treats a three-dash "---" line as the scissors; the "--"
> here is two dashes, so mailinfo keeps everything above the diffstat.
> Worth a "---" on the repost so the note stays out of the log.
>
ops, I will fix that.
> > I did not add Fixes tag as this is currently dormant and not breaking
> > anything.
> > ---
> > arch/arm64/kvm/hyp/include/nvhe/clock.h | 8 ++++----
> > arch/arm64/kvm/hyp/nvhe/clock.c | 4 ++--
> > arch/arm64/kvm/hyp/nvhe/trace.c | 4 ++--
> > 3 files changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/hyp/include/nvhe/clock.h b/arch/arm64/kvm/hyp/include/nvhe/clock.h
> > index 9f429f5c0664..c2ccd0e8bf22 100644
> > --- a/arch/arm64/kvm/hyp/include/nvhe/clock.h
> > +++ b/arch/arm64/kvm/hyp/include/nvhe/clock.h
> > @@ -6,11 +6,11 @@
> > #include <asm/kvm_hyp.h>
> >
> > #ifdef CONFIG_NVHE_EL2_TRACING
> > -void trace_clock_update(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc);
> > -u64 trace_clock(void);
> > +void hyp_trace_clock_update(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc);
> > +u64 hyp_trace_clock(void);
>
> hyp_trace_clock overlaps the host side: arch/arm64/kvm/hyp_trace.c
> already has a struct hyp_trace_clock and static helpers
> hyp_trace_clock_enable() / hyp_trace_clock_show() for the debugfs view
> of the same clock. No actual collision, so this is only a readability
> point, but a reader grepping hyp_trace_clock now gets two unrelated
> things. Maybe you'd want to consider a different name, but naming is
> hard :)
Yes, that shouldn't be a problem, I just added hyp_ prefix, but I
am ok with any suggestions!
Thanks,
Mostafa
>
> Reviewed-by: Fuad Tabba <fuad.tabba@linux.dev>
> Tested-by: Fuad Tabba < fuad.tabba@linux.dev>
>
> Test: builds fine with the different config enables.
>
> Cheers,
> /fuad
On Sun, 12 Jul 2026 at 20:19, Mostafa Saleh <smostafa@google.com> wrote:
>
> Hi Fuad,
>
> On Sun, Jul 12, 2026 at 05:19:22PM +0100, Fuad Tabba wrote:
> > Hi Mostafa,
> >
> > On Sun, 12 Jul 2026 at 16:38, Mostafa Saleh <smostafa@google.com> wrote:
> > >
> > > When the hypervisor tracing (CONFIG_NVHE_EL2_TRACING) is disabled, it
> > > defines a static inline stub for trace_clock().
> > >
> > > However, trace_clock() is already declared as an extern function in
> > > linux/trace_clock.h which is pulled in EL2 compilation.
> >
> > The rename itself looks correct and complete: all three sites (the
> > declarations and stubs in nvhe/clock.h, the definitions in clock.c,
> > and the two callers in trace.c) are updated, and no other reference to
> > the old names remains under arch/arm64/kvm.
> >
> > Two small things...
> >
> > >
> > > If the file <nvhe/clock.h> is included when CONFIG_NVHE_EL2_TRACING
> > > is disabled (by including it manually in setup.c) it will cause:
> > > In file included from arch/arm64/kvm/hyp/nvhe/setup.c:22:
> > >
> > > ./arch/arm64/kvm/hyp/include/nvhe/clock.h:14:19: error: static declaration of ‘trace_clock’ follows non-static declaration
> > >
> > > 14 | static inline u64 trace_clock(void) { return 0; }
> > >
> > > | ^~~~~~~~~~~
> > >
> > > on GCC and a linker error on LLVM (it seems to change the linkage to
> > > global)
> > >
> > > Although that is not a problem at the moment, as no other files
> > > include <nvhe/clock.h>. That does not seem to be the intent of
> > > this code and that will cause issues with more users as the SMMUv3
> > > driver.
> > >
> > > Signed-off-by: Mostafa Saleh <smostafa@google.com>
> > >
> > > --
> >
> > git am only treats a three-dash "---" line as the scissors; the "--"
> > here is two dashes, so mailinfo keeps everything above the diffstat.
> > Worth a "---" on the repost so the note stays out of the log.
> >
>
> ops, I will fix that.
>
>
> > > I did not add Fixes tag as this is currently dormant and not breaking
> > > anything.
> > > ---
> > > arch/arm64/kvm/hyp/include/nvhe/clock.h | 8 ++++----
> > > arch/arm64/kvm/hyp/nvhe/clock.c | 4 ++--
> > > arch/arm64/kvm/hyp/nvhe/trace.c | 4 ++--
> > > 3 files changed, 8 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/arch/arm64/kvm/hyp/include/nvhe/clock.h b/arch/arm64/kvm/hyp/include/nvhe/clock.h
> > > index 9f429f5c0664..c2ccd0e8bf22 100644
> > > --- a/arch/arm64/kvm/hyp/include/nvhe/clock.h
> > > +++ b/arch/arm64/kvm/hyp/include/nvhe/clock.h
> > > @@ -6,11 +6,11 @@
> > > #include <asm/kvm_hyp.h>
> > >
> > > #ifdef CONFIG_NVHE_EL2_TRACING
> > > -void trace_clock_update(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc);
> > > -u64 trace_clock(void);
> > > +void hyp_trace_clock_update(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc);
> > > +u64 hyp_trace_clock(void);
> >
> > hyp_trace_clock overlaps the host side: arch/arm64/kvm/hyp_trace.c
> > already has a struct hyp_trace_clock and static helpers
> > hyp_trace_clock_enable() / hyp_trace_clock_show() for the debugfs view
> > of the same clock. No actual collision, so this is only a readability
> > point, but a reader grepping hyp_trace_clock now gets two unrelated
> > things. Maybe you'd want to consider a different name, but naming is
> > hard :)
>
> Yes, that shouldn't be a problem, I just added hyp_ prefix, but I
> am ok with any suggestions!
Didn't I say naming is hard? :) How about, el2_? Not really happy with
that either tbh... but can't think of a better one...
/fuad
>
> Thanks,
> Mostafa
>
> >
> > Reviewed-by: Fuad Tabba <fuad.tabba@linux.dev>
> > Tested-by: Fuad Tabba < fuad.tabba@linux.dev>
> >
> > Test: builds fine with the different config enables.
> >
> > Cheers,
> > /fuad
[...] > > > > > > I did not add Fixes tag as this is currently dormant and not breaking > > > > anything. > > > > --- > > > > arch/arm64/kvm/hyp/include/nvhe/clock.h | 8 ++++---- > > > > arch/arm64/kvm/hyp/nvhe/clock.c | 4 ++-- > > > > arch/arm64/kvm/hyp/nvhe/trace.c | 4 ++-- > > > > 3 files changed, 8 insertions(+), 8 deletions(-) > > > > > > > > diff --git a/arch/arm64/kvm/hyp/include/nvhe/clock.h b/arch/arm64/kvm/hyp/include/nvhe/clock.h > > > > index 9f429f5c0664..c2ccd0e8bf22 100644 > > > > --- a/arch/arm64/kvm/hyp/include/nvhe/clock.h > > > > +++ b/arch/arm64/kvm/hyp/include/nvhe/clock.h > > > > @@ -6,11 +6,11 @@ > > > > #include <asm/kvm_hyp.h> > > > > > > > > #ifdef CONFIG_NVHE_EL2_TRACING > > > > -void trace_clock_update(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc); > > > > -u64 trace_clock(void); > > > > +void hyp_trace_clock_update(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc); > > > > +u64 hyp_trace_clock(void); > > > > > > hyp_trace_clock overlaps the host side: arch/arm64/kvm/hyp_trace.c > > > already has a struct hyp_trace_clock and static helpers > > > hyp_trace_clock_enable() / hyp_trace_clock_show() for the debugfs view > > > of the same clock. No actual collision, so this is only a readability > > > point, but a reader grepping hyp_trace_clock now gets two unrelated > > > things. Maybe you'd want to consider a different name, but naming is > > > hard :) If we were to rename: the clock is a "hyp_clock" so probably trace_hyp_clock() is the right thing here. while the "hyp_trace_" is the prefix for hyp_trace.c file content. Although I would like to see why we pull trace_clock() from the kernel into EL2. That bit sounds wrong and if we have a way around perhaps that's better? > > > > Yes, that shouldn't be a problem, I just added hyp_ prefix, but I > > am ok with any suggestions! > > Didn't I say naming is hard? :) How about, el2_? Not really happy with > that either tbh... but can't think of a better one... > > /fuad > > > > > Thanks, > > Mostafa > > > > > > > > Reviewed-by: Fuad Tabba <fuad.tabba@linux.dev> > > > Tested-by: Fuad Tabba < fuad.tabba@linux.dev> > > > > > > Test: builds fine with the different config enables. > > > > > > Cheers, > > > /fuad
On Mon, Jul 13, 2026 at 09:11:27AM +0100, Vincent Donnefort wrote:
> [...]
>
> > >
> > > > > I did not add Fixes tag as this is currently dormant and not breaking
> > > > > anything.
> > > > > ---
> > > > > arch/arm64/kvm/hyp/include/nvhe/clock.h | 8 ++++----
> > > > > arch/arm64/kvm/hyp/nvhe/clock.c | 4 ++--
> > > > > arch/arm64/kvm/hyp/nvhe/trace.c | 4 ++--
> > > > > 3 files changed, 8 insertions(+), 8 deletions(-)
> > > > >
> > > > > diff --git a/arch/arm64/kvm/hyp/include/nvhe/clock.h b/arch/arm64/kvm/hyp/include/nvhe/clock.h
> > > > > index 9f429f5c0664..c2ccd0e8bf22 100644
> > > > > --- a/arch/arm64/kvm/hyp/include/nvhe/clock.h
> > > > > +++ b/arch/arm64/kvm/hyp/include/nvhe/clock.h
> > > > > @@ -6,11 +6,11 @@
> > > > > #include <asm/kvm_hyp.h>
> > > > >
> > > > > #ifdef CONFIG_NVHE_EL2_TRACING
> > > > > -void trace_clock_update(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc);
> > > > > -u64 trace_clock(void);
> > > > > +void hyp_trace_clock_update(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc);
> > > > > +u64 hyp_trace_clock(void);
> > > >
> > > > hyp_trace_clock overlaps the host side: arch/arm64/kvm/hyp_trace.c
> > > > already has a struct hyp_trace_clock and static helpers
> > > > hyp_trace_clock_enable() / hyp_trace_clock_show() for the debugfs view
> > > > of the same clock. No actual collision, so this is only a readability
> > > > point, but a reader grepping hyp_trace_clock now gets two unrelated
> > > > things. Maybe you'd want to consider a different name, but naming is
> > > > hard :)
>
> If we were to rename: the clock is a "hyp_clock" so probably trace_hyp_clock()
> is the right thing here.
>
> while the "hyp_trace_" is the prefix for hyp_trace.c file content.
>
> Although I would like to see why we pull trace_clock() from the kernel into EL2.
> That bit sounds wrong and if we have a way around perhaps that's better?
Most include path look like this:
In file included from ./include/linux/ftrace.h:11,
from ./include/linux/kprobes.h:28,
from ./include/linux/kgdb.h:17,
from ./arch/arm64/include/asm/cacheflush.h:11,
from ./include/linux/cacheflush.h:5,
from ./include/linux/highmem.h:8,
from ./include/linux/bvec.h:10,
from ./include/linux/blk_types.h:10,
from ./include/linux/writeback.h:13,
from ./include/linux/memcontrol.h:23,
from ./include/linux/resume_user_mode.h:8,
from ./include/linux/entry-virt.h:6,
from ./include/linux/kvm_host.h:5,
Although, some files have ./arch/arm64/include/asm/cacheflush.h
directly.
Thanks,
Mostafa
>
> > >
> > > Yes, that shouldn't be a problem, I just added hyp_ prefix, but I
> > > am ok with any suggestions!
> >
> > Didn't I say naming is hard? :) How about, el2_? Not really happy with
> > that either tbh... but can't think of a better one...
> >
> > /fuad
> >
> > >
> > > Thanks,
> > > Mostafa
> > >
> > > >
> > > > Reviewed-by: Fuad Tabba <fuad.tabba@linux.dev>
> > > > Tested-by: Fuad Tabba < fuad.tabba@linux.dev>
> > > >
> > > > Test: builds fine with the different config enables.
> > > >
> > > > Cheers,
> > > > /fuad
On Mon, Jul 13, 2026 at 08:26:35AM +0000, Mostafa Saleh wrote: > On Mon, Jul 13, 2026 at 09:11:27AM +0100, Vincent Donnefort wrote: > > [...] > > > > > > > > > > > > I did not add Fixes tag as this is currently dormant and not breaking > > > > > > anything. > > > > > > --- > > > > > > arch/arm64/kvm/hyp/include/nvhe/clock.h | 8 ++++---- > > > > > > arch/arm64/kvm/hyp/nvhe/clock.c | 4 ++-- > > > > > > arch/arm64/kvm/hyp/nvhe/trace.c | 4 ++-- > > > > > > 3 files changed, 8 insertions(+), 8 deletions(-) > > > > > > > > > > > > diff --git a/arch/arm64/kvm/hyp/include/nvhe/clock.h b/arch/arm64/kvm/hyp/include/nvhe/clock.h > > > > > > index 9f429f5c0664..c2ccd0e8bf22 100644 > > > > > > --- a/arch/arm64/kvm/hyp/include/nvhe/clock.h > > > > > > +++ b/arch/arm64/kvm/hyp/include/nvhe/clock.h > > > > > > @@ -6,11 +6,11 @@ > > > > > > #include <asm/kvm_hyp.h> > > > > > > > > > > > > #ifdef CONFIG_NVHE_EL2_TRACING > > > > > > -void trace_clock_update(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc); > > > > > > -u64 trace_clock(void); > > > > > > +void hyp_trace_clock_update(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc); > > > > > > +u64 hyp_trace_clock(void); > > > > > > > > > > hyp_trace_clock overlaps the host side: arch/arm64/kvm/hyp_trace.c > > > > > already has a struct hyp_trace_clock and static helpers > > > > > hyp_trace_clock_enable() / hyp_trace_clock_show() for the debugfs view > > > > > of the same clock. No actual collision, so this is only a readability > > > > > point, but a reader grepping hyp_trace_clock now gets two unrelated > > > > > things. Maybe you'd want to consider a different name, but naming is > > > > > hard :) > > > > If we were to rename: the clock is a "hyp_clock" so probably trace_hyp_clock() > > is the right thing here. > > > > while the "hyp_trace_" is the prefix for hyp_trace.c file content. > > > > Although I would like to see why we pull trace_clock() from the kernel into EL2. > > That bit sounds wrong and if we have a way around perhaps that's better? > > Most include path look like this: > In file included from ./include/linux/ftrace.h:11, > from ./include/linux/kprobes.h:28, > from ./include/linux/kgdb.h:17, > from ./arch/arm64/include/asm/cacheflush.h:11, > from ./include/linux/cacheflush.h:5, > from ./include/linux/highmem.h:8, > from ./include/linux/bvec.h:10, > from ./include/linux/blk_types.h:10, > from ./include/linux/writeback.h:13, > from ./include/linux/memcontrol.h:23, > from ./include/linux/resume_user_mode.h:8, > from ./include/linux/entry-virt.h:6, > from ./include/linux/kvm_host.h:5, > > Although, some files have ./arch/arm64/include/asm/cacheflush.h > directly. Ha sad, no way around that... So about trace_hyp_clock() / trace_hyp_clock_update() ? > > Thanks, > Mostafa > > > > > > > > > > > Yes, that shouldn't be a problem, I just added hyp_ prefix, but I > > > > am ok with any suggestions! > > > > > > Didn't I say naming is hard? :) How about, el2_? Not really happy with > > > that either tbh... but can't think of a better one... > > > > > > /fuad > > > > > > > > > > > Thanks, > > > > Mostafa > > > > > > > > > > > > > > Reviewed-by: Fuad Tabba <fuad.tabba@linux.dev> > > > > > Tested-by: Fuad Tabba < fuad.tabba@linux.dev> > > > > > > > > > > Test: builds fine with the different config enables. > > > > > > > > > > Cheers, > > > > > /fuad
On Mon, Jul 13, 2026 at 09:34:32AM +0100, Vincent Donnefort wrote: > On Mon, Jul 13, 2026 at 08:26:35AM +0000, Mostafa Saleh wrote: > > On Mon, Jul 13, 2026 at 09:11:27AM +0100, Vincent Donnefort wrote: > > > [...] > > > > > > > > > > > > > > > I did not add Fixes tag as this is currently dormant and not breaking > > > > > > > anything. > > > > > > > --- > > > > > > > arch/arm64/kvm/hyp/include/nvhe/clock.h | 8 ++++---- > > > > > > > arch/arm64/kvm/hyp/nvhe/clock.c | 4 ++-- > > > > > > > arch/arm64/kvm/hyp/nvhe/trace.c | 4 ++-- > > > > > > > 3 files changed, 8 insertions(+), 8 deletions(-) > > > > > > > > > > > > > > diff --git a/arch/arm64/kvm/hyp/include/nvhe/clock.h b/arch/arm64/kvm/hyp/include/nvhe/clock.h > > > > > > > index 9f429f5c0664..c2ccd0e8bf22 100644 > > > > > > > --- a/arch/arm64/kvm/hyp/include/nvhe/clock.h > > > > > > > +++ b/arch/arm64/kvm/hyp/include/nvhe/clock.h > > > > > > > @@ -6,11 +6,11 @@ > > > > > > > #include <asm/kvm_hyp.h> > > > > > > > > > > > > > > #ifdef CONFIG_NVHE_EL2_TRACING > > > > > > > -void trace_clock_update(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc); > > > > > > > -u64 trace_clock(void); > > > > > > > +void hyp_trace_clock_update(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc); > > > > > > > +u64 hyp_trace_clock(void); > > > > > > > > > > > > hyp_trace_clock overlaps the host side: arch/arm64/kvm/hyp_trace.c > > > > > > already has a struct hyp_trace_clock and static helpers > > > > > > hyp_trace_clock_enable() / hyp_trace_clock_show() for the debugfs view > > > > > > of the same clock. No actual collision, so this is only a readability > > > > > > point, but a reader grepping hyp_trace_clock now gets two unrelated > > > > > > things. Maybe you'd want to consider a different name, but naming is > > > > > > hard :) > > > > > > If we were to rename: the clock is a "hyp_clock" so probably trace_hyp_clock() > > > is the right thing here. > > > > > > while the "hyp_trace_" is the prefix for hyp_trace.c file content. > > > > > > Although I would like to see why we pull trace_clock() from the kernel into EL2. > > > That bit sounds wrong and if we have a way around perhaps that's better? > > > > Most include path look like this: > > In file included from ./include/linux/ftrace.h:11, > > from ./include/linux/kprobes.h:28, > > from ./include/linux/kgdb.h:17, > > from ./arch/arm64/include/asm/cacheflush.h:11, > > from ./include/linux/cacheflush.h:5, > > from ./include/linux/highmem.h:8, > > from ./include/linux/bvec.h:10, > > from ./include/linux/blk_types.h:10, > > from ./include/linux/writeback.h:13, > > from ./include/linux/memcontrol.h:23, > > from ./include/linux/resume_user_mode.h:8, > > from ./include/linux/entry-virt.h:6, > > from ./include/linux/kvm_host.h:5, > > > > Although, some files have ./arch/arm64/include/asm/cacheflush.h > > directly. > > Ha sad, no way around that... > > So about trace_hyp_clock() / trace_hyp_clock_update() ? > Makes sense, I will respin with that. Thanks, Mostafa
On Mon, Jul 13, 2026 at 08:40:58AM +0000, Mostafa Saleh wrote: > On Mon, Jul 13, 2026 at 09:34:32AM +0100, Vincent Donnefort wrote: > > On Mon, Jul 13, 2026 at 08:26:35AM +0000, Mostafa Saleh wrote: > > > On Mon, Jul 13, 2026 at 09:11:27AM +0100, Vincent Donnefort wrote: > > > > [...] > > > > > > > > > > > > > > > > > > I did not add Fixes tag as this is currently dormant and not breaking > > > > > > > > anything. > > > > > > > > --- > > > > > > > > arch/arm64/kvm/hyp/include/nvhe/clock.h | 8 ++++---- > > > > > > > > arch/arm64/kvm/hyp/nvhe/clock.c | 4 ++-- > > > > > > > > arch/arm64/kvm/hyp/nvhe/trace.c | 4 ++-- > > > > > > > > 3 files changed, 8 insertions(+), 8 deletions(-) > > > > > > > > > > > > > > > > diff --git a/arch/arm64/kvm/hyp/include/nvhe/clock.h b/arch/arm64/kvm/hyp/include/nvhe/clock.h > > > > > > > > index 9f429f5c0664..c2ccd0e8bf22 100644 > > > > > > > > --- a/arch/arm64/kvm/hyp/include/nvhe/clock.h > > > > > > > > +++ b/arch/arm64/kvm/hyp/include/nvhe/clock.h > > > > > > > > @@ -6,11 +6,11 @@ > > > > > > > > #include <asm/kvm_hyp.h> > > > > > > > > > > > > > > > > #ifdef CONFIG_NVHE_EL2_TRACING > > > > > > > > -void trace_clock_update(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc); > > > > > > > > -u64 trace_clock(void); > > > > > > > > +void hyp_trace_clock_update(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc); > > > > > > > > +u64 hyp_trace_clock(void); > > > > > > > > > > > > > > hyp_trace_clock overlaps the host side: arch/arm64/kvm/hyp_trace.c > > > > > > > already has a struct hyp_trace_clock and static helpers > > > > > > > hyp_trace_clock_enable() / hyp_trace_clock_show() for the debugfs view > > > > > > > of the same clock. No actual collision, so this is only a readability > > > > > > > point, but a reader grepping hyp_trace_clock now gets two unrelated > > > > > > > things. Maybe you'd want to consider a different name, but naming is > > > > > > > hard :) > > > > > > > > If we were to rename: the clock is a "hyp_clock" so probably trace_hyp_clock() > > > > is the right thing here. > > > > > > > > while the "hyp_trace_" is the prefix for hyp_trace.c file content. > > > > > > > > Although I would like to see why we pull trace_clock() from the kernel into EL2. > > > > That bit sounds wrong and if we have a way around perhaps that's better? > > > > > > Most include path look like this: > > > In file included from ./include/linux/ftrace.h:11, > > > from ./include/linux/kprobes.h:28, > > > from ./include/linux/kgdb.h:17, > > > from ./arch/arm64/include/asm/cacheflush.h:11, > > > from ./include/linux/cacheflush.h:5, > > > from ./include/linux/highmem.h:8, > > > from ./include/linux/bvec.h:10, > > > from ./include/linux/blk_types.h:10, > > > from ./include/linux/writeback.h:13, > > > from ./include/linux/memcontrol.h:23, > > > from ./include/linux/resume_user_mode.h:8, > > > from ./include/linux/entry-virt.h:6, > > > from ./include/linux/kvm_host.h:5, > > > > > > Although, some files have ./arch/arm64/include/asm/cacheflush.h > > > directly. > > > > Ha sad, no way around that... > > > > So about trace_hyp_clock() / trace_hyp_clock_update() ? > > > > Makes sense, I will respin with that. Thanks. And small nit for the patch title: use "hyp tracing" to distinguish from the kernel tracing. > > Thanks, > Mostafa >
© 2016 - 2026 Red Hat, Inc.