From: Sean Christopherson <seanjc@xxxxxxxxxx> Sent: Wednesday, February 26, 2025 6:18 PM
>
> Register the Hyper-V timer callbacks or saving/restoring its PV sched_clock
s/or/for/
> if and only if the timer is actually being used for sched_clock.
> Currently, Hyper-V overrides the save/restore hooks if the reference TSC
> available, whereas the Hyper-V timer code only overrides sched_clock if
> the reference TSC is available *and* it's not invariant. The flaw is
> effectively papered over by invoking the "old" save/restore callbacks as
> part of save/restore, but that's unnecessary and fragile.
The Hyper-V specific terminology here isn't quite right. There is a
PV "Hyper-V timer", but it is loaded by the guest OS with a specific value
and generates an interrupt when that value is reached. In Linux, it is used
for clockevents, but it's not a clocksource and is not used for sched_clock.
The correct Hyper-V term is "Hyper-V reference counter" (or "refcounter"
for short). The refcounter behaves like the TSC -- it's a monotonically
increasing value that is read-only, and can serve as the sched_clock.
And yes, both the Hyper-V timer and Hyper-V refcounter code is in a
source file with a name containing "timer" but not "refcounter". But
that seems to be the pattern for many of the drivers in
drivers/clocksource. :-)
>
> To avoid introducing more complexity, and to allow for additional cleanups
> of the PV sched_clock code, move the save/restore hooks and logic into
> hyperv_timer.c and simply wire up the hooks when overriding sched_clock
> itself.
>
> Note, while the Hyper-V timer code is intended to be architecture neutral,
> CONFIG_PARAVIRT is firmly x86-only, i.e. adding a small amount of x86
> specific code (which will be reduced in future cleanups) doesn't
> meaningfully pollute generic code.
I'm good with this approach.
>
> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
Modulo the terminology used in the commit message,
Reviewed-by: Michael Kelley <mhklinux@xxxxxxxxxxx>
Tested-by: Michael Kelley <mhklinux@xxxxxxxxxxx>
> ---
> arch/x86/kernel/cpu/mshyperv.c | 58 ------------------------------
> drivers/clocksource/hyperv_timer.c | 50 ++++++++++++++++++++++++++
> 2 files changed, 50 insertions(+), 58 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
> index aa60491bf738..174f6a71c899 100644
> --- a/arch/x86/kernel/cpu/mshyperv.c
> +++ b/arch/x86/kernel/cpu/mshyperv.c
> @@ -223,63 +223,6 @@ static void hv_machine_crash_shutdown(struct pt_regs *regs)
> hyperv_cleanup();
> }
> #endif /* CONFIG_CRASH_DUMP */
> -
> -static u64 hv_ref_counter_at_suspend;
> -static void (*old_save_sched_clock_state)(void);
> -static void (*old_restore_sched_clock_state)(void);
> -
> -/*
> - * Hyper-V clock counter resets during hibernation. Save and restore clock
> - * offset during suspend/resume, while also considering the time passed
> - * before suspend. This is to make sure that sched_clock using hv tsc page
> - * based clocksource, proceeds from where it left off during suspend and
> - * it shows correct time for the timestamps of kernel messages after resume.
> - */
> -static void save_hv_clock_tsc_state(void)
> -{
> - hv_ref_counter_at_suspend = hv_read_reference_counter();
> -}
> -
> -static void restore_hv_clock_tsc_state(void)
> -{
> - /*
> - * Adjust the offsets used by hv tsc clocksource to
> - * account for the time spent before hibernation.
> - * adjusted value = reference counter (time) at suspend
> - * - reference counter (time) now.
> - */
> - hv_adj_sched_clock_offset(hv_ref_counter_at_suspend -
> hv_read_reference_counter());
> -}
> -
> -/*
> - * Functions to override save_sched_clock_state and restore_sched_clock_state
> - * functions of x86_platform. The Hyper-V clock counter is reset during
> - * suspend-resume and the offset used to measure time needs to be
> - * corrected, post resume.
> - */
> -static void hv_save_sched_clock_state(void)
> -{
> - old_save_sched_clock_state();
> - save_hv_clock_tsc_state();
> -}
> -
> -static void hv_restore_sched_clock_state(void)
> -{
> - restore_hv_clock_tsc_state();
> - old_restore_sched_clock_state();
> -}
> -
> -static void __init x86_setup_ops_for_tsc_pg_clock(void)
> -{
> - if (!(ms_hyperv.features & HV_MSR_REFERENCE_TSC_AVAILABLE))
> - return;
> -
> - old_save_sched_clock_state = x86_platform.save_sched_clock_state;
> - x86_platform.save_sched_clock_state = hv_save_sched_clock_state;
> -
> - old_restore_sched_clock_state = x86_platform.restore_sched_clock_state;
> - x86_platform.restore_sched_clock_state = hv_restore_sched_clock_state;
> -}
> #endif /* CONFIG_HYPERV */
>
> static uint32_t __init ms_hyperv_platform(void)
> @@ -635,7 +578,6 @@ static void __init ms_hyperv_init_platform(void)
>
> /* Register Hyper-V specific clocksource */
> hv_init_clocksource();
> - x86_setup_ops_for_tsc_pg_clock();
> hv_vtl_init_platform();
> #endif
> /*
> diff --git a/drivers/clocksource/hyperv_timer.c b/drivers/clocksource/hyperv_timer.c
> index f00019b078a7..86a55167bf5d 100644
> --- a/drivers/clocksource/hyperv_timer.c
> +++ b/drivers/clocksource/hyperv_timer.c
> @@ -534,10 +534,60 @@ static __always_inline void hv_setup_sched_clock(void
> *sched_clock)
> sched_clock_register(sched_clock, 64, NSEC_PER_SEC);
> }
> #elif defined CONFIG_PARAVIRT
> +static u64 hv_ref_counter_at_suspend;
> +static void (*old_save_sched_clock_state)(void);
> +static void (*old_restore_sched_clock_state)(void);
> +
> +/*
> + * Hyper-V clock counter resets during hibernation. Save and restore clock
> + * offset during suspend/resume, while also considering the time passed
> + * before suspend. This is to make sure that sched_clock using hv tsc page
> + * based clocksource, proceeds from where it left off during suspend and
> + * it shows correct time for the timestamps of kernel messages after resume.
> + */
> +static void save_hv_clock_tsc_state(void)
> +{
> + hv_ref_counter_at_suspend = hv_read_reference_counter();
> +}
> +
> +static void restore_hv_clock_tsc_state(void)
> +{
> + /*
> + * Adjust the offsets used by hv tsc clocksource to
> + * account for the time spent before hibernation.
> + * adjusted value = reference counter (time) at suspend
> + * - reference counter (time) now.
> + */
> + hv_adj_sched_clock_offset(hv_ref_counter_at_suspend -
> hv_read_reference_counter());
> +}
> +/*
> + * Functions to override save_sched_clock_state and restore_sched_clock_state
> + * functions of x86_platform. The Hyper-V clock counter is reset during
> + * suspend-resume and the offset used to measure time needs to be
> + * corrected, post resume.
> + */
> +static void hv_save_sched_clock_state(void)
> +{
> + old_save_sched_clock_state();
> + save_hv_clock_tsc_state();
> +}
> +
> +static void hv_restore_sched_clock_state(void)
> +{
> + restore_hv_clock_tsc_state();
> + old_restore_sched_clock_state();
> +}
> +
> static __always_inline void hv_setup_sched_clock(void *sched_clock)
> {
> /* We're on x86/x64 *and* using PV ops */
> paravirt_set_sched_clock(sched_clock);
> +
> + old_save_sched_clock_state = x86_platform.save_sched_clock_state;
> + x86_platform.save_sched_clock_state = hv_save_sched_clock_state;
> +
> + old_restore_sched_clock_state = x86_platform.restore_sched_clock_state;
> + x86_platform.restore_sched_clock_state = hv_restore_sched_clock_state;
> }
> #else /* !CONFIG_GENERIC_SCHED_CLOCK && !CONFIG_PARAVIRT */
> static __always_inline void hv_setup_sched_clock(void *sched_clock) {}
> --
> 2.48.1.711.g2feabab25a-goog
>
