From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH 01/13] x86/time.c: Use system time to calculate elapsed_nsec in tsc_get_info() Date: Fri, 9 Oct 2015 11:56:56 -0400 Message-ID: <5617E3C8.60701@oracle.com> References: <1443424438-13404-1-git-send-email-haozhong.zhang@intel.com> <1443424438-13404-2-git-send-email-haozhong.zhang@intel.com> <5617801402000078000A992C@prv-mh.provo.novell.com> <20151009143515.GB10042@hzzhang-OptiPlex-9020.sh.intel.com> <5617EEC102000078000A9C68@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5617EEC102000078000A9C68@prv-mh.provo.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich , Haozhong Zhang Cc: Kevin Tian , Keir Fraser , Suravee Suthikulpanit , Andrew Cooper , xen-devel@lists.xen.org, Aravind Gopalakrishnan , Jun Nakajima List-Id: xen-devel@lists.xenproject.org On 10/09/2015 10:43 AM, Jan Beulich wrote: >>>> On 09.10.15 at 16:35, wrote: >> On Fri, Oct 09, 2015 at 12:51:32AM -0600, Jan Beulich wrote: >>>>>> On 28.09.15 at 09:13, wrote: >>>> When the TSC mode of a domain is TSC_MODE_DEFAULT and no TSC emulation >>>> is used, the existing tsc_get_info() calculates elapsed_nsec by scaling >>>> the host TSC with a ratio between guest TSC rate and >>>> nanoseconds. However, the result will be incorrect if the guest TSC rate >>>> differs from the host TSC rate. This patch fixes this problem by using >>>> the system time as elapsed_nsec. >>> For both this and patch 2, while at a first glance (and taking into >>> account just the visible patch context) what you say seems to >>> make sense, the explanation is far from sufficient namely when >>> looking at the function as a whole. For one, effects on existing >>> cases need to be explicitly described, in particular why SVM's TSC >>> ratio code works without that change (or whether it has been >>> broken all along, in which case these would become backporting >>> candidates; input from SVM maintainers would be appreciated >>> too). That may in particular mean being more specific about >>> what is actually wrong with scaling the host TSC here (i.e. in >>> which way both results differ), when supposedly that matches >>> what the hardware does when TSC ratio is supported. >>> >>> Then a reason needs to be given why the similar logic in the >>> PVRDTSCP case does not also get adjusted. >>> >>> Plus, looking at the respective code in tsc_set_info(), I'm >>> getting the impression that what you're trying to do is not in line >>> with what is intended so far: Especially the comment there >>> suggests that the intention is for the guest TSC to be made >>> match the host one. Considering migration this indeed looks >>> suspicious, but then that would need changing too. >>> >> Do you mean the following comment? >> /* >> * In default mode use native TSC if the host has safe TSC and: >> * HVM/PVH: host and guest frequencies are the same (either >> * "naturally" or via TSC scaling) >> * PV: guest has not migrated yet (and thus arch.tsc_khz == cpu_khz) >> */ >> >> To my understanding, >> >> 1. "naturally" responds to the case that a domain is >> newly created (rather than being migrated from other machine) so that >> its TSC frequency (d->arch.tsc_khz) is identical to the host TSC >> frequency (cpu_khz). >> >> 2. "via TSC scaling" means the case that the domain is migrated from >> another machine of different host TSC rate so that d->arch.tsc_khz >> != cpu_khz. In this case the guest still reads the (host) TSC >> natively, but SVM TSC ratio makes sure that TSC value is a scaled >> host TSC. This point can be confirmed by svm_tsc_ratio_load() which >> sets MSR_AMD64_TSC_RATIO to d->arch.tsc_khz/cpu_khz. > I.e. they are _not_ the same (unless the quotient happens to be 1, > in which case scaling wouldn't be necessary in the first place). I.e. > imo the comment would need to be > > /* > * In default mode use native TSC if the host has safe TSC and: > * HVM/PVH: host and guest frequencies are the same or TSC > * scaling is in use Yes, that's what I meant to say. I was referring to "virtual" frequency. -boris > * PV: guest has not migrated yet (and thus arch.tsc_khz == cpu_khz) > */ > > Jan >