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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 10/09/2015 12:19 PM, Jan Beulich wrote:
> > > > On 09.10.15 at 18:09, <boris.ostrovsky@xxxxxxxxxx> wrote:
> > On 10/09/2015 11:11 AM, Jan Beulich wrote:
> > > > > > On 09.10.15 at 16:00, <haozhong.zhang@xxxxxxxxx> wrote:
> > > > 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.
> > > In which case - afaict - the code becomes identical to that of the
> > > TSC_MODE_ALWAYS_EMULATE case as well as the
> > > TSC_MODE_DEFAULT w/ d->arch.vtsc true. Which seems quite
> > > unlikely to be correct.
> > *elapsed_nsec = *gtsc_khz = 0; ? Because we are effectively in
> > TSC_MODE_NEVER.
> How that? Talk here has been about TSC_MODE_DEFAULT...
AFAIUI, TSC_MODE_DEFAULT is a shorthand for saying "I will let the 
hypervisor pick whether the guest will be in TSC_MODE_ALWAYS_EMULATE or 
TSC_MODE_NEVER". d->arch.vtsc is what ends up being internal 
implementation of user-provided mode (for the most parts; I think 
hvm_cpuid() being the only true exception --- and perhaps it needs to be 
looked at).
So if we have d->arch.vtsc=0 (which is the case we are talking about 
here) then we are really in NEVER mode

-boris

> > That can't be right...
> Why not? tsc_set_info() doesn't care about any of its other input
> values when that mode is in effect.
>
> Jan

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