Messages in this thread | | | Subject | Re: [PATCH v3 01/11] xen/manage: keep track of the on-going suspend mode | From | boris.ostrovsky@oracle ... | Date | Sun, 13 Sep 2020 11:43:30 -0400 |
| |
On 8/21/20 6:25 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 API to keep track of > SHUTDOWN_SUSPEND state. > Introduce a simple function that keeps track of on-going suspend mode > so that PM hibernation 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.
lock_system_sleep() is not taking this mutex.
> > Though, accquirng 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.
Is it Xen's shutdown or suspend code that needs to address this? (Or I may not understand what the problem is that you are describing)
> > + > +static int xen_pm_notifier(struct notifier_block *notifier, > + unsigned long pm_event, void *unused) > +{ > + int ret; > + > + switch (pm_event) { > + case PM_SUSPEND_PREPARE: > + case PM_HIBERNATION_PREPARE: > + /* Guest hibernation is not supported for aarch64 currently*/ > + if (IS_ENABLED(CONFIG_ARM64)) { > + ret = NOTIFY_BAD; > + break; > + }
Indentation.
> + case PM_RESTORE_PREPARE: > + case PM_POST_SUSPEND: > + case PM_POST_HIBERNATION: > + case PM_POST_RESTORE: > + default: > + ret = NOTIFY_OK; > + } > + return ret; > +};
This whole routine now is
if (IS_ENABLED(CONFIG_ARM64)) return NOTIFY_BAD;
return NOTIFY_OK;
isn't it?
> + > +static struct notifier_block xen_pm_notifier_block = { > + .notifier_call = xen_pm_notifier > +}; > + > +static int xen_setup_pm_notifier(void) > +{ > + if (!xen_hvm_domain() || xen_initial_domain()) > + return -ENODEV;
I don't think this works anymore.
In the past your notifier would set suspend_mode (or something) but now it really doesn't do anything except reports an error in some (ARM) cases.
So I think you should move this check into the notifier.
(And BTW I still think PM_SUSPEND_PREPARE should return an error too. The fact that we are using "suspend" in xen routine names is irrelevant)
-boris
> + return register_pm_notifier(&xen_pm_notifier_block); > +} > +
| |