[PATCH 0/7] Enhance the hv_vmbus driver to support hibernation

[PATCH 0/7] Enhance the hv_vmbus driver to support hibernation

[Dexuan Cui]

Hi,
This is the first patchset to enable hibernation when Linux VM runs on Hyper-V.

A second patchset to enhance the high-level VSC drivers (hv_netvsc,
hv_storvsc, etc.) for hibernation will be posted later. The second patchset
depends on this first patchset, so I hope this pathset can be accepted soon.

The changes in this patchset are mostly straightforward new code that only
runs when the VM enters hibernation, so IMHO it's pretty safe to have this
patchset, because the hibernation feature never worked for Linux VM running
on Hyper-V.

The patchset is rebased on Linus's tree today.

Please review.

Thanks,
Dexuan

Dexuan Cui (7):
  x86/hyper-v: Suspend/resume the hypercall page for hibernation
  clocksource/drivers: Suspend/resume Hyper-V clocksource for
    hibernation
  Drivers: hv: vmbus: Split hv_synic_init/cleanup into regs and timer
    settings
  Drivers: hv: vmbus: Suspend/resume the synic for hibernation
  Drivers: hv: vmbus: Ignore the offers when resuming from hibernation
  Drivers: hv: vmbus: Suspend/resume the vmbus itself for hibernation
  Drivers: hv: vmbus: Implement suspend/resume for VSC drivers for
    hibernation

 arch/x86/hyperv/hv_init.c          |  34 +++++++++++
 drivers/clocksource/hyperv_timer.c |  25 ++++++++
 drivers/hv/channel_mgmt.c          |  28 ++++++++-
 drivers/hv/connection.c            |   3 +-
 drivers/hv/hv.c                    |  66 +++++++++++---------
 drivers/hv/hyperv_vmbus.h          |   4 ++
 drivers/hv/vmbus_drv.c             | 122 +++++++++++++++++++++++++++++++++++++
 include/linux/hyperv.h             |   3 +
 8 files changed, 253 insertions(+), 32 deletions(-)

-- 
1.8.3.1

[PATCH 1/7] x86/hyper-v: Suspend/resume the hypercall page for hibernation

[Dexuan Cui]

This is needed for hibernation, e.g. when we resume the old kernel, we need
to disable the "current" kernel's hypercall page and then resume the old
kernel's.

Signed-off-by: Dexuan Cui <decui@microsoft.com>
---
 arch/x86/hyperv/hv_init.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
index 0e033ef..3005871 100644
--- a/arch/x86/hyperv/hv_init.c
+++ b/arch/x86/hyperv/hv_init.c
@@ -20,6 +20,7 @@
 #include <linux/hyperv.h>
 #include <linux/slab.h>
 #include <linux/cpuhotplug.h>
+#include <linux/syscore_ops.h>
 #include <clocksource/hyperv_timer.h>
 
 void *hv_hypercall_pg;
@@ -214,6 +215,34 @@ static int __init hv_pci_init(void)
 	return 1;
 }
 
+static int hv_suspend(void)
+{
+	union hv_x64_msr_hypercall_contents hypercall_msr;
+
+	/* Reset the hypercall page */
+	rdmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
+	hypercall_msr.enable = 0;
+	wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
+
+	return 0;
+}
+
+static void hv_resume(void)
+{
+	union hv_x64_msr_hypercall_contents hypercall_msr;
+
+	/* Re-enable the hypercall page */
+	rdmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
+	hypercall_msr.enable = 1;
+	hypercall_msr.guest_physical_address = vmalloc_to_pfn(hv_hypercall_pg);
+	wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
+}
+
+static struct syscore_ops hv_syscore_ops = {
+	.suspend = hv_suspend,
+	.resume = hv_resume,
+};
+
 /*
  * This function is to be invoked early in the boot sequence after the
  * hypervisor has been detected.
@@ -294,6 +323,9 @@ void __init hyperv_init(void)
 
 	/* Register Hyper-V specific clocksource */
 	hv_init_clocksource();
+
+	register_syscore_ops(&hv_syscore_ops);
+
 	return;
 
 remove_cpuhp_state:
@@ -313,6 +345,8 @@ void hyperv_cleanup(void)
 {
 	union hv_x64_msr_hypercall_contents hypercall_msr;
 
+	unregister_syscore_ops(&hv_syscore_ops);
+
 	/* Reset our OS id */
 	wrmsrl(HV_X64_MSR_GUEST_OS_ID, 0);
 
-- 
1.8.3.1

[PATCH 2/7] clocksource/drivers: Suspend/resume Hyper-V clocksource for hibernation

[Dexuan Cui]

This is needed for hibernation, e.g. when we resume the old kernel, we need
to disable the "current" kernel's TSC page and then resume the old kernel's.

Signed-off-by: Dexuan Cui <decui@microsoft.com>
---
 drivers/clocksource/hyperv_timer.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/clocksource/hyperv_timer.c b/drivers/clocksource/hyperv_timer.c
index ba2c79e6..41c31a7 100644
--- a/drivers/clocksource/hyperv_timer.c
+++ b/drivers/clocksource/hyperv_timer.c
@@ -237,12 +237,37 @@ static u64 read_hv_clock_tsc(struct clocksource *arg)
 	return read_hv_sched_clock_tsc();
 }
 
+static void suspend_hv_clock_tsc(struct clocksource *arg)
+{
+	u64 tsc_msr;
+
+	/* Disable the TSC page */
+	hv_get_reference_tsc(tsc_msr);
+	tsc_msr &= ~BIT_ULL(0);
+	hv_set_reference_tsc(tsc_msr);
+}
+
+
+static void resume_hv_clock_tsc(struct clocksource *arg)
+{
+	phys_addr_t phys_addr = page_to_phys(vmalloc_to_page(tsc_pg));
+	u64 tsc_msr;
+
+	/* Re-enable the TSC page */
+	hv_get_reference_tsc(tsc_msr);
+	tsc_msr &= GENMASK_ULL(11, 0);
+	tsc_msr |= BIT_ULL(0) | (u64)phys_addr;
+	hv_set_reference_tsc(tsc_msr);
+}
+
 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,
