Linux-PM Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2 00/11] Fix PM hibernation in Xen guests
@ 2020-07-02 18:21 Anchal Agarwal
  2020-07-02 18:21 ` [PATCH v2 01/11] xen/manage: keep track of the on-going suspend mode Anchal Agarwal
                   ` (11 more replies)
  0 siblings, 12 replies; 44+ messages in thread
From: Anchal Agarwal @ 2020-07-02 18:21 UTC (permalink / raw)
  To: tglx, mingo, bp, hpa, x86, boris.ostrovsky, jgross, linux-pm,
	linux-mm, kamatam, sstabellini, konrad.wilk, roger.pau, axboe,
	davem, rjw, len.brown, pavel, peterz, eduval, sblbir, anchalag,
	xen-devel, vkuznets, netdev, linux-kernel, dwmw, benh

Hello,
This series fixes PM hibernation for hvm guests running on xen hypervisor.
The running guest could now be hibernated and resumed successfully at a
later time. The fixes for PM hibernation are added to block and
network device drivers i.e xen-blkfront and xen-netfront. Any other driver
that needs to add S4 support if not already, can follow same method of
introducing freeze/thaw/restore callbacks.
The patches had been tested against upstream kernel and xen4.11. Large
scale testing is also done on Xen based Amazon EC2 instances. All this testing
involved running memory exhausting workload in the background.

Doing guest hibernation does not involve any support from hypervisor and
this way guest has complete control over its state. Infrastructure
restrictions for saving up guest state can be overcome by guest initiated
hibernation.

These patches were send out as RFC before and all the feedback had been
incorporated in the patches. The last v1 could be found here:

[v1]: https://lkml.org/lkml/2020/5/19/1312
All comments and feedback from v1 had been incorporated in v2 series.
Any comments/suggestions are welcome

Known issues:
1.KASLR causes intermittent hibernation failures. VM fails to resumes and
has to be restarted. I will investigate this issue separately and shouldn't
be a blocker for this patch series.
2. During hibernation, I observed sometimes that freezing of tasks fails due
to busy XFS workqueuei[xfs-cil/xfs-sync]. This is also intermittent may be 1
out of 200 runs and hibernation is aborted in this case. Re-trying hibernation
may work. Also, this is a known issue with hibernation and some
filesystems like XFS has been discussed by the community for years with not an
effectve resolution at this point.

Testing How to:
---------------
1. Setup xen hypervisor on a physical machine[ I used Ubuntu 16.04 +upstream
xen-4.11]
2. Bring up a HVM guest w/t kernel compiled with hibernation patches
[I used ubuntu18.04 netboot bionic images and also Amazon Linux on-prem images].
3. Create a swap file size=RAM size
4. Update grub parameters and reboot
5. Trigger pm-hibernation from within the VM

Example:
Set up a file-backed swap space. Swap file size>=Total memory on the system
sudo dd if=/dev/zero of=/swap bs=$(( 1024 * 1024 )) count=4096 # 4096MiB
sudo chmod 600 /swap
sudo mkswap /swap
sudo swapon /swap

Update resume device/resume offset in grub if using swap file:
resume=/dev/xvda1 resume_offset=200704 no_console_suspend=1

Execute:
--------
sudo pm-hibernate
OR
echo disk > /sys/power/state && echo reboot > /sys/power/disk

Compute resume offset code:
"
#!/usr/bin/env python
import sys
import array
import fcntl

#swap file
f = open(sys.argv[1], 'r')
buf = array.array('L', [0])

#FIBMAP
ret = fcntl.ioctl(f.fileno(), 0x01, buf)
print buf[0]
"


Aleksei Besogonov (1):
  PM / hibernate: update the resume offset on SNAPSHOT_SET_SWAP_AREA

Anchal Agarwal (4):
  x86/xen: Introduce new function to map HYPERVISOR_shared_info on
    Resume
  x86/xen: save and restore steal clock during PM hibernation
  xen: Introduce wrapper for save/restore sched clock offset
  xen: Update sched clock offset to avoid system instability in
    hibernation

Munehisa Kamata (5):
  xen/manage: keep track of the on-going suspend mode
  xenbus: add freeze/thaw/restore callbacks support
  x86/xen: add system core suspend and resume callbacks
  xen-blkfront: add callbacks for PM suspend and hibernation
  xen-netfront: add callbacks for PM suspend and hibernation

Thomas Gleixner (1):
  genirq: Shutdown irq chips in suspend/resume during hibernation

 arch/x86/xen/enlighten_hvm.c      |   7 ++
 arch/x86/xen/suspend.c            |  53 +++++++++++++
 arch/x86/xen/time.c               |  15 +++-
 arch/x86/xen/xen-ops.h            |   3 +
 drivers/block/xen-blkfront.c      | 122 +++++++++++++++++++++++++++++-
 drivers/net/xen-netfront.c        |  98 +++++++++++++++++++++++-
 drivers/xen/events/events_base.c  |   1 +
 drivers/xen/manage.c              |  60 +++++++++++++++
 drivers/xen/xenbus/xenbus_probe.c |  96 +++++++++++++++++++----
 include/linux/irq.h               |   2 +
 include/xen/xen-ops.h             |   3 +
 include/xen/xenbus.h              |   3 +
 kernel/irq/chip.c                 |   2 +-
 kernel/irq/internals.h            |   1 +
 kernel/irq/pm.c                   |  31 +++++---
 kernel/power/user.c               |   6 +-
 16 files changed, 470 insertions(+), 33 deletions(-)

-- 
2.20.1


^ permalink raw reply	[flat|nested] 44+ messages in thread

* [PATCH v2 01/11] xen/manage: keep track of the on-going suspend mode
  2020-07-02 18:21 [PATCH v2 00/11] Fix PM hibernation in Xen guests Anchal Agarwal
@ 2020-07-02 18:21 ` Anchal Agarwal
  2020-07-13 15:52   ` Boris Ostrovsky
  2020-07-02 18:21 ` [PATCH v2 03/11] x86/xen: Introduce new function to map HYPERVISOR_shared_info on Resume Anchal Agarwal
                   ` (10 subsequent siblings)
  11 siblings, 1 reply; 44+ messages in thread
From: Anchal Agarwal @ 2020-07-02 18:21 UTC (permalink / raw)
  To: tglx, mingo, bp, hpa, x86, boris.ostrovsky, jgross, linux-pm,
	linux-mm, kamatam, sstabellini, konrad.wilk, roger.pau, axboe,
	davem, rjw, len.brown, pavel, peterz, eduval, sblbir, anchalag,
	xen-devel, vkuznets, netdev, linux-kernel, dwmw, benh

From: Munehisa Kamata <kamatam@amazon.com>

Guest hibernation is different from xen suspend/resume/live migration.
Xen save/restore does not use pm_ops as is needed by guest hibernation.
Hibernation in guest follows ACPI path and is guest inititated , the
hibernation image is saved within guest as compared to later modes
which are xen toolstack assisted and image creation/storage is in
control of hypervisor/host machine.
To differentiate between Xen suspend and PM hibernation, keep track
of the on-going suspend mode by mainly using a new PM notifier.
Introduce simple functions which help to know the on-going suspend mode
so that other Xen-related code can behave differently according to the
current suspend mode.
Since Xen suspend doesn't have corresponding PM event, its main logic
is modfied to acquire pm_mutex and set the current mode.

Though, acquirng pm_mutex is still right thing to do, we may
see deadlock if PM hibernation is interrupted by Xen suspend.
PM hibernation depends on xenwatch thread to process xenbus state
transactions, but the thread will sleep to wait pm_mutex which is
already held by PM hibernation context in the scenario. Xen shutdown
code may need some changes to avoid the issue.

[Anchal Agarwal: Changelog]:
 RFC v1->v2: Code refactoring
 v1->v2:     Remove unused functions for PM SUSPEND/PM hibernation

Signed-off-by: Anchal Agarwal <anchalag@amazon.com>
Signed-off-by: Munehisa Kamata <kamatam@amazon.com>
---
 drivers/xen/manage.c  | 60 +++++++++++++++++++++++++++++++++++++++++++
 include/xen/xen-ops.h |  1 +
 2 files changed, 61 insertions(+)

diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
index cd046684e0d1..69833fd6cfd1 100644
--- a/drivers/xen/manage.c
+++ b/drivers/xen/manage.c
@@ -14,6 +14,7 @@
 #include <linux/freezer.h>
 #include <linux/syscore_ops.h>
 #include <linux/export.h>
+#include <linux/suspend.h>
 
 #include <xen/xen.h>
 #include <xen/xenbus.h>
@@ -40,6 +41,20 @@ enum shutdown_state {
 /* Ignore multiple shutdown requests. */
 static enum shutdown_state shutting_down = SHUTDOWN_INVALID;
 
+enum suspend_modes {
+	NO_SUSPEND = 0,
+	XEN_SUSPEND,
+	PM_HIBERNATION,
+};
+
+/* Protected by pm_mutex */
+static enum suspend_modes suspend_mode = NO_SUSPEND;
+
+bool xen_is_xen_suspend(void)
+{
+	return suspend_mode == XEN_SUSPEND;
+}
+
 struct suspend_info {
 	int cancelled;
 };
@@ -99,6 +114,10 @@ static void do_suspend(void)
 	int err;
 	struct suspend_info si;
 
+	lock_system_sleep();
+
+	suspend_mode = XEN_SUSPEND;
+
 	shutting_down = SHUTDOWN_SUSPEND;
 
 	err = freeze_processes();
@@ -162,6 +181,10 @@ static void do_suspend(void)
 	thaw_processes();
 out:
 	shutting_down = SHUTDOWN_INVALID;
+
+	suspend_mode = NO_SUSPEND;
+
+	unlock_system_sleep();
 }
 #endif	/* CONFIG_HIBERNATE_CALLBACKS */
 
@@ -387,3 +410,40 @@ int xen_setup_shutdown_event(void)
 EXPORT_SYMBOL_GPL(xen_setup_shutdown_event);
 
 subsys_initcall(xen_setup_shutdown_event);
+
+static int xen_pm_notifier(struct notifier_block *notifier,
+			unsigned long pm_event, void *unused)
+{
+	switch (pm_event) {
+	case PM_SUSPEND_PREPARE:
+	case PM_HIBERNATION_PREPARE:
+	case PM_RESTORE_PREPARE:
+		suspend_mode = PM_HIBERNATION;
+		break;
+	case PM_POST_SUSPEND:
+	case PM_POST_RESTORE:
+	case PM_POST_HIBERNATION:
+		/* Set back to the default */
+		suspend_mode = NO_SUSPEND;
+		break;
+	default:
+		pr_warn("Receive unknown PM event 0x%lx\n", pm_event);
+		return -EINVAL;
+	}
+
+	return 0;
+};
+
+static struct notifier_block xen_pm_notifier_block = {
+	.notifier_call = xen_pm_notifier
+};
+
+static int xen_setup_pm_notifier(void)
+{
+	if (!xen_hvm_domain())
+		return -ENODEV;
+
+	return register_pm_notifier(&xen_pm_notifier_block);
+}
+
+subsys_initcall(xen_setup_pm_notifier);
diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
index 39a5580f8feb..2521d6a306cd 100644
--- a/include/xen/xen-ops.h
+++ b/include/xen/xen-ops.h
@@ -40,6 +40,7 @@ u64 xen_steal_clock(int cpu);
 
 int xen_setup_shutdown_event(void);
 
+bool xen_is_xen_suspend(void);
 extern unsigned long *xen_contiguous_bitmap;
 
 #if defined(CONFIG_XEN_PV) || defined(CONFIG_ARM) || defined(CONFIG_ARM64)
-- 
2.20.1


^ permalink raw reply	[flat|nested] 44+ messages in thread

* [PATCH v2 03/11] x86/xen: Introduce new function to map HYPERVISOR_shared_info on Resume
  2020-07-02 18:21 [PATCH v2 00/11] Fix PM hibernation in Xen guests Anchal Agarwal
  2020-07-02 18:21 ` [PATCH v2 01/11] xen/manage: keep track of the on-going suspend mode Anchal Agarwal
@ 2020-07-02 18:21 ` Anchal Agarwal
  2020-07-02 18:22 ` [PATCH v2 04/11] x86/xen: add system core suspend and resume callbacks Anchal Agarwal
                   ` (9 subsequent siblings)
  11 siblings, 0 replies; 44+ messages in thread
From: Anchal Agarwal @ 2020-07-02 18:21 UTC (permalink / raw)
  To: tglx, mingo, bp, hpa, x86, boris.ostrovsky, jgross, linux-pm,
	linux-mm, kamatam, sstabellini, konrad.wilk, roger.pau, axboe,
	davem, rjw, len.brown, pavel, peterz, eduval, sblbir, anchalag,
	xen-devel, vkuznets, netdev, linux-kernel, dwmw, benh

Introduce a small function which re-uses shared page's PA allocated
during guest initialization time in reserve_shared_info() and not
allocate new page during resume flow.
It also  does the mapping of shared_info_page by calling
xen_hvm_init_shared_info() to use the function.

Changelog:
v1->v2: Remove extra check for shared_info_pfn to be NULL

Signed-off-by: Anchal Agarwal <anchalag@amazon.com>
---
 arch/x86/xen/enlighten_hvm.c | 6 ++++++
 arch/x86/xen/xen-ops.h       | 1 +
 2 files changed, 7 insertions(+)

diff --git a/arch/x86/xen/enlighten_hvm.c b/arch/x86/xen/enlighten_hvm.c
index 3e89b0067ff0..d91099928746 100644
--- a/arch/x86/xen/enlighten_hvm.c
+++ b/arch/x86/xen/enlighten_hvm.c
@@ -28,6 +28,12 @@
 
 static unsigned long shared_info_pfn;
 
