Re: [PATCH v6 01/25] KVM: arm64: Generalize trace clock

[Mostafa Saleh]

Hi Vincent,

On Fri, Jul 10, 2026 at 03:19:51PM +0100, Vincent Donnefort wrote:
> Hi Mostafa,
> 
> Sorry I should have looked at this a while ago!
> 
> On Fri, May 01, 2026 at 11:19:03AM +0000, Mostafa Saleh wrote:
> > IOMMU drivers need to track time, mainly for timeouts.
> > Generalize the tracing clock functions in the hypervsior, so they can
> > be used from IOMMU drivers.
> > 
> > 1) Make the compilation independent from tracing.
> > 
> > 2) As drivers might need to use that quite early, provide default
> >    values for the clock data based on cntfrq_el0, the driver can
> >    keep using these values without calling hyp_clock_update() as
> >    they don't need to sync with the host timers.
> > 
> > This is mainly used for timeouts, so a malicious host can DoS the
> > system or cause premature timeouts which likely end up in hyp panic,
> > that should be acceptable as neither of those would undermine the
> > security guarantees.
> 
> This still sounds very risky if the usage shift, and as I explain later I don't
> believe this is completely OK as regard to mult/shift updates.

I see, I still think it is not a security risk though.
My main concern is switching banks between polling might cause
problems.

> 
> > 
> > Signed-off-by: Mostafa Saleh <smostafa@xxxxxxxxxx>
> > ---
> >  arch/arm64/kvm/hyp/include/nvhe/clock.h | 11 ++-----
> >  arch/arm64/kvm/hyp/nvhe/Makefile        |  4 +--
> >  arch/arm64/kvm/hyp/nvhe/clock.c         | 44 ++++++++++++++++++++++---
> >  arch/arm64/kvm/hyp/nvhe/setup.c         |  5 +++
> >  arch/arm64/kvm/hyp/nvhe/trace.c         |  4 +--
> >  5 files changed, 51 insertions(+), 17 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/hyp/include/nvhe/clock.h b/arch/arm64/kvm/hyp/include/nvhe/clock.h
> > index 9f429f5c0664..e6a0e43af88d 100644
> > --- a/arch/arm64/kvm/hyp/include/nvhe/clock.h
> > +++ b/arch/arm64/kvm/hyp/include/nvhe/clock.h
> > @@ -5,12 +5,7 @@
> >  
> >  #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);
> > -#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; }
> > -#endif
> > +void hyp_clock_update(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc);
> > +u64 hyp_clock_ns(void);
> > +int hyp_clock_init(void);
> >  #endif
> > diff --git a/arch/arm64/kvm/hyp/nvhe/Makefile b/arch/arm64/kvm/hyp/nvhe/Makefile
> > index 62cdfbff7562..89d0533921f9 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/Makefile
> > +++ b/arch/arm64/kvm/hyp/nvhe/Makefile
> > @@ -26,10 +26,10 @@ hyp-obj-y := timer-sr.o sysreg-sr.o debug-sr.o switch.o tlb.o hyp-init.o host.o
> >  	 hyp-main.o hyp-smp.o psci-relay.o early_alloc.o page_alloc.o \
> >  	 cache.o setup.o mm.o mem_protect.o sys_regs.o pkvm.o stacktrace.o ffa.o
> >  hyp-obj-y += ../vgic-v3-sr.o ../aarch32.o ../vgic-v2-cpuif-proxy.o ../entry.o \
> > -	 ../fpsimd.o ../hyp-entry.o ../exception.o ../pgtable.o ../vgic-v5-sr.o
> > +	 ../fpsimd.o ../hyp-entry.o ../exception.o ../pgtable.o ../vgic-v5-sr.o clock.o
> >  hyp-obj-y += ../../../kernel/smccc-call.o
> >  hyp-obj-$(CONFIG_LIST_HARDENED) += list_debug.o
> > -hyp-obj-$(CONFIG_NVHE_EL2_TRACING) += clock.o trace.o events.o
> > +hyp-obj-$(CONFIG_NVHE_EL2_TRACING) += trace.o events.o
> >  hyp-obj-y += $(lib-objs)
> >  
> >  # Path to simple_ring_buffer.c
> > diff --git a/arch/arm64/kvm/hyp/nvhe/clock.c b/arch/arm64/kvm/hyp/nvhe/clock.c
> > index 32fc4313fe43..53d0bd55e866 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/clock.c
> > +++ b/arch/arm64/kvm/hyp/nvhe/clock.c
> > @@ -18,7 +18,41 @@ static struct clock_data {
> >  		u64 cyc_overflow64;
> >  	} data[2];
> >  	u64 cur;
> > -} trace_clock_data;
> > +} clock_data;
> 
> I believe this should be separated. the trace_clock is "unsafe" but synchronised
> with the host. the clock you need must be "safe" but we don't really need to be
> synchronised. 
> 
> So overall, I would keep all the trace_clock_* interface unchanged and introduce
> in the same file a hyp_clock, with its own hyp_clock_data.

Makes sense, I will do that, I will make the file compile outside of
tracing but I will keep the tracing code as is. I will introduce 2
functions to read the system timer and to init the hypervisor which
was similar to the earlier versions.

> 
> > +
> > +#define HYP_CLK_SEC_TO_NS 1000000000UL
> 
> Not possible to use NSEC_PER_SEC?
>

It should, we have to include include/vdso/time64.h for that.

> > +
> > +int hyp_clock_init(void)
> > +{
> > +	u32 timer_freq = read_sysreg(cntfrq_el0);
> > +	u32 shift = 32;
> > +	u64 mult;
> > +
> > +	/*
> > +	 * KVM will not initialize if FW didn't set cntfrq_el0, that is already
> > +	 * part of the boot protocol.
> > +	 */
> > +	if (!timer_freq)
> > +		return -ENODEV;
> > +
> > +	/* Timer freq can't be larger than 1Ghz by spec. */
> > +	if (timer_freq > HYP_CLK_SEC_TO_NS)
> > +		return -EINVAL;
> > +
> > +	/* Simplified logic from clocks_calc_mult_shift() */
> > +	do {
> > +		mult = (HYP_CLK_SEC_TO_NS << shift);
> > +		mult = div_u64(mult, timer_freq);
> > +		if (mult <= (~0U))
> > +			break;
> > +		shift--;
> > +	} while (shift > 0);
> > +
> > +	clock_data.data[0].shift = shift;
> > +	clock_data.data[0].mult = mult;
> > +	clock_data.data[0].cyc_overflow64 = ULONG_MAX / mult;
> 
> I don't know if we want to update this from time to time, but we can't rely on
> host provided data. Perhaps the call to that clock could self-update?
> 
> Worst case we fallback on 128-bits mult. but perhaps this isn't a problem for a
> wait loop()?
> 
> And as this is wait loop, perhaps we could reduce the resolution quite heavily,
> so we reduce the risk of overflow?

As mentioned, I will drop this and rely on a wrapper for the system
counter instead, and the caller can handle the overflow as in
__delay()

> 
> > +	return 0;
> > +}
> >  
> >  static u64 __clock_mult_uint128(u64 cyc, u32 mult, u32 shift)
> >  {
> > @@ -30,9 +64,9 @@ static u64 __clock_mult_uint128(u64 cyc, u32 mult, u32 shift)
> >  }
> >  
> >  /* Does not guarantee no reader on the modified bank. */
> 
>  ^
> And here's another reason why you should have another clock. It works because of
> tricks in the tracing interface.

Agreed, this was my concern too.

> 
> > -void trace_clock_update(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc)
> > +void hyp_clock_update(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc)
> >  {
> > -	struct clock_data *clock = &trace_clock_data;
> > +	struct clock_data *clock = &clock_data;
> >  	u64 bank = clock->cur ^ 1;
> >  
> >  	clock->data[bank].mult			= mult;
> > @@ -45,9 +79,9 @@ void trace_clock_update(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc)
> >  }
> >  
> >  /* Use untrusted host data */
> > -u64 trace_clock(void)
> > +u64 hyp_clock_ns(void)
> >  {
> > -	struct clock_data *clock = &trace_clock_data;
> > +	struct clock_data *clock = &clock_data;
> >  	u64 bank = smp_load_acquire(&clock->cur);
> 
> The whole bank system is not necessary if not updated by the host.

But tracing can be enabled along side the SMMUv3.

Thanks,
Mostafa