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