[PATCH v5 0/3] Enhance Hyper-V for hibernation

[PATCH v5 0/3] Enhance Hyper-V for hibernation

[Dexuan Cui]

This patchset (consisting of 3 patches) was part of the v4 patchset (consisting
of 12 patches):
    https://lkml.org/lkml/2019/9/2/894

I realized these 3 patches must go through the tip.git tree, because I have
to rebase 2 of the 3 patches due to recent changes from others in the tip
tree.

All the 3 patches are now rebased to the tip tree's timers/core branch:
https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/log/?h=timers/core
, and all the 3 patches have Michael Kelley's Signed-off-by's.

Please review.

Thanks!
Dexuan

Dexuan Cui (3):
  x86/hyper-v: Suspend/resume the hypercall page for hibernation
  x86/hyper-v: Implement hv_is_hibernation_supported()
  clocksource/drivers: Suspend/resume Hyper-V clocksource for
    hibernation

 arch/x86/hyperv/hv_init.c          | 40 ++++++++++++++++++++++++++++++++++++++
 drivers/clocksource/hyperv_timer.c | 25 ++++++++++++++++++++++++
 include/asm-generic/mshyperv.h     |  2 ++
 3 files changed, 67 insertions(+)

-- 
1.8.3.1

[PATCH v5 1/3] 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>
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
---
 arch/x86/hyperv/hv_init.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
index 866dfb3..037b0f3 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;
@@ -223,6 +224,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.
@@ -301,6 +330,8 @@ void __init hyperv_init(void)
 
 	x86_init.pci.arch_init = hv_pci_init;
 
+	register_syscore_ops(&hv_syscore_ops);
+
 	return;
 
 remove_cpuhp_state:
@@ -320,6 +351,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 v5 2/3] x86/hyper-v: Implement hv_is_hibernation_supported()

[Dexuan Cui]

The API will be used by the hv_balloon and hv_vmbus drivers.

Balloon up/down and hot-add of memory must not be active if the user
wants the Linux VM to support hibernation, because they are incompatible
with hibernation according to Hyper-V team, e.g. upon suspend the
balloon VSP doesn't save any info about the ballooned-out pages (if any);
so, after Linux resumes, Linux balloon VSC expects that the VSP will
return the pages if Linux is under memory pressure, but the VSP will
never do that, since the VSP thinks it never stole the pages from the VM.

So, if the user wants Linux VM to support hibernation, Linux must forbid
balloon up/down and hot-add, and the only functionality of the balloon VSC
driver is reporting the VM's memory pressure to the host.

Ideally, when Linux detects that the user wants it to support hibernation,
the balloon VSC should tell the VSP that it does not support ballooning
and hot-add. However, the current version of the VSP requires the VSC
should support these capabilities, otherwise the capability negotiation
fails and the VSC can not load at all, so with the later changes to the
VSC driver, Linux VM still reports to the VSP that the VSC supports these
capabilities, but the VSC ignores the VSP's requests of balloon up/down
and hot add, and reports an error to the VSP, when applicable. BTW, in
the future the balloon VSP driver will allow the VSC to not support the
capabilities of balloon up/down and hot add.

The ACPI S4 state is not a must for hibernation to work, because Linux is
able to hibernate as long as the system can shut down. However in practice
we decide to artificially use the presence of the virtual ACPI S4 state as
an indicator of the user's intent of using hibernation, because Linux VM
must find a way to know if the user wants to use the hibernation feature
or not.

By default, Hyper-V does not enable the virtual ACPI S4 state; on recent
Hyper-V hosts (e.g. RS5, 19H1), the administrator is able to enable the
state for a VM by WMI commands.

Once all the vmbus and VSC patches for the hibernation feature are
accepted, an extra patch will be submitted to forbid hibernation if the
virtual ACPI S4 state is absent, i.e. hv_is_hibernation_supported() is
false.

