Subject: Re: [PATCH net-next v8 3/3] gve: implement PTP gettimex64

From: Thomas Gleixner <[email-masked]>
Date: Thu, 21 May 2026 20:06:08 +0200

On Thu, May 21 2026 at 12:43, David Woodhouse wrote:
> On Thu, 2026-05-14 at 22:58 +0000, Harshitha Ramamurthy wrote:
>> +	if (IS_ENABLED(CONFIG_X86))
>> +		return system_clock_source == CSID_X86_TSC;
>> +	else if (IS_ENABLED(CONFIG_ARM64))
>> +		return system_clock_source == CSID_ARM_ARCH_COUNTER;
>> +
>> +	return false;
>
> Strictly, you could manage to convert to CSID_X86_KVM_CLK here too. The
> x86 kvm_arch_ptp_get_crosststamp() for ptp_kvm does so unconditionally.
>
> For ptp_vmclock we support both; we do the conversion only if the cs_id
> in the snapshot *is* the KVM clock, which seemed a bit more reasonable.
>
> And here you add a third variant which *only* copes with CSID_X86_TSC,
> to complete the set of possibilities :)

Yeah, let's sprinkle that all over the tree so nobody can figure out a
coherent picture anymore. :)

> With PTM and the various virt enlightenments, there are an increasing
> number of "PTP" clocks which literally *just* tell us a cycle count at
> the moment of the corresponding reading, and we don't need the ABA
> sandwich of local clock readings around it any more.

That's what the PRECISE ioctl and ptp::info::getcrosstimestamp() is for,
which utilizes get_device_system_crosststamp() and provides the
translation from PTM, on x86 that's ART which is converted to TSC.

It then correlates the converted system time stamp to CLOCK_REALTIME and
CLOCK_MONOTONIC_RAW.

That has been implemented long ago exactly for that purpose, but people
need to reinvent the wheel over and over just because it makes the
kernel so more maintainable.

> I've got another one waiting in the wings which will literally just
> latch the counter value when a PPS signal comes in, too.

That can use a similar mechanism.

> It might be good to harmonise the way we convert to KVM clock if we're
> going to support that (although I'll accept an argument that any guest
> which chooses to use the KVM clock doesn't *deserve* to have accurate
> timekeeping).

:)

> I'd also like consensus on exposing it to userspace. I think Thomas is
> going to suggest that we should always convert to MONOTONIC_RAW, but
> what I asked Arthur to do in 
> https://lore.kernel.org/all/[email-masked]/
> exposes the raw counter values instead.

I'm not against that per se, but that really wants to be a consistent
snapshot and not some pulled out of thin air cycles value.

ktime_get_snapshot() provides that already today, so that can be
arguably used for pre/post timestamp mechanisms, except that it lacks
support for AUX clocks.

get_device_system_crosststamp() does not provide cycles and clock id,
but that's not rocket science to extend.

> From the dedicating hosting point of view, we *only* care about the
> counter, and absolutely DGAF about the host's timekeeping. Migrating
> KVM guests requires getting the guest TSC right (in terms of scaling
> factors and offset from the host TSC), and feeding accurate time to
> guests is done in terms of the guest TSC to realtime relationship.

The offset has nothing to do with the scaling. It's the delta to the TSC
value which the migrated guest saw last when it was packed up for
migration.

The scaling is not required to be cycles based either. You need to know
the frequency ratio of the guest TSC to the new host TSC. cycles are not
telling you that. So you need extra information from the previous host,
i.e. the assumed raw TSC frequency of the guest plus the steered
frequency in your virtual PCH interface.

So sure you can utilize the raw counter values for that, but you can do
the same with CLOCK_MONOTONIC_RAW by exposing the mult/shift factor
which the guest keeps unmodified after boot or a frequency value derived
from that. That's IMO way more sensible than measuring it somehow and
the shift/mult conversion inaccuracy you mentioned is laughable TBH. TSC
usually ends up with a shift value of 24, so the conversion loss of the
TSC to CLOCK_MONOTONIC_RAW is +/- 1ns, which is totally irrelevant in
the larger picture.

> So our ideal state is that we discipline the TSC against real time in
> userspace, and enabling those PHC drivers to *give* us the raw cycle
> counts that they actually measured seemed best.

As I said, I'm not against that per se, but then we want to provide
interfaces which are generic enough so they can be used for your
purposes, but also for the requirements of chrony et al. Looking at the
patch you linked to above:

