V5:
It was decided to split the series
"make L2's kvm-clock stable, get rid of pvclock_gtod_copy in KVM"
into two parts to make the changes finer grained:
1. get rid of pvclock_gtod_copy in KVM
2. make L2's kvm-clock stable
This V5 series implements the 1st part. This part gets rid
of pvclock_gtod_copy by switching to use the timekeeper functionality
being extended. This new scheme doesn't add any new notifiers and keeps
using the existing one (see patch 6 description) which had been used by
the pvclock_gtod_copy to keep track timekeeper data changes.
The 2nd part is being discussed and is going to be sent later on.
V4:
* removed "is stable" function with vague definition of stability
there is the only function which does time with cycle stamp getting
* some variables renamed
* some patches split into smaller once
* atomic64_t usage is replaced with atomic_t
V3:
Changing the timekeeper interface for clocksource reading looks like
an overkill to achive the goal of getting cycles stamp for KVM.
Instead extend the timekeeping interface and add functions which provide
necessary data: read clocksource with cycles stamp, check whether the
clock source is stable.
Use those functions and improve existing timekeeper functionality to
replace pvclock_gtod_copy scheme in masterclock data calculation.
V2:
The main goal is to make L2 kvm-clock be stable when it's running over L1
with stable kvm-clock.
The patch series is for x86 architecture only. If the series is approved
I'll do changes for other architectures but I don't have an ability to
compile and check for every single on (help needed)
The patch series do the following:
* change timekeeper interface to get cycles stamp value from
the timekeeper
* get rid of pvclock copy in KVM by using the changed timekeeper
interface: get time and cycles right from the timekeeper
* make KVM recognize a stable kvm-clock as stable clocksource
and use the KVM masterclock in this case, which means making
L2 stable when running over stable L1 kvm-clock
Denis Plotnikov (6):
timekeeper: introduce extended clocksource reading callback
timekeeper: introduce boot field in system_time_snapshot
timekeeper: use the extended reading function on snapshot acquiring
tsc: implement the extended tsc reading function
KVM: x86: switch to masterclock update using timekeeper functionality
KVM: x86: remove not used pvclock_gtod_copy
arch/x86/include/asm/kvm_host.h | 2 +-
arch/x86/kernel/tsc.c | 10 ++
arch/x86/kvm/trace.h | 31 ++---
arch/x86/kvm/x86.c | 245 +++++++++-------------------------------
include/linux/clocksource.h | 11 +-
include/linux/timekeeping.h | 5 +
kernel/time/timekeeping.c | 17 ++-
7 files changed, 104 insertions(+), 217 deletions(-)
--
2.7.4
By doing that, add tsc clocksource to a list of KVM clocksources
providing valid cycle values, meaning that KVM can use its masterclock.
This is a part of the work aiming to move to a more simple scheme of
masterclock related values calculation in KVM.
Signed-off-by: Denis Plotnikov <[email protected]>
---
arch/x86/kernel/tsc.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 796d96b..8786454 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -1036,6 +1036,15 @@ static void tsc_cs_tick_stable(struct clocksource *cs)
sched_clock_tick_stable();
}
+static bool tsc_read_with_stamp(struct clocksource *cs,
+ u64 *cycles, u64 *cycles_stamp)
+{
+ u64 tsc = read_tsc(cs);
+ *cycles = tsc;
+ *cycles_stamp = tsc;
+ return true;
+}
+
/*
* .mask MUST be CLOCKSOURCE_MASK(64). See comment above read_tsc()
*/
@@ -1043,6 +1052,7 @@ static struct clocksource clocksource_tsc = {
.name = "tsc",
.rating = 300,
.read = read_tsc,
+ .read_with_stamp = tsc_read_with_stamp,
.mask = CLOCKSOURCE_MASK(64),
.flags = CLOCK_SOURCE_IS_CONTINUOUS |
CLOCK_SOURCE_MUST_VERIFY,
--
2.7.4
It is reasonable to switch KVM to using a more simple, effective
and conceptually correct scheme of dealing with the data needed
for kvm masterclock values calculation.
With the current scheme the kvm needs to have an up-to-date copy of
some timekeeper data to provide a guest using kvmclock with necessary
information.
This is not:
- simple
KVM has to have a lot of code to do that, instead KVM could use
a timekeeper function to get all the data it needs
- effective
the copy of the data used for time data calculation is updated
every time it changed although this is not necessary since
the updated piece of time data is needed in certain moments only
(e.g masterclock updating), instead KVM can request this data
directly form the timekeeper at the moments when it's really needed
- conceptually correct
to do the work (calculate the time data) which the other part
of the system (timekeeper) has been designed and is able to do
is not the proper way, instead deligate the work to the proper part
This patch switches KVM to using the improved timekeeper function for
the kvm masterclock time data.
Removing the leftovers of the old scheme is the matter of the next patches.
Signed-off-by: Denis Plotnikov <[email protected]>
---
arch/x86/kvm/x86.c | 26 ++++++++++++++++++--------
1 file changed, 18 insertions(+), 8 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 05a5e57..0e86729 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1643,22 +1643,32 @@ static int do_realtime(struct timespec *ts, u64 *cycle_now)
/* returns true if host is using tsc clocksource */
static bool kvm_get_time_and_clockread(s64 *kernel_ns, u64 *cycle_now)
{
- /* checked again under seqlock below */
- if (pvclock_gtod_data.clock.vclock_mode != VCLOCK_TSC)
- return false;
+ struct system_time_snapshot systime_snapshot;
+
+ ktime_get_snapshot(&systime_snapshot);
+
+ if (systime_snapshot.cycles_valid) {
+ *kernel_ns = ktime_to_ns(systime_snapshot.boot);
+ *cycle_now = systime_snapshot.cycles;
+ }
- return do_monotonic_boot(kernel_ns, cycle_now) == VCLOCK_TSC;
+ return systime_snapshot.cycles_valid;
}
/* returns true if host is using tsc clocksource */
static bool kvm_get_walltime_and_clockread(struct timespec *ts,
u64 *cycle_now)
{
- /* checked again under seqlock below */
- if (pvclock_gtod_data.clock.vclock_mode != VCLOCK_TSC)
- return false;
+ struct system_time_snapshot systime_snapshot;
+
+ ktime_get_snapshot(&systime_snapshot);
+
+ if (systime_snapshot.cycles_valid) {
+ *ts = ktime_to_timespec(systime_snapshot.real);
+ *cycle_now = systime_snapshot.cycles;
+ }
- return do_realtime(ts, cycle_now) == VCLOCK_TSC;
+ return systime_snapshot.cycles_valid;
}
#endif
--
2.7.4
Make use of the extended reading function on time snapshot getting:
get raw cycles value if extended function is defined.
The raw cycles value then is used for KVM masterclock.
This is a part of the work aiming to move to a more simple scheme of
masterclock related values calculation in KVM
Signed-off-by: Denis Plotnikov <[email protected]>
---
include/linux/timekeeping.h | 3 +++
kernel/time/timekeeping.c | 13 +++++++++++--
2 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
index 5008e3e..528d088 100644
--- a/include/linux/timekeeping.h
+++ b/include/linux/timekeeping.h
@@ -286,6 +286,8 @@ extern void ktime_get_raw_and_real_ts64(struct timespec64 *ts_raw,
* @boot: Monotonic time since boot
* @clock_was_set_seq: The sequence number of clock was set events
* @cs_was_changed_seq: The sequence number of clocksource change events
+ * @cycles_valid: The flag is true when @cycles contain actual
+ * number of cycles instead some cycle derivative
*/
struct system_time_snapshot {
u64 cycles;
@@ -294,6 +296,7 @@ struct system_time_snapshot {
ktime_t boot;
unsigned int clock_was_set_seq;
u8 cs_was_changed_seq;
+ bool cycles_valid;
};
/*
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 66f7701..d1aa575 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -957,12 +957,22 @@ void ktime_get_snapshot(struct system_time_snapshot *systime_snapshot)
u64 nsec_raw;
u64 nsec_real;
u64 now;
+ struct clocksource *clock;
WARN_ON_ONCE(timekeeping_suspended);
do {
seq = read_seqcount_begin(&tk_core.seq);
- now = tk_clock_read(&tk->tkr_mono);
+ clock = READ_ONCE(tk->tkr_mono.clock);
+ if (clock->read_with_stamp)
+ systime_snapshot->cycles_valid =
+ clock->read_with_stamp(
+ clock, &now, &systime_snapshot->cycles);
+ else {
+ systime_snapshot->cycles_valid = false;
+ now = clock->read(clock);
+ systime_snapshot->cycles = now;
+ }
systime_snapshot->cs_was_changed_seq = tk->cs_was_changed_seq;
systime_snapshot->clock_was_set_seq = tk->clock_was_set_seq;
base_real = ktime_add(tk->tkr_mono.base,
@@ -974,7 +984,6 @@ void ktime_get_snapshot(struct system_time_snapshot *systime_snapshot)
nsec_raw = timekeeping_cycles_to_ns(&tk->tkr_raw, now);
} while (read_seqcount_retry(&tk_core.seq, seq));
- systime_snapshot->cycles = now;
systime_snapshot->real = ktime_add_ns(base_real, nsec_real);
systime_snapshot->raw = ktime_add_ns(base_raw, nsec_raw);
systime_snapshot->boot = ktime_add_ns(base_boot, nsec_real);
--
2.7.4
The callback provides extended information about just read
clocksource value.
It's going to be used in cases when detailed system information
needed for further time related values calculation, e.g in KVM
masterclock settings calculation.
Signed-off-by: Denis Plotnikov <[email protected]>
---
include/linux/clocksource.h | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
index a78cb18..786a522 100644
--- a/include/linux/clocksource.h
+++ b/include/linux/clocksource.h
@@ -48,7 +48,14 @@ struct module;
* 400-499: Perfect
* The ideal clocksource. A must-use where
* available.
- * @read: returns a cycle value, passes clocksource as argument
+ * @read: returns a cycle value (might be not quite cycles:
+ * see pvclock) passes clocksource as argument
+ * @read_with_stamp: saves cycles value (got from timekeeper) and cycles
+ * stamp (got from hardware counter value and used by
+ * timekeeper to calculate the cycles value) to
+ * corresponding input pointers return true if cycles
+ * stamp holds real cycles and false if it holds some
+ * time derivative value
* @enable: optional function to enable the clocksource
* @disable: optional function to disable the clocksource
* @mask: bitmask for two's complement
@@ -78,6 +85,8 @@ struct module;
*/
struct clocksource {
u64 (*read)(struct clocksource *cs);
+ bool (*read_with_stamp)(struct clocksource *cs,
+ u64 *cycles, u64 *cycles_stamp);
u64 mask;
u32 mult;
u32 shift;
--
2.7.4
Since, KVM has been switched to getting masterclock related data
right from the timekeeper by the previous patches, now we are able
to remove all the parts related to the old scheme of getting
masterclock data.
This patch removes those parts.
Using of pvclock_gtod_copy notifier might be redundant because
the current scheme doesn't need to update any shadow timekeeper
data structure (pvclock_gtod_copy) every time the timekeeper does
changes. The notifier has been left unremoved intentionally and its
replacing with something else is the matter of future work.
Signed-off-by: Denis Plotnikov <[email protected]>
---
arch/x86/include/asm/kvm_host.h | 2 +-
arch/x86/kvm/trace.h | 31 ++----
arch/x86/kvm/x86.c | 219 +++++++---------------------------------
3 files changed, 46 insertions(+), 206 deletions(-)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index f4d120a..a1dc06c 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -792,7 +792,7 @@ struct kvm_arch {
u64 cur_tsc_generation;
int nr_vcpus_matched_tsc;
- spinlock_t pvclock_gtod_sync_lock;
+ spinlock_t masterclock_lock;
bool use_master_clock;
u64 master_kernel_ns;
u64 master_cycle_now;
diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
index 0a6cc67..923ab31 100644
--- a/arch/x86/kvm/trace.h
+++ b/arch/x86/kvm/trace.h
@@ -807,45 +807,39 @@ TRACE_EVENT(kvm_write_tsc_offset,
#ifdef CONFIG_X86_64
-#define host_clocks \
- {VCLOCK_NONE, "none"}, \
- {VCLOCK_TSC, "tsc"} \
-
TRACE_EVENT(kvm_update_master_clock,
- TP_PROTO(bool use_master_clock, unsigned int host_clock, bool offset_matched),
- TP_ARGS(use_master_clock, host_clock, offset_matched),
+ TP_PROTO(bool use_master_clock, bool host_clock_stable,
+ bool offset_matched),
+ TP_ARGS(use_master_clock, host_clock_stable, offset_matched),
TP_STRUCT__entry(
__field( bool, use_master_clock )
- __field( unsigned int, host_clock )
+ __field( bool, host_clock_stable )
__field( bool, offset_matched )
),
TP_fast_assign(
__entry->use_master_clock = use_master_clock;
- __entry->host_clock = host_clock;
+ __entry->host_clock_stable = host_clock_stable;
__entry->offset_matched = offset_matched;
),
- TP_printk("masterclock %d hostclock %s offsetmatched %u",
+ TP_printk("masterclock %d hostclock stable %u offsetmatched %u",
__entry->use_master_clock,
- __print_symbolic(__entry->host_clock, host_clocks),
+ __entry->host_clock_stable,
__entry->offset_matched)
);
TRACE_EVENT(kvm_track_tsc,
TP_PROTO(unsigned int vcpu_id, unsigned int nr_matched,
- unsigned int online_vcpus, bool use_master_clock,
- unsigned int host_clock),
- TP_ARGS(vcpu_id, nr_matched, online_vcpus, use_master_clock,
- host_clock),
+ unsigned int online_vcpus, bool use_master_clock),
+ TP_ARGS(vcpu_id, nr_matched, online_vcpus, use_master_clock),
TP_STRUCT__entry(
__field( unsigned int, vcpu_id )
__field( unsigned int, nr_vcpus_matched_tsc )
__field( unsigned int, online_vcpus )
__field( bool, use_master_clock )
- __field( unsigned int, host_clock )
),
TP_fast_assign(
@@ -853,14 +847,11 @@ TRACE_EVENT(kvm_track_tsc,
__entry->nr_vcpus_matched_tsc = nr_matched;
__entry->online_vcpus = online_vcpus;
__entry->use_master_clock = use_master_clock;
- __entry->host_clock = host_clock;
),
- TP_printk("vcpu_id %u masterclock %u offsetmatched %u nr_online %u"
- " hostclock %s",
+ TP_printk("vcpu_id %u masterclock %u offsetmatched %u nr_online %u",
__entry->vcpu_id, __entry->use_master_clock,
- __entry->nr_vcpus_matched_tsc, __entry->online_vcpus,
- __print_symbolic(__entry->host_clock, host_clocks))
+ __entry->nr_vcpus_matched_tsc, __entry->online_vcpus)
);
#endif /* CONFIG_X86_64 */
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 0e86729..fc180b5 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1134,50 +1134,6 @@ static int do_set_msr(struct kvm_vcpu *vcpu, unsigned index, u64 *data)
return kvm_set_msr(vcpu, &msr);
}
-#ifdef CONFIG_X86_64
-struct pvclock_gtod_data {
- seqcount_t seq;
-
- struct { /* extract of a clocksource struct */
- int vclock_mode;
- u64 cycle_last;
- u64 mask;
- u32 mult;
- u32 shift;
- } clock;
-
- u64 boot_ns;
- u64 nsec_base;
- u64 wall_time_sec;
-};
-
-static struct pvclock_gtod_data pvclock_gtod_data;
-
-static void update_pvclock_gtod(struct timekeeper *tk)
-{
- struct pvclock_gtod_data *vdata = &pvclock_gtod_data;
- u64 boot_ns;
-
- boot_ns = ktime_to_ns(ktime_add(tk->tkr_mono.base, tk->offs_boot));
-
- write_seqcount_begin(&vdata->seq);
-
- /* copy pvclock gtod data */
- vdata->clock.vclock_mode = tk->tkr_mono.clock->archdata.vclock_mode;
- vdata->clock.cycle_last = tk->tkr_mono.cycle_last;
- vdata->clock.mask = tk->tkr_mono.mask;
- vdata->clock.mult = tk->tkr_mono.mult;
- vdata->clock.shift = tk->tkr_mono.shift;
-
- vdata->boot_ns = boot_ns;
- vdata->nsec_base = tk->tkr_mono.xtime_nsec;
-
- vdata->wall_time_sec = tk->xtime_sec;
-
- write_seqcount_end(&vdata->seq);
-}
-#endif
-
void kvm_set_pending_timer(struct kvm_vcpu *vcpu)
{
/*
@@ -1269,10 +1225,6 @@ static void kvm_get_time_scale(uint64_t scaled_hz, uint64_t base_hz,
__func__, base_hz, scaled_hz, shift, *pmultiplier);
}
-#ifdef CONFIG_X86_64
-static atomic_t kvm_guest_has_master_clock = ATOMIC_INIT(0);
-#endif
-
static DEFINE_PER_CPU(unsigned long, cpu_tsc_khz);
static unsigned long max_tsc_khz;
@@ -1366,7 +1318,6 @@ static void kvm_track_tsc_matching(struct kvm_vcpu *vcpu)
#ifdef CONFIG_X86_64
bool vcpus_matched;
struct kvm_arch *ka = &vcpu->kvm->arch;
- struct pvclock_gtod_data *gtod = &pvclock_gtod_data;
vcpus_matched = (ka->nr_vcpus_matched_tsc + 1 ==
atomic_read(&vcpu->kvm->online_vcpus));
@@ -1379,13 +1330,12 @@ static void kvm_track_tsc_matching(struct kvm_vcpu *vcpu)
* and the vcpus need to have matched TSCs. When that happens,
* perform request to enable masterclock.
*/
- if (ka->use_master_clock ||
- (gtod->clock.vclock_mode == VCLOCK_TSC && vcpus_matched))
+ if (ka->use_master_clock || vcpus_matched)
kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);
trace_kvm_track_tsc(vcpu->vcpu_id, ka->nr_vcpus_matched_tsc,
- atomic_read(&vcpu->kvm->online_vcpus),
- ka->use_master_clock, gtod->clock.vclock_mode);
+ atomic_read(&vcpu->kvm->online_vcpus),
+ ka->use_master_clock);
#endif
}
@@ -1538,7 +1488,7 @@ void kvm_write_tsc(struct kvm_vcpu *vcpu, struct msr_data *msr)
kvm_vcpu_write_tsc_offset(vcpu, offset);
raw_spin_unlock_irqrestore(&kvm->arch.tsc_write_lock, flags);
- spin_lock(&kvm->arch.pvclock_gtod_sync_lock);
+ spin_lock(&kvm->arch.masterclock_lock);
if (!matched) {
kvm->arch.nr_vcpus_matched_tsc = 0;
} else if (!already_matched) {
@@ -1546,9 +1496,8 @@ void kvm_write_tsc(struct kvm_vcpu *vcpu, struct msr_data *msr)
}
kvm_track_tsc_matching(vcpu);
- spin_unlock(&kvm->arch.pvclock_gtod_sync_lock);
+ spin_unlock(&kvm->arch.masterclock_lock);
}
-
EXPORT_SYMBOL_GPL(kvm_write_tsc);
static inline void adjust_tsc_offset_guest(struct kvm_vcpu *vcpu,
@@ -1567,79 +1516,6 @@ static inline void adjust_tsc_offset_host(struct kvm_vcpu *vcpu, s64 adjustment)
#ifdef CONFIG_X86_64
-static u64 read_tsc(void)
-{
- u64 ret = (u64)rdtsc_ordered();
- u64 last = pvclock_gtod_data.clock.cycle_last;
-
- if (likely(ret >= last))
- return ret;
-
- /*
- * GCC likes to generate cmov here, but this branch is extremely
- * predictable (it's just a function of time and the likely is
- * very likely) and there's a data dependence, so force GCC
- * to generate a branch instead. I don't barrier() because
- * we don't actually need a barrier, and if this function
- * ever gets inlined it will generate worse code.
- */
- asm volatile ("");
- return last;
-}
-
-static inline u64 vgettsc(u64 *cycle_now)
-{
- long v;
- struct pvclock_gtod_data *gtod = &pvclock_gtod_data;
-
- *cycle_now = read_tsc();
-
- v = (*cycle_now - gtod->clock.cycle_last) & gtod->clock.mask;
- return v * gtod->clock.mult;
-}
-
-static int do_monotonic_boot(s64 *t, u64 *cycle_now)
-{
- struct pvclock_gtod_data *gtod = &pvclock_gtod_data;
- unsigned long seq;
- int mode;
- u64 ns;
-
- do {
- seq = read_seqcount_begin(>od->seq);
- mode = gtod->clock.vclock_mode;
- ns = gtod->nsec_base;
- ns += vgettsc(cycle_now);
- ns >>= gtod->clock.shift;
- ns += gtod->boot_ns;
- } while (unlikely(read_seqcount_retry(>od->seq, seq)));
- *t = ns;
-
- return mode;
-}
-
-static int do_realtime(struct timespec *ts, u64 *cycle_now)
-{
- struct pvclock_gtod_data *gtod = &pvclock_gtod_data;
- unsigned long seq;
- int mode;
- u64 ns;
-
- do {
- seq = read_seqcount_begin(>od->seq);
- mode = gtod->clock.vclock_mode;
- ts->tv_sec = gtod->wall_time_sec;
- ns = gtod->nsec_base;
- ns += vgettsc(cycle_now);
- ns >>= gtod->clock.shift;
- } while (unlikely(read_seqcount_retry(>od->seq, seq)));
-
- ts->tv_sec += __iter_div_u64_rem(ns, NSEC_PER_SEC, &ns);
- ts->tv_nsec = ns;
-
- return mode;
-}
-
/* returns true if host is using tsc clocksource */
static bool kvm_get_time_and_clockread(s64 *kernel_ns, u64 *cycle_now)
{
@@ -1713,34 +1589,34 @@ static bool kvm_get_walltime_and_clockread(struct timespec *ts,
*
*/
-static void pvclock_update_vm_gtod_copy(struct kvm *kvm)
+/* returns true if the value of ka->use_master_clock is changed*/
+static bool update_masterclock(struct kvm *kvm)
{
#ifdef CONFIG_X86_64
struct kvm_arch *ka = &kvm->arch;
- int vclock_mode;
- bool host_tsc_clocksource, vcpus_matched;
+ bool host_clocksource_stable, vcpus_matched, use_master_clock, r;
vcpus_matched = (ka->nr_vcpus_matched_tsc + 1 ==
atomic_read(&kvm->online_vcpus));
/*
- * If the host uses TSC clock, then passthrough TSC as stable
- * to the guest.
+ * kvm_get_time_and_clockread returns true if clocksource is stable
*/
- host_tsc_clocksource = kvm_get_time_and_clockread(
+ host_clocksource_stable = kvm_get_time_and_clockread(
&ka->master_kernel_ns,
&ka->master_cycle_now);
- ka->use_master_clock = host_tsc_clocksource && vcpus_matched
+ use_master_clock = host_clocksource_stable && vcpus_matched
&& !ka->backwards_tsc_observed
&& !ka->boot_vcpu_runs_old_kvmclock;
- if (ka->use_master_clock)
- atomic_set(&kvm_guest_has_master_clock, 1);
+ r = ka->use_master_clock ^ use_master_clock;
+ ka->use_master_clock = use_master_clock;
+
+ trace_kvm_update_master_clock(ka->use_master_clock,
+ host_clocksource_stable, vcpus_matched);
- vclock_mode = pvclock_gtod_data.clock.vclock_mode;
- trace_kvm_update_master_clock(ka->use_master_clock, vclock_mode,
- vcpus_matched);
+ return r;
#endif
}
@@ -1756,19 +1632,18 @@ static void kvm_gen_update_masterclock(struct kvm *kvm)
struct kvm_vcpu *vcpu;
struct kvm_arch *ka = &kvm->arch;
- spin_lock(&ka->pvclock_gtod_sync_lock);
+ spin_lock(&ka->masterclock_lock);
kvm_make_mclock_inprogress_request(kvm);
/* no guest entries from this point */
- pvclock_update_vm_gtod_copy(kvm);
-
- kvm_for_each_vcpu(i, vcpu, kvm)
- kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
+ if (update_masterclock(kvm) || !ka->use_master_clock)
+ kvm_for_each_vcpu(i, vcpu, kvm)
+ kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
/* guest entries allowed */
kvm_for_each_vcpu(i, vcpu, kvm)
kvm_clear_request(KVM_REQ_MCLOCK_INPROGRESS, vcpu);
- spin_unlock(&ka->pvclock_gtod_sync_lock);
+ spin_unlock(&ka->masterclock_lock);
#endif
}
@@ -1778,15 +1653,15 @@ u64 get_kvmclock_ns(struct kvm *kvm)
struct pvclock_vcpu_time_info hv_clock;
u64 ret;
- spin_lock(&ka->pvclock_gtod_sync_lock);
+ spin_lock(&ka->masterclock_lock);
if (!ka->use_master_clock) {
- spin_unlock(&ka->pvclock_gtod_sync_lock);
+ spin_unlock(&ka->masterclock_lock);
return ktime_get_boot_ns() + ka->kvmclock_offset;
}
hv_clock.tsc_timestamp = ka->master_cycle_now;
hv_clock.system_time = ka->master_kernel_ns + ka->kvmclock_offset;
- spin_unlock(&ka->pvclock_gtod_sync_lock);
+ spin_unlock(&ka->masterclock_lock);
/* both __this_cpu_read() and rdtsc() should be on the same cpu */
get_cpu();
@@ -1872,13 +1747,13 @@ static int kvm_guest_time_update(struct kvm_vcpu *v)
* If the host uses TSC clock, then passthrough TSC as stable
* to the guest.
*/
- spin_lock(&ka->pvclock_gtod_sync_lock);
+ spin_lock(&ka->masterclock_lock);
use_master_clock = ka->use_master_clock;
if (use_master_clock) {
host_tsc = ka->master_cycle_now;
kernel_ns = ka->master_kernel_ns;
}
- spin_unlock(&ka->pvclock_gtod_sync_lock);
+ spin_unlock(&ka->masterclock_lock);
/* Keep irq disabled to prevent changes to the clock */
local_irq_save(flags);
@@ -4220,11 +4095,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
goto out;
r = 0;
- /*
- * TODO: userspace has to take care of races with VCPU_RUN, so
- * kvm_gen_update_masterclock() can be cut down to locked
- * pvclock_update_vm_gtod_copy().
- */
+
kvm_gen_update_masterclock(kvm);
now_ns = get_kvmclock_ns(kvm);
kvm->arch.kvmclock_offset += user_ns.clock - now_ns;
@@ -6053,7 +5924,8 @@ static void kvm_set_mmio_spte_mask(void)
}
#ifdef CONFIG_X86_64
-static void pvclock_gtod_update_fn(struct work_struct *work)
+static int request_masterclock_update(struct notifier_block *nb,
+ unsigned long unused0, void *unused1)
{
struct kvm *kvm;
@@ -6064,35 +5936,12 @@ static void pvclock_gtod_update_fn(struct work_struct *work)
list_for_each_entry(kvm, &vm_list, vm_list)
kvm_for_each_vcpu(i, vcpu, kvm)
kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);
- atomic_set(&kvm_guest_has_master_clock, 0);
spin_unlock(&kvm_lock);
-}
-
-static DECLARE_WORK(pvclock_gtod_work, pvclock_gtod_update_fn);
-
-/*
- * Notification about pvclock gtod data update.
- */
-static int pvclock_gtod_notify(struct notifier_block *nb, unsigned long unused,
- void *priv)
-{
- struct pvclock_gtod_data *gtod = &pvclock_gtod_data;
- struct timekeeper *tk = priv;
-
- update_pvclock_gtod(tk);
-
- /* disable master clock if host does not trust, or does not
- * use, TSC clocksource
- */
- if (gtod->clock.vclock_mode != VCLOCK_TSC &&
- atomic_read(&kvm_guest_has_master_clock) != 0)
- queue_work(system_long_wq, &pvclock_gtod_work);
-
return 0;
}
static struct notifier_block pvclock_gtod_notifier = {
- .notifier_call = pvclock_gtod_notify,
+ .notifier_call = request_masterclock_update,
};
#endif
@@ -6145,7 +5994,7 @@ int kvm_arch_init(void *opaque)
kvm_lapic_init();
#ifdef CONFIG_X86_64
- pvclock_gtod_register_notifier(&pvclock_gtod_notifier);
+ pvclock_gtod_unregister_notifier(&pvclock_gtod_notifier);
#endif
return 0;
@@ -8071,10 +7920,10 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
raw_spin_lock_init(&kvm->arch.tsc_write_lock);
mutex_init(&kvm->arch.apic_map_lock);
mutex_init(&kvm->arch.hyperv.hv_lock);
- spin_lock_init(&kvm->arch.pvclock_gtod_sync_lock);
+ spin_lock_init(&kvm->arch.masterclock_lock);
kvm->arch.kvmclock_offset = -ktime_get_boot_ns();
- pvclock_update_vm_gtod_copy(kvm);
+ update_masterclock(kvm);
INIT_DELAYED_WORK(&kvm->arch.kvmclock_update_work, kvmclock_update_fn);
INIT_DELAYED_WORK(&kvm->arch.kvmclock_sync_work, kvmclock_sync_fn);
--
2.7.4
The field keeps monotonic time since boot.
The value of boot will be used in KVM for masterclock.
This is a part of the work aiming to move to a more simple scheme
of masterclock related values calculation in KVM
Signed-off-by: Denis Plotnikov <[email protected]>
---
include/linux/timekeeping.h | 2 ++
kernel/time/timekeeping.c | 4 ++++
2 files changed, 6 insertions(+)
diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
index ddc229f..5008e3e 100644
--- a/include/linux/timekeeping.h
+++ b/include/linux/timekeeping.h
@@ -283,6 +283,7 @@ extern void ktime_get_raw_and_real_ts64(struct timespec64 *ts_raw,
* @cycles: Clocksource counter value to produce the system times
* @real: Realtime system time
* @raw: Monotonic raw system time
+ * @boot: Monotonic time since boot
* @clock_was_set_seq: The sequence number of clock was set events
* @cs_was_changed_seq: The sequence number of clocksource change events
*/
@@ -290,6 +291,7 @@ struct system_time_snapshot {
u64 cycles;
ktime_t real;
ktime_t raw;
+ ktime_t boot;
unsigned int clock_was_set_seq;
u8 cs_was_changed_seq;
};
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index cedafa0..66f7701 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -953,6 +953,7 @@ void ktime_get_snapshot(struct system_time_snapshot *systime_snapshot)
unsigned long seq;
ktime_t base_raw;
ktime_t base_real;
+ ktime_t base_boot;
u64 nsec_raw;
u64 nsec_real;
u64 now;
@@ -967,6 +968,8 @@ void ktime_get_snapshot(struct system_time_snapshot *systime_snapshot)
base_real = ktime_add(tk->tkr_mono.base,
tk_core.timekeeper.offs_real);
base_raw = tk->tkr_raw.base;
+ base_boot = ktime_add(tk->tkr_mono.base,
+ tk_core.timekeeper.offs_boot);
nsec_real = timekeeping_cycles_to_ns(&tk->tkr_mono, now);
nsec_raw = timekeeping_cycles_to_ns(&tk->tkr_raw, now);
} while (read_seqcount_retry(&tk_core.seq, seq));
@@ -974,6 +977,7 @@ void ktime_get_snapshot(struct system_time_snapshot *systime_snapshot)
systime_snapshot->cycles = now;
systime_snapshot->real = ktime_add_ns(base_real, nsec_real);
systime_snapshot->raw = ktime_add_ns(base_raw, nsec_raw);
+ systime_snapshot->boot = ktime_add_ns(base_boot, nsec_real);
}
EXPORT_SYMBOL_GPL(ktime_get_snapshot);
--
2.7.4
ping!
On 30.08.2017 18:23, Denis Plotnikov wrote:
> V5:
> It was decided to split the series
> "make L2's kvm-clock stable, get rid of pvclock_gtod_copy in KVM"
> into two parts to make the changes finer grained:
> 1. get rid of pvclock_gtod_copy in KVM
> 2. make L2's kvm-clock stable
> This V5 series implements the 1st part. This part gets rid
> of pvclock_gtod_copy by switching to use the timekeeper functionality
> being extended. This new scheme doesn't add any new notifiers and keeps
> using the existing one (see patch 6 description) which had been used by
> the pvclock_gtod_copy to keep track timekeeper data changes.
>
> The 2nd part is being discussed and is going to be sent later on.
>
> V4:
> * removed "is stable" function with vague definition of stability
> there is the only function which does time with cycle stamp getting
> * some variables renamed
> * some patches split into smaller once
> * atomic64_t usage is replaced with atomic_t
>
> V3:
> Changing the timekeeper interface for clocksource reading looks like
> an overkill to achive the goal of getting cycles stamp for KVM.
> Instead extend the timekeeping interface and add functions which provide
> necessary data: read clocksource with cycles stamp, check whether the
> clock source is stable.
>
> Use those functions and improve existing timekeeper functionality to
> replace pvclock_gtod_copy scheme in masterclock data calculation.
>
> V2:
> The main goal is to make L2 kvm-clock be stable when it's running over L1
> with stable kvm-clock.
>
> The patch series is for x86 architecture only. If the series is approved
> I'll do changes for other architectures but I don't have an ability to
> compile and check for every single on (help needed)
>
> The patch series do the following:
>
> * change timekeeper interface to get cycles stamp value from
> the timekeeper
> * get rid of pvclock copy in KVM by using the changed timekeeper
> interface: get time and cycles right from the timekeeper
> * make KVM recognize a stable kvm-clock as stable clocksource
> and use the KVM masterclock in this case, which means making
> L2 stable when running over stable L1 kvm-clock
>
> Denis Plotnikov (6):
> timekeeper: introduce extended clocksource reading callback
> timekeeper: introduce boot field in system_time_snapshot
> timekeeper: use the extended reading function on snapshot acquiring
> tsc: implement the extended tsc reading function
> KVM: x86: switch to masterclock update using timekeeper functionality
> KVM: x86: remove not used pvclock_gtod_copy
>
> arch/x86/include/asm/kvm_host.h | 2 +-
> arch/x86/kernel/tsc.c | 10 ++
> arch/x86/kvm/trace.h | 31 ++---
> arch/x86/kvm/x86.c | 245 +++++++++-------------------------------
> include/linux/clocksource.h | 11 +-
> include/linux/timekeeping.h | 5 +
> kernel/time/timekeeping.c | 17 ++-
> 7 files changed, 104 insertions(+), 217 deletions(-)
>
--
Best,
Denis
On Mon, 11 Sep 2017, Denis Plotnikov wrote:
> ping!
I'll have a look next week as I'm not much around this week.
Thanks,
tglx
On Wed, 30 Aug 2017, Denis Plotnikov wrote:
> The callback provides extended information about just read
> clocksource value.
>
> It's going to be used in cases when detailed system information
> needed for further time related values calculation, e.g in KVM
> masterclock settings calculation.
>
> Signed-off-by: Denis Plotnikov <[email protected]>
> ---
> include/linux/clocksource.h | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
> index a78cb18..786a522 100644
> --- a/include/linux/clocksource.h
> +++ b/include/linux/clocksource.h
> @@ -48,7 +48,14 @@ struct module;
> * 400-499: Perfect
> * The ideal clocksource. A must-use where
> * available.
> - * @read: returns a cycle value, passes clocksource as argument
> + * @read: returns a cycle value (might be not quite cycles:
> + * see pvclock) passes clocksource as argument
> + * @read_with_stamp: saves cycles value (got from timekeeper) and cycles
> + * stamp (got from hardware counter value and used by
> + * timekeeper to calculate the cycles value) to
> + * corresponding input pointers return true if cycles
> + * stamp holds real cycles and false if it holds some
> + * time derivative value
Neither the changelog nor this comment make any sense. Not to talk about
the actual TSC side implementation which does the same as tsc_read() - it
actually uses tsc_read() - but stores the same value twice and
unconditionally returns true.
I have no idea why you need this extra voodoo function if you can achieve
the same thing with a simple property bit in clocksource->flags.
Neither do I understand the rest of the magic you introduce in the snapshot
code:
> + if (clock->read_with_stamp)
> + systime_snapshot->cycles_valid =
> + clock->read_with_stamp(
> + clock, &now, &systime_snapshot->cycles);
> + else {
> + systime_snapshot->cycles_valid = false;
> + now = clock->read(clock);
> + systime_snapshot->cycles = now;
> + }
The only difference between the two code pathes is the effect on
systime_snapshot->cycles_valid. The explanation of that bit is not making
much sense either.
+ * @cycles_valid: The flag is true when @cycles contain actual
+ * number of cycles instead some cycle derivative
Why the heck would cycles be something different than what clock->read()
returns?
What you really want to convey is the information whether
now = tk_clock_read(&tk->tkr_mono);
is a value which you can use for your pvclock correlation or not.
Unless I'm missing something important all of this can be achieved with a
minimal and actually understandable patch. See below.
Thanks,
tglx
8<------------------
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -1045,7 +1045,8 @@ static struct clocksource clocksource_ts
.read = read_tsc,
.mask = CLOCKSOURCE_MASK(64),
.flags = CLOCK_SOURCE_IS_CONTINUOUS |
- CLOCK_SOURCE_MUST_VERIFY,
+ CLOCK_SOURCE_MUST_VERIFY |
+ CLOCK_SOURCE_VALID_FOR_PVCLOCK_UPDATE,
.archdata = { .vclock_mode = VCLOCK_TSC },
.resume = tsc_resume,
.mark_unstable = tsc_cs_mark_unstable,
--- a/include/linux/clocksource.h
+++ b/include/linux/clocksource.h
@@ -118,7 +118,9 @@ struct clocksource {
#define CLOCK_SOURCE_VALID_FOR_HRES 0x20
#define CLOCK_SOURCE_UNSTABLE 0x40
#define CLOCK_SOURCE_SUSPEND_NONSTOP 0x80
-#define CLOCK_SOURCE_RESELECT 0x100
+#define CLOCK_SOURCE_VALID_FOR_PVCLOCK_UPDATE 0x100
+
+#define CLOCK_SOURCE_RESELECT 0x200
/* simplify initialization of mask field */
#define CLOCKSOURCE_MASK(bits) GENMASK_ULL((bits) - 1, 0)
--- a/include/linux/timekeeping.h
+++ b/include/linux/timekeeping.h
@@ -285,6 +285,8 @@ extern void ktime_get_raw_and_real_ts64(
* @raw: Monotonic raw system time
* @clock_was_set_seq: The sequence number of clock was set events
* @cs_was_changed_seq: The sequence number of clocksource change events
+ * @valid_for_pvclock_update: @cycles is from a clocksource which
+ * can be used for pvclock updates
*/
struct system_time_snapshot {
u64 cycles;
@@ -292,6 +294,7 @@ struct system_time_snapshot {
ktime_t raw;
unsigned int clock_was_set_seq;
u8 cs_was_changed_seq;
+ bool valid_for_pvclock_update;
};
/*
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -948,7 +948,7 @@ time64_t __ktime_get_real_seconds(void)
void ktime_get_snapshot(struct system_time_snapshot *systime_snapshot)
{
struct timekeeper *tk = &tk_core.timekeeper;
- unsigned long seq;
+ unsigned long seq, clk_flags;
ktime_t base_raw;
ktime_t base_real;
u64 nsec_raw;
@@ -960,6 +960,7 @@ void ktime_get_snapshot(struct system_ti
do {
seq = read_seqcount_begin(&tk_core.seq);
now = tk_clock_read(&tk->tkr_mono);
+ clk_flags = tk->tkr_mono.clock->flags;
systime_snapshot->cs_was_changed_seq = tk->cs_was_changed_seq;
systime_snapshot->clock_was_set_seq = tk->clock_was_set_seq;
base_real = ktime_add(tk->tkr_mono.base,
@@ -970,6 +971,8 @@ void ktime_get_snapshot(struct system_ti
} while (read_seqcount_retry(&tk_core.seq, seq));
systime_snapshot->cycles = now;
+ systime_snapshot->valid_for_pvclock_update =
+ clk_flags & CLOCK_SOURCE_VALID_FOR_PVCLOCK_UPDATE,
systime_snapshot->real = ktime_add_ns(base_real, nsec_real);
systime_snapshot->raw = ktime_add_ns(base_raw, nsec_raw);
}
On 26/09/2017 00:00, Thomas Gleixner wrote:
>> + * @read_with_stamp: saves cycles value (got from timekeeper) and cycles
>> + * stamp (got from hardware counter value and used by
>> + * timekeeper to calculate the cycles value) to
>> + * corresponding input pointers return true if cycles
>> + * stamp holds real cycles and false if it holds some
>> + * time derivative value
>
> Neither the changelog nor this comment make any sense. Not to talk about
> the actual TSC side implementation which does the same as tsc_read() - it
> actually uses tsc_read() - but stores the same value twice and
> unconditionally returns true.
>
> Unless I'm missing something important all of this can be achieved with a
> minimal and actually understandable patch. See below.
If that is acceptable, that's certainly okay for us too as a start, in
order to clean up the KVM code.
*However*, I must once more ask for help understanding
ktime_get_snapshot, otherwise there is no way that I can start using it
in kvmclock.
In particular I'd like to understand the contract of ktime_get_snapshot,
namely the meaning of the "cycles" field in struct system_time_snapshot.
Does it have to be system clock cycles, like TSC, or can it be just the
value of the clock source? And if the latter, are the users broken,
because they can receive any random clocksource output in ->cycles? It
doesn't help that, for all of the users of ->cycles (which are all
reached from get_device_system_crosststamp), the code that actually uses
the member is never called in the current git master.
I asked these questions to John in the previous review, but got no
answer. My understanding is that get_device_system_crosststamp is
broken for !tsc clocksource. Because struct system_counterval_t must
have a TSC value, history_begin needs to contain a cross-timestamp of
the TSC (in ->cycles) and the clock (in ->real and ->raw). And if this
cross-timestamp is not available, get_device_system_crosststamp needs to
know, so that it doesn't use history_begin at all.
Now, such cross-timestamp functionality is exactly what
"read_with_stamp" provides. Patch 3 also introduces "->cycles_valid" in
struct system_time_snapshot, to report whether the cross-timestamp is
available. In the case of the TSC clocksource, of course, the two
values of course are the same, and always available (so the function
always returns true).
However, if get_device_system_crosststamp ran with kvmclock or Hyper-V
clocksource, the two values stored by read_with_stamp would be
different, basically a (TSC, nanoseconds) pair. ktime_get_snapshot
would then return the TSC in ->cycles, and use the nanoseconds value to
compute ->real and ->raw. Because the cross timestamp is available,
->cycles_valid would be true.
So, if history_begin _is_ broken, after fixing it (with
"read_with_stamp" or otherwise) ktime_get_snapshot would provide KVM
with everything it needs. If not, I'd be completely off base, and I'd
have to apologize to Denis for screwing up.
Paolo
On Tue, 26 Sep 2017, Paolo Bonzini wrote:
> On 26/09/2017 00:00, Thomas Gleixner wrote:
> >> + * @read_with_stamp: saves cycles value (got from timekeeper) and cycles
> >> + * stamp (got from hardware counter value and used by
> >> + * timekeeper to calculate the cycles value) to
> >> + * corresponding input pointers return true if cycles
> >> + * stamp holds real cycles and false if it holds some
> >> + * time derivative value
> >
> > Neither the changelog nor this comment make any sense. Not to talk about
> > the actual TSC side implementation which does the same as tsc_read() - it
> > actually uses tsc_read() - but stores the same value twice and
> > unconditionally returns true.
> >
> > Unless I'm missing something important all of this can be achieved with a
> > minimal and actually understandable patch. See below.
>
> If that is acceptable, that's certainly okay for us too as a start, in
> order to clean up the KVM code.
>
> *However*, I must once more ask for help understanding
> ktime_get_snapshot, otherwise there is no way that I can start using it
> in kvmclock.
>
> In particular I'd like to understand the contract of ktime_get_snapshot,
> namely the meaning of the "cycles" field in struct system_time_snapshot.
>
> Does it have to be system clock cycles, like TSC, or can it be just the
> value of the clock source? And if the latter, are the users broken,
> because they can receive any random clocksource output in ->cycles? It
> doesn't help that, for all of the users of ->cycles (which are all
> reached from get_device_system_crosststamp), the code that actually uses
> the member is never called in the current git master.
stamp->cycles is always the raw counter value retrieved via
clock->read(). clock is the current clock source.
> I asked these questions to John in the previous review, but got no
> answer. My understanding is that get_device_system_crosststamp is
> broken for !tsc clocksource.
>
> Because struct system_counterval_t must have a TSC value, history_begin
> needs to contain a cross-timestamp of the TSC (in ->cycles) and the clock
> (in ->real and ->raw). And if this cross-timestamp is not available,
> get_device_system_crosststamp needs to know, so that it doesn't use
> history_begin at all.
Forget about TSC. TSC is one particular use case.
What's required here are two clocks which are strictly coupled, i.e. have a
constant frequency ratio and a constant offset. And that is enforced:
/*
* Verify that the clocksource associated with the captured
* system counter value is the same as the currently installed
* timekeeper clocksource
*/
if (tk->tkr_mono.clock != system_counterval.cs)
return -ENODEV;
And that's not broken, that merily makes sure that the function CAN provide
valid data and is not trying to correlate stuff which is not coupled.
> Now, such cross-timestamp functionality is exactly what
> "read_with_stamp" provides.
No, it's a hack.
> So, if history_begin _is_ broken, after fixing it (with
> "read_with_stamp" or otherwise) ktime_get_snapshot would provide KVM
> with everything it needs. If not, I'd be completely off base, and I'd
> have to apologize to Denis for screwing up.
I told you folks before that you are trying to abuse infrastructure which
was designed for a different purpose. Now you're claiming it's broken. It's
not broken, it works perfectly fine for the intended use cases.
It correlates hardware captured timestamps with system time. One particular
use case is high precision PTP.
The network card can capture both PTP time and ART time atomically. ART and
TSC have a constant ratio and constant offset. The cross time stamp
machinery relates that values, which might be outside of the current
timekeeping window, back to CLOCK_MONOTONIC and CLOCK_REALTIME.
But there is no requirement that current clocksource is TSC. The
requirement is:
The hardware reference clock, the one which can be captured atomically
with device clock (PTP, audio whatever), is the coupled clock of the
current timekeeping clocksource. Both have a fixed ratio and offset.
That's completely independend of TSC. TSC/ART are a particular hardware
implementation which can use that infrastructure because they fulfil the
requirement.
So please stop these uninformed claims about brokeness and TSC
requirements. Instead please sit down and figure out whether your
particular use case of kvmclock/hyperv clock actually fit into that
functionality, i.e. whether you have
1) 'device time'
2) 'system reference time'
3) 'system time'
where
#1 and #2 can be captured atomically
#2 and #3 are coupled clocks with a fixed ratio and offset
If those requirements are fulfilled then you can simply use it as is and it
will give you CLOCK_MONOTONIC and CLOCK_REALTIME for the captured device
time.
If your use case is different and does not fit into that picture then
please write it up clearly what you are trying to achieve and we can
discuss how to make it work w/o adding duct tape hackery.
Thanks,
tglx
On Tue, 26 Sep 2017, Paolo Bonzini wrote:
> However, if get_device_system_crosststamp ran with kvmclock or Hyper-V
> clocksource, the two values stored by read_with_stamp would be
> different, basically a (TSC, nanoseconds) pair.
And that's exactly the problem. The cross time stamp infrastructure keeps
the system clocksource (e.g. TSC) and the reference clock (e.g. ART)
separate. There is enforcement that the two are coupled which I pointed out
in the other mail, but conceptually they are different things and we are
not going to change that and add voodoo callbacks which return different
timestamps from different sources into the clocksource.
So if you want to map kvmclock to the existing infrastructure then:
device clock: maps to the hypervisor clock
system reference clock: maps to REF_TSC
system timekeeper clock: maps to TSC
Even if REF_TSC and TSC are the same physically conceptually they are
different.
To make use of the existing infrastructure you have to provide means to
capture HV clock and reference clock atomically. Thats probably something
he hypervisor provides as a value pair HVCLOCK_NSEC, REF_TSC_VAL.
And then you can correlate system time to HVCLOCK_NSEC via REF_TSC_VAL
because that has a fixed ratio / offset to the system TSC.
Thanks,
tglx
On 27/09/2017 10:52, Thomas Gleixner wrote:
> But there is no requirement that current clocksource is TSC. The
> requirement is:
>
> The hardware reference clock, the one which can be captured atomically
> with device clock (PTP, audio whatever), is the coupled clock of the
> current timekeeping clocksource. Both have a fixed ratio and offset.
>
> That's completely independend of TSC. TSC/ART are a particular hardware
> implementation which can use that infrastructure because they fulfil the
> requirement.
Ok, this helps.
> So please stop these uninformed claims about brokeness and TSC
> requirements.
It was a question, not an uninformed claim. You answered the question now.
> Instead please sit down and figure out whether your
> particular use case of kvmclock/hyperv clock actually fit into that
> functionality, i.e. whether you have
>
> 1) 'device time'
> 2) 'system reference time'
> 3) 'system time'
>
> where
>
> #1 and #2 can be captured atomically
>
> #2 and #3 are coupled clocks with a fixed ratio and offset
>
> If those requirements are fulfilled then you can simply use it as is and it
> will give you CLOCK_MONOTONIC and CLOCK_REALTIME for the captured device
> time.
>
> If your use case is different and does not fit into that picture then
> please write it up clearly what you are trying to achieve and we can
> discuss how to make it work w/o adding duct tape hackery.
Yes, I understand better now why you consider read_with_stamp a hack.
And it is---but I was confused and couldn't think of anything better.
The definitions do fit KVM, and indeed there is ptp-kvm that does
something very similar to what you describe in the other mail. We have:
#1 is host time
#2 is host TSC
#3 is guest TSC
We know that #2 and #3 have a fixed ratio and offset. The point is
whether #1 and #2 can be captured atomically.
For PTP-KVM, the host tells the guest; if capturing the two is
impossible, it fails the hypercall and ioctl(PTP_SYS_OFFSET_PRECISE)
fails too.
Right now, the hypercall fails if the host clocksource is not the TSC
clocksource, which is safe.
These patches are about ascertaining whether #1 and #2 can be captured
atomically in a more generic way. In the read_with_stamp case:
- if it returns true, it gives an atomic reading of #1 and #2
- if it returns false, it gives a reading of #1 only.
I think the hook should be specific to x86. For example it could be an
array of function pointers, indexed by vclock_mode, with the same
semantics as read_with_stamp.
Paolo
On Wed, 27 Sep 2017, Paolo Bonzini wrote:
> The definitions do fit KVM, and indeed there is ptp-kvm that does
> something very similar to what you describe in the other mail. We have:
>
> #1 is host time
> #2 is host TSC
> #3 is guest TSC
>
> We know that #2 and #3 have a fixed ratio and offset. The point is
> whether #1 and #2 can be captured atomically.
>
> For PTP-KVM, the host tells the guest; if capturing the two is
> impossible, it fails the hypercall and ioctl(PTP_SYS_OFFSET_PRECISE)
> fails too.
>
> Right now, the hypercall fails if the host clocksource is not the TSC
> clocksource, which is safe.
>
> These patches are about ascertaining whether #1 and #2 can be captured
> atomically in a more generic way. In the read_with_stamp case:
>
> - if it returns true, it gives an atomic reading of #1 and #2
>
> - if it returns false, it gives a reading of #1 only.
>
>
> I think the hook should be specific to x86. For example it could be an
> array of function pointers, indexed by vclock_mode, with the same
> semantics as read_with_stamp.
I don't think you need that.
The get_time_fn() which is handed in to get_device_system_crossstamp() can
convey that information:
/*
* Try to synchronously capture device time and a system
* counter value calling back into the device driver
*/
ret = get_time_fn(&xtstamp->device, &system_counterval, ctx);
if (ret)
return ret;
So in your case get_time_fn() would be kvmclock or hyperv clock specific
and the actual hypercall implementation can return a failure code if the
requirements are not met:
1) host clock source is TSC
2) capturing of host time and TSC is atomic
The host side of the hypercall can use ktime_get_snapshot() with the boot
time extension and the extra bits I posted yesterday and fail if the
snapshot is not valid for that purpose, i.e. host clocksource is not valid
for guest correlation.
The hypercall side on the guest can then do:
if (hypercall(hypercalldata))
return -ENODEV;
system_counterval->cs = tsc_clocksource;
system_counterval->data = hypercalldata;
return 0;
Then the following check in get_device_system_crossstamp() will do the
last sanity check:
/*
* Verify that the clocksource associated with the captured
* system counter value is the same as the currently installed
* timekeeper clocksource
*/
if (tk->tkr_mono.clock != system_counterval.cs)
return -ENODEV;
which catches the case where the guest side clock source is !TSC.
Thanks,
tglx
On 27/09/2017 13:53, Thomas Gleixner wrote:
>> I think the hook should be specific to x86. For example it could be an
>> array of function pointers, indexed by vclock_mode, with the same
>> semantics as read_with_stamp.
> I don't think you need that.
>
> The get_time_fn() which is handed in to get_device_system_crossstamp() can
> convey that information:
>
> /*
> * Try to synchronously capture device time and a system
> * counter value calling back into the device driver
> */
> ret = get_time_fn(&xtstamp->device, &system_counterval, ctx);
> if (ret)
> return ret;
>
> So in your case get_time_fn() would be kvmclock or hyperv clock specific
> and the actual hypercall implementation can return a failure code if the
> requirements are not met:
>
> 1) host clock source is TSC
> 2) capturing of host time and TSC is atomic
So you are suggesting reusing the cross-timestamp hypercall to implement
nested pvclock. There are advantages and disadvantages to that.
With read_with_stamp-like callbacks:
+ running on old KVM or on Hyper-V is supported
- pvclock_gtod_copy does not go away
With hypercall-based callbacks on the contrary:
+ KVM can use ktime_get_snapshot for the bare metal case
- only very new KVM is supported
Paolo
On Wed, 27 Sep 2017, Paolo Bonzini wrote:
> On 27/09/2017 13:53, Thomas Gleixner wrote:
> >> I think the hook should be specific to x86. For example it could be an
> >> array of function pointers, indexed by vclock_mode, with the same
> >> semantics as read_with_stamp.
> > I don't think you need that.
> >
> > The get_time_fn() which is handed in to get_device_system_crossstamp() can
> > convey that information:
> >
> > /*
> > * Try to synchronously capture device time and a system
> > * counter value calling back into the device driver
> > */
> > ret = get_time_fn(&xtstamp->device, &system_counterval, ctx);
> > if (ret)
> > return ret;
> >
> > So in your case get_time_fn() would be kvmclock or hyperv clock specific
> > and the actual hypercall implementation can return a failure code if the
> > requirements are not met:
> >
> > 1) host clock source is TSC
> > 2) capturing of host time and TSC is atomic
>
> So you are suggesting reusing the cross-timestamp hypercall to implement
> nested pvclock. There are advantages and disadvantages to that.
>
> With read_with_stamp-like callbacks:
>
> + running on old KVM or on Hyper-V is supported
> - pvclock_gtod_copy does not go away
>
> With hypercall-based callbacks on the contrary:
>
> + KVM can use ktime_get_snapshot for the bare metal case
> - only very new KVM is supported
I don't think that it's an either or decision.
get_device_system_crossstamp(get_time_fn, ......)
So you can have specific get_time_fn() implementations for your situation:
old_kvm_fn()
retrieve data from pvclock_gtod copy
new_kvm_fn()
use hypercall
hyperv_fn()
do what must be done
All implementations need a way to tell you:
1) Host time
2) Host TSC timestamp which corresponds to #1
3) Validity
For old_kvm_fn() pvclock_gtod_data.clock.vclock_mode == VCLOCK_TSC
For new_kvm_fn() hypercall result
For hyperv_fn() whatever it takes
Thanks,
tglx