| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20170113174014.GA9310@amt.cnet>
Date: Fri, 13 Jan 2017 15:40:14 -0200
From: Marcelo Tosatti <mtosatti@...hat.com>
To: Radim Krcmar <rkrcmar@...hat.com>
Cc: kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
Paolo Bonzini <pbonzini@...hat.com>,
Richard Cochran <richardcochran@...il.com>,
Miroslav Lichvar <mlichvar@...hat.com>
Subject: Re: [patch 3/3] PTP: add kvm PTP driver
On Fri, Jan 13, 2017 at 04:56:58PM +0100, Radim Krcmar wrote:
> 2017-01-13 10:01-0200, Marcelo Tosatti:
> > Add a driver with gettime method returning hosts realtime clock.
> > This allows Chrony to synchronize host and guest clocks with
> > high precision (see results below).
> >
> > chronyc> sources
> > MS Name/IP address Stratum Poll Reach LastRx Last sample
> > ===============================================================================
> > #* PHC0 0 3 377 6 +4ns[ +4ns] +/- 3ns
> >
> > To configure Chronyd to use PHC refclock, add the
> > following line to its configuration file:
> >
> > refclock PHC /dev/ptpX poll 3 dpoll -2 offset 0
> >
> > Where /dev/ptpX is the kvmclock PTP clock.
> >
> >
> > ---
> > drivers/ptp/Kconfig | 12 +++
> > drivers/ptp/Makefile | 1
> > drivers/ptp/ptp_kvm.c | 180 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 193 insertions(+)
> >
> > Index: kvm-ptpdriver/drivers/ptp/Kconfig
> > ===================================================================
> > --- kvm-ptpdriver.orig/drivers/ptp/Kconfig 2017-01-13 09:17:31.724568567 -0200
> > +++ kvm-ptpdriver/drivers/ptp/Kconfig 2017-01-13 09:55:33.344208894 -0200
> > @@ -90,4 +90,16 @@
> > To compile this driver as a module, choose M here: the module
> > will be called ptp_pch.
> >
> > +config PTP_1588_CLOCK_KVM
> > + tristate "KVM virtual PTP clock"
> > + depends on PTP_1588_CLOCK
> > + depends on KVM_GUEST
> > + default y
> > + help
> > + This driver adds support for using kvm infrastructure as a PTP
> > + clock. This clock is only useful if you are using KVM guests.
> > +
> > + To compile this driver as a module, choose M here: the module
> > + will be called ptp_kvm.
> > +
> > endmenu
> > Index: kvm-ptpdriver/drivers/ptp/ptp_kvm.c
> > ===================================================================
> > --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> > +++ kvm-ptpdriver/drivers/ptp/ptp_kvm.c 2017-01-13 09:57:55.013440645 -0200
> > @@ -0,0 +1,180 @@
> > +/*
> > + * Virtual PTP 1588 clock for use with KVM guests
> > + *
> > + * Copyright (C) 2017 Red Hat Inc.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + */
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/init.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <uapi/linux/kvm_para.h>
> > +#include <asm/kvm_para.h>
> > +#include <asm/pvclock.h>
> > +#include <uapi/asm/kvm_para.h>
> > +
> > +#include <linux/ptp_clock_kernel.h>
> > +
> > +struct kvm_ptp_clock {
> > + struct ptp_clock *ptp_clock;
> > + struct ptp_clock_info caps;
> > +};
> > +
> > +DEFINE_SPINLOCK(kvm_ptp_lock);
> > +
> > +static struct pvclock_vsyscall_time_info *hv_clock;
> > +
> > +/*
> > + * PTP clock operations
> > + */
> > +
> > +static int ptp_kvm_adjfreq(struct ptp_clock_info *ptp, s32 ppb)
> > +{
> > + return -EOPNOTSUPP;
> > +}
> > +
> > +static int ptp_kvm_adjtime(struct ptp_clock_info *ptp, s64 delta)
> > +{
> > + return -EOPNOTSUPP;
> > +}
> > +
> > +static struct kvm_clock_offset clock_off;
> > +static phys_addr_t clock_off_gpa;
> > +
> > +static int ptp_kvm_gettime(struct ptp_clock_info *ptp, struct timespec64 *ts)
> > +{
> > + unsigned long ret;
> > + struct timespec64 tspec;
> > + u64 delta;
> > + cycle_t offset;
> > + unsigned version;
> > + int cpu;
> > + struct pvclock_vcpu_time_info *src;
> > +
> > + preempt_disable_notrace();
> > + cpu = smp_processor_id();
> > + src = &hv_clock[cpu].pvti;
> > +
> > + spin_lock(&kvm_ptp_lock);
> > +
> > + do {
> > + /*
> > + * We are measuring the delay between
> > + * kvm_hypercall and rdtsc using TSC,
> > + * and converting that delta to
> > + * tsc_to_system_mul and tsc_shift
> > + * So any changes to tsc_to_system_mul
> > + * and tsc_shift in this region
> > + * invalidate the measurement.
> > + */
>
> This assumes that the host uses kvmclock, but the guest can be using
> just TSC and even have kvmclock disabled. This code should at least
> check that kvmclock enabled.
Fixed.
> (If we made kvmclock mandatory, then we could fix pvclock_wall_clock
> interface, because it is already defined to provide real time based on
> kvmclock ...)
>
> > + version = pvclock_read_begin(src);
> > +
> > + ret = kvm_hypercall2(KVM_HC_CLOCK_OFFSET,
> > + clock_off_gpa,
> > + KVM_CLOCK_OFFSET_WALLCLOCK);
> > + if (ret != 0) {
> > + pr_err("clock offset hypercall ret %lu\n", ret);
> > + spin_unlock(&kvm_ptp_lock);
> > + preempt_enable_notrace();
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + tspec.tv_sec = clock_off.sec;
> > + tspec.tv_nsec = clock_off.nsec;
> > +
> > + delta = rdtsc_ordered() - clock_off.tsc;
> > +
> > + offset = pvclock_scale_delta(delta, src->tsc_to_system_mul,
> > + src->tsc_shift);
> > +
> > + } while (pvclock_read_retry(src, version));
> > +
> > + preempt_enable_notrace();
> > +
> > + tspec.tv_nsec = tspec.tv_nsec + offset;
> > +
> > + spin_unlock(&kvm_ptp_lock);
> > +
> > + if (tspec.tv_nsec >= NSEC_PER_SEC) {
> > + u64 secs = tspec.tv_nsec;
> > +
> > + tspec.tv_nsec = do_div(secs, NSEC_PER_SEC);
> > + tspec.tv_sec += secs;
> > + }
> > +
> > + memcpy(ts, &tspec, sizeof(struct timespec64));
>
> But the whole idea is of improving the time by reading tsc a bit later
> is just weird ... why is it better to provide
>
> tsc + x, time + tsc_delta_to_time(x)
>
> than just
>
> tsc, time
>
> ?
Because you want to calculate the value of the host realtime clock
at the moment of ptp_kvm_gettime.
We do:
1. kvm_hypercall.
2. get {sec, nsec, guest_tsc}.
3. kvm_hypercall returns.
4. delay = rdtsc() - guest_tsc.
Where delay is the delta (measured with the TSC) between points 2 and 4.
> Because we'll always be quering the time at tsc + y, where y >> x, and
> we'd likely have other problems if shifting the time base by few
> thousand cycles made a difference.
Radim, i didnt get your "tsc + x", "time + tsc_delta_to_time(x)"
formulas above. Can you be more verbose please?
> > +
> > + return 0;
> > +}
> > +
> > +static int ptp_kvm_settime(struct ptp_clock_info *ptp,
> > + const struct timespec64 *ts)
> > +{
> > + return -EOPNOTSUPP;
> > +}
> > +
> > +static int ptp_kvm_enable(struct ptp_clock_info *ptp,
> > + struct ptp_clock_request *rq, int on)
> > +{
> > + return -EOPNOTSUPP;
> > +}
> > +
> > +static struct ptp_clock_info ptp_kvm_caps = {
> > + .owner = THIS_MODULE,
> > + .name = "KVM virtual PTP",
> > + .max_adj = 0,
> > + .n_ext_ts = 0,
> > + .n_pins = 0,
> > + .pps = 0,
> > + .adjfreq = ptp_kvm_adjfreq,
> > + .adjtime = ptp_kvm_adjtime,
> > + .gettime64 = ptp_kvm_gettime,
> > + .settime64 = ptp_kvm_settime,
> > + .enable = ptp_kvm_enable,
> > +};
> > +
> > +/* module operations */
> > +
> > +static struct kvm_ptp_clock kvm_ptp_clock;
> > +
> > +static void __exit ptp_kvm_exit(void)
> > +{
> > + ptp_clock_unregister(kvm_ptp_clock.ptp_clock);
> > +}
> > +
> > +static int __init ptp_kvm_init(void)
> > +{
> > + if (!kvm_para_available())
> > + return -ENODEV;
> > +
> > + kvm_ptp_clock.caps = ptp_kvm_caps;
> > +
> > + kvm_ptp_clock.ptp_clock = ptp_clock_register(&kvm_ptp_clock.caps, NULL);
>
> It is a shame that the infrastructure uses polling when the guest could
> be notified on every host real time change, but this should be good
> enough.
Well, the whole NTP scheme is a feedback loop, so it wants to query
the value of the clock at the moments the feedback loop logic decides it.
> > + if (IS_ERR(kvm_ptp_clock.ptp_clock))
> > + return PTR_ERR(kvm_ptp_clock.ptp_clock);
> > +
> > + clock_off_gpa = slow_virt_to_phys(&clock_off);
> > +
> > + hv_clock = pvclock_pvti_cpu0_va();
>
> Would safer to assign required globals before the registration -- races
> could get ugly.
Fixed, thanks.
Powered by blists - more mailing lists