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 12:39 PM, Haozhong Zhang wrote:
> On Fri, Oct 09, 2015 at 11:37:06AM -0400, Boris Ostrovsky wrote:
>> On 10/09/2015 10:39 AM, Jan Beulich wrote:
>>>>>> On 09.10.15 at 15:41, <boris.ostrovsky@oracle.com> wrote:
>>>> On 10/09/2015 02:51 AM, 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.
>>>> If elapsed_nsec is the time that guest has been running then how can
>>>> get_s_time(), which is system time, be the right answer here? But what
>>>> confuses me even more is that existing code is not doing that neither.
>>>>
>>>> Shouldn't elapsed_nsec be offset by d->arch.vtsc_offset on the get side?
>>>> I.e.
>>>>
>>>> *elapsed_nsec = get_s_time() - d->arch.vtsc_offset?
>>> Doesn't whether or not to adjust be the offset depend on d-arch.vtsc?
>> We only use elapsed_nsec when vtsc is set, I think. In native case (vtsc=0)
>> elapsed_nsec and d->arch.vtsc_offset are ignored.
>>
> But it is used in tsc_set_info() if a HVM domain in TSC_MODE_DEFAULT
> is migrated to a machine and the following if condition in
> tsc_set_info() is false.
>
> if ( tsc_mode == TSC_MODE_DEFAULT && host_tsc_is_safe() &&
>       (has_hvm_container_domain(d) ?
>        d->arch.tsc_khz == cpu_khz || cpu_has_tsc_ratio :
>        incarnation == 0) )


Ah, yes, then we do need to save it.

-boris