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

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

[Peter Hilber]

Overview
--------

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. The pointers can be problematic to get hold of.

The series is also available at

	https://github.com/OpenSynergy/linux clocksource-id-for-xtstamp-v2

Motivation
----------

The immediate motivation for this patch series is to enable the virtio_rtc
RFC driver (v2 cf. [1], v3 to be published after this series) to refer to
the Arm Generic Timer clocksource 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 could obtain all information for calling
get_device_system_crosststamp() from their bound device, except for
clocksource identification. Often, 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.

Affected Code
-------------

This series modifies code which is relevant to
get_device_system_crosststamp(), in timekeeping, ptp/kvm, x86/kvm, and
x86/tsc.

There are two sorts of get_device_system_crosststamp() callers in the
current kernel:

1) On Intel platforms, some PTP hardware clocks, and the HDA controller,
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 (x86) 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.

Changes
-------

The series does the following:

- add clocksource id to the get_device_system_crosststamp() param type

- add required clocksource ids and set them in
  get_device_system_crosststamp() users

- evaluate clocksource id in get_device_system_crosststamp(), rather than
  clocksource pointer

- remove now obsolete clocksource pointer field and related code

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
(of which RFC v3 is to be posted).

v2:

- Align existing changes with sketch [2] by Thomas Gleixner (omitting
  additional clocksource base changes from [2]).

- Add follow-up improvements in ptp_kvm and kvmclock.

- Split patches differently (Thomas Gleixner).

- Refer to clocksource IDs as such in comments (Thomas Gleixner).

- Update comments which were still referring to clocksource pointers.

[1] https://lore.kernel.org/all/20230818012014.212155-1-peter.hilber@opensynergy.com/
[2] https://lore.kernel.org/lkml/87lec15i4b.ffs@tglx/


Peter Hilber (7):
  timekeeping: Add clocksource ID to struct system_counterval_t
  x86/tsc: Add clocksource ID, set system_counterval_t.cs_id
  x86/kvm, ptp/kvm: Add clocksource ID, set system_counterval_t.cs_id
  ptp/kvm, arm_arch_timer: Set system_counterval_t.cs_id to constant
  timekeeping: Evaluate system_counterval_t.cs_id instead of .cs
  treewide: Remove system_counterval_t.cs, which is never read
  kvmclock: Unexport kvmclock clocksource

 arch/x86/include/asm/kvmclock.h      |  2 --
 arch/x86/kernel/kvmclock.c           |  5 +++--
 arch/x86/kernel/tsc.c                | 29 +++++++++++++++++-----------
 drivers/clocksource/arm_arch_timer.c |  6 +++---
 drivers/ptp/ptp_kvm_common.c         | 10 +++++-----
 drivers/ptp/ptp_kvm_x86.c            |  4 ++--
 include/linux/clocksource_ids.h      |  3 +++
 include/linux/ptp_kvm.h              |  4 ++--
 include/linux/timekeeping.h          | 10 ++++++----
 kernel/time/timekeeping.c            |  9 +++++----
 10 files changed, 47 insertions(+), 35 deletions(-)


base-commit: 17cb8a20bde66a520a2ca7aad1063e1ce7382240
-- 
2.40.1

[RFC PATCH v2 1/7] timekeeping: Add clocksource ID to struct system_counterval_t

[Peter Hilber]

Clocksource pointers can be problematic to obtain for drivers which are not
clocksource drivers themselves. In particular, the RFC virtio_rtc driver
[1] would require a new helper function to obtain a pointer to the Arm
Generic Timer clocksource. The ptp_kvm driver also required a similar
workaround.

Add a clocksource ID member to struct system_counterval_t, which in the
future shall identify the clocksource, and shall replace the struct
clocksource * member. By this, get_device_system_crosststamp() callers
(such as virtio_rtc and ptp_kvm) will be able to supply easily accessible
clocksource ids instead of clocksource pointers.

[1] https://lore.kernel.org/lkml/20230818012014.212155-1-peter.hilber@opensynergy.com/

Signed-off-by: Peter Hilber <peter.hilber@opensynergy.com>
---

Notes:
    v2:
    
    - Refer to clocksource IDs as such in comments (Thomas Gleixner).

 include/linux/timekeeping.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
