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

[Haozhong Zhang <haozhong.zhang@xxxxxxxxx>]

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.

If my understanding, especially the second point, is correct, this
patch set intends to do the same thing with VMX TSC scaling.

Boris, I notice this comment was added by your commit 82713ec8. Is my
understanding correct?

Thanks,
Haozhong

> Jan
> 

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