From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH] tsc: use kvmclock for calibration Date: Thu, 09 Aug 2012 17:01:34 +0300 Message-ID: <5023C2BE.5070702@redhat.com> References: <1344513463-7329-1-git-send-email-kraxel@redhat.com> <5023B2C4.90302@redhat.com> <5023C1D2.5090103@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: seabios@seabios.org, kvm@vger.kernel.org To: Gerd Hoffmann Return-path: Received: from mx1.redhat.com ([209.132.183.28]:63066 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1030188Ab2HIOVo (ORCPT ); Thu, 9 Aug 2012 10:21:44 -0400 In-Reply-To: <5023C1D2.5090103@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On 08/09/2012 04:57 PM, Gerd Hoffmann wrote: > Hi, > >>> +u64 kvm_tsc_khz(void) >>> +{ >>> + u32 eax, ebx, ecx, edx, msr; >>> + struct pvclock_vcpu_time_info time; >>> + u32 addr = (u32)(&time); >>> + u64 khz; >>> + >>> + /* check presence and figure msr number */ >>> + cpuid(KVM_CPUID_FEATURES, &eax, &ebx, &ecx, &edx); >>> + if (eax & KVM_FEATURE_CLOCKSOURCE2) { >>> + msr = MSR_KVM_SYSTEM_TIME_NEW; >>> + } else if (eax & KVM_FEATURE_CLOCKSOURCE) { >>> + msr = MSR_KVM_SYSTEM_TIME; >>> + } else { >>> + return 0; >>> + } >>> + >>> + /* ask kvm hypervisor to fill struct */ >>> + memset(&time, 0, sizeof(time)); >>> + wrmsr(msr, addr | 1); >> >> How can this work? > > It did in my testing, although maybe by pure luck ... > >> There is a 64-byte alignment requirement. > > 64 bytes? Sure? The whole struct is only 32 bytes in size ... er, the documentation says 4 bytes (so stack alignment works). I distinctly remember having a large alignment requirement so we don't cross a page or slot boundary... something's wrong here. > > Easily fixable though, just need to grab some memory with memalign > instead of using the stack. > >>> + wrmsr(msr, 0); >>> + if (time.version < 2 || time.tsc_to_system_mul == 0) >>> + return 0; >>> + >>> + /* go figure tsc frequency */ >>> + khz = pvclock_tsc_khz(&time); >>> + dprintf(1, "Using kvmclock, msr 0x%x, tsc %d MHz\n", >>> + msr, (u32)khz / 1000); >>> + return khz; >> >> That's a meaningless number. You can be migrated to a cpu or a machine >> with very different tsc. > >> You want accurate time on kvm, don't use the tsc. > > seabios uses the tsc for timeout calculations only, so it doesn't need > to be 100% accurate. The order of magnitude should be correct though. > The Linux kernel uses the value for delay loops too, so using it for the > given purpose can't be *that* horrible after all ... > > It is certainly an improvement over the current code which tries to > calibrate the tsc and gets totally broken results in case the busy host > happens to schedule the guest in the middle of calibration. > > So what do you suggest? The options I see are: > > (1) Use this patch (with alignment issue fixed of course). > (2) Do a full kvmclock implementation. Feels a bit like overkill. > (3) SeaBIOS can fallback to the PIT for timing on machines which > have no TSC. We could do that too in case we detect kvm ... What sort of timeouts are these? If seconds, maybe the rtc would be best. -- error compiling committee.c: too many arguments to function