+void xen_hvm_map_shared_info(void)
+{
+	xen_hvm_init_shared_info();
+	HYPERVISOR_shared_info = __va(PFN_PHYS(shared_info_pfn));
+}
+
 void xen_hvm_init_shared_info(void)
 {
 	struct xen_add_to_physmap xatp;
diff --git a/arch/x86/xen/xen-ops.h b/arch/x86/xen/xen-ops.h
index 53b224fd6177..41e9e9120f2d 100644
--- a/arch/x86/xen/xen-ops.h
+++ b/arch/x86/xen/xen-ops.h
@@ -54,6 +54,7 @@ void xen_enable_sysenter(void);
 void xen_enable_syscall(void);
 void xen_vcpu_restore(void);
 
+void xen_hvm_map_shared_info(void);
 void xen_hvm_init_shared_info(void);
 void xen_unplug_emulated_devices(void);
 
-- 
2.20.1


^ permalink raw reply	[flat|nested] 44+ messages in thread

* [PATCH v2 04/11] x86/xen: add system core suspend and resume callbacks
  2020-07-02 18:21 [PATCH v2 00/11] Fix PM hibernation in Xen guests Anchal Agarwal
  2020-07-02 18:21 ` [PATCH v2 01/11] xen/manage: keep track of the on-going suspend mode Anchal Agarwal
  2020-07-02 18:21 ` [PATCH v2 03/11] x86/xen: Introduce new function to map HYPERVISOR_shared_info on Resume Anchal Agarwal
@ 2020-07-02 18:22 ` Anchal Agarwal
  2020-07-05 17:22   ` kernel test robot
  2020-07-22  9:08   ` Julien Grall
  2020-07-02 18:22 ` [PATCH v2 05/11] genirq: Shutdown irq chips in suspend/resume during hibernation Anchal Agarwal
                   ` (8 subsequent siblings)
  11 siblings, 2 replies; 44+ messages in thread
From: Anchal Agarwal @ 2020-07-02 18:22 UTC (permalink / raw)
  To: tglx, mingo, bp, hpa, x86, boris.ostrovsky, jgross, linux-pm,
	linux-mm, kamatam, sstabellini, konrad.wilk, roger.pau, axboe,
	davem, rjw, len.brown, pavel, peterz, eduval, sblbir, anchalag,
	xen-devel, vkuznets, netdev, linux-kernel, dwmw, benh

From: Munehisa Kamata <kamatam@amazon.com>

Add Xen PVHVM specific system core callbacks for PM
hibernation support. The callbacks suspend and resume
Xen primitives like shared_info, pvclock and grant table.
These syscore_ops are specifically for domU hibernation.
xen_suspend() calls syscore_suspend() during Xen suspend
operation however, during xen suspend lock_system_sleep()
lock is taken and thus system cannot trigger hibernation.
These system core callbacks will be called only from the
hibernation context.

[Anchal Agarwal: Changelog]:
v1->v2: Edit commit message
        Fixed syscore_suspend() to call gnntab_suspend
	Removed suspend mode check in syscore_suspend()/
	syscore_resume()
Signed-off-by: Agarwal Anchal <anchalag@amazon.com>
Signed-off-by: Munehisa Kamata <kamatam@amazon.com>
---
 arch/x86/xen/enlighten_hvm.c |  1 +
 arch/x86/xen/suspend.c       | 47 ++++++++++++++++++++++++++++++++++++
 include/xen/xen-ops.h        |  2 ++
 3 files changed, 50 insertions(+)

diff --git a/arch/x86/xen/enlighten_hvm.c b/arch/x86/xen/enlighten_hvm.c
index d91099928746..bd6bf6eb2052 100644
--- a/arch/x86/xen/enlighten_hvm.c
+++ b/arch/x86/xen/enlighten_hvm.c
@@ -215,6 +215,7 @@ static void __init xen_hvm_guest_init(void)
 	if (xen_feature(XENFEAT_hvm_callback_vector))
 		xen_have_vector_callback = 1;
 
+	xen_setup_syscore_ops();
 	xen_hvm_smp_init();
 	WARN_ON(xen_cpuhp_setup(xen_cpu_up_prepare_hvm, xen_cpu_dead_hvm));
 	xen_unplug_emulated_devices();
diff --git a/arch/x86/xen/suspend.c b/arch/x86/xen/suspend.c
index 1d83152c761b..e8c924e93fc5 100644
--- a/arch/x86/xen/suspend.c
+++ b/arch/x86/xen/suspend.c
@@ -2,17 +2,22 @@
 #include <linux/types.h>
 #include <linux/tick.h>
 #include <linux/percpu-defs.h>
+#include <linux/syscore_ops.h>
+#include <linux/kernel_stat.h>
 
 #include <xen/xen.h>
 #include <xen/interface/xen.h>
+#include <xen/interface/memory.h>
 #include <xen/grant_table.h>
 #include <xen/events.h>
+#include <xen/xen-ops.h>
 
 #include <asm/cpufeatures.h>
 #include <asm/msr-index.h>
 #include <asm/xen/hypercall.h>
 #include <asm/xen/page.h>
 #include <asm/fixmap.h>
+#include <asm/pvclock.h>
 
 #include "xen-ops.h"
 #include "mmu.h"
@@ -82,3 +87,45 @@ void xen_arch_suspend(void)
 
 	on_each_cpu(xen_vcpu_notify_suspend, NULL, 1);
 }
+
+static int xen_syscore_suspend(void)
+{
+	struct xen_remove_from_physmap xrfp;
+	int ret;
+
+	gnttab_suspend();
+
+	xrfp.domid = DOMID_SELF;
+	xrfp.gpfn = __pa(HYPERVISOR_shared_info) >> PAGE_SHIFT;
+
+	ret = HYPERVISOR_memory_op(XENMEM_remove_from_physmap, &xrfp);
+	if (!ret)
+		HYPERVISOR_shared_info = &xen_dummy_shared_info;
+
+	return ret;
+}
+
+static void xen_syscore_resume(void)
+{
+	/* No need to setup vcpu_info as it's already moved off */
+	xen_hvm_map_shared_info();
+
+	pvclock_resume();
+
+	gnttab_resume();
+}
+
+/*
+ * These callbacks will be called with interrupts disabled and when having only
+ * one CPU online.
+ */
+static struct syscore_ops xen_hvm_syscore_ops = {
+	.suspend = xen_syscore_suspend,
+	.resume = xen_syscore_resume
+};
+
+void __init xen_setup_syscore_ops(void)
+{
+	if (xen_hvm_domain())
+		register_syscore_ops(&xen_hvm_syscore_ops);
+}
diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
index 2521d6a306cd..9fa8a4082d68 100644
--- a/include/xen/xen-ops.h
+++ b/include/xen/xen-ops.h
@@ -41,6 +41,8 @@ u64 xen_steal_clock(int cpu);
 int xen_setup_shutdown_event(void);
 
 bool xen_is_xen_suspend(void);
+void xen_setup_syscore_ops(void);
+
 extern unsigned long *xen_contiguous_bitmap;
 
 #if defined(CONFIG_XEN_PV) || defined(CONFIG_ARM) || defined(CONFIG_ARM64)
-- 
2.20.1


^ permalink raw reply	[flat|nested] 44+ messages in thread

* [PATCH v2 05/11] genirq: Shutdown irq chips in suspend/resume during hibernation
  2020-07-02 18:21 [PATCH v2 00/11] Fix PM hibernation in Xen guests Anchal Agarwal
                   ` (2 preceding siblings ...)
  2020-07-02 18:22 ` [PATCH v2 04/11] x86/xen: add system core suspend and resume callbacks Anchal Agarwal
@ 2020-07-02 18:22 ` Anchal Agarwal
  2020-07-02 18:22 ` [PATCH v2 06/11] xen-blkfront: add callbacks for PM suspend and hibernation Anchal Agarwal
                   ` (7 subsequent siblings)
  11 siblings, 0 replies; 44+ messages in thread
From: Anchal Agarwal @ 2020-07-02 18:22 UTC (permalink / raw)
  To: tglx, mingo, bp, hpa, x86, boris.ostrovsky, jgross, linux-pm,
	linux-mm, kamatam, sstabellini, konrad.wilk, roger.pau, axboe,
	davem, rjw, len.brown, pavel, peterz, eduval, sblbir, anchalag,
	xen-devel, vkuznets, netdev, linux-kernel, dwmw, benh

From: Thomas Gleixner <tglx@linutronix.de>

Many legacy device drivers do not implement power management (PM)
functions which means that interrupts requested by these drivers stay
in active state when the kernel is hibernated.

This does not matter on bare metal and on most hypervisors because the
interrupt is restored on resume without any noticeable side effects as
it stays connected to the same physical or virtual interrupt line.

The XEN interrupt mechanism is different as it maintains a mapping
between the Linux interrupt number and a XEN event channel. If the
interrupt stays active on hibernation this mapping is preserved but
there is unfortunately no guarantee that on resume the same event
channels are reassigned to these devices. This can result in event
channel conflicts which prevent the affected devices from being
restored correctly.

One way to solve this would be to add the necessary power management
functions to all affected legacy device drivers, but that's a
questionable effort which does not provide any benefits on non-XEN
environments.

The least intrusive and most efficient solution is to provide a
mechanism which allows the core interrupt code to tear down these
interrupts on hibernation and bring them back up again on resume. This
allows the XEN event channel mechanism to assign an arbitrary event
channel on resume without affecting the functionality of these
devices.

Fortunately all these device interrupts are handled by a dedicated XEN
interrupt chip so the chip can be marked that all interrupts connected
to it are handled this way. This is pretty much in line with the other
interrupt chip specific quirks, e.g. IRQCHIP_MASK_ON_SUSPEND.

Add a new quirk flag IRQCHIP_SHUTDOWN_ON_SUSPEND and add support for
it the core interrupt suspend/resume paths.

Changelog:
RFCv2-RFCv3: Incorporated tglx@'s patch to work with xen code
v1->v2: Corrected the author's name to tglx@

Signed-off-by: Anchal Agarwal <anchalag@amazon.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/xen/events/events_base.c |  1 +
 include/linux/irq.h              |  2 ++
 kernel/irq/chip.c                |  2 +-
 kernel/irq/internals.h           |  1 +
 kernel/irq/pm.c                  | 31 ++++++++++++++++++++++---------
 5 files changed, 27 insertions(+), 10 deletions(-)

diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index 140c7bf33a98..958dea2a4916 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -1611,6 +1611,7 @@ static struct irq_chip xen_pirq_chip __read_mostly = {
 	.irq_set_affinity	= set_affinity_irq,
 
 	.irq_retrigger		= retrigger_dynirq,
+	.flags                  = IRQCHIP_SHUTDOWN_ON_SUSPEND,
 };
 
 static struct irq_chip xen_percpu_chip __read_mostly = {
diff --git a/include/linux/irq.h b/include/linux/irq.h
index 8d5bc2c237d7..94cb8c994d06 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -542,6 +542,7 @@ struct irq_chip {
  * IRQCHIP_EOI_THREADED:	Chip requires eoi() on unmask in threaded mode
  * IRQCHIP_SUPPORTS_LEVEL_MSI	Chip can provide two doorbells for Level MSIs
  * IRQCHIP_SUPPORTS_NMI:	Chip can deliver NMIs, only for root irqchips
+ * IRQCHIP_SHUTDOWN_ON_SUSPEND: Shutdown non wake irqs in the suspend path
  */
 enum {
 	IRQCHIP_SET_TYPE_MASKED		= (1 <<  0),
@@ -553,6 +554,7 @@ enum {
 	IRQCHIP_EOI_THREADED		= (1 <<  6),
 	IRQCHIP_SUPPORTS_LEVEL_MSI	= (1 <<  7),
 	IRQCHIP_SUPPORTS_NMI		= (1 <<  8),
+	IRQCHIP_SHUTDOWN_ON_SUSPEND     = (1 <<  9),
 };
 
 #include <linux/irqdesc.h>
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 41e7e37a0928..fd59489ff14b 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -233,7 +233,7 @@ __irq_startup_managed(struct irq_desc *desc, struct cpumask *aff, bool force)
 }
 #endif
 
-static int __irq_startup(struct irq_desc *desc)
+int __irq_startup(struct irq_desc *desc)
 {
 	struct irq_data *d = irq_desc_get_irq_data(desc);
 	int ret = 0;
diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
index 7db284b10ac9..b6fca5eacff7 100644
--- a/kernel/irq/internals.h
+++ b/kernel/irq/internals.h
@@ -80,6 +80,7 @@ extern void __enable_irq(struct irq_desc *desc);
 extern int irq_activate(struct irq_desc *desc);
 extern int irq_activate_and_startup(struct irq_desc *desc, bool resend);
 extern int irq_startup(struct irq_desc *desc, bool resend, bool force);
+extern int __irq_startup(struct irq_desc *desc);
 
 extern void irq_shutdown(struct irq_desc *desc);
 extern void irq_shutdown_and_deactivate(struct irq_desc *desc);
diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
index 8f557fa1f4fe..dc48a25f1756 100644
--- a/kernel/irq/pm.c
+++ b/kernel/irq/pm.c
@@ -85,16 +85,25 @@ static bool suspend_device_irq(struct irq_desc *desc)
 	}
 
 	desc->istate |= IRQS_SUSPENDED;
-	__disable_irq(desc);
-
 	/*
-	 * Hardware which has no wakeup source configuration facility
-	 * requires that the non wakeup interrupts are masked at the
-	 * chip level. The chip implementation indicates that with
-	 * IRQCHIP_MASK_ON_SUSPEND.
+	 * Some irq chips (e.g. XEN PIRQ) require a full shutdown on suspend
+	 * as some of the legacy drivers(e.g. floppy) do nothing during the
+	 * suspend path
 	 */
-	if (irq_desc_get_chip(desc)->flags & IRQCHIP_MASK_ON_SUSPEND)
-		mask_irq(desc);
+	if (irq_desc_get_chip(desc)->flags & IRQCHIP_SHUTDOWN_ON_SUSPEND) {
+		irq_shutdown(desc);
+	} else {
+		__disable_irq(desc);
+
+	       /*
+		* Hardware which has no wakeup source configuration facility
+		* requires that the non wakeup interrupts are masked at the
+		* chip level. The chip implementation indicates that with
+		* IRQCHIP_MASK_ON_SUSPEND.
+		*/
+		if (irq_desc_get_chip(desc)->flags & IRQCHIP_MASK_ON_SUSPEND)
+			mask_irq(desc);
+	}
 	return true;
 }
 
@@ -152,7 +161,11 @@ static void resume_irq(struct irq_desc *desc)
 	irq_state_set_masked(desc);
 resume:
 	desc->istate &= ~IRQS_SUSPENDED;
-	__enable_irq(desc);
+
+	if (irq_desc_get_chip(desc)->flags & IRQCHIP_SHUTDOWN_ON_SUSPEND)
+		__irq_startup(desc);
+	else
+		__enable_irq(desc);
 }
 
 static void resume_irqs(bool want_early)
-- 
2.20.1


^ permalink raw reply	[flat|nested] 44+ messages in thread

* [PATCH v2 06/11] xen-blkfront: add callbacks for PM suspend and hibernation
  2020-07-02 18:21 [PATCH v2 00/11] Fix PM hibernation in Xen guests Anchal Agarwal
                   ` (3 preceding siblings ...)
  2020-07-02 18:22 ` [PATCH v2 05/11] genirq: Shutdown irq chips in suspend/resume during hibernation Anchal Agarwal
@ 2020-07-02 18:22 ` Anchal Agarwal
  2020-07-02 18:22 ` [PATCH v2 07/11] xen-netfront: " Anchal Agarwal
                   ` (6 subsequent siblings)
  11 siblings, 0 replies; 44+ messages in thread
From: Anchal Agarwal @ 2020-07-02 18:22 UTC (permalink / raw)
  To: tglx, mingo, bp, hpa, x86, boris.ostrovsky, jgross, linux-pm,
	linux-mm, kamatam, sstabellini, konrad.wilk, roger.pau, axboe,
	davem, rjw, len.brown, pavel, peterz, eduval, sblbir, anchalag,
	xen-devel, vkuznets, netdev, linux-kernel, dwmw, benh

From: Munehisa Kamata <kamatam@amazon.com>

S4 power transisiton states are much different than xen
suspend/resume. Former is visible to the guest and frontend drivers should
be aware of the state transistions and should be able to take appropriate
actions when needed. In transition to S4 we need to make sure that at least
all the in-flight blkif requests get completed, since they probably contain
bits of the guest's memory image and that's not going to get saved any
other way. Hence, re-issuing of in-flight requests as in case of xen resume
will not work here. This is in contrast to xen-suspend where we need to
freeze with as little processing as possible to avoid dirtying RAM late in
the migration cycle and we know that in-flight data can wait.

Add freeze, thaw and restore callbacks for PM suspend and hibernation
support. All frontend drivers that needs to use PM_HIBERNATION/PM_SUSPEND
events, need to implement these xenbus_driver callbacks. The freeze handler
stops block-layer queue and disconnect the frontend from the backend while
freeing ring_info and associated resources. Before disconnecting from the
backend, we need to prevent any new IO from being queued and wait for
existing IO to complete. Freeze/unfreeze of the queues will guarantee
that there are no requests in use on the shared ring. However, for sanity
we should check state of the ring before disconnecting to make sure that
there are no outstanding requests to be processed on the ring.
The restore handler re-allocates ring_info, unquiesces and unfreezes the
queue and re-connect to the backend, so that rest of the kernel can
continue to use the block device transparently.

Note:For older backends,if a backend doesn't have commit'12ea729645ace'
xen/blkback: unmap all persistent grants when frontend gets disconnected,
the frontend may see massive amount of grant table warning when freeing
resources.
[   36.852659] deferring g.e. 0xf9 (pfn 0xffffffffffffffff)
[   36.855089] xen:grant_table: WARNING:e.g. 0x112 still in use!

In this case, persistent grants would need to be disabled.

[Anchal Agarwal: Changelog]:
RFC v1->v2: Removed timeout per request before disconnect during
	    blkfront freeze.
	    Added queue freeze/quiesce to the blkfront_freeze
	    Code cleanup
RFC v2->v3: None
RFC v3->v1: Code cleanup, Refractoring
    v1->v2: * remove err variable in blkfront_freeze
            * BugFix: error handling if rings are still busy
              after queue freeze/quiesce and returnign driver to
              connected state
            * add TODO if blkback fails to disconnect on freeze
            * Code formatting

Signed-off-by: Anchal Agarwal <anchalag@amazon.com>
Signed-off-by: Munehisa Kamata <kamatam@amazon.com>
---
 drivers/block/xen-blkfront.c | 122 +++++++++++++++++++++++++++++++++--
 1 file changed, 118 insertions(+), 4 deletions(-)

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index 3b889ea950c2..9e3ed1b9f509 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -48,6 +48,8 @@
 #include <linux/list.h>
 #include <linux/workqueue.h>
 #include <linux/sched/mm.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
 
 #include <xen/xen.h>
 #include <xen/xenbus.h>
@@ -80,6 +82,8 @@ enum blkif_state {
 	BLKIF_STATE_DISCONNECTED,
 	BLKIF_STATE_CONNECTED,
 	BLKIF_STATE_SUSPENDED,
+	BLKIF_STATE_FREEZING,
+	BLKIF_STATE_FROZEN,
 };
 
 struct grant {
@@ -219,6 +223,7 @@ struct blkfront_info
 	struct list_head requests;
 	struct bio_list bio_list;
 	struct list_head info_list;
+	struct completion wait_backend_disconnected;
 };
 
 static unsigned int nr_minors;
@@ -1005,6 +1010,7 @@ static int xlvbd_init_blk_queue(struct gendisk *gd, u16 sector_size,
 	info->sector_size = sector_size;
 	info->physical_sector_size = physical_sector_size;
 	blkif_set_queue_limits(info);
+	init_completion(&info->wait_backend_disconnected);
 
 	return 0;
 }
@@ -1353,6 +1359,8 @@ static void blkif_free(struct blkfront_info *info, int suspend)
 	unsigned int i;
 	struct blkfront_ring_info *rinfo;
 
+	if (info->connected == BLKIF_STATE_FREEZING)
+		goto free_rings;
 	/* Prevent new requests being issued until we fix things up. */
 	info->connected = suspend ?
 		BLKIF_STATE_SUSPENDED : BLKIF_STATE_DISCONNECTED;
@@ -1360,6 +1368,7 @@ static void blkif_free(struct blkfront_info *info, int suspend)
 	if (info->rq)
 		blk_mq_stop_hw_queues(info->rq);
 
+free_rings:
 	for_each_rinfo(info, rinfo, i)
 		blkif_free_ring(rinfo);
 
@@ -1563,8 +1572,10 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
 	struct blkfront_ring_info *rinfo = (struct blkfront_ring_info *)dev_id;
 	struct blkfront_info *info = rinfo->dev_info;
 
-	if (unlikely(info->connected != BLKIF_STATE_CONNECTED))
+	if (unlikely(info->connected != BLKIF_STATE_CONNECTED &&
+			info->connected != BLKIF_STATE_FREEZING)) {
 		return IRQ_HANDLED;
+	}
 
 	spin_lock_irqsave(&rinfo->ring_lock, flags);
  again:
@@ -2026,6 +2037,7 @@ static int blkif_recover(struct blkfront_info *info)
 	struct bio *bio;
 	unsigned int segs;
 	struct blkfront_ring_info *rinfo;
+	bool frozen = info->connected == BLKIF_STATE_FROZEN;
 
 	blkfront_gather_backend_features(info);
 	/* Reset limits changed by blk_mq_update_nr_hw_queues(). */
@@ -2048,6 +2060,9 @@ static int blkif_recover(struct blkfront_info *info)
 		kick_pending_request_queues(rinfo);
 	}
 
+	if (frozen)
+		return 0;
+
 	list_for_each_entry_safe(req, n, &info->requests, queuelist) {
 		/* Requeue pending requests (flush or discard) */
 		list_del_init(&req->queuelist);
@@ -2364,6 +2379,7 @@ static void blkfront_connect(struct blkfront_info *info)
 
 		return;
 	case BLKIF_STATE_SUSPENDED:
+	case BLKIF_STATE_FROZEN:
 		/*
 		 * If we are recovering from suspension, we need to wait
 		 * for the backend to announce it's features before
@@ -2481,12 +2497,37 @@ static void blkback_changed(struct xenbus_device *dev,
 		break;
 
 	case XenbusStateClosed:
-		if (dev->state == XenbusStateClosed)
+		if (dev->state == XenbusStateClosed) {
+			if (info->connected == BLKIF_STATE_FREEZING) {
+				blkif_free(info, 0);
+				info->connected = BLKIF_STATE_FROZEN;
+				complete(&info->wait_backend_disconnected);
+			}
 			break;
+		}
+		/*
+		 * We receive backend's Closed again while thawing
+		 * or restoring and it causes thawing or restoring to fail.
+		 * During blkfront_restore, backend is still in Closed state
+		 * and we receive backend as closed here while frontend's
+		 * dev->state is set to XenBusStateInitialized.
+		 * Ignore such unexpected state regardless of the backend's
+		 * state.
+		 */
+		if (info->connected == BLKIF_STATE_FROZEN) {
+			dev_dbg(&dev->dev, "Thawing/Restoring, ignore the backend's Closed state: %s",
+				dev->nodename);
+			break;
+		}
+
 		/* fall through */
 	case XenbusStateClosing:
-		if (info)
-			blkfront_closing(info);
+		if (info) {
+			if (info->connected == BLKIF_STATE_FREEZING)
+				xenbus_frontend_closed(dev);
+			else
+				blkfront_closing(info);
+		}
 		break;
 	}
 }
@@ -2630,6 +2671,76 @@ static void blkif_release(struct gendisk *disk, fmode_t mode)
 	mutex_unlock(&blkfront_mutex);
 }
 
+static int blkfront_freeze(struct xenbus_device *dev)
+{
+	unsigned int i;
+	struct blkfront_info *info = dev_get_drvdata(&dev->dev);
+	struct blkfront_ring_info *rinfo;
+	/* This would be reasonable timeout as used in xenbus_dev_shutdown() */
+	unsigned int timeout = 5 * HZ;
+	unsigned long flags;
+
+	info->connected = BLKIF_STATE_FREEZING;
+
+	blk_mq_freeze_queue(info->rq);
+	blk_mq_quiesce_queue(info->rq);
+
+	for_each_rinfo(info, rinfo, i) {
+		/* No more gnttab callback work. */
+		gnttab_cancel_free_callback(&rinfo->callback);
+		/* Flush gnttab callback work. Must be done with no locks held. */
+		flush_work(&rinfo->work);
+	}
+
+	for_each_rinfo(info, rinfo, i) {
+		spin_lock_irqsave(&rinfo->ring_lock, flags);
+		if (RING_FULL(&rinfo->ring) ||
+			RING_HAS_UNCONSUMED_RESPONSES(&rinfo->ring)) {
+			spin_unlock_irqrestore(&rinfo->ring_lock, flags);
+			xenbus_dev_error(dev, -EBUSY, "Hibernation Failed. The ring is still busy");
+			info->connected = BLKIF_STATE_CONNECTED;
+			blk_mq_unquiesce_queue(info->rq);
+			blk_mq_unfreeze_queue(info->rq);
+			return -EBUSY;
+		}
+		spin_unlock_irqrestore(&rinfo->ring_lock, flags);
+	}
+	/* Kick the backend to disconnect */
+	xenbus_switch_state(dev, XenbusStateClosing);
+
+	/*
+	 * We don't want to move forward before the frontend is diconnected
+	 * from the backend cleanly.
+	 * TODO:Handle timeout by falling back to the normal
+	 * disconnect path and just wait for the backend to close before
+	 * reconnecting. Bring the system back to its original state by
+	 * failing hibernation gracefully.
+	 */
+	timeout = wait_for_completion_timeout(&info->wait_backend_disconnected,
+						timeout);
+	if (!timeout) {
+		xenbus_dev_error(dev, -EBUSY, "Freezing timed out;"
+			"the device may become inconsistent state");
+		return -EBUSY;
+	}
+
+	return 0;
+}
+
+static int blkfront_restore(struct xenbus_device *dev)
+{
+	struct blkfront_info *info = dev_get_drvdata(&dev->dev);
+	int err;
+
+	err = talk_to_blkback(dev, info);
+	if (!err) {
+		blk_mq_update_nr_hw_queues(&info->tag_set, info->nr_rings);
+		blk_mq_unquiesce_queue(info->rq);
+		blk_mq_unfreeze_queue(info->rq);
+	}
+	return err;
+}
+
 static const struct block_device_operations xlvbd_block_fops =
 {
 	.owner = THIS_MODULE,
@@ -2653,6 +2764,9 @@ static struct xenbus_driver blkfront_driver = {
 	.resume = blkfront_resume,
 	.otherend_changed = blkback_changed,
 	.is_ready = blkfront_is_ready,
+	.freeze = blkfront_freeze,
+	.thaw = blkfront_restore,
+	.restore = blkfront_restore
 };
 
 static void purge_persistent_grants(struct blkfront_info *info)
-- 
2.20.1


^ permalink raw reply	[flat|nested] 44+ messages in thread

* [PATCH v2 07/11] xen-netfront: add callbacks for PM suspend and hibernation
  2020-07-02 18:21 [PATCH v2 00/11] Fix PM hibernation in Xen guests Anchal Agarwal
                   ` (4 preceding siblings ...)
  2020-07-02 18:22 ` [PATCH v2 06/11] xen-blkfront: add callbacks for PM suspend and hibernation Anchal Agarwal
@ 2020-07-02 18:22 ` Anchal Agarwal
  2020-07-02 18:22 ` [PATCH v2 08/11] x86/xen: save and restore steal clock during PM hibernation Anchal Agarwal
                   ` (5 subsequent siblings)
  11 siblings, 0 replies; 44+ messages in thread
From: Anchal Agarwal @ 2020-07-02 18:22 UTC (permalink / raw)
  To: tglx, mingo, bp, hpa, x86, boris.ostrovsky, jgross, linux-pm,
	linux-mm, kamatam, sstabellini, konrad.wilk, roger.pau, axboe,
	davem, rjw, len.brown, pavel, peterz, eduval, sblbir, anchalag,
	xen-devel, vkuznets, netdev, linux-kernel, dwmw, benh

From: Munehisa Kamata <kamatam@amazon.com>

Add freeze, thaw and restore callbacks for PM suspend and hibernation
support. The freeze handler simply disconnects the frotnend from the
backend and frees resources associated with queues after disabling the
net_device from the system. The restore handler just changes the
frontend state and let the xenbus handler to re-allocate the resources
and re-connect to the backend. This can be performed transparently to
the rest of the system. The handlers are used for both PM suspend and
hibernation so that we can keep the existing suspend/resume callbacks
for Xen suspend without modification. Freezing netfront devices is
normally expected to finish within a few hundred milliseconds, but it
can rarely take more than 5 seconds and hit the hard coded timeout,
it would depend on backend state which may be congested and/or have
complex configuration. While it's rare case, longer default timeout
seems a bit more reasonable here to avoid hitting the timeout.
Also, make it configurable via module parameter so that we can cover
broader setups than what we know currently.

[Anchal Agarwal: Changelog]:
RFCv1->RFCv2: Variable name fix and checkpatch.pl fixes]

Signed-off-by: Anchal Agarwal <anchalag@amazon.com>
Signed-off-by: Munehisa Kamata <kamatam@amazon.com>
---
 drivers/net/xen-netfront.c | 98 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 97 insertions(+), 1 deletion(-)

diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index 482c6c8b0fb7..65edcdd6e05f 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -43,6 +43,7 @@
 #include <linux/moduleparam.h>
 #include <linux/mm.h>
 #include <linux/slab.h>
+#include <linux/completion.h>
 #include <net/ip.h>
 
 #include <xen/xen.h>
@@ -56,6 +57,12 @@
 #include <xen/interface/memory.h>
 #include <xen/interface/grant_table.h>
 
+enum netif_freeze_state {
+	NETIF_FREEZE_STATE_UNFROZEN,
+	NETIF_FREEZE_STATE_FREEZING,
+	NETIF_FREEZE_STATE_FROZEN,
+};
+
 /* Module parameters */
 #define MAX_QUEUES_DEFAULT 8
 static unsigned int xennet_max_queues;
@@ -63,6 +70,12 @@ module_param_named(max_queues, xennet_max_queues, uint, 0644);
 MODULE_PARM_DESC(max_queues,
 		 "Maximum number of queues per virtual interface");
 
+static unsigned int netfront_freeze_timeout_secs = 10;
+module_param_named(freeze_timeout_secs,
+		   netfront_freeze_timeout_secs, uint, 0644);
+MODULE_PARM_DESC(freeze_timeout_secs,
+		 "timeout when freezing netfront device in seconds");
+
 static const struct ethtool_ops xennet_ethtool_ops;
 
 struct netfront_cb {
@@ -160,6 +173,10 @@ struct netfront_info {
 	struct netfront_stats __percpu *tx_stats;
 
 	atomic_t rx_gso_checksum_fixup;
+
+	int freeze_state;
+
+	struct completion wait_backend_disconnected;
 };
 
 struct netfront_rx_info {
@@ -721,6 +738,21 @@ static int xennet_close(struct net_device *dev)
 	return 0;
 }
 
+static int xennet_disable_interrupts(struct net_device *dev)
+{
+	struct netfront_info *np = netdev_priv(dev);
+	unsigned int num_queues = dev->real_num_tx_queues;
+	unsigned int queue_index;
+	struct netfront_queue *queue;
+
+	for (queue_index = 0; queue_index < num_queues; ++queue_index) {
+		queue = &np->queues[queue_index];
+		disable_irq(queue->tx_irq);
+		disable_irq(queue->rx_irq);
+	}
+	return 0;
+}
+
 static void xennet_move_rx_slot(struct netfront_queue *queue, struct sk_buff *skb,
 				grant_ref_t ref)
 {
@@ -1301,6 +1333,8 @@ static struct net_device *xennet_create_dev(struct xenbus_device *dev)
 
 	np->queues = NULL;
 
+	init_completion(&np->wait_backend_disconnected);
+
 	err = -ENOMEM;
 	np->rx_stats = netdev_alloc_pcpu_stats(struct netfront_stats);
 	if (np->rx_stats == NULL)
@@ -1794,6 +1828,50 @@ static int xennet_create_queues(struct netfront_info *info,
 	return 0;
 }
 
+static int netfront_freeze(struct xenbus_device *dev)
+{
+	struct netfront_info *info = dev_get_drvdata(&dev->dev);
+	unsigned long timeout = netfront_freeze_timeout_secs * HZ;
+	int err = 0;
+
+	xennet_disable_interrupts(info->netdev);
+
+	netif_device_detach(info->netdev);
+
+	info->freeze_state = NETIF_FREEZE_STATE_FREEZING;
+
+	/* Kick the backend to disconnect */
+	xenbus_switch_state(dev, XenbusStateClosing);
+
+	/* We don't want to move forward before the frontend is diconnected
+	 * from the backend cleanly.
+	 */
+	timeout = wait_for_completion_timeout(&info->wait_backend_disconnected,
+					      timeout);
+	if (!timeout) {
+		err = -EBUSY;
+		xenbus_dev_error(dev, err, "Freezing timed out;"
+				 "the device may become inconsistent state");
+		return err;
+	}
+
+	/* Tear down queues */
+	xennet_disconnect_backend(info);
+	xennet_destroy_queues(info);
+
+	info->freeze_state = NETIF_FREEZE_STATE_FROZEN;
+
+	return err;
+}
+
+static int netfront_restore(struct xenbus_device *dev)
+{
+	/* Kick the backend to re-connect */
+	xenbus_switch_state(dev, XenbusStateInitialising);
+
+	return 0;
+}
+
 /* Common code used when first setting up, and when resuming. */
 static int talk_to_netback(struct xenbus_device *dev,
 			   struct netfront_info *info)
@@ -1999,6 +2077,8 @@ static int xennet_connect(struct net_device *dev)
 		spin_unlock_bh(&queue->rx_lock);
 	}
 
+	np->freeze_state = NETIF_FREEZE_STATE_UNFROZEN;
+
 	return 0;
 }
 
@@ -2036,10 +2116,23 @@ static void netback_changed(struct xenbus_device *dev,
 		break;
 
 	case XenbusStateClosed:
-		if (dev->state == XenbusStateClosed)
+		if (dev->state == XenbusStateClosed) {
+		     /* dpm context is waiting for the backend */
+			if (np->freeze_state == NETIF_FREEZE_STATE_FREEZING)
+				complete(&np->wait_backend_disconnected);
 			break;
+		}
+
 		/* Fall through - Missed the backend's CLOSING state. */
 	case XenbusStateClosing:
+	       /* We may see unexpected Closed or Closing from the backend.
+		* Just ignore it not to prevent the frontend from being
+		* re-connected in the case of PM suspend or hibernation.
+		*/
+		if (np->freeze_state == NETIF_FREEZE_STATE_FROZEN &&
+		    dev->state == XenbusStateInitialising) {
+			break;
+		}
 		xenbus_frontend_closed(dev);
 		break;
 	}
@@ -2186,6 +2279,9 @@ static struct xenbus_driver netfront_driver = {
 	.probe = netfront_probe,
 	.remove = xennet_remove,
 	.resume = netfront_resume,
+	.freeze = netfront_freeze,
+	.thaw	= netfront_restore,
+	.restore = netfront_restore,
 	.otherend_changed = netback_changed,
 };
 
-- 
2.20.1


^ permalink raw reply	[flat|nested] 44+ messages in thread

* [PATCH v2 08/11] x86/xen: save and restore steal clock during PM hibernation
  2020-07-02 18:21 [PATCH v2 00/11] Fix PM hibernation in Xen guests Anchal Agarwal
                   ` (5 preceding siblings ...)
  2020-07-02 18:22 ` [PATCH v2 07/11] xen-netfront: " Anchal Agarwal