index fe1e467ba046..74dc7c8b036f 100644
--- a/include/linux/timekeeping.h
+++ b/include/linux/timekeeping.h
@@ -272,10 +272,15 @@ struct system_device_crosststamp {
  * @cycles:	System counter value
  * @cs:		Clocksource corresponding to system counter value. Used by
  *		timekeeping code to verify comparibility of two cycle values
+ * @cs_id:	Clocksource ID corresponding to system counter value. To be
+ *		used instead of cs in the future.
+ *		The default ID, CSID_GENERIC, does not identify a specific
+ *		clocksource.
  */
 struct system_counterval_t {
 	u64			cycles;
 	struct clocksource	*cs;
+	enum clocksource_ids	cs_id;
 };
 
 /*
-- 
2.40.1

[RFC PATCH v2 2/7] x86/tsc: Add clocksource ID, set system_counterval_t.cs_id

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

Also, set the recently added struct system_counterval_t member cs_id to the
TSC ID in the cases where the clocksource member is being set to the TSC
clocksource. In the future, this will keep get_device_system_crosststamp()
working, when it will compare the clocksource id in struct
system_counterval_t, rather than the clocksource.

For the x86 ART related code, system_counterval_t.cs == NULL corresponds to
system_counterval_t.cs_id == CSID_GENERIC (0).

Signed-off-by: Peter Hilber <peter.hilber@opensynergy.com>
---

Notes:
    v2:
    
    - Name clock id according to Thomas Gleixner's mockup.
    
    - Refer to clocksource IDs as such in comments (Thomas Gleixner).
    
    - Update comments which were still referring to clocksource pointers.

 arch/x86/kernel/tsc.c           | 31 ++++++++++++++++++++++++-------
 include/linux/clocksource_ids.h |  2 ++
 2 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 15f97c0abc9d..9367174f7920 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>
@@ -54,6 +55,7 @@ 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 */
@@ -1168,6 +1170,7 @@ static struct clocksource clocksource_tsc_early = {
 	.mask			= CLOCKSOURCE_MASK(64),
 	.flags			= CLOCK_SOURCE_IS_CONTINUOUS |
 				  CLOCK_SOURCE_MUST_VERIFY,
+	.id			= CSID_X86_TSC_EARLY,
 	.vdso_clock_mode	= VDSO_CLOCKMODE_TSC,
 	.enable			= tsc_cs_enable,
 	.resume			= tsc_resume,
@@ -1190,6 +1193,7 @@ static struct clocksource clocksource_tsc = {
 				  CLOCK_SOURCE_VALID_FOR_HRES |
 				  CLOCK_SOURCE_MUST_VERIFY |
 				  CLOCK_SOURCE_VERIFY_PERCPU,
+	.id			= CSID_X86_TSC,
 	.vdso_clock_mode	= VDSO_CLOCKMODE_TSC,
 	.enable			= tsc_cs_enable,
 	.resume			= tsc_resume,
@@ -1309,8 +1313,11 @@ 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 = art_related_clocksource,
+		.cs_id = have_art ? CSID_X86_TSC : CSID_GENERIC,
+		.cycles = res
+	};
 }
 EXPORT_SYMBOL(convert_art_to_tsc);
 
@@ -1327,12 +1334,15 @@ EXPORT_SYMBOL(convert_art_to_tsc);
  * that this flag is set before conversion to TSC is attempted.
  *
  * Return:
- * struct system_counterval_t - system counter value with the pointer to the
+ * struct system_counterval_t - system counter value with the ID of the
  *	corresponding clocksource
  *	@cycles:	System counter value
  *	@cs:		Clocksource corresponding to system counter value. Used
  *			by timekeeping code to verify comparability of two cycle
  *			values.
+ *	@cs_id:		Clocksource ID corresponding to system counter value.
+ *			Used by timekeeping code to verify comparability of two
+ *			cycle values.
  */
 
 struct system_counterval_t convert_art_ns_to_tsc(u64 art_ns)
@@ -1347,8 +1357,11 @@ 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 = art_related_clocksource,
+		.cs_id = have_art ? CSID_X86_TSC : CSID_GENERIC,
+		.cycles = res
+	};
 }
 EXPORT_SYMBOL(convert_art_ns_to_tsc);
 
@@ -1454,8 +1467,10 @@ static void tsc_refine_calibration_work(struct work_struct *work)
 	if (tsc_unstable)
 		goto unreg;
 
-	if (boot_cpu_has(X86_FEATURE_ART))
+	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);
@@ -1480,8 +1495,10 @@ static int __init init_tsc_clocksource(void)
 	 * the refined calibration and directly register it as a clocksource.
 	 */
 	if (boot_cpu_has(X86_FEATURE_TSC_KNOWN_FREQ)) {
-		if (boot_cpu_has(X86_FEATURE_ART))
+		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);
 
diff --git a/include/linux/clocksource_ids.h b/include/linux/clocksource_ids.h
index 16775d7d8f8d..f8467946e9ee 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_X86_TSC_EARLY,
+	CSID_X86_TSC,
 	CSID_MAX,
 };
 
-- 
2.40.1

[RFC PATCH v2 3/7] x86/kvm, ptp/kvm: Add clocksource ID, set system_counterval_t.cs_id

[Peter Hilber]

Add a clocksource ID for the x86 kvmclock.

Also, for ptp_kvm, set the recently added struct system_counterval_t member
cs_id to the clocksource ID (x86 kvmclock or Arm Generic Timer). In the
future, this will keep get_device_system_crosststamp() working, when it
will compare the clocksource id in struct system_counterval_t, rather than
the clocksource.

For now, to avoid touching too many subsystems at once, extract the
clocksource ID from the clocksource. The clocksource dereference will be
removed in the following.

Signed-off-by: Peter Hilber <peter.hilber@opensynergy.com>
---

