[RFC PATCH 0/4] treewide: Use clocksource id for get_device_system_crosststamp()

[RFC PATCH 0/4] treewide: Use clocksource id for get_device_system_crosststamp()

[Peter Hilber]

This patch series changes struct system_counterval_t to identify the
clocksource through enum clocksource_ids, rather than through struct
clocksource *. The net effect of the patch series is that
get_device_system_crosststamp() callers can supply clocksource ids instead
of clocksource pointers, which can be problematic to get hold of.

For this, modify some code which is relevant to
get_device_system_crosststamp(), in timekeeping, ptp/kvm, x86/kvm, and
x86/tsc.

The series does the following: First, introduce clocksource ids for x86 TSC
and kvm-clock. Then, refactor the x86 TSC a tiny bit to keep changes in the
last, "treewide" patch to a minimum. In the treewide patch, replace
system_counterval_t.cs by .cs_id.

This series should not alter any behavior. Out of the existing
get_device_system_crosststamp() users, only ptp_kvm has been tested (on
x86-64 and arm64). This series is a prerequisite for the virtio_rtc driver
(RFC v2 to be posted). Through this series, virtio_rtc can work without
modifying arm_arch_timer.


Peter Hilber (4):
  x86/tsc: Add clocksource ids for TSC and early TSC
  x86/kvm: Add clocksource id for kvm-clock
  x86/tsc: Use bool, not pointer, for ART availability
  treewide: Use clocksource id for struct system_counterval_t

 arch/x86/kernel/kvmclock.c      |  2 ++
 arch/x86/kernel/tsc.c           | 23 +++++++++++++++--------
 drivers/ptp/ptp_kvm_common.c    |  3 ++-
 include/linux/clocksource_ids.h |  3 +++
 include/linux/timekeeping.h     |  4 ++--
 kernel/time/timekeeping.c       |  3 ++-
 6 files changed, 26 insertions(+), 12 deletions(-)


base-commit: 47762f08697484cf0c2f2904b8c52375ed26c8cb
-- 
2.39.2

[RFC PATCH 1/4] x86/tsc: Add clocksource ids for TSC and early TSC

[Peter Hilber]

Add a clocksource id for TSC and a distinct one for the early TSC.

Use distinct ids for TSC and early TSC, since those also have distinct
clocksource structs. This should help to keep existing semantics when
comparing clocksources.

This change will keep ioctl PTP_SYS_OFFSET_PRECISE working on x86 in the
future, when get_device_system_crosststamp() would be changed to compare
enum clocksource_ids, rather than struct clocksource *. It also makes
identifying TSC easier for outside code in general.

Signed-off-by: Peter Hilber <peter.hilber@opensynergy.com>
---
 arch/x86/kernel/tsc.c           | 3 +++
 include/linux/clocksource_ids.h | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 15f97c0abc9d..132045be76d0 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -11,6 +11,7 @@
 #include <linux/cpufreq.h>
 #include <linux/delay.h>
 #include <linux/clocksource.h>
+#include <linux/clocksource_ids.h>
 #include <linux/percpu.h>
 #include <linux/timex.h>
 #include <linux/static_key.h>