@ 2020-07-02 18:22 ` Anchal Agarwal
  2020-07-02 18:23 ` [PATCH v2 09/11] xen: Introduce wrapper for save/restore sched clock offset Anchal Agarwal
                   ` (4 subsequent siblings)
  11 siblings, 0 replies; 44+ messages in thread
From: Anchal Agarwal @ 2020-07-02 18:22 UTC (permalink / raw)
  To: tglx, mingo, bp, hpa, x86, boris.ostrovsky, jgross, linux-pm,
	linux-mm, kamatam, sstabellini, konrad.wilk, roger.pau, axboe,
	davem, rjw, len.brown, pavel, peterz, eduval, sblbir, anchalag,
	xen-devel, vkuznets, netdev, linux-kernel, dwmw, benh

Save/restore steal times in syscore suspend/resume during PM
hibernation. Commit '5e25f5db6abb9: ("xen/time: do not
decrease steal time after live migration on xen")' fixes xen
guest steal time handling during migration. A similar issue is seen
during PM hibernation.
Currently, steal time accounting code in scheduler expects steal clock
callback to provide monotonically increasing value. If the accounting
code receives a smaller value than previous one, it uses a negative
value to calculate steal time and results in incorrectly updated idle
and steal time accounting. This breaks userspace tools which read
/proc/stat.

top - 08:05:35 up  2:12,  3 users,  load average: 0.00, 0.07, 0.23
Tasks:  80 total,   1 running,  79 sleeping,   0 stopped,   0 zombie
Cpu(s):  0.0%us,  0.0%sy,  0.0%ni,30100.0%id,  0.0%wa,  0.0%hi, 0.0%si,-1253874204672.0%st

This can actually happen when a Xen PVHVM guest gets restored from
hibernation, because such a restored guest is just a fresh domain from
Xen perspective and the time information in runstate info starts over
from scratch.

Changelog:
v1->v2: Removed patches that introduced new function calls for saving/restoring
        sched clock offset and using existing ones that are used during LM

Signed-off-by: Anchal Agarwal <anchalag@amazon.com>
---
 arch/x86/xen/suspend.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/x86/xen/suspend.c b/arch/x86/xen/suspend.c
index e8c924e93fc5..10cd14326472 100644
--- a/arch/x86/xen/suspend.c
+++ b/arch/x86/xen/suspend.c
@@ -94,10 +94,9 @@ static int xen_syscore_suspend(void)
 	int ret;
 
 	gnttab_suspend();
-
+	xen_manage_runstate_time(-1);
 	xrfp.domid = DOMID_SELF;
 	xrfp.gpfn = __pa(HYPERVISOR_shared_info) >> PAGE_SHIFT;
-
 	ret = HYPERVISOR_memory_op(XENMEM_remove_from_physmap, &xrfp);
 	if (!ret)
 		HYPERVISOR_shared_info = &xen_dummy_shared_info;
@@ -111,7 +110,7 @@ static void xen_syscore_resume(void)
 	xen_hvm_map_shared_info();
 
 	pvclock_resume();
-
+	xen_manage_runstate_time(0);
 	gnttab_resume();
 }
 
-- 
2.20.1


^ permalink raw reply	[flat|nested] 44+ messages in thread

* [PATCH v2 09/11] xen: Introduce wrapper for save/restore sched clock offset
  2020-07-02 18:21 [PATCH v2 00/11] Fix PM hibernation in Xen guests Anchal Agarwal
                   ` (6 preceding siblings ...)
  2020-07-02 18:22 ` [PATCH v2 08/11] x86/xen: save and restore steal clock during PM hibernation Anchal Agarwal
@ 2020-07-02 18:23 ` Anchal Agarwal
  2020-07-02 18:23 ` [PATCH v2 10/11] xen: Update sched clock offset to avoid system instability in hibernation Anchal Agarwal
                   ` (3 subsequent siblings)
  11 siblings, 0 replies; 44+ messages in thread
From: Anchal Agarwal @ 2020-07-02 18:23 UTC (permalink / raw)
  To: tglx, mingo, bp, hpa, x86, boris.ostrovsky, jgross, linux-pm,
	linux-mm, kamatam, sstabellini, konrad.wilk, roger.pau, axboe,
	davem, rjw, len.brown, pavel, peterz, eduval, sblbir, anchalag,
	xen-devel, vkuznets, netdev, linux-kernel, dwmw, benh

Introduce wrappers for save/restore xen_sched_clock_offset to be
used by PM hibernation code to avoid system instability during resume.