+	.suspend= suspend_hv_clock_tsc,
+	.resume	= resume_hv_clock_tsc,
 };
 #endif
 
-- 
1.8.3.1

[PATCH 3/7] Drivers: hv: vmbus: Split hv_synic_init/cleanup into regs and timer settings

[Dexuan Cui]

There is only one functional change: the unnecessary check
"if (sctrl.enable != 1) return -EFAULT;" is removed, because when we're in
hv_synic_cleanup(), we're absolutely sure sctrl.enable must be 1.

The new functions hv_synic_disable/enable_regs() will be used by a later patch
to support hibernation.

Signed-off-by: Dexuan Cui <decui@microsoft.com>
---
 drivers/hv/hv.c           | 66 ++++++++++++++++++++++++++---------------------
 drivers/hv/hyperv_vmbus.h |  2 ++
 2 files changed, 39 insertions(+), 29 deletions(-)

diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
index 6188fb7..fcc5279 100644
--- a/drivers/hv/hv.c
+++ b/drivers/hv/hv.c
@@ -154,7 +154,7 @@ void hv_synic_free(void)
  * retrieve the initialized message and event pages.  Otherwise, we create and
  * initialize the message and event pages.
  */
-int hv_synic_init(unsigned int cpu)
+void hv_synic_enable_regs(unsigned int cpu)
 {
 	struct hv_per_cpu_context *hv_cpu
 		= per_cpu_ptr(hv_context.cpu_context, cpu);
@@ -196,6 +196,11 @@ int hv_synic_init(unsigned int cpu)
 	sctrl.enable = 1;
 
 	hv_set_synic_state(sctrl.as_uint64);
+}
+
+int hv_synic_init(unsigned int cpu)
+{
+	hv_synic_enable_regs(cpu);
 
 	hv_stimer_init(cpu);
 
@@ -205,20 +210,45 @@ int hv_synic_init(unsigned int cpu)
 /*
  * hv_synic_cleanup - Cleanup routine for hv_synic_init().
  */
-int hv_synic_cleanup(unsigned int cpu)
+void hv_synic_disable_regs(unsigned int cpu)
 {
 	union hv_synic_sint shared_sint;
 	union hv_synic_simp simp;
 	union hv_synic_siefp siefp;
 	union hv_synic_scontrol sctrl;
+
+	hv_get_synint_state(VMBUS_MESSAGE_SINT, shared_sint.as_uint64);
+
+	shared_sint.masked = 1;
+
+	/* Need to correctly cleanup in the case of SMP!!! */
+	/* Disable the interrupt */
+	hv_set_synint_state(VMBUS_MESSAGE_SINT, shared_sint.as_uint64);
+
+	hv_get_simp(simp.as_uint64);
+	simp.simp_enabled = 0;
+	simp.base_simp_gpa = 0;
+
+	hv_set_simp(simp.as_uint64);
+
+	hv_get_siefp(siefp.as_uint64);
+	siefp.siefp_enabled = 0;
+	siefp.base_siefp_gpa = 0;
+
+	hv_set_siefp(siefp.as_uint64);
+
+	/* Disable the global synic bit */
+	hv_get_synic_state(sctrl.as_uint64);
+	sctrl.enable = 0;
+	hv_set_synic_state(sctrl.as_uint64);
+}
+
+int hv_synic_cleanup(unsigned int cpu)
+{
 	struct vmbus_channel *channel, *sc;
 	bool channel_found = false;
 	unsigned long flags;
 
-	hv_get_synic_state(sctrl.as_uint64);
-	if (sctrl.enable != 1)
-		return -EFAULT;
-
 	/*
 	 * Search for channels which are bound to the CPU we're about to
 	 * cleanup. In case we find one and vmbus is still connected we need to
@@ -249,29 +279,7 @@ int hv_synic_cleanup(unsigned int cpu)
 
 	hv_stimer_cleanup(cpu);
 
-	hv_get_synint_state(VMBUS_MESSAGE_SINT, shared_sint.as_uint64);
-
-	shared_sint.masked = 1;
-
-	/* Need to correctly cleanup in the case of SMP!!! */
-	/* Disable the interrupt */
-	hv_set_synint_state(VMBUS_MESSAGE_SINT, shared_sint.as_uint64);
-
-	hv_get_simp(simp.as_uint64);
-	simp.simp_enabled = 0;
-	simp.base_simp_gpa = 0;
-
-	hv_set_simp(simp.as_uint64);
-
-	hv_get_siefp(siefp.as_uint64);
-	siefp.siefp_enabled = 0;
-	siefp.base_siefp_gpa = 0;
-
-	hv_set_siefp(siefp.as_uint64);
-
-	/* Disable the global synic bit */
-	sctrl.enable = 0;
-	hv_set_synic_state(sctrl.as_uint64);
+	hv_synic_disable_regs(cpu);
 
 	return 0;
 }
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index 362e70e..26ea161 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -171,8 +171,10 @@ extern int hv_post_message(union hv_connection_id connection_id,
 
 extern void hv_synic_free(void);
 
+extern void hv_synic_enable_regs(unsigned int cpu);
 extern int hv_synic_init(unsigned int cpu);
 
+extern void hv_synic_disable_regs(unsigned int cpu);
 extern int hv_synic_cleanup(unsigned int cpu);
 
 /* Interface */
-- 
1.8.3.1

[PATCH 4/7] Drivers: hv: vmbus: Suspend/resume the synic for hibernation

[Dexuan Cui]

This is needed when we resume the old kernel from the "current" kernel.

Signed-off-by: Dexuan Cui <decui@microsoft.com>
---
 drivers/hv/vmbus_drv.c | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 72d5a7c..1c2d935 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -30,6 +30,7 @@
 #include <linux/kdebug.h>
 #include <linux/efi.h>
 #include <linux/random.h>
+#include <linux/syscore_ops.h>
 #include <clocksource/hyperv_timer.h>
 #include "hyperv_vmbus.h"
 
@@ -2088,6 +2089,41 @@ static void hv_crash_handler(struct pt_regs *regs)
 	hyperv_cleanup();
 };
 
+static int hv_synic_suspend(void)
+{
+	/*
+	 * Here we only need to care about CPU0: when the other CPUs are
+	 * offlined, hv_synic_cleanup() has been called for them, and the
+	 * timers on them have been automatically disabled and deleted in
+	 * tick_cleanup_dead_cpu().
+	 */
+	hv_stimer_cleanup(0);
+
+	hv_synic_disable_regs(0);
+
+	return 0;
+}
+
+static void hv_synic_resume(void)
+{
+	/*
+	 * Here we only need to care about CPU0: when the other CPUs are
+	 * onlined, hv_synic_init() has been called, and the timers are
+	 * added there.
+	 *
+	 * Note: we don't need to call hv_stimer_init() for stimer0, because
+	 * it is not deleted before hibernation and it's resumed in
+	 * timekeeping_resume().
+	 */
+	hv_synic_enable_regs(0);
+}
+
+/* The callbacks run only on CPU0, with irqs_disabled. */
+static struct syscore_ops hv_synic_syscore_ops = {
+	.suspend = hv_synic_suspend,
+	.resume = hv_synic_resume,
+};
+
 static int __init hv_acpi_init(void)
 {
 	int ret, t;
@@ -2118,6 +2154,8 @@ static int __init hv_acpi_init(void)
 	hv_setup_kexec_handler(hv_kexec_handler);
 	hv_setup_crash_handler(hv_crash_handler);
 
+	register_syscore_ops(&hv_synic_syscore_ops);
+
 	return 0;
 
 cleanup:
@@ -2130,6 +2168,8 @@ static void __exit vmbus_exit(void)
 {
 	int cpu;
 
+	unregister_syscore_ops(&hv_synic_syscore_ops);
+
 	hv_remove_kexec_handler();
 	hv_remove_crash_handler();
 	vmbus_connection.conn_state = DISCONNECTED;
-- 
1.8.3.1

[PATCH 5/7] Drivers: hv: vmbus: Ignore the offers when resuming from hibernation

[Dexuan Cui]

When the VM resumes, the host re-sends the offers. We should not add the
offers to the global vmbus_connection.chn_list again.

Added some debug code, in case the host screws up the exact info related to
the offers.

Signed-off-by: Dexuan Cui <decui@microsoft.com>
---
 drivers/hv/channel_mgmt.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index addcef5..a9aeeab 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -854,12 +854,38 @@ void vmbus_initiate_unload(bool crash)
 static void vmbus_onoffer(struct vmbus_channel_message_header *hdr)
 {
 	struct vmbus_channel_offer_channel *offer;
-	struct vmbus_channel *newchannel;
+	struct vmbus_channel *oldchannel, *newchannel;
+	size_t offer_sz;
 
 	offer = (struct vmbus_channel_offer_channel *)hdr;
 
 	trace_vmbus_onoffer(offer);
 
+	mutex_lock(&vmbus_connection.channel_mutex);
+	oldchannel = relid2channel(offer->child_relid);
+	mutex_unlock(&vmbus_connection.channel_mutex);
+
+	if (oldchannel != NULL) {
+		atomic_dec(&vmbus_connection.offer_in_progress);
+
+		/*
+		 * We're resuming from hibernation: we expect the host to send
+		 * exactly the same offers that we had before the hibernation.
+		 */
+		offer_sz = sizeof(*offer);
+		if (memcmp(offer, &oldchannel->offermsg, offer_sz) == 0)
+			return;
+
+		pr_err("Mismatched offer from the host (relid=%d)!\n",
+		       offer->child_relid);
+
+		print_hex_dump_debug("Old vmbus offer: ", DUMP_PREFIX_OFFSET, 4,
+				     4, &oldchannel->offermsg, offer_sz, false);
+		print_hex_dump_debug("New vmbus offer: ", DUMP_PREFIX_OFFSET, 4,
+				     4, offer, offer_sz, false);
+		return;
+	}
+
 	/* Allocate the channel object and save this offer. */
 	newchannel = alloc_channel();
 	if (!newchannel) {
-- 
1.8.3.1

[PATCH 6/7] Drivers: hv: vmbus: Suspend/resume the vmbus itself for hibernation

[Dexuan Cui]

This is needed when we resume the old kernel from the "current" kernel.

Signed-off-by: Dexuan Cui <decui@microsoft.com>
---
 drivers/hv/connection.c   |  3 +--
 drivers/hv/hyperv_vmbus.h |  2 ++
 drivers/hv/vmbus_drv.c    | 40 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
index 09829e1..806319c 100644
--- a/drivers/hv/connection.c
+++ b/drivers/hv/connection.c
@@ -59,8 +59,7 @@ static __u32 vmbus_get_next_version(__u32 current_version)
 	}
 }
 
-static int vmbus_negotiate_version(struct vmbus_channel_msginfo *msginfo,
-					__u32 version)
+int vmbus_negotiate_version(struct vmbus_channel_msginfo *msginfo, u32 version)
 {
 	int ret = 0;
 	unsigned int cur_cpu;
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index 26ea161..e657197 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -274,6 +274,8 @@ struct vmbus_msginfo {
 
 extern struct vmbus_connection vmbus_connection;
 
+int vmbus_negotiate_version(struct vmbus_channel_msginfo *msginfo, u32 version);
+
 static inline void vmbus_send_interrupt(u32 relid)
 {
 	sync_set_bit(relid, vmbus_connection.send_int_page);
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 1c2d935..1730e7b 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -2045,6 +2045,41 @@ static int vmbus_acpi_add(struct acpi_device *device)
 	return ret_val;
 }
 
+static int vmbus_bus_suspend(struct device *dev)
+{
+	vmbus_initiate_unload(false);
+
+	vmbus_connection.conn_state = DISCONNECTED;
+
+	return 0;
+}
+
+static int vmbus_bus_resume(struct device *dev)
+{
+	struct vmbus_channel_msginfo *msginfo;
+	size_t msgsize;
+	int ret;
+
+	msgsize = sizeof(*msginfo) +
+		  sizeof(struct vmbus_channel_initiate_contact);
+
+	msginfo = kzalloc(msgsize, GFP_KERNEL);
+
+	if (msginfo == NULL)
+		return -ENOMEM;
+
+	ret = vmbus_negotiate_version(msginfo, vmbus_proto_version);
+
+	kfree(msginfo);
+
+	if (ret != 0)
+		return ret;
+
+	vmbus_request_offers();
+
+	return 0;
+}
+
 static const struct acpi_device_id vmbus_acpi_device_ids[] = {
 	{"VMBUS", 0},
 	{"VMBus", 0},
@@ -2052,6 +2087,10 @@ static int vmbus_acpi_add(struct acpi_device *device)
 };
 MODULE_DEVICE_TABLE(acpi, vmbus_acpi_device_ids);
 
+static const struct dev_pm_ops vmbus_bus_pm = {
+	SET_SYSTEM_SLEEP_PM_OPS(vmbus_bus_suspend, vmbus_bus_resume)
+};
+
 static struct acpi_driver vmbus_acpi_driver = {
 	.name = "vmbus",
 	.ids = vmbus_acpi_device_ids,
@@ -2059,6 +2098,7 @@ static int vmbus_acpi_add(struct acpi_device *device)
 		.add = vmbus_acpi_add,
 		.remove = vmbus_acpi_remove,
 	},
+	.drv.pm = &vmbus_bus_pm,
 };
 
 static void hv_kexec_handler(void)
-- 
1.8.3.1

[PATCH 7/7] Drivers: hv: vmbus: Implement suspend/resume for VSC drivers for hibernation

[Dexuan Cui]

The high-level VSC drivers will implement device-specific callbacks.

Signed-off-by: Dexuan Cui <decui@microsoft.com>
---
 drivers/hv/vmbus_drv.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/hyperv.h |  3 +++
 2 files changed, 45 insertions(+)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 1730e7b..e29e2171 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -911,6 +911,43 @@ static void vmbus_shutdown(struct device *child_device)
 		drv->shutdown(dev);
 }
 
+/*
+ * vmbus_suspend - Suspend a vmbus device
+ */
+static int vmbus_suspend(struct device *child_device)
+{
+	struct hv_driver *drv;
+	struct hv_device *dev = device_to_hv_device(child_device);
+
+	/* The device may not be attached yet */
+	if (!child_device->driver)
+		return 0;
+
+	drv = drv_to_hv_drv(child_device->driver);
+	if (!drv->suspend)
+		return -EOPNOTSUPP;
+
+	return drv->suspend(dev);
+}
+
+/*
+ * vmbus_resume - Resume a vmbus device
+ */
+static int vmbus_resume(struct device *child_device)
+{
+	struct hv_driver *drv;
+	struct hv_device *dev = device_to_hv_device(child_device);
+
+	/* The device may not be attached yet */
+	if (!child_device->driver)
+		return 0;
+
+	drv = drv_to_hv_drv(child_device->driver);
+	if (!drv->resume)
+		return -EOPNOTSUPP;
+
+	return drv->resume(dev);
+}
 
 /*
  * vmbus_device_release - Final callback release of the vmbus child device
@@ -926,6 +963,10 @@ static void vmbus_device_release(struct device *device)
 	kfree(hv_dev);
 }
 
+static const struct dev_pm_ops vmbus_pm = {
+	SET_SYSTEM_SLEEP_PM_OPS(vmbus_suspend, vmbus_resume)
+};
+
 /* The one and only one */
 static struct bus_type  hv_bus = {
 	.name =		"vmbus",
@@ -936,6 +977,7 @@ static void vmbus_device_release(struct device *device)
 	.uevent =		vmbus_uevent,
 	.dev_groups =		vmbus_dev_groups,
 	.drv_groups =		vmbus_drv_groups,
+	.pm =			&vmbus_pm,
 };
 
 struct onmessage_work_context {
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 6256cc3..94443c4 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -1149,6 +1149,9 @@ struct hv_driver {
 	int (*remove)(struct hv_device *);
 	void (*shutdown)(struct hv_device *);
 
+	int (*suspend)(struct hv_device *);
+	int (*resume)(struct hv_device *);
+
 };
 
 /* Base device object */
-- 
1.8.3.1

RE: [PATCH 1/7] x86/hyper-v: Suspend/resume the hypercall page for hibernation

[Michael Kelley]

From: Dexuan Cui <decui@microsoft.com> Sent: Monday, July 8, 2019 10:29 PM
> 
> This is needed for hibernation, e.g. when we resume the old kernel, we need
> to disable the "current" kernel's hypercall page and then resume the old
> kernel's.
> 
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
>  arch/x86/hyperv/hv_init.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index 0e033ef..3005871 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -20,6 +20,7 @@
>  #include <linux/hyperv.h>
>  #include <linux/slab.h>
>  #include <linux/cpuhotplug.h>
> +#include <linux/syscore_ops.h>
>  #include <clocksource/hyperv_timer.h>
> 
>  void *hv_hypercall_pg;
> @@ -214,6 +215,34 @@ static int __init hv_pci_init(void)
>  	return 1;
>  }
> 
> +static int hv_suspend(void)
> +{
> +	union hv_x64_msr_hypercall_contents hypercall_msr;
> +
> +	/* Reset the hypercall page */
> +	rdmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> +	hypercall_msr.enable = 0;
> +	wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> +
> +	return 0;
> +}
> +
> +static void hv_resume(void)
> +{
> +	union hv_x64_msr_hypercall_contents hypercall_msr;
> +
> +	/* Re-enable the hypercall page */
> +	rdmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> +	hypercall_msr.enable = 1;
> +	hypercall_msr.guest_physical_address = vmalloc_to_pfn(hv_hypercall_pg);
> +	wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> +}
> +
> +static struct syscore_ops hv_syscore_ops = {
> +	.suspend = hv_suspend,
> +	.resume = hv_resume,
> +};
> +
>  /*
>   * This function is to be invoked early in the boot sequence after the
>   * hypervisor has been detected.
> @@ -294,6 +323,9 @@ void __init hyperv_init(void)
> 
>  	/* Register Hyper-V specific clocksource */
>  	hv_init_clocksource();
> +
> +	register_syscore_ops(&hv_syscore_ops);
> +
>  	return;
> 
>  remove_cpuhp_state:
> @@ -313,6 +345,8 @@ void hyperv_cleanup(void)
>  {
>  	union hv_x64_msr_hypercall_contents hypercall_msr;
> 
> +	unregister_syscore_ops(&hv_syscore_ops);
> +
>  	/* Reset our OS id */
>  	wrmsrl(HV_X64_MSR_GUEST_OS_ID, 0);
> 
> --
> 1.8.3.1

Reviewed-by: Michael Kelley <mikelley@microsoft.com>

RE: [PATCH 2/7] clocksource/drivers: Suspend/resume Hyper-V clocksource for hibernation

[Michael Kelley]

From: Dexuan Cui <decui@microsoft.com> Sent: Monday, July 8, 2019 10:29 PM
> 
> This is needed for hibernation, e.g. when we resume the old kernel, we need
> to disable the "current" kernel's TSC page and then resume the old kernel's.
> 
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
>  drivers/clocksource/hyperv_timer.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/clocksource/hyperv_timer.c b/drivers/clocksource/hyperv_timer.c
> index ba2c79e6..41c31a7 100644
> --- a/drivers/clocksource/hyperv_timer.c
> +++ b/drivers/clocksource/hyperv_timer.c
> @@ -237,12 +237,37 @@ static u64 read_hv_clock_tsc(struct clocksource *arg)
>  	return read_hv_sched_clock_tsc();
>  }
> 
> +static void suspend_hv_clock_tsc(struct clocksource *arg)
> +{
> +	u64 tsc_msr;
> +
> +	/* Disable the TSC page */
> +	hv_get_reference_tsc(tsc_msr);
> +	tsc_msr &= ~BIT_ULL(0);
> +	hv_set_reference_tsc(tsc_msr);
> +}
> +
> +
> +static void resume_hv_clock_tsc(struct clocksource *arg)
> +{
> +	phys_addr_t phys_addr = page_to_phys(vmalloc_to_page(tsc_pg));
> +	u64 tsc_msr;
> +
> +	/* Re-enable the TSC page */
> +	hv_get_reference_tsc(tsc_msr);
> +	tsc_msr &= GENMASK_ULL(11, 0);
> +	tsc_msr |= BIT_ULL(0) | (u64)phys_addr;
> +	hv_set_reference_tsc(tsc_msr);
> +}
> +
>  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,
> +	.suspend= suspend_hv_clock_tsc,
> +	.resume	= resume_hv_clock_tsc,
>  };
>  #endif
> 
> --
> 1.8.3.1

Reviewed-by: Michael Kelley <mikelley@microsoft.com>

RE: [PATCH 3/7] Drivers: hv: vmbus: Split hv_synic_init/cleanup into regs and timer settings

[Michael Kelley]

From: Dexuan Cui <decui@microsoft.com>  Sent: Monday, July 8, 2019 10:29 PM
> 
> There is only one functional change: the unnecessary check
> "if (sctrl.enable != 1) return -EFAULT;" is removed, because when we're in
> hv_synic_cleanup(), we're absolutely sure sctrl.enable must be 1.
> 
> The new functions hv_synic_disable/enable_regs() will be used by a later patch
> to support hibernation.

Seems like this commit message doesn't really describe the main change.
How about:

Break out synic enable and disable operations into separate
hv_synic_disable_regs() and hv_synic_enable_regs() functions for use by a
later patch to support hibernation.

There is no functional change except the unnecessary check
"if (sctrl.enable != 1) return -EFAULT;" is removed, because when we're in
hv_synic_cleanup(), we're absolutely sure sctrl.enable must be 1.

Otherwise,

Reviewed-by:  Michael Kelley <mikelley@microsoft.com>

> 
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
>  drivers/hv/hv.c           | 66 ++++++++++++++++++++++++++---------------------
>  drivers/hv/hyperv_vmbus.h |  2 ++
>  2 files changed, 39 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
> index 6188fb7..fcc5279 100644
> --- a/drivers/hv/hv.c
> +++ b/drivers/hv/hv.c
> @@ -154,7 +154,7 @@ void hv_synic_free(void)
>   * retrieve the initialized message and event pages.  Otherwise, we create and
>   * initialize the message and event pages.
>   */
> -int hv_synic_init(unsigned int cpu)
> +void hv_synic_enable_regs(unsigned int cpu)
>  {
>  	struct hv_per_cpu_context *hv_cpu
>  		= per_cpu_ptr(hv_context.cpu_context, cpu);
> @@ -196,6 +196,11 @@ int hv_synic_init(unsigned int cpu)
>  	sctrl.enable = 1;
> 
>  	hv_set_synic_state(sctrl.as_uint64);
> +}
> +
> +int hv_synic_init(unsigned int cpu)
> +{
> +	hv_synic_enable_regs(cpu);
> 
>  	hv_stimer_init(cpu);
> 
> @@ -205,20 +210,45 @@ int hv_synic_init(unsigned int cpu)
>  /*
>   * hv_synic_cleanup - Cleanup routine for hv_synic_init().
>   */
> -int hv_synic_cleanup(unsigned int cpu)
> +void hv_synic_disable_regs(unsigned int cpu)
>  {
>  	union hv_synic_sint shared_sint;
>  	union hv_synic_simp simp;
>  	union hv_synic_siefp siefp;
>  	union hv_synic_scontrol sctrl;
> +
> +	hv_get_synint_state(VMBUS_MESSAGE_SINT, shared_sint.as_uint64);
> +
> +	shared_sint.masked = 1;
> +
> +	/* Need to correctly cleanup in the case of SMP!!! */
> +	/* Disable the interrupt */
> +	hv_set_synint_state(VMBUS_MESSAGE_SINT, shared_sint.as_uint64);
> +
> +	hv_get_simp(simp.as_uint64);
> +	simp.simp_enabled = 0;
> +	simp.base_simp_gpa = 0;
> +
> +	hv_set_simp(simp.as_uint64);
> +
> +	hv_get_siefp(siefp.as_uint64);
> +	siefp.siefp_enabled = 0;
> +	siefp.base_siefp_gpa = 0;
> +
> +	hv_set_siefp(siefp.as_uint64);
> +
> +	/* Disable the global synic bit */
> +	hv_get_synic_state(sctrl.as_uint64);
> +	sctrl.enable = 0;
> +	hv_set_synic_state(sctrl.as_uint64);
> +}
> +
> +int hv_synic_cleanup(unsigned int cpu)
> +{
>  	struct vmbus_channel *channel, *sc;
>  	bool channel_found = false;
>  	unsigned long flags;
> 
> -	hv_get_synic_state(sctrl.as_uint64);
> -	if (sctrl.enable != 1)
> -		return -EFAULT;
> -
>  	/*
>  	 * Search for channels which are bound to the CPU we're about to
>  	 * cleanup. In case we find one and vmbus is still connected we need to
> @@ -249,29 +279,7 @@ int hv_synic_cleanup(unsigned int cpu)
> 
>  	hv_stimer_cleanup(cpu);
> 
> -	hv_get_synint_state(VMBUS_MESSAGE_SINT, shared_sint.as_uint64);
> -
> -	shared_sint.masked = 1;
> -
> -	/* Need to correctly cleanup in the case of SMP!!! */
> -	/* Disable the interrupt */
> -	hv_set_synint_state(VMBUS_MESSAGE_SINT, shared_sint.as_uint64);
> -
> -	hv_get_simp(simp.as_uint64);
> -	simp.simp_enabled = 0;
> -	simp.base_simp_gpa = 0;
> -
> -	hv_set_simp(simp.as_uint64);
> -
> -	hv_get_siefp(siefp.as_uint64);
> -	siefp.siefp_enabled = 0;
> -	siefp.base_siefp_gpa = 0;
> -
> -	hv_set_siefp(siefp.as_uint64);
> -
> -	/* Disable the global synic bit */
> -	sctrl.enable = 0;
> -	hv_set_synic_state(sctrl.as_uint64);
> +	hv_synic_disable_regs(cpu);
> 
>  	return 0;
>  }
> diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
> index 362e70e..26ea161 100644
> --- a/drivers/hv/hyperv_vmbus.h
> +++ b/drivers/hv/hyperv_vmbus.h
> @@ -171,8 +171,10 @@ extern int hv_post_message(union hv_connection_id
> connection_id,
> 
>  extern void hv_synic_free(void);
> 
> +extern void hv_synic_enable_regs(unsigned int cpu);
>  extern int hv_synic_init(unsigned int cpu);
> 
> +extern void hv_synic_disable_regs(unsigned int cpu);
>  extern int hv_synic_cleanup(unsigned int cpu);
> 
>  /* Interface */
> --
> 1.8.3.1

RE: [PATCH 5/7] Drivers: hv: vmbus: Ignore the offers when resuming from hibernation

[Michael Kelley]

From: Dexuan Cui <decui@microsoft.com> Sent: Monday, July 8, 2019 10:29 PM
> 
> When the VM resumes, the host re-sends the offers. We should not add the
> offers to the global vmbus_connection.chn_list again.
> 
> Added some debug code, in case the host screws up the exact info related to
> the offers.
> 
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
>  drivers/hv/channel_mgmt.c | 28 +++++++++++++++++++++++++++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
> index addcef5..a9aeeab 100644
> --- a/drivers/hv/channel_mgmt.c
> +++ b/drivers/hv/channel_mgmt.c
> @@ -854,12 +854,38 @@ void vmbus_initiate_unload(bool crash)
>  static void vmbus_onoffer(struct vmbus_channel_message_header *hdr)
>  {
>  	struct vmbus_channel_offer_channel *offer;
> -	struct vmbus_channel *newchannel;
> +	struct vmbus_channel *oldchannel, *newchannel;
> +	size_t offer_sz;
> 
>  	offer = (struct vmbus_channel_offer_channel *)hdr;
> 
>  	trace_vmbus_onoffer(offer);
> 
> +	mutex_lock(&vmbus_connection.channel_mutex);
> +	oldchannel = relid2channel(offer->child_relid);
> +	mutex_unlock(&vmbus_connection.channel_mutex);
> +
> +	if (oldchannel != NULL) {
> +		atomic_dec(&vmbus_connection.offer_in_progress);
> +
> +		/*
> +		 * We're resuming from hibernation: we expect the host to send
> +		 * exactly the same offers that we had before the hibernation.
> +		 */
> +		offer_sz = sizeof(*offer);
> +		if (memcmp(offer, &oldchannel->offermsg, offer_sz) == 0)
> +			return;

The offermsg contains "reserved" and "padding" fields.  Does Hyper-V
guarantee that all these fields are the same in the new offer after resuming
from hibernation?  Or should a less stringent check be made?  For example,
I could imagine a newer version of Hyper-V allowing a VM that was
hibernated on an older version to be resumed.  But one of the reserved fields
might be used in the newer version, and the comparison could fail
unnecessarily.

> +
> +		pr_err("Mismatched offer from the host (relid=%d)!\n",
> +		       offer->child_relid);
> +
> +		print_hex_dump_debug("Old vmbus offer: ", DUMP_PREFIX_OFFSET, 4,
> +				     4, &oldchannel->offermsg, offer_sz, false);
> +		print_hex_dump_debug("New vmbus offer: ", DUMP_PREFIX_OFFSET, 4,
> +				     4, offer, offer_sz, false);

The third argument to print_hex_dump() is the rowsize and is specified as must
be 16 or 32.  

> +		return;
> +	}
> +
>  	/* Allocate the channel object and save this offer. */
>  	newchannel = alloc_channel();
>  	if (!newchannel) {
> --
> 1.8.3.1

RE: [PATCH 3/7] Drivers: hv: vmbus: Split hv_synic_init/cleanup into regs and timer settings

[Dexuan Cui]

> From: Michael Kelley <mikelley@microsoft.com>
> Sent: Tuesday, July 30, 2019 3:36 PM
> 
> From: Dexuan Cui <decui@microsoft.com>  Sent: Monday, July 8, 2019 10:29
> PM
> >
> > There is only one functional change: the unnecessary check
> > "if (sctrl.enable != 1) return -EFAULT;" is removed, because when we're in
> > hv_synic_cleanup(), we're absolutely sure sctrl.enable must be 1.
> >
> > The new functions hv_synic_disable/enable_regs() will be used by a later
> patch
> > to support hibernation.
> 
> Seems like this commit message doesn't really describe the main change.
> How about:
> 
> Break out synic enable and disable operations into separate
> hv_synic_disable_regs() and hv_synic_enable_regs() functions for use by a
> later patch to support hibernation.
> 
> There is no functional change except the unnecessary check
> "if (sctrl.enable != 1) return -EFAULT;" is removed, because when we're in
> hv_synic_cleanup(), we're absolutely sure sctrl.enable must be 1.
> 
> Otherwise,
> 
> Reviewed-by:  Michael Kelley <mikelley@microsoft.com>

Thanks! I'll use your version as the changelog of v2. I'll change the 
Subject accordingly.

Thanks,
-- Dexuan

RE: [PATCH 6/7] Drivers: hv: vmbus: Suspend/resume the vmbus itself for hibernation

[Michael Kelley]

From: Dexuan Cui <decui@microsoft.com>  Sent: Monday, July 8, 2019 10:30 PM
> 
> This is needed when we resume the old kernel from the "current" kernel.

Perhaps a bit more descriptive commit message could be supplied?

> 
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
>  drivers/hv/connection.c   |  3 +--
>  drivers/hv/hyperv_vmbus.h |  2 ++
>  drivers/hv/vmbus_drv.c    | 40 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 43 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
> index 09829e1..806319c 100644
> --- a/drivers/hv/connection.c
> +++ b/drivers/hv/connection.c
> @@ -59,8 +59,7 @@ static __u32 vmbus_get_next_version(__u32 current_version)
>  	}
>  }
> 
> -static int vmbus_negotiate_version(struct vmbus_channel_msginfo *msginfo,
> -					__u32 version)
> +int vmbus_negotiate_version(struct vmbus_channel_msginfo *msginfo, u32 version)
>  {
>  	int ret = 0;
>  	unsigned int cur_cpu;
> diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
> index 26ea161..e657197 100644
> --- a/drivers/hv/hyperv_vmbus.h
> +++ b/drivers/hv/hyperv_vmbus.h
> @@ -274,6 +274,8 @@ struct vmbus_msginfo {
> 
>  extern struct vmbus_connection vmbus_connection;
> 
> +int vmbus_negotiate_version(struct vmbus_channel_msginfo *msginfo, u32 version);
> +
>  static inline void vmbus_send_interrupt(u32 relid)
>  {
>  	sync_set_bit(relid, vmbus_connection.send_int_page);
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 1c2d935..1730e7b 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -2045,6 +2045,41 @@ static int vmbus_acpi_add(struct acpi_device *device)
>  	return ret_val;
>  }
> 
> +static int vmbus_bus_suspend(struct device *dev)
> +{
> +	vmbus_initiate_unload(false);
> +
> +	vmbus_connection.conn_state = DISCONNECTED;
> +
> +	return 0;
> +}
> +
> +static int vmbus_bus_resume(struct device *dev)
> +{
> +	struct vmbus_channel_msginfo *msginfo;
> +	size_t msgsize;
> +	int ret;
> +
> +	msgsize = sizeof(*msginfo) +
> +		  sizeof(struct vmbus_channel_initiate_contact);
> +
> +	msginfo = kzalloc(msgsize, GFP_KERNEL);
> +
> +	if (msginfo == NULL)
> +		return -ENOMEM;
> +
> +	ret = vmbus_negotiate_version(msginfo, vmbus_proto_version);

I think this code answers my earlier question:  Upon resume, we negotiate the same
VMbus protocol version we had at time of hibernation, even if running on a newer
version of Hyper-V that might support newer protocol versions.  Hence the offer
messages should not have any previously reserved fields now being used.   A
comment to this effect would be useful.

> +
> +	kfree(msginfo);
> +
> +	if (ret != 0)
> +		return ret;
> +
> +	vmbus_request_offers();
> +
> +	return 0;
> +}
> +
>  static const struct acpi_device_id vmbus_acpi_device_ids[] = {
>  	{"VMBUS", 0},
>  	{"VMBus", 0},
> @@ -2052,6 +2087,10 @@ static int vmbus_acpi_add(struct acpi_device *device)
>  };
>  MODULE_DEVICE_TABLE(acpi, vmbus_acpi_device_ids);
> 
> +static const struct dev_pm_ops vmbus_bus_pm = {
> +	SET_SYSTEM_SLEEP_PM_OPS(vmbus_bus_suspend, vmbus_bus_resume)
> +};
> +
>  static struct acpi_driver vmbus_acpi_driver = {
>  	.name = "vmbus",
>  	.ids = vmbus_acpi_device_ids,
> @@ -2059,6 +2098,7 @@ static int vmbus_acpi_add(struct acpi_device *device)
>  		.add = vmbus_acpi_add,
>  		.remove = vmbus_acpi_remove,
>  	},
> +	.drv.pm = &vmbus_bus_pm,
>  };
> 
>  static void hv_kexec_handler(void)
> --
> 1.8.3.1

RE: [PATCH 7/7] Drivers: hv: vmbus: Implement suspend/resume for VSC drivers for hibernation

[Michael Kelley]

From: Dexuan Cui <decui@microsoft.com> Sent: Monday, July 8, 2019 10:30 PM
> 
> The high-level VSC drivers will implement device-specific callbacks.
> 
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
>  drivers/hv/vmbus_drv.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/hyperv.h |  3 +++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 1730e7b..e29e2171 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -911,6 +911,43 @@ static void vmbus_shutdown(struct device *child_device)
>  		drv->shutdown(dev);
>  }
> 
> +/*
> + * vmbus_suspend - Suspend a vmbus device
> + */
> +static int vmbus_suspend(struct device *child_device)
> +{
> +	struct hv_driver *drv;
> +	struct hv_device *dev = device_to_hv_device(child_device);
> +
> +	/* The device may not be attached yet */
> +	if (!child_device->driver)
> +		return 0;
> +
> +	drv = drv_to_hv_drv(child_device->driver);
> +	if (!drv->suspend)
> +		return -EOPNOTSUPP;
> +
> +	return drv->suspend(dev);
> +}
> +
> +/*
> + * vmbus_resume - Resume a vmbus device
> + */
> +static int vmbus_resume(struct device *child_device)
> +{
> +	struct hv_driver *drv;
> +	struct hv_device *dev = device_to_hv_device(child_device);
> +
> +	/* The device may not be attached yet */
> +	if (!child_device->driver)
> +		return 0;
> +
> +	drv = drv_to_hv_drv(child_device->driver);
> +	if (!drv->resume)
> +		return -EOPNOTSUPP;
> +
> +	return drv->resume(dev);
> +}
> 
>  /*
>   * vmbus_device_release - Final callback release of the vmbus child device
> @@ -926,6 +963,10 @@ static void vmbus_device_release(struct device *device)
>  	kfree(hv_dev);
>  }
> 
> +static const struct dev_pm_ops vmbus_pm = {
> +	SET_SYSTEM_SLEEP_PM_OPS(vmbus_suspend, vmbus_resume)
> +};
> +
>  /* The one and only one */
>  static struct bus_type  hv_bus = {
>  	.name =		"vmbus",
> @@ -936,6 +977,7 @@ static void vmbus_device_release(struct device *device)
>  	.uevent =		vmbus_uevent,
>  	.dev_groups =		vmbus_dev_groups,
>  	.drv_groups =		vmbus_drv_groups,
> +	.pm =			&vmbus_pm,
>  };
> 
>  struct onmessage_work_context {
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index 6256cc3..94443c4 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -1149,6 +1149,9 @@ struct hv_driver {
>  	int (*remove)(struct hv_device *);
>  	void (*shutdown)(struct hv_device *);
> 
> +	int (*suspend)(struct hv_device *);
> +	int (*resume)(struct hv_device *);
> +
>  };
> 
>  /* Base device object */
> --
> 1.8.3.1

Reviewed-by: Michael Kelley <mikelley@microsoft.com>

RE: [PATCH 5/7] Drivers: hv: vmbus: Ignore the offers when resuming from hibernation

[Dexuan Cui]

> From: Michael Kelley <mikelley@microsoft.com>
> Sent: Tuesday, July 30, 2019 4:07 PM
> > +
> > +	if (oldchannel != NULL) {
> > +		atomic_dec(&vmbus_connection.offer_in_progress);
> > +
> > +		/*
> > +		 * We're resuming from hibernation: we expect the host to send
> > +		 * exactly the same offers that we had before the hibernation.
> > +		 */
> > +		offer_sz = sizeof(*offer);
> > +		if (memcmp(offer, &oldchannel->offermsg, offer_sz) == 0)
> > +			return;
> 
> The offermsg contains "reserved" and "padding" fields.  Does Hyper-V
> guarantee that all these fields are the same in the new offer after resuming
> from hibernation? 

Yes. I confirmed this with Hyper-V team. The reserved/padding fields don't change
across hiberantion. BTW, the fields are filled with zeros since they're not used.

>  Or should a less stringent check be made?  For example,
> I could imagine a newer version of Hyper-V allowing a VM that was
> hibernated on an older version to be resumed.  But one of the reserved fields
> might be used in the newer version, and the comparison could fail
> unnecessarily.

Upon resume, Linux VM always uses the same version, which was used when the
VM firstly booted up before suspend, to re-negotiate with the host.
 
> > +
> > +		pr_err("Mismatched offer from the host (relid=%d)!\n",
> > +		       offer->child_relid);
> > +
> > +		print_hex_dump_debug("Old vmbus offer: ", DUMP_PREFIX_OFFSET,
> 4,
> > +				     4, &oldchannel->offermsg, offer_sz, false);
> > +		print_hex_dump_debug("New vmbus offer: ",
> DUMP_PREFIX_OFFSET, 4,
> > +				     4, offer, offer_sz, false);
> 
> The third argument to print_hex_dump() is the rowsize and is specified as must
> be 16 or 32.

Thanks! I misunderstood the argument. I'll change it to 16.

Thanks,
-- Dexuan

RE: [PATCH 6/7] Drivers: hv: vmbus: Suspend/resume the vmbus itself for hibernation

[Dexuan Cui]

> From: Michael Kelley <mikelley@microsoft.com>
> Sent: Tuesday, July 30, 2019 4:26 PM
> From: Dexuan Cui <decui@microsoft.com>  Sent: Monday, July 8, 2019 10:30
> PM
> >
> > This is needed when we resume the old kernel from the "current" kernel.
> 
> Perhaps a bit more descriptive commit message could be supplied?

I'll use this as the new changelog:

Before Linux enters hibernation, it sends the CHANNELMSG_UNLOAD message to
the host so all the offers are gone. After hibernation, Linux needs to re-negotiate
with the host using the same vmbus protocol version (which was in use
before hibernation), and ask the host to re-offer the vmbus devices. 

> > +
> > +	ret = vmbus_negotiate_version(msginfo, vmbus_proto_version);
> 
> I think this code answers my earlier question:  Upon resume, we negotiate
> the same
> VMbus protocol version we had at time of hibernation, even if running on a
> newer
> version of Hyper-V that might support newer protocol versions.  Hence the
> offer
> messages should not have any previously reserved fields now being used.   A
> comment to this effect would be useful.

Ok, let me add a comment before the line.

I'll post a v2 of the patchset, including the "[PATCH 4/7]", which needs a small change.

Thanks,
-- Dexuan