Re: [PATCH] x86/hyperv: Set X86_FEATURE_TSC_RELIABLE unconditionally

[Stanislav Kinsburskii <skinsburskii@linux.microsoft.com>]

On Fri, Nov 22, 2024 at 06:33:12PM +0000, Michael Kelley wrote:
> From: Stanislav Kinsburskii <skinsburskii@linux.microsoft.com> Sent: Tuesday, November 19, 2024 4:51 PM
> > 
> > On Tue, Nov 12, 2024 at 07:48:06PM +0000, Michael Kelley wrote:
> > > From: Stanislav Kinsburskii <skinsburskii@linux.microsoft.com> Sent: Tuesday,
> > November 12, 2024 10:18 AM
> > > >
> > > > Enable X86_FEATURE_TSC_RELIABLE by default as X86_FEATURE_TSC_RELIABLE is
> > > > independent from invariant TSC and should have never been gated by the
> > > > HV_ACCESS_TSC_INVARIANT privilege.
> > >
> > > I think originally X86_FEATURE_TSC_RELIABLE was gated by the Hyper-V
> > > TSC Invariant feature because otherwise VM live migration may cause
> > > the TSC value reported by the RDTSC/RDTSCP instruction in the guest
> > > to abruptly change frequency and value. In such cases, the TSC isn't
> > > useable by the kernel or user space.
> > >
> > > Enabling the Hyper-V TSC Invariant feature fixes that by using the
> > > hardware scaling available in more recent processors to automatically
> > > fixup the TSC value returned by RDTSC/RDTSCP in the guest.
> > >
> > > Is there a practical problem that is fixed by always enabling
> > > X86_FEATURE_TSC_RELIABLE?
> > >
> > 
> > The particular problem is that HV_ACCESS_TSC_INVARIANT is not set for the
> > nested root, which in turn leads to keeping tsc clocksource watchdog
> > thread and TSC sycn check timer around.
> 
> I have trouble keeping all the different TSC "features" conceptually
> separate. :-( The TSC frequency not changing (and the value not
> abruptly jumping?) should already be represented by
> X86_FEATURE_TSC_CONSTANT.  In the kernel, X86_FEATURE_TSC_RELIABLE
> effectively only controls whether the TSC clocksource watchdog is
> enabled, and in spite of the live migration foibles, I don't see a need
> for that watchdog in a Hyper-V VM. So maybe it's OK to always set
> X86_FEATURE_TSC_RELIABLE in a Hyper-V VM, as you have
> proposed.
> 
> The "tsc_reliable" flag is also exposed to user space as part of the
> /proc/cpuinfo "flags" output, so theoretically some user space
> program could change behavior based on that flag. But that seems
> a bit far-fetched. I know there are user space programs that check
> the CPUID INVARIANT_TSC flag to know whether they can use
> the raw RDTSC instruction output to do start/stop timing. The
> Hyper-V TSC Invariant feature makes that work correctly, even
> across live migrations.
> 

It sounds to me that if X86_FEATURE_TSC_CONSTANT is available on Hyper-V, then we
can set X86_FEATURE_TSC_RELIABLE.
Is it what you are saying?

Stanislav

> Michael
> 
> > 
> > But the live migration concern you raised is indeed still out there.
> > 
> > Thank you for the input Michael, I'll think more about it.
> > 
> > Stanislav
> > 
> > > Michael
> > >
> > > >
> > > > To elaborate, the HV_ACCESS_TSC_INVARIANT privilege allows certain types of
> > > > guests to opt-in to invariant TSC by writing the
> > > > HV_X64_MSR_TSC_INVARIANT_CONTROL register. Not all guests will have this
> > > > privilege and the hypervisor will automatically opt-in certain types of
> > > > guests (e.g. EXO partitions) to invariant TSC, but this functionality is
> > > > unrelated to the TSC reliability.
> > > >
> > > > Signed-off-by: Stanislav Kinsburskii <skinsburskii@linux.microsoft.com>
> > > > ---
> > > >  arch/x86/kernel/cpu/mshyperv.c |    6 +++---
> > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
> > > > index d18078834ded..14412afcc398 100644
> > > > --- a/arch/x86/kernel/cpu/mshyperv.c
> > > > +++ b/arch/x86/kernel/cpu/mshyperv.c
> > > > @@ -515,7 +515,7 @@ static void __init ms_hyperv_init_platform(void)
> > > >  	machine_ops.crash_shutdown = hv_machine_crash_shutdown;
> > > >  #endif
> > > >  #endif
> > > > -	if (ms_hyperv.features & HV_ACCESS_TSC_INVARIANT) {
> > > > +	if (ms_hyperv.features & HV_ACCESS_TSC_INVARIANT)
> > > >  		/*
> > > >  		 * Writing to synthetic MSR 0x40000118 updates/changes the
> > > >  		 * guest visible CPUIDs. Setting bit 0 of this MSR  enables
> > > > @@ -526,8 +526,8 @@ static void __init ms_hyperv_init_platform(void)
> > > >  		 * is called.
> > > >  		 */
> > > >  		wrmsrl(HV_X64_MSR_TSC_INVARIANT_CONTROL,
> > HV_EXPOSE_INVARIANT_TSC);
> > > > -		setup_force_cpu_cap(X86_FEATURE_TSC_RELIABLE);
> > > > -	}
> > > > +
> > > > +	setup_force_cpu_cap(X86_FEATURE_TSC_RELIABLE);
> > > >
> > > >  	/*
> > > >  	 * Generation 2 instances don't support reading the NMI status from
> > > >
> > > >
> > >