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

[Vincent Donnefort <vdonnefort@google.com>]

On Thu, Aug 22, 2024 at 10:13:34AM +0100, Marc Zyngier wrote:
> 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.

I meant the counter but happy to drop this entire paragraph and just keep the
following one!

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

Yep, my bad!

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