Signed-off-by: Anchal Agarwal <anchalag@amazon.com>
---
 arch/x86/xen/time.c    | 15 +++++++++++++--
 arch/x86/xen/xen-ops.h |  2 ++
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
index c8897aad13cd..676950eb0cb5 100644
--- a/arch/x86/xen/time.c
+++ b/arch/x86/xen/time.c
@@ -386,12 +386,23 @@ static const struct pv_time_ops xen_time_ops __initconst = {
 static struct pvclock_vsyscall_time_info *xen_clock __read_mostly;
 static u64 xen_clock_value_saved;
 
+/*This is needed to maintain a monotonic clock value during PM hibernation */
+void xen_save_sched_clock_offset(void)
+{
+	xen_clock_value_saved = xen_clocksource_read() - xen_sched_clock_offset;
+}
+
+void xen_restore_sched_clock_offset(void)
+{
+	xen_sched_clock_offset = xen_clocksource_read() - xen_clock_value_saved;
+}
+
 void xen_save_time_memory_area(void)
 {
 	struct vcpu_register_time_memory_area t;
 	int ret;
 
-	xen_clock_value_saved = xen_clocksource_read() - xen_sched_clock_offset;
+	xen_save_sched_clock_offset();
 
 	if (!xen_clock)
 		return;
@@ -434,7 +445,7 @@ void xen_restore_time_memory_area(void)
 out:
 	/* Need pvclock_resume() before using xen_clocksource_read(). */
 	pvclock_resume();
-	xen_sched_clock_offset = xen_clocksource_read() - xen_clock_value_saved;
+	xen_restore_sched_clock_offset();
 }
 
 static void xen_setup_vsyscall_time_info(void)
diff --git a/arch/x86/xen/xen-ops.h b/arch/x86/xen/xen-ops.h
index 41e9e9120f2d..f4b78b19493b 100644
--- a/arch/x86/xen/xen-ops.h
+++ b/arch/x86/xen/xen-ops.h
@@ -70,6 +70,8 @@ void xen_save_time_memory_area(void);
 void xen_restore_time_memory_area(void);
 void xen_init_time_ops(void);
 void xen_hvm_init_time_ops(void);
+void xen_save_sched_clock_offset(void);
+void xen_restore_sched_clock_offset(void);
 
 irqreturn_t xen_debug_interrupt(int irq, void *dev_id);
 
-- 
2.20.1


^ permalink raw reply	[flat|nested] 44+ messages in thread

* [PATCH v2 10/11] xen: Update sched clock offset to avoid system instability in hibernation
  2020-07-02 18:21 [PATCH v2 00/11] Fix PM hibernation in Xen guests Anchal Agarwal
                   ` (7 preceding siblings ...)
  2020-07-02 18:23 ` [PATCH v2 09/11] xen: Introduce wrapper for save/restore sched clock offset Anchal Agarwal
@ 2020-07-02 18:23 ` Anchal Agarwal
  2020-07-02 18:23 ` [PATCH v2 11/11] PM / hibernate: update the resume offset on SNAPSHOT_SET_SWAP_AREA Anchal Agarwal
                   ` (2 subsequent siblings)
  11 siblings, 0 replies; 44+ messages in thread
From: Anchal Agarwal @ 2020-07-02 18:23 UTC (permalink / raw)
  To: tglx, mingo, bp, hpa, x86, boris.ostrovsky, jgross, linux-pm,
	linux-mm, kamatam, sstabellini, konrad.wilk, roger.pau, axboe,
	davem, rjw, len.brown, pavel, peterz, eduval, sblbir, anchalag,
	xen-devel, vkuznets, netdev, linux-kernel, dwmw, benh

Save/restore xen_sched_clock_offset in syscore suspend/resume during PM
hibernation. Commit '867cefb4cb1012: ("xen: Fix x86 sched_clock() interface
for xen")' fixes xen guest time handling during migration. A similar issue
is seen during PM hibernation when system runs CPU intensive workload.
Post resume pvclock resets the value to 0 however, xen sched_clock_offset
is never updated. System instability is seen during resume from hibernation
when system is under heavy CPU load. Since xen_sched_clock_offset is not
updated, system does not see the monotonic clock value and the scheduler
would then think that heavy CPU hog tasks need more time in CPU, causing
the system to freeze

Signed-off-by: Anchal Agarwal <anchalag@amazon.com>
---
 arch/x86/xen/suspend.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/x86/xen/suspend.c b/arch/x86/xen/suspend.c
index 10cd14326472..4d8b1d2390b9 100644
--- a/arch/x86/xen/suspend.c
+++ b/arch/x86/xen/suspend.c
@@ -95,6 +95,7 @@ static int xen_syscore_suspend(void)
 
 	gnttab_suspend();
 	xen_manage_runstate_time(-1);
+	xen_save_sched_clock_offset();
 	xrfp.domid = DOMID_SELF;
 	xrfp.gpfn = __pa(HYPERVISOR_shared_info) >> PAGE_SHIFT;
 	ret = HYPERVISOR_memory_op(XENMEM_remove_from_physmap, &xrfp);
@@ -110,6 +111,12 @@ static void xen_syscore_resume(void)
 	xen_hvm_map_shared_info();
 
 	pvclock_resume();
+	/*
+	 * Restore xen_sched_clock_offset during resume to maintain
+	 * monotonic clock value
+	 */
+	xen_restore_sched_clock_offset();
+
 	xen_manage_runstate_time(0);
 	gnttab_resume();
 }
-- 
2.20.1


^ permalink raw reply	[flat|nested] 44+ messages in thread

* [PATCH v2 11/11] PM / hibernate: update the resume offset on SNAPSHOT_SET_SWAP_AREA
  2020-07-02 18:21 [PATCH v2 00/11] Fix PM hibernation in Xen guests Anchal Agarwal
                   ` (8 preceding siblings ...)
  2020-07-02 18:23 ` [PATCH v2 10/11] xen: Update sched clock offset to avoid system instability in hibernation Anchal Agarwal
@ 2020-07-02 18:23 ` Anchal Agarwal
  2020-07-02 18:25 ` [PATCH v2 02/11] xenbus: add freeze/thaw/restore callbacks support Anchal Agarwal
  2020-07-10 18:17 ` [PATCH v2 00/11] Fix PM hibernation in Xen guests Agarwal, Anchal
  11 siblings, 0 replies; 44+ messages in thread
From: Anchal Agarwal @ 2020-07-02 18:23 UTC (permalink / raw)
  To: tglx, mingo, bp, hpa, x86, boris.ostrovsky, jgross, linux-pm,
	linux-mm, kamatam, sstabellini, konrad.wilk, roger.pau, axboe,
	davem, rjw, len.brown, pavel, peterz, eduval, sblbir, anchalag,
	xen-devel, vkuznets, netdev, linux-kernel, dwmw, benh

From: Aleksei Besogonov <cyberax@amazon.com>

The SNAPSHOT_SET_SWAP_AREA is supposed to be used to set the hibernation
offset on a running kernel to enable hibernating to a swap file.
However, it doesn't actually update the swsusp_resume_block variable. As
a result, the hibernation fails at the last step (after all the data is
written out) in the validation of the swap signature in
mark_swapfiles().

Before this patch, the command line processing was the only place where
swsusp_resume_block was set.

[Anchal Agarwal: Changelog: Resolved patch conflict as code fragmented to
snapshot_set_swap_area]

Signed-off-by: Aleksei Besogonov <cyberax@amazon.com>
Signed-off-by: Munehisa Kamata <kamatam@amazon.com>
Signed-off-by: Anchal Agarwal <anchalag@amazon.com>
---
 kernel/power/user.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/kernel/power/user.c b/kernel/power/user.c
index d5eedc2baa2a..e1209cefc103 100644
--- a/kernel/power/user.c
+++ b/kernel/power/user.c
@@ -242,8 +242,12 @@ static int snapshot_set_swap_area(struct snapshot_data *data,
 		return -EINVAL;
 	}
 	data->swap = swap_type_of(swdev, offset, &bdev);
-	if (data->swap < 0)
+	if (data->swap < 0) {
 		return -ENODEV;
+	} else {
+		swsusp_resume_device = swdev;
+		swsusp_resume_block = offset;
+	}
 
 	data->bd_inode = bdev->bd_inode;
 	bdput(bdev);
-- 
2.20.1


^ permalink raw reply	[flat|nested] 44+ messages in thread

* [PATCH v2 02/11] xenbus: add freeze/thaw/restore callbacks support
  2020-07-02 18:21 [PATCH v2 00/11] Fix PM hibernation in Xen guests Anchal Agarwal
                   ` (9 preceding siblings ...)
  2020-07-02 18:23 ` [PATCH v2 11/11] PM / hibernate: update the resume offset on SNAPSHOT_SET_SWAP_AREA Anchal Agarwal
@ 2020-07-02 18:25 ` Anchal Agarwal
  2020-07-10 18:17 ` [PATCH v2 00/11] Fix PM hibernation in Xen guests Agarwal, Anchal
  11 siblings, 0 replies; 44+ messages in thread
From: Anchal Agarwal @ 2020-07-02 18:25 UTC (permalink / raw)
  To: tglx, mingo, bp, hpa, x86, boris.ostrovsky, jgross, linux-pm,
	linux-mm, kamatam, sstabellini, konrad.wilk, roger.pau, axboe,
	davem, rjw, len.brown, pavel, peterz, eduval, sblbir, anchalag,
	xen-devel, vkuznets, netdev, linux-kernel, dwmw, benh

From: Munehisa Kamata <kamatam@amazon.com>

Since commit b3e96c0c7562 ("xen: use freeze/restore/thaw PM events for
suspend/resume/chkpt"), xenbus uses PMSG_FREEZE, PMSG_THAW and
PMSG_RESTORE events for Xen suspend. However, they're actually assigned
to xenbus_dev_suspend(), xenbus_dev_cancel() and xenbus_dev_resume()
respectively, and only suspend and resume callbacks are supported at
driver level. To support PM suspend and PM hibernation, modify the bus
level PM callbacks to invoke not only device driver's suspend/resume but
also freeze/thaw/restore.

Note that we'll use freeze/restore callbacks even for PM suspend whereas
suspend/resume callbacks are normally used in the case, becausae the
existing xenbus device drivers already have suspend/resume callbacks
specifically designed for Xen suspend. So we can allow the device
drivers to keep the existing callbacks wihtout modification.

[Anchal Agarwal: Changelog]:
RFC v1->v2: Refactored the callbacks code
    v1->v2: Use dev_warn instead of pr_warn, naming/initialization
            conventions
Signed-off-by: Agarwal Anchal <anchalag@amazon.com>
Signed-off-by: Munehisa Kamata <kamatam@amazon.com>
---
 drivers/xen/xenbus/xenbus_probe.c | 96 ++++++++++++++++++++++++++-----
 include/xen/xenbus.h              |  3 +
 2 files changed, 84 insertions(+), 15 deletions(-)

diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c
index 38725d97d909..715919aacd28 100644
--- a/drivers/xen/xenbus/xenbus_probe.c
+++ b/drivers/xen/xenbus/xenbus_probe.c
@@ -50,6 +50,7 @@
 #include <linux/io.h>
 #include <linux/slab.h>
 #include <linux/module.h>
+#include <linux/suspend.h>
 
 #include <asm/page.h>
 #include <asm/xen/hypervisor.h>
@@ -599,16 +600,33 @@ int xenbus_dev_suspend(struct device *dev)
 	struct xenbus_driver *drv;
 	struct xenbus_device *xdev
 		= container_of(dev, struct xenbus_device, dev);
+	bool xen_suspend = xen_is_xen_suspend();
 
 	DPRINTK("%s", xdev->nodename);
 
 	if (dev->driver == NULL)
 		return 0;
 	drv = to_xenbus_driver(dev->driver);
-	if (drv->suspend)
-		err = drv->suspend(xdev);
-	if (err)
-		dev_warn(dev, "suspend failed: %i\n", err);
+	if (xen_suspend) {
+		if (drv->suspend)
+			err = drv->suspend(xdev);
+	} else {
+		if (drv->freeze) {
+			err = drv->freeze(xdev);
+			if (!err) {
+				free_otherend_watch(xdev);
+				free_otherend_details(xdev);
+				return 0;
+			}
+		}
+	}
+
+	if (err) {
+		dev_warn(&xdev->dev, "%s %s failed: %d\n", xen_suspend ?
+				"suspend" : "freeze", xdev->nodename, err);
+		return err;
+	}
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(xenbus_dev_suspend);
@@ -619,6 +637,7 @@ int xenbus_dev_resume(struct device *dev)
 	struct xenbus_driver *drv;
 	struct xenbus_device *xdev
 		= container_of(dev, struct xenbus_device, dev);
+	bool xen_suspend = xen_is_xen_suspend();
 
 	DPRINTK("%s", xdev->nodename);
 
@@ -627,23 +646,34 @@ int xenbus_dev_resume(struct device *dev)
 	drv = to_xenbus_driver(dev->driver);
 	err = talk_to_otherend(xdev);
 	if (err) {
-		dev_warn(dev, "resume (talk_to_otherend) failed: %i\n", err);
+		dev_warn(&xdev->dev, "%s (talk_to_otherend) %s failed: %d\n",
+				xen_suspend ? "resume" : "restore",
+				xdev->nodename, err);
 		return err;
 	}
 
-	xdev->state = XenbusStateInitialising;
+	if (xen_suspend) {
+		xdev->state = XenbusStateInitialising;
+		if (drv->resume)
+			err = drv->resume(xdev);
+	} else {
+		if (drv->restore)
+			err = drv->restore(xdev);
+	}
 
-	if (drv->resume) {
-		err = drv->resume(xdev);
-		if (err) {
-			dev_warn(dev, "resume failed: %i\n", err);
-			return err;
-		}
+	if (err) {
+		dev_warn(&xdev->dev, "%s %s failed: %d\n",
+				xen_suspend ? "resume" : "restore",
+				xdev->nodename, err);
+		return err;
 	}
 
 	err = watch_otherend(xdev);
 	if (err) {
-		dev_warn(dev, "resume (watch_otherend) failed: %d\n", err);
+		dev_warn(&xdev->dev, "%s (watch_otherend) %s failed: %d.\n",
+				xen_suspend ? "resume" : "restore",
+				xdev->nodename, err);
+
 		return err;
 	}
 
@@ -653,8 +683,44 @@ EXPORT_SYMBOL_GPL(xenbus_dev_resume);
 
 int xenbus_dev_cancel(struct device *dev)
 {
-	/* Do nothing */
-	DPRINTK("cancel");
+	int err;
+	struct xenbus_driver *drv;
+	struct xenbus_device *xendev = to_xenbus_device(dev);
+	bool xen_suspend = xen_is_xen_suspend();
+
+	if (xen_suspend) {
+		/* Do nothing */
+		DPRINTK("cancel");
+		return 0;
+	}
+
+	DPRINTK("%s", xendev->nodename);
+
+	if (dev->driver == NULL)
+		return 0;
+	drv = to_xenbus_driver(dev->driver);
+	err = talk_to_otherend(xendev);
+	if (err) {
+		dev_warn(&xendev->dev, "thaw (talk_to_otherend) %s failed: %d.\n",
+			xendev->nodename, err);
+		return err;
+	}
+
+	if (drv->thaw) {
+		err = drv->thaw(xendev);
+		if (err) {
+			dev_warn(&xendev->dev, "thaw %s failed: %d\n", xendev->nodename, err);
+			return err;
+		}
+	}
+
+	err = watch_otherend(xendev);
+	if (err) {
+		dev_warn(&xendev->dev, "thaw (watch_otherend) %s failed: %d.\n",
+			xendev->nodename, err);
+		return err;
+	}
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(xenbus_dev_cancel);
diff --git a/include/xen/xenbus.h b/include/xen/xenbus.h
index 5a8315e6d8a6..8da964763255 100644
--- a/include/xen/xenbus.h
+++ b/include/xen/xenbus.h
@@ -104,6 +104,9 @@ struct xenbus_driver {
 	int (*remove)(struct xenbus_device *dev);
 	int (*suspend)(struct xenbus_device *dev);
 	int (*resume)(struct xenbus_device *dev);
+	int (*freeze)(struct xenbus_device *dev);
+	int (*thaw)(struct xenbus_device *dev);
+	int (*restore)(struct xenbus_device *dev);
 	int (*uevent)(struct xenbus_device *, struct kobj_uevent_env *);
 	struct device_driver driver;
 	int (*read_otherend_details)(struct xenbus_device *dev);
-- 
2.20.1


^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v2 04/11] x86/xen: add system core suspend and resume callbacks
  2020-07-02 18:22 ` [PATCH v2 04/11] x86/xen: add system core suspend and resume callbacks Anchal Agarwal
@ 2020-07-05 17:22   ` kernel test robot
  2020-07-22  9:08   ` Julien Grall
  1 sibling, 0 replies; 44+ messages in thread
From: kernel test robot @ 2020-07-05 17:22 UTC (permalink / raw)
  To: Anchal Agarwal, tglx, mingo, bp, hpa, x86, boris.ostrovsky,
	jgross, linux-pm, linux-mm, kamatam
  Cc: kbuild-all


[-- Attachment #1: Type: text/plain, Size: 1413 bytes --]

Hi Anchal,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on tip/auto-latest]
[also build test ERROR on linus/master v5.8-rc3 next-20200703]
[cannot apply to xen-tip/linux-next tip/irq/core]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Anchal-Agarwal/Fix-PM-hibernation-in-Xen-guests/20200703-022819
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 7e44a91e0445a854af5d34ca0f5baceccd518e73
config: x86_64-randconfig-m001-20200705 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-14) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   ld: arch/x86/xen/suspend.o: in function `xen_syscore_resume':
>> arch/x86/xen/suspend.c:111: undefined reference to `xen_hvm_map_shared_info'

vim +111 arch/x86/xen/suspend.c

   107	
   108	static void xen_syscore_resume(void)
   109	{
   110		/* No need to setup vcpu_info as it's already moved off */
 > 111		xen_hvm_map_shared_info();
   112	
   113		pvclock_resume();
   114	
   115		gnttab_resume();
   116	}
   117	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 38333 bytes --]

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v2 00/11] Fix PM hibernation in Xen guests
  2020-07-02 18:21 [PATCH v2 00/11] Fix PM hibernation in Xen guests Anchal Agarwal
                   ` (10 preceding siblings ...)
  2020-07-02 18:25 ` [PATCH v2 02/11] xenbus: add freeze/thaw/restore callbacks support Anchal Agarwal
@ 2020-07-10 18:17 ` Agarwal, Anchal
  2020-07-13 19:43   ` Boris Ostrovsky
  11 siblings, 1 reply; 44+ messages in thread
From: Agarwal, Anchal @ 2020-07-10 18:17 UTC (permalink / raw)
  To: tglx, mingo, bp, hpa, x86, boris.ostrovsky, jgross, linux-pm,
	linux-mm, Kamata, Munehisa, sstabellini, konrad.wilk, roger.pau,
	axboe, davem, rjw, len.brown, pavel, peterz, Valentin, Eduardo,
	Singh, Balbir, xen-devel, vkuznets, netdev, linux-kernel,
	Woodhouse, David, benh

Gentle ping on this series. 

--
Anchal

    Hello,
    This series fixes PM hibernation for hvm guests running on xen hypervisor.
    The running guest could now be hibernated and resumed successfully at a
    later time. The fixes for PM hibernation are added to block and
    network device drivers i.e xen-blkfront and xen-netfront. Any other driver
    that needs to add S4 support if not already, can follow same method of
    introducing freeze/thaw/restore callbacks.
    The patches had been tested against upstream kernel and xen4.11. Large
    scale testing is also done on Xen based Amazon EC2 instances. All this testing
    involved running memory exhausting workload in the background.

    Doing guest hibernation does not involve any support from hypervisor and
    this way guest has complete control over its state. Infrastructure
    restrictions for saving up guest state can be overcome by guest initiated
    hibernation.

    These patches were send out as RFC before and all the feedback had been
    incorporated in the patches. The last v1 could be found here:

    [v1]: https://lkml.org/lkml/2020/5/19/1312
    All comments and feedback from v1 had been incorporated in v2 series.
    Any comments/suggestions are welcome

    Known issues:
    1.KASLR causes intermittent hibernation failures. VM fails to resumes and
    has to be restarted. I will investigate this issue separately and shouldn't
    be a blocker for this patch series.
    2. During hibernation, I observed sometimes that freezing of tasks fails due
    to busy XFS workqueuei[xfs-cil/xfs-sync]. This is also intermittent may be 1
    out of 200 runs and hibernation is aborted in this case. Re-trying hibernation
    may work. Also, this is a known issue with hibernation and some
    filesystems like XFS has been discussed by the community for years with not an
    effectve resolution at this point.

    Testing How to:
    ---------------
    1. Setup xen hypervisor on a physical machine[ I used Ubuntu 16.04 +upstream
    xen-4.11]
    2. Bring up a HVM guest w/t kernel compiled with hibernation patches
    [I used ubuntu18.04 netboot bionic images and also Amazon Linux on-prem images].
    3. Create a swap file size=RAM size
    4. Update grub parameters and reboot
    5. Trigger pm-hibernation from within the VM

    Example:
    Set up a file-backed swap space. Swap file size>=Total memory on the system
    sudo dd if=/dev/zero of=/swap bs=$(( 1024 * 1024 )) count=4096 # 4096MiB
    sudo chmod 600 /swap
    sudo mkswap /swap
    sudo swapon /swap

    Update resume device/resume offset in grub if using swap file:
    resume=/dev/xvda1 resume_offset=200704 no_console_suspend=1

    Execute:
    --------
    sudo pm-hibernate
    OR
    echo disk > /sys/power/state && echo reboot > /sys/power/disk

    Compute resume offset code:
    "
    #!/usr/bin/env python
    import sys
    import array
    import fcntl

    #swap file
    f = open(sys.argv[1], 'r')
    buf = array.array('L', [0])

    #FIBMAP
    ret = fcntl.ioctl(f.fileno(), 0x01, buf)
    print buf[0]
    "


    Aleksei Besogonov (1):
      PM / hibernate: update the resume offset on SNAPSHOT_SET_SWAP_AREA

    Anchal Agarwal (4):
      x86/xen: Introduce new function to map HYPERVISOR_shared_info on
        Resume
      x86/xen: save and restore steal clock during PM hibernation
      xen: Introduce wrapper for save/restore sched clock offset
      xen: Update sched clock offset to avoid system instability in
        hibernation

    Munehisa Kamata (5):
      xen/manage: keep track of the on-going suspend mode
      xenbus: add freeze/thaw/restore callbacks support
      x86/xen: add system core suspend and resume callbacks
      xen-blkfront: add callbacks for PM suspend and hibernation
      xen-netfront: add callbacks for PM suspend and hibernation

    Thomas Gleixner (1):
      genirq: Shutdown irq chips in suspend/resume during hibernation

     arch/x86/xen/enlighten_hvm.c      |   7 ++
     arch/x86/xen/suspend.c            |  53 +++++++++++++
     arch/x86/xen/time.c               |  15 +++-
     arch/x86/xen/xen-ops.h            |   3 +
     drivers/block/xen-blkfront.c      | 122 +++++++++++++++++++++++++++++-
     drivers/net/xen-netfront.c        |  98 +++++++++++++++++++++++-
     drivers/xen/events/events_base.c  |   1 +
     drivers/xen/manage.c              |  60 +++++++++++++++
     drivers/xen/xenbus/xenbus_probe.c |  96 +++++++++++++++++++----
     include/linux/irq.h               |   2 +
     include/xen/xen-ops.h             |   3 +
     include/xen/xenbus.h              |   3 +
     kernel/irq/chip.c                 |   2 +-
     kernel/irq/internals.h            |   1 +
     kernel/irq/pm.c                   |  31 +++++---
     kernel/power/user.c               |   6 +-
     16 files changed, 470 insertions(+), 33 deletions(-)

    -- 
    2.20.1



^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v2 01/11] xen/manage: keep track of the on-going suspend mode
  2020-07-02 18:21 ` [PATCH v2 01/11] xen/manage: keep track of the on-going suspend mode Anchal Agarwal
@ 2020-07-13 15:52   ` Boris Ostrovsky
  2020-07-15 20:49     ` Anchal Agarwal
  0 siblings, 1 reply; 44+ messages in thread
From: Boris Ostrovsky @ 2020-07-13 15:52 UTC (permalink / raw)
  To: Anchal Agarwal, tglx, mingo, bp, hpa, x86, jgross, linux-pm,
	linux-mm, kamatam, sstabellini, konrad.wilk, roger.pau, axboe,
	davem, rjw, len.brown, pavel, peterz, eduval, sblbir, xen-devel,
	vkuznets, netdev, linux-kernel, dwmw, benh

On 7/2/20 2:21 PM, Anchal Agarwal wrote:
> From: Munehisa Kamata <kamatam@amazon.com>
>
> Guest hibernation is different from xen suspend/resume/live migration.
> Xen save/restore does not use pm_ops as is needed by guest hibernation.
> Hibernation in guest follows ACPI path and is guest inititated , the
> hibernation image is saved within guest as compared to later modes
> which are xen toolstack assisted and image creation/storage is in
> control of hypervisor/host machine.
> To differentiate between Xen suspend and PM hibernation, keep track
> of the on-going suspend mode by mainly using a new PM notifier.
> Introduce simple functions which help to know the on-going suspend mode
> so that other Xen-related code can behave differently according to the
> current suspend mode.
> Since Xen suspend doesn't have corresponding PM event, its main logic
> is modfied to acquire pm_mutex and set the current mode.
>
> Though, acquirng pm_mutex is still right thing to do, we may
> see deadlock if PM hibernation is interrupted by Xen suspend.
> PM hibernation depends on xenwatch thread to process xenbus state
> transactions, but the thread will sleep to wait pm_mutex which is
> already held by PM hibernation context in the scenario. Xen shutdown
> code may need some changes to avoid the issue.
>
> [Anchal Agarwal: Changelog]:
>  RFC v1->v2: Code refactoring
>  v1->v2:     Remove unused functions for PM SUSPEND/PM hibernation
>
> Signed-off-by: Anchal Agarwal <anchalag@amazon.com>
> Signed-off-by: Munehisa Kamata <kamatam@amazon.com>
> ---
>  drivers/xen/manage.c  | 60 +++++++++++++++++++++++++++++++++++++++++++
>  include/xen/xen-ops.h |  1 +
>  2 files changed, 61 insertions(+)
>
> diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
> index cd046684e0d1..69833fd6cfd1 100644
> --- a/drivers/xen/manage.c
> +++ b/drivers/xen/manage.c
> @@ -14,6 +14,7 @@
>  #include <linux/freezer.h>
>  #include <linux/syscore_ops.h>
>  #include <linux/export.h>
> +#include <linux/suspend.h>
>  
>  #include <xen/xen.h>
>  #include <xen/xenbus.h>
> @@ -40,6 +41,20 @@ enum shutdown_state {
>  /* Ignore multiple shutdown requests. */
>  static enum shutdown_state shutting_down = SHUTDOWN_INVALID;
>  
> +enum suspend_modes {
> +	NO_SUSPEND = 0,
> +	XEN_SUSPEND,
> +	PM_HIBERNATION,
> +};
> +
> +/* Protected by pm_mutex */
> +static enum suspend_modes suspend_mode = NO_SUSPEND;
> +
> +bool xen_is_xen_suspend(void)


Weren't you going to call this pv suspend? (And also --- is this suspend
or hibernation? Your commit messages and cover letter talk about fixing
hibernation).


> +{
> +	return suspend_mode == XEN_SUSPEND;
> +}
> +



> +
> +static int xen_pm_notifier(struct notifier_block *notifier,
> +			unsigned long pm_event, void *unused)
> +{
> +	switch (pm_event) {
> +	case PM_SUSPEND_PREPARE:
> +	case PM_HIBERNATION_PREPARE:
> +	case PM_RESTORE_PREPARE:
> +		suspend_mode = PM_HIBERNATION;


Do you ever use this mode? It seems to me all you care about is whether
or not we are doing XEN_SUSPEND. And so perhaps suspend_mode could
become a boolean. And then maybe even drop it altogether because it you
should be able to key off (shutting_down == SHUTDOWN_SUSPEND).


> +		break;
> +	case PM_POST_SUSPEND:
> +	case PM_POST_RESTORE:
> +	case PM_POST_HIBERNATION:
> +		/* Set back to the default */
> +		suspend_mode = NO_SUSPEND;
> +		break;
> +	default:
> +		pr_warn("Receive unknown PM event 0x%lx\n", pm_event);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +};



> +static int xen_setup_pm_notifier(void)
> +{
> +	if (!xen_hvm_domain())
> +		return -ENODEV;


I forgot --- what did we decide about non-x86 (i.e. ARM)?


And PVH dom0.


-boris




^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v2 00/11] Fix PM hibernation in Xen guests
  2020-07-10 18:17 ` [PATCH v2 00/11] Fix PM hibernation in Xen guests Agarwal, Anchal
@ 2020-07-13 19:43   ` Boris Ostrovsky
  2020-07-15 19:49     ` Anchal Agarwal
  0 siblings, 1 reply; 44+ messages in thread
From: Boris Ostrovsky @ 2020-07-13 19:43 UTC (permalink / raw)
  To: Agarwal, Anchal, tglx, mingo, bp, hpa, x86, jgross, linux-pm,
	linux-mm, Kamata, Munehisa, sstabellini, konrad.wilk, roger.pau,
	axboe, davem, rjw, len.brown, pavel, peterz, Valentin, Eduardo,
	Singh, Balbir, xen-devel, vkuznets, netdev, linux-kernel,
	Woodhouse, David, benh

On 7/10/20 2:17 PM, Agarwal, Anchal wrote:
> Gentle ping on this series. 


Have you tested save/restore?


-bois




^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v2 00/11] Fix PM hibernation in Xen guests
  2020-07-13 19:43   ` Boris Ostrovsky
@ 2020-07-15 19:49     ` Anchal Agarwal
  2020-07-15 20:49       ` Boris Ostrovsky
  0 siblings, 1 reply; 44+ messages in thread
From: Anchal Agarwal @ 2020-07-15 19:49 UTC (permalink / raw)
  To: Boris Ostrovsky
  Cc: tglx, mingo, bp, hpa, x86, jgross, linux-pm, linux-mm, Kamata,
	Munehisa, sstabellini, konrad.wilk, roger.pau, axboe, davem, rjw,
	len.brown, pavel, peterz, Valentin, Eduardo, Singh, Balbir,
	xen-devel, vkuznets, netdev, linux-kernel, Woodhouse, David,
	benh

On Mon, Jul 13, 2020 at 03:43:33PM -0400, Boris Ostrovsky wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> On 7/10/20 2:17 PM, Agarwal, Anchal wrote:
> > Gentle ping on this series.
> 
> 
> Have you tested save/restore?
>
No, not with the last few series. But a good point, I will test that and get
back to you. Do you see anything specific in the series that suggests otherwise?

Thanks,
Anchal
> 
> -bois
> 
> 
> 

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v2 01/11] xen/manage: keep track of the on-going suspend mode
  2020-07-13 15:52   ` Boris Ostrovsky
@ 2020-07-15 20:49     ` Anchal Agarwal
  2020-07-15 21:18       ` Boris Ostrovsky
  0 siblings, 1 reply; 44+ messages in thread
From: Anchal Agarwal @ 2020-07-15 20:49 UTC (permalink / raw)
  To: Boris Ostrovsky
  Cc: tglx, mingo, bp, hpa, x86, jgross, linux-pm, linux-mm, kamatam,
	sstabellini, konrad.wilk, roger.pau, axboe, davem, rjw,
	len.brown, pavel, peterz, eduval, sblbir, xen-devel, vkuznets,
	netdev, linux-kernel, dwmw, benh, anchalag

On Mon, Jul 13, 2020 at 11:52:01AM -0400, Boris Ostrovsky wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> On 7/2/20 2:21 PM, Anchal Agarwal wrote:
> > From: Munehisa Kamata <kamatam@amazon.com>
> >
> > Guest hibernation is different from xen suspend/resume/live migration.
> > Xen save/restore does not use pm_ops as is needed by guest hibernation.
> > Hibernation in guest follows ACPI path and is guest inititated , the
> > hibernation image is saved within guest as compared to later modes
> > which are xen toolstack assisted and image creation/storage is in
> > control of hypervisor/host machine.
> > To differentiate between Xen suspend and PM hibernation, keep track
> > of the on-going suspend mode by mainly using a new PM notifier.
> > Introduce simple functions which help to know the on-going suspend mode
> > so that other Xen-related code can behave differently according to the
> > current suspend mode.
> > Since Xen suspend doesn't have corresponding PM event, its main logic
> > is modfied to acquire pm_mutex and set the current mode.
> >
> > Though, acquirng pm_mutex is still right thing to do, we may
> > see deadlock if PM hibernation is interrupted by Xen suspend.
> > PM hibernation depends on xenwatch thread to process xenbus state
> > transactions, but the thread will sleep to wait pm_mutex which is
> > already held by PM hibernation context in the scenario. Xen shutdown
> > code may need some changes to avoid the issue.
> >
> > [Anchal Agarwal: Changelog]:
> >  RFC v1->v2: Code refactoring
> >  v1->v2:     Remove unused functions for PM SUSPEND/PM hibernation
> >
> > Signed-off-by: Anchal Agarwal <anchalag@amazon.com>
> > Signed-off-by: Munehisa Kamata <kamatam@amazon.com>
> > ---
> >  drivers/xen/manage.c  | 60 +++++++++++++++++++++++++++++++++++++++++++
> >  include/xen/xen-ops.h |  1 +
> >  2 files changed, 61 insertions(+)
> >
> > diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
> > index cd046684e0d1..69833fd6cfd1 100644
> > --- a/drivers/xen/manage.c
> > +++ b/drivers/xen/manage.c
> > @@ -14,6 +14,7 @@
> >  #include <linux/freezer.h>
> >  #include <linux/syscore_ops.h>
> >  #include <linux/export.h>
> > +#include <linux/suspend.h>
> >
> >  #include <xen/xen.h>
> >  #include <xen/xenbus.h>
> > @@ -40,6 +41,20 @@ enum shutdown_state {
> >  /* Ignore multiple shutdown requests. */
> >  static enum shutdown_state shutting_down = SHUTDOWN_INVALID;
> >
> > +enum suspend_modes {
> > +     NO_SUSPEND = 0,
> > +     XEN_SUSPEND,
> > +     PM_HIBERNATION,
> > +};
> > +
> > +/* Protected by pm_mutex */
> > +static enum suspend_modes suspend_mode = NO_SUSPEND;
> > +
> > +bool xen_is_xen_suspend(void)
> 
> 
> Weren't you going to call this pv suspend? (And also --- is this suspend
> or hibernation? Your commit messages and cover letter talk about fixing
> hibernation).
> 
> 
This is for hibernation is for pvhvm/hvm/pv-on-hvm guests as you may call it.
The method is just there to check if "xen suspend" is in progress.
I do not see "xen_suspend" differentiating between pv or hvm
domain until later in the code hence, I abstracted it to xen_is_xen_suspend.
> > +{
> > +     return suspend_mode == XEN_SUSPEND;
> > +}
> > +
> 
> 
> 
> > +
> > +static int xen_pm_notifier(struct notifier_block *notifier,
> > +                     unsigned long pm_event, void *unused)
> > +{
> > +     switch (pm_event) {
> > +     case PM_SUSPEND_PREPARE:
> > +     case PM_HIBERNATION_PREPARE:
> > +     case PM_RESTORE_PREPARE:
> > +             suspend_mode = PM_HIBERNATION;
> 
> 
> Do you ever use this mode? It seems to me all you care about is whether
> or not we are doing XEN_SUSPEND. And so perhaps suspend_mode could
> become a boolean. And then maybe even drop it altogether because it you
> should be able to key off (shutting_down == SHUTDOWN_SUSPEND).
> 
> 
The mode was left there in case its needed for restore prepare cases. But you
are right the only thing I currently care about whether shutting_down ==
SHUTDOWN_SUSPEND. Infact, the notifier may not be needed in first place.
xen_is_xen_suspend could work right off the bat using 'shutting_down' variable
itself. *I think so* I will test it on my end and send an updated patch.
> > +             break;
> > +     case PM_POST_SUSPEND:
> > +     case PM_POST_RESTORE:
> > +     case PM_POST_HIBERNATION:
> > +             /* Set back to the default */
> > +             suspend_mode = NO_SUSPEND;
> > +             break;
> > +     default:
> > +             pr_warn("Receive unknown PM event 0x%lx\n", pm_event);
> > +             return -EINVAL;
> > +     }
> > +
> > +     return 0;
> > +};
> 
> 
> 
> > +static int xen_setup_pm_notifier(void)
> > +{
> > +     if (!xen_hvm_domain())
> > +             return -ENODEV;
> 
> 
> I forgot --- what did we decide about non-x86 (i.e. ARM)?
It would be great to support that however, its  out of
scope for this patch set.
I’ll be happy to discuss it separately.
> 
> 
> And PVH dom0.
That's another good use case to make it work with however, I still
think that should be tested/worked upon separately as the feature itself
(PVH Dom0) is very new.
> 
> 
Thanks,
Anchal
> -boris
> 
> 
> 

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v2 00/11] Fix PM hibernation in Xen guests
  2020-07-15 19:49     ` Anchal Agarwal
@ 2020-07-15 20:49       ` Boris Ostrovsky
  2020-07-16 23:28         ` Anchal Agarwal
  0 siblings, 1 reply; 44+ messages in thread
From: Boris Ostrovsky @ 2020-07-15 20:49 UTC (permalink / raw)
  To: Anchal Agarwal
  Cc: tglx, mingo, bp, hpa, x86, jgross, linux-pm, linux-mm, Kamata,
	Munehisa, sstabellini, konrad.wilk, roger.pau, axboe, davem, rjw,
	len.brown, pavel, peterz, Valentin, Eduardo, Singh, Balbir,
	xen-devel, vkuznets, netdev, linux-kernel, Woodhouse, David,
	benh

On 7/15/20 3:49 PM, Anchal Agarwal wrote:
> On Mon, Jul 13, 2020 at 03:43:33PM -0400, Boris Ostrovsky wrote:
>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>>
>>
>>
>> On 7/10/20 2:17 PM, Agarwal, Anchal wrote:
>>> Gentle ping on this series.
>>
>> Have you tested save/restore?
>>
> No, not with the last few series. But a good point, I will test that and get
> back to you. Do you see anything specific in the series that suggests otherwise?


root@ovs104> xl save pvh saved
Saving to saved new xl format (info 0x3/0x0/1699)
xc: info: Saving domain 3, type x86 HVM
xc: Frames: 1044480/1044480  100%
xc: End of stream: 0/0    0%
root@ovs104> xl restore saved
Loading new save file saved (new xl fmt info 0x3/0x0/1699)
 Savefile contains xl domain config in JSON format
Parsing config from <saved>
xc: info: Found x86 HVM domain from Xen 4.13
xc: info: Restoring domain
xc: info: Restore successful
xc: info: XenStore: mfn 0xfeffc, dom 0, evt 1
xc: info: Console: mfn 0xfefff, dom 0, evt 2
root@ovs104> xl console pvh
[  139.943872] ------------[ cut here ]------------
[  139.943872] kernel BUG at arch/x86/xen/enlighten.c:205!
[  139.943872] invalid opcode: 0000 [#1] SMP PTI
[  139.943872] CPU: 0 PID: 11 Comm: migration/0 Not tainted 5.8.0-rc5 #26
[  139.943872] RIP: 0010:xen_vcpu_setup+0x16d/0x180
[  139.943872] Code: 4a 8b 14 f5 40 c9 1b 82 48 89 d8 48 89 2c 02 8b 05
a4 d4 40 01 85 c0 0f 85 15 ff ff ff 4a 8b 04 f5 40 c9 1b 82 e9 f4 fe ff
ff <0f> 0b b8 ed ff ff ff e9 14 ff ff ff e8 12 4f 86 00 66 90 66 66 66
[  139.943872] RSP: 0018:ffffc9000006bdb0 EFLAGS: 00010046
[  139.943872] RAX: 0000000000000000 RBX: ffffc9000014fe00 RCX:
0000000000000000
[  139.943872] RDX: ffff88803fc00000 RSI: 0000000000016128 RDI:
0000000000000000
[  139.943872] RBP: 0000000000000000 R08: 0000000000000000 R09:
0000000000000000
[  139.943872] R10: ffffffff826174a0 R11: ffffc9000006bcb4 R12:
0000000000016120
[  139.943872] R13: 0000000000016120 R14: 0000000000016128 R15:
0000000000000000
[  139.943872] FS:  0000000000000000(0000) GS:ffff88803fc00000(0000)
knlGS:0000000000000000
[  139.943872] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  139.943872] CR2: 00007f704be8b000 CR3: 000000003901a004 CR4:
00000000000606f0
[  139.943872] Call Trace:
[  139.943872]  ? __kmalloc+0x167/0x260
[  139.943872]  ? xen_manage_runstate_time+0x14a/0x170
[  139.943872]  xen_vcpu_restore+0x134/0x170
[  139.943872]  xen_hvm_post_suspend+0x1d/0x30
[  139.943872]  xen_arch_post_suspend+0x13/0x30
[  139.943872]  xen_suspend+0x87/0x190
[  139.943872]  multi_cpu_stop+0x6d/0x110
[  139.943872]  ? stop_machine_yield+0x10/0x10
[  139.943872]  cpu_stopper_thread+0x47/0x100
[  139.943872]  smpboot_thread_fn+0xc5/0x160
[  139.943872]  ? sort_range+0x20/0x20
[  139.943872]  kthread+0xfe/0x140
[  139.943872]  ? kthread_park+0x90/0x90
[  139.943872]  ret_from_fork+0x22/0x30
[  139.943872] Modules linked in:
[  139.943872] ---[ end trace 74716859a6b4f0a8 ]---
[  139.943872] RIP: 0010:xen_vcpu_setup+0x16d/0x180
[  139.943872] Code: 4a 8b 14 f5 40 c9 1b 82 48 89 d8 48 89 2c 02 8b 05
a4 d4 40 01 85 c0 0f 85 15 ff ff ff 4a 8b 04 f5 40 c9 1b 82 e9 f4 fe ff
ff <0f> 0b b8 ed ff ff ff e9 14 ff ff ff e8 12 4f 86 00 66 90 66 66 66
[  139.943872] RSP: 0018:ffffc9000006bdb0 EFLAGS: 00010046
[  139.943872] RAX: 0000000000000000 RBX: ffffc9000014fe00 RCX:
0000000000000000
[  139.943872] RDX: ffff88803fc00000 RSI: 0000000000016128 RDI:
0000000000000000
[  139.943872] RBP: 0000000000000000 R08: 0000000000000000 R09:
0000000000000000
[  139.943872] R10: ffffffff826174a0 R11: ffffc9000006bcb4 R12:
0000000000016120
[  139.943872] R13: 0000000000016120 R14: 0000000000016128 R15:
0000000000000000
[  139.943872] FS:  0000000000000000(0000) GS:ffff88803fc00000(0000)
knlGS:0000000000000000
[  139.943872] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  139.943872] CR2: 00007f704be8b000 CR3: 000000003901a004 CR4:
00000000000606f0
[  139.943872] Kernel panic - not syncing: Fatal exception
[  139.943872] Shutting down cpus with NMI
[  143.927559] Kernel Offset: disabled
root@ovs104>


^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v2 01/11] xen/manage: keep track of the on-going suspend mode
  2020-07-15 20:49     ` Anchal Agarwal
@ 2020-07-15 21:18       ` Boris Ostrovsky
  2020-07-17 19:10         ` Anchal Agarwal
  0 siblings, 1 reply; 44+ messages in thread
From: Boris Ostrovsky @ 2020-07-15 21:18 UTC (permalink / raw)
  To: Anchal Agarwal
  Cc: tglx, mingo, bp, hpa, x86, jgross, linux-pm, linux-mm, kamatam,
	sstabellini, konrad.wilk, roger.pau, axboe, davem, rjw,
	len.brown, pavel, peterz, eduval, sblbir, xen-devel, vkuznets,
	netdev, linux-kernel, dwmw, benh

On 7/15/20 4:49 PM, Anchal Agarwal wrote:
> On Mon, Jul 13, 2020 at 11:52:01AM -0400, Boris Ostrovsky wrote:
>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>>
>>
>>
>> On 7/2/20 2:21 PM, Anchal Agarwal wrote:
>>> +
>>> +bool xen_is_xen_suspend(void)
>>
>> Weren't you going to call this pv suspend? (And also --- is this suspend
>> or hibernation? Your commit messages and cover letter talk about fixing
>> hibernation).
>>
>>
> This is for hibernation is for pvhvm/hvm/pv-on-hvm guests as you may call it.
> The method is just there to check if "xen suspend" is in progress.
> I do not see "xen_suspend" differentiating between pv or hvm
> domain until later in the code hence, I abstracted it to xen_is_xen_suspend.


I meant "pv suspend" in the sense that this is paravirtual suspend, not
suspend for paravirtual guests. Just like pv drivers are for both pv and
hvm guests.


And then --- should it be pv suspend or pv hibernation?



>>> +{
>>> +     return suspend_mode == XEN_SUSPEND;
>>> +}
>>> +
>>
>> +static int xen_setup_pm_notifier(void)
>> +{
>> +     if (!xen_hvm_domain())
>> +             return -ENODEV;
>>
>> I forgot --- what did we decide about non-x86 (i.e. ARM)?
> It would be great to support that however, its  out of
> scope for this patch set.
> I’ll be happy to discuss it separately.


I wasn't implying that this *should* work on ARM but rather whether this
will break ARM somehow (because xen_hvm_domain() is true there).



>>
>> And PVH dom0.
> That's another good use case to make it work with however, I still
> think that should be tested/worked upon separately as the feature itself
> (PVH Dom0) is very new.


Same question here --- will this break PVH dom0?


-boris



^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v2 00/11] Fix PM hibernation in Xen guests
  2020-07-15 20:49       ` Boris Ostrovsky
@ 2020-07-16 23:28         ` Anchal Agarwal
  0 siblings, 0 replies; 44+ messages in thread
From: Anchal Agarwal @ 2020-07-16 23:28 UTC (permalink / raw)
  To: Boris Ostrovsky
  Cc: tglx, mingo, bp, hpa, x86, jgross, linux-pm, linux-mm, Kamata,
	Munehisa, sstabellini, konrad.wilk, roger.pau, axboe, davem, rjw,
	len.brown, pavel, peterz, Valentin, Eduardo, Singh, Balbir,
	xen-devel, vkuznets, netdev, linux-kernel, Woodhouse, David,
	benh

On Wed, Jul 15, 2020 at 04:49:57PM -0400, Boris Ostrovsky wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> On 7/15/20 3:49 PM, Anchal Agarwal wrote:
> > On Mon, Jul 13, 2020 at 03:43:33PM -0400, Boris Ostrovsky wrote:
> >> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> >>
> >>
> >>
> >> On 7/10/20 2:17 PM, Agarwal, Anchal wrote:
> >>> Gentle ping on this series.
> >>
> >> Have you tested save/restore?
> >>
> > No, not with the last few series. But a good point, I will test that and get
> > back to you. Do you see anything specific in the series that suggests otherwise?
> 
> 
> root@ovs104> xl save pvh saved
> Saving to saved new xl format (info 0x3/0x0/1699)
> xc: info: Saving domain 3, type x86 HVM
> xc: Frames: 1044480/1044480  100%
> xc: End of stream: 0/0    0%
> root@ovs104> xl restore saved
> Loading new save file saved (new xl fmt info 0x3/0x0/1699)
>  Savefile contains xl domain config in JSON format
> Parsing config from <saved>
> xc: info: Found x86 HVM domain from Xen 4.13
> xc: info: Restoring domain
> xc: info: Restore successful
> xc: info: XenStore: mfn 0xfeffc, dom 0, evt 1
> xc: info: Console: mfn 0xfefff, dom 0, evt 2
> root@ovs104> xl console pvh
> [  139.943872] ------------[ cut here ]------------
> [  139.943872] kernel BUG at arch/x86/xen/enlighten.c:205!
> [  139.943872] invalid opcode: 0000 [#1] SMP PTI
> [  139.943872] CPU: 0 PID: 11 Comm: migration/0 Not tainted 5.8.0-rc5 #26
> [  139.943872] RIP: 0010:xen_vcpu_setup+0x16d/0x180
> [  139.943872] Code: 4a 8b 14 f5 40 c9 1b 82 48 89 d8 48 89 2c 02 8b 05
> a4 d4 40 01 85 c0 0f 85 15 ff ff ff 4a 8b 04 f5 40 c9 1b 82 e9 f4 fe ff
> ff <0f> 0b b8 ed ff ff ff e9 14 ff ff ff e8 12 4f 86 00 66 90 66 66 66
> [  139.943872] RSP: 0018:ffffc9000006bdb0 EFLAGS: 00010046
> [  139.943872] RAX: 0000000000000000 RBX: ffffc9000014fe00 RCX:
> 0000000000000000
> [  139.943872] RDX: ffff88803fc00000 RSI: 0000000000016128 RDI:
> 0000000000000000
> [  139.943872] RBP: 0000000000000000 R08: 0000000000000000 R09:
> 0000000000000000
> [  139.943872] R10: ffffffff826174a0 R11: ffffc9000006bcb4 R12:
> 0000000000016120
> [  139.943872] R13: 0000000000016120 R14: 0000000000016128 R15:
> 0000000000000000
> [  139.943872] FS:  0000000000000000(0000) GS:ffff88803fc00000(0000)
> knlGS:0000000000000000
> [  139.943872] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  139.943872] CR2: 00007f704be8b000 CR3: 000000003901a004 CR4:
> 00000000000606f0
> [  139.943872] Call Trace:
> [  139.943872]  ? __kmalloc+0x167/0x260
> [  139.943872]  ? xen_manage_runstate_time+0x14a/0x170
> [  139.943872]  xen_vcpu_restore+0x134/0x170
> [  139.943872]  xen_hvm_post_suspend+0x1d/0x30
> [  139.943872]  xen_arch_post_suspend+0x13/0x30
> [  139.943872]  xen_suspend+0x87/0x190
> [  139.943872]  multi_cpu_stop+0x6d/0x110
> [  139.943872]  ? stop_machine_yield+0x10/0x10
> [  139.943872]  cpu_stopper_thread+0x47/0x100
> [  139.943872]  smpboot_thread_fn+0xc5/0x160
> [  139.943872]  ? sort_range+0x20/0x20
> [  139.943872]  kthread+0xfe/0x140
> [  139.943872]  ? kthread_park+0x90/0x90
> [  139.943872]  ret_from_fork+0x22/0x30
> [  139.943872] Modules linked in:
> [  139.943872] ---[ end trace 74716859a6b4f0a8 ]---
> [  139.943872] RIP: 0010:xen_vcpu_setup+0x16d/0x180
> [  139.943872] Code: 4a 8b 14 f5 40 c9 1b 82 48 89 d8 48 89 2c 02 8b 05
> a4 d4 40 01 85 c0 0f 85 15 ff ff ff 4a 8b 04 f5 40 c9 1b 82 e9 f4 fe ff
> ff <0f> 0b b8 ed ff ff ff e9 14 ff ff ff e8 12 4f 86 00 66 90 66 66 66
> [  139.943872] RSP: 0018:ffffc9000006bdb0 EFLAGS: 00010046
> [  139.943872] RAX: 0000000000000000 RBX: ffffc9000014fe00 RCX:
> 0000000000000000
> [  139.943872] RDX: ffff88803fc00000 RSI: 0000000000016128 RDI:
> 0000000000000000
> [  139.943872] RBP: 0000000000000000 R08: 0000000000000000 R09:
> 0000000000000000
> [  139.943872] R10: ffffffff826174a0 R11: ffffc9000006bcb4 R12:
> 0000000000016120
> [  139.943872] R13: 0000000000016120 R14: 0000000000016128 R15:
> 0000000000000000
> [  139.943872] FS:  0000000000000000(0000) GS:ffff88803fc00000(0000)
> knlGS:0000000000000000
> [  139.943872] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  139.943872] CR2: 00007f704be8b000 CR3: 000000003901a004 CR4:
> 00000000000606f0
> [  139.943872] Kernel panic - not syncing: Fatal exception
> [  139.943872] Shutting down cpus with NMI
> [  143.927559] Kernel Offset: disabled
> root@ovs104>
>
I think I may have found a bug. There were no issues with V1 version  that I
send however, there were issues with V2. I tested both series and found xl
save/restore to be working in V1 but not in V2. I should have tested it.
Anyways, looks the issue is coming from executing syscore ops registered for
hibernation use case during call to xen_suspend. 
I remember your comment from earlier where you did ask why we need to
check xen_suspend mode xen_syscore_suspend [patch-004] and I removed that based
on my theoretical understanding of your suggestion that since lock_system_sleep() lock
is taken, we cannot initialize hibernation. I skipped to check the part in the
code where during xen_suspend(), all registered syscore_ops suspend callbacks are
called. Hence the ones registered for PM hibernation will also be called.
With no check there on suspend mode, it fails to return from the function and
they never should be executed in case of xen suspend.
I will revert a part of that check in Patch-004 from V1 and send an updated patch with
the fix.

Thanks,
Anchal

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v2 01/11] xen/manage: keep track of the on-going suspend mode
  2020-07-15 21:18       ` Boris Ostrovsky
@ 2020-07-17 19:10         ` Anchal Agarwal
  2020-07-19  1:47           ` Boris Ostrovsky
  0 siblings, 1 reply; 44+ messages in thread
From: Anchal Agarwal @ 2020-07-17 19:10 UTC (permalink / raw)
  To: Boris Ostrovsky
  Cc: tglx, mingo, bp, hpa, x86, jgross, linux-pm, linux-mm, kamatam,
	sstabellini, konrad.wilk, roger.pau, axboe, davem, rjw,
	len.brown, pavel, peterz, eduval, sblbir, xen-devel, vkuznets,
	netdev, linux-kernel, dwmw, benh

On Wed, Jul 15, 2020 at 05:18:08PM -0400, Boris Ostrovsky wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> On 7/15/20 4:49 PM, Anchal Agarwal wrote:
> > On Mon, Jul 13, 2020 at 11:52:01AM -0400, Boris Ostrovsky wrote:
> >> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> >>
> >>
> >>
> >> On 7/2/20 2:21 PM, Anchal Agarwal wrote:
> >>> +
> >>> +bool xen_is_xen_suspend(void)
> >>
> >> Weren't you going to call this pv suspend? (And also --- is this suspend
> >> or hibernation? Your commit messages and cover letter talk about fixing
> >> hibernation).
> >>
> >>
> > This is for hibernation is for pvhvm/hvm/pv-on-hvm guests as you may call it.
> > The method is just there to check if "xen suspend" is in progress.
> > I do not see "xen_suspend" differentiating between pv or hvm
> > domain until later in the code hence, I abstracted it to xen_is_xen_suspend.
> 
> 
> I meant "pv suspend" in the sense that this is paravirtual suspend, not
> suspend for paravirtual guests. Just like pv drivers are for both pv and
> hvm guests.
> 
> 
> And then --- should it be pv suspend or pv hibernation?
> 
>
Ok so I think I am lot confused by this question. Here is what this
function for, function xen_is_xen_suspend() just tells us whether 
the guest is in "SHUTDOWN_SUSPEND" state or not. This check is needed
for correct invocation of syscore_ops callbacks registered for guest's
hibernation and for xenbus to invoke respective callbacks[suspend/resume
vs freeze/thaw/restore].
Since "shutting_down" state is defined static and is not directly available
to other parts of the code, the function solves the purpose.

I am having hard time understanding why this should be called pv
suspend/hibernation unless you are suggesting something else?
Am I missing your point here? 
> 
> >>> +{
> >>> +     return suspend_mode == XEN_SUSPEND;
> >>> +}
> >>> +
> >>
> >> +static int xen_setup_pm_notifier(void)
> >> +{
> >> +     if (!xen_hvm_domain())
> >> +             return -ENODEV;
> >>
> >> I forgot --- what did we decide about non-x86 (i.e. ARM)?
> > It would be great to support that however, its  out of
> > scope for this patch set.
> > I’ll be happy to discuss it separately.
> 
> 
> I wasn't implying that this *should* work on ARM but rather whether this
> will break ARM somehow (because xen_hvm_domain() is true there).
> 
> 
Ok makes sense. TBH, I haven't tested this part of code on ARM and the series
was only support x86 guests hibernation.
Moreover, this notifier is there to distinguish between 2 PM
events PM SUSPEND and PM hibernation. Now since we only care about PM
HIBERNATION I may just remove this code and rely on "SHUTDOWN_SUSPEND" state.
However, I may have to fix other patches in the series where this check may
appear and cater it only for x86 right?
> 
> >>
> >> And PVH dom0.
> > That's another good use case to make it work with however, I still
> > think that should be tested/worked upon separately as the feature itself
> > (PVH Dom0) is very new.
> 
> 
> Same question here --- will this break PVH dom0?
> 
I haven't tested it as a part of this series. Is that a blocker here?
> 
Thanks,
Anchal
> -boris
> 
> 

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v2 01/11] xen/manage: keep track of the on-going suspend mode
  2020-07-17 19:10         ` Anchal Agarwal
@ 2020-07-19  1:47           ` Boris Ostrovsky
  2020-07-20  9:37             ` Roger Pau Monné
  2020-07-21  0:03             ` Anchal Agarwal
  0 siblings, 2 replies; 44+ messages in thread
From: Boris Ostrovsky @ 2020-07-19  1:47 UTC (permalink / raw)
  To: Anchal Agarwal
  Cc: tglx, mingo, bp, hpa, x86, jgross, linux-pm, linux-mm, kamatam,
	sstabellini, konrad.wilk, roger.pau, axboe, davem, rjw,
	len.brown, pavel, peterz, eduval, sblbir, xen-devel, vkuznets,
	netdev, linux-kernel, dwmw, benh

(Roger, question for you at the very end)

On 7/17/20 3:10 PM, Anchal Agarwal wrote:
> On Wed, Jul 15, 2020 at 05:18:08PM -0400, Boris Ostrovsky wrote:
>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>>
>>
>>
>> On 7/15/20 4:49 PM, Anchal Agarwal wrote:
>>> On Mon, Jul 13, 2020 at 11:52:01AM -0400, Boris Ostrovsky wrote:
>>>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>>>>
>>>>
>>>>
>>>> On 7/2/20 2:21 PM, Anchal Agarwal wrote:
>>>>> +
>>>>> +bool xen_is_xen_suspend(void)
>>>> Weren't you going to call this pv suspend? (And also --- is this suspend
>>>> or hibernation? Your commit messages and cover letter talk about fixing
>>>> hibernation).
>>>>
>>>>
>>> This is for hibernation is for pvhvm/hvm/pv-on-hvm guests as you may call it.
>>> The method is just there to check if "xen suspend" is in progress.
>>> I do not see "xen_suspend" differentiating between pv or hvm
>>> domain until later in the code hence, I abstracted it to xen_is_xen_suspend.
>>
>> I meant "pv suspend" in the sense that this is paravirtual suspend, not
>> suspend for paravirtual guests. Just like pv drivers are for both pv and
>> hvm guests.
>>
>>
>> And then --- should it be pv suspend or pv hibernation?
>>
>>
> Ok so I think I am lot confused by this question. Here is what this
> function for, function xen_is_xen_suspend() just tells us whether 
> the guest is in "SHUTDOWN_SUSPEND" state or not. This check is needed
> for correct invocation of syscore_ops callbacks registered for guest's
> hibernation and for xenbus to invoke respective callbacks[suspend/resume
> vs freeze/thaw/restore].
> Since "shutting_down" state is defined static and is not directly available
> to other parts of the code, the function solves the purpose.
>
> I am having hard time understanding why this should be called pv
> suspend/hibernation unless you are suggesting something else?
> Am I missing your point here? 