Notes:
    v2:
    
    - Name clock id according to Thomas Gleixner's mockup.

 arch/x86/kernel/kvmclock.c      | 2 ++
 drivers/ptp/ptp_kvm_common.c    | 2 ++
 include/linux/clocksource_ids.h | 1 +
 3 files changed, 5 insertions(+)

diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
index fb8f52149be9..25d6bf743b03 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_X86_KVM_CLK,
 	.enable	= kvm_cs_enable,
 };
 EXPORT_SYMBOL_GPL(kvm_clock);
diff --git a/drivers/ptp/ptp_kvm_common.c b/drivers/ptp/ptp_kvm_common.c
index 2418977989be..b0b36f135347 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>
@@ -47,6 +48,7 @@ static int ptp_kvm_get_time_fn(ktime_t *device_time,
 
 	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/clocksource_ids.h b/include/linux/clocksource_ids.h
index f8467946e9ee..a4fa3436940c 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_X86_TSC_EARLY,
 	CSID_X86_TSC,
+	CSID_X86_KVM_CLK,
 	CSID_MAX,
 };
 
-- 
2.40.1

[RFC PATCH v2 4/7] ptp/kvm, arm_arch_timer: Set system_counterval_t.cs_id to constant

[Peter Hilber]

Identify the clocksources used by ptp_kvm by setting clocksource ID enum
constants. This avoids dereferencing struct clocksource. Once the
system_counterval_t.cs member will be removed, this will also avoid the
need to obtain clocksource pointers from kvm_arch_ptp_get_crosststamp().

The clocksource IDs are associated to timestamps requested from the KVM
hypervisor, so the proper clocksource ID is known at the ptp_kvm request
site.

While at it, also rectify the ptp_kvm_get_time_fn() ret type.

Signed-off-by: Peter Hilber <peter.hilber@opensynergy.com>
---

