Re: [PATCH] KVM: arm64: Avoid naming collision in tracing

[Fuad Tabba]

On Sun, 12 Jul 2026 at 20:19, Mostafa Saleh <smostafa@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > >
> > > --
> >
> > 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@xxxxxxxxx>
> > Tested-by: Fuad Tabba < fuad.tabba@xxxxxxxxx>
> >
> > Test: builds fine with the different config enables.
> >
> > Cheers,
> > /fuad