I think I understand now what you are trying to say --- it's whether we
are going to use xen_suspend() routine, right? If that's the case then
sure, you can use "xen_suspend" term. (I'd probably still change
xen_is_xen_suspend() to is_xen_suspend())


>>>>> +{
>>>>> +     return suspend_mode == XEN_SUSPEND;
>>>>> +}
>>>>> +
>>>> +static int xen_setup_pm_notifier(void)
>>>> +{
>>>> +     if (!xen_hvm_domain())
>>>> +             return -ENODEV;
>>>>
>>>> I forgot --- what did we decide about non-x86 (i.e. ARM)?
>>> It would be great to support that however, its  out of
>>> scope for this patch set.
>>> I’ll be happy to discuss it separately.
>>
>> I wasn't implying that this *should* work on ARM but rather whether this
>> will break ARM somehow (because xen_hvm_domain() is true there).
>>
>>
> Ok makes sense. TBH, I haven't tested this part of code on ARM and the series
> was only support x86 guests hibernation.
> Moreover, this notifier is there to distinguish between 2 PM
> events PM SUSPEND and PM hibernation. Now since we only care about PM
> HIBERNATION I may just remove this code and rely on "SHUTDOWN_SUSPEND" state.
> However, I may have to fix other patches in the series where this check may
> appear and cater it only for x86 right?


I don't know what would happen if ARM guest tries to handle hibernation
callbacks. The only ones that you are introducing are in block and net
fronts and that's arch-independent.


You do add a bunch of x86-specific code though (syscore ops), would
something similar be needed for ARM?


>>>> And PVH dom0.
>>> That's another good use case to make it work with however, I still
>>> think that should be tested/worked upon separately as the feature itself
>>> (PVH Dom0) is very new.
>>
>> Same question here --- will this break PVH dom0?
>>
> I haven't tested it as a part of this series. Is that a blocker here?


I suspect dom0 will not do well now as far as hibernation goes, in which
case you are not breaking anything.


Roger?


-boris




^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v2 01/11] xen/manage: keep track of the on-going suspend mode
  2020-07-19  1:47           ` Boris Ostrovsky
@ 2020-07-20  9:37             ` Roger Pau Monné
  2020-07-21  0:17               ` Anchal Agarwal
  2020-07-21  0:03             ` Anchal Agarwal
  1 sibling, 1 reply; 44+ messages in thread
From: Roger Pau Monné @ 2020-07-20  9:37 UTC (permalink / raw)
  To: Boris Ostrovsky
  Cc: Anchal Agarwal, tglx, mingo, bp, hpa, x86, jgross, linux-pm,
	linux-mm, kamatam, sstabellini, konrad.wilk, axboe, davem, rjw,
	len.brown, pavel, peterz, eduval, sblbir, xen-devel, vkuznets,
	netdev, linux-kernel, dwmw, benh

On Sat, Jul 18, 2020 at 09:47:04PM -0400, Boris Ostrovsky wrote:
> (Roger, question for you at the very end)
> 
> On 7/17/20 3:10 PM, Anchal Agarwal wrote:
> > On Wed, Jul 15, 2020 at 05:18:08PM -0400, Boris Ostrovsky wrote:
> >> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> >>
> >>
> >>
> >> On 7/15/20 4:49 PM, Anchal Agarwal wrote:
> >>> On Mon, Jul 13, 2020 at 11:52:01AM -0400, Boris Ostrovsky wrote:
> >>>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> >>>>
> >>>>
> >>>>
> >>>> On 7/2/20 2:21 PM, Anchal Agarwal wrote:
> >>>> And PVH dom0.
> >>> That's another good use case to make it work with however, I still
> >>> think that should be tested/worked upon separately as the feature itself
> >>> (PVH Dom0) is very new.
> >>
> >> Same question here --- will this break PVH dom0?
> >>
> > I haven't tested it as a part of this series. Is that a blocker here?
> 
> 
> I suspect dom0 will not do well now as far as hibernation goes, in which
> case you are not breaking anything.
> 
> 
> Roger?

I sadly don't have any box ATM that supports hibernation where I
could test it. We have hibernation support for PV dom0, so while I
haven't done anything specific to support or test hibernation on PVH
dom0 I would at least aim to not make this any worse, and hence the
check should at least also fail for a PVH dom0?

if (!xen_hvm_domain() || xen_initial_domain())
    return -ENODEV;

Ie: none of this should be applied to a PVH dom0, as it doesn't have
PV devices and hence should follow the bare metal device suspend.

Also I would contact the QubesOS guys, they rely heavily on the
suspend feature for dom0, and that's something not currently tested by
osstest so any breakages there go unnoticed.

Thanks, Roger.

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v2 01/11] xen/manage: keep track of the on-going suspend mode
  2020-07-19  1:47           ` Boris Ostrovsky
  2020-07-20  9:37             ` Roger Pau Monné
@ 2020-07-21  0:03             ` Anchal Agarwal
  2020-07-21 21:48               ` Boris Ostrovsky
  1 sibling, 1 reply; 44+ messages in thread
From: Anchal Agarwal @ 2020-07-21  0:03 UTC (permalink / raw)
  To: Boris Ostrovsky
  Cc: tglx, mingo, bp, hpa, x86, jgross, linux-pm, linux-mm, kamatam,
	sstabellini, konrad.wilk, roger.pau, axboe, davem, rjw,
	len.brown, pavel, peterz, eduval, sblbir, xen-devel, vkuznets,
	netdev, linux-kernel, dwmw, benh, anchalag

On Sat, Jul 18, 2020 at 09:47:04PM -0400, Boris Ostrovsky wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> (Roger, question for you at the very end)
> 
> On 7/17/20 3:10 PM, Anchal Agarwal wrote:
> > On Wed, Jul 15, 2020 at 05:18:08PM -0400, Boris Ostrovsky wrote:
> >> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> >>
> >>
> >>
> >> On 7/15/20 4:49 PM, Anchal Agarwal wrote:
> >>> On Mon, Jul 13, 2020 at 11:52:01AM -0400, Boris Ostrovsky wrote:
> >>>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> >>>>
> >>>>
> >>>>
> >>>> On 7/2/20 2:21 PM, Anchal Agarwal wrote:
> >>>>> +
> >>>>> +bool xen_is_xen_suspend(void)
> >>>> Weren't you going to call this pv suspend? (And also --- is this suspend
> >>>> or hibernation? Your commit messages and cover letter talk about fixing
> >>>> hibernation).
> >>>>
> >>>>
> >>> This is for hibernation is for pvhvm/hvm/pv-on-hvm guests as you may call it.
> >>> The method is just there to check if "xen suspend" is in progress.
> >>> I do not see "xen_suspend" differentiating between pv or hvm
> >>> domain until later in the code hence, I abstracted it to xen_is_xen_suspend.
> >>
> >> I meant "pv suspend" in the sense that this is paravirtual suspend, not
> >> suspend for paravirtual guests. Just like pv drivers are for both pv and
> >> hvm guests.
> >>
> >>
> >> And then --- should it be pv suspend or pv hibernation?
> >>
> >>
> > Ok so I think I am lot confused by this question. Here is what this
> > function for, function xen_is_xen_suspend() just tells us whether
> > the guest is in "SHUTDOWN_SUSPEND" state or not. This check is needed
> > for correct invocation of syscore_ops callbacks registered for guest's
> > hibernation and for xenbus to invoke respective callbacks[suspend/resume
> > vs freeze/thaw/restore].
> > Since "shutting_down" state is defined static and is not directly available
> > to other parts of the code, the function solves the purpose.
> >
> > I am having hard time understanding why this should be called pv
> > suspend/hibernation unless you are suggesting something else?
> > Am I missing your point here?
> 
> 
> 
> I think I understand now what you are trying to say --- it's whether we
> are going to use xen_suspend() routine, right? If that's the case then
> sure, you can use "xen_suspend" term. (I'd probably still change
> xen_is_xen_suspend() to is_xen_suspend())
>
I think so too. Will change it.
> 
> >>>>> +{
> >>>>> +     return suspend_mode == XEN_SUSPEND;
> >>>>> +}
> >>>>> +
> >>>> +static int xen_setup_pm_notifier(void)
> >>>> +{
> >>>> +     if (!xen_hvm_domain())
> >>>> +             return -ENODEV;
> >>>>
> >>>> I forgot --- what did we decide about non-x86 (i.e. ARM)?
> >>> It would be great to support that however, its  out of
> >>> scope for this patch set.
> >>> I’ll be happy to discuss it separately.
> >>
> >> I wasn't implying that this *should* work on ARM but rather whether this
> >> will break ARM somehow (because xen_hvm_domain() is true there).
> >>
> >>
> > Ok makes sense. TBH, I haven't tested this part of code on ARM and the series
> > was only support x86 guests hibernation.
> > Moreover, this notifier is there to distinguish between 2 PM
> > events PM SUSPEND and PM hibernation. Now since we only care about PM
> > HIBERNATION I may just remove this code and rely on "SHUTDOWN_SUSPEND" state.
> > However, I may have to fix other patches in the series where this check may
> > appear and cater it only for x86 right?
> 
> 
> I don't know what would happen if ARM guest tries to handle hibernation
> callbacks. The only ones that you are introducing are in block and net
> fronts and that's arch-independent.
> 
> 
> You do add a bunch of x86-specific code though (syscore ops), would
> something similar be needed for ARM?
> 
> 
I don't expect this to work out of the box on ARM. To start with something
similar will be needed for ARM too.
We may still want to keep the driver code as-is.

I understand the concern here wrt ARM, however, currently the support is only
proposed for x86 guests here and similar work could be carried out for ARM.
Also, if regular hibernation works correctly on arm, then all is needed is to
fix Xen side of things.

I am not sure what could be done to achieve any assurances on arm side as far as
this series is concerned.
> >>>> And PVH dom0.
> >>> That's another good use case to make it work with however, I still
> >>> think that should be tested/worked upon separately as the feature itself
> >>> (PVH Dom0) is very new.
> >>
> >> Same question here --- will this break PVH dom0?
> >>
> > I haven't tested it as a part of this series. Is that a blocker here?
> 
> 
> I suspect dom0 will not do well now as far as hibernation goes, in which
> case you are not breaking anything.
> 
> 
> Roger?
> 
> 
> -boris

Thanks,
Anchal
> 
> 
> 

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v2 01/11] xen/manage: keep track of the on-going suspend mode
  2020-07-20  9:37             ` Roger Pau Monné
@ 2020-07-21  0:17               ` Anchal Agarwal
  2020-07-21  8:30                 ` Roger Pau Monné
  0 siblings, 1 reply; 44+ messages in thread
From: Anchal Agarwal @ 2020-07-21  0:17 UTC (permalink / raw)
  To: Roger Pau Monné
  Cc: Boris Ostrovsky, tglx, mingo, bp, hpa, x86, jgross, linux-pm,
	linux-mm, kamatam, sstabellini, konrad.wilk, axboe, davem, rjw,
	len.brown, pavel, peterz, eduval, sblbir, xen-devel, vkuznets,
	netdev, linux-kernel, dwmw, benh

On Mon, Jul 20, 2020 at 11:37:05AM +0200, Roger Pau Monné wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> On Sat, Jul 18, 2020 at 09:47:04PM -0400, Boris Ostrovsky wrote:
> > (Roger, question for you at the very end)
> >
> > On 7/17/20 3:10 PM, Anchal Agarwal wrote:
> > > On Wed, Jul 15, 2020 at 05:18:08PM -0400, Boris Ostrovsky wrote:
> > >> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> > >>
> > >>
> > >>
> > >> On 7/15/20 4:49 PM, Anchal Agarwal wrote:
> > >>> On Mon, Jul 13, 2020 at 11:52:01AM -0400, Boris Ostrovsky wrote:
> > >>>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> > >>>>
> > >>>>
> > >>>>
> > >>>> On 7/2/20 2:21 PM, Anchal Agarwal wrote:
> > >>>> And PVH dom0.
> > >>> That's another good use case to make it work with however, I still
> > >>> think that should be tested/worked upon separately as the feature itself
> > >>> (PVH Dom0) is very new.
> > >>
> > >> Same question here --- will this break PVH dom0?
> > >>
> > > I haven't tested it as a part of this series. Is that a blocker here?
> >
> >
> > I suspect dom0 will not do well now as far as hibernation goes, in which
> > case you are not breaking anything.
> >
> >
> > Roger?
> 
> I sadly don't have any box ATM that supports hibernation where I
> could test it. We have hibernation support for PV dom0, so while I
> haven't done anything specific to support or test hibernation on PVH
> dom0 I would at least aim to not make this any worse, and hence the
> check should at least also fail for a PVH dom0?
> 
> if (!xen_hvm_domain() || xen_initial_domain())
>     return -ENODEV;
> 
> Ie: none of this should be applied to a PVH dom0, as it doesn't have
> PV devices and hence should follow the bare metal device suspend.
>
So from what I understand you meant for any guest running on pvh dom0 should not 
hibernate if hibernation is triggered from within the guest or should they?

> Also I would contact the QubesOS guys, they rely heavily on the
> suspend feature for dom0, and that's something not currently tested by
> osstest so any breakages there go unnoticed.
> 
Was this for me or Boris? If its the former then I have no idea how to?
> Thanks, Roger.

Thanks,
Anchal

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v2 01/11] xen/manage: keep track of the on-going suspend mode
  2020-07-21  0:17               ` Anchal Agarwal