Notes:
    v2:
    
    Added in v2.

 drivers/clocksource/arm_arch_timer.c |  5 ++++-
 drivers/ptp/ptp_kvm_arm.c            |  2 +-
 drivers/ptp/ptp_kvm_common.c         | 10 +++++-----
 drivers/ptp/ptp_kvm_x86.c            |  4 +++-
 include/linux/ptp_kvm.h              |  4 +++-
 5 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index e054de92de91..45a02872669e 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -1807,7 +1807,8 @@ TIMER_ACPI_DECLARE(arch_timer, ACPI_SIG_GTDT, arch_timer_acpi_init);
 #endif
 
 int kvm_arch_ptp_get_crosststamp(u64 *cycle, struct timespec64 *ts,
-				 struct clocksource **cs)
+				 struct clocksource **cs,
+				 enum clocksource_ids *cs_id)
 {
 	struct arm_smccc_res hvc_res;
 	u32 ptp_counter;
@@ -1833,6 +1834,8 @@ int kvm_arch_ptp_get_crosststamp(u64 *cycle, struct timespec64 *ts,
 		*cycle = (u64)hvc_res.a2 << 32 | hvc_res.a3;
 	if (cs)
 		*cs = &clocksource_counter;
+	if (cs_id)
+		*cs_id = CSID_ARM_ARCH_COUNTER;
 
 	return 0;
 }
diff --git a/drivers/ptp/ptp_kvm_arm.c b/drivers/ptp/ptp_kvm_arm.c
index e68e6943167b..017bb5f03b14 100644
--- a/drivers/ptp/ptp_kvm_arm.c
+++ b/drivers/ptp/ptp_kvm_arm.c
@@ -28,5 +28,5 @@ void kvm_arch_ptp_exit(void)
 
 int kvm_arch_ptp_get_clock(struct timespec64 *ts)
 {
-	return kvm_arch_ptp_get_crosststamp(NULL, ts, NULL);
+	return kvm_arch_ptp_get_crosststamp(NULL, ts, NULL, NULL);
 }
diff --git a/drivers/ptp/ptp_kvm_common.c b/drivers/ptp/ptp_kvm_common.c
index b0b36f135347..f6683ba0ab3c 100644
--- a/drivers/ptp/ptp_kvm_common.c
+++ b/drivers/ptp/ptp_kvm_common.c
@@ -4,7 +4,6 @@
  *
  * Copyright (C) 2017 Red Hat Inc.
  */
-#include <linux/clocksource.h>
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/init.h>
@@ -29,15 +28,16 @@ static int ptp_kvm_get_time_fn(ktime_t *device_time,
 			       struct system_counterval_t *system_counter,
 			       void *ctx)
 {
-	long ret;
-	u64 cycle;
+	enum clocksource_ids cs_id;
 	struct timespec64 tspec;
 	struct clocksource *cs;
+	u64 cycle;
+	int ret;
 
 	spin_lock(&kvm_ptp_lock);
 
 	preempt_disable_notrace();
-	ret = kvm_arch_ptp_get_crosststamp(&cycle, &tspec, &cs);
+	ret = kvm_arch_ptp_get_crosststamp(&cycle, &tspec, &cs, &cs_id);
 	if (ret) {
 		spin_unlock(&kvm_ptp_lock);
 		preempt_enable_notrace();
@@ -48,7 +48,7 @@ static int ptp_kvm_get_time_fn(ktime_t *device_time,
 
 	system_counter->cycles = cycle;
 	system_counter->cs = cs;
-	system_counter->cs_id = cs->id;
+	system_counter->cs_id = cs_id;
 
 	*device_time = timespec64_to_ktime(tspec);
 
diff --git a/drivers/ptp/ptp_kvm_x86.c b/drivers/ptp/ptp_kvm_x86.c
index 902844cc1a17..2782442922cb 100644
--- a/drivers/ptp/ptp_kvm_x86.c
+++ b/drivers/ptp/ptp_kvm_x86.c
@@ -93,7 +93,8 @@ int kvm_arch_ptp_get_clock(struct timespec64 *ts)
 }
 
 int kvm_arch_ptp_get_crosststamp(u64 *cycle, struct timespec64 *tspec,
-			      struct clocksource **cs)
+			      struct clocksource **cs,
+			      enum clocksource_ids *cs_id)
 {
 	struct pvclock_vcpu_time_info *src;
 	unsigned int version;
@@ -124,6 +125,7 @@ int kvm_arch_ptp_get_crosststamp(u64 *cycle, struct timespec64 *tspec,
 	} while (pvclock_read_retry(src, version));
 
 	*cs = &kvm_clock;
+	*cs_id = CSID_X86_KVM_CLK;
 
 	return 0;
 }
diff --git a/include/linux/ptp_kvm.h b/include/linux/ptp_kvm.h
index 746fd67c3480..95b3d4d0d7dd 100644
--- a/include/linux/ptp_kvm.h
+++ b/include/linux/ptp_kvm.h
@@ -8,6 +8,7 @@
 #ifndef _PTP_KVM_H_
 #define _PTP_KVM_H_
 
+#include <linux/clocksource_ids.h>
 #include <linux/types.h>
 
 struct timespec64;
@@ -17,6 +18,7 @@ int kvm_arch_ptp_init(void);
 void kvm_arch_ptp_exit(void);
 int kvm_arch_ptp_get_clock(struct timespec64 *ts);
 int kvm_arch_ptp_get_crosststamp(u64 *cycle,
-		struct timespec64 *tspec, struct clocksource **cs);
+		struct timespec64 *tspec, struct clocksource **cs,
+		enum clocksource_ids *cs_id);
 
 #endif /* _PTP_KVM_H_ */
-- 
2.40.1

[RFC PATCH v2 5/7] timekeeping: Evaluate system_counterval_t.cs_id instead of .cs

[Peter Hilber]

Clocksource pointers can be problematic to obtain for drivers which are not
clocksource drivers themselves. In particular, the RFC virtio_rtc driver
[1] would require a new helper function to obtain a pointer to the Arm
Generic Timer clocksource. The ptp_kvm driver also required a similar
workaround.

Address this by evaluating the clocksource ID, rather than the clocksource
pointer, of struct system_counterval_t. By this, setting the clocksource
pointer becomes unneeded, and it will be dropped from struct
system_counterval_t in the future. By this, get_device_system_crosststamp()
callers (such as virtio_rtc and ptp_kvm) will no longer need to supply
clocksource pointers.

This change should not alter any behavior, as the struct
system_counterval_t clocksource ID is already being set wherever the
clocksource pointer is set. get_device_system_crosststamp() will now fail
if the clocksource has id CSID_GENERIC, but all currently relevant
clocksources have a custom clocksource id.

[1] https://lore.kernel.org/lkml/20230818012014.212155-1-peter.hilber@opensynergy.com/

Signed-off-by: Peter Hilber <peter.hilber@opensynergy.com>
---

Notes:
    v2:
    
    - Refer to clocksource IDs as such in comments (Thomas Gleixner).
    
    - Update comments which were still referring to clocksource pointers.

 include/linux/timekeeping.h | 10 +++++-----
 kernel/time/timekeeping.c   |  9 +++++----
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
index 74dc7c8b036f..75e957171bd5 100644
--- a/include/linux/timekeeping.h
+++ b/include/linux/timekeeping.h
@@ -267,13 +267,13 @@ struct system_device_crosststamp {
 };
 
 /**
- * struct system_counterval_t - system counter value with the pointer to the
+ * struct system_counterval_t - system counter value with the ID of the
  *				corresponding clocksource
  * @cycles:	System counter value
- * @cs:		Clocksource corresponding to system counter value. Used by
- *		timekeeping code to verify comparibility of two cycle values
- * @cs_id:	Clocksource ID corresponding to system counter value. To be
- *		used instead of cs in the future.
+ * @cs:		Clocksource corresponding to system counter value. Timekeeping
+ *		code now evaluates cs_id instead.
+ * @cs_id:	Clocksource ID corresponding to system counter value. Used by
+ *		timekeeping code to verify comparability of two cycle values.
  *		The default ID, CSID_GENERIC, does not identify a specific
  *		clocksource.
  */
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 266d02809dbb..0ff065c5d25b 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1232,11 +1232,12 @@ int get_device_system_crosststamp(int (*get_time_fn)
 			return ret;
 
 		/*
-		 * Verify that the clocksource associated with the captured
-		 * system counter value is the same as the currently installed
-		 * timekeeper clocksource
+		 * Verify that the clocksource ID associated with the captured
+		 * system counter value is the same as for 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.40.1

[RFC PATCH v2 6/7] treewide: Remove system_counterval_t.cs, which is never read

[Peter Hilber]

The clocksource pointer in struct system_counterval_t is not evaluated any
more. Remove the code setting the member, and the member itself.

Signed-off-by: Peter Hilber <peter.hilber@opensynergy.com>
---
 arch/x86/kernel/tsc.c                | 14 ++------------
 drivers/clocksource/arm_arch_timer.c |  3 ---
 drivers/ptp/ptp_kvm_arm.c            |  2 +-
 drivers/ptp/ptp_kvm_common.c         |  4 +---
 drivers/ptp/ptp_kvm_x86.c            |  2 --
 include/linux/ptp_kvm.h              |  4 +---
 include/linux/timekeeping.h          |  3 ---
 7 files changed, 5 insertions(+), 27 deletions(-)

diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 9367174f7920..868f09966b0f 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -54,7 +54,6 @@ 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 {
@@ -1314,7 +1313,6 @@ struct system_counterval_t convert_art_to_tsc(u64 art)
 	res += tmp + art_to_tsc_offset;
 
 	return (struct system_counterval_t) {
-		.cs = art_related_clocksource,
 		.cs_id = have_art ? CSID_X86_TSC : CSID_GENERIC,
 		.cycles = res
 	};
@@ -1337,9 +1335,6 @@ EXPORT_SYMBOL(convert_art_to_tsc);
  * struct system_counterval_t - system counter value with the ID of the
  *	corresponding clocksource
  *	@cycles:	System counter value
- *	@cs:		Clocksource corresponding to system counter value. Used
- *			by timekeeping code to verify comparability of two cycle
- *			values.
  *	@cs_id:		Clocksource ID corresponding to system counter value.
  *			Used by timekeeping code to verify comparability of two
  *			cycle values.
@@ -1358,7 +1353,6 @@ struct system_counterval_t convert_art_ns_to_tsc(u64 art_ns)
 	res += tmp;
 
 	return (struct system_counterval_t) {
-		.cs = art_related_clocksource,
 		.cs_id = have_art ? CSID_X86_TSC : CSID_GENERIC,
 		.cycles = res
 	};
@@ -1467,10 +1461,8 @@ static void tsc_refine_calibration_work(struct work_struct *work)
 	if (tsc_unstable)
 		goto unreg;
 
-	if (boot_cpu_has(X86_FEATURE_ART)) {
-		art_related_clocksource = &clocksource_tsc;
+	if (boot_cpu_has(X86_FEATURE_ART))
 		have_art = true;
-	}
 	clocksource_register_khz(&clocksource_tsc, tsc_khz);
 unreg:
 	clocksource_unregister(&clocksource_tsc_early);
@@ -1495,10 +1487,8 @@ static int __init init_tsc_clocksource(void)
 	 * the refined calibration and directly register it as a clocksource.
 	 */
 	if (boot_cpu_has(X86_FEATURE_TSC_KNOWN_FREQ)) {
-		if (boot_cpu_has(X86_FEATURE_ART)) {
-			art_related_clocksource = &clocksource_tsc;
+		if (boot_cpu_has(X86_FEATURE_ART))
 			have_art = true;
-		}
 		clocksource_register_khz(&clocksource_tsc, tsc_khz);
 		clocksource_unregister(&clocksource_tsc_early);
 
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 45a02872669e..8d4a52056684 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -1807,7 +1807,6 @@ TIMER_ACPI_DECLARE(arch_timer, ACPI_SIG_GTDT, arch_timer_acpi_init);
 #endif
 
 int kvm_arch_ptp_get_crosststamp(u64 *cycle, struct timespec64 *ts,
-				 struct clocksource **cs,
 				 enum clocksource_ids *cs_id)
 {
 	struct arm_smccc_res hvc_res;
@@ -1832,8 +1831,6 @@ int kvm_arch_ptp_get_crosststamp(u64 *cycle, struct timespec64 *ts,
 	*ts = ktime_to_timespec64(ktime);
 	if (cycle)
 		*cycle = (u64)hvc_res.a2 << 32 | hvc_res.a3;
-	if (cs)
-		*cs = &clocksource_counter;
 	if (cs_id)
 		*cs_id = CSID_ARM_ARCH_COUNTER;
 
diff --git a/drivers/ptp/ptp_kvm_arm.c b/drivers/ptp/ptp_kvm_arm.c
index 017bb5f03b14..e68e6943167b 100644
--- a/drivers/ptp/ptp_kvm_arm.c
+++ b/drivers/ptp/ptp_kvm_arm.c
@@ -28,5 +28,5 @@ void kvm_arch_ptp_exit(void)
 
 int kvm_arch_ptp_get_clock(struct timespec64 *ts)
 {
-	return kvm_arch_ptp_get_crosststamp(NULL, ts, NULL, NULL);
+	return kvm_arch_ptp_get_crosststamp(NULL, ts, NULL);
 }
diff --git a/drivers/ptp/ptp_kvm_common.c b/drivers/ptp/ptp_kvm_common.c
index f6683ba0ab3c..15ccb7dd2ed0 100644
--- a/drivers/ptp/ptp_kvm_common.c
+++ b/drivers/ptp/ptp_kvm_common.c
@@ -30,14 +30,13 @@ static int ptp_kvm_get_time_fn(ktime_t *device_time,
 {
 	enum clocksource_ids cs_id;
 	struct timespec64 tspec;
-	struct clocksource *cs;
 	u64 cycle;
 	int ret;
 
 	spin_lock(&kvm_ptp_lock);
 
 	preempt_disable_notrace();
-	ret = kvm_arch_ptp_get_crosststamp(&cycle, &tspec, &cs, &cs_id);
+	ret = kvm_arch_ptp_get_crosststamp(&cycle, &tspec, &cs_id);
 	if (ret) {
 		spin_unlock(&kvm_ptp_lock);
 		preempt_enable_notrace();
@@ -47,7 +46,6 @@ 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/drivers/ptp/ptp_kvm_x86.c b/drivers/ptp/ptp_kvm_x86.c
index 2782442922cb..617c8d6706d3 100644
--- a/drivers/ptp/ptp_kvm_x86.c
+++ b/drivers/ptp/ptp_kvm_x86.c
@@ -93,7 +93,6 @@ int kvm_arch_ptp_get_clock(struct timespec64 *ts)
 }
 
 int kvm_arch_ptp_get_crosststamp(u64 *cycle, struct timespec64 *tspec,
-			      struct clocksource **cs,
 			      enum clocksource_ids *cs_id)
 {
 	struct pvclock_vcpu_time_info *src;
@@ -124,7 +123,6 @@ int kvm_arch_ptp_get_crosststamp(u64 *cycle, struct timespec64 *tspec,
 		*cycle = __pvclock_read_cycles(src, clock_pair->tsc);
 	} while (pvclock_read_retry(src, version));
 
-	*cs = &kvm_clock;
 	*cs_id = CSID_X86_KVM_CLK;
 
 	return 0;
diff --git a/include/linux/ptp_kvm.h b/include/linux/ptp_kvm.h
index 95b3d4d0d7dd..e8c74fa3f455 100644
--- a/include/linux/ptp_kvm.h
+++ b/include/linux/ptp_kvm.h
@@ -12,13 +12,11 @@
 #include <linux/types.h>
 
 struct timespec64;
-struct clocksource;
 
 int kvm_arch_ptp_init(void);
 void kvm_arch_ptp_exit(void);
 int kvm_arch_ptp_get_clock(struct timespec64 *ts);
 int kvm_arch_ptp_get_crosststamp(u64 *cycle,
-		struct timespec64 *tspec, struct clocksource **cs,
-		enum clocksource_ids *cs_id);
+		struct timespec64 *tspec, enum clocksource_ids *cs_id);
 
 #endif /* _PTP_KVM_H_ */
diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
index 75e957171bd5..0f00f382bb5d 100644
--- a/include/linux/timekeeping.h
+++ b/include/linux/timekeeping.h
@@ -270,8 +270,6 @@ struct system_device_crosststamp {
  * struct system_counterval_t - system counter value with the ID of the
  *				corresponding clocksource
  * @cycles:	System counter value
- * @cs:		Clocksource corresponding to system counter value. Timekeeping
- *		code now evaluates cs_id instead.
  * @cs_id:	Clocksource ID corresponding to system counter value. Used by
  *		timekeeping code to verify comparability of two cycle values.
  *		The default ID, CSID_GENERIC, does not identify a specific
@@ -279,7 +277,6 @@ struct system_device_crosststamp {
  */
 struct system_counterval_t {
 	u64			cycles;
-	struct clocksource	*cs;
 	enum clocksource_ids	cs_id;
 };
 
-- 
2.40.1

[RFC PATCH v2 7/7] kvmclock: Unexport kvmclock clocksource

[Peter Hilber]

The KVM PTP driver now refers to the clocksource id CSID_X86_KVM_CLK, so
does not need to refer to the clocksource itself any more. There are no
remaining users of the clocksource export.

Therefore, make the clocksource static again.

Signed-off-by: Peter Hilber <peter.hilber@opensynergy.com>
---

Notes:
    v2:
    
    Added in v2.

 arch/x86/include/asm/kvmclock.h | 2 --
 arch/x86/kernel/kvmclock.c      | 3 +--
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/kvmclock.h b/arch/x86/include/asm/kvmclock.h
index 511b35069187..f163176d6f7f 100644
--- a/arch/x86/include/asm/kvmclock.h
+++ b/arch/x86/include/asm/kvmclock.h
@@ -4,8 +4,6 @@
 
 #include <linux/percpu.h>
 
-extern struct clocksource kvm_clock;
-
 DECLARE_PER_CPU(struct pvclock_vsyscall_time_info *, hv_clock_per_cpu);
 
 static __always_inline struct pvclock_vcpu_time_info *this_cpu_pvti(void)
diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
index 25d6bf743b03..9dfbcd2f4244 100644
--- a/arch/x86/kernel/kvmclock.c
+++ b/arch/x86/kernel/kvmclock.c
@@ -155,7 +155,7 @@ static int kvm_cs_enable(struct clocksource *cs)
 	return 0;
 }
 
-struct clocksource kvm_clock = {
+static struct clocksource kvm_clock = {
 	.name	= "kvm-clock",
 	.read	= kvm_clock_get_cycles,
 	.rating	= 400,
@@ -164,7 +164,6 @@ struct clocksource kvm_clock = {
 	.id     = CSID_X86_KVM_CLK,
 	.enable	= kvm_cs_enable,
 };
-EXPORT_SYMBOL_GPL(kvm_clock);
 
 static void kvm_register_clock(char *txt)
 {
-- 
2.40.1

Re: [RFC PATCH v2 3/7] x86/kvm, ptp/kvm: Add clocksource ID, set system_counterval_t.cs_id

[Andy Shevchenko]

On Fri, Dec 15, 2023 at 11:06:08PM +0100, Peter Hilber wrote:
> Add a clocksource ID for the x86 kvmclock.
> 
> Also, for ptp_kvm, set the recently added struct system_counterval_t member
> cs_id to the clocksource ID (x86 kvmclock or Arm Generic Timer). In the
> future, this will keep get_device_system_crosststamp() working, when it
> will compare the clocksource id in struct system_counterval_t, rather than
> the clocksource.
> 
> For now, to avoid touching too many subsystems at once, extract the
> clocksource ID from the clocksource. The clocksource dereference will be
> removed in the following.

...

>  #include <linux/clocksource.h>
> +#include <linux/clocksource_ids.h>

It's the second file that includes both.

I'm just wondering if it makes sense to always (?) include the latter into
the former.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH v2 2/7] x86/tsc: Add clocksource ID, set system_counterval_t.cs_id

[Simon Horman]

On Fri, Dec 15, 2023 at 11:06:07PM +0100, Peter Hilber wrote:
> 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.
> 
> Also, set the recently added struct system_counterval_t member cs_id to the
> TSC ID in the cases where the clocksource member is being set to the TSC
> clocksource. In the future, this will keep get_device_system_crosststamp()
> working, when it will compare the clocksource id in struct
> system_counterval_t, rather than the clocksource.
> 
> For the x86 ART related code, system_counterval_t.cs == NULL corresponds to
> system_counterval_t.cs_id == CSID_GENERIC (0).
> 
> Signed-off-by: Peter Hilber <peter.hilber@opensynergy.com>

Hi Peter,

some minor feedback from my side that you may consider for
a future revision.

> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c

...

> @@ -1327,12 +1334,15 @@ EXPORT_SYMBOL(convert_art_to_tsc);
>   * that this flag is set before conversion to TSC is attempted.
>   *
>   * Return:
> - * struct system_counterval_t - system counter value with the pointer to the
> + * struct system_counterval_t - system counter value with the ID of the
>   *	corresponding clocksource
>   *	@cycles:	System counter value
>   *	@cs:		Clocksource corresponding to system counter value. Used
>   *			by timekeeping code to verify comparability of two cycle
>   *			values.
> + *	@cs_id:		Clocksource ID corresponding to system counter value.
> + *			Used by timekeeping code to verify comparability of two
> + *			cycle values.

None of the documented parameters to convert_art_ns_to_tsc() above
correspond to the parameters of convert_art_ns_to_tsc() below.

I would suggest a separate patch to address this.
And dropping this hunk from this patch.

The same patch that corrects the kernel doc for convert_art_ns_to_tsc()
could also correct the kernel doc for tsc_refine_calibration_work()
by documenting it's work parameter.

>   */
>  
>  struct system_counterval_t convert_art_ns_to_tsc(u64 art_ns)
> @@ -1347,8 +1357,11 @@ 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 = art_related_clocksource,
> +		.cs_id = have_art ? CSID_X86_TSC : CSID_GENERIC,
> +		.cycles = res
> +	};
>  }
>  EXPORT_SYMBOL(convert_art_ns_to_tsc);
>  
> @@ -1454,8 +1467,10 @@ static void tsc_refine_calibration_work(struct work_struct *work)
>  	if (tsc_unstable)
>  		goto unreg;
>  
> -	if (boot_cpu_has(X86_FEATURE_ART))
> +	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);

...

Re: [RFC PATCH v2 3/7] x86/kvm, ptp/kvm: Add clocksource ID, set system_counterval_t.cs_id

[Peter Hilber]

On 18.12.23 11:23, Andy Shevchenko wrote:
> On Fri, Dec 15, 2023 at 11:06:08PM +0100, Peter Hilber wrote:
>> Add a clocksource ID for the x86 kvmclock.
>>
>> Also, for ptp_kvm, set the recently added struct system_counterval_t member
>> cs_id to the clocksource ID (x86 kvmclock or Arm Generic Timer). In the
>> future, this will keep get_device_system_crosststamp() working, when it
>> will compare the clocksource id in struct system_counterval_t, rather than
>> the clocksource.
>>
>> For now, to avoid touching too many subsystems at once, extract the
>> clocksource ID from the clocksource. The clocksource dereference will be
>> removed in the following.
> 
> ...
> 
>>  #include <linux/clocksource.h>
>> +#include <linux/clocksource_ids.h>
> 
> It's the second file that includes both.
> 
> I'm just wondering if it makes sense to always (?) include the latter into
> the former.
> 

Actually, clocksource.h already includes clocksource_ids.h, always since
the latter was created. So I'll just omit the unnecessary clocksource_ids.h
includes in other files.

Thanks for the comment,

Peter

Re: [RFC PATCH v2 2/7] x86/tsc: Add clocksource ID, set system_counterval_t.cs_id

[Peter Hilber]

On 24.12.23 17:27, Simon Horman wrote:
> On Fri, Dec 15, 2023 at 11:06:07PM +0100, Peter Hilber wrote:
>> 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.
>>
>> Also, set the recently added struct system_counterval_t member cs_id to the
>> TSC ID in the cases where the clocksource member is being set to the TSC
>> clocksource. In the future, this will keep get_device_system_crosststamp()
>> working, when it will compare the clocksource id in struct
>> system_counterval_t, rather than the clocksource.
>>
>> For the x86 ART related code, system_counterval_t.cs == NULL corresponds to
>> system_counterval_t.cs_id == CSID_GENERIC (0).
>>
>> Signed-off-by: Peter Hilber <peter.hilber@opensynergy.com>
> 
> Hi Peter,
> 
> some minor feedback from my side that you may consider for
> a future revision.
> 
>> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> 
> ...
> 
>> @@ -1327,12 +1334,15 @@ EXPORT_SYMBOL(convert_art_to_tsc);
>>   * that this flag is set before conversion to TSC is attempted.
>>   *
>>   * Return:
>> - * struct system_counterval_t - system counter value with the pointer to the
>> + * struct system_counterval_t - system counter value with the ID of the
>>   *	corresponding clocksource
>>   *	@cycles:	System counter value
>>   *	@cs:		Clocksource corresponding to system counter value. Used
>>   *			by timekeeping code to verify comparability of two cycle
>>   *			values.
>> + *	@cs_id:		Clocksource ID corresponding to system counter value.
>> + *			Used by timekeeping code to verify comparability of two
>> + *			cycle values.
> 
> None of the documented parameters to convert_art_ns_to_tsc() above
> correspond to the parameters of convert_art_ns_to_tsc() below.
> 
> I would suggest a separate patch to address this.
> And dropping this hunk from this patch.
> 

In the quoted documentation, @cycles, @cs and @cs_id document members of
the return type struct system_counterval_t (not parameters). I will just
drop the members documentation, since they are documented at the struct
definition site anyway.

> The same patch that corrects the kernel doc for convert_art_ns_to_tsc()
> could also correct the kernel doc for tsc_refine_calibration_work()
> by documenting it's work parameter.
> 

OK.

Thanks for the comments,

Peter

Re: [RFC PATCH v2 2/7] x86/tsc: Add clocksource ID, set system_counterval_t.cs_id

[Thomas Gleixner]

On Sun, Dec 24 2023 at 16:27, Simon Horman wrote:
> On Fri, Dec 15, 2023 at 11:06:07PM +0100, Peter Hilber wrote:
>> @@ -1327,12 +1334,15 @@ EXPORT_SYMBOL(convert_art_to_tsc);
>>   * that this flag is set before conversion to TSC is attempted.
>>   *
>>   * Return:
>> - * struct system_counterval_t - system counter value with the pointer to the
>> + * struct system_counterval_t - system counter value with the ID of the
>>   *	corresponding clocksource
>>   *	@cycles:	System counter value
>>   *	@cs:		Clocksource corresponding to system counter value. Used
>>   *			by timekeeping code to verify comparability of two cycle
>>   *			values.
>> + *	@cs_id:		Clocksource ID corresponding to system counter value.
>> + *			Used by timekeeping code to verify comparability of two
>> + *			cycle values.
>
> None of the documented parameters to convert_art_ns_to_tsc() above
> correspond to the parameters of convert_art_ns_to_tsc() below.

Obviously not because they document the return value. The sole argument
of the function @art_ns is documented correctly.

> The same patch that corrects the kernel doc for convert_art_ns_to_tsc()
> could also correct the kernel doc for tsc_refine_calibration_work()
> by documenting it's work parameter.

That's a separate cleanup. Feel free to send a patch for that.

Thanks,

        tglx