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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

Jan