@@ -1168,6 +1169,7 @@ static struct clocksource clocksource_tsc_early = {
 	.mask			= CLOCKSOURCE_MASK(64),
 	.flags			= CLOCK_SOURCE_IS_CONTINUOUS |
 				  CLOCK_SOURCE_MUST_VERIFY,
+	.id			= CSID_TSC_EARLY,
 	.vdso_clock_mode	= VDSO_CLOCKMODE_TSC,
 	.enable			= tsc_cs_enable,
 	.resume			= tsc_resume,
@@ -1190,6 +1192,7 @@ static struct clocksource clocksource_tsc = {
 				  CLOCK_SOURCE_VALID_FOR_HRES |
 				  CLOCK_SOURCE_MUST_VERIFY |
 				  CLOCK_SOURCE_VERIFY_PERCPU,
+	.id			= CSID_TSC,
 	.vdso_clock_mode	= VDSO_CLOCKMODE_TSC,
 	.enable			= tsc_cs_enable,
 	.resume			= tsc_resume,
diff --git a/include/linux/clocksource_ids.h b/include/linux/clocksource_ids.h
index 16775d7d8f8d..86d23abfde2a 100644
--- a/include/linux/clocksource_ids.h
+++ b/include/linux/clocksource_ids.h
@@ -6,6 +6,8 @@
 enum clocksource_ids {
 	CSID_GENERIC		= 0,
 	CSID_ARM_ARCH_COUNTER,
+	CSID_TSC_EARLY,
+	CSID_TSC,
 	CSID_MAX,
 };
 
-- 
2.39.2

[RFC PATCH 2/4] x86/kvm: Add clocksource id for kvm-clock

[Peter Hilber]

Add a clocksource id for kvm-clock, so that it can be identified through
enum clocksource_ids.

This will keep ptp_kvm working on x86 in the future, when
get_device_system_crosststamp() would be changed to compare enum
clocksource_ids, rather than struct clocksource *. It also makes
identifying kvm-clock easier for outside code in general.

Signed-off-by: Peter Hilber <peter.hilber@opensynergy.com>
---
 arch/x86/kernel/kvmclock.c      | 2 ++
 include/linux/clocksource_ids.h | 1 +
 2 files changed, 3 insertions(+)

diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
index fb8f52149be9..a686733a2d20 100644
--- a/arch/x86/kernel/kvmclock.c
+++ b/arch/x86/kernel/kvmclock.c
@@ -4,6 +4,7 @@
 */
 
 #include <linux/clocksource.h>
+#include <linux/clocksource_ids.h>
 #include <linux/kvm_para.h>
 #include <asm/pvclock.h>
 #include <asm/msr.h>
@@ -160,6 +161,7 @@ struct clocksource kvm_clock = {
 	.rating	= 400,
 	.mask	= CLOCKSOURCE_MASK(64),
 	.flags	= CLOCK_SOURCE_IS_CONTINUOUS,
+	.id     = CSID_KVM_CLOCK,
 	.enable	= kvm_cs_enable,
 };
 EXPORT_SYMBOL_GPL(kvm_clock);
diff --git a/include/linux/clocksource_ids.h b/include/linux/clocksource_ids.h
index 86d23abfde2a..11d3cc318dc1 100644
--- a/include/linux/clocksource_ids.h
+++ b/include/linux/clocksource_ids.h
@@ -8,6 +8,7 @@ enum clocksource_ids {
 	CSID_ARM_ARCH_COUNTER,
 	CSID_TSC_EARLY,
 	CSID_TSC,
+	CSID_KVM_CLOCK,
 	CSID_MAX,
 };
 
-- 
2.39.2

[RFC PATCH 3/4] x86/tsc: Use bool, not pointer, for ART availability

[Peter Hilber]

So far, ART availability has been implicitly encoded in
art_related_clocksource, which is NULL if ART is not available.

Replace art_related_clocksource by bool have_art to explicitly indicate
ART availability.

This should reduce the changes during a tree-wide switch of struct
system_counterval_t.cs to enum clocksource_ids (when the clocksource
pointer will not be needed any more).

Signed-off-by: Peter Hilber <peter.hilber@opensynergy.com>
---
 arch/x86/kernel/tsc.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 132045be76d0..e56cc4e97d0d 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -54,7 +54,7 @@ static int __read_mostly tsc_force_recalibrate;
 static u32 art_to_tsc_numerator;
 static u32 art_to_tsc_denominator;
 static u64 art_to_tsc_offset;
-static struct clocksource *art_related_clocksource;
+static bool have_art;
 
 struct cyc2ns {
 	struct cyc2ns_data data[2];	/*  0 + 2*16 = 32 */
@@ -1312,8 +1312,10 @@ struct system_counterval_t convert_art_to_tsc(u64 art)
 	do_div(tmp, art_to_tsc_denominator);
 	res += tmp + art_to_tsc_offset;
 
-	return (struct system_counterval_t) {.cs = art_related_clocksource,
-			.cycles = res};
+	return (struct system_counterval_t) {
+		.cs = have_art ? &clocksource_tsc : NULL,
+		.cycles = res
+	};
 }
 EXPORT_SYMBOL(convert_art_to_tsc);
 
@@ -1350,8 +1352,10 @@ struct system_counterval_t convert_art_ns_to_tsc(u64 art_ns)
 	do_div(tmp, USEC_PER_SEC);
 	res += tmp;
 
-	return (struct system_counterval_t) { .cs = art_related_clocksource,
-					      .cycles = res};
+	return (struct system_counterval_t) {
+		.cs = have_art ? &clocksource_tsc : NULL,
+		.cycles = res
+	};
 }
 EXPORT_SYMBOL(convert_art_ns_to_tsc);
 
@@ -1458,7 +1462,7 @@ static void tsc_refine_calibration_work(struct work_struct *work)
 		goto unreg;
 
 	if (boot_cpu_has(X86_FEATURE_ART))
-		art_related_clocksource = &clocksource_tsc;
+		have_art = true;
 	clocksource_register_khz(&clocksource_tsc, tsc_khz);
 unreg:
 	clocksource_unregister(&clocksource_tsc_early);
@@ -1484,7 +1488,7 @@ static int __init init_tsc_clocksource(void)
 	 */
 	if (boot_cpu_has(X86_FEATURE_TSC_KNOWN_FREQ)) {
 		if (boot_cpu_has(X86_FEATURE_ART))
-			art_related_clocksource = &clocksource_tsc;
+			have_art = true;
 		clocksource_register_khz(&clocksource_tsc, tsc_khz);
 		clocksource_unregister(&clocksource_tsc_early);
 
-- 
2.39.2

[RFC PATCH 4/4] treewide: Use clocksource id for struct system_counterval_t

[Peter Hilber]

Use enum clocksource_ids, rather than struct clocksource *, in struct
system_counterval_t. This allows drivers to use
get_device_system_crosststamp() without referring to clocksource structs,
which are clocksource driver implementation details.

As a "treewide" change, this modifies code in timekeeping, ptp/kvm, and
x86/tsc.

So far, get_device_system_crosststamp() requires the caller to provide a
clocksource * as part of the cross-timestamp (through the
system_counterval_t * parameter in the get_time_fn() callback). But
clocksource * are not easily accessible to most code. As a workaround,
custom functions returning the clocksource * may be put into the
clocksource driver code (cf. kvm_arch_ptp_get_crosststamp() in
arm_arch_timer.c).

To avoid more interference with clocksource driver implementation details
from future code, change system_counterval_t to use enum clocksource_ids,
which serves as a unique identifier for relevant clocksources.

This change will allow the virtio_rtc driver, available as an RFC patch
series, to work without modifying the arm_arch_timer implementation.

This change should not alter any behavior. For the x86 ART related code,
.cs == NULL corresponds to system_counterval_t.cs_id == CSID_GENERIC (0).

The one drawback of this change is that clocksources now need a custom id
to work with get_device_system_crosststamp(), since they cannot be
distinguished otherwise.

Signed-off-by: Peter Hilber <peter.hilber@opensynergy.com>
---
 arch/x86/kernel/tsc.c        | 6 +++---
 drivers/ptp/ptp_kvm_common.c | 3 ++-
 include/linux/timekeeping.h  | 4 ++--
 kernel/time/timekeeping.c    | 3 ++-
 4 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index e56cc4e97d0d..eadfb819a0b5 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -1313,7 +1313,7 @@ struct system_counterval_t convert_art_to_tsc(u64 art)
 	res += tmp + art_to_tsc_offset;
 
 	return (struct system_counterval_t) {
-		.cs = have_art ? &clocksource_tsc : NULL,
+		.cs_id = have_art ? CSID_TSC : CSID_GENERIC,
 		.cycles = res
 	};
 }
@@ -1335,7 +1335,7 @@ EXPORT_SYMBOL(convert_art_to_tsc);
  * struct system_counterval_t - system counter value with the pointer to the
  *	corresponding clocksource
  *	@cycles:	System counter value
- *	@cs:		Clocksource corresponding to system counter value. Used
+ *	@cs_id:		Clocksource corresponding to system counter value. Used
  *			by timekeeping code to verify comparability of two cycle
  *			values.
  */
@@ -1353,7 +1353,7 @@ struct system_counterval_t convert_art_ns_to_tsc(u64 art_ns)
 	res += tmp;
 
 	return (struct system_counterval_t) {
-		.cs = have_art ? &clocksource_tsc : NULL,
+		.cs_id = have_art ? CSID_TSC : CSID_GENERIC,
 		.cycles = res
 	};
 }
