All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] KVM: arm64: Avoid naming collision in tracing
@ 2026-07-12 15:38 Mostafa Saleh
  2026-07-12 16:19 ` Fuad Tabba
  2026-07-13  8:02 ` Vincent Donnefort
  0 siblings, 2 replies; 12+ messages in thread
From: Mostafa Saleh @ 2026-07-12 15:38 UTC (permalink / raw)
  To: linux-kernel, kvmarm, linux-arm-kernel
  Cc: maz, oupton, seiden, joey.gouly, suzuki.poulose, yuzenghui,
	catalin.marinas, will, vdonnefort, tabba, Mostafa Saleh

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-goog



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] KVM: arm64: Avoid naming collision in tracing
  2026-07-12 15:38 [PATCH] KVM: arm64: Avoid naming collision in tracing Mostafa Saleh
@ 2026-07-12 16:19 ` Fuad Tabba
  2026-07-12 19:19   ` Mostafa Saleh
  2026-07-13  8:02 ` Vincent Donnefort
  1 sibling, 1 reply; 12+ messages in thread
From: Fuad Tabba @ 2026-07-12 16:19 UTC (permalink / raw)
  To: Mostafa Saleh
  Cc: linux-kernel, kvmarm, linux-arm-kernel, maz, oupton, seiden,
	joey.gouly, suzuki.poulose, yuzenghui, catalin.marinas, will,
	vdonnefort

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
>


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] KVM: arm64: Avoid naming collision in tracing
  2026-07-12 16:19 ` Fuad Tabba
@ 2026-07-12 19:19   ` Mostafa Saleh
  2026-07-12 19:30     ` Fuad Tabba
  0 siblings, 1 reply; 12+ messages in thread
From: Mostafa Saleh @ 2026-07-12 19:19 UTC (permalink / raw)
  To: Fuad Tabba
  Cc: linux-kernel, kvmarm, linux-arm-kernel, maz, oupton, seiden,
	joey.gouly, suzuki.poulose, yuzenghui, catalin.marinas, will,
	vdonnefort

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] KVM: arm64: Avoid naming collision in tracing
  2026-07-12 19:19   ` Mostafa Saleh
@ 2026-07-12 19:30     ` Fuad Tabba
  2026-07-13  8:11       ` Vincent Donnefort
  0 siblings, 1 reply; 12+ messages in thread
From: Fuad Tabba @ 2026-07-12 19:30 UTC (permalink / raw)
  To: Mostafa Saleh
  Cc: linux-kernel, kvmarm, linux-arm-kernel, maz, oupton, seiden,
	joey.gouly, suzuki.poulose, yuzenghui, catalin.marinas, will,
	vdonnefort

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] KVM: arm64: Avoid naming collision in tracing
  2026-07-12 15:38 [PATCH] KVM: arm64: Avoid naming collision in tracing Mostafa Saleh
  2026-07-12 16:19 ` Fuad Tabba
@ 2026-07-13  8:02 ` Vincent Donnefort
  2026-07-13  8:05   ` Vincent Donnefort
  1 sibling, 1 reply; 12+ messages in thread
From: Vincent Donnefort @ 2026-07-13  8:02 UTC (permalink / raw)
  To: Mostafa Saleh
  Cc: linux-kernel, kvmarm, linux-arm-kernel, maz, oupton, seiden,
	joey.gouly, suzuki.poulose, yuzenghui, catalin.marinas, will,
	tabba

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

> 
> ./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
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] KVM: arm64: Avoid naming collision in tracing
  2026-07-13  8:02 ` Vincent Donnefort
@ 2026-07-13  8:05   ` Vincent Donnefort
  2026-07-13  8:19     ` Mostafa Saleh
  0 siblings, 1 reply; 12+ messages in thread
From: Vincent Donnefort @ 2026-07-13  8:05 UTC (permalink / raw)
  To: Mostafa Saleh
  Cc: linux-kernel, kvmarm, linux-arm-kernel, maz, oupton, seiden,
	joey.gouly, suzuki.poulose, yuzenghui, catalin.marinas, will,
	tabba

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>
> > 
> > --

[...]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] KVM: arm64: Avoid naming collision in tracing
  2026-07-12 19:30     ` Fuad Tabba
@ 2026-07-13  8:11       ` Vincent Donnefort
  2026-07-13  8:26         ` Mostafa Saleh
  0 siblings, 1 reply; 12+ messages in thread
