From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753030AbdATPDx (ORCPT ); Fri, 20 Jan 2017 10:03:53 -0500 Received: from mx1.redhat.com ([209.132.183.28]:35262 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752729AbdATPDu (ORCPT ); Fri, 20 Jan 2017 10:03:50 -0500 Date: Fri, 20 Jan 2017 13:00:28 -0200 From: Marcelo Tosatti To: Radim Krcmar Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org, Paolo Bonzini , Richard Cochran , Miroslav Lichvar Subject: Re: [patch 5/5] PTP: add kvm PTP driver Message-ID: <20170120150025.GB6350@amt.cnet> References: <20170120122025.665985919@redhat.com> <20170120122503.842086637@redhat.com> <20170120141255.GA1365@potion> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170120141255.GA1365@potion> User-Agent: Mutt/1.5.21 (2010-09-15) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Fri, 20 Jan 2017 15:03:51 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Jan 20, 2017 at 03:12:56PM +0100, Radim Krcmar wrote: > 2017-01-20 10:20-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 4 +162ns[ -683ns] +/- 11ns > > > > 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. > > > > Signed-off-by: Marcelo Tosatti > > > > --- > > drivers/ptp/Kconfig | 12 ++ > > drivers/ptp/Makefile | 1 > > drivers/ptp/ptp_kvm.c | 213 ++++++++++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 226 insertions(+) > > > > v2: check for kvmclock (Radim) > > initialize global variables before device registration (Radim) > > v3: use cross timestamps callback (Paolo, Miroslav, Radim) > > > > 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-20 10:19:20.555311672 -0200 > > @@ -0,0 +1,213 @@ > > +/* > > + * 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 > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#include > > + > > +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; > > + > > +static struct kvm_clock_offset clock_off; > > +static phys_addr_t clock_off_gpa; > > + > > +/* > > + * system_counterval.cycles: kvmclock value com TSC do host. > > + * system_counterval.cs: kvmclock clocksource. > > + * device_time: host realtime clock. > > + * > > + */ > > +static int ptp_kvm_get_time_fn(ktime_t *device_time, > > + struct system_counterval_t *system_counter, > > + void *ctx) > > +{ > > + unsigned long ret; > > + struct timespec64 tspec; > > + unsigned version; > > + u8 flags; > > + 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); > > What does the lock prevent? Protects access to "struct kvm_clock_offset clock_off". > > + > > + 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. > > + */ > > + version = pvclock_read_begin(src); > > + > > + ret = kvm_hypercall2(KVM_HC_CLOCK_PAIRING, > > + clock_off_gpa, > > + KVM_CLOCK_PAIRING_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; > > + ret = __pvclock_read_cycles(src, clock_off.tsc); > > + flags = src->flags; > > + } while (pvclock_read_retry(src, version)); > > + > > + preempt_enable_notrace(); > > + > > + system_counter->cycles = ret; > > + system_counter->cs = get_kvmclock_cs(); > > Can't we use clocksource_tsc and just pass the tsc without kvmclock in > the middle? No, it has to be the kvmclock value. > > + tspec.tv_nsec = tspec.tv_nsec; Oops, yes, will resend only this patch if there are no further comments. > > (This looks extraneous.) > > Thanks.