Re: [PATCH linux-next v2] x86/xen/time: prefer tsc as clocksource when it is invariant

[Krister Johansen]

On Wed, Dec 14, 2022 at 09:17:24AM +0100, Jan Beulich wrote:
> On 13.12.2022 19:58, Krister Johansen wrote:
> > On Tue, Dec 13, 2022 at 08:23:29AM +0100, Jan Beulich wrote:
> >> On 12.12.2022 23:05, Krister Johansen wrote:
> >>> On Mon, Dec 12, 2022 at 05:46:29PM +0100, Jan Beulich wrote:
> >>>> On 12.12.2022 17:05, Krister Johansen wrote:
> >>>>> Both the Intel SDM[4] and the Xen tsc documentation explain that marking
> >>>>> a tsc as invariant means that it should be considered stable by the OS
> >>>>> and is elibile to be used as a wall clock source.  The Xen documentation
> >>>>> further clarifies that this is only reliable on HVM and PVH because PV
> >>>>> cannot intercept a cpuid instruction.
> >>>>
> >>>> Without meaning to express a view on the argumentation as a whole, this
> >>>> PV aspect is suspicious. Unless you open-code a use of the CPUID insn
> >>>> in the kernel, all uses of CPUID are going to be processed by Xen by
> >>>> virtue of the respective pvops hook. Documentation says what it says
> >>>> for environments where this might not be the case.
> >>>
> >>> Thanks, appreciate the clarification here. Just restating this for my
> >>> own understanding: your advice would be to drop this check below?
> >>
> >> No, I'm unconvinced of if/where this transformation is really appropriate.
> > 
> > Ah, I see.  You're saying that you're not convinced that this code
> > should ever lower the priority of xen clocksource in favor of the tsc?
> > If so, are you willing to say a bit more about what you find to be
> > unconvincing?
> 
> With the not-for-PV justification not really applicable, the main question
> is how else you mean to justify that aspect. Once limited back to HVM/PVH,
> it may all be okay.

Got it.  I think I can provide an answer for this.

> >> My comment was merely to indicate that the justification for ...
> >>
> >>>>> +	if (!(xen_hvm_domain() || xen_pvh_domain()))
> >>>>> +		return 0;
> >>
> >> ... this isn't really correct.
> > 
> > The rationale for this bit of code was the justification that turns
> > out to be incorrect.  That sounds to me like I have unnessary code,
> > unless I was right by mistake?
> 
> The PV clock interface was specifically introduced because the TSC could
> not be reliably used by PV domains. This may have been purely due to
> limitations of the TSC at the time, so taking into account more modern
> stability guarantees may make it okay to be used by PV as well. But
> migration needs to be considered, and validity (for PV) of the deriving
> of the two synthetic feature bits you use also needs to be checked

The current incarnation of the patch confines itself to just cases where
tsc_mode is never emulate.  In mode default, a non-virtualized tsc is
only selected if the host tsc is safe (this is also derived from the
CPUID[80000007].EDX[8] -- tsc invariant bit), and the domain is either
hvm with tsc scaling support, or a domain where the guest tsc frequency
matches the host tsc frequency.

For the PV case, or any case I think, the administrator would have had
to take an explicit action to ensure that no virutalize is enabled on
the tsc, and the underlying feature flags would have either been
inherited from the base cpu, or explicitly added by intervention. I
presume that we'd want to honor these settings if they were enabled
manually?

I may once again be misreading the documentation, but my interpretation
of the requirements around migration was that if you set no-emulate on
the tsc it was up to you to ensure that the target system for the
migration either had the same tsc frequency, or supported scaling.

> (I find X86_FEATURE_NONSTOP_TSC particularly interesting, because PV domains
> don't really have any notion of "C states"). Note that e.g.
> early_init_intel() derives the two bits from CPUID[80000007].EDX[8],
> which is an opt-in feature for all guest types as per the present CPUID
> policy logic in the hypervisor, but then goes on and sets
> X86_FEATURE_NONSTOP_TSC_S3 (which you don't use in your patch, so just to
> point out a possible pitfall) purely based on family/model information.

Thanks for pointing this out.  It looks like it's specific to Intel Atom
systems that have a tsc that does not stop during suspend.  If this is
set, then the clocksource is eligible to be used to for suspend timing.
Since we don't set this, it looks like there's no behavior change.  E.g.
the tsc remains ineligible for consideration as a suspend clocksource.

-K