Signed-off-by: Dexuan Cui <decui@microsoft.com>
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
---
 arch/x86/hyperv/hv_init.c      | 7 +++++++
 include/asm-generic/mshyperv.h | 2 ++
 2 files changed, 9 insertions(+)

diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
index 037b0f3..9a26071 100644
--- a/arch/x86/hyperv/hv_init.c
+++ b/arch/x86/hyperv/hv_init.c
@@ -7,6 +7,7 @@
  * Author : K. Y. Srinivasan <kys@microsoft.com>
  */
 
+#include <linux/acpi.h>
 #include <linux/efi.h>
 #include <linux/types.h>
 #include <asm/apic.h>
@@ -450,3 +451,9 @@ bool hv_is_hyperv_initialized(void)
 	return hypercall_msr.enable;
 }
 EXPORT_SYMBOL_GPL(hv_is_hyperv_initialized);
+
+bool hv_is_hibernation_supported(void)
+{
+	return acpi_sleep_state_supported(ACPI_STATE_S4);
+}
+EXPORT_SYMBOL_GPL(hv_is_hibernation_supported);
diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
index 18d8e2d..b3f1082 100644
--- a/include/asm-generic/mshyperv.h
+++ b/include/asm-generic/mshyperv.h
@@ -166,10 +166,12 @@ static inline int cpumask_to_vpset(struct hv_vpset *vpset,
 void hyperv_report_panic(struct pt_regs *regs, long err);
 void hyperv_report_panic_msg(phys_addr_t pa, size_t size);
 bool hv_is_hyperv_initialized(void);
+bool hv_is_hibernation_supported(void);
 void hyperv_cleanup(void);
 void hv_setup_sched_clock(void *sched_clock);
 #else /* CONFIG_HYPERV */
 static inline bool hv_is_hyperv_initialized(void) { return false; }
+static inline bool hv_is_hibernation_supported(void) { return false; }
 static inline void hyperv_cleanup(void) {}
 #endif /* CONFIG_HYPERV */
 
-- 
1.8.3.1

[PATCH v5 3/3] 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>
Reviewed-by: Michael Kelley <mikelley@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 726a65e..07f4747 100644
--- a/drivers/clocksource/hyperv_timer.c
+++ b/drivers/clocksource/hyperv_timer.c
@@ -237,12 +237,37 @@ static u64 read_hv_sched_clock_tsc(void)
 	return read_hv_clock_tsc(NULL) - hv_sched_clock_offset;
 }
 
+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 = virt_to_phys(&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,
 };
 
 static u64 notrace read_hv_clock_msr(struct clocksource *arg)
-- 
1.8.3.1

RE: [PATCH v5 0/3] Enhance Hyper-V for hibernation

[Dexuan Cui]

> From: Dexuan Cui <decui@microsoft.com>
> Sent: Thursday, September 5, 2019 3:47 PM
> 
> This patchset (consisting of 3 patches) was part of the v4 patchset (consisting
> of 12 patches): https://lkml.org/lkml/2019/9/2/894
> 
> I realized these 3 patches must go through the tip.git tree, because I have
> to rebase 2 of the 3 patches due to recent changes from others in the tip
> tree.
> 
> All the 3 patches are now rebased to the tip tree's timers/core branch:
> https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/log/?h=timers/core
> , and all the 3 patches have Michael Kelley's Signed-off-by's.
> 
> Please review.
> 
> Thanks!
> Dexuan
> 
> Dexuan Cui (3):
>   x86/hyper-v: Suspend/resume the hypercall page for hibernation
>   x86/hyper-v: Implement hv_is_hibernation_supported()
>   clocksource/drivers: Suspend/resume Hyper-V clocksource for
>     hibernation
> 
>  arch/x86/hyperv/hv_init.c          | 40
> ++++++++++++++++++++++++++++++++++++++
>  drivers/clocksource/hyperv_timer.c | 25 ++++++++++++++++++++++++
>  include/asm-generic/mshyperv.h     |  2 ++
>  3 files changed, 67 insertions(+)

