Re: [Xen-devel] [PATCH 01/13] x86/time.c: Use system time to calculate elapsed_nsec in tsc_get_info()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.10.15 at 16:35, <haozhong.zhang@xxxxxxxxx> wrote:
> On Fri, Oct 09, 2015 at 12:51:32AM -0600, Jan Beulich wrote:
>> >>> On 28.09.15 at 09:13, <haozhong.zhang@xxxxxxxxx> 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
 *  PV: guest has not migrated yet (and thus arch.tsc_khz == cpu_khz)
 */

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel