From patchwork Mon Nov 14 14:52:40 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: =?utf-8?b?UmFkaW0gS3LEjW3DocWZ?= X-Patchwork-Id: 9427667 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 4D2996047D for ; Mon, 14 Nov 2016 14:59:25 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 409AA28965 for ; Mon, 14 Nov 2016 14:59:25 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 33BE82897C; Mon, 14 Nov 2016 14:59:25 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.9 required=2.0 tests=BAYES_00,HK_RANDOM_FROM, RCVD_IN_DNSWL_HI autolearn=unavailable version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id AFE6928965 for ; Mon, 14 Nov 2016 14:59:24 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753761AbcKNO7A (ORCPT ); Mon, 14 Nov 2016 09:59:00 -0500 Received: from mx1.redhat.com ([209.132.183.28]:55334 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932730AbcKNO66 (ORCPT ); Mon, 14 Nov 2016 09:58:58 -0500 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 629D964E4; Mon, 14 Nov 2016 14:52:43 +0000 (UTC) Received: from potion (dhcp-1-247.brq.redhat.com [10.34.1.247]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with SMTP id uAEEqeuQ023525; Mon, 14 Nov 2016 09:52:41 -0500 Received: by potion (sSMTP sendmail emulation); Mon, 14 Nov 2016 15:52:40 +0100 Date: Mon, 14 Nov 2016 15:52:40 +0100 From: Radim =?utf-8?B?S3LEjW3DocWZ?= To: Paolo Bonzini Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org, mtosatti@redhat.com Subject: Re: [PATCH] KVM: x86: do not go through vcpu in __get_kvmclock_ns Message-ID: <20161114145239.GA2185@potion> References: <1478859141-25146-1-git-send-email-pbonzini@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1478859141-25146-1-git-send-email-pbonzini@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.22 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Mon, 14 Nov 2016 14:52:43 +0000 (UTC) Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 2016-11-11 11:12+0100, Paolo Bonzini: > Going through the first VCPU is wrong if you follow a KVM_SET_CLOCK with > a KVM_GET_CLOCK immediately after, without letting the VCPU run and > call kvm_guest_time_update. > > This is easily fixed however, because kvm_get_time_and_clockread provides > the information we want. > > Reported-by: Marcelo Tosatti > Signed-off-by: Paolo Bonzini > --- > arch/x86/kvm/x86.c | 18 ++++++++++-------- > 1 file changed, 10 insertions(+), 8 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 1ba08278a9a9..1c16c6d7df7a 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -1620,6 +1620,11 @@ static bool kvm_get_time_and_clockread(s64 *kernel_ns, cycle_t *cycle_now) > > return do_monotonic_boot(kernel_ns, cycle_now) == VCLOCK_TSC; > } > +#else > +static inline bool kvm_get_time_and_clockread(s64 *kernel_ns, cycle_t *cycle_now) > +{ > + return false; > +} > #endif > > /* > @@ -1724,18 +1729,15 @@ static void kvm_gen_update_masterclock(struct kvm *kvm) > > static u64 __get_kvmclock_ns(struct kvm *kvm) > { > - struct kvm_vcpu *vcpu = kvm_get_vcpu(kvm, 0); > struct kvm_arch *ka = &kvm->arch; > + cycle_t cycle_now; > s64 ns; > > - if (vcpu->arch.hv_clock.flags & PVCLOCK_TSC_STABLE_BIT) { > - u64 tsc = kvm_read_l1_tsc(vcpu, rdtsc()); > - ns = __pvclock_read_cycles(&vcpu->arch.hv_clock, tsc); This patch regresses the behavior as well, because the assumption that kvm_get_time_and_clockread() and __pvclock_read_cycles() count the same time doesn't hold. See the end of the message for a quick test. kvm_get_time_and_clockread() is actually the same as ktime_get_boot_ns() (if it works), so we'd be just obfucating the code. :) I think that making kvmclock count as ktime_get_boot_ns() would be the best solution, but not possible this late in 4.9 ... As a quick hack, I think it would be better to duplicate the update that would happen when running the VCPU before calling __pvclock_read_cycles(), i.e. paste something like this: if (kvm_check_request(KVM_REQ_CLOCK_UPDATE, vcpu)) kvm_guest_time_update(vcpu); > - } else { > - ns = ktime_get_boot_ns() + ka->kvmclock_offset; > - } > + if (!ka->use_master_clock || > + !kvm_get_time_and_clockread(&ns, &cycle_now)) > + ns = ktime_get_boot_ns(); > > - return ns; > + return ns + ka->kvmclock_offset; > } The hunk below should return the same value in pvclock_ns and kernel_ns if they can be used interchangeably. boot_ns is expected to be a bit delayed, because it is read late. boot_ns shows a bounded offset from kernel_ns, unlike the drifting pvclock_ns. and a sample output: [ 2386.172552] ns diff: -1 270 [ 2386.175695] ns diff: 14 246 [ 2386.178831] ns diff: 28 157 [ 2386.181962] ns diff: 43 176 [ 2386.185089] ns diff: 57 146 [ 2386.188228] ns diff: 71 170 [ 2386.191364] ns diff: 85 142 [ 2386.194494] ns diff: 100 138 [ 2386.197717] ns diff: 115 129 [ 2386.200935] ns diff: 128 190 [ 2386.204156] ns diff: 144 125 [ 2386.207797] ns diff: 161 145 [ 2386.211018] ns diff: 175 120 [ 2386.214239] ns diff: 189 176 [ 2386.217460] ns diff: 203 149 [ 2386.220677] ns diff: 219 110 [ 2386.223900] ns diff: 234 124 [ 2386.227125] ns diff: 248 303 [ 2386.230345] ns diff: 262 111 [ 2386.233565] ns diff: 278 107 [ 2386.236783] ns diff: 292 107 [ 2386.240001] ns diff: 306 170 [ 2386.243228] ns diff: 321 140 [ 2386.246447] ns diff: 336 101 [ 2386.249667] ns diff: 351 96 [ 2386.252787] ns diff: 365 145 [ 2386.256008] ns diff: 379 107 [ 2386.259225] ns diff: 394 99 [ 2386.262346] ns diff: 409 95 [ 2386.265467] ns diff: 423 95 [ 2386.268588] ns diff: 437 97 [ 2386.271708] ns diff: 451 165 [ 2386.274925] ns diff: 467 97 [ 2386.278047] ns diff: 480 95 [ 2386.281176] ns diff: 494 120 [ 2386.284398] ns diff: 509 408 [ 2386.287616] ns diff: 524 88 [ 2386.290736] ns diff: 538 87 [ 2386.293858] ns diff: 552 89 [...] [ 2403.966262] ns diff: 81022 68 [ 2403.969567] ns diff: 81037 68 [ 2403.972878] ns diff: 81052 68 [ 2403.976189] ns diff: 81067 66 [ 2403.979504] ns diff: 81082 69 [ 2403.982815] ns diff: 81097 67 [ 2403.986127] ns diff: 81113 67 [ 2403.989439] ns diff: 81127 69 [ 2403.992752] ns diff: 81143 70 [ 2403.996065] ns diff: 81158 70 [...] [ 2753.756071] ns diff: 1686562 65 [ 2753.759567] ns diff: 1686578 65 [ 2753.763073] ns diff: 1686596 65 [ 2753.766570] ns diff: 1686611 66 [ 2753.770075] ns diff: 1686627 67 [ 2753.773580] ns diff: 1686643 67 [ 2753.777086] ns diff: 1686658 66 [ 2753.780584] ns diff: 1686674 65 [ 2753.784093] ns diff: 1686690 69 [ 2753.787589] ns diff: 1686706 66 --- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 83990ad3710e..30d4d3d02ac7 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -6653,6 +6653,17 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) goto cancel_injection; } + if (vcpu->kvm->arch.use_master_clock) { + s64 kernel_ns; + cycle_t tsc_now, pvclock_ns, boot_ns; + + kvm_get_time_and_clockread(&kernel_ns, &tsc_now); + pvclock_ns = __pvclock_read_cycles(&vcpu->arch.hv_clock, kvm_read_l1_tsc(vcpu, tsc_now)) - vcpu->kvm->arch.kvmclock_offset; + boot_ns = ktime_get_boot_ns(); + + printk("ns diff: %lld %lld\n", pvclock_ns - kernel_ns, boot_ns - kernel_ns); + } + preempt_disable(); kvm_x86_ops->prepare_guest_switch(vcpu);