Hi tglx and all,
Can you please take a look at this patchset (3 patches in total)?

IMO it's better for the 3 patches to go through the tip.git tree, but if you
(the x86 maintainers) have no objection, the patches can also go through
Saha Levin's Hyper-V tree, because the dependent patches have landed
in Linus's tree now.

Thanks,
-- Dexuan

Re: [PATCH v5 3/3] clocksource/drivers: Suspend/resume Hyper-V clocksource for hibernation

[Daniel Lezcano]

On 06/09/2019 00:47, Dexuan Cui wrote:
> 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>
> Reviewed-by: Michael Kelley <mikelley@microsoft.com>

I can take this patch if needed.

> ---
>  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 726a65e..07f4747 100644
> --- a/drivers/clocksource/hyperv_timer.c
> +++ b/drivers/clocksource/hyperv_timer.c
> @@ -237,12 +237,37 @@ static u64 read_hv_sched_clock_tsc(void)
>  	return read_hv_clock_tsc(NULL) - hv_sched_clock_offset;
>  }
>  
> +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 = virt_to_phys(&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_ts,>  };
>  
>  static u64 notrace read_hv_clock_msr(struct clocksource *arg)
> 


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

RE: [PATCH v5 3/3] clocksource/drivers: Suspend/resume Hyper-V clocksource for hibernation

[Dexuan Cui]

> From: Daniel Lezcano <daniel.lezcano@linaro.org>
> Sent: Wednesday, September 25, 2019 4:21 PM
> To: Dexuan Cui <decui@microsoft.com>; arnd@arndb.de; bp@alien8.de;
> Haiyang Zhang <haiyangz@microsoft.com>; hpa@zytor.com; KY Srinivasan
> <kys@microsoft.com>; linux-hyperv@vger.kernel.org;
> linux-kernel@vger.kernel.org; mingo@redhat.com; sashal@kernel.org; Stephen
> Hemminger <sthemmin@microsoft.com>; tglx@linutronix.de; x86@kernel.org;
> Michael Kelley <mikelley@microsoft.com>; Sasha Levin
> <Alexander.Levin@microsoft.com>
> Cc: linux-arch@vger.kernel.org
> Subject: Re: [PATCH v5 3/3] clocksource/drivers: Suspend/resume Hyper-V
> clocksource for hibernation
> 
> On 06/09/2019 00:47, Dexuan Cui wrote:
> > 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>
> > Reviewed-by: Michael Kelley <mikelley@microsoft.com>
> 
> I can take this patch if needed.

Thanks, Daniel! Usually tglx takes care of the patches, but it looks recently he
may be too busy to handle the 3 patches. 

I guess you can take the patch, if tglx has no objection. :-)
If you take the patch, please take all the 3 patches.

Thanks,
-- Dexuan

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

[Vitaly Kuznetsov]

Dexuan Cui <decui@microsoft.com> writes:

> 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>
> Reviewed-by: Michael Kelley <mikelley@microsoft.com>
> ---
>  arch/x86/hyperv/hv_init.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
>
> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index 866dfb3..037b0f3 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;
> @@ -223,6 +224,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);
> +

