[PATCH 0/2] Drivers: hv: Move Hyper-V clock/timer code to separate clocksource driver

[PATCH 0/2] Drivers: hv: Move Hyper-V clock/timer code to separate clocksource driver

[Michael Kelley]

This patch series moves Hyper-V clock/timer code to a separate Hyper-V
clocksource driver. Previously, Hyper-V clock/timer code and data
structures were mixed in with other Hyper-V code in the ISA independent
drivers/hv code as well as in arch dependent code. The new Hyper-V
clocksource driver is ISA independent, with a just few dependencies on
arch specific functions. The patch series does not change any behavior
or functionality -- it only reorganizes the existing code and fixes up
the linkages. A few places outside of Hyper-V code are fixed up to use
the new #include file structure.

This restructuring is in response to Marc Zyngier's review comments
on supporting Hyper-V running on ARM64, and is a good idea in general.
It increases the amount of code shared between the x86 and ARM64
architectures, and reduces the size of the new code for supporting
Hyper-V on ARM64. A new version of the Hyper-V on ARM64 patches will
follow once this clocksource restructuring is accepted.

The code is currently diff'ed against Linux 5.0.  I'll rebase
to linux-next once 5.1-rc1 is available.

Michael Kelley (2):
  Drivers: hv: Move Hyper-V clockevents code to new clocksource driver
  Drivers: hv: Move Hyper-V clocksource code to new clocksource driver

 MAINTAINERS                           |   2 +
 arch/x86/entry/vdso/vclock_gettime.c  |   1 +
 arch/x86/entry/vdso/vma.c             |   2 +-
 arch/x86/hyperv/hv_init.c             |  91 +---------
 arch/x86/include/asm/hyperv-tlfs.h    |   6 +
 arch/x86/include/asm/mshyperv.h       |  80 ++-------
 arch/x86/kernel/cpu/mshyperv.c        |   2 +
 arch/x86/kvm/x86.c                    |   1 +
 drivers/clocksource/Makefile          |   1 +
 drivers/clocksource/hyperv_syntimer.c | 328 ++++++++++++++++++++++++++++++++++
 drivers/hv/hv.c                       | 154 ----------------
 drivers/hv/hyperv_vmbus.h             |   3 -
 drivers/hv/vmbus_drv.c                |  39 ++--
 include/clocksource/hyperv_syntimer.h | 104 +++++++++++
 14 files changed, 482 insertions(+), 332 deletions(-)
 create mode 100644 drivers/clocksource/hyperv_syntimer.c
 create mode 100644 include/clocksource/hyperv_syntimer.h

-- 
1.8.3.1

[PATCH 1/2] Drivers: hv: Move Hyper-V clockevents code to new clocksource driver

[Michael Kelley]

Clockevents code for Hyper-V synthetic timers is currently mixed
in with other Hyper-V code. Move the code to a Hyper-V specific
driver in the "clocksource" directory. Update the VMbus driver
to call initialization and cleanup routines since the Hyper-V
synthetic timers are not independently enumerated in ACPI.

No behavior is changed and no new functionality is added.

Signed-off-by: Michael Kelley <mikelley@microsoft.com>
---
 MAINTAINERS                           |   2 +
 arch/x86/include/asm/hyperv-tlfs.h    |   6 +
 arch/x86/kernel/cpu/mshyperv.c        |   2 +
 drivers/clocksource/Makefile          |   1 +
 drivers/clocksource/hyperv_syntimer.c | 206 ++++++++++++++++++++++++++++++++++
 drivers/hv/hv.c                       | 154 -------------------------
 drivers/hv/hyperv_vmbus.h             |   3 -
 drivers/hv/vmbus_drv.c                |  39 +++----
 include/clocksource/hyperv_syntimer.h |  26 +++++
 9 files changed, 263 insertions(+), 176 deletions(-)
 create mode 100644 drivers/clocksource/hyperv_syntimer.c
 create mode 100644 include/clocksource/hyperv_syntimer.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 21ab064..3352716 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7159,6 +7159,7 @@ F:	arch/x86/include/asm/trace/hyperv.h
 F:	arch/x86/include/asm/hyperv-tlfs.h
 F:	arch/x86/kernel/cpu/mshyperv.c
 F:	arch/x86/hyperv
+F:	drivers/clocksource/hyperv_syntimer.c
 F:	drivers/hid/hid-hyperv.c
 F:	drivers/hv/
 F:	drivers/input/serio/hyperv-keyboard.c
@@ -7168,6 +7169,7 @@ F:	drivers/scsi/storvsc_drv.c
 F:	drivers/uio/uio_hv_generic.c
 F:	drivers/video/fbdev/hyperv_fb.c
 F:	net/vmw_vsock/hyperv_transport.c
+F:	include/clocksource/hyperv_syntimer.h
 F:	include/linux/hyperv.h
 F:	include/uapi/linux/hyperv.h
 F:	tools/hv/
diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h
index 2bdbbbc..ee62f57 100644
--- a/arch/x86/include/asm/hyperv-tlfs.h
+++ b/arch/x86/include/asm/hyperv-tlfs.h
@@ -401,6 +401,12 @@ enum HV_GENERIC_SET_FORMAT {
 #define HV_STATUS_INVALID_CONNECTION_ID		18
 #define HV_STATUS_INSUFFICIENT_BUFFERS		19
 
+/*
+ * The Hyper-V TimeRefCount register and the TSC
+ * page provide a guest VM clock with 100ns tick rate
+ */
+#define HV_CLOCK_HZ (NSEC_PER_SEC/100)
+
 typedef struct _HV_REFERENCE_TSC_PAGE {
 	__u32 tsc_sequence;
 	__u32 res1;
diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
index e81a2db..f53a35a 100644
--- a/arch/x86/kernel/cpu/mshyperv.c
+++ b/arch/x86/kernel/cpu/mshyperv.c
@@ -21,6 +21,7 @@
 #include <linux/irq.h>
 #include <linux/kexec.h>
 #include <linux/i8253.h>
+#include <linux/random.h>
 #include <asm/processor.h>
 #include <asm/hypervisor.h>
 #include <asm/hyperv-tlfs.h>
@@ -84,6 +85,7 @@ __visible void __irq_entry hv_stimer0_vector_handler(struct pt_regs *regs)
 	inc_irq_stat(hyperv_stimer0_count);
 	if (hv_stimer0_handler)
 		hv_stimer0_handler();
+	add_interrupt_randomness(HYPERV_STIMER0_VECTOR, 0);
 	ack_APIC_irq();
 
 	exiting_irq();
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index be6e0fb..a887955 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -83,3 +83,4 @@ obj-$(CONFIG_ATCPIT100_TIMER)		+= timer-atcpit100.o
 obj-$(CONFIG_RISCV_TIMER)		+= timer-riscv.o
 obj-$(CONFIG_CSKY_MP_TIMER)		+= timer-mp-csky.o
 obj-$(CONFIG_GX6605S_TIMER)		+= timer-gx6605s.o
+obj-$(CONFIG_HYPERV)			+= hyperv_syntimer.o
diff --git a/drivers/clocksource/hyperv_syntimer.c b/drivers/clocksource/hyperv_syntimer.c
new file mode 100644
index 0000000..7276308
--- /dev/null
+++ b/drivers/clocksource/hyperv_syntimer.c
@@ -0,0 +1,206 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Clocksource driver for the synthetic counter and timers
+ * provided by the Hyper-V hypervisor to guest VMs, as described
+ * in the Hyper-V Top Level Functional Spec (TLFS). This driver
+ * is instruction set architecture independent.
+ *
+ * Copyright (C) 2019, Microsoft, Inc.
+ *
+ * Author:  Michael Kelley <mikelley@microsoft.com>
+ */
+
+#include <linux/percpu.h>
+#include <linux/cpumask.h>
+#include <linux/clockchips.h>
+#include <linux/cpuhotplug.h>
+#include <linux/mm.h>
+#include <clocksource/hyperv_syntimer.h>
+#include <asm/hyperv-tlfs.h>
+#include <asm/mshyperv.h>
+
+static struct clock_event_device __percpu *hv_clock_event;
+
+/*
+ * If false, we're using the old mechanism for stimer0 interrupts
+ * where it sends a VMbus message when it expires. The old
+ * 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.
+ */
+static bool direct_mode_enabled;
+
+static int stimer0_irq;
+static int stimer0_vector;
+static int stimer0_message_sint;
+static int stimer0_cpuhp_online;
+
+/*
+ * ISR for when stimer0 is operating in Direct Mode.  Direct Mode
+ * does not use VMbus or any VMbus messages, so process here and not
+ * in the VMbus driver code.
+ */
+void hv_stimer0_isr(void)
+{
+	struct clock_event_device *ce;
+
+	ce = this_cpu_ptr(hv_clock_event);
+	ce->event_handler(ce);
+}
+EXPORT_SYMBOL_GPL(hv_stimer0_isr);
+
+static int hv_ce_set_next_event(unsigned long delta,
+				struct clock_event_device *evt)
+{
+	u64 current_tick;
+
+	WARN_ON(!clockevent_state_oneshot(evt));
+
+	current_tick = hyperv_cs->read(NULL);
+	current_tick += delta;
+	hv_init_timer(0, current_tick);
+	return 0;
+}
+
+static int hv_ce_shutdown(struct clock_event_device *evt)
+{
+	hv_init_timer(0, 0);
+	hv_init_timer_config(0, 0);
+	if (direct_mode_enabled)
+		hv_disable_stimer0_percpu_irq(stimer0_irq);
+
+	return 0;
+}
+
+static int hv_ce_set_oneshot(struct clock_event_device *evt)
+{
+	union hv_stimer_config timer_cfg;
+
+	timer_cfg.as_uint64 = 0;
+	timer_cfg.enable = 1;
+	timer_cfg.auto_enable = 1;
+	if (direct_mode_enabled) {
+		/*
+		 * When it expires, the timer will directly interrupt
+		 * on the specified hardware vector/IRQ.
+		 */
+		timer_cfg.direct_mode = 1;
+		timer_cfg.apic_vector = stimer0_vector;
+		hv_enable_stimer0_percpu_irq(stimer0_irq);
+	} else {
+		/*
+		 * When it expires, the timer will generate a VMbus message,
+		 * to be handled by the normal VMbus interrupt handler.
+		 */
+		timer_cfg.direct_mode = 0;
+		timer_cfg.sintx = stimer0_message_sint;
+	}
+	hv_init_timer_config(0, timer_cfg.as_uint64);
+	return 0;
+}
+
+/*
+ * hv_syntimer_init - Per-cpu initialization of the clockevent
+ */
+static int hv_syntimer_init(unsigned int cpu)
+{
+	struct clock_event_device *ce;
+
+	if (ms_hyperv.features & HV_MSR_SYNTIMER_AVAILABLE) {
+		ce = per_cpu_ptr(hv_clock_event, cpu);
+		ce->name = "Hyper-V clockevent";
+		ce->features = CLOCK_EVT_FEAT_ONESHOT;
+		ce->cpumask = cpumask_of(cpu);
+		ce->rating = 1000;
+		ce->set_state_shutdown = hv_ce_shutdown;
+		ce->set_state_oneshot = hv_ce_set_oneshot;
+		ce->set_next_event = hv_ce_set_next_event;
+
+		clockevents_config_and_register(ce,
+						HV_CLOCK_HZ,
+						HV_MIN_DELTA_TICKS,
+						HV_MAX_MAX_DELTA_TICKS);
+	}
+	return 0;
+}
+
+/*
+ * hv_syntimer_cleanup - Per-cpu cleanup of the clockevent
+ */
+int hv_syntimer_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);
+		clockevents_unbind_device(ce, cpu);
+		hv_ce_shutdown(ce);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(hv_syntimer_cleanup);
+
+/* hv_syntimer_alloc - Global initialization of the clockevent and stimer0 */
+int hv_syntimer_alloc(int sint)
+{
+	int	ret;
+
+	hv_clock_event = alloc_percpu(struct clock_event_device);
+	if (!hv_clock_event)
+		return -ENOMEM;
+
+	direct_mode_enabled = ms_hyperv.misc_features &
+			HV_STIMER_DIRECT_MODE_AVAILABLE;
+	if (direct_mode_enabled &&
+	    hv_setup_stimer0_irq(&stimer0_irq, &stimer0_vector,
+				hv_stimer0_isr))
+		goto err_irq;
+
+	stimer0_message_sint = sint;
+
+	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "hyperv/stimer0:online",
+				hv_syntimer_init, hv_syntimer_cleanup);
+	if (ret < 0)
+		goto err_cpuhp;
+	stimer0_cpuhp_online = ret;
+	return 0;
+
+err_cpuhp:
+	if (direct_mode_enabled)
+		hv_remove_stimer0_irq(stimer0_irq);
+err_irq:
+	free_percpu(hv_clock_event);
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(hv_syntimer_alloc);
+
+/* hv_syntimer_free - Free global resources allocated by hv_syntimer_alloc() */
+void hv_syntimer_free(void)
+{
+	cpuhp_remove_state(stimer0_cpuhp_online);
+	if (direct_mode_enabled)
+		hv_remove_stimer0_irq(stimer0_irq);
+	free_percpu(hv_clock_event);
+}
+EXPORT_SYMBOL_GPL(hv_syntimer_free);
+
+/*
+ * Do a global cleanup of clockevents for the cases of kexec and
+ * vmbus exit
+ */
+void hv_syntimer_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);
+		}
+	hv_syntimer_free();
+}
+EXPORT_SYMBOL_GPL(hv_syntimer_global_cleanup);
diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
index 632d256..e3ee010 100644
--- a/drivers/hv/hv.c
+++ b/drivers/hv/hv.c
@@ -36,21 +36,6 @@
 struct hv_context hv_context;
 
 /*
- * If false, we're using the old mechanism for stimer0 interrupts
- * where it sends a VMbus message when it expires. The old
- * 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.
- */
-static bool direct_mode_enabled;
-static int stimer0_irq;
-static int stimer0_vector;
-
-#define HV_TIMER_FREQUENCY (10 * 1000 * 1000) /* 100ns period */
-#define HV_MAX_MAX_DELTA_TICKS 0xffffffff
-#define HV_MIN_DELTA_TICKS 1
-
-/*
  * hv_init - Main initialization routine.
  *
  * This routine must be called before any other routines in here are called
@@ -60,9 +45,6 @@ int hv_init(void)
 	hv_context.cpu_context = alloc_percpu(struct hv_per_cpu_context);
 	if (!hv_context.cpu_context)
 		return -ENOMEM;
-
-	direct_mode_enabled = ms_hyperv.misc_features &
-			HV_STIMER_DIRECT_MODE_AVAILABLE;
 	return 0;
 }
 
@@ -101,89 +83,6 @@ int hv_post_message(union hv_connection_id connection_id,
 	return status & 0xFFFF;
 }
 
-/*
- * ISR for when stimer0 is operating in Direct Mode.  Direct Mode
- * does not use VMbus or any VMbus messages, so process here and not
- * in the VMbus driver code.
- */
-
-static void hv_stimer0_isr(void)
-{
-	struct hv_per_cpu_context *hv_cpu;
-
-	hv_cpu = this_cpu_ptr(hv_context.cpu_context);
-	hv_cpu->clk_evt->event_handler(hv_cpu->clk_evt);
-	add_interrupt_randomness(stimer0_vector, 0);
-}
-
-static int hv_ce_set_next_event(unsigned long delta,
-				struct clock_event_device *evt)
-{
-	u64 current_tick;
-
-	WARN_ON(!clockevent_state_oneshot(evt));
-
-	current_tick = hyperv_cs->read(NULL);
-	current_tick += delta;
-	hv_init_timer(0, current_tick);
-	return 0;
-}
-
-static int hv_ce_shutdown(struct clock_event_device *evt)
-{
-	hv_init_timer(0, 0);
-	hv_init_timer_config(0, 0);
-	if (direct_mode_enabled)
-		hv_disable_stimer0_percpu_irq(stimer0_irq);
-
-	return 0;
-}
-
-static int hv_ce_set_oneshot(struct clock_event_device *evt)
-{
-	union hv_stimer_config timer_cfg;
-
-	timer_cfg.as_uint64 = 0;
-	timer_cfg.enable = 1;
-	timer_cfg.auto_enable = 1;
-	if (direct_mode_enabled) {
-		/*
-		 * When it expires, the timer will directly interrupt
-		 * on the specified hardware vector/IRQ.
-		 */
-		timer_cfg.direct_mode = 1;
-		timer_cfg.apic_vector = stimer0_vector;
-		hv_enable_stimer0_percpu_irq(stimer0_irq);
-	} else {
-		/*
-		 * When it expires, the timer will generate a VMbus message,
-		 * to be handled by the normal VMbus interrupt handler.
-		 */
-		timer_cfg.direct_mode = 0;
-		timer_cfg.sintx = VMBUS_MESSAGE_SINT;
-	}
-	hv_init_timer_config(0, timer_cfg.as_uint64);
-	return 0;
-}
-
-static void hv_init_clockevent_device(struct clock_event_device *dev, int cpu)
-{
-	dev->name = "Hyper-V clockevent";
-	dev->features = CLOCK_EVT_FEAT_ONESHOT;
-	dev->cpumask = cpumask_of(cpu);
-	dev->rating = 1000;
-	/*
-	 * Avoid settint dev->owner = THIS_MODULE deliberately as doing so will
-	 * result in clockevents_config_and_register() taking additional
-	 * references to the hv_vmbus module making it impossible to unload.
-	 */
-
-	dev->set_state_shutdown = hv_ce_shutdown;
-	dev->set_state_oneshot = hv_ce_set_oneshot;
-	dev->set_next_event = hv_ce_set_next_event;
-}
-
-
 int hv_synic_alloc(void)
 {
 	int cpu;
@@ -212,14 +111,6 @@ int hv_synic_alloc(void)
 		tasklet_init(&hv_cpu->msg_dpc,
 			     vmbus_on_msg_dpc, (unsigned long) hv_cpu);
 
-		hv_cpu->clk_evt = kzalloc(sizeof(struct clock_event_device),
-					  GFP_KERNEL);
-		if (hv_cpu->clk_evt == NULL) {
-			pr_err("Unable to allocate clock event device\n");
-			goto err;
-		}
-		hv_init_clockevent_device(hv_cpu->clk_evt, cpu);
-
 		hv_cpu->synic_message_page =
 			(void *)get_zeroed_page(GFP_ATOMIC);
 		if (hv_cpu->synic_message_page == NULL) {
@@ -242,11 +133,6 @@ int hv_synic_alloc(void)
 		INIT_LIST_HEAD(&hv_cpu->chan_list);
 	}
 
-	if (direct_mode_enabled &&
-	    hv_setup_stimer0_irq(&stimer0_irq, &stimer0_vector,
-				hv_stimer0_isr))
-		goto err;
-
 	return 0;
 err:
 	/*
@@ -265,7 +151,6 @@ void hv_synic_free(void)
 		struct hv_per_cpu_context *hv_cpu
 			= per_cpu_ptr(hv_context.cpu_context, cpu);
 
-		kfree(hv_cpu->clk_evt);
 		free_page((unsigned long)hv_cpu->synic_event_page);
 		free_page((unsigned long)hv_cpu->synic_message_page);
 		free_page((unsigned long)hv_cpu->post_msg_page);
@@ -324,39 +209,10 @@ int hv_synic_init(unsigned int cpu)
 
 	hv_set_synic_state(sctrl.as_uint64);
 
-	/*
-	 * Register the per-cpu clockevent source.
-	 */
-	if (ms_hyperv.features & HV_MSR_SYNTIMER_AVAILABLE)
-		clockevents_config_and_register(hv_cpu->clk_evt,
-						HV_TIMER_FREQUENCY,
-						HV_MIN_DELTA_TICKS,
-						HV_MAX_MAX_DELTA_TICKS);
 	return 0;
 }
 
 /*
- * hv_synic_clockevents_cleanup - Cleanup clockevent devices
- */
-void hv_synic_clockevents_cleanup(void)
-{
-	int cpu;
-
-	if (!(ms_hyperv.features & HV_MSR_SYNTIMER_AVAILABLE))
-		return;
-
-	if (direct_mode_enabled)
-		hv_remove_stimer0_irq(stimer0_irq);
-
-	for_each_present_cpu(cpu) {
-		struct hv_per_cpu_context *hv_cpu
-			= per_cpu_ptr(hv_context.cpu_context, cpu);
-
-		clockevents_unbind_device(hv_cpu->clk_evt, cpu);
-	}
-}
-
-/*
  * hv_synic_cleanup - Cleanup routine for hv_synic_init().
  */
 int hv_synic_cleanup(unsigned int cpu)
@@ -401,16 +257,6 @@ int hv_synic_cleanup(unsigned int cpu)
 	if (channel_found && vmbus_connection.conn_state == CONNECTED)
 		return -EBUSY;
 
-	/* 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);
-		put_cpu_ptr(hv_cpu);
-	}
-
 	hv_get_synint_state(VMBUS_MESSAGE_SINT, shared_sint.as_uint64);
 
 	shared_sint.masked = 1;
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index cb86b133..ffd4ad8 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -151,7 +151,6 @@ struct hv_per_cpu_context {
 	 * per-cpu list of the channels based on their CPU affinity.
 	 */
 	struct list_head chan_list;
-	struct clock_event_device *clk_evt;
 };
 
 struct hv_context {
@@ -189,8 +188,6 @@ extern int hv_post_message(union hv_connection_id connection_id,
 
 extern int hv_synic_cleanup(unsigned int cpu);
 
-extern void hv_synic_clockevents_cleanup(void);
-
 /* Interface */
 
 
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 000b53e..8e442c5 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -43,6 +43,7 @@
 #include <linux/kdebug.h>
 #include <linux/efi.h>
 #include <linux/random.h>
+#include <clocksource/hyperv_syntimer.h>
 #include "hyperv_vmbus.h"
 
 struct vmbus_dynid {
@@ -939,17 +940,6 @@ static void vmbus_onmessage_work(struct work_struct *work)
 	kfree(ctx);
 }
 
-static void hv_process_timer_expiration(struct hv_message *msg,
-					struct hv_per_cpu_context *hv_cpu)
-{
-	struct clock_event_device *dev = hv_cpu->clk_evt;
-
-	if (dev->event_handler)
-		dev->event_handler(dev);
-
-	vmbus_signal_eom(msg, HVMSG_TIMER_EXPIRED);
-}
-
 void vmbus_on_msg_dpc(unsigned long data)
 {
 	struct hv_per_cpu_context *hv_cpu = (void *)data;
@@ -1143,9 +1133,10 @@ static void vmbus_isr(void)
 
 	/* Check if there are actual msgs to be processed */
 	if (msg->header.message_type != HVMSG_NONE) {
-		if (msg->header.message_type == HVMSG_TIMER_EXPIRED)
-			hv_process_timer_expiration(msg, hv_cpu);
-		else
+		if (msg->header.message_type == HVMSG_TIMER_EXPIRED) {
+			hv_stimer0_isr();
+			vmbus_signal_eom(msg, HVMSG_TIMER_EXPIRED);
+		} else
 			tasklet_schedule(&hv_cpu->msg_dpc);
 	}
 
@@ -1248,8 +1239,8 @@ static int vmbus_bus_init(void)
 	if (ret)
 		goto err_alloc;
 	/*
-	 * Initialize the per-cpu interrupt state and
-	 * connect to the host.
+	 * Initialize the per-cpu interrupt state and syntimer state.
+	 * Then connect to the host.
 	 */
 	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "hyperv/vmbus:online",
 				hv_synic_init, hv_synic_cleanup);
@@ -1257,6 +1248,10 @@ static int vmbus_bus_init(void)
 		goto err_alloc;
 	hyperv_cpuhp_online = ret;
 
+	ret = hv_syntimer_alloc(VMBUS_MESSAGE_SINT);
+	if (ret < 0)
+		goto err_syntimer_alloc;
+
 	ret = vmbus_connect();
 	if (ret)
 		goto err_connect;
@@ -1301,6 +1296,8 @@ static int vmbus_bus_init(void)
 	return 0;
 
 err_connect:
+	hv_syntimer_free();
+err_syntimer_alloc:
 	cpuhp_remove_state(hyperv_cpuhp_online);
 err_alloc:
 	hv_synic_free();
@@ -1971,7 +1968,7 @@ static int vmbus_acpi_add(struct acpi_device *device)
 
 static void hv_kexec_handler(void)
 {
-	hv_synic_clockevents_cleanup();
+	hv_syntimer_global_cleanup();
 	vmbus_initiate_unload(false);
 	vmbus_connection.conn_state = DISCONNECTED;
 	/* Make sure conn_state is set as hv_synic_cleanup checks for it */
@@ -1982,6 +1979,8 @@ static void hv_kexec_handler(void)
 
 static void hv_crash_handler(struct pt_regs *regs)
 {
+	int cpu;
+
 	vmbus_initiate_unload(true);
 	/*
 	 * In crash handler we can't schedule synic cleanup for all CPUs,
@@ -1989,7 +1988,9 @@ static void hv_crash_handler(struct pt_regs *regs)
 	 * for kdump.
 	 */
 	vmbus_connection.conn_state = DISCONNECTED;
-	hv_synic_cleanup(smp_processor_id());
+	cpu = smp_processor_id();
+	hv_syntimer_cleanup(cpu);
+	hv_synic_cleanup(cpu);
 	hyperv_cleanup();
 };
 
@@ -2038,7 +2039,7 @@ static void __exit vmbus_exit(void)
 	hv_remove_kexec_handler();
 	hv_remove_crash_handler();
 	vmbus_connection.conn_state = DISCONNECTED;
-	hv_synic_clockevents_cleanup();
+	hv_syntimer_global_cleanup();
 	vmbus_disconnect();
 	hv_remove_vmbus_irq();
 	for_each_online_cpu(cpu) {
diff --git a/include/clocksource/hyperv_syntimer.h b/include/clocksource/hyperv_syntimer.h
new file mode 100644
index 0000000..154138b
--- /dev/null
+++ b/include/clocksource/hyperv_syntimer.h
@@ -0,0 +1,26 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * Definitions for the clocksource provided by the Hyper-V
+ * hypervisor to guest VMs, as described in the Hyper-V Top
+ * Level Functional Spec (TLFS).
+ *
+ * Copyright (C) 2019, Microsoft, Inc.
+ *
+ * Author:  Michael Kelley <mikelley@microsoft.com>
+ */
+
+#ifndef __CLKSOURCE_HYPERV_SYNTIMER_H
+#define __CLKSOURCE_HYPERV_SYNTIMER_H
+
+#define HV_MAX_MAX_DELTA_TICKS 0xffffffff
+#define HV_MIN_DELTA_TICKS 1
+
+/* Routines called by the VMbus driver */
+extern int hv_syntimer_alloc(int sint);
+extern void hv_syntimer_free(void);
+extern int hv_syntimer_cleanup(unsigned int cpu);
+extern void hv_syntimer_global_cleanup(void);
+extern void hv_stimer0_isr(void);
+
+#endif
-- 
1.8.3.1

Re: [PATCH 1/2] Drivers: hv: Move Hyper-V clockevents code to new clocksource driver

[Vitaly Kuznetsov]

Michael Kelley <mikelley@microsoft.com> writes:

> Clockevents code for Hyper-V synthetic timers is currently mixed
> in with other Hyper-V code. Move the code to a Hyper-V specific
> driver in the "clocksource" directory. Update the VMbus driver
> to call initialization and cleanup routines since the Hyper-V
> synthetic timers are not independently enumerated in ACPI.
>

I like the idea! Would it also make sense to consider moving Hyper-V
clocksource from arch/x86/hyperv/hv_init.c? The thing is that we use it
from arch-independent code (e.g. seee 'hyperv_cs' usage in
drivers/hv/hv_util.c).

> No behavior is changed and no new functionality is added.
>
> Signed-off-by: Michael Kelley <mikelley@microsoft.com>
> ---
>  MAINTAINERS                           |   2 +
>  arch/x86/include/asm/hyperv-tlfs.h    |   6 +
>  arch/x86/kernel/cpu/mshyperv.c        |   2 +
>  drivers/clocksource/Makefile          |   1 +
>  drivers/clocksource/hyperv_syntimer.c | 206 ++++++++++++++++++++++++++++++++++
>  drivers/hv/hv.c                       | 154 -------------------------
>  drivers/hv/hyperv_vmbus.h             |   3 -
>  drivers/hv/vmbus_drv.c                |  39 +++----
>  include/clocksource/hyperv_syntimer.h |  26 +++++
>  9 files changed, 263 insertions(+), 176 deletions(-)
>  create mode 100644 drivers/clocksource/hyperv_syntimer.c
>  create mode 100644 include/clocksource/hyperv_syntimer.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 21ab064..3352716 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7159,6 +7159,7 @@ F:	arch/x86/include/asm/trace/hyperv.h
>  F:	arch/x86/include/asm/hyperv-tlfs.h
>  F:	arch/x86/kernel/cpu/mshyperv.c
>  F:	arch/x86/hyperv
> +F:	drivers/clocksource/hyperv_syntimer.c
>  F:	drivers/hid/hid-hyperv.c
>  F:	drivers/hv/
>  F:	drivers/input/serio/hyperv-keyboard.c
> @@ -7168,6 +7169,7 @@ F:	drivers/scsi/storvsc_drv.c
>  F:	drivers/uio/uio_hv_generic.c
>  F:	drivers/video/fbdev/hyperv_fb.c
>  F:	net/vmw_vsock/hyperv_transport.c
> +F:	include/clocksource/hyperv_syntimer.h

Nitpicking:

This has nothing to do with your patch but we have a mix of 'stimer',
'timer', 'syntimer' names when we refer to Hyper-V Synthetic timers, it
may make sense to try to converge on something (stimer would probably be
my personal preference but I don't really care as long as we use the
same abbreviation). Examples:

hv_init_timer_config(...)
HV_MSR_SYNTIMER_AVAILABLE
HYPERV_STIMER0_VECTOR
hv_syntimer_init(...)
...

>  F:	include/linux/hyperv.h
>  F:	include/uapi/linux/hyperv.h
>  F:	tools/hv/
> diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h
> index 2bdbbbc..ee62f57 100644
> --- a/arch/x86/include/asm/hyperv-tlfs.h
> +++ b/arch/x86/include/asm/hyperv-tlfs.h
> @@ -401,6 +401,12 @@ enum HV_GENERIC_SET_FORMAT {
>  #define HV_STATUS_INVALID_CONNECTION_ID		18
>  #define HV_STATUS_INSUFFICIENT_BUFFERS		19
>  
> +/*
> + * The Hyper-V TimeRefCount register and the TSC
> + * page provide a guest VM clock with 100ns tick rate
> + */
> +#define HV_CLOCK_HZ (NSEC_PER_SEC/100)
> +
>  typedef struct _HV_REFERENCE_TSC_PAGE {
>  	__u32 tsc_sequence;
>  	__u32 res1;
> diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
> index e81a2db..f53a35a 100644
> --- a/arch/x86/kernel/cpu/mshyperv.c
> +++ b/arch/x86/kernel/cpu/mshyperv.c
> @@ -21,6 +21,7 @@
>  #include <linux/irq.h>
>  #include <linux/kexec.h>
>  #include <linux/i8253.h>
> +#include <linux/random.h>
>  #include <asm/processor.h>
>  #include <asm/hypervisor.h>
>  #include <asm/hyperv-tlfs.h>
> @@ -84,6 +85,7 @@ __visible void __irq_entry hv_stimer0_vector_handler(struct pt_regs *regs)
>  	inc_irq_stat(hyperv_stimer0_count);
>  	if (hv_stimer0_handler)
>  		hv_stimer0_handler();
> +	add_interrupt_randomness(HYPERV_STIMER0_VECTOR, 0);
>  	ack_APIC_irq();
>  
>  	exiting_irq();
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index be6e0fb..a887955 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -83,3 +83,4 @@ obj-$(CONFIG_ATCPIT100_TIMER)		+= timer-atcpit100.o
>  obj-$(CONFIG_RISCV_TIMER)		+= timer-riscv.o
>  obj-$(CONFIG_CSKY_MP_TIMER)		+= timer-mp-csky.o
>  obj-$(CONFIG_GX6605S_TIMER)		+= timer-gx6605s.o
> +obj-$(CONFIG_HYPERV)			+= hyperv_syntimer.o

(just a couple of spare thoughs)

CONFIG_HYPERV can also be a module, are we OK with that? (we'll have to
support module loading/unloading then and honestly I see no reason for
that. I would prefer everything but VMBus devices to be in
kernel.) If we don't want it to be a module we can create a hidden
CONFIG_HYPERV_STIMER or something like that - just like we already do
for CONFIG_HYPERV_TSCPAGE.

There is, however, one additional dependency here: when running in
non-direct mode, Hyper-V clockevent devices require functional Hyper-V
messaging - which currently lives in VMBus code so that may explain why
you may want to keep stimer code in the same entity. Or, alternatively,
we can move Hyper-V messaging out of VMBus code (is it actually
architecture-agnostic?)

> diff --git a/drivers/clocksource/hyperv_syntimer.c b/drivers/clocksource/hyperv_syntimer.c
> new file mode 100644
> index 0000000..7276308
> --- /dev/null
> +++ b/drivers/clocksource/hyperv_syntimer.c
> @@ -0,0 +1,206 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * Clocksource driver for the synthetic counter and timers
> + * provided by the Hyper-V hypervisor to guest VMs, as described
> + * in the Hyper-V Top Level Functional Spec (TLFS). This driver
> + * is instruction set architecture independent.
> + *
> + * Copyright (C) 2019, Microsoft, Inc.
> + *
> + * Author:  Michael Kelley <mikelley@microsoft.com>
> + */
> +
> +#include <linux/percpu.h>
> +#include <linux/cpumask.h>
> +#include <linux/clockchips.h>
> +#include <linux/cpuhotplug.h>
> +#include <linux/mm.h>
> +#include <clocksource/hyperv_syntimer.h>
> +#include <asm/hyperv-tlfs.h>
> +#include <asm/mshyperv.h>
> +
> +static struct clock_event_device __percpu *hv_clock_event;
> +
> +/*
> + * If false, we're using the old mechanism for stimer0 interrupts
> + * where it sends a VMbus message when it expires. The old
> + * 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.
> + */
> +static bool direct_mode_enabled;
> +
> +static int stimer0_irq;
> +static int stimer0_vector;
> +static int stimer0_message_sint;
> +static int stimer0_cpuhp_online;
> +
> +/*
> + * ISR for when stimer0 is operating in Direct Mode.  Direct Mode
> + * does not use VMbus or any VMbus messages, so process here and not
> + * in the VMbus driver code.
> + */
> +void hv_stimer0_isr(void)
> +{
> +	struct clock_event_device *ce;
> +
> +	ce = this_cpu_ptr(hv_clock_event);
> +	ce->event_handler(ce);
> +}
> +EXPORT_SYMBOL_GPL(hv_stimer0_isr);
> +
> +static int hv_ce_set_next_event(unsigned long delta,
> +				struct clock_event_device *evt)
> +{
> +	u64 current_tick;
> +
> +	WARN_ON(!clockevent_state_oneshot(evt));
> +
> +	current_tick = hyperv_cs->read(NULL);
> +	current_tick += delta;
> +	hv_init_timer(0, current_tick);
> +	return 0;
> +}
> +
> +static int hv_ce_shutdown(struct clock_event_device *evt)
> +{
> +	hv_init_timer(0, 0);
> +	hv_init_timer_config(0, 0);
> +	if (direct_mode_enabled)
> +		hv_disable_stimer0_percpu_irq(stimer0_irq);
> +
> +	return 0;
> +}
> +
> +static int hv_ce_set_oneshot(struct clock_event_device *evt)
> +{
> +	union hv_stimer_config timer_cfg;
> +
> +	timer_cfg.as_uint64 = 0;
> +	timer_cfg.enable = 1;
> +	timer_cfg.auto_enable = 1;
> +	if (direct_mode_enabled) {
> +		/*
> +		 * When it expires, the timer will directly interrupt
> +		 * on the specified hardware vector/IRQ.
> +		 */
> +		timer_cfg.direct_mode = 1;
> +		timer_cfg.apic_vector = stimer0_vector;
> +		hv_enable_stimer0_percpu_irq(stimer0_irq);
> +	} else {
> +		/*
> +		 * When it expires, the timer will generate a VMbus message,
> +		 * to be handled by the normal VMbus interrupt handler.
> +		 */
> +		timer_cfg.direct_mode = 0;
> +		timer_cfg.sintx = stimer0_message_sint;
> +	}
> +	hv_init_timer_config(0, timer_cfg.as_uint64);
> +	return 0;
> +}
> +
> +/*
> + * hv_syntimer_init - Per-cpu initialization of the clockevent
> + */
> +static int hv_syntimer_init(unsigned int cpu)
> +{
> +	struct clock_event_device *ce;
> +
> +	if (ms_hyperv.features & HV_MSR_SYNTIMER_AVAILABLE) {
> +		ce = per_cpu_ptr(hv_clock_event, cpu);
> +		ce->name = "Hyper-V clockevent";
> +		ce->features = CLOCK_EVT_FEAT_ONESHOT;
> +		ce->cpumask = cpumask_of(cpu);
> +		ce->rating = 1000;
> +		ce->set_state_shutdown = hv_ce_shutdown;
> +		ce->set_state_oneshot = hv_ce_set_oneshot;
> +		ce->set_next_event = hv_ce_set_next_event;
> +
> +		clockevents_config_and_register(ce,
> +						HV_CLOCK_HZ,
> +						HV_MIN_DELTA_TICKS,
> +						HV_MAX_MAX_DELTA_TICKS);
> +	}
> +	return 0;
> +}
> +
> +/*
> + * hv_syntimer_cleanup - Per-cpu cleanup of the clockevent
> + */
> +int hv_syntimer_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);
> +		clockevents_unbind_device(ce, cpu);
> +		hv_ce_shutdown(ce);
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(hv_syntimer_cleanup);
> +
> +/* hv_syntimer_alloc - Global initialization of the clockevent and stimer0 */
> +int hv_syntimer_alloc(int sint)
> +{
> +	int	ret;
> +
> +	hv_clock_event = alloc_percpu(struct clock_event_device);
> +	if (!hv_clock_event)
> +		return -ENOMEM;
> +
> +	direct_mode_enabled = ms_hyperv.misc_features &
> +			HV_STIMER_DIRECT_MODE_AVAILABLE;
> +	if (direct_mode_enabled &&
> +	    hv_setup_stimer0_irq(&stimer0_irq, &stimer0_vector,
> +				hv_stimer0_isr))
> +		goto err_irq;
> +
> +	stimer0_message_sint = sint;
> +
> +	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "hyperv/stimer0:online",
> +				hv_syntimer_init, hv_syntimer_cleanup);
> +	if (ret < 0)
> +		goto err_cpuhp;
> +	stimer0_cpuhp_online = ret;
> +	return 0;
> +
> +err_cpuhp:
> +	if (direct_mode_enabled)
> +		hv_remove_stimer0_irq(stimer0_irq);
> +err_irq:
> +	free_percpu(hv_clock_event);
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(hv_syntimer_alloc);
> +
> +/* hv_syntimer_free - Free global resources allocated by hv_syntimer_alloc() */
> +void hv_syntimer_free(void)
> +{
> +	cpuhp_remove_state(stimer0_cpuhp_online);
> +	if (direct_mode_enabled)
> +		hv_remove_stimer0_irq(stimer0_irq);
> +	free_percpu(hv_clock_event);
> +}
> +EXPORT_SYMBOL_GPL(hv_syntimer_free);
> +
> +/*
> + * Do a global cleanup of clockevents for the cases of kexec and
> + * vmbus exit
> + */
> +void hv_syntimer_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);
> +		}
> +	hv_syntimer_free();
> +}
> +EXPORT_SYMBOL_GPL(hv_syntimer_global_cleanup);
> diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
> index 632d256..e3ee010 100644
> --- a/drivers/hv/hv.c
> +++ b/drivers/hv/hv.c
> @@ -36,21 +36,6 @@
>  struct hv_context hv_context;
>  
>  /*
> - * If false, we're using the old mechanism for stimer0 interrupts
> - * where it sends a VMbus message when it expires. The old
> - * 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.
> - */
> -static bool direct_mode_enabled;
> -static int stimer0_irq;
> -static int stimer0_vector;
> -
> -#define HV_TIMER_FREQUENCY (10 * 1000 * 1000) /* 100ns period */
> -#define HV_MAX_MAX_DELTA_TICKS 0xffffffff
> -#define HV_MIN_DELTA_TICKS 1
> -
> -/*
>   * hv_init - Main initialization routine.
>   *
>   * This routine must be called before any other routines in here are called
> @@ -60,9 +45,6 @@ int hv_init(void)
>  	hv_context.cpu_context = alloc_percpu(struct hv_per_cpu_context);
>  	if (!hv_context.cpu_context)
>  		return -ENOMEM;
> -
> -	direct_mode_enabled = ms_hyperv.misc_features &
> -			HV_STIMER_DIRECT_MODE_AVAILABLE;
>  	return 0;
>  }
>  
> @@ -101,89 +83,6 @@ int hv_post_message(union hv_connection_id connection_id,
>  	return status & 0xFFFF;
>  }
>  
> -/*
> - * ISR for when stimer0 is operating in Direct Mode.  Direct Mode
> - * does not use VMbus or any VMbus messages, so process here and not
> - * in the VMbus driver code.
> - */
> -
> -static void hv_stimer0_isr(void)
> -{
> -	struct hv_per_cpu_context *hv_cpu;
> -
> -	hv_cpu = this_cpu_ptr(hv_context.cpu_context);
> -	hv_cpu->clk_evt->event_handler(hv_cpu->clk_evt);
> -	add_interrupt_randomness(stimer0_vector, 0);
> -}
> -
> -static int hv_ce_set_next_event(unsigned long delta,
> -				struct clock_event_device *evt)
> -{
> -	u64 current_tick;
> -
> -	WARN_ON(!clockevent_state_oneshot(evt));
> -
> -	current_tick = hyperv_cs->read(NULL);
> -	current_tick += delta;
> -	hv_init_timer(0, current_tick);
> -	return 0;
> -}
> -
> -static int hv_ce_shutdown(struct clock_event_device *evt)
> -{
> -	hv_init_timer(0, 0);
> -	hv_init_timer_config(0, 0);
> -	if (direct_mode_enabled)
> -		hv_disable_stimer0_percpu_irq(stimer0_irq);
> -
> -	return 0;
> -}
> -
> -static int hv_ce_set_oneshot(struct clock_event_device *evt)
> -{
> -	union hv_stimer_config timer_cfg;
> -
> -	timer_cfg.as_uint64 = 0;
> -	timer_cfg.enable = 1;
> -	timer_cfg.auto_enable = 1;
> -	if (direct_mode_enabled) {
> -		/*
> -		 * When it expires, the timer will directly interrupt
> -		 * on the specified hardware vector/IRQ.
> -		 */
> -		timer_cfg.direct_mode = 1;
> -		timer_cfg.apic_vector = stimer0_vector;
> -		hv_enable_stimer0_percpu_irq(stimer0_irq);
> -	} else {
> -		/*
> -		 * When it expires, the timer will generate a VMbus message,
> -		 * to be handled by the normal VMbus interrupt handler.
> -		 */
> -		timer_cfg.direct_mode = 0;
> -		timer_cfg.sintx = VMBUS_MESSAGE_SINT;
> -	}
> -	hv_init_timer_config(0, timer_cfg.as_uint64);
> -	return 0;
> -}
> -
> -static void hv_init_clockevent_device(struct clock_event_device *dev, int cpu)
> -{
> -	dev->name = "Hyper-V clockevent";
> -	dev->features = CLOCK_EVT_FEAT_ONESHOT;
> -	dev->cpumask = cpumask_of(cpu);
> -	dev->rating = 1000;
> -	/*
> -	 * Avoid settint dev->owner = THIS_MODULE deliberately as doing so will
> -	 * result in clockevents_config_and_register() taking additional
> -	 * references to the hv_vmbus module making it impossible to unload.
> -	 */
> -
> -	dev->set_state_shutdown = hv_ce_shutdown;
> -	dev->set_state_oneshot = hv_ce_set_oneshot;
> -	dev->set_next_event = hv_ce_set_next_event;
> -}
> -
> -
>  int hv_synic_alloc(void)
>  {
>  	int cpu;
> @@ -212,14 +111,6 @@ int hv_synic_alloc(void)
>  		tasklet_init(&hv_cpu->msg_dpc,
>  			     vmbus_on_msg_dpc, (unsigned long) hv_cpu);
>  
> -		hv_cpu->clk_evt = kzalloc(sizeof(struct clock_event_device),
> -					  GFP_KERNEL);
> -		if (hv_cpu->clk_evt == NULL) {
> -			pr_err("Unable to allocate clock event device\n");
> -			goto err;
> -		}
> -		hv_init_clockevent_device(hv_cpu->clk_evt, cpu);
> -
>  		hv_cpu->synic_message_page =
>  			(void *)get_zeroed_page(GFP_ATOMIC);
>  		if (hv_cpu->synic_message_page == NULL) {
> @@ -242,11 +133,6 @@ int hv_synic_alloc(void)
>  		INIT_LIST_HEAD(&hv_cpu->chan_list);
>  	}
>  
> -	if (direct_mode_enabled &&
> -	    hv_setup_stimer0_irq(&stimer0_irq, &stimer0_vector,
> -				hv_stimer0_isr))
> -		goto err;
> -
>  	return 0;
>  err:
>  	/*
> @@ -265,7 +151,6 @@ void hv_synic_free(void)
>  		struct hv_per_cpu_context *hv_cpu
>  			= per_cpu_ptr(hv_context.cpu_context, cpu);
>  
> -		kfree(hv_cpu->clk_evt);
>  		free_page((unsigned long)hv_cpu->synic_event_page);
>  		free_page((unsigned long)hv_cpu->synic_message_page);
>  		free_page((unsigned long)hv_cpu->post_msg_page);
> @@ -324,39 +209,10 @@ int hv_synic_init(unsigned int cpu)
>  
>  	hv_set_synic_state(sctrl.as_uint64);
>  
> -	/*
> -	 * Register the per-cpu clockevent source.
> -	 */
> -	if (ms_hyperv.features & HV_MSR_SYNTIMER_AVAILABLE)
> -		clockevents_config_and_register(hv_cpu->clk_evt,
> -						HV_TIMER_FREQUENCY,
> -						HV_MIN_DELTA_TICKS,
> -						HV_MAX_MAX_DELTA_TICKS);
>  	return 0;
>  }
>  
>  /*
> - * hv_synic_clockevents_cleanup - Cleanup clockevent devices
> - */
> -void hv_synic_clockevents_cleanup(void)
> -{
> -	int cpu;
> -
> -	if (!(ms_hyperv.features & HV_MSR_SYNTIMER_AVAILABLE))
> -		return;
> -
> -	if (direct_mode_enabled)
> -		hv_remove_stimer0_irq(stimer0_irq);
> -
> -	for_each_present_cpu(cpu) {
> -		struct hv_per_cpu_context *hv_cpu
> -			= per_cpu_ptr(hv_context.cpu_context, cpu);
> -
> -		clockevents_unbind_device(hv_cpu->clk_evt, cpu);
> -	}
> -}
> -
> -/*
>   * hv_synic_cleanup - Cleanup routine for hv_synic_init().
>   */
>  int hv_synic_cleanup(unsigned int cpu)
> @@ -401,16 +257,6 @@ int hv_synic_cleanup(unsigned int cpu)
>  	if (channel_found && vmbus_connection.conn_state == CONNECTED)
>  		return -EBUSY;
>  
> -	/* 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);
> -		put_cpu_ptr(hv_cpu);
> -	}
> -
>  	hv_get_synint_state(VMBUS_MESSAGE_SINT, shared_sint.as_uint64);
>  
>  	shared_sint.masked = 1;
> diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
> index cb86b133..ffd4ad8 100644
> --- a/drivers/hv/hyperv_vmbus.h
> +++ b/drivers/hv/hyperv_vmbus.h
> @@ -151,7 +151,6 @@ struct hv_per_cpu_context {
>  	 * per-cpu list of the channels based on their CPU affinity.
>  	 */
>  	struct list_head chan_list;
> -	struct clock_event_device *clk_evt;
>  };
>  
>  struct hv_context {
> @@ -189,8 +188,6 @@ extern int hv_post_message(union hv_connection_id connection_id,
>  
>  extern int hv_synic_cleanup(unsigned int cpu);
>  
> -extern void hv_synic_clockevents_cleanup(void);
> -
>  /* Interface */
>  
>  
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 000b53e..8e442c5 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -43,6 +43,7 @@
>  #include <linux/kdebug.h>
>  #include <linux/efi.h>
>  #include <linux/random.h>
> +#include <clocksource/hyperv_syntimer.h>
>  #include "hyperv_vmbus.h"
>  
>  struct vmbus_dynid {
> @@ -939,17 +940,6 @@ static void vmbus_onmessage_work(struct work_struct *work)
>  	kfree(ctx);
>  }
>  
> -static void hv_process_timer_expiration(struct hv_message *msg,
> -					struct hv_per_cpu_context *hv_cpu)
> -{
> -	struct clock_event_device *dev = hv_cpu->clk_evt;
> -
> -	if (dev->event_handler)
> -		dev->event_handler(dev);
> -
> -	vmbus_signal_eom(msg, HVMSG_TIMER_EXPIRED);
> -}
> -
>  void vmbus_on_msg_dpc(unsigned long data)
>  {
>  	struct hv_per_cpu_context *hv_cpu = (void *)data;
> @@ -1143,9 +1133,10 @@ static void vmbus_isr(void)
>  
>  	/* Check if there are actual msgs to be processed */
>  	if (msg->header.message_type != HVMSG_NONE) {
> -		if (msg->header.message_type == HVMSG_TIMER_EXPIRED)
> -			hv_process_timer_expiration(msg, hv_cpu);
> -		else
> +		if (msg->header.message_type == HVMSG_TIMER_EXPIRED) {
> +			hv_stimer0_isr();
> +			vmbus_signal_eom(msg, HVMSG_TIMER_EXPIRED);
> +		} else
>  			tasklet_schedule(&hv_cpu->msg_dpc);
>  	}
>  
> @@ -1248,8 +1239,8 @@ static int vmbus_bus_init(void)
>  	if (ret)
>  		goto err_alloc;
>  	/*
> -	 * Initialize the per-cpu interrupt state and
> -	 * connect to the host.
> +	 * Initialize the per-cpu interrupt state and syntimer state.
> +	 * Then connect to the host.
>  	 */
>  	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "hyperv/vmbus:online",
>  				hv_synic_init, hv_synic_cleanup);
> @@ -1257,6 +1248,10 @@ static int vmbus_bus_init(void)
>  		goto err_alloc;
>  	hyperv_cpuhp_online = ret;
>  
> +	ret = hv_syntimer_alloc(VMBUS_MESSAGE_SINT);
> +	if (ret < 0)
> +		goto err_syntimer_alloc;
> +
>  	ret = vmbus_connect();
>  	if (ret)
>  		goto err_connect;
> @@ -1301,6 +1296,8 @@ static int vmbus_bus_init(void)
>  	return 0;
>  
>  err_connect:
> +	hv_syntimer_free();
> +err_syntimer_alloc:
>  	cpuhp_remove_state(hyperv_cpuhp_online);
>  err_alloc:
>  	hv_synic_free();
> @@ -1971,7 +1968,7 @@ static int vmbus_acpi_add(struct acpi_device *device)
>  
>  static void hv_kexec_handler(void)
>  {
> -	hv_synic_clockevents_cleanup();
> +	hv_syntimer_global_cleanup();
>  	vmbus_initiate_unload(false);
>  	vmbus_connection.conn_state = DISCONNECTED;
>  	/* Make sure conn_state is set as hv_synic_cleanup checks for it */
> @@ -1982,6 +1979,8 @@ static void hv_kexec_handler(void)
>  
>  static void hv_crash_handler(struct pt_regs *regs)
>  {
> +	int cpu;
> +
>  	vmbus_initiate_unload(true);
>  	/*
>  	 * In crash handler we can't schedule synic cleanup for all CPUs,
> @@ -1989,7 +1988,9 @@ static void hv_crash_handler(struct pt_regs *regs)
>  	 * for kdump.
>  	 */
>  	vmbus_connection.conn_state = DISCONNECTED;
> -	hv_synic_cleanup(smp_processor_id());
> +	cpu = smp_processor_id();
> +	hv_syntimer_cleanup(cpu);
> +	hv_synic_cleanup(cpu);
>  	hyperv_cleanup();
>  };
>  
> @@ -2038,7 +2039,7 @@ static void __exit vmbus_exit(void)
>  	hv_remove_kexec_handler();
>  	hv_remove_crash_handler();
>  	vmbus_connection.conn_state = DISCONNECTED;
> -	hv_synic_clockevents_cleanup();
> +	hv_syntimer_global_cleanup();
>  	vmbus_disconnect();
>  	hv_remove_vmbus_irq();
>  	for_each_online_cpu(cpu) {
> diff --git a/include/clocksource/hyperv_syntimer.h b/include/clocksource/hyperv_syntimer.h
> new file mode 100644
> index 0000000..154138b
> --- /dev/null
> +++ b/include/clocksource/hyperv_syntimer.h
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +/*
> + * Definitions for the clocksource provided by the Hyper-V
> + * hypervisor to guest VMs, as described in the Hyper-V Top
> + * Level Functional Spec (TLFS).
> + *
> + * Copyright (C) 2019, Microsoft, Inc.
> + *
> + * Author:  Michael Kelley <mikelley@microsoft.com>
> + */
> +
> +#ifndef __CLKSOURCE_HYPERV_SYNTIMER_H
> +#define __CLKSOURCE_HYPERV_SYNTIMER_H
> +
> +#define HV_MAX_MAX_DELTA_TICKS 0xffffffff
> +#define HV_MIN_DELTA_TICKS 1
> +
> +/* Routines called by the VMbus driver */
> +extern int hv_syntimer_alloc(int sint);
> +extern void hv_syntimer_free(void);
> +extern int hv_syntimer_cleanup(unsigned int cpu);
> +extern void hv_syntimer_global_cleanup(void);
> +extern void hv_stimer0_isr(void);
> +
> +#endif

-- 
Vitaly

RE: [PATCH 1/2] Drivers: hv: Move Hyper-V clockevents code to new clocksource driver

[Michael Kelley]

From: Vitaly Kuznetsov <vkuznets@redhat.com>  Sent: Wednesday, March 13, 2019 1:28 AM
> 
> > Clockevents code for Hyper-V synthetic timers is currently mixed
> > in with other Hyper-V code. Move the code to a Hyper-V specific
> > driver in the "clocksource" directory. Update the VMbus driver
> > to call initialization and cleanup routines since the Hyper-V
> > synthetic timers are not independently enumerated in ACPI.
> >
> 
> I like the idea! Would it also make sense to consider moving Hyper-V
> clocksource from arch/x86/hyperv/hv_init.c? The thing is that we use it
> from arch-independent code (e.g. seee 'hyperv_cs' usage in
> drivers/hv/hv_util.c).

That's what the second patch in the series does. :-)  But let me
know if there's something you think I've missed.

> 
> Nitpicking:
> 
> This has nothing to do with your patch but we have a mix of 'stimer',
> 'timer', 'syntimer' names when we refer to Hyper-V Synthetic timers, it
> may make sense to try to converge on something (stimer would probably be
> my personal preference but I don't really care as long as we use the
> same abbreviation). Examples:
> 
> hv_init_timer_config(...)
> HV_MSR_SYNTIMER_AVAILABLE
> HYPERV_STIMER0_VECTOR
> hv_syntimer_init(...)
> ...

Yes, you are right about the mish-mash of names.  I also like converging
on "stimer".  I'll certainly change things like hv_syntimer_init() to
hv_stimer_init() as part of the patch and avoid making the problem
worse.

What about the name of the new .c and .h files?  They include code
both the stimer-based clockevents, as well as the Hyper-V
reference time source based clocksource.  Stay with "syntimer" or
shorten to "stimer", even though the code is slightly broader than
just the stimers?  (which highlights the generic Linux naming issue that
"clocksource drivers" include code for both clockevents and clocksources)

> 
> > diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> > index be6e0fb..a887955 100644
> > --- a/drivers/clocksource/Makefile
> > +++ b/drivers/clocksource/Makefile
> > @@ -83,3 +83,4 @@ obj-$(CONFIG_ATCPIT100_TIMER)		+= timer-atcpit100.o
> >  obj-$(CONFIG_RISCV_TIMER)		+= timer-riscv.o
> >  obj-$(CONFIG_CSKY_MP_TIMER)		+= timer-mp-csky.o
> >  obj-$(CONFIG_GX6605S_TIMER)		+= timer-gx6605s.o
> > +obj-$(CONFIG_HYPERV)			+= hyperv_syntimer.o
> 
> (just a couple of spare thoughs)
> 
> CONFIG_HYPERV can also be a module, are we OK with that? (we'll have to
> support module loading/unloading then and honestly I see no reason for
> that. I would prefer everything but VMBus devices to be in
> kernel.) If we don't want it to be a module we can create a hidden
> CONFIG_HYPERV_STIMER or something like that - just like we already do
> for CONFIG_HYPERV_TSCPAGE.
> 
> There is, however, one additional dependency here: when running in
> non-direct mode, Hyper-V clockevent devices require functional Hyper-V
> messaging - which currently lives in VMBus code so that may explain why
> you may want to keep stimer code in the same entity. Or, alternatively,
> we can move Hyper-V messaging out of VMBus code (is it actually
> architecture-agnostic?)
> 

I thought about introducing CONFIG_HYPERV_STIMER, but in my
judgment it was just unnecessary complexity.  The Hyper-V clocksource
driver can't exist independent of Hyper-V, and vice versa.  When both the
clocksource and clockevents code is considered, the VMbus driver and
Hyper-V initialization code has to call directly into the driver since the
Hyper-V synthetic timers and reference time counter aren't independently
enumerated.  Even if we could get the Hyper-V messaging out of VMbus
code, we would still need the clocksource initialization call directly from
hyperv_init(), which is not in a module (see the 2nd patch of the series).

Michael

RE: [PATCH 1/2] Drivers: hv: Move Hyper-V clockevents code to new clocksource driver

[Vitaly Kuznetsov]

Michael Kelley <mikelley@microsoft.com> writes:

> From: Vitaly Kuznetsov <vkuznets@redhat.com>  Sent: Wednesday, March 13, 2019 1:28 AM
>> 
>> > Clockevents code for Hyper-V synthetic timers is currently mixed
>> > in with other Hyper-V code. Move the code to a Hyper-V specific
>> > driver in the "clocksource" directory. Update the VMbus driver
>> > to call initialization and cleanup routines since the Hyper-V
>> > synthetic timers are not independently enumerated in ACPI.
>> >
>> 
>> I like the idea! Would it also make sense to consider moving Hyper-V
>> clocksource from arch/x86/hyperv/hv_init.c? The thing is that we use it
>> from arch-independent code (e.g. seee 'hyperv_cs' usage in
>> drivers/hv/hv_util.c).
>
> That's what the second patch in the series does. :-)  But let me
> know if there's something you think I've missed.
>

Oh, sorry, it's just me - your subject lines messed with my brain :-) I
like your idea even more then!

>> 
>> Nitpicking:
>> 
>> This has nothing to do with your patch but we have a mix of 'stimer',
>> 'timer', 'syntimer' names when we refer to Hyper-V Synthetic timers, it
>> may make sense to try to converge on something (stimer would probably be
>> my personal preference but I don't really care as long as we use the
>> same abbreviation). Examples:
>> 
>> hv_init_timer_config(...)
>> HV_MSR_SYNTIMER_AVAILABLE
>> HYPERV_STIMER0_VECTOR
>> hv_syntimer_init(...)
>> ...
>
> Yes, you are right about the mish-mash of names.  I also like converging
> on "stimer".  I'll certainly change things like hv_syntimer_init() to
> hv_stimer_init() as part of the patch and avoid making the problem
> worse.
>
> What about the name of the new .c and .h files?  They include code
> both the stimer-based clockevents, as well as the Hyper-V
> reference time source based clocksource.  Stay with "syntimer" or
> shorten to "stimer", even though the code is slightly broader than
> just the stimers?  (which highlights the generic Linux naming issue that
> "clocksource drivers" include code for both clockevents and
> clocksources)

"hyperv_timers.*" maybe? (those who read TLFS may get confused because
'Synthetic timers' doesn't refer to clocksources - just to clockevents).

>
>> 
>> > diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
>> > index be6e0fb..a887955 100644
>> > --- a/drivers/clocksource/Makefile
>> > +++ b/drivers/clocksource/Makefile
>> > @@ -83,3 +83,4 @@ obj-$(CONFIG_ATCPIT100_TIMER)		+= timer-atcpit100.o
>> >  obj-$(CONFIG_RISCV_TIMER)		+= timer-riscv.o
>> >  obj-$(CONFIG_CSKY_MP_TIMER)		+= timer-mp-csky.o
>> >  obj-$(CONFIG_GX6605S_TIMER)		+= timer-gx6605s.o
>> > +obj-$(CONFIG_HYPERV)			+= hyperv_syntimer.o
>> 
>> (just a couple of spare thoughs)
>> 
>> CONFIG_HYPERV can also be a module, are we OK with that? (we'll have to
>> support module loading/unloading then and honestly I see no reason for
>> that. I would prefer everything but VMBus devices to be in
>> kernel.) If we don't want it to be a module we can create a hidden
>> CONFIG_HYPERV_STIMER or something like that - just like we already do
>> for CONFIG_HYPERV_TSCPAGE.
>> 
>> There is, however, one additional dependency here: when running in
>> non-direct mode, Hyper-V clockevent devices require functional Hyper-V
>> messaging - which currently lives in VMBus code so that may explain why
>> you may want to keep stimer code in the same entity. Or, alternatively,
>> we can move Hyper-V messaging out of VMBus code (is it actually
>> architecture-agnostic?)
>> 
>
> I thought about introducing CONFIG_HYPERV_STIMER, but in my
> judgment it was just unnecessary complexity.  The Hyper-V clocksource
> driver can't exist independent of Hyper-V, and vice versa.  When both the
> clocksource and clockevents code is considered, the VMbus driver and
> Hyper-V initialization code has to call directly into the driver since the
> Hyper-V synthetic timers and reference time counter aren't independently
> enumerated.  Even if we could get the Hyper-V messaging out of VMbus
> code, we would still need the clocksource initialization call directly from
> hyperv_init(), which is not in a module (see the 2nd patch of the series).
>

Right, so hv_init_clocksource() cannot live in hv_vmbus module and we
need to somehow prevent hyperv_syntimer.o from going in there. And

+obj-$(CONFIG_HYPERV)                      += hyperv_syntimer.o

will do exactly the opposite - put it in hv_vmbus module. Or am I
missing something? (I haven't tried to build your code yet, sorry).

> Michael

-- 
Vitaly

RE: [PATCH 1/2] Drivers: hv: Move Hyper-V clockevents code to new clocksource driver

[Michael Kelley]

From: Vitaly Kuznetsov <vkuznets@redhat.com> Sent: Wednesday, March 13, 2019 7:23 AM

> >> > diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> >> > index be6e0fb..a887955 100644
> >> > --- a/drivers/clocksource/Makefile
> >> > +++ b/drivers/clocksource/Makefile
> >> > @@ -83,3 +83,4 @@ obj-$(CONFIG_ATCPIT100_TIMER)		+= timer-atcpit100.o
> >> >  obj-$(CONFIG_RISCV_TIMER)		+= timer-riscv.o
> >> >  obj-$(CONFIG_CSKY_MP_TIMER)		+= timer-mp-csky.o
> >> >  obj-$(CONFIG_GX6605S_TIMER)		+= timer-gx6605s.o
> >> > +obj-$(CONFIG_HYPERV)			+= hyperv_syntimer.o
> >>
> >> (just a couple of spare thoughs)
> >>
> >> CONFIG_HYPERV can also be a module, are we OK with that? (we'll have to
> >> support module loading/unloading then and honestly I see no reason for
> >> that. I would prefer everything but VMBus devices to be in
> >> kernel.) If we don't want it to be a module we can create a hidden
> >> CONFIG_HYPERV_STIMER or something like that - just like we already do
> >> for CONFIG_HYPERV_TSCPAGE.
> >>
> >> There is, however, one additional dependency here: when running in
> >> non-direct mode, Hyper-V clockevent devices require functional Hyper-V
> >> messaging - which currently lives in VMBus code so that may explain why
> >> you may want to keep stimer code in the same entity. Or, alternatively,
> >> we can move Hyper-V messaging out of VMBus code (is it actually
> >> architecture-agnostic?)
> >>
> >
> > I thought about introducing CONFIG_HYPERV_STIMER, but in my
> > judgment it was just unnecessary complexity.  The Hyper-V clocksource
> > driver can't exist independent of Hyper-V, and vice versa.  When both the
> > clocksource and clockevents code is considered, the VMbus driver and
> > Hyper-V initialization code has to call directly into the driver since the
> > Hyper-V synthetic timers and reference time counter aren't independently
> > enumerated.  Even if we could get the Hyper-V messaging out of VMbus
> > code, we would still need the clocksource initialization call directly from
> > hyperv_init(), which is not in a module (see the 2nd patch of the series).
> >
> 
> Right, so hv_init_clocksource() cannot live in hv_vmbus module and we
> need to somehow prevent hyperv_syntimer.o from going in there. And
> 
> +obj-$(CONFIG_HYPERV)                      += hyperv_syntimer.o
> 
> will do exactly the opposite - put it in hv_vmbus module. Or am I
> missing something? (I haven't tried to build your code yet, sorry).
> 

That line just controls whether hyperv_syntimer.o is built.  It doesn't put
it in the hv_vmbus module.  All of the clocksource .o files that are built go
into the kernel, not in a module.  But thinking about it more, the above works
correctly when CONFIG_HYPERV=y, but not when CONFIG_HYPERV=m.  I'll
have to introduce CONFIG_HYPERV_TIMER after all.  Will fix this in v2.  Thanks
for the discussion!
 
Michael

RE: [PATCH 1/2] Drivers: hv: Move Hyper-V clockevents code to new clocksource driver

[Vitaly Kuznetsov]

Michael Kelley <mikelley@microsoft.com> writes:

> From: Vitaly Kuznetsov <vkuznets@redhat.com> Sent: Wednesday, March 13, 2019 7:23 AM
>
>> >> > diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
>> >> > index be6e0fb..a887955 100644
>> >> > --- a/drivers/clocksource/Makefile
>> >> > +++ b/drivers/clocksource/Makefile
>> >> > @@ -83,3 +83,4 @@ obj-$(CONFIG_ATCPIT100_TIMER)		+= timer-atcpit100.o
>> >> >  obj-$(CONFIG_RISCV_TIMER)		+= timer-riscv.o
>> >> >  obj-$(CONFIG_CSKY_MP_TIMER)		+= timer-mp-csky.o
>> >> >  obj-$(CONFIG_GX6605S_TIMER)		+= timer-gx6605s.o
>> >> > +obj-$(CONFIG_HYPERV)			+= hyperv_syntimer.o
>> >>
>> >> (just a couple of spare thoughs)
>> >>
>> >> CONFIG_HYPERV can also be a module, are we OK with that? (we'll have to
>> >> support module loading/unloading then and honestly I see no reason for
>> >> that. I would prefer everything but VMBus devices to be in
>> >> kernel.) If we don't want it to be a module we can create a hidden
>> >> CONFIG_HYPERV_STIMER or something like that - just like we already do
>> >> for CONFIG_HYPERV_TSCPAGE.
>> >>
>> >> There is, however, one additional dependency here: when running in
>> >> non-direct mode, Hyper-V clockevent devices require functional Hyper-V
>> >> messaging - which currently lives in VMBus code so that may explain why
>> >> you may want to keep stimer code in the same entity. Or, alternatively,
>> >> we can move Hyper-V messaging out of VMBus code (is it actually
>> >> architecture-agnostic?)
>> >>
>> >
>> > I thought about introducing CONFIG_HYPERV_STIMER, but in my
>> > judgment it was just unnecessary complexity.  The Hyper-V clocksource
>> > driver can't exist independent of Hyper-V, and vice versa.  When both the
>> > clocksource and clockevents code is considered, the VMbus driver and
>> > Hyper-V initialization code has to call directly into the driver since the
>> > Hyper-V synthetic timers and reference time counter aren't independently
>> > enumerated.  Even if we could get the Hyper-V messaging out of VMbus
>> > code, we would still need the clocksource initialization call directly from
>> > hyperv_init(), which is not in a module (see the 2nd patch of the series).
>> >
>> 
>> Right, so hv_init_clocksource() cannot live in hv_vmbus module and we
>> need to somehow prevent hyperv_syntimer.o from going in there. And
>> 
>> +obj-$(CONFIG_HYPERV)                      += hyperv_syntimer.o
>> 
>> will do exactly the opposite - put it in hv_vmbus module. Or am I
>> missing something? (I haven't tried to build your code yet, sorry).
>> 
>
> That line just controls whether hyperv_syntimer.o is built.  It doesn't put
> it in the hv_vmbus module.  All of the clocksource .o files that are built go
> into the kernel, not in a module.  But thinking about it more, the above works
> correctly when CONFIG_HYPERV=y, but not when CONFIG_HYPERV=m.

Yes, that's what I meant.

> I'll have to introduce CONFIG_HYPERV_TIMER after all.  Will fix this in v2.  Thanks
> for the discussion!

Thanks!

-- 
Vitaly

[PATCH 2/2] Drivers: hv: Move Hyper-V clocksource code to new clocksource driver

[Michael Kelley]

Code for the Hyper-V specific clocksources is currently mixed
in with other Hyper-V code. Move the code to a Hyper-V specific
driver in the "clocksource" directory, while separating out
ISA dependencies so that the new clocksource driver is ISA
independent. Update the Hyper-V initialization code to call
initialization and cleanup routines since the Hyper-V synthetic
timers are not independently enumerated in ACPI. Update Hyper-V
clocksource users KVM and VDSO to get definitions from a new
include file.

No behavior is changed and no new functionality is added.

Signed-off-by: Michael Kelley <mikelley@microsoft.com>
---
 arch/x86/entry/vdso/vclock_gettime.c  |   1 +
 arch/x86/entry/vdso/vma.c             |   2 +-
 arch/x86/hyperv/hv_init.c             |  91 ++-----------------------
 arch/x86/include/asm/mshyperv.h       |  80 +++-------------------
 arch/x86/kvm/x86.c                    |   1 +
 drivers/clocksource/hyperv_syntimer.c | 122 ++++++++++++++++++++++++++++++++++
 include/clocksource/hyperv_syntimer.h |  78 ++++++++++++++++++++++
 7 files changed, 219 insertions(+), 156 deletions(-)

diff --git a/arch/x86/entry/vdso/vclock_gettime.c b/arch/x86/entry/vdso/vclock_gettime.c
index 007b3fe9..b0de2a2 100644
--- a/arch/x86/entry/vdso/vclock_gettime.c
+++ b/arch/x86/entry/vdso/vclock_gettime.c
@@ -21,6 +21,7 @@
 #include <linux/math64.h>
 #include <linux/time.h>
 #include <linux/kernel.h>
+#include <clocksource/hyperv_syntimer.h>
 
 #define gtod (&VVAR(vsyscall_gtod_data))
 
diff --git a/arch/x86/entry/vdso/vma.c b/arch/x86/entry/vdso/vma.c
index babc4e7..8b81c91 100644
--- a/arch/x86/entry/vdso/vma.c
+++ b/arch/x86/entry/vdso/vma.c
@@ -22,7 +22,7 @@
 #include <asm/page.h>
 #include <asm/desc.h>
 #include <asm/cpufeature.h>
-#include <asm/mshyperv.h>
+#include <clocksource/hyperv_syntimer.h>
 
 #if defined(CONFIG_X86_64)
 unsigned int __read_mostly vdso64_enabled = 1;
diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
index 7abb09e..eb80f65 100644
--- a/arch/x86/hyperv/hv_init.c
+++ b/arch/x86/hyperv/hv_init.c
@@ -27,64 +27,13 @@
 #include <linux/version.h>
 #include <linux/vmalloc.h>
 #include <linux/mm.h>
-#include <linux/clockchips.h>
 #include <linux/hyperv.h>
 #include <linux/slab.h>
 #include <linux/cpuhotplug.h>
-
-#ifdef CONFIG_HYPERV_TSCPAGE
-
-static struct ms_hyperv_tsc_page *tsc_pg;
-
-struct ms_hyperv_tsc_page *hv_get_tsc_page(void)
-{
-	return tsc_pg;
-}
-EXPORT_SYMBOL_GPL(hv_get_tsc_page);
-
-static u64 read_hv_clock_tsc(struct clocksource *arg)
-{
-	u64 current_tick = hv_read_tsc_page(tsc_pg);
-
-	if (current_tick == U64_MAX)
-		rdmsrl(HV_X64_MSR_TIME_REF_COUNT, current_tick);
-
-	return current_tick;
-}
-
-static struct clocksource hyperv_cs_tsc = {
-		.name		= "hyperv_clocksource_tsc_page",
-		.rating		= 400,
-		.read		= read_hv_clock_tsc,
-		.mask		= CLOCKSOURCE_MASK(64),
-		.flags		= CLOCK_SOURCE_IS_CONTINUOUS,
-};
-#endif
-
-static u64 read_hv_clock_msr(struct clocksource *arg)
-{
-	u64 current_tick;
-	/*
-	 * Read the partition counter to get the current tick count. This count
-	 * is set to 0 when the partition is created and is incremented in
-	 * 100 nanosecond units.
-	 */
-	rdmsrl(HV_X64_MSR_TIME_REF_COUNT, current_tick);
-	return current_tick;
-}
-
-static struct clocksource hyperv_cs_msr = {
-	.name		= "hyperv_clocksource_msr",
-	.rating		= 400,
-	.read		= read_hv_clock_msr,
-	.mask		= CLOCKSOURCE_MASK(64),
-	.flags		= CLOCK_SOURCE_IS_CONTINUOUS,
-};
+#include <clocksource/hyperv_syntimer.h>
 
 void *hv_hypercall_pg;
 EXPORT_SYMBOL_GPL(hv_hypercall_pg);
-struct clocksource *hyperv_cs;
-EXPORT_SYMBOL_GPL(hyperv_cs);
 
 u32 *hv_vp_index;
 EXPORT_SYMBOL_GPL(hv_vp_index);
@@ -349,41 +298,11 @@ void __init hyperv_init(void)
 	x86_init.pci.arch_init = hv_pci_init;
 
 	/*
-	 * Register Hyper-V specific clocksource.
+	 * Register Hyper-V specific clocksource. Pass 'false' as the
+	 * arguemnt, indicating to not register the clocksource as the
+	 * sched clock.
 	 */
-#ifdef CONFIG_HYPERV_TSCPAGE
-	if (ms_hyperv.features & HV_MSR_REFERENCE_TSC_AVAILABLE) {
-		union hv_x64_msr_hypercall_contents tsc_msr;
-
-		tsc_pg = __vmalloc(PAGE_SIZE, GFP_KERNEL, PAGE_KERNEL);
-		if (!tsc_pg)
-			goto register_msr_cs;
-
-		hyperv_cs = &hyperv_cs_tsc;
-
-		rdmsrl(HV_X64_MSR_REFERENCE_TSC, tsc_msr.as_uint64);
-
-		tsc_msr.enable = 1;
-		tsc_msr.guest_physical_address = vmalloc_to_pfn(tsc_pg);
-
-		wrmsrl(HV_X64_MSR_REFERENCE_TSC, tsc_msr.as_uint64);
-
-		hyperv_cs_tsc.archdata.vclock_mode = VCLOCK_HVCLOCK;
-
-		clocksource_register_hz(&hyperv_cs_tsc, NSEC_PER_SEC/100);
-		return;
-	}
-register_msr_cs:
-#endif
-	/*
-	 * For 32 bit guests just use the MSR based mechanism for reading
-	 * the partition counter.
-	 */
-
-	hyperv_cs = &hyperv_cs_msr;
-	if (ms_hyperv.features & HV_MSR_TIME_REF_COUNT_AVAILABLE)
-		clocksource_register_hz(&hyperv_cs_msr, NSEC_PER_SEC/100);
-
+	hv_init_clocksource(false);
 	return;
 
 remove_cpuhp_state:
diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
index cc60e61..9326689 100644
--- a/arch/x86/include/asm/mshyperv.h
+++ b/arch/x86/include/asm/mshyperv.h
@@ -105,6 +105,17 @@ static inline void vmbus_signal_eom(struct hv_message *msg, u32 old_msg_type)
 #define hv_get_crash_ctl(val) \
 	rdmsrl(HV_X64_MSR_CRASH_CTL, val)
 
+#define hv_get_time_ref_count(val) \
+	rdmsrl(HV_X64_MSR_TIME_REF_COUNT, val)
+
+#define hv_get_reference_tsc(val) \
+	rdmsrl(HV_X64_MSR_REFERENCE_TSC, val)
+#define hv_set_reference_tsc(val) \
+	wrmsrl(HV_X64_MSR_REFERENCE_TSC, val)
+#define hv_set_clocksource_vdso(val) \
+	((val).archdata.vclock_mode = VCLOCK_HVCLOCK)
+#define hv_get_raw_timer() rdtsc_ordered()
+
 void hyperv_callback_vector(void);
 void hyperv_reenlightenment_vector(void);
 #ifdef CONFIG_TRACING
@@ -387,73 +398,4 @@ static inline int hyperv_flush_guest_mapping_range(u64 as,
 }
 #endif /* CONFIG_HYPERV */
 
-#ifdef CONFIG_HYPERV_TSCPAGE
-struct ms_hyperv_tsc_page *hv_get_tsc_page(void);
-static inline u64 hv_read_tsc_page_tsc(const struct ms_hyperv_tsc_page *tsc_pg,
-				       u64 *cur_tsc)
-{
-	u64 scale, offset;
-	u32 sequence;
-
-	/*
-	 * The protocol for reading Hyper-V TSC page is specified in Hypervisor
-	 * Top-Level Functional Specification ver. 3.0 and above. To get the
-	 * reference time we must do the following:
-	 * - READ ReferenceTscSequence
-	 *   A special '0' value indicates the time source is unreliable and we
-	 *   need to use something else. The currently published specification
-	 *   versions (up to 4.0b) contain a mistake and wrongly claim '-1'
-	 *   instead of '0' as the special value, see commit c35b82ef0294.
-	 * - ReferenceTime =
-	 *        ((RDTSC() * ReferenceTscScale) >> 64) + ReferenceTscOffset
-	 * - READ ReferenceTscSequence again. In case its value has changed
-	 *   since our first reading we need to discard ReferenceTime and repeat
-	 *   the whole sequence as the hypervisor was updating the page in
-	 *   between.
-	 */
-	do {
-		sequence = READ_ONCE(tsc_pg->tsc_sequence);
-		if (!sequence)
-			return U64_MAX;
-		/*
-		 * Make sure we read sequence before we read other values from
-		 * TSC page.
-		 */
-		smp_rmb();
-
-		scale = READ_ONCE(tsc_pg->tsc_scale);
-		offset = READ_ONCE(tsc_pg->tsc_offset);
-		*cur_tsc = rdtsc_ordered();
-
-		/*
-		 * Make sure we read sequence after we read all other values
-		 * from TSC page.
-		 */
-		smp_rmb();
-
-	} while (READ_ONCE(tsc_pg->tsc_sequence) != sequence);
-
-	return mul_u64_u64_shr(*cur_tsc, scale, 64) + offset;
-}
-
-static inline u64 hv_read_tsc_page(const struct ms_hyperv_tsc_page *tsc_pg)
-{
-	u64 cur_tsc;
-
-	return hv_read_tsc_page_tsc(tsc_pg, &cur_tsc);
-}
-
-#else
-static inline struct ms_hyperv_tsc_page *hv_get_tsc_page(void)
-{
-	return NULL;
-}
-
-static inline u64 hv_read_tsc_page_tsc(const struct ms_hyperv_tsc_page *tsc_pg,
-				       u64 *cur_tsc)
-{
-	BUG();
-	return U64_MAX;
-}
-#endif
 #endif
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 941f932..4f83437 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -70,6 +70,7 @@
 #include <asm/mshyperv.h>
 #include <asm/hypervisor.h>
 #include <asm/intel_pt.h>
+#include <clocksource/hyperv_syntimer.h>
 
 #define CREATE_TRACE_POINTS
 #include "trace.h"
diff --git a/drivers/clocksource/hyperv_syntimer.c b/drivers/clocksource/hyperv_syntimer.c
index 7276308..57e10bf 100644
--- a/drivers/clocksource/hyperv_syntimer.c
+++ b/drivers/clocksource/hyperv_syntimer.c
@@ -14,6 +14,8 @@
 #include <linux/percpu.h>
 #include <linux/cpumask.h>
 #include <linux/clockchips.h>
+#include <linux/clocksource.h>
+#include <linux/sched_clock.h>
 #include <linux/cpuhotplug.h>
 #include <linux/mm.h>
 #include <clocksource/hyperv_syntimer.h>
@@ -204,3 +206,123 @@ void hv_syntimer_global_cleanup(void)
 	hv_syntimer_free();
 }
 EXPORT_SYMBOL_GPL(hv_syntimer_global_cleanup);
+
+/*
+ * Code and definitions for the Hyper-V clocksources.  Two
+ * clocksources are defined: one that reads the Hyper-V defined MSR, and
+ * the other that uses the TSC reference page feature as defined in the
+ * TLFS.  The MSR version is for compatibility with old versions of
+ * Hyper-V and 32-bit x86.  The TSC reference page version is preferred.
+ */
+
+struct clocksource *hyperv_cs;
+EXPORT_SYMBOL_GPL(hyperv_cs);
+
+#ifdef CONFIG_HYPERV_TSCPAGE
+
+static struct ms_hyperv_tsc_page *tsc_pg;
+
+struct ms_hyperv_tsc_page *hv_get_tsc_page(void)
+{
+	return tsc_pg;
+}
+EXPORT_SYMBOL_GPL(hv_get_tsc_page);
+
+static u64 read_hv_sched_clock_tsc(void)
+{
+	u64 current_tick = hv_read_tsc_page(tsc_pg);
+
+	if (current_tick == U64_MAX)
+		hv_get_time_ref_count(current_tick);
+
+	return current_tick;
+}
+
+static u64 read_hv_clock_tsc(struct clocksource *arg)
+{
+	return read_hv_sched_clock_tsc();
+}
+
+static struct clocksource hyperv_cs_tsc = {
+		.name		= "hyperv_clocksource_tsc_page",
+		.rating		= 400,
+		.read		= read_hv_clock_tsc,
+		.mask		= CLOCKSOURCE_MASK(64),
+		.flags		= CLOCK_SOURCE_IS_CONTINUOUS,
+};
+#endif
+
+static u64 read_hv_sched_clock_msr(void)
+{
+	u64 current_tick;
+	/*
+	 * Read the partition counter to get the current tick count. This count
+	 * is set to 0 when the partition is created and is incremented in
+	 * 100 nanosecond units.
+	 */
+	hv_get_time_ref_count(current_tick);
+	return current_tick;
+}
+
+static u64 read_hv_clock_msr(struct clocksource *arg)
+{
+	return read_hv_sched_clock_msr();
+}
+
+static struct clocksource hyperv_cs_msr = {
+	.name		= "hyperv_clocksource_msr",
+	.rating		= 400,
+	.read		= read_hv_clock_msr,
+	.mask		= CLOCKSOURCE_MASK(64),
+	.flags		= CLOCK_SOURCE_IS_CONTINUOUS,
+};
+
+void __init hv_init_clocksource(bool reg_sched_clock)
+{
+#ifdef CONFIG_HYPERV_TSCPAGE
+	if (ms_hyperv.features & HV_MSR_REFERENCE_TSC_AVAILABLE) {
+
+		u64		tsc_msr;
+		phys_addr_t	phys_addr;
+
+		tsc_pg = vmalloc(PAGE_SIZE);
+		if (!tsc_pg)
+			goto register_msr_cs;
+
+		hyperv_cs = &hyperv_cs_tsc;
+		phys_addr = page_to_phys(vmalloc_to_page(tsc_pg));
+
+		/* The Hyper-V TLFS specifies to preserve the value of
+		 * reserved bits in registers.  So read the existing
+		 * value, preserve the low order 12 bits, and add in
+		 * the guest physical address (which already has at
+		 * least the low 12 bits set to zero since it is page
+		 * aligned). Also set the "enable" bit, which is bit 0.
+		 */
+		hv_get_reference_tsc(tsc_msr);
+		tsc_msr &= GENMASK_ULL(11, 0);
+		tsc_msr = tsc_msr | 0x1 | (u64)phys_addr;
+		hv_set_reference_tsc(tsc_msr);
+
+		hv_set_clocksource_vdso(hyperv_cs_tsc);
+		clocksource_register_hz(&hyperv_cs_tsc, NSEC_PER_SEC/100);
+		if (reg_sched_clock)
+			sched_clock_register(read_hv_sched_clock_tsc,
+						64, HV_CLOCK_HZ);
+		return;
+	}
+register_msr_cs:
+#endif
+	/*
+	 * For 32 bit guests just use the MSR based mechanism for reading
+	 * the partition counter.
+	 */
+	hyperv_cs = &hyperv_cs_msr;
+	if (ms_hyperv.features & HV_MSR_TIME_REF_COUNT_AVAILABLE) {
+		clocksource_register_hz(&hyperv_cs_msr, NSEC_PER_SEC/100);
+		if (reg_sched_clock)
+			sched_clock_register(read_hv_sched_clock_msr,
+						64, HV_CLOCK_HZ);
+	}
+}
+EXPORT_SYMBOL_GPL(hv_init_clocksource);
diff --git a/include/clocksource/hyperv_syntimer.h b/include/clocksource/hyperv_syntimer.h
index 154138b..8ce351b 100644
--- a/include/clocksource/hyperv_syntimer.h
+++ b/include/clocksource/hyperv_syntimer.h
@@ -13,6 +13,10 @@
 #ifndef __CLKSOURCE_HYPERV_SYNTIMER_H
 #define __CLKSOURCE_HYPERV_SYNTIMER_H
 
+#include <linux/clocksource.h>
+#include <linux/math64.h>
+#include <asm/mshyperv.h>
+
 #define HV_MAX_MAX_DELTA_TICKS 0xffffffff
 #define HV_MIN_DELTA_TICKS 1
 
@@ -23,4 +27,78 @@
 extern void hv_syntimer_global_cleanup(void);
 extern void hv_stimer0_isr(void);
 
+#ifdef CONFIG_HYPERV
+extern struct clocksource *hyperv_cs;
+extern void hv_init_clocksource(bool reg_sched_clock);
+#endif /* CONFIG_HYPERV */
+
+#ifdef CONFIG_HYPERV_TSCPAGE
+extern struct ms_hyperv_tsc_page *hv_get_tsc_page(void);
+static inline u64 hv_read_tsc_page_tsc(const struct ms_hyperv_tsc_page *tsc_pg,
+				       u64 *cur_tsc)
+{
+	u64 scale, offset;
+	u32 sequence;
+
+	/*
+	 * The protocol for reading Hyper-V TSC page is specified in Hypervisor
+	 * Top-Level Functional Specification ver. 3.0 and above. To get the
+	 * reference time we must do the following:
+	 * - READ ReferenceTscSequence
+	 *   A special '0' value indicates the time source is unreliable and we
+	 *   need to use something else. The currently published specification
+	 *   versions (up to 4.0b) contain a mistake and wrongly claim '-1'
+	 *   instead of '0' as the special value, see commit c35b82ef0294.
+	 * - ReferenceTime =
+	 *        ((RDTSC() * ReferenceTscScale) >> 64) + ReferenceTscOffset
+	 * - READ ReferenceTscSequence again. In case its value has changed
+	 *   since our first reading we need to discard ReferenceTime and repeat
+	 *   the whole sequence as the hypervisor was updating the page in
+	 *   between.
+	 */
+	do {
+		sequence = READ_ONCE(tsc_pg->tsc_sequence);
+		if (!sequence)
+			return U64_MAX;
+		/*
+		 * Make sure we read sequence before we read other values from
+		 * TSC page.
+		 */
+		smp_rmb();
+
+		scale = READ_ONCE(tsc_pg->tsc_scale);
+		offset = READ_ONCE(tsc_pg->tsc_offset);
+		*cur_tsc = hv_get_raw_timer();
+
+		/*
+		 * Make sure we read sequence after we read all other values
+		 * from TSC page.
+		 */
+		smp_rmb();
+
+	} while (READ_ONCE(tsc_pg->tsc_sequence) != sequence);
+
+	return mul_u64_u64_shr(*cur_tsc, scale, 64) + offset;
+}
+
+static inline u64 hv_read_tsc_page(const struct ms_hyperv_tsc_page *tsc_pg)
+{
+	u64 cur_tsc;
+
+	return hv_read_tsc_page_tsc(tsc_pg, &cur_tsc);
+}
+
+#else /* CONFIG_HYPERV_TSC_PAGE */
+static inline struct ms_hyperv_tsc_page *hv_get_tsc_page(void)
+{
+	return NULL;
+}
+
+static inline u64 hv_read_tsc_page_tsc(const struct ms_hyperv_tsc_page *tsc_pg,
+				       u64 *cur_tsc)
+{
+	return U64_MAX;
+}
+#endif /* CONFIG_HYPERV_TSCPAGE */
+
 #endif
-- 
1.8.3.1

Re: [PATCH 0/2] Drivers: hv: Move Hyper-V clock/timer code to separate clocksource driver

[gregkh]

On Tue, Mar 12, 2019 at 09:42:09PM +0000, Michael Kelley wrote:
> This patch series moves Hyper-V clock/timer code to a separate Hyper-V
> clocksource driver. Previously, Hyper-V clock/timer code and data
> structures were mixed in with other Hyper-V code in the ISA independent
> drivers/hv code as well as in arch dependent code. The new Hyper-V
> clocksource driver is ISA independent, with a just few dependencies on
> arch specific functions. The patch series does not change any behavior
> or functionality -- it only reorganizes the existing code and fixes up
> the linkages. A few places outside of Hyper-V code are fixed up to use
> the new #include file structure.
> 
> This restructuring is in response to Marc Zyngier's review comments
> on supporting Hyper-V running on ARM64, and is a good idea in general.
> It increases the amount of code shared between the x86 and ARM64
> architectures, and reduces the size of the new code for supporting
> Hyper-V on ARM64. A new version of the Hyper-V on ARM64 patches will
> follow once this clocksource restructuring is accepted.
> 
> The code is currently diff'ed against Linux 5.0.  I'll rebase
> to linux-next once 5.1-rc1 is available.
> 
> Michael Kelley (2):
>   Drivers: hv: Move Hyper-V clockevents code to new clocksource driver
>   Drivers: hv: Move Hyper-V clocksource code to new clocksource driver

You have two different patches that do different things, yet have the
same identical shortlog text :(

That's not ok, and something that I reject for trivial patches, it
should never happen for a "real" patch as you don't do the same thing in
both of these patches.

thanks,

greg k-h

RE: [PATCH 0/2] Drivers: hv: Move Hyper-V clock/timer code to separate clocksource driver

[Michael Kelley]

From: gregkh@linuxfoundation.org <gregkh@linuxfoundation.org>  Sent: Tuesday, March 12, 2019 2:47 PM
> >
> > Michael Kelley (2):
> >   Drivers: hv: Move Hyper-V clockevents code to new clocksource driver
> >   Drivers: hv: Move Hyper-V clocksource code to new clocksource driver
> 
> You have two different patches that do different things, yet have the
> same identical shortlog text :(
> 
> That's not ok, and something that I reject for trivial patches, it
> should never happen for a "real" patch as you don't do the same thing in
> both of these patches.

Hmmm.  Not identical.  The first patch is "clockevents" code, and the second
patch is "clocksource" code.

Michael

Re: [PATCH 0/2] Drivers: hv: Move Hyper-V clock/timer code to separate clocksource driver

[gregkh]

On Tue, Mar 12, 2019 at 09:53:28PM +0000, Michael Kelley wrote:
> From: gregkh@linuxfoundation.org <gregkh@linuxfoundation.org>  Sent: Tuesday, March 12, 2019 2:47 PM
> > >
> > > Michael Kelley (2):
> > >   Drivers: hv: Move Hyper-V clockevents code to new clocksource driver
> > >   Drivers: hv: Move Hyper-V clocksource code to new clocksource driver
> > 
> > You have two different patches that do different things, yet have the
> > same identical shortlog text :(
> > 
> > That's not ok, and something that I reject for trivial patches, it
> > should never happen for a "real" patch as you don't do the same thing in
> > both of these patches.
> 
> Hmmm.  Not identical.  The first patch is "clockevents" code, and the second
> patch is "clocksource" code.

Wow, that's not obvious at all, sorry.  You still might want to make it
a bit more different :)

greg k-h