| Message ID | f6bab47230b00cecb67f2c5d94716c8236609967.1676610413.git.kjlx@templeofstupid.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/xen TSC related cleanups | expand |
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; } Thanks, tglx
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. -K
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? -K
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
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
On Tue, Feb 21, 2023 at 10:14:54AM +0100, Thomas Gleixner wrote: > 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; > } Thanks, I'm happy to make it look like ^ that. I should have thought to do this myself. :/ I'll send out a v2 making this correction. > 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. Unless there are objections, I think I'd prefer to err on the side of paranoia here. Sorry for the confusion. -K
On Tue, Feb 21, 2023 at 09:47:24AM +0100, Juergen Gross wrote: > 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. Thanks, and apologies. I misunderstood and thought a more complex change was requested. -K
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. kvmclock_init will lower the rating of kvmclock so that TSC clocksource can be used instead: /* * X86_FEATURE_NONSTOP_TSC is TSC runs at constant rate * with P/T states and does not stop in deep C-states. * * Invariant TSC exposed by host means kvmclock is not necessary: * can use TSC as clocksource. * */ if (boot_cpu_has(X86_FEATURE_CONSTANT_TSC) && boot_cpu_has(X86_FEATURE_NONSTOP_TSC) && !check_tsc_unstable()) kvm_clock.rating = 299;
Hi Marcelo, On Thu, Feb 23, 2023 at 11:34:06AM -0300, Marcelo Tosatti 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. > > kvmclock_init will lower the rating of kvmclock so that TSC clocksource > can be used instead: > > /* > * X86_FEATURE_NONSTOP_TSC is TSC runs at constant rate > * with P/T states and does not stop in deep C-states. > * > * Invariant TSC exposed by host means kvmclock is not necessary: > * can use TSC as clocksource. > * > */ > if (boot_cpu_has(X86_FEATURE_CONSTANT_TSC) && > boot_cpu_has(X86_FEATURE_NONSTOP_TSC) && > !check_tsc_unstable()) > kvm_clock.rating = 299; Yes, I saw the change you made to the kvmclock to do this and was inspired to try to do something similar for Xen: https://lore.kernel.org/xen-devel/20221216162118.GB2633@templeofstupid.com/ Thanks, -K
diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c index 95140609c8a8..cf6dd9f9fa6a 100644 --- a/arch/x86/xen/time.c +++ b/arch/x86/xen/time.c @@ -20,6 +20,7 @@ #include <asm/pvclock.h> #include <asm/xen/hypervisor.h> #include <asm/xen/hypercall.h> +#include <asm/xen/cpuid.h> #include <xen/events.h> #include <xen/features.h> @@ -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;
Modifies xen_tsc_safe_clocksource() to use newly defined constants from arch/x86/include/asm/xen/cpuid.h. This replaces a numeric value with XEN_CPUID_TSC_MODE_NEVER_EMULATE, and deletes a comment that is now self explanatory. There should be no change in the function's behavior. Signed-off-by: Krister Johansen <kjlx@templeofstupid.com> --- arch/x86/xen/time.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)