(trying to think out loud, not sure there's a real issue):

When PV IPIs (or PV TLB flush) are enabled we do the following checks:

	if (!hv_hypercall_pg)
		return false;

or
        if (!hv_hypercall_pg)
                goto do_native;

which will pass as we're not invalidating the pointer. Can we actually
be sure that the kernel will never try to send an IPI/do TLB flush
before we resume?

-- 
Vitaly

Re: [PATCH v5 3/3] clocksource/drivers: Suspend/resume Hyper-V clocksource for hibernation

[Daniel Lezcano]

On 26/09/2019 01:35, Dexuan Cui wrote:
>> From: Daniel Lezcano <daniel.lezcano@linaro.org>
>> Sent: Wednesday, September 25, 2019 4:21 PM
>> To: Dexuan Cui <decui@microsoft.com>; arnd@arndb.de; bp@alien8.de;
>> Haiyang Zhang <haiyangz@microsoft.com>; hpa@zytor.com; KY Srinivasan
>> <kys@microsoft.com>; linux-hyperv@vger.kernel.org;
>> linux-kernel@vger.kernel.org; mingo@redhat.com; sashal@kernel.org; Stephen
>> Hemminger <sthemmin@microsoft.com>; tglx@linutronix.de; x86@kernel.org;
>> Michael Kelley <mikelley@microsoft.com>; Sasha Levin
>> <Alexander.Levin@microsoft.com>
>> Cc: linux-arch@vger.kernel.org
>> Subject: Re: [PATCH v5 3/3] clocksource/drivers: Suspend/resume Hyper-V
>> clocksource for hibernation
>>
>> On 06/09/2019 00:47, Dexuan Cui wrote:
>>> 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>
>>> Reviewed-by: Michael Kelley <mikelley@microsoft.com>
>>
>> I can take this patch if needed.
> 
> Thanks, Daniel! Usually tglx takes care of the patches, but it looks recently he
> may be too busy to handle the 3 patches. 
> 
> I guess you can take the patch, if tglx has no objection. :-)
> If you take the patch, please take all the 3 patches.

I maintain drivers/clocksource for the tip/timers/core branch. I don't
want to proxy another tip branch as it is out of my jurisdiction.

So I can take patch 3/3 but will let the other 2 patches to be picked by
the right person. It is your call.



-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

RE: [PATCH v5 3/3] clocksource/drivers: Suspend/resume Hyper-V clocksource for hibernation

[Dexuan Cui]

> From: Daniel Lezcano <daniel.lezcano@linaro.org>
> Sent: Thursday, September 26, 2019 6:17 AM
> >>
> >> I can take this patch if needed.
> >
> > Thanks, Daniel! Usually tglx takes care of the patches, but it looks recently he
> > may be too busy to handle the 3 patches.
> >
> > I guess you can take the patch, if tglx has no objection. :-)
> > If you take the patch, please take all the 3 patches.
> 
> I maintain drivers/clocksource for the tip/timers/core branch. I don't
> want to proxy another tip branch as it is out of my jurisdiction.

I see. Thanks for the explanation! I learned more about the tip tree. :-)

> So I can take patch 3/3 but will let the other 2 patches to be picked by
> the right person. It is your call.

Sounds good. Daniel, then please take this patch, e.g. patch 3/3.

Patch 2/3 may as well go through Sasha's hyper-v tree, since it's required by
other changes to the drivers hv_balloon, hv_utils and hv_vmbus.

I suppose tglx is the best person to take patch 1/3, but if he's too busy to
handle it, it can also go through the hyper-v tree, since the related other
patches have been in the mainline now.

Thanks,
-- Dexuan

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

[Dexuan Cui]