diff --git a/drivers/ptp/ptp_kvm_common.c b/drivers/ptp/ptp_kvm_common.c
index 2418977989be..d72e6ed71b7a 100644
--- a/drivers/ptp/ptp_kvm_common.c
+++ b/drivers/ptp/ptp_kvm_common.c
@@ -4,6 +4,7 @@
  *
  * Copyright (C) 2017 Red Hat Inc.
  */
+#include <linux/clocksource.h>
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/init.h>
@@ -46,7 +47,7 @@ static int ptp_kvm_get_time_fn(ktime_t *device_time,
 	preempt_enable_notrace();
 
 	system_counter->cycles = cycle;
-	system_counter->cs = cs;
+	system_counter->cs_id = cs->id;
 
 	*device_time = timespec64_to_ktime(tspec);
 
diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
index fe1e467ba046..2e7ae0f7f19e 100644
--- a/include/linux/timekeeping.h
+++ b/include/linux/timekeeping.h
@@ -270,12 +270,12 @@ struct system_device_crosststamp {
  * struct system_counterval_t - system counter value with the pointer to the
  *				corresponding clocksource
  * @cycles:	System counter value
- * @cs:		Clocksource corresponding to system counter value. Used by
+ * @cs_id:	Clocksource corresponding to system counter value. Used by
  *		timekeeping code to verify comparibility of two cycle values
  */
 struct system_counterval_t {
 	u64			cycles;
-	struct clocksource	*cs;
+	enum clocksource_ids	cs_id;
 };
 
 /*
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 266d02809dbb..56428eadf4c1 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1236,7 +1236,8 @@ int get_device_system_crosststamp(int (*get_time_fn)
 		 * system counter value is the same as the currently installed
 		 * timekeeper clocksource
 		 */
-		if (tk->tkr_mono.clock != system_counterval.cs)
+		if (system_counterval.cs_id == CSID_GENERIC ||
+		    tk->tkr_mono.clock->id != system_counterval.cs_id)
 			return -ENODEV;
 		cycles = system_counterval.cycles;
 
-- 
2.39.2

Re: [RFC PATCH 4/4] treewide: Use clocksource id for struct system_counterval_t

[Thomas Gleixner]

Peter!

On Fri, Aug 18 2023 at 03:12, Peter Hilber wrote:
> --- a/arch/x86/kernel/tsc.c
> +++ b/arch/x86/kernel/tsc.c
> @@ -1313,7 +1313,7 @@ struct system_counterval_t convert_art_to_tsc(u64 art)
>  	res += tmp + art_to_tsc_offset;
>  
>  	return (struct system_counterval_t) {
> -		.cs = have_art ? &clocksource_tsc : NULL,
> +		.cs_id = have_art ? CSID_TSC : CSID_GENERIC,
>  		.cycles = res

Can you please change all of this so that:

    patch 1:   Adds cs_id to struct system_counterval_t
    patch 2-4: Add the clocksource ID and set the cs_id field
    patch 5:   Switches the core to evaluate cs_id
    patch 6:   Remove the cs field from system_counterval_t


> --- a/include/linux/timekeeping.h
> +++ b/include/linux/timekeeping.h
> @@ -270,12 +270,12 @@ struct system_device_crosststamp {
>   * struct system_counterval_t - system counter value with the pointer to the
>   *				corresponding clocksource
>   * @cycles:	System counter value
> - * @cs:		Clocksource corresponding to system counter value. Used by
> + * @cs_id:	Clocksource corresponding to system counter value. Used by
>   *		timekeeping code to verify comparibility of two cycle values

That comment is inaccurate. It's not longer the clocksource itself. It's
the ID which is used for validation.

Thanks,

        tglx

Re: [RFC PATCH 4/4] treewide: Use clocksource id for struct system_counterval_t

[Peter Hilber]

On 15.09.23 15:30, Thomas Gleixner wrote:
> Peter!
> 
> On Fri, Aug 18 2023 at 03:12, Peter Hilber wrote:
>> --- a/arch/x86/kernel/tsc.c
>> +++ b/arch/x86/kernel/tsc.c
>> @@ -1313,7 +1313,7 @@ struct system_counterval_t convert_art_to_tsc(u64 art)
>>  	res += tmp + art_to_tsc_offset;
>>  
>>  	return (struct system_counterval_t) {
>> -		.cs = have_art ? &clocksource_tsc : NULL,
>> +		.cs_id = have_art ? CSID_TSC : CSID_GENERIC,
>>  		.cycles = res
> 
> Can you please change all of this so that:
> 
>     patch 1:   Adds cs_id to struct system_counterval_t
>     patch 2-4: Add the clocksource ID and set the cs_id field
>     patch 5:   Switches the core to evaluate cs_id
>     patch 6:   Remove the cs field from system_counterval_t

OK. For 2-4, I assume split x86/tsc, x86/kvm, drivers/ptp (which
also handles the CSID_ARM_ARCH_COUNTER case).

>> --- a/include/linux/timekeeping.h
>> +++ b/include/linux/timekeeping.h
>> @@ -270,12 +270,12 @@ struct system_device_crosststamp {
>>   * struct system_counterval_t - system counter value with the pointer to the
>>   *				corresponding clocksource
>>   * @cycles:	System counter value
>> - * @cs:		Clocksource corresponding to system counter value. Used by
>> + * @cs_id:	Clocksource corresponding to system counter value. Used by
>>   *		timekeeping code to verify comparibility of two cycle values
> 
> That comment is inaccurate. It's not longer the clocksource itself. It's
> the ID which is used for validation.

I will change the comment to refer to "Clocksource ID".

Thanks for the advice!

Peter

Re: [RFC PATCH 0/4] treewide: Use clocksource id for get_device_system_crosststamp()

[John Stultz]

On Thu, Aug 17, 2023 at 6:13 PM Peter Hilber
<peter.hilber@opensynergy.com> wrote:
>
> This patch series changes struct system_counterval_t to identify the
> clocksource through enum clocksource_ids, rather than through struct
> clocksource *. The net effect of the patch series is that
> get_device_system_crosststamp() callers can supply clocksource ids instead
> of clocksource pointers, which can be problematic to get hold of.

Hey Peter,
  Thanks for sending this out. I'm a little curious though, can you
expand a bit on how clocksource pointers can be problematic to get a
hold of? What exactly is the problem that is motivating this change?

I just worry that switching to an enumeration solution might be
eventually exposing more than we would like to userland.

thanks
-john

Re: [RFC PATCH 0/4] treewide: Use clocksource id for get_device_system_crosststamp()

[Peter Hilber]

On Fri, Aug 25, 2023 6:18 John Stultz <jstultz@google.com> wrote:
> On Thu, Aug 17, 2023 at 6:13 PM Peter Hilber
> <peter.hilber@opensynergy.com> wrote:
>>
>> This patch series changes struct system_counterval_t to identify the
>> clocksource through enum clocksource_ids, rather than through struct
>> clocksource *. The net effect of the patch series is that
>> get_device_system_crosststamp() callers can supply clocksource ids instead
>> of clocksource pointers, which can be problematic to get hold of.
>
> Hey Peter,
>   Thanks for sending this out. I'm a little curious though, can you
> expand a bit on how clocksource pointers can be problematic to get a
> hold of? What exactly is the problem that is motivating this change?
> 

Hi John,

I'm very sorry for the late reply; there was some unexpected delay.

Thank you for the remark; I'll expand on the motivation in the next patch
series iteration, similar to the explanation below.

The immediate motivation for this patch series is to enable the virtio_rtc
RFC v2 driver [4] to refer to the Arm Generic Timer without requiring new
helper functions in the arm_arch_timer driver. Other future
get_device_system_crosststamp() users may profit from this change as well.

Clocksource structs are normally private to clocksource drivers. Therefore,
get_device_system_crosststamp() callers require that clocksource drivers
expose the clocksource of interest in some way.

Drivers such as virtio_rtc [4] could obtain all information for calling
get_device_system_crosststamp() from their bound device, except for
clocksource identification. Such drivers' only direct relation with the
clocksource driver is clocksource identification. So using the clocksource
enum, rather than obtaining pointers in a clocksource driver specific way,
would reduce the coupling between the get_device_system_crosststamp()
callers and clocksource drivers.

Next, I provide some details to support the low coupling argument. There
are two sorts of get_device_system_crosststamp() callers in the current
kernel:

1) On Intel platforms, some PTP hardware clocks obtain the clocksource
pointer for get_device_system_crosststamp() using convert_art_to_tsc()
or convert_art_ns_to_tsc() from arch/x86.

2) The ptp_kvm driver uses kvm_arch_ptp_get_crosststamp(), which is
implemented for platforms with kvm_clock or arm_arch_timer.
Amongst other things, kvm_arch_ptp_get_crosststamp() returns a clocksource
pointer. The Arm implementation is in the arm_arch_timer driver.

When I proposed in the virtio_rtc RFC v1 patch series [3] to obtain the
clocksource pointer of the arm_arch_timer driver through a generic
helper function, one of the maintainers wasn't very enthusiastic about
it and suggested reusing kvm_arch_ptp_get_crosststamp() somehow [1]. But
to me there seems not to be much in common [2].

Quoting myself from [2]:

> If[!] &clocksource_counter should not be exposed, then I can see two
> alternatives:
> 
> Alternative 1: Put a function of type
> 
> 	int (*get_time_fn) (ktime_t *device_time, 
> 	                    struct system_counterval_t *sys_counterval,
> 			    void *ctx)
> 
> into arm_arch_timer.c, as required by get_device_system_crosststamp()
> (and include a virtio_rtc header).

This looks inelegant, since it would require virtio_rtc to put part of its
code into arm_arch_timer.c, and would require including a virtio_rtc header
in arm_arch_timer.c.

The second alternative is using this patch series to expand the use of the
clocksource enum to get_device_system_crosststamp(). This should also make
it easy to use get_device_system_crosststamp() with other clocksources in
the future, by just extending the clocksource enum.

> I just worry that switching to an enumeration solution might be
> eventually exposing more than we would like to userland.

ATM the enum is not in a UAPI header. So IMHO exposing this to userland in
the future would require a pretty explicit change.

Thanks for the review,

Peter

[1] https://lore.kernel.org/all/87ila4qwuw.wl-maz@kernel.org/
[2] https://lore.kernel.org/all/151befb2-8fbc-b796-47bb-39960a979065@opensynergy.com/
[3] https://lore.kernel.org/all/20230630171052.985577-1-peter.hilber@opensynergy.com/
[4] https://lore.kernel.org/all/20230818012014.212155-1-peter.hilber@opensynergy.com/