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 09:41:36AM -0400, Boris Ostrovsky wrote:
> On 10/09/2015 02:51 AM, 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.
> 
> 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?
>

Yes, I should minus d->arch.vtsc_offset here.

> -boris
> 
> >
> >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.
> >
> >Jan
> >
> >
> >_______________________________________________
> >Xen-devel mailing list
> >Xen-devel@xxxxxxxxxxxxx
> >http://lists.xen.org/xen-devel
> 

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