2019-07-09 05:31:33

by Dexuan Cui

[permalink] [raw]
Subject: [PATCH 0/7] Enhance the hv_vmbus driver to support hibernation

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


2019-07-09 05:32:24

by Dexuan Cui

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

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

Signed-off-by: Dexuan Cui <[email protected]>
---
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

2019-07-09 05:32:25

by Dexuan Cui

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

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 <[email protected]>
---
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

2019-07-31 00:21:14

by Michael Kelley (LINUX)

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

From: Dexuan Cui <[email protected]> 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 <[email protected]>

>
> Signed-off-by: Dexuan Cui <[email protected]>
> ---
> 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

2019-07-31 00:23:59

by Dexuan Cui

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

> From: Michael Kelley <[email protected]>
> Sent: Tuesday, July 30, 2019 3:36 PM
>
> From: Dexuan Cui <[email protected]> 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 <[email protected]>

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

Thanks,
-- Dexuan