> +static long ptp_sys_offset_precise_attrs(struct ptp_clock *ptp, void __user *arg)
> +{
> +	struct ptp_sys_offset_precise_attrs precise_offset_attrs;
> +	struct system_device_crosststamp xtstamp;
> +	struct ptp_clock_attributes att = {};
> +	struct timespec64 ts;
> +	int err;
> +
> +	if (!ptp->info->getcrosststampattrs)
> +		return -EOPNOTSUPP;
> +
> +	err = ptp->info->getcrosststampattrs(ptp->info, &xtstamp, &att);
> +	if (err)
> +		return err;
> +
> +	memset(&precise_offset_attrs, 0, sizeof(precise_offset_attrs));
> +	ts = ktime_to_timespec64(xtstamp.device);
> +	precise_offset_attrs.device.pct.sec = ts.tv_sec;
> +	precise_offset_attrs.device.pct.nsec = ts.tv_nsec;
> +	precise_offset_attrs.device.att.error_bound = att.error_bound;
> +	precise_offset_attrs.device.att.timescale = att.timescale;
> +	precise_offset_attrs.device.att.status = att.status;
> +	precise_offset_attrs.device.att.counter_id = att.counter_id;
> +	precise_offset_attrs.device.att.counter_value = att.counter_value;

That's exactly the hackery which is counterproductive. Why?

Where does ptp::info::getcrosststampattrs() retrieve the system counter
value from?

Either it has to convert from the ART based timestamp to TSC magically
by circumventing the core code or it has to use get_cycles() separately
which defeats the whole purpose of a hardware latched combo timestamp.

While this:

diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
index aee2c1a46e47..43a55f618235 100644
--- a/include/linux/timekeeping.h
+++ b/include/linux/timekeeping.h
@@ -296,19 +296,6 @@ struct system_time_snapshot {
 	u8			cs_was_changed_seq;
 };
 
-/**
- * struct system_device_crosststamp - system/device cross-timestamp
- *				      (synchronized capture)
- * @device:		Device time
- * @sys_realtime:	Realtime simultaneous with device time
- * @sys_monoraw:	Monotonic raw simultaneous with device time
- */
-struct system_device_crosststamp {
-	ktime_t device;
-	ktime_t sys_realtime;
-	ktime_t sys_monoraw;
-};
-
 /**
  * struct system_counterval_t - system counter value with the ID of the
  *				corresponding clocksource
@@ -325,6 +312,21 @@ struct system_counterval_t {
 	bool			use_nsecs;
 };
 
+/**
+ * struct system_device_crosststamp - system/device cross-timestamp
+ *				      (synchronized capture)
+ * @device:		Device time
+ * @sys_counter:	Clocksource counter value simultaneous with device time
+ * @sys_realtime:	Realtime simultaneous with device time
+ * @sys_monoraw:	Monotonic raw simultaneous with device time
+ */
+struct system_device_crosststamp {
+	ktime_t				device;
+	struct system_counterval_t	sys_counter;
+	ktime_t				sys_realtime;
+	ktime_t				sys_monoraw;
+};
+
 extern bool ktime_real_to_base_clock(ktime_t treal,
 				     enum clocksource_ids base_id, u64 *cycles);
 extern bool timekeeping_clocksource_has_base(enum clocksource_ids id);
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index c493a4010305..11df7e25bdca 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1506,6 +1506,7 @@ int get_device_system_crosststamp(int (*get_time_fn)
 		nsec_raw = timekeeping_cycles_to_ns(&tk->tkr_raw, cycles);
 	} while (read_seqcount_retry(&tk_core.seq, seq));
 
+	xtstamp->sys_counter = system_counterval;
 	xtstamp->sys_realtime = ktime_add_ns(base_real, nsec_real);
 	xtstamp->sys_monoraw = ktime_add_ns(base_raw, nsec_raw);
 
Provides the information coherently and without any extra hacks. No?

Extending get_device_system_crosststamp() for AUX clocks is on my todo
list for a long time, but I did not come around to it yet. It's not
really complicated to make that work.

The actual ena_phc_gettimexattrs64() implementation does not provide the
counter value at all despite the fact that it could trivially do so with
a modified ktime_get_snapshot() except for AUX clock IDs, but that's a
solvable problem. See uncompiled PoC below.

I really have to ask why we put a lot of effort into consolidated
infrastructure, when every drivers/foo/ subsystem decides it needs it's
own absymal hacks just because sitting down and extending the generic
infrastructure is asked too much. TBH, that's frustrating as hell and
no, none of these usecases is so special that it would justify a single
one of these hacks.

> Could we do that all on MONOTONIC_RAW, if Thomas insists? Maybe...
> although I like the model of disciplining one *specific* counter, and
> then telling the kernel about *that* counter. If the kernel for some
> reason has randomly switched to the HPET then the kernel is free to
> ignore what we're telling it about the TSC.

Well, if the kernel has switched to HPET for timekeeping, then there is
a good reason especially with the reworked clocksource watchdog, which
is not longer being stupid under load and on big machines. If the
sysadmin thinks it's cool to switch to HPET, then he can keep the
pieces. In those cases the machine has other problems than taming the
TSC for guests....

Thanks,

        tglx
---
--- a/include/linux/timekeeping.h
+++ b/include/linux/timekeeping.h
@@ -276,19 +276,24 @@ static inline bool ktime_get_aux_ts64(cl
 #endif
 
 /**
- * struct system_time_snapshot - simultaneous raw/real time capture with
+ * struct system_time_snapshot - simultaneous raw/CLOCK_REALTIME or CLOCK_AUX$N time capture with
  *				 counter value
  * @cycles:	Clocksource counter value to produce the system times
  * @real:	Realtime system time
+ * @aux:	Auxiliary system time if selected in ktime_get_snapshot_id()
  * @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
+ * @clock_valid:	The clock is valid
  */
 struct system_time_snapshot {
 	u64			cycles;
-	ktime_t			real;
+	union {
+		ktime_t		real;
+		ktime_t		aux;
+	};
 	ktime_t			boot;
 	ktime_t			raw;
 	enum clocksource_ids	cs_id;
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -67,6 +67,7 @@ static inline bool tk_is_aux(const struc
 {
 	return tk->id >= TIMEKEEPER_AUX_FIRST && tk->id <= TIMEKEEPER_AUX_LAST;
 }
+static inline struct tk_data *aux_get_tk_data(clockid_t id);
 #else
 static inline bool tk_get_aux_ts64(unsigned int tkid, struct timespec64 *ts)
 {
@@ -77,6 +78,10 @@ static inline bool tk_is_aux(const struc
 {
 	return false;
 }
+static inline struct tk_data *aux_get_tk_data(clockid_t id)
+{
+	return NULL;
+}
 #endif
 
 static inline void tk_update_aux_offs(struct timekeeper *tk, ktime_t offs)
@@ -1183,15 +1188,18 @@ noinstr time64_t __ktime_get_real_second
 }
 
 /**
- * ktime_get_snapshot - snapshots the realtime/monotonic raw clocks with counter
+ * ktime_get_snapshot_id - snapshots the monotonic raw clock, the selected clock and the counter
  * @systime_snapshot:	pointer to struct receiving the system time snapshot
+ * @clock_id:		The clock ID to snapshot
  */
-void ktime_get_snapshot(struct system_time_snapshot *systime_snapshot)
+bool ktime_get_snapshot_id(struct system_time_snapshot *systime_snapshot, clockid_t clock_id)
 {
-	struct timekeeper *tk = &tk_core.timekeeper;
+	struct timekeeper *tk;
+	struct tk_data *tkd;
 	unsigned int seq;
 	ktime_t base_raw;
 	ktime_t base_real;
+	ktime_t base_mono;
 	ktime_t base_boot;
 	u64 nsec_raw;
 	u64 nsec_real;
@@ -1199,27 +1207,56 @@ void ktime_get_snapshot(struct system_ti
 
 	WARN_ON_ONCE(timekeeping_suspended);
 
+	switch (clock_id) {
+	case CLOCK_REALTIME:
+		tkd = &tk_core;
+		break;
+	case CLOCK_AUX ... CLOCK_AUX_LAST: {
+		tkd = aux_get_tk_data(clock_id);
+		if (!tkd)
+			return false;
+		break;
+	}
+	default:
+		WARN_ON_ONCE(1);
+		return false;
+	}
+
+	tk = &tkd->timekeeper;
+
 	do {
-		seq = read_seqcount_begin(&tk_core.seq);
+		seq = read_seqcount_begin(&tkd->seq);
+
+		/* Aux clocks can be invalid */
+		if (!tk->clock_valid)
+			return false;
+
 		now = tk_clock_read(&tk->tkr_mono);
 		systime_snapshot->cs_id = tk->tkr_mono.clock->id;
 		systime_snapshot->cs_was_changed_seq = tk->cs_was_changed_seq;
 		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_mono = tk->tkr_mono.base;
+		if (clock_id == CLOCK_REALTIME) {
+			base_real = ktime_add(base_mono, tk_core.timekeeper.offs_real);
+			base_boot = ktime_add(base_mono, 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);
-	} while (read_seqcount_retry(&tk_core.seq, seq));
+	} while (read_seqcount_retry(&tkd->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);
+	if (clock_id == CLOCK_REALTIME) {
+		systime_snapshot->real = ktime_add_ns(base_real, nsec_real);
+		systime_snapshot->boot = ktime_add_ns(base_boot, nsec_real);
+	} else {
+		systime_snapshot->aux = ktime_add_ns(base_mono, nsec_real);
+		systime_snapshot->boot = 0;
+	}
 	systime_snapshot->raw = ktime_add_ns(base_raw, nsec_raw);
+	return true;
 }
-EXPORT_SYMBOL_GPL(ktime_get_snapshot);
+EXPORT_SYMBOL_GPL(ktime_get_snapshot_id);
 
 /* Scale base by mult/div checking for overflow */
 static int scale64_check_overflow(u64 mult, u64 div, u64 *base)