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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 10/09/2015 10:43 AM, Jan Beulich wrote:
>>>> On 09.10.15 at 16:35, <haozhong.zhang@intel.com> wrote:
>> On Fri, Oct 09, 2015 at 12:51:32AM -0600, Jan Beulich wrote:
>>>>>> On 28.09.15 at 09:13, <haozhong.zhang@intel.com> 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
>