Re: [PATCH linux-next 2/2] x86/xen/time: cleanup xen_tsc_safe_clocksource

[Juergen Gross <jgross@suse.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 2049 bytes --]

On 21.02.23 06:51, Krister Johansen wrote:
> On Mon, Feb 20, 2023 at 08:14:40PM -0800, Krister Johansen wrote:
>> On Mon, Feb 20, 2023 at 11:01:18PM +0100, Thomas Gleixner wrote:
>>> On Mon, Feb 20 2023 at 09:17, Krister Johansen wrote:
>>>> @@ -495,8 +496,7 @@ static int __init xen_tsc_safe_clocksource(void)
>>>>   	/* Leaf 4, sub-leaf 0 (0x40000x03) */
>>>>   	cpuid_count(xen_cpuid_base() + 3, 0, &eax, &ebx, &ecx, &edx);
>>>>   
>>>> -	/* tsc_mode = no_emulate (2) */
>>>> -	if (ebx != 2)
>>>> +	if (ebx != XEN_CPUID_TSC_MODE_NEVER_EMULATE)
>>>>   		return 0;
>>>>   
>>>>   	return 1;
>>>
>>> What about removing more stupidity from that function?
>>>
>>> static bool __init xen_tsc_safe_clocksource(void)
>>> {
>>> 	u32 eax, ebx. ecx, edx;
>>>   
>>> 	/* Leaf 4, sub-leaf 0 (0x40000x03) */
>>> 	cpuid_count(xen_cpuid_base() + 3, 0, &eax, &ebx, &ecx, &edx);
>>>
>>> 	return ebx == XEN_CPUID_TSC_MODE_NEVER_EMULATE;
>>> }
>>
>> I'm all for simplifying.  I'm happy to clean up that return to be more
>> idiomatic.  I was under the impression, perhaps mistaken, though, that
>> the X86_FEATURE_CONSTANT_TSC, X86_FEATURE_NONSTOP_TSC, and
>> check_tsc_unstable() checks were actually serving a purpose: to ensure
>> that we don't rely on the tsc in environments where it's being emulated
>> and the OS would be better served by using a PV clock.  Specifically,
>> kvmclock_init() makes a very similar set of checks that I also thought
>> were load-bearing.
> 
> Bah, what I meant to say was emulated, unstable, or otherwise unsuitable
> for use as a clocksource.  IOW, even if TSC_MODE_NEVER_EMULATE is
> set, it's possible that a user is attempting a migration from a cpu
> that's not invariant, and we'd still want to check for that case and
> fall back to a PV clocksource, correct?

But Thomas' suggestion wasn't changing any behavior compared to your
patch. It just makes it easier to read.

If you are unsure your patch is correct, please verify the correctness
before sending it.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]