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/[email protected]/
[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
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/[email protected]/
Signed-off-by: Peter Hilber <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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/[email protected]/
Signed-off-by: Peter Hilber <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
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);
...
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 <[email protected]>
>
> 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
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