From: Vincent Donnefort @ 2026-07-13  8:11 UTC (permalink / raw)
  To: Fuad Tabba
  Cc: Mostafa Saleh, linux-kernel, kvmarm, linux-arm-kernel, maz,
	oupton, seiden, joey.gouly, suzuki.poulose, yuzenghui,
	catalin.marinas, will

[...]

> >
> > > > 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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] KVM: arm64: Avoid naming collision in tracing
  2026-07-13  8:05   ` Vincent Donnefort
@ 2026-07-13  8:19     ` Mostafa Saleh
  0 siblings, 0 replies; 12+ messages in thread
From: Mostafa Saleh @ 2026-07-13  8:19 UTC (permalink / raw)
  To: Vincent Donnefort
  Cc: linux-kernel, kvmarm, linux-arm-kernel, maz, oupton, seiden,
	joey.gouly, suzuki.poulose, yuzenghui, catalin.marinas, will,
	tabba

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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] KVM: arm64: Avoid naming collision in tracing
  2026-07-13  8:11       ` Vincent Donnefort
@ 2026-07-13  8:26         ` Mostafa Saleh
  2026-07-13  8:34           ` Vincent Donnefort
  0 siblings, 1 reply; 12+ messages in thread
From: Mostafa Saleh @ 2026-07-13  8:26 UTC (permalink / raw)
  To: Vincent Donnefort
  Cc: Fuad Tabba, linux-kernel, kvmarm, linux-arm-kernel, maz, oupton,
	seiden, joey.gouly, suzuki.poulose, yuzenghui, catalin.marinas,
	will

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] KVM: arm64: Avoid naming collision in tracing
  2026-07-13  8:26         ` Mostafa Saleh
@ 2026-07-13  8:34           ` Vincent Donnefort
  2026-07-13  8:40             ` Mostafa Saleh
  0 siblings, 1 reply; 12+ messages in thread
From: Vincent Donnefort @ 2026-07-13  8:34 UTC (permalink / raw)
  To: Mostafa Saleh
  Cc: Fuad Tabba, linux-kernel, kvmarm, linux-arm-kernel, maz, oupton,
	seiden, joey.gouly, suzuki.poulose, yuzenghui, catalin.marinas,
	will

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] KVM: arm64: Avoid naming collision in tracing
  2026-07-13  8:34           ` Vincent Donnefort
@ 2026-07-13  8:40             ` Mostafa Saleh
  2026-07-13  8:55               ` Vincent Donnefort
  0 siblings, 1 reply; 12+ messages in thread
From: Mostafa Saleh @ 2026-07-13  8:40 UTC (permalink / raw)
  To: Vincent Donnefort
  Cc: Fuad Tabba, linux-kernel, kvmarm, linux-arm-kernel, maz, oupton,
	seiden, joey.gouly, suzuki.poulose, yuzenghui, catalin.marinas,
	will

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] KVM: arm64: Avoid naming collision in tracing
  2026-07-13  8:40             ` Mostafa Saleh
@ 2026-07-13  8:55               ` Vincent Donnefort
  0 siblings, 0 replies; 12+ messages in thread
From: Vincent Donnefort @ 2026-07-13  8:55 UTC (permalink / raw)
  To: Mostafa Saleh
  Cc: Fuad Tabba, linux-kernel, kvmarm, linux-arm-kernel, maz, oupton,
	seiden, joey.gouly, suzuki.poulose, yuzenghui, catalin.marinas,
	will

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
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2026-07-13  8:55 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-12 15:38 [PATCH] KVM: arm64: Avoid naming collision in tracing Mostafa Saleh
2026-07-12 16:19 ` Fuad Tabba
2026-07-12 19:19   ` Mostafa Saleh
2026-07-12 19:30     ` Fuad Tabba
2026-07-13  8:11       ` Vincent Donnefort
2026-07-13  8:26         ` Mostafa Saleh
2026-07-13  8:34           ` Vincent Donnefort
2026-07-13  8:40             ` Mostafa Saleh
2026-07-13  8:55               ` Vincent Donnefort
2026-07-13  8:02 ` Vincent Donnefort
2026-07-13  8:05   ` Vincent Donnefort
2026-07-13  8:19     ` Mostafa Saleh

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.