@ 2020-07-21  8:30                 ` Roger Pau Monné
  2020-07-21 19:55                   ` Anchal Agarwal
  0 siblings, 1 reply; 44+ messages in thread
From: Roger Pau Monné @ 2020-07-21  8:30 UTC (permalink / raw)
  To: Anchal Agarwal, marmarek
  Cc: Boris Ostrovsky, tglx, mingo, bp, hpa, x86, jgross, linux-pm,
	linux-mm, kamatam, sstabellini, konrad.wilk, axboe, davem, rjw,
	len.brown, pavel, peterz, eduval, sblbir, xen-devel, vkuznets,
	netdev, linux-kernel, dwmw, benh

Marek: I'm adding you in case you could be able to give this a try and
make sure it doesn't break suspend for dom0.

On Tue, Jul 21, 2020 at 12:17:36AM +0000, Anchal Agarwal wrote:
> On Mon, Jul 20, 2020 at 11:37:05AM +0200, Roger Pau Monné wrote:
> > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> > 
> > 
> > 
> > On Sat, Jul 18, 2020 at 09:47:04PM -0400, Boris Ostrovsky wrote:
> > > (Roger, question for you at the very end)
> > >
> > > On 7/17/20 3:10 PM, Anchal Agarwal wrote:
> > > > On Wed, Jul 15, 2020 at 05:18:08PM -0400, Boris Ostrovsky wrote:
> > > >> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> > > >>
> > > >>
> > > >>
> > > >> On 7/15/20 4:49 PM, Anchal Agarwal wrote:
> > > >>> On Mon, Jul 13, 2020 at 11:52:01AM -0400, Boris Ostrovsky wrote:
> > > >>>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> > > >>>>
> > > >>>>
> > > >>>>
> > > >>>> On 7/2/20 2:21 PM, Anchal Agarwal wrote:
> > > >>>> And PVH dom0.
> > > >>> That's another good use case to make it work with however, I still
> > > >>> think that should be tested/worked upon separately as the feature itself
> > > >>> (PVH Dom0) is very new.
> > > >>
> > > >> Same question here --- will this break PVH dom0?
> > > >>
> > > > I haven't tested it as a part of this series. Is that a blocker here?
> > >
> > >
> > > I suspect dom0 will not do well now as far as hibernation goes, in which
> > > case you are not breaking anything.
> > >
> > >
> > > Roger?
> > 
> > I sadly don't have any box ATM that supports hibernation where I
> > could test it. We have hibernation support for PV dom0, so while I
> > haven't done anything specific to support or test hibernation on PVH
> > dom0 I would at least aim to not make this any worse, and hence the
> > check should at least also fail for a PVH dom0?
> > 
> > if (!xen_hvm_domain() || xen_initial_domain())
> >     return -ENODEV;
> > 
> > Ie: none of this should be applied to a PVH dom0, as it doesn't have
> > PV devices and hence should follow the bare metal device suspend.
> >
> So from what I understand you meant for any guest running on pvh dom0 should not 
> hibernate if hibernation is triggered from within the guest or should they?

Er no to both I think. What I meant is that a PVH dom0 should be able
to properly suspend, and we should make sure this work doesn't make
this any harder (or breaks it if it's currently working).

Or at least that's how I understood the question raised by Boris.

You are adding code to the generic suspend path that's also used by dom0
in order to perform bare metal suspension. This is fine now for a PV
dom0 because the code is gated on xen_hvm_domain, but you should also
take into account that a PVH dom0 is considered a HVM domain, and
hence will get the notifier registered.

> > Also I would contact the QubesOS guys, they rely heavily on the
> > suspend feature for dom0, and that's something not currently tested by
> > osstest so any breakages there go unnoticed.
> > 
> Was this for me or Boris? If its the former then I have no idea how to?

I've now added Marek.

Roger.

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v2 01/11] xen/manage: keep track of the on-going suspend mode
  2020-07-21  8:30                 ` Roger Pau Monné
@ 2020-07-21 19:55                   ` Anchal Agarwal
  2020-07-22  8:27                     ` Roger Pau Monné
  0 siblings, 1 reply; 44+ messages in thread
From: Anchal Agarwal @ 2020-07-21 19:55 UTC (permalink / raw)
  To: Roger Pau Monné
  Cc: marmarek, Boris Ostrovsky, tglx, mingo, bp, hpa, x86, jgross,
	linux-pm, linux-mm, kamatam, sstabellini, konrad.wilk, axboe,
	davem, rjw, len.brown, pavel, peterz, eduval, sblbir, xen-devel,
	vkuznets, netdev, linux-kernel, dwmw, benh, anchalag

On Tue, Jul 21, 2020 at 10:30:18AM +0200, Roger Pau Monné wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> Marek: I'm adding you in case you could be able to give this a try and
> make sure it doesn't break suspend for dom0.
> 
> On Tue, Jul 21, 2020 at 12:17:36AM +0000, Anchal Agarwal wrote:
> > On Mon, Jul 20, 2020 at 11:37:05AM +0200, Roger Pau Monné wrote:
> > > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> > >
> > >
> > >
> > > On Sat, Jul 18, 2020 at 09:47:04PM -0400, Boris Ostrovsky wrote:
> > > > (Roger, question for you at the very end)
> > > >
> > > > On 7/17/20 3:10 PM, Anchal Agarwal wrote:
> > > > > On Wed, Jul 15, 2020 at 05:18:08PM -0400, Boris Ostrovsky wrote:
> > > > >> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> > > > >>
> > > > >>
> > > > >>
> > > > >> On 7/15/20 4:49 PM, Anchal Agarwal wrote:
> > > > >>> On Mon, Jul 13, 2020 at 11:52:01AM -0400, Boris Ostrovsky wrote:
> > > > >>>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> > > > >>>>
> > > > >>>>
> > > > >>>>
> > > > >>>> On 7/2/20 2:21 PM, Anchal Agarwal wrote:
> > > > >>>> And PVH dom0.
> > > > >>> That's another good use case to make it work with however, I still
> > > > >>> think that should be tested/worked upon separately as the feature itself
> > > > >>> (PVH Dom0) is very new.
> > > > >>
> > > > >> Same question here --- will this break PVH dom0?
> > > > >>
> > > > > I haven't tested it as a part of this series. Is that a blocker here?
> > > >
> > > >
> > > > I suspect dom0 will not do well now as far as hibernation goes, in which
> > > > case you are not breaking anything.
> > > >
> > > >
> > > > Roger?
> > >
> > > I sadly don't have any box ATM that supports hibernation where I
> > > could test it. We have hibernation support for PV dom0, so while I
> > > haven't done anything specific to support or test hibernation on PVH
> > > dom0 I would at least aim to not make this any worse, and hence the
> > > check should at least also fail for a PVH dom0?
> > >
> > > if (!xen_hvm_domain() || xen_initial_domain())
> > >     return -ENODEV;
> > >
> > > Ie: none of this should be applied to a PVH dom0, as it doesn't have
> > > PV devices and hence should follow the bare metal device suspend.
> > >
> > So from what I understand you meant for any guest running on pvh dom0 should not
> > hibernate if hibernation is triggered from within the guest or should they?
> 
> Er no to both I think. What I meant is that a PVH dom0 should be able
> to properly suspend, and we should make sure this work doesn't make
> this any harder (or breaks it if it's currently working).
> 
> Or at least that's how I understood the question raised by Boris.
> 
> You are adding code to the generic suspend path that's also used by dom0
> in order to perform bare metal suspension. This is fine now for a PV
> dom0 because the code is gated on xen_hvm_domain, but you should also
> take into account that a PVH dom0 is considered a HVM domain, and
> hence will get the notifier registered.
>
Ok that makes sense now. This is good to be safe, but my patch series is only to
support domU hibernation, so I am not sure if this will affect pvh dom0.
However, since I do not have a good way of testing sure I will add the check.

Moreover, in Patch-0004, I do register suspend/resume syscore_ops specifically for domU
hibernation only if its xen_hvm_domain. I don't see any reason that should not
be registered for domU's running on pvh dom0. Those suspend/resume callbacks will
only be invoked in case hibernation and will be skipped if generic suspend path
is in progress. Do you see any issue with that?

> > > Also I would contact the QubesOS guys, they rely heavily on the
> > > suspend feature for dom0, and that's something not currently tested by
> > > osstest so any breakages there go unnoticed.
> > >
> > Was this for me or Boris? If its the former then I have no idea how to?
> 
> I've now added Marek.
> 
> Roger.
Anchal

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v2 01/11] xen/manage: keep track of the on-going suspend mode
  2020-07-21  0:03             ` Anchal Agarwal
@ 2020-07-21 21:48               ` Boris Ostrovsky
  2020-07-22  0:18                 ` Stefano Stabellini
  0 siblings, 1 reply; 44+ messages in thread
From: Boris Ostrovsky @ 2020-07-21 21:48 UTC (permalink / raw)
  To: Anchal Agarwal
  Cc: tglx, mingo, bp, hpa, x86, jgross, linux-pm, linux-mm, kamatam,
	sstabellini, konrad.wilk, roger.pau, axboe, davem, rjw,
	len.brown, pavel, peterz, eduval, sblbir, xen-devel, vkuznets,
	netdev, linux-kernel, dwmw, benh


>>>>>> +static int xen_setup_pm_notifier(void)
>>>>>> +{
>>>>>> +     if (!xen_hvm_domain())
>>>>>> +             return -ENODEV;
>>>>>>
>>>>>> I forgot --- what did we decide about non-x86 (i.e. ARM)?
>>>>> It would be great to support that however, its  out of
>>>>> scope for this patch set.
>>>>> I’ll be happy to discuss it separately.
>>>>
>>>> I wasn't implying that this *should* work on ARM but rather whether this
>>>> will break ARM somehow (because xen_hvm_domain() is true there).
>>>>
>>>>
>>> Ok makes sense. TBH, I haven't tested this part of code on ARM and the series
>>> was only support x86 guests hibernation.
>>> Moreover, this notifier is there to distinguish between 2 PM
>>> events PM SUSPEND and PM hibernation. Now since we only care about PM
>>> HIBERNATION I may just remove this code and rely on "SHUTDOWN_SUSPEND" state.
>>> However, I may have to fix other patches in the series where this check may
>>> appear and cater it only for x86 right?
>>
>>
>> I don't know what would happen if ARM guest tries to handle hibernation
>> callbacks. The only ones that you are introducing are in block and net
>> fronts and that's arch-independent.
>>
>>
>> You do add a bunch of x86-specific code though (syscore ops), would
>> something similar be needed for ARM?
>>
>>
> I don't expect this to work out of the box on ARM. To start with something
> similar will be needed for ARM too.
> We may still want to keep the driver code as-is.
> 
> I understand the concern here wrt ARM, however, currently the support is only
> proposed for x86 guests here and similar work could be carried out for ARM.
> Also, if regular hibernation works correctly on arm, then all is needed is to
> fix Xen side of things.
> 
> I am not sure what could be done to achieve any assurances on arm side as far as
> this series is concerned.


If you are not sure what the effects are (or sure that it won't work) on
ARM then I'd add IS_ENABLED(CONFIG_X86) check, i.e.


if (!IS_ENABLED(CONFIG_X86) || !xen_hvm_domain())
	return -ENODEV;


(plus '|| xen_initial_domain()' for PVH dom0 as Roger suggested.)

-boris

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v2 01/11] xen/manage: keep track of the on-going suspend mode
  2020-07-21 21:48               ` Boris Ostrovsky
@ 2020-07-22  0:18                 ` Stefano Stabellini
  2020-07-22 18:02                   ` Anchal Agarwal
  0 siblings, 1 reply; 44+ messages in thread
From: Stefano Stabellini @ 2020-07-22  0:18 UTC (permalink / raw)
  To: Boris Ostrovsky
  Cc: Anchal Agarwal, tglx, mingo, bp, hpa, x86, jgross, linux-pm,
	linux-mm, kamatam, sstabellini, konrad.wilk, roger.pau, axboe,
	davem, rjw, len.brown, pavel, peterz, eduval, sblbir, xen-devel,
	vkuznets, netdev, linux-kernel, dwmw, benh


[-- Attachment #1: Type: text/plain, Size: 3720 bytes --]

On Tue, 21 Jul 2020, Boris Ostrovsky wrote:
> >>>>>> +static int xen_setup_pm_notifier(void)
> >>>>>> +{
> >>>>>> +     if (!xen_hvm_domain())
> >>>>>> +             return -ENODEV;
> >>>>>>
> >>>>>> I forgot --- what did we decide about non-x86 (i.e. ARM)?
> >>>>> It would be great to support that however, its  out of
> >>>>> scope for this patch set.
> >>>>> I’ll be happy to discuss it separately.
> >>>>
> >>>> I wasn't implying that this *should* work on ARM but rather whether this
> >>>> will break ARM somehow (because xen_hvm_domain() is true there).
> >>>>
> >>>>
> >>> Ok makes sense. TBH, I haven't tested this part of code on ARM and the series
> >>> was only support x86 guests hibernation.
> >>> Moreover, this notifier is there to distinguish between 2 PM
> >>> events PM SUSPEND and PM hibernation. Now since we only care about PM
> >>> HIBERNATION I may just remove this code and rely on "SHUTDOWN_SUSPEND" state.
> >>> However, I may have to fix other patches in the series where this check may
> >>> appear and cater it only for x86 right?
> >>
> >>
> >> I don't know what would happen if ARM guest tries to handle hibernation
> >> callbacks. The only ones that you are introducing are in block and net
> >> fronts and that's arch-independent.
> >>
> >>
> >> You do add a bunch of x86-specific code though (syscore ops), would
> >> something similar be needed for ARM?
> >>
> >>
> > I don't expect this to work out of the box on ARM. To start with something
> > similar will be needed for ARM too.
> > We may still want to keep the driver code as-is.
> > 
> > I understand the concern here wrt ARM, however, currently the support is only
> > proposed for x86 guests here and similar work could be carried out for ARM.
> > Also, if regular hibernation works correctly on arm, then all is needed is to
> > fix Xen side of things.
> > 
> > I am not sure what could be done to achieve any assurances on arm side as far as
> > this series is concerned.

Just to clarify: new features don't need to work on ARM or cause any
addition efforts to you to make them work on ARM. The patch series only
needs not to break existing code paths (on ARM and any other platforms).
It should also not make it overly difficult to implement the ARM side of
things (if there is one) at some point in the future.

FYI drivers/xen/manage.c is compiled and working on ARM today, however
Xen suspend/resume is not supported. I don't know for sure if
guest-initiated hibernation works because I have not tested it.


 
> If you are not sure what the effects are (or sure that it won't work) on
> ARM then I'd add IS_ENABLED(CONFIG_X86) check, i.e.
> 
> 
> if (!IS_ENABLED(CONFIG_X86) || !xen_hvm_domain())
> 	return -ENODEV;

That is a good principle to have and thanks for suggesting it. However,
in this specific case there is nothing in this patch that doesn't work
on ARM. From an ARM perspective I think we should enable it and
&xen_pm_notifier_block should be registered.

Given that all guests are HVM guests on ARM, it should work fine as is.


I gave a quick look at the rest of the series and everything looks fine
to me from an ARM perspective. I cannot imaging that the new freeze,
thaw, and restore callbacks for net and block are going to cause any
trouble on ARM. The two main x86-specific functions are
xen_syscore_suspend/resume and they look trivial to implement on ARM (in
the sense that they are likely going to look exactly the same.)


One question for Anchal: what's going to happen if you trigger a
hibernation, you have the new callbacks, but you are missing
xen_syscore_suspend/resume?

Is it any worse than not having the new freeze, thaw and restore
callbacks at all and try to do a hibernation?

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v2 01/11] xen/manage: keep track of the on-going suspend mode
  2020-07-21 19:55                   ` Anchal Agarwal
@ 2020-07-22  8:27                     ` Roger Pau Monné
  0 siblings, 0 replies; 44+ messages in thread
From: Roger Pau Monné @ 2020-07-22  8:27 UTC (permalink / raw)
  To: Anchal Agarwal
  Cc: marmarek, Boris Ostrovsky, tglx, mingo, bp, hpa, x86, jgross,
	linux-pm, linux-mm, kamatam, sstabellini, konrad.wilk, axboe,
	davem, rjw, len.brown, pavel, peterz, eduval, sblbir, xen-devel,
	vkuznets, netdev, linux-kernel, dwmw, benh

On Tue, Jul 21, 2020 at 07:55:09PM +0000, Anchal Agarwal wrote:
> On Tue, Jul 21, 2020 at 10:30:18AM +0200, Roger Pau Monné wrote:
> > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> > 
> > 
> > 
> > Marek: I'm adding you in case you could be able to give this a try and
> > make sure it doesn't break suspend for dom0.
> > 
> > On Tue, Jul 21, 2020 at 12:17:36AM +0000, Anchal Agarwal wrote:
> > > On Mon, Jul 20, 2020 at 11:37:05AM +0200, Roger Pau Monné wrote:
> > > > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> > > >
> > > >
> > > >
> > > > On Sat, Jul 18, 2020 at 09:47:04PM -0400, Boris Ostrovsky wrote:
> > > > > (Roger, question for you at the very end)
> > > > >
> > > > > On 7/17/20 3:10 PM, Anchal Agarwal wrote:
> > > > > > On Wed, Jul 15, 2020 at 05:18:08PM -0400, Boris Ostrovsky wrote:
> > > > > >> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> > > > > >>
> > > > > >>
> > > > > >>
> > > > > >> On 7/15/20 4:49 PM, Anchal Agarwal wrote:
> > > > > >>> On Mon, Jul 13, 2020 at 11:52:01AM -0400, Boris Ostrovsky wrote:
> > > > > >>>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> > > > > >>>>
> > > > > >>>>
> > > > > >>>>
> > > > > >>>> On 7/2/20 2:21 PM, Anchal Agarwal wrote:
> > > > > >>>> And PVH dom0.
> > > > > >>> That's another good use case to make it work with however, I still
> > > > > >>> think that should be tested/worked upon separately as the feature itself
> > > > > >>> (PVH Dom0) is very new.
> > > > > >>
> > > > > >> Same question here --- will this break PVH dom0?
> > > > > >>
> > > > > > I haven't tested it as a part of this series. Is that a blocker here?
> > > > >
> > > > >
> > > > > I suspect dom0 will not do well now as far as hibernation goes, in which
> > > > > case you are not breaking anything.
> > > > >
> > > > >
> > > > > Roger?
> > > >
> > > > I sadly don't have any box ATM that supports hibernation where I
> > > > could test it. We have hibernation support for PV dom0, so while I
> > > > haven't done anything specific to support or test hibernation on PVH
> > > > dom0 I would at least aim to not make this any worse, and hence the
> > > > check should at least also fail for a PVH dom0?
> > > >
> > > > if (!xen_hvm_domain() || xen_initial_domain())
> > > >     return -ENODEV;
> > > >
> > > > Ie: none of this should be applied to a PVH dom0, as it doesn't have
> > > > PV devices and hence should follow the bare metal device suspend.
> > > >
> > > So from what I understand you meant for any guest running on pvh dom0 should not
> > > hibernate if hibernation is triggered from within the guest or should they?
> > 
> > Er no to both I think. What I meant is that a PVH dom0 should be able
> > to properly suspend, and we should make sure this work doesn't make
> > this any harder (or breaks it if it's currently working).
> > 
> > Or at least that's how I understood the question raised by Boris.
> > 
> > You are adding code to the generic suspend path that's also used by dom0
> > in order to perform bare metal suspension. This is fine now for a PV
> > dom0 because the code is gated on xen_hvm_domain, but you should also
> > take into account that a PVH dom0 is considered a HVM domain, and
> > hence will get the notifier registered.
> >
> Ok that makes sense now. This is good to be safe, but my patch series is only to
> support domU hibernation, so I am not sure if this will affect pvh dom0.
> However, since I do not have a good way of testing sure I will add the check.
> 
> Moreover, in Patch-0004, I do register suspend/resume syscore_ops specifically for domU
> hibernation only if its xen_hvm_domain.

So if the hooks are only registered for domU, do you still need this
xen_hvm_domain check here?

I have to admit I'm not familiar with Linux PM suspend.

> I don't see any reason that should not
> be registered for domU's running on pvh dom0.

To be clear: it should be registered for all HVM domUs, regardless of
whether they are running on a PV or a PVH dom0. My intention was never
to suggest otherwise. It should be enabled for all HVM domUs, but
shouldn't be enabled for HVM dom0.

> Those suspend/resume callbacks will
> only be invoked in case hibernation and will be skipped if generic suspend path
> is in progress. Do you see any issue with that?

No, I think it's fine.

Roger.

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v2 04/11] x86/xen: add system core suspend and resume callbacks
  2020-07-02 18:22 ` [PATCH v2 04/11] x86/xen: add system core suspend and resume callbacks Anchal Agarwal
  2020-07-05 17:22   ` kernel test robot
@ 2020-07-22  9:08   ` Julien Grall
  1 sibling, 0 replies; 44+ messages in thread
From: Julien Grall @ 2020-07-22  9:08 UTC (permalink / raw)
  To: Anchal Agarwal, tglx, mingo, bp, hpa, x86, boris.ostrovsky,
	jgross, linux-pm, linux-mm, kamatam, sstabellini, konrad.wilk,
	roger.pau, axboe, davem, rjw, len.brown, pavel, peterz, eduval,
	sblbir, xen-devel, vkuznets, netdev, linux-kernel, dwmw, benh

Hi,

On 02/07/2020 19:22, Anchal Agarwal wrote:
> diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
> index 2521d6a306cd..9fa8a4082d68 100644
> --- a/include/xen/xen-ops.h
> +++ b/include/xen/xen-ops.h
> @@ -41,6 +41,8 @@ u64 xen_steal_clock(int cpu);
>   int xen_setup_shutdown_event(void);
>   
>   bool xen_is_xen_suspend(void);
> +void xen_setup_syscore_ops(void);

The function is only implemented and used by x86. So shouldn't this be 
declared in an x86 header?

Cheers,

-- 
Julien Grall

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v2 01/11] xen/manage: keep track of the on-going suspend mode
  2020-07-22  0:18                 ` Stefano Stabellini
@ 2020-07-22 18:02                   ` Anchal Agarwal
  2020-07-22 22:45                     ` Boris Ostrovsky
  2020-07-22 23:49                     ` Stefano Stabellini
  0 siblings, 2 replies; 44+ messages in thread
