[PATCH 1/1] x86/hyperv: Initialize clockevents earlier in CPU onlining

[PATCH 1/1] x86/hyperv: Initialize clockevents earlier in CPU onlining

[Michael Kelley]

Hyper-V has historically initialized stimer-based clockevents late
in the process of onlining a CPU because clockevents depend on
stimer interrupts. In the original Hyper-V design, stimer
interrupts generate a VMbus message, so the VMbus machinery must
be running first, and VMbus can't be initialized until relatively
late. On x86/64, LAPIC timer based clockevents are used during early
initialization before VMbus and stimer-based clockevents are ready,
and again during CPU offlining after the stimer clockevents have been
shut down.

Unfortunately, this design creates problems when offling CPUs for
hibernation or other purposes. stimer-based clockevents are shut
down relatively early in the offlining process, so
clockevents_unbind_device() must be used to fallback to the
LAPIC-based clockevents for the remainder of the offlining process.
Furthermore, the late initialization and early shutdown of
stimer-based clockevents doesn't work well on ARM64 since there
is no other timer like the LAPIC to fallback to. So CPU onlining and
offlining doesn't work properly.

Fix this by recognizing that stimer Direct Mode is the normal path
for newer versions of Hyper-V on x86/64, and the only path on other
architectures. With stimer Direct Mode, stimer interrupts don't
require any VMbus machinery. stimer clockevents can be initialized
and shut down consistent with how it is done for other clockevent
devices. While the old VMbus-based stimer interrupts must still
be supported for backward compatibility on x86/64, that mode of
operation can be treated as legacy.

So add a new Hyper-V stimer entry in the CPU hotplug state
list, and use that new state when in Direct Mode. Update the
Hyper-V clocksource driver to allocate and initialize stimer
clockevents earlier during boot. Update Hyper-V initialization
and the VMbus driver to use this new design. As a result,
the LAPIC timer is no longer used during boot or CPU
onlining/offlining and clockevents_unbind_device() is not
called.  But retain the old design as a legacy implementation
for older versions of Hyper-V that don't support Direct Mode.

Signed-off-by: Michael Kelley <mikelley@microsoft.com>
---
 arch/x86/hyperv/hv_init.c          |   7 ++
 drivers/clocksource/hyperv_timer.c | 127 ++++++++++++++++++++++++++++++-------
 drivers/hv/hv.c                    |   4 +-
 drivers/hv/vmbus_drv.c             |  31 ++++-----
 include/clocksource/hyperv_timer.h |   6 +-
 include/linux/cpuhotplug.h         |   1 +
 6 files changed, 131 insertions(+), 45 deletions(-)

diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
index b2daf0e..453d5fa 100644
--- a/arch/x86/hyperv/hv_init.c
+++ b/arch/x86/hyperv/hv_init.c
@@ -323,8 +323,15 @@ void __init hyperv_init(void)
 
 	x86_init.pci.arch_init = hv_pci_init;
 
+	if (hv_stimer_alloc())
+		goto remove_hypercall_page;
+
 	return;
 
+remove_hypercall_page:
+	hypercall_msr.as_uint64 = 0;
+	wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
+	hv_hypercall_pg = NULL;
 remove_cpuhp_state:
 	cpuhp_remove_state(cpuhp);
 free_vp_assist_page:
diff --git a/drivers/clocksource/hyperv_timer.c b/drivers/clocksource/hyperv_timer.c
index 2317d4e..00f6429 100644
--- a/drivers/clocksource/hyperv_timer.c
+++ b/drivers/clocksource/hyperv_timer.c
@@ -17,6 +17,7 @@
 #include <linux/clocksource.h>
 #include <linux/sched_clock.h>
 #include <linux/mm.h>
+#include <linux/cpuhotplug.h>
 #include <clocksource/hyperv_timer.h>
 #include <asm/hyperv-tlfs.h>
 #include <asm/mshyperv.h>
@@ -30,12 +31,22 @@
  * mechanism is used when running on older versions of Hyper-V
  * that don't support Direct Mode. While Hyper-V provides
  * four stimer's per CPU, Linux uses only stimer0.
