Messages in this thread | | | From | Thomas Gleixner <> | Subject | Re: [PATCH linux-next 2/2] x86/xen/time: cleanup xen_tsc_safe_clocksource | Date | Tue, 21 Feb 2023 10:14:54 +0100 |
| |
On Mon, Feb 20 2023 at 21:51, Krister Johansen wrote: > On Mon, Feb 20, 2023 at 08:14:40PM -0800, Krister Johansen wrote: >> > 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?
Sure. But a life migration from a NEVER_EMULATE to a non-invariant host is a patently bad idea and has nothing to do with the __init function, which is gone at that point already.
What I wanted to say:
static bool __init xen_tsc_safe_clocksource(void) { ......
/* 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 didn't have the full context and was just looking at the condition. Now I checked the full context and I think that except for the
if (check_tsc_unstable())
check everything else can go away unless you do not trust the hypervisor that it only sets the NEVER_EMULATE bit when CONSTANT and NONSTOP are set as well. But yeah, you might prefer to be paranoid. It's virt after all.
Thanks,
tglx
| |