From: Anchal Agarwal @ 2020-07-22 18:02 UTC (permalink / raw)
  To: Stefano Stabellini
  Cc: Boris Ostrovsky, tglx, mingo, bp, hpa, x86, jgross, linux-pm,
	linux-mm, kamatam, konrad.wilk, roger.pau, axboe, davem, rjw,
	len.brown, pavel, peterz, eduval, sblbir, xen-devel, vkuznets,
	netdev, linux-kernel, dwmw, benh

On Tue, Jul 21, 2020 at 05:18:34PM -0700, Stefano Stabellini wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> On Tue, 21 Jul 2020, Boris Ostrovsky wrote:
> > >>>>>> +static int xen_setup_pm_notifier(void)
> > >>>>>> +{
> > >>>>>> +     if (!xen_hvm_domain())
> > >>>>>> +             return -ENODEV;
> > >>>>>>
> > >>>>>> I forgot --- what did we decide about non-x86 (i.e. ARM)?
> > >>>>> It would be great to support that however, its  out of
> > >>>>> scope for this patch set.
> > >>>>> I’ll be happy to discuss it separately.
> > >>>>
> > >>>> I wasn't implying that this *should* work on ARM but rather whether this
> > >>>> will break ARM somehow (because xen_hvm_domain() is true there).
> > >>>>
> > >>>>
> > >>> Ok makes sense. TBH, I haven't tested this part of code on ARM and the series
> > >>> was only support x86 guests hibernation.
> > >>> Moreover, this notifier is there to distinguish between 2 PM
> > >>> events PM SUSPEND and PM hibernation. Now since we only care about PM
> > >>> HIBERNATION I may just remove this code and rely on "SHUTDOWN_SUSPEND" state.
> > >>> However, I may have to fix other patches in the series where this check may
> > >>> appear and cater it only for x86 right?
> > >>
> > >>
> > >> I don't know what would happen if ARM guest tries to handle hibernation
> > >> callbacks. The only ones that you are introducing are in block and net
> > >> fronts and that's arch-independent.
> > >>
> > >>
> > >> You do add a bunch of x86-specific code though (syscore ops), would
> > >> something similar be needed for ARM?
> > >>
> > >>
> > > I don't expect this to work out of the box on ARM. To start with something
> > > similar will be needed for ARM too.
> > > We may still want to keep the driver code as-is.
> > >
> > > I understand the concern here wrt ARM, however, currently the support is only
> > > proposed for x86 guests here and similar work could be carried out for ARM.
> > > Also, if regular hibernation works correctly on arm, then all is needed is to
> > > fix Xen side of things.
> > >
> > > I am not sure what could be done to achieve any assurances on arm side as far as
> > > this series is concerned.
> 
> Just to clarify: new features don't need to work on ARM or cause any
> addition efforts to you to make them work on ARM. The patch series only
> needs not to break existing code paths (on ARM and any other platforms).
> It should also not make it overly difficult to implement the ARM side of
> things (if there is one) at some point in the future.
> 
> FYI drivers/xen/manage.c is compiled and working on ARM today, however
> Xen suspend/resume is not supported. I don't know for sure if
> guest-initiated hibernation works because I have not tested it.
> 
> 
> 
> > If you are not sure what the effects are (or sure that it won't work) on
> > ARM then I'd add IS_ENABLED(CONFIG_X86) check, i.e.
> >
> >
> > if (!IS_ENABLED(CONFIG_X86) || !xen_hvm_domain())
> >       return -ENODEV;
> 
> That is a good principle to have and thanks for suggesting it. However,
> in this specific case there is nothing in this patch that doesn't work
> on ARM. From an ARM perspective I think we should enable it and
> &xen_pm_notifier_block should be registered.
> 
This question is for Boris, I think you we decided to get rid of the notifier
in V3 as all we need  to check is SHUTDOWN_SUSPEND state which sounds plausible
to me. So this check may go away. It may still be needed for sycore_ops
callbacks registration.
> Given that all guests are HVM guests on ARM, it should work fine as is.
> 
> 
> I gave a quick look at the rest of the series and everything looks fine
> to me from an ARM perspective. I cannot imaging that the new freeze,
> thaw, and restore callbacks for net and block are going to cause any
> trouble on ARM. The two main x86-specific functions are
> xen_syscore_suspend/resume and they look trivial to implement on ARM (in
> the sense that they are likely going to look exactly the same.)
> 
Yes but for now since things are not tested I will put this
!IS_ENABLED(CONFIG_X86) on syscore_ops calls registration part just to be safe
and not break anything.
> 
> One question for Anchal: what's going to happen if you trigger a
> hibernation, you have the new callbacks, but you are missing
> xen_syscore_suspend/resume?
> 
> Is it any worse than not having the new freeze, thaw and restore
> callbacks at all and try to do a hibernation?
If callbacks are not there, I don't expect hibernation to work correctly.
These callbacks takes care of xen primitives like shared_info_page,
grant table, sched clock, runstate time which are important to save the correct
state of the guest and bring it back up. Other patches in the series, adds all
the logic to these syscore callbacks. Freeze/thaw/restore are just there for at driver
level.

Thanks,
Anchal

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v2 01/11] xen/manage: keep track of the on-going suspend mode
  2020-07-22 18:02                   ` Anchal Agarwal
@ 2020-07-22 22:45                     ` Boris Ostrovsky
  2020-07-22 23:49                     ` Stefano Stabellini
  1 sibling, 0 replies; 44+ messages in thread
From: Boris Ostrovsky @ 2020-07-22 22:45 UTC (permalink / raw)
  To: Anchal Agarwal, Stefano Stabellini
  Cc: tglx, mingo, bp, hpa, x86, jgross, linux-pm, linux-mm, kamatam,
	konrad.wilk, roger.pau, axboe, davem, rjw, len.brown, pavel,
	peterz, eduval, sblbir, xen-devel, vkuznets, netdev,
	linux-kernel, dwmw, benh

On 7/22/20 2:02 PM, Anchal Agarwal wrote:
> On Tue, Jul 21, 2020 at 05:18:34PM -0700, Stefano Stabellini wrote:
>>
>>
>>> If you are not sure what the effects are (or sure that it won't work) on
>>> ARM then I'd add IS_ENABLED(CONFIG_X86) check, i.e.
>>>
>>>
>>> if (!IS_ENABLED(CONFIG_X86) || !xen_hvm_domain())
>>>       return -ENODEV;
>> That is a good principle to have and thanks for suggesting it. However,
>> in this specific case there is nothing in this patch that doesn't work
>> on ARM. From an ARM perspective I think we should enable it and
>> &xen_pm_notifier_block should be registered.
>>
> This question is for Boris, I think you we decided to get rid of the notifier
> in V3 as all we need  to check is SHUTDOWN_SUSPEND state which sounds plausible
> to me. So this check may go away. It may still be needed for sycore_ops
> callbacks registration.


If this check is going away then I guess there is nothing to do here.


My concern isn't about this particular notifier but rather whether this
feature may affect existing functionality (ARM and PVH dom0). If Stefano
feels this should be fine for ARM then so be it.


-boris


>> Given that all guests are HVM guests on ARM, it should work fine as is.
>>
>>
>> I gave a quick look at the rest of the series and everything looks fine
>> to me from an ARM perspective. I cannot imaging that the new freeze,
>> thaw, and restore callbacks for net and block are going to cause any
>> trouble on ARM. The two main x86-specific functions are
>> xen_syscore_suspend/resume and they look trivial to implement on ARM (in
>> the sense that they are likely going to look exactly the same.)
>>
> Yes but for now since things are not tested I will put this
> !IS_ENABLED(CONFIG_X86) on syscore_ops calls registration part just to be safe
> and not break anything.
>> One question for Anchal: what's going to happen if you trigger a
>> hibernation, you have the new callbacks, but you are missing
>> xen_syscore_suspend/resume?
>>
>> Is it any worse than not having the new freeze, thaw and restore
>> callbacks at all and try to do a hibernation?
> If callbacks are not there, I don't expect hibernation to work correctly.
> These callbacks takes care of xen primitives like shared_info_page,
> grant table, sched clock, runstate time which are important to save the correct
> state of the guest and bring it back up. Other patches in the series, adds all
> the logic to these syscore callbacks. Freeze/thaw/restore are just there for at driver
> level.
>
> Thanks,
> Anchal




^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v2 01/11] xen/manage: keep track of the on-going suspend mode
  2020-07-22 18:02                   ` Anchal Agarwal
  2020-07-22 22:45                     ` Boris Ostrovsky
@ 2020-07-22 23:49                     ` Stefano Stabellini
  2020-07-23 22:57                       ` Anchal Agarwal
  1 sibling, 1 reply; 44+ messages in thread
From: Stefano Stabellini @ 2020-07-22 23:49 UTC (permalink / raw)
  To: Anchal Agarwal
  Cc: Stefano Stabellini, Boris Ostrovsky, tglx, mingo, bp, hpa, x86,
	jgross, linux-pm, linux-mm, kamatam, konrad.wilk, roger.pau,
	axboe, davem, rjw, len.brown, pavel, peterz, eduval, sblbir,
	xen-devel, vkuznets, netdev, linux-kernel, dwmw, benh


[-- Attachment #1: Type: text/plain, Size: 5299 bytes --]

On Wed, 22 Jul 2020, Anchal Agarwal wrote:
> On Tue, Jul 21, 2020 at 05:18:34PM -0700, Stefano Stabellini wrote:
> > On Tue, 21 Jul 2020, Boris Ostrovsky wrote:
> > > >>>>>> +static int xen_setup_pm_notifier(void)
> > > >>>>>> +{
> > > >>>>>> +     if (!xen_hvm_domain())
> > > >>>>>> +             return -ENODEV;
> > > >>>>>>
> > > >>>>>> I forgot --- what did we decide about non-x86 (i.e. ARM)?
> > > >>>>> It would be great to support that however, its  out of
> > > >>>>> scope for this patch set.
> > > >>>>> I’ll be happy to discuss it separately.
> > > >>>>
> > > >>>> I wasn't implying that this *should* work on ARM but rather whether this
> > > >>>> will break ARM somehow (because xen_hvm_domain() is true there).
> > > >>>>
> > > >>>>
> > > >>> Ok makes sense. TBH, I haven't tested this part of code on ARM and the series
> > > >>> was only support x86 guests hibernation.
> > > >>> Moreover, this notifier is there to distinguish between 2 PM
> > > >>> events PM SUSPEND and PM hibernation. Now since we only care about PM
> > > >>> HIBERNATION I may just remove this code and rely on "SHUTDOWN_SUSPEND" state.
> > > >>> However, I may have to fix other patches in the series where this check may
> > > >>> appear and cater it only for x86 right?
> > > >>
> > > >>
> > > >> I don't know what would happen if ARM guest tries to handle hibernation
> > > >> callbacks. The only ones that you are introducing are in block and net
> > > >> fronts and that's arch-independent.
> > > >>
> > > >>
> > > >> You do add a bunch of x86-specific code though (syscore ops), would
> > > >> something similar be needed for ARM?
> > > >>
> > > >>
> > > > I don't expect this to work out of the box on ARM. To start with something
> > > > similar will be needed for ARM too.
> > > > We may still want to keep the driver code as-is.
> > > >
> > > > I understand the concern here wrt ARM, however, currently the support is only
> > > > proposed for x86 guests here and similar work could be carried out for ARM.
> > > > Also, if regular hibernation works correctly on arm, then all is needed is to
> > > > fix Xen side of things.
> > > >
> > > > I am not sure what could be done to achieve any assurances on arm side as far as
> > > > this series is concerned.
> > 
> > Just to clarify: new features don't need to work on ARM or cause any
> > addition efforts to you to make them work on ARM. The patch series only
> > needs not to break existing code paths (on ARM and any other platforms).
> > It should also not make it overly difficult to implement the ARM side of
> > things (if there is one) at some point in the future.
> > 
> > FYI drivers/xen/manage.c is compiled and working on ARM today, however
> > Xen suspend/resume is not supported. I don't know for sure if
> > guest-initiated hibernation works because I have not tested it.
> > 
> > 
> > 
> > > If you are not sure what the effects are (or sure that it won't work) on
> > > ARM then I'd add IS_ENABLED(CONFIG_X86) check, i.e.
> > >
> > >
> > > if (!IS_ENABLED(CONFIG_X86) || !xen_hvm_domain())
> > >       return -ENODEV;
> > 
> > That is a good principle to have and thanks for suggesting it. However,
> > in this specific case there is nothing in this patch that doesn't work
> > on ARM. From an ARM perspective I think we should enable it and
> > &xen_pm_notifier_block should be registered.
> > 
> This question is for Boris, I think you we decided to get rid of the notifier
> in V3 as all we need  to check is SHUTDOWN_SUSPEND state which sounds plausible
> to me. So this check may go away. It may still be needed for sycore_ops
> callbacks registration.
> > Given that all guests are HVM guests on ARM, it should work fine as is.
> > 
> > 
> > I gave a quick look at the rest of the series and everything looks fine
> > to me from an ARM perspective. I cannot imaging that the new freeze,
> > thaw, and restore callbacks for net and block are going to cause any
> > trouble on ARM. The two main x86-specific functions are
> > xen_syscore_suspend/resume and they look trivial to implement on ARM (in
> > the sense that they are likely going to look exactly the same.)
> > 
> Yes but for now since things are not tested I will put this
> !IS_ENABLED(CONFIG_X86) on syscore_ops calls registration part just to be safe
> and not break anything.
> > 
> > One question for Anchal: what's going to happen if you trigger a
> > hibernation, you have the new callbacks, but you are missing
> > xen_syscore_suspend/resume?
> > 
> > Is it any worse than not having the new freeze, thaw and restore
> > callbacks at all and try to do a hibernation?
> If callbacks are not there, I don't expect hibernation to work correctly.
> These callbacks takes care of xen primitives like shared_info_page,
> grant table, sched clock, runstate time which are important to save the correct
> state of the guest and bring it back up. Other patches in the series, adds all
> the logic to these syscore callbacks. Freeze/thaw/restore are just there for at driver
> level.

I meant the other way around :-)  Let me rephrase the question.

Do you think that implementing freeze/thaw/restore at the driver level
without having xen_syscore_suspend/resume can potentially make things
worse compared to not having freeze/thaw/restore at the driver level at
all?

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v2 01/11] xen/manage: keep track of the on-going suspend mode
  2020-07-22 23:49                     ` Stefano Stabellini
@ 2020-07-23 22:57                       ` Anchal Agarwal
  2020-07-24 23:01                         ` Stefano Stabellini
  0 siblings, 1 reply; 44+ messages in thread
From: Anchal Agarwal @ 2020-07-23 22:57 UTC (permalink / raw)
  To: Stefano Stabellini
  Cc: Boris Ostrovsky, tglx, mingo, bp, hpa, x86, jgross, linux-pm,
	linux-mm, kamatam, konrad.wilk, roger.pau, axboe, davem, rjw,
	len.brown, pavel, peterz, eduval, sblbir, xen-devel, vkuznets,
	netdev, linux-kernel, dwmw, benh

On Wed, Jul 22, 2020 at 04:49:16PM -0700, Stefano Stabellini wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> On Wed, 22 Jul 2020, Anchal Agarwal wrote:
> > On Tue, Jul 21, 2020 at 05:18:34PM -0700, Stefano Stabellini wrote:
> > > On Tue, 21 Jul 2020, Boris Ostrovsky wrote:
> > > > >>>>>> +static int xen_setup_pm_notifier(void)
> > > > >>>>>> +{
> > > > >>>>>> +     if (!xen_hvm_domain())
> > > > >>>>>> +             return -ENODEV;
> > > > >>>>>>
> > > > >>>>>> I forgot --- what did we decide about non-x86 (i.e. ARM)?
> > > > >>>>> It would be great to support that however, its  out of
> > > > >>>>> scope for this patch set.
> > > > >>>>> I’ll be happy to discuss it separately.
> > > > >>>>
> > > > >>>> I wasn't implying that this *should* work on ARM but rather whether this
> > > > >>>> will break ARM somehow (because xen_hvm_domain() is true there).
> > > > >>>>
> > > > >>>>
> > > > >>> Ok makes sense. TBH, I haven't tested this part of code on ARM and the series
> > > > >>> was only support x86 guests hibernation.
> > > > >>> Moreover, this notifier is there to distinguish between 2 PM
> > > > >>> events PM SUSPEND and PM hibernation. Now since we only care about PM
> > > > >>> HIBERNATION I may just remove this code and rely on "SHUTDOWN_SUSPEND" state.
> > > > >>> However, I may have to fix other patches in the series where this check may
> > > > >>> appear and cater it only for x86 right?
> > > > >>
> > > > >>
> > > > >> I don't know what would happen if ARM guest tries to handle hibernation
> > > > >> callbacks. The only ones that you are introducing are in block and net
> > > > >> fronts and that's arch-independent.
> > > > >>
> > > > >>
> > > > >> You do add a bunch of x86-specific code though (syscore ops), would
> > > > >> something similar be needed for ARM?
> > > > >>
> > > > >>
> > > > > I don't expect this to work out of the box on ARM. To start with something
> > > > > similar will be needed for ARM too.
> > > > > We may still want to keep the driver code as-is.
> > > > >
> > > > > I understand the concern here wrt ARM, however, currently the support is only
> > > > > proposed for x86 guests here and similar work could be carried out for ARM.
> > > > > Also, if regular hibernation works correctly on arm, then all is needed is to
> > > > > fix Xen side of things.
> > > > >
> > > > > I am not sure what could be done to achieve any assurances on arm side as far as
> > > > > this series is concerned.
> > >
> > > Just to clarify: new features don't need to work on ARM or cause any
> > > addition efforts to you to make them work on ARM. The patch series only
> > > needs not to break existing code paths (on ARM and any other platforms).
> > > It should also not make it overly difficult to implement the ARM side of
> > > things (if there is one) at some point in the future.
> > >
> > > FYI drivers/xen/manage.c is compiled and working on ARM today, however
> > > Xen suspend/resume is not supported. I don't know for sure if
> > > guest-initiated hibernation works because I have not tested it.
> > >
> > >
> > >
> > > > If you are not sure what the effects are (or sure that it won't work) on
> > > > ARM then I'd add IS_ENABLED(CONFIG_X86) check, i.e.
> > > >
> > > >
> > > > if (!IS_ENABLED(CONFIG_X86) || !xen_hvm_domain())
> > > >       return -ENODEV;
> > >
> > > That is a good principle to have and thanks for suggesting it. However,
> > > in this specific case there is nothing in this patch that doesn't work
> > > on ARM. From an ARM perspective I think we should enable it and
> > > &xen_pm_notifier_block should be registered.
> > >
> > This question is for Boris, I think you we decided to get rid of the notifier
> > in V3 as all we need  to check is SHUTDOWN_SUSPEND state which sounds plausible
> > to me. So this check may go away. It may still be needed for sycore_ops
> > callbacks registration.
> > > Given that all guests are HVM guests on ARM, it should work fine as is.
> > >
> > >
> > > I gave a quick look at the rest of the series and everything looks fine
> > > to me from an ARM perspective. I cannot imaging that the new freeze,
> > > thaw, and restore callbacks for net and block are going to cause any
> > > trouble on ARM. The two main x86-specific functions are
> > > xen_syscore_suspend/resume and they look trivial to implement on ARM (in
> > > the sense that they are likely going to look exactly the same.)
> > >
> > Yes but for now since things are not tested I will put this
> > !IS_ENABLED(CONFIG_X86) on syscore_ops calls registration part just to be safe
> > and not break anything.
> > >
> > > One question for Anchal: what's going to happen if you trigger a
> > > hibernation, you have the new callbacks, but you are missing
> > > xen_syscore_suspend/resume?
> > >
> > > Is it any worse than not having the new freeze, thaw and restore
> > > callbacks at all and try to do a hibernation?
> > If callbacks are not there, I don't expect hibernation to work correctly.
> > These callbacks takes care of xen primitives like shared_info_page,
> > grant table, sched clock, runstate time which are important to save the correct
> > state of the guest and bring it back up. Other patches in the series, adds all
> > the logic to these syscore callbacks. Freeze/thaw/restore are just there for at driver
> > level.
> 
> I meant the other way around :-)  Let me rephrase the question.
> 
> Do you think that implementing freeze/thaw/restore at the driver level
> without having xen_syscore_suspend/resume can potentially make things
> worse compared to not having freeze/thaw/restore at the driver level at
> all?
I think in both the cases I don't expect it to work. System may end up in
different state if you register vs not. Hibernation does not work properly
at least for domU instances without these changes on x86 and I am assuming the
same for ARM.

If you do not register freeze/thaw/restore callbacks for arm, then on
invocation of xenbus_dev_suspend, default suspend/resume callbacks
will be called for each driver and since you do not have any code to save domU's
xen primitives state (syscore_ops), hibernation will either fail or will demand a reboot.
I do no have setup to test the current state of ARM's hibernation

If you only register freeze/thaw/restore and no syscore_ops, it will again fail.
Since, I do not have an ARM setup running, I quickly ran a similar test on x86,
may not be an apple to apple comparison but instance failed to resume or I
should say stuck showing huge jump in time and required a reboot.

Now if this doesn't happen currently when you trigger hibernation on arm domU
instances or if system is still alive when you trigger hibernation in xen guest
then not registering the callbacks may be a better idea. In that case  may be 
I need to put arch specific check when registering freeze/thaw/restore handlers.

Hope that answers your question.

Thanks,
Anchal


^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v2 01/11] xen/manage: keep track of the on-going suspend mode
  2020-07-23 22:57                       ` Anchal Agarwal
@ 2020-07-24 23:01                         ` Stefano Stabellini
  2020-07-27 22:08                           ` Boris Ostrovsky
  0 siblings, 1 reply; 44+ messages in thread
From: Stefano Stabellini @ 2020-07-24 23:01 UTC (permalink / raw)
  To: Anchal Agarwal
  Cc: Stefano Stabellini, Boris Ostrovsky, tglx, mingo, bp, hpa, x86,
	jgross, linux-pm, linux-mm, kamatam, konrad.wilk, roger.pau,
	axboe, davem, rjw, len.brown, pavel, peterz, eduval, sblbir,
	xen-devel, vkuznets, netdev, linux-kernel, dwmw, benh


[-- Attachment #1: Type: text/plain, Size: 7712 bytes --]

On Thu, 23 Jul 2020, Anchal Agarwal wrote:
> On Wed, Jul 22, 2020 at 04:49:16PM -0700, Stefano Stabellini wrote:
> > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> > 
> > 
> > 
> > On Wed, 22 Jul 2020, Anchal Agarwal wrote:
> > > On Tue, Jul 21, 2020 at 05:18:34PM -0700, Stefano Stabellini wrote:
> > > > On Tue, 21 Jul 2020, Boris Ostrovsky wrote:
> > > > > >>>>>> +static int xen_setup_pm_notifier(void)
> > > > > >>>>>> +{
> > > > > >>>>>> +     if (!xen_hvm_domain())
> > > > > >>>>>> +             return -ENODEV;
> > > > > >>>>>>
> > > > > >>>>>> I forgot --- what did we decide about non-x86 (i.e. ARM)?
> > > > > >>>>> It would be great to support that however, its  out of
> > > > > >>>>> scope for this patch set.
> > > > > >>>>> I’ll be happy to discuss it separately.
> > > > > >>>>
> > > > > >>>> I wasn't implying that this *should* work on ARM but rather whether this
> > > > > >>>> will break ARM somehow (because xen_hvm_domain() is true there).
> > > > > >>>>
> > > > > >>>>
> > > > > >>> Ok makes sense. TBH, I haven't tested this part of code on ARM and the series
> > > > > >>> was only support x86 guests hibernation.
> > > > > >>> Moreover, this notifier is there to distinguish between 2 PM
> > > > > >>> events PM SUSPEND and PM hibernation. Now since we only care about PM
> > > > > >>> HIBERNATION I may just remove this code and rely on "SHUTDOWN_SUSPEND" state.
> > > > > >>> However, I may have to fix other patches in the series where this check may
> > > > > >>> appear and cater it only for x86 right?
> > > > > >>
> > > > > >>
> > > > > >> I don't know what would happen if ARM guest tries to handle hibernation
> > > > > >> callbacks. The only ones that you are introducing are in block and net
> > > > > >> fronts and that's arch-independent.
> > > > > >>
> > > > > >>
> > > > > >> You do add a bunch of x86-specific code though (syscore ops), would
> > > > > >> something similar be needed for ARM?
> > > > > >>
> > > > > >>
> > > > > > I don't expect this to work out of the box on ARM. To start with something
> > > > > > similar will be needed for ARM too.
> > > > > > We may still want to keep the driver code as-is.
> > > > > >
> > > > > > I understand the concern here wrt ARM, however, currently the support is only
> > > > > > proposed for x86 guests here and similar work could be carried out for ARM.
> > > > > > Also, if regular hibernation works correctly on arm, then all is needed is to
> > > > > > fix Xen side of things.
> > > > > >
> > > > > > I am not sure what could be done to achieve any assurances on arm side as far as
> > > > > > this series is concerned.
> > > >
> > > > Just to clarify: new features don't need to work on ARM or cause any
> > > > addition efforts to you to make them work on ARM. The patch series only
> > > > needs not to break existing code paths (on ARM and any other platforms).
> > > > It should also not make it overly difficult to implement the ARM side of
> > > > things (if there is one) at some point in the future.
> > > >
> > > > FYI drivers/xen/manage.c is compiled and working on ARM today, however
> > > > Xen suspend/resume is not supported. I don't know for sure if
> > > > guest-initiated hibernation works because I have not tested it.
> > > >
> > > >
> > > >
> > > > > If you are not sure what the effects are (or sure that it won't work) on
> > > > > ARM then I'd add IS_ENABLED(CONFIG_X86) check, i.e.
> > > > >
> > > > >
> > > > > if (!IS_ENABLED(CONFIG_X86) || !xen_hvm_domain())
> > > > >       return -ENODEV;
> > > >
> > > > That is a good principle to have and thanks for suggesting it. However,
> > > > in this specific case there is nothing in this patch that doesn't work
> > > > on ARM. From an ARM perspective I think we should enable it and
> > > > &xen_pm_notifier_block should be registered.
> > > >
> > > This question is for Boris, I think you we decided to get rid of the notifier
> > > in V3 as all we need  to check is SHUTDOWN_SUSPEND state which sounds plausible
> > > to me. So this check may go away. It may still be needed for sycore_ops
> > > callbacks registration.
> > > > Given that all guests are HVM guests on ARM, it should work fine as is.
> > > >
> > > >
> > > > I gave a quick look at the rest of the series and everything looks fine
> > > > to me from an ARM perspective. I cannot imaging that the new freeze,
> > > > thaw, and restore callbacks for net and block are going to cause any
> > > > trouble on ARM. The two main x86-specific functions are
> > > > xen_syscore_suspend/resume and they look trivial to implement on ARM (in
> > > > the sense that they are likely going to look exactly the same.)
> > > >
> > > Yes but for now since things are not tested I will put this
> > > !IS_ENABLED(CONFIG_X86) on syscore_ops calls registration part just to be safe
> > > and not break anything.
> > > >
> > > > One question for Anchal: what's going to happen if you trigger a
> > > > hibernation, you have the new callbacks, but you are missing
> > > > xen_syscore_suspend/resume?
> > > >
> > > > Is it any worse than not having the new freeze, thaw and restore
> > > > callbacks at all and try to do a hibernation?
> > > If callbacks are not there, I don't expect hibernation to work correctly.
> > > These callbacks takes care of xen primitives like shared_info_page,
> > > grant table, sched clock, runstate time which are important to save the correct
> > > state of the guest and bring it back up. Other patches in the series, adds all
> > > the logic to these syscore callbacks. Freeze/thaw/restore are just there for at driver
> > > level.
> > 
> > I meant the other way around :-)  Let me rephrase the question.
> > 
> > Do you think that implementing freeze/thaw/restore at the driver level
> > without having xen_syscore_suspend/resume can potentially make things
> > worse compared to not having freeze/thaw/restore at the driver level at
> > all?
> I think in both the cases I don't expect it to work. System may end up in
> different state if you register vs not. Hibernation does not work properly
> at least for domU instances without these changes on x86 and I am assuming the
> same for ARM.
> 
> If you do not register freeze/thaw/restore callbacks for arm, then on
> invocation of xenbus_dev_suspend, default suspend/resume callbacks
> will be called for each driver and since you do not have any code to save domU's
> xen primitives state (syscore_ops), hibernation will either fail or will demand a reboot.
> I do no have setup to test the current state of ARM's hibernation
> 
> If you only register freeze/thaw/restore and no syscore_ops, it will again fail.
> Since, I do not have an ARM setup running, I quickly ran a similar test on x86,
> may not be an apple to apple comparison but instance failed to resume or I
> should say stuck showing huge jump in time and required a reboot.
> 
> Now if this doesn't happen currently when you trigger hibernation on arm domU
> instances or if system is still alive when you trigger hibernation in xen guest
> then not registering the callbacks may be a better idea. In that case  may be 
> I need to put arch specific check when registering freeze/thaw/restore handlers.
> 
> Hope that answers your question.

Yes, it does, thank you. I'd rather not introduce unknown regressions so
I would recommend to add an arch-specific check on registering
freeze/thaw/restore handlers. Maybe something like the following:

#ifdef CONFIG_X86
    .freeze = blkfront_freeze,
    .thaw = blkfront_restore,
    .restore = blkfront_restore
#endif


maybe Boris has a better suggestion on how to do it

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v2 01/11] xen/manage: keep track of the on-going suspend mode
  2020-07-24 23:01                         ` Stefano Stabellini
@ 2020-07-27 22:08                           ` Boris Ostrovsky
  2020-07-30 23:06                             ` Anchal Agarwal
  0 siblings, 1 reply; 44+ messages in thread
From: Boris Ostrovsky @ 2020-07-27 22:08 UTC (permalink / raw)
  To: Stefano Stabellini, Anchal Agarwal
  Cc: tglx, mingo, bp, hpa, x86, jgross, linux-pm, linux-mm, kamatam,
	konrad.wilk, roger.pau, axboe, davem, rjw, len.brown, pavel,
	peterz, eduval, sblbir, xen-devel, vkuznets, netdev,
	linux-kernel, dwmw, benh

On 7/24/20 7:01 PM, Stefano Stabellini wrote:
> Yes, it does, thank you. I'd rather not introduce unknown regressions so
> I would recommend to add an arch-specific check on registering
> freeze/thaw/restore handlers. Maybe something like the following:
>
> #ifdef CONFIG_X86
>     .freeze = blkfront_freeze,
>     .thaw = blkfront_restore,
>     .restore = blkfront_restore
> #endif
>
>
> maybe Boris has a better suggestion on how to do it


An alternative might be to still install pm notifier in
drivers/xen/manage.c (I think as result of latest discussions we decided
we won't need it) and return -ENOTSUPP for ARM for
PM_HIBERNATION_PREPARE and friends. Would that work?


-boris



^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v2 01/11] xen/manage: keep track of the on-going suspend mode
  2020-07-27 22:08                           ` Boris Ostrovsky
@ 2020-07-30 23:06                             ` Anchal Agarwal
  2020-07-31 14:13                               ` Boris Ostrovsky
  0 siblings, 1 reply; 44+ messages in thread
From: Anchal Agarwal @ 2020-07-30 23:06 UTC (permalink / raw)
  To: Boris Ostrovsky
  Cc: Stefano Stabellini, tglx, mingo, bp, hpa, x86, jgross, linux-pm,
	linux-mm, kamatam, konrad.wilk, roger.pau, axboe, davem, rjw,
	len.brown, pavel, peterz, eduval, sblbir, xen-devel, vkuznets,
	netdev, linux-kernel, dwmw, benh

On Mon, Jul 27, 2020 at 06:08:29PM -0400, Boris Ostrovsky wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> On 7/24/20 7:01 PM, Stefano Stabellini wrote:
> > Yes, it does, thank you. I'd rather not introduce unknown regressions so
> > I would recommend to add an arch-specific check on registering
> > freeze/thaw/restore handlers. Maybe something like the following:
> >
> > #ifdef CONFIG_X86
> >     .freeze = blkfront_freeze,
> >     .thaw = blkfront_restore,
> >     .restore = blkfront_restore
> > #endif
> >
> >
> > maybe Boris has a better suggestion on how to do it
> 
> 
> An alternative might be to still install pm notifier in
> drivers/xen/manage.c (I think as result of latest discussions we decided
> we won't need it) and return -ENOTSUPP for ARM for
> PM_HIBERNATION_PREPARE and friends. Would that work?
>
I think the question here is for registering driver specific freeze/thaw/restore
callbacks for x86 only. I have dropped the pm_notifier in the v3 still pending
testing. So I think just registering driver specific callbacks for x86 only is a
good option. What do you think?

Anchal
> 
> -boris
> 
> 
> 

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v2 01/11] xen/manage: keep track of the on-going suspend mode
  2020-07-30 23:06                             ` Anchal Agarwal
@ 2020-07-31 14:13                               ` Boris Ostrovsky
  2020-07-31 14:25                                 ` Rafael J. Wysocki
  2020-08-04 23:42                                 ` Anchal Agarwal
  0 siblings, 2 replies; 44+ messages in thread
From: Boris Ostrovsky @ 2020-07-31 14:13 UTC (permalink / raw)
  To: Anchal Agarwal
  Cc: Stefano Stabellini, tglx, mingo, bp, hpa, x86, jgross, linux-pm,
	linux-mm, kamatam, konrad.wilk, roger.pau, axboe, davem, rjw,
	len.brown, pavel, peterz, eduval, sblbir, xen-devel, vkuznets,
	netdev, linux-kernel, dwmw, benh

On 7/30/20 7:06 PM, Anchal Agarwal wrote:
> On Mon, Jul 27, 2020 at 06:08:29PM -0400, Boris Ostrovsky wrote:
>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>>
>>
>>
>> On 7/24/20 7:01 PM, Stefano Stabellini wrote:
>>> Yes, it does, thank you. I'd rather not introduce unknown regressions so
>>> I would recommend to add an arch-specific check on registering
>>> freeze/thaw/restore handlers. Maybe something like the following:
>>>
>>> #ifdef CONFIG_X86
>>>     .freeze = blkfront_freeze,
>>>     .thaw = blkfront_restore,
>>>     .restore = blkfront_restore
>>> #endif
>>>
>>>
>>> maybe Boris has a better suggestion on how to do it
>>
>> An alternative might be to still install pm notifier in
>> drivers/xen/manage.c (I think as result of latest discussions we decided
>> we won't need it) and return -ENOTSUPP for ARM for
>> PM_HIBERNATION_PREPARE and friends. Would that work?
>>
> I think the question here is for registering driver specific freeze/thaw/restore
> callbacks for x86 only. I have dropped the pm_notifier in the v3 still pending
> testing. So I think just registering driver specific callbacks for x86 only is a
> good option. What do you think?


I suggested using the notifier under assumption that if it returns an
error then that will prevent callbacks to be called because hibernation
will be effectively disabled. But I haven't looked at PM code so I don't
know whether this is actually the case.


The advantage of doing it in the notifier is that instead of adding
ifdefs to each driver you will be able to prevent callbacks from a
single place. Plus you can use this do disable hibernation for PVH dom0
as well.



-boris




^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v2 01/11] xen/manage: keep track of the on-going suspend mode
  2020-07-31 14:13                               ` Boris Ostrovsky
@ 2020-07-31 14:25                                 ` Rafael J. Wysocki
  2020-08-04 23:42                                 ` Anchal Agarwal
  1 sibling, 0 replies; 44+ messages in thread
From: Rafael J. Wysocki @ 2020-07-31 14:25 UTC (permalink / raw)
  To: Boris Ostrovsky
  Cc: Anchal Agarwal, Stefano Stabellini, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, H. Peter Anvin, the arch/x86 maintainers,
	Juergen Gross, Linux PM, Linux Memory Management List, Kamata,
	Munehisa, Konrad Rzeszutek Wilk, roger.pau, Jens Axboe,
	David Miller, Rafael J. Wysocki, Len Brown, Pavel Machek,
	Peter Zijlstra, Eduardo Valentin, Singh, Balbir, xen-devel,
	Vitaly Kuznetsov, netdev, Linux Kernel Mailing List,
	David Woodhouse, Benjamin Herrenschmidt

On Fri, Jul 31, 2020 at 4:14 PM Boris Ostrovsky
<boris.ostrovsky@oracle.com> wrote:
>
> On 7/30/20 7:06 PM, Anchal Agarwal wrote:
> > On Mon, Jul 27, 2020 at 06:08:29PM -0400, Boris Ostrovsky wrote:
> >> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> >>
> >>
> >>
> >> On 7/24/20 7:01 PM, Stefano Stabellini wrote:
> >>> Yes, it does, thank you. I'd rather not introduce unknown regressions so
> >>> I would recommend to add an arch-specific check on registering
> >>> freeze/thaw/restore handlers. Maybe something like the following:
> >>>
> >>> #ifdef CONFIG_X86
> >>>     .freeze = blkfront_freeze,
> >>>     .thaw = blkfront_restore,
> >>>     .restore = blkfront_restore
> >>> #endif
> >>>
> >>>
> >>> maybe Boris has a better suggestion on how to do it
> >>
> >> An alternative might be to still install pm notifier in
> >> drivers/xen/manage.c (I think as result of latest discussions we decided
> >> we won't need it) and return -ENOTSUPP for ARM for
> >> PM_HIBERNATION_PREPARE and friends. Would that work?
> >>
> > I think the question here is for registering driver specific freeze/thaw/restore
> > callbacks for x86 only. I have dropped the pm_notifier in the v3 still pending
> > testing. So I think just registering driver specific callbacks for x86 only is a
> > good option. What do you think?
>
>
> I suggested using the notifier under assumption that if it returns an
> error then that will prevent callbacks to be called because hibernation
> will be effectively disabled.

That's correct.

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v2 01/11] xen/manage: keep track of the on-going suspend mode
  2020-07-31 14:13                               ` Boris Ostrovsky
  2020-07-31 14:25                                 ` Rafael J. Wysocki
@ 2020-08-04 23:42                                 ` Anchal Agarwal
  2020-08-05 13:31                                   ` Boris Ostrovsky
  1 sibling, 1 reply; 44+ messages in thread
From: Anchal Agarwal @ 2020-08-04 23:42 UTC (permalink / raw)
  To: Boris Ostrovsky
  Cc: Stefano Stabellini, tglx, mingo, bp, hpa, x86, jgross, linux-pm,
	linux-mm, kamatam, konrad.wilk, roger.pau, axboe, davem, rjw,
	len.brown, pavel, peterz, eduval, sblbir, xen-devel, vkuznets,
	netdev, linux-kernel, dwmw, benh

On Fri, Jul 31, 2020 at 10:13:48AM -0400, Boris Ostrovsky wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> On 7/30/20 7:06 PM, Anchal Agarwal wrote:
> > On Mon, Jul 27, 2020 at 06:08:29PM -0400, Boris Ostrovsky wrote:
> >> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> >>
> >>
> >>
> >> On 7/24/20 7:01 PM, Stefano Stabellini wrote:
> >>> Yes, it does, thank you. I'd rather not introduce unknown regressions so
> >>> I would recommend to add an arch-specific check on registering
> >>> freeze/thaw/restore handlers. Maybe something like the following:
> >>>
> >>> #ifdef CONFIG_X86
> >>>     .freeze = blkfront_freeze,
> >>>     .thaw = blkfront_restore,
> >>>     .restore = blkfront_restore
> >>> #endif
> >>>
> >>>
> >>> maybe Boris has a better suggestion on how to do it
> >>
> >> An alternative might be to still install pm notifier in
> >> drivers/xen/manage.c (I think as result of latest discussions we decided
> >> we won't need it) and return -ENOTSUPP for ARM for
> >> PM_HIBERNATION_PREPARE and friends. Would that work?
> >>
> > I think the question here is for registering driver specific freeze/thaw/restore
> > callbacks for x86 only. I have dropped the pm_notifier in the v3 still pending
> > testing. So I think just registering driver specific callbacks for x86 only is a
> > good option. What do you think?
> 
> 
> I suggested using the notifier under assumption that if it returns an
> error then that will prevent callbacks to be called because hibernation
> will be effectively disabled. But I haven't looked at PM code so I don't
> know whether this is actually the case.
>
I think this could be done. PM_HIBERNATION_PREPARE could return -ENOTSUPP
for arm and pvh dom0 when the notifier call chain is invoked for this case
in hibernate(). This will then be an empty notifier just for checking two
usecases.
Also, for pvh dom0, the earlier code didn't register any notifier,
with this approach you are suggesting setup the notifier for hvm/pvh dom0 and
arm but fail during notifier call chain during PM_HIBERNATION_PREPARE ?

I think still getting rid of suspend mode that was earlier a part of this
notifier is a good idea as it seems redundant as you pointed out earlier. 
> 
> The advantage of doing it in the notifier is that instead of adding
> ifdefs to each driver you will be able to prevent callbacks from a
> single place. Plus you can use this do disable hibernation for PVH dom0
> as well.
> 
> 
> 
> -boris
> 
Anchal
> 
> 

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v2 01/11] xen/manage: keep track of the on-going suspend mode
  2020-08-04 23:42                                 ` Anchal Agarwal
@ 2020-08-05 13:31                                   ` Boris Ostrovsky
  2020-08-05 17:42                                     ` Anchal Agarwal
  0 siblings, 1 reply; 44+ messages in thread
From: Boris Ostrovsky @ 2020-08-05 13:31 UTC (permalink / raw)
  To: Anchal Agarwal
  Cc: Stefano Stabellini, tglx, mingo, bp, hpa, x86, jgross, linux-pm,
	linux-mm, kamatam, konrad.wilk, roger.pau, axboe, davem, rjw,
	len.brown, pavel, peterz, eduval, sblbir, xen-devel, vkuznets,
	netdev, linux-kernel, dwmw, benh

On 8/4/20 7:42 PM, Anchal Agarwal wrote:
>
> I think this could be done. PM_HIBERNATION_PREPARE could return -ENOTSUPP
> for arm and pvh dom0 when the notifier call chain is invoked for this case
> in hibernate(). This will then be an empty notifier just for checking two
> usecases.
> Also, for pvh dom0, the earlier code didn't register any notifier,
> with this approach you are suggesting setup the notifier for hvm/pvh dom0 and
> arm but fail during notifier call chain during PM_HIBERNATION_PREPARE ?


Right.


(Although the earlier code did register the notifier:
xen_setup_pm_notifier() would return an error for !xen_hvm_domain() and
PVH *is* an HVM domain, so registration would actually happen)


>
> I think still getting rid of suspend mode that was earlier a part of this
> notifier is a good idea as it seems redundant as you pointed out earlier. 


Yes.


-boris



^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v2 01/11] xen/manage: keep track of the on-going suspend mode
  2020-08-05 13:31                                   ` Boris Ostrovsky
@ 2020-08-05 17:42                                     ` Anchal Agarwal
  0 siblings, 0 replies; 44+ messages in thread
From: Anchal Agarwal @ 2020-08-05 17:42 UTC (permalink / raw)
  To: Boris Ostrovsky
  Cc: Stefano Stabellini, tglx, mingo, bp, hpa, x86, jgross, linux-pm,
	linux-mm, kamatam, konrad.wilk, roger.pau, axboe, davem, rjw,
	len.brown, pavel, peterz, eduval, sblbir, xen-devel, vkuznets,
	netdev, linux-kernel, dwmw, benh

On Wed, Aug 05, 2020 at 09:31:13AM -0400, Boris Ostrovsky wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> On 8/4/20 7:42 PM, Anchal Agarwal wrote:
> >
> > I think this could be done. PM_HIBERNATION_PREPARE could return -ENOTSUPP
> > for arm and pvh dom0 when the notifier call chain is invoked for this case
> > in hibernate(). This will then be an empty notifier just for checking two
> > usecases.
> > Also, for pvh dom0, the earlier code didn't register any notifier,
> > with this approach you are suggesting setup the notifier for hvm/pvh dom0 and
> > arm but fail during notifier call chain during PM_HIBERNATION_PREPARE ?
> 
> 
> Right.
> 
> 
> (Although the earlier code did register the notifier:
> xen_setup_pm_notifier() would return an error for !xen_hvm_domain() and
> PVH *is* an HVM domain, so registration would actually happen)
>
Yes you are right. My bad, what I meant with "earlier code" was whatever we
discussed w.r.t to removing the notifier all together, it won't be registered for
pvh dom0.
Anyways got the point :)
> 
> >
> > I think still getting rid of suspend mode that was earlier a part of this
> > notifier is a good idea as it seems redundant as you pointed out earlier.
> 
> 
> Yes.
> 
> 
> -boris
Thanks,
Anchal
> 
> 

^ permalink raw reply	[flat|nested] 44+ messages in thread

end of thread, back to index

Thread overview: 44+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-02 18:21 [PATCH v2 00/11] Fix PM hibernation in Xen guests Anchal Agarwal
2020-07-02 18:21 ` [PATCH v2 01/11] xen/manage: keep track of the on-going suspend mode Anchal Agarwal
2020-07-13 15:52   ` Boris Ostrovsky
2020-07-15 20:49     ` Anchal Agarwal
2020-07-15 21:18       ` Boris Ostrovsky
2020-07-17 19:10         ` Anchal Agarwal
2020-07-19  1:47           ` Boris Ostrovsky
2020-07-20  9:37             ` Roger Pau Monné
2020-07-21  0:17               ` Anchal Agarwal
2020-07-21  8:30                 ` Roger Pau Monné
2020-07-21 19:55                   ` Anchal Agarwal
2020-07-22  8:27                     ` Roger Pau Monné
2020-07-21  0:03             ` Anchal Agarwal
2020-07-21 21:48               ` Boris Ostrovsky
2020-07-22  0:18                 ` Stefano Stabellini
2020-07-22 18:02                   ` Anchal Agarwal
2020-07-22 22:45                     ` Boris Ostrovsky
2020-07-22 23:49                     ` Stefano Stabellini
2020-07-23 22:57                       ` Anchal Agarwal
2020-07-24 23:01                         ` Stefano Stabellini
2020-07-27 22:08                           ` Boris Ostrovsky
2020-07-30 23:06                             ` Anchal Agarwal
2020-07-31 14:13                               ` Boris Ostrovsky
2020-07-31 14:25                                 ` Rafael J. Wysocki
2020-08-04 23:42                                 ` Anchal Agarwal
2020-08-05 13:31                                   ` Boris Ostrovsky
2020-08-05 17:42                                     ` Anchal Agarwal
2020-07-02 18:21 ` [PATCH v2 03/11] x86/xen: Introduce new function to map HYPERVISOR_shared_info on Resume Anchal Agarwal
2020-07-02 18:22 ` [PATCH v2 04/11] x86/xen: add system core suspend and resume callbacks Anchal Agarwal
2020-07-05 17:22   ` kernel test robot
2020-07-22  9:08   ` Julien Grall
2020-07-02 18:22 ` [PATCH v2 05/11] genirq: Shutdown irq chips in suspend/resume during hibernation Anchal Agarwal
2020-07-02 18:22 ` [PATCH v2 06/11] xen-blkfront: add callbacks for PM suspend and hibernation Anchal Agarwal
2020-07-02 18:22 ` [PATCH v2 07/11] xen-netfront: " Anchal Agarwal
2020-07-02 18:22 ` [PATCH v2 08/11] x86/xen: save and restore steal clock during PM hibernation Anchal Agarwal
2020-07-02 18:23 ` [PATCH v2 09/11] xen: Introduce wrapper for save/restore sched clock offset Anchal Agarwal
2020-07-02 18:23 ` [PATCH v2 10/11] xen: Update sched clock offset to avoid system instability in hibernation Anchal Agarwal
2020-07-02 18:23 ` [PATCH v2 11/11] PM / hibernate: update the resume offset on SNAPSHOT_SET_SWAP_AREA Anchal Agarwal
2020-07-02 18:25 ` [PATCH v2 02/11] xenbus: add freeze/thaw/restore callbacks support Anchal Agarwal
2020-07-10 18:17 ` [PATCH v2 00/11] Fix PM hibernation in Xen guests Agarwal, Anchal
2020-07-13 19:43   ` Boris Ostrovsky
2020-07-15 19:49     ` Anchal Agarwal
2020-07-15 20:49       ` Boris Ostrovsky
2020-07-16 23:28         ` Anchal Agarwal

Linux-PM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pm/0 linux-pm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pm linux-pm/ https://lore.kernel.org/linux-pm \
		linux-pm@vger.kernel.org
	public-inbox-index linux-pm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git