+ *
+ * Because Direct Mode does not require processing a VMbus
+ * message, stimer interrupts can be enabled earlier in the
+ * process of booting a CPU, and consistent with when timer
+ * interrupts are enabled for other clocksource drivers.
+ * However, for legacy versions of Hyper-V when Direct Mode
+ * is not enabled, setting up stimer interrupts must be
+ * delayed until VMbus is initialized and can process the
+ * interrupt message.
  */
 static bool direct_mode_enabled;
 
 static int stimer0_irq;
 static int stimer0_vector;
 static int stimer0_message_sint;
+static int stimer0_cpuhp;
 
 /*
  * ISR for when stimer0 is operating in Direct Mode.  Direct Mode
@@ -102,7 +113,7 @@ static int hv_ce_set_oneshot(struct clock_event_device *evt)
 /*
  * hv_stimer_init - Per-cpu initialization of the clockevent
  */
-void hv_stimer_init(unsigned int cpu)
+static int hv_stimer_init(unsigned int cpu)
 {
 	struct clock_event_device *ce;
 
@@ -112,7 +123,7 @@ void hv_stimer_init(unsigned int cpu)
 	 * clocksource based on emulated PIT or LAPIC timer hardware.
 	 */
 	if (!(ms_hyperv.features & HV_MSR_SYNTIMER_AVAILABLE))
-		return;
+		return 0;
 
 	ce = per_cpu_ptr(hv_clock_event, cpu);
 	ce->name = "Hyper-V clockevent";
@@ -127,28 +138,42 @@ void hv_stimer_init(unsigned int cpu)
 					HV_CLOCK_HZ,
 					HV_MIN_DELTA_TICKS,
 					HV_MAX_MAX_DELTA_TICKS);
+	return 0;
 }
-EXPORT_SYMBOL_GPL(hv_stimer_init);
 
 /*
  * hv_stimer_cleanup - Per-cpu cleanup of the clockevent
  */
-void hv_stimer_cleanup(unsigned int cpu)
+static int hv_stimer_cleanup(unsigned int cpu)
 {
 	struct clock_event_device *ce;
 
 	/* Turn off clockevent device */
 	if (ms_hyperv.features & HV_MSR_SYNTIMER_AVAILABLE) {
 		ce = per_cpu_ptr(hv_clock_event, cpu);
+
+		/*
+		 * In the legacy case where Direct Mode is not enabled
+		 * (which can only be on x86/64), stimer cleanup happens
+		 * relatively early in the CPU offlining process. We
+		 * must unbind the stimer-based clockevent device so
+		 * that the LAPIC timer can take over until clockevents
+		 * are no longer needed in the offlining process. The
+		 * unbind should not be done when Direct Mode is enabled
+		 * because we may be on an architecture where there are
+		 * no other clockevents devices to fallback to.
+		 */
+		if (!direct_mode_enabled)
+			clockevents_unbind_device(ce, cpu);
 		hv_ce_shutdown(ce);
 	}
+	return 0;
 }
-EXPORT_SYMBOL_GPL(hv_stimer_cleanup);
 
 /* hv_stimer_alloc - Global initialization of the clockevent and stimer0 */
-int hv_stimer_alloc(int sint)
+int hv_stimer_alloc(void)
 {
-	int ret;
+	int ret = 0;
 
 	hv_clock_event = alloc_percpu(struct clock_event_device);
 	if (!hv_clock_event)
@@ -159,24 +184,83 @@ int hv_stimer_alloc(int sint)
 	if (direct_mode_enabled) {
 		ret = hv_setup_stimer0_irq(&stimer0_irq, &stimer0_vector,
 				hv_stimer0_isr);
-		if (ret) {
-			free_percpu(hv_clock_event);
-			hv_clock_event = NULL;
-			return ret;
-		}
+		if (ret)
+			goto free_percpu;
+
+		/*
+		 * Since we are in Direct Mode, stimer initialization
+		 * can be done now with a CPUHP value in the same range
+		 * as other clockevent devices.
+		 */
+		ret = cpuhp_setup_state(CPUHP_AP_HYPERV_TIMER_STARTING,
+				"clockevents/hyperv/stimer:starting",
+				hv_stimer_init, hv_stimer_cleanup);
+		if (ret < 0)
+			goto free_stimer0_irq;
+		stimer0_cpuhp = ret;
 	}
+	return ret;
 
-	stimer0_message_sint = sint;
-	return 0;
+free_stimer0_irq:
+	hv_remove_stimer0_irq(stimer0_irq);
+free_percpu:
+	free_percpu(hv_clock_event);
+	hv_clock_event = NULL;
+	return ret;
 }
 EXPORT_SYMBOL_GPL(hv_stimer_alloc);
 
+/*
+ * hv_stimer_legacy_init -- Called from the VMbus driver to handle
+ * the case when Direct Mode is not enabled, and the stimer
+ * must be initialized late in the CPU onlining process.
+ *
+ */
+void hv_stimer_legacy_init(unsigned int cpu, int sint)
+{
+	if (direct_mode_enabled)
+		return;
+
+	/*
+	 * This function gets called by each vCPU, so setting the
+	 * global stimer_message_sint value each time is conceptually
+	 * not ideal, but the value passed in is always the same and
+	 * it avoids introducing yet another interface into this
+	 * clocksource driver just to set the sint in the legacy
+	 * (i.e., no Direct Mode) case.
+	 */
+	stimer0_message_sint = sint;
+	(void)hv_stimer_init(cpu);
+}
+EXPORT_SYMBOL_GPL(hv_stimer_legacy_init);
+
+/*
+ * hv_stimer_legacy_cleanup -- Called from the VMbus driver to
+ * handle the case when Direct Mode is not enabled, and the
+ * stimer must be cleaned up early in the CPU offlining
+ * process.
+ */
+void hv_stimer_legacy_cleanup(unsigned int cpu)
+{
+	if (direct_mode_enabled)
+		return;
+	(void)hv_stimer_cleanup(cpu);
+}
+EXPORT_SYMBOL_GPL(hv_stimer_legacy_cleanup);
+
+
 /* hv_stimer_free - Free global resources allocated by hv_stimer_alloc() */
 void hv_stimer_free(void)
 {
-	if (direct_mode_enabled && (stimer0_irq != 0)) {
-		hv_remove_stimer0_irq(stimer0_irq);
-		stimer0_irq = 0;
+	if (direct_mode_enabled) {
+		if (stimer0_cpuhp) {
+			cpuhp_remove_state(stimer0_cpuhp);
+			stimer0_cpuhp = 0;
+		}
+		if (stimer0_irq) {
+			hv_remove_stimer0_irq(stimer0_irq);
+			stimer0_irq = 0;
+		}
 	}
 	free_percpu(hv_clock_event);
 	hv_clock_event = NULL;
@@ -190,14 +274,11 @@ void hv_stimer_free(void)
 void hv_stimer_global_cleanup(void)
 {
 	int	cpu;
-	struct clock_event_device *ce;
 
-	if (ms_hyperv.features & HV_MSR_SYNTIMER_AVAILABLE) {
-		for_each_present_cpu(cpu) {
-			ce = per_cpu_ptr(hv_clock_event, cpu);
-			clockevents_unbind_device(ce, cpu);
-		}
+	for_each_present_cpu(cpu) {
+		hv_stimer_cleanup(cpu);
 	}
+
 	hv_stimer_free();
 }
 EXPORT_SYMBOL_GPL(hv_stimer_global_cleanup);
diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
index fcc5279..6098e0c 100644
--- a/drivers/hv/hv.c
+++ b/drivers/hv/hv.c
@@ -202,7 +202,7 @@ int hv_synic_init(unsigned int cpu)
 {
 	hv_synic_enable_regs(cpu);
 
-	hv_stimer_init(cpu);
+	hv_stimer_legacy_init(cpu, VMBUS_MESSAGE_SINT);
 
 	return 0;
 }
@@ -277,7 +277,7 @@ int hv_synic_cleanup(unsigned int cpu)
 	if (channel_found && vmbus_connection.conn_state == CONNECTED)
 		return -EBUSY;
 
-	hv_stimer_cleanup(cpu);
+	hv_stimer_legacy_cleanup(cpu);
 
 	hv_synic_disable_regs(cpu);
 
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 0ab126b..18fdddb 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1342,10 +1342,6 @@ static int vmbus_bus_init(void)
 	if (ret)
 		goto err_alloc;
 
-	ret = hv_stimer_alloc(VMBUS_MESSAGE_SINT);
-	if (ret < 0)
-		goto err_alloc;
-
 	/*
 	 * Initialize the per-cpu interrupt state and stimer state.
 	 * Then connect to the host.
@@ -1402,9 +1398,8 @@ static int vmbus_bus_init(void)
 err_connect:
 	cpuhp_remove_state(hyperv_cpuhp_online);
 err_cpuhp:
-	hv_stimer_free();
-err_alloc:
 	hv_synic_free();
+err_alloc:
 	hv_remove_vmbus_irq();
 
 	bus_unregister(&hv_bus);
@@ -2310,7 +2305,6 @@ static void hv_crash_handler(struct pt_regs *regs)
 	 */
 	vmbus_connection.conn_state = DISCONNECTED;
 	cpu = smp_processor_id();
-	hv_stimer_cleanup(cpu);
 	hv_synic_cleanup(cpu);
 	hyperv_cleanup();
 };
@@ -2318,20 +2312,23 @@ static void hv_crash_handler(struct pt_regs *regs)
 static int hv_synic_suspend(void)
 {
 	/*
-	 * When we reach here, all the non-boot CPUs have been offlined, and
-	 * the stimers on them have been unbound in hv_synic_cleanup() ->
+	 * When we reach here, all the non-boot CPUs have been offlined.
+	 * If we're in a legacy configuration where stimer Direct Mode is
+	 * not enabled, the stimers on the non-boot CPUs have been unbound
+	 * in hv_synic_cleanup() -> hv_stimer_legacy_cleanup() ->
 	 * hv_stimer_cleanup() -> clockevents_unbind_device().
 	 *
-	 * hv_synic_suspend() only runs on CPU0 with interrupts disabled. Here
-	 * we do not unbind the stimer on CPU0 because: 1) it's unnecessary
-	 * because the interrupts remain disabled between syscore_suspend()
-	 * and syscore_resume(): see create_image() and resume_target_kernel();
+	 * hv_synic_suspend() only runs on CPU0 with interrupts disabled.
+	 * Here we do not call hv_stimer_legacy_cleanup() on CPU0 because:
+	 * 1) it's unnecessary as interrupts remain disabled between
+	 * syscore_suspend() and syscore_resume(): see create_image() and
+	 * resume_target_kernel()
 	 * 2) the stimer on CPU0 is automatically disabled later by
 	 * syscore_suspend() -> timekeeping_suspend() -> tick_suspend() -> ...
-	 * -> clockevents_shutdown() -> ... -> hv_ce_shutdown(); 3) a warning
-	 * would be triggered if we call clockevents_unbind_device(), which
-	 * may sleep, in an interrupts-disabled context. So, we intentionally
-	 * don't call hv_stimer_cleanup(0) here.
+	 * -> clockevents_shutdown() -> ... -> hv_ce_shutdown()
+	 * 3) a warning would be triggered if we call
+	 * clockevents_unbind_device(), which may sleep, in an
+	 * interrupts-disabled context.
 	 */
 
 	hv_synic_disable_regs(0);
diff --git a/include/clocksource/hyperv_timer.h b/include/clocksource/hyperv_timer.h
index 422f5e5..9b477c4 100644
--- a/include/clocksource/hyperv_timer.h
+++ b/include/clocksource/hyperv_timer.h
@@ -21,10 +21,10 @@
 #define HV_MIN_DELTA_TICKS 1
 
 /* Routines called by the VMbus driver */
-extern int hv_stimer_alloc(int sint);
+extern int hv_stimer_alloc(void);
 extern void hv_stimer_free(void);
-extern void hv_stimer_init(unsigned int cpu);
-extern void hv_stimer_cleanup(unsigned int cpu);
+extern void hv_stimer_legacy_init(unsigned int cpu, int sint);
+extern void hv_stimer_legacy_cleanup(unsigned int cpu);
 extern void hv_stimer_global_cleanup(void);
 extern void hv_stimer0_isr(void);
 
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index 89d75ed..e51ee77 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -129,6 +129,7 @@ enum cpuhp_state {
 	CPUHP_AP_ARC_TIMER_STARTING,
 	CPUHP_AP_RISCV_TIMER_STARTING,
 	CPUHP_AP_CSKY_TIMER_STARTING,
+	CPUHP_AP_HYPERV_TIMER_STARTING,
 	CPUHP_AP_KVM_STARTING,
 	CPUHP_AP_KVM_ARM_VGIC_INIT_STARTING,
 	CPUHP_AP_KVM_ARM_VGIC_STARTING,
-- 
1.8.3.1

RE: [PATCH 1/1] x86/hyperv: Initialize clockevents earlier in CPU onlining

[Dexuan Cui]

> From: Michael Kelley <mikelley@microsoft.com>
> Sent: Sunday, October 27, 2019 7:10 PM
> ...
> Signed-off-by: Michael Kelley <mikelley@microsoft.com>

Should we add the 2 lines:

Cc: stable@vger.kernel.org
Fixes: fd1fea6834d0 ("clocksource/drivers: Make Hyper-V clocksource ISA agnostic")

fd1fea6834d0() removes the clockevents_unbind_device() call and this patch adds it back.

> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -323,8 +323,15 @@ void __init hyperv_init(void)
> 
>  	x86_init.pci.arch_init = hv_pci_init;
> 
> +	if (hv_stimer_alloc())
> +		goto remove_hypercall_page;
> +

The error handling is imperfect here: when I force hv_stimer_alloc() to return
-ENOMEM, I get a panic in hv_apic_eoi_write(). It looks it is because we have cleared 
the pointer 'hv_vp_assist_page' to NULL, but hv_apic_eoi_write() is still in-use.

In case hv_stimer_alloc() fails, can we set 'direct_mode_enabled' to false
and go on with the legacy Hyper-V timer or LAPIC timer? If not, maybe
we can use a BUG_ON() to explicitly panic?

>  	return;
> 
> +remove_hypercall_page:
> +	hypercall_msr.as_uint64 = 0;
> +	wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> +	hv_hypercall_pg = NULL;
>  remove_cpuhp_state:
>  	cpuhp_remove_state(cpuhp);
>  free_vp_assist_page:

> -void hv_stimer_cleanup(unsigned int cpu)
> +static int hv_stimer_cleanup(unsigned int cpu)
>  {
>  	struct clock_event_device *ce;
> 
>  	/* Turn off clockevent device */
>  	if (ms_hyperv.features & HV_MSR_SYNTIMER_AVAILABLE) {
>  		ce = per_cpu_ptr(hv_clock_event, cpu);
> +
> +		/*
> +		 * In the legacy case where Direct Mode is not enabled
> +		 * (which can only be on x86/64), stimer cleanup happens
> +		 * relatively early in the CPU offlining process. We
> +		 * must unbind the stimer-based clockevent device so
> +		 * that the LAPIC timer can take over until clockevents
> +		 * are no longer needed in the offlining process. The
> +		 * unbind should not be done when Direct Mode is enabled
> +		 * because we may be on an architecture where there are
> +		 * no other clockevents devices to fallback to.
> +		 */
> +		if (!direct_mode_enabled)
> +			clockevents_unbind_device(ce, cpu);
>  		hv_ce_shutdown(ce);

In the legacy stimer0 mode, IMO this hv_ce_shutdown() is unnecessary, 
because "clockevents_unbind_device(ce, cpu)" automatically calls 
ce->set_state_shutdown(), if ce is active:
clockevents_unbind
   __clockevents_unbind
    clockevents_replace
      tick_install_replacement
        clockevents_exchange_device
          clockevents_switch_state(old, CLOCK_EVT_STATE_DETACHED)
            __clockevents_switch_state

And, in both modes (legacy mode and direct mode), it looks incorrect to
call hv_ce_shutdown() if the current processid id != 'cpu', because
hv_ce_shutdown() -> hv_init_timer() can only access the current CPU's
MSR. Maybe we should use an IPI to run hv_ce_shutdown() on the target
CPU in direct mode?

> -int hv_stimer_alloc(int sint)
> +int hv_stimer_alloc(void)
> ...
> +		ret = cpuhp_setup_state(CPUHP_AP_HYPERV_TIMER_STARTING,
> +				"clockevents/hyperv/stimer:starting",
> +				hv_stimer_init, hv_stimer_cleanup);
> +		if (ret < 0)
> +			goto free_stimer0_irq;
> +		stimer0_cpuhp = ret;
>  	}
> +	return ret;

stimer0_cpuhp is 0 when the call is successful, so IMO the logic in 
hv_stimer_free() is incorrect. Please see below.

>  void hv_stimer_free(void)
>  {
> -	if (direct_mode_enabled && (stimer0_irq != 0)) {
> -		hv_remove_stimer0_irq(stimer0_irq);
> -		stimer0_irq = 0;
> +	if (direct_mode_enabled) {
> +		if (stimer0_cpuhp) {
> +			cpuhp_remove_state(stimer0_cpuhp);
> +			stimer0_cpuhp = 0;
> +		}
> +		if (stimer0_irq) {
> +			hv_remove_stimer0_irq(stimer0_irq);
> +			stimer0_irq = 0;
> +		}
>  	}

IMO this should be 
if (direct_mode_enabled) {
        if (stimer0_cpuhp == 0)
                cpuhp_remove_state(CPUHP_AP_HYPERV_TIMER_STARTING);

        if (stimer0_irq) {
                hv_remove_stimer0_irq(stimer0_irq);
                stimer0_irq = 0;
        }
}

BTW, the default value of 'stimer0_cpuhp' is 0, which means success.
Should we change the default value to a non-zero value, e.g. -1 ?

>  	free_percpu(hv_clock_event);
>  	hv_clock_event = NULL;
> @@ -190,14 +274,11 @@ void hv_stimer_free(void)
>  void hv_stimer_global_cleanup(void)
>  {
>  	int	cpu;
> -	struct clock_event_device *ce;
> 
> -	if (ms_hyperv.features & HV_MSR_SYNTIMER_AVAILABLE) {
> -		for_each_present_cpu(cpu) {
> -			ce = per_cpu_ptr(hv_clock_event, cpu);
> -			clockevents_unbind_device(ce, cpu);
> -		}
> +	for_each_present_cpu(cpu) {
> +		hv_stimer_cleanup(cpu);

hv_stimer_cleanup() -> hv_ce_shutdown() -> hv_init_timer() can not
access a remote CPU's MSR.

> @@ -2310,7 +2305,6 @@ static void hv_crash_handler(struct pt_regs *regs)
>  	 */
>  	vmbus_connection.conn_state = DISCONNECTED;
>  	cpu = smp_processor_id();
> -	hv_stimer_cleanup(cpu);
>  	hv_synic_cleanup(cpu);
>  	hyperv_cleanup();

Why should we remove the line hv_stimer_cleanup() in the crash handler?
In the crash handler, IMO we'd better disable the timer before we proceed.

Thanks,
-- Dexuan

RE: [PATCH 1/1] x86/hyperv: Initialize clockevents earlier in CPU onlining

[Michael Kelley]

From: Dexuan Cui <decui@microsoft.com> Sent: Tuesday, October 29, 2019 1:34 PM
> 
> Should we add the 2 lines:
> 
> Cc: stable@vger.kernel.org
> Fixes: fd1fea6834d0 ("clocksource/drivers: Make Hyper-V clocksource ISA agnostic")
> 
> fd1fea6834d0() removes the clockevents_unbind_device() call and this patch adds it back.

I thought about this, but the changes in this patch seem more extensive than
we might want to take as a stable fix.  The previous removal of
clockevents_unbind_device() does not have any negative effects except on
the in-progress hibernation work.  We really want to have this patch
go with the hibernation patches, so backporting these changes to stable
isn't necessary for hibernation support.  But whether to backport to
stable is a judgment call, and I'm open to arguments in favor.

> 
> > --- a/arch/x86/hyperv/hv_init.c
> > +++ b/arch/x86/hyperv/hv_init.c
> > @@ -323,8 +323,15 @@ void __init hyperv_init(void)
> >
> >  	x86_init.pci.arch_init = hv_pci_init;
> >
> > +	if (hv_stimer_alloc())
> > +		goto remove_hypercall_page;
> > +
> 
> The error handling is imperfect here: when I force hv_stimer_alloc() to return
> -ENOMEM, I get a panic in hv_apic_eoi_write(). It looks it is because we have cleared
> the pointer 'hv_vp_assist_page' to NULL, but hv_apic_eoi_write() is still in-use.

This can be fixed by doing hv_stimer_alloc() before the call to hv_apic_init().
> 
> In case hv_stimer_alloc() fails, can we set 'direct_mode_enabled' to false
> and go on with the legacy Hyper-V timer or LAPIC timer? If not, maybe
> we can use a BUG_ON() to explicitly panic?

I'll look into what can be done here.

> 
> >  	return;
> >
> > +remove_hypercall_page:
> > +	hypercall_msr.as_uint64 = 0;
> > +	wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> > +	hv_hypercall_pg = NULL;
> >  remove_cpuhp_state:
> >  	cpuhp_remove_state(cpuhp);
> >  free_vp_assist_page:
> 
> > -void hv_stimer_cleanup(unsigned int cpu)
> > +static int hv_stimer_cleanup(unsigned int cpu)
> >  {
> >  	struct clock_event_device *ce;
> >
> >  	/* Turn off clockevent device */
> >  	if (ms_hyperv.features & HV_MSR_SYNTIMER_AVAILABLE) {
> >  		ce = per_cpu_ptr(hv_clock_event, cpu);
> > +
> > +		/*
> > +		 * In the legacy case where Direct Mode is not enabled
> > +		 * (which can only be on x86/64), stimer cleanup happens
> > +		 * relatively early in the CPU offlining process. We
> > +		 * must unbind the stimer-based clockevent device so
> > +		 * that the LAPIC timer can take over until clockevents
> > +		 * are no longer needed in the offlining process. The
> > +		 * unbind should not be done when Direct Mode is enabled
> > +		 * because we may be on an architecture where there are
> > +		 * no other clockevents devices to fallback to.
> > +		 */
> > +		if (!direct_mode_enabled)
> > +			clockevents_unbind_device(ce, cpu);
> >  		hv_ce_shutdown(ce);
> 
> In the legacy stimer0 mode, IMO this hv_ce_shutdown() is unnecessary,
> because "clockevents_unbind_device(ce, cpu)" automatically calls
> ce->set_state_shutdown(), if ce is active:
> clockevents_unbind
>    __clockevents_unbind
>     clockevents_replace
>       tick_install_replacement
>         clockevents_exchange_device
>           clockevents_switch_state(old, CLOCK_EVT_STATE_DETACHED)
>             __clockevents_switch_state

Agreed.  This is a carryover from even before reorganizing the code
into the separate Hyper-V clocksource driver, where this code was
present in hv_synic_cleanup():

	/* Turn off clockevent device */
	if (ms_hyperv.features & HV_MSR_SYNTIMER_AVAILABLE) {
		struct hv_per_cpu_context *hv_cpu
			= this_cpu_ptr(hv_context.cpu_context);

		clockevents_unbind_device(hv_cpu->clk_evt, cpu);
		hv_ce_shutdown(hv_cpu->clk_evt);
	}

But I'll fix it.

> 
> And, in both modes (legacy mode and direct mode), it looks incorrect to
> call hv_ce_shutdown() if the current processid id != 'cpu', because
> hv_ce_shutdown() -> hv_init_timer() can only access the current CPU's
> MSR. Maybe we should use an IPI to run hv_ce_shutdown() on the target
> CPU in direct mode?

Agreed.  I'll figure out a fix.

> 
> > -int hv_stimer_alloc(int sint)
> > +int hv_stimer_alloc(void)
> > ...
> > +		ret = cpuhp_setup_state(CPUHP_AP_HYPERV_TIMER_STARTING,
> > +				"clockevents/hyperv/stimer:starting",
> > +				hv_stimer_init, hv_stimer_cleanup);
> > +		if (ret < 0)
> > +			goto free_stimer0_irq;
> > +		stimer0_cpuhp = ret;
> >  	}
> > +	return ret;
> 
> stimer0_cpuhp is 0 when the call is successful, so IMO the logic in
> hv_stimer_free() is incorrect. Please see below.
> 
> >  void hv_stimer_free(void)
> >  {
> > -	if (direct_mode_enabled && (stimer0_irq != 0)) {
> > -		hv_remove_stimer0_irq(stimer0_irq);
> > -		stimer0_irq = 0;
> > +	if (direct_mode_enabled) {
> > +		if (stimer0_cpuhp) {
> > +			cpuhp_remove_state(stimer0_cpuhp);
> > +			stimer0_cpuhp = 0;
> > +		}
> > +		if (stimer0_irq) {
> > +			hv_remove_stimer0_irq(stimer0_irq);
> > +			stimer0_irq = 0;
> > +		}
> >  	}
> 
> IMO this should be
> if (direct_mode_enabled) {
>         if (stimer0_cpuhp == 0)
>                 cpuhp_remove_state(CPUHP_AP_HYPERV_TIMER_STARTING);
> 
>         if (stimer0_irq) {
>                 hv_remove_stimer0_irq(stimer0_irq);
>                 stimer0_irq = 0;
>         }
> }
> 
> BTW, the default value of 'stimer0_cpuhp' is 0, which means success.
> Should we change the default value to a non-zero value, e.g. -1 ?

Indeed, you are correct.  I was treating the return value from 
cphup_setup_state() like the case when the state is
CPUHP_AP_ONLINE_DYN.  I'll fix it.

> 
> >  	free_percpu(hv_clock_event);
> >  	hv_clock_event = NULL;
> > @@ -190,14 +274,11 @@ void hv_stimer_free(void)
> >  void hv_stimer_global_cleanup(void)
> >  {
> >  	int	cpu;
> > -	struct clock_event_device *ce;
> >
> > -	if (ms_hyperv.features & HV_MSR_SYNTIMER_AVAILABLE) {
> > -		for_each_present_cpu(cpu) {
> > -			ce = per_cpu_ptr(hv_clock_event, cpu);
> > -			clockevents_unbind_device(ce, cpu);
> > -		}
> > +	for_each_present_cpu(cpu) {
> > +		hv_stimer_cleanup(cpu);
> 
> hv_stimer_cleanup() -> hv_ce_shutdown() -> hv_init_timer() can not
> access a remote CPU's MSR.

Agreed, per above.

> 
> > @@ -2310,7 +2305,6 @@ static void hv_crash_handler(struct pt_regs *regs)
> >  	 */
> >  	vmbus_connection.conn_state = DISCONNECTED;
> >  	cpu = smp_processor_id();
> > -	hv_stimer_cleanup(cpu);
> >  	hv_synic_cleanup(cpu);
> >  	hyperv_cleanup();
> 
> Why should we remove the line hv_stimer_cleanup() in the crash handler?
> In the crash handler, IMO we'd better disable the timer before we proceed.

With the stimer in legacy mode, the cleanup should happen in
hv_synic_cleanup().  But that actually highlights a previously unrecognized
problem in that hv_synic_cleanup() usually doesn't do anything because it
returns -EBUSY if there is a channel interrupt assigned to "cpu".  So even
in the older code before the clock/timer reorg, none of the timers or
other synic functionality actually got cleaned up.

With the stimer in DirectMode, the cleanup should probably happen in
hyperv_cleanup(), to mirror the initialization in hyperv_init().  I'll make
that change.  Fixing the legacy path is probably a separate patch
because we'll need fix the synic cleanup problem by distinguishing
between a normal cleanup due to CPU offlining or hibernation, and the
crash path.

Thanks for the careful review!

Michael