From mboxrd@z Thu Jan 1 00:00:00 1970 From: Radim =?utf-8?B?S3LEjW3DocWZ?= Subject: Re: [PATCH v2] KVM: x86: fix KVM_SET_CLOCK relative to setting correct clock value Date: Wed, 10 May 2017 20:04:31 +0200 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-Type: text/plain; charset=us-ascii Cc: Paolo Bonzini , kvm-devel To: Marcelo Tosatti Return-path: 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) Content-Disposition: inline In-Reply-To: <20170503134341.GB10468@amt.cnet> Sender: kvm-owner@vger.kernel.org List-ID: 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: 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: { Thanks.