From patchwork Wed May 10 18:04:31 2017 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: 9720399 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 2D75660365 for ; Wed, 10 May 2017 18:04:57 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 5199B26E78 for ; Wed, 10 May 2017 18:04:57 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 4611128617; Wed, 10 May 2017 18:04:57 +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=-6.9 required=2.0 tests=BAYES_00,HK_RANDOM_FROM, RCVD_IN_DNSWL_HI autolearn=ham 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 BA31126E78 for ; Wed, 10 May 2017 18:04:56 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932295AbdEJSEy (ORCPT ); Wed, 10 May 2017 14:04:54 -0400 Received: from mx1.redhat.com ([209.132.183.28]:36462 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932275AbdEJSEj (ORCPT ); Wed, 10 May 2017 14:04:39 -0400 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D7CF780F90 for ; Wed, 10 May 2017 18:04:33 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com D7CF780F90 Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=rkrcmar@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com D7CF780F90 Received: from potion (dhcp-1-124.brq.redhat.com [10.34.1.124]) by smtp.corp.redhat.com (Postfix) with SMTP id 30DAB5DD6A; Wed, 10 May 2017 18:04:31 +0000 (UTC) Received: by potion (sSMTP sendmail emulation); Wed, 10 May 2017 20:04:31 +0200 Date: Wed, 10 May 2017 20:04:31 +0200 From: Radim =?utf-8?B?S3LEjW3DocWZ?= To: Marcelo Tosatti Cc: Paolo Bonzini , kvm-devel Subject: Re: [PATCH v2] KVM: x86: fix KVM_SET_CLOCK relative to setting correct clock value Message-ID: <20170510180430.GA2240@potion> References: <20170502213616.GA24837@amt.cnet> <2499ef65-1dfe-8460-ec41-661b05cc5023@redhat.com> <20170503134341.GB10468@amt.cnet> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20170503134341.GB10468@amt.cnet> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Wed, 10 May 2017 18:04:34 +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 2017-05-03 10:43-0300, Marcelo Tosatti: > In the masterclock enabled case, kvmclock_offset must be adjusted so > that user_ns.clock = master_kernel_ns + kvmclock_offset (that is, the > value set from KVM_SET_CLOCK is the one visible at system_timestamp). IIUC, we want to achieve user_ns.clock == get_kvmclock_ns(kvm) and the important fix for kvm master clock is the move of kvm_gen_update_masterclock() before we read the time. The rest is just a minor optimization that also ignores time since master_kernel_ns() and therefore pins user_ns.clock to a slightly earlier time. But all attention was given to the "minor optimization" -- have I missed something about the direct use of ka->master_kernel_ns? (If not, I'd rather have just the one-line move.) --- Detailed reasoning. kvm_gen_update_masterclock() shifts the master clock (changes its percieved frequency, because it is reset from kernel boot clock again, even though they are diverging), so we must not compute the kvmclock_offset with an old master clock and apply it to a shifted one. Using offset from ka->master_kernel_ns or get_kvmclock_ns() ("ka->master_kernel_ns + small delta") doesn't make much difference then, because the user_ns is already an indeterminable point in the past. (We just assume that user_ns.clock is now, whenever that is.) --- And a possible improvement. Using kvm_gen_update_masterclock() seems superfluous. There are three major cases depending on state of the master clock: enabled) We can just match the kvmclock_offset so the following holds user_ns.clock == get_kvmclock_ns(kvm) No need to update the master clock. disabled) The kvmclock_offset is set from the kernel boot clock and that is already correct. disabled, but call to kvm_gen_update_masterclock() would enable it) The master clock will be updates with the kernel boot clock when a VCPU runs. This means that master clock and kernel boot clock will begin diverging at a later point, but the initial offset is the same. Userspace can only tell the difference by calling KVM_GET_CLOCK afterwards and seeing the stable bit unset. Guest is mostly unaffected -- the inaccuracy from calling KVM_SET_CLOCK is much bigger than accumulation of a slightly different frequency will in few jiffies. Do you see a reason to use kvm_gen_update_masterclock()? I think that the code could be just: Thanks. diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 464da936c53d..f024216a858d 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -4177,7 +4177,7 @@ long kvm_arch_vm_ioctl(struct file *filp, r = 0; now_ns = get_kvmclock_ns(kvm); kvm->arch.kvmclock_offset += user_ns.clock - now_ns; - kvm_gen_update_masterclock(kvm); + kvm_make_all_cpus_request(kvm, KVM_REQ_CLOCK_UPDATE); break; } case KVM_GET_CLOCK: {