> From: Vitaly Kuznetsov <vkuznets@redhat.com>
> Sent: Thursday, September 26, 2019 3:44 AM
> > [...]
> > +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);
> > +
> 
> (trying to think out loud, not sure there's a real issue):
> 
> When PV IPIs (or PV TLB flush) are enabled we do the following checks:
> 
> 	if (!hv_hypercall_pg)
> 		return false;
> 
> or
>         if (!hv_hypercall_pg)
>                 goto do_native;
> 
> which will pass as we're not invalidating the pointer. Can we actually
> be sure that the kernel will never try to send an IPI/do TLB flush
> before we resume?
> 
> Vitaly

When hv_suspend() and hv_resume() are called by syscore_suspend()
and syscore_resume(), respectively, all the non-boot CPUs are disabled and
only CPU0 is active and interrupts are disabled, e.g. see

hibernate() -> 
  hibernation_snapshot() ->
    create_image() ->
      suspend_disable_secondary_cpus()
      local_irq_disable()

      syscore_suspend()
      swsusp_arch_suspend
      syscore_resume

      local_irq_enable
      enable_nonboot_cpus


So, I'm pretty sure no IPI can happen between hv_suspend() and hv_resume().
self-IPI is not supposed to happen either, since interrupts are disabled.

IMO TLB flush should not be an issue either, unless the kernel changes page
tables between hv_suspend() and hv_resume(), which is not the case as I
checked the related code, but it looks in theory that might happen, say, in
the future, so if you insist we should save the variable "hv_hypercall_pg"
to a temporary variable and set the "hv_hypercall_pg" to NULL before we
disable the hypercall page, I would be happy to post a new version of this
patch, or we can keep this patch as is and I can make an extra patch.

Thanks,
-- Dexuan

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

[Vitaly Kuznetsov]

Dexuan Cui <decui@microsoft.com> writes:

>> From: Vitaly Kuznetsov <vkuznets@redhat.com>
>> Sent: Thursday, September 26, 2019 3:44 AM
>> > [...]
>> > +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);
>> > +
>> 
>> (trying to think out loud, not sure there's a real issue):
>> 
>> When PV IPIs (or PV TLB flush) are enabled we do the following checks:
>> 
>> 	if (!hv_hypercall_pg)
>> 		return false;
>> 
>> or
>>         if (!hv_hypercall_pg)
>>                 goto do_native;
>> 
>> which will pass as we're not invalidating the pointer. Can we actually
>> be sure that the kernel will never try to send an IPI/do TLB flush
>> before we resume?
>> 
>> Vitaly
>
> When hv_suspend() and hv_resume() are called by syscore_suspend()
> and syscore_resume(), respectively, all the non-boot CPUs are disabled and
> only CPU0 is active and interrupts are disabled, e.g. see
>
> hibernate() -> 
>   hibernation_snapshot() ->
>     create_image() ->
>       suspend_disable_secondary_cpus()
>       local_irq_disable()
>
>       syscore_suspend()
>       swsusp_arch_suspend
>       syscore_resume
>
>       local_irq_enable
>       enable_nonboot_cpus
>
>
> So, I'm pretty sure no IPI can happen between hv_suspend() and hv_resume().
> self-IPI is not supposed to happen either, since interrupts are disabled.
>
> IMO TLB flush should not be an issue either, unless the kernel changes page
> tables between hv_suspend() and hv_resume(), which is not the case as I
> checked the related code, but it looks in theory that might happen, say, in
> the future, so if you insist we should save the variable "hv_hypercall_pg"
> to a temporary variable and set the "hv_hypercall_pg" to NULL before we
> disable the hypercall page

Let's do it as a future proof so we can keep relying on !hv_hypercall_pg
everywhere we need. No need to change this patch IMO, a follow-up would
do.

-- 
Vitaly

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

[Dexuan Cui]

> From: Vitaly Kuznetsov <vkuznets@redhat.com>
> Sent: Friday, September 27, 2019 2:05 AM
> To: Dexuan Cui <decui@microsoft.com>
> 
> Dexuan Cui <decui@microsoft.com> writes:
> ...
> > So, I'm pretty sure no IPI can happen between hv_suspend() and
> hv_resume().
> > self-IPI is not supposed to happen either, since interrupts are disabled.
> >
> > IMO TLB flush should not be an issue either, unless the kernel changes page
> > tables between hv_suspend() and hv_resume(), which is not the case as I
> > checked the related code, but it looks in theory that might happen, say, in
> > the future, so if you insist we should save the variable "hv_hypercall_pg"
> > to a temporary variable and set the "hv_hypercall_pg" to NULL before we
> > disable the hypercall page
> 
> Let's do it as a future proof so we can keep relying on !hv_hypercall_pg
> everywhere we need. No need to change this patch IMO, a follow-up would
> do.
> Vitaly

Now I think it would be better to do it in this patch. :-)
I'll post a v6 to follow your suggestion.

Thanks,
-- Dexuan