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

[Vincent Donnefort <vdonnefort@google.com>]

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.

> 
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> ---
>  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.

> +
> +#define HYP_CLK_SEC_TO_NS 1000000000UL

Not possible to use NSEC_PER_SEC?

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

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

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

>  	u64 cyc, ns;
>  
> diff --git a/arch/arm64/kvm/hyp/nvhe/setup.c b/arch/arm64/kvm/hyp/nvhe/setup.c
> index d8e5b563fd3d..8041f6e80cd1 100644
> --- a/arch/arm64/kvm/hyp/nvhe/setup.c
> +++ b/arch/arm64/kvm/hyp/nvhe/setup.c
> @@ -10,6 +10,7 @@
>  #include <asm/kvm_pgtable.h>
>  #include <asm/kvm_pkvm.h>
>  
> +#include <nvhe/clock.h>
>  #include <nvhe/early_alloc.h>
>  #include <nvhe/ffa.h>
>  #include <nvhe/gfp.h>
> @@ -312,6 +313,10 @@ void __noreturn __pkvm_init_finalise(void)
>  	};
>  	pkvm_pgtable.mm_ops = &pkvm_pgtable_mm_ops;
>  
> +	ret = hyp_clock_init();
> +	if (ret)
> +		goto out;
> +
>  	ret = fix_host_ownership();
>  	if (ret)
>  		goto out;
> diff --git a/arch/arm64/kvm/hyp/nvhe/trace.c b/arch/arm64/kvm/hyp/nvhe/trace.c
> index a6ca27b18e15..e30de840e6c2 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_clock_ns());
>  }
>  
>  void tracing_commit_entry(void)
> @@ -285,7 +285,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_clock_update(mult, shift, epoch_ns, epoch_cyc);
>  }
>  
>  int __tracing_reset(unsigned int cpu)
> -- 
> 2.54.0.545.g6539524ca2-goog
>