Re: [RFC PATCH 04/11] timekeeping: Export the boot clock in snapshots

[Marc Zyngier <maz@kernel.org>]

On Mon, 05 Aug 2024 18:32:27 +0100,
Vincent Donnefort <vdonnefort@google.com> wrote:
> 
> On arm64 systems, the arch timer can be accessible by both EL1 and EL2.
> This means when running with nVHE or protected KVM, it is easy to
> generate clock values from the hypervisor, synchronized with the kernel.

When you say "arch_timer" here, are you talking about the data
structure describing the timer? Or about the actual *counter*, a
system register provided by the HW?

I'm not sure the architecture-specific details are massively relevant,
given that this is an arch-agnostic change.

>
> For tracing purpose, the boot clock is interesting as it doesn't stop on
> suspend. Export it as part of the time snapshot. This will later allow
> the hypervisor to add boot clock timestamps to its events.

Isn't that the actual description of the change? By getting the boot
time as well as the parameters to compute an increment, you allow any
subsystem able to perform a snapshot to compute a delta from boot time
as long as they have access to the counter source.

> 
> Signed-off-by: Vincent Donnefort <vdonnefort@google.com>
> 
> diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
> index fc12a9ba2c88..0fc6a61d64bd 100644
> --- a/include/linux/timekeeping.h
> +++ b/include/linux/timekeeping.h
> @@ -275,18 +275,24 @@ struct ktime_timestamps {
>   *				 counter value
>   * @cycles:	Clocksource counter value to produce the system times
>   * @real:	Realtime system time
> + * @boot:	Boot time
>   * @raw:	Monotonic raw system time
>   * @cs_id:	Clocksource ID
>   * @clock_was_set_seq:	The sequence number of clock-was-set events
>   * @cs_was_changed_seq:	The sequence number of clocksource change events
> + * @mono_shift:	The monotonic clock slope shift
> + * @mono_mult:	The monotonic clock slope mult
>   */
>  struct system_time_snapshot {
>  	u64			cycles;
>  	ktime_t			real;
> +	ktime_t			boot;
>  	ktime_t			raw;
>  	enum clocksource_ids	cs_id;
>  	unsigned int		clock_was_set_seq;
>  	u8			cs_was_changed_seq;
> +	u32			mono_shift;
> +	u32			mono_mult;
>  };
>  
>  /**
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index 2fa87dcfeda9..6d0488a555a7 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -1057,9 +1057,11 @@ noinstr time64_t __ktime_get_real_seconds(void)
>  void ktime_get_snapshot(struct system_time_snapshot *systime_snapshot)
>  {
>  	struct timekeeper *tk = &tk_core.timekeeper;
> +	u32 mono_mult, mono_shift;
>  	unsigned int seq;
>  	ktime_t base_raw;
>  	ktime_t base_real;
> +	ktime_t base_boot;
>  	u64 nsec_raw;
>  	u64 nsec_real;
>  	u64 now;
> @@ -1074,14 +1076,21 @@ void ktime_get_snapshot(struct system_time_snapshot *systime_snapshot)
>  		systime_snapshot->clock_was_set_seq = tk->clock_was_set_seq;
>  		base_real = ktime_add(tk->tkr_mono.base,
>  				      tk_core.timekeeper.offs_real);
> +		base_boot = ktime_add(tk->tkr_mono.base,
> +				      tk_core.timekeeper.offs_boot);
>  		base_raw = tk->tkr_raw.base;
>  		nsec_real = timekeeping_cycles_to_ns(&tk->tkr_mono, now);
>  		nsec_raw  = timekeeping_cycles_to_ns(&tk->tkr_raw, now);
> +		mono_mult = tk->tkr_mono.mult;
> +		mono_shift = tk->tkr_mono.shift;
>  	} while (read_seqcount_retry(&tk_core.seq, seq));
>  
>  	systime_snapshot->cycles = now;
>  	systime_snapshot->real = ktime_add_ns(base_real, nsec_real);
> +	systime_snapshot->boot = ktime_add_ns(base_boot, nsec_real);
>  	systime_snapshot->raw = ktime_add_ns(base_raw, nsec_raw);
> +	systime_snapshot->mono_shift = mono_shift;
> +	systime_snapshot->mono_mult = mono_mult;
>  }
>  EXPORT_SYMBOL_GPL(ktime_get_snapshot);
>  

This looks good to me, but you should probably Cc the timekeeping
maintainers (tglx, John Stultz, and Stephen Boyd).

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.