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

[Haozhong Zhang <haozhong.zhang@intel.com>]

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.
>

I just found that patch 1 is in fact not necessary for supporting VMX
TSC scaling/SVM TSC ratio, because

 1. VMX TSC scaling and SVM TSC ratio are only used for HVM domains.
 
 2. The value of elapsed_nsec, which is modified by patch 1, is used
    to compute d->arch.vtsc_offset by tsc_set_info() for domains using
    TSC_MODE_[DEFAULT|ALWAYS_EMULATE].
    
 3. d->arch.vtsc_offset is then used in three places:
   - gtime_to_gtsc() and gtsc_to_gtime()
    In these two functions, d->arch.vtsc_offset does not take effect
    for HVM domains.
   - cpuid_time_leaf()
    It's only used for domains using TSC_MODE_PVRDTSCP.

Therefore, I think patch 1 can be removed.


However, patch 2 is still necessary. The existing tsc_get_info() uses
the host TSC frequency as the guest TSC frequency for a domain in
TSC_MODE_DEFAULT, which could cause errors in the following example:
 - A domain d using TSC_MODE_DEFAULT is created on host A, then
   migrated to host B, and finally migrated to host C.
 - The host TSC frequencies of three hosts are f_a, f_b and f_c
   respectively and f_a != f_b and f_b != f_c.
 - Both host B and host C support TSC scaling (either VMX TSC scaling
   or SVM TSC ratio).

In above example w/o patch 2,
 1. Initially, d->arch.tsc_khz == f_a.
 
 2. In the first migration, tsc_get_info() on host A passes f_a as the
    guest TSC frequency to tsc_set_info() on host B, so that after the
    migration it's still that d->arch.tsc_khz == f_a. As TSC scaling
    takes effect, guest programs can still observe TSC in frequency f_a.
    So far so good.
    
 3. However, in the second migration, f_b (!= f_a) is passed as the
    guest TSC frequency to tsc_set_info() on host C so that after the
    migration d->arch.tsc_khz is not f_a any more. As TSC scaling
    takes effect on host C as well, the TSC frequency observed by
    guest programs changes and may break some TSC sensitive programs

    At least in my test for VMX TSC scaling, guest Linux kernel would
    complain tsc clocksource is unstable. SVM TSC ratio should have
    the same problem.

W/ patch 2, tsc_get_info() in the above case always gets the guest TSC
frequency from d->arch.tsc_khz. Then in the first migration above, it
behaves the same as before, while in the second migration it can
maintain the guest TSC frequency correctly.

> Then a reason needs to be given why the similar logic in the
> PVRDTSCP case does not also get adjusted.
>

It's just because I didn't consider about PVRDTSCP case. I will add it
in the next version.

- Haozhong

> 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.
> 
> Jan
>