lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Wed, 15 Jul 2020 20:49:43 +0000 From: Anchal Agarwal <anchalag@...zon.com> To: Boris Ostrovsky <boris.ostrovsky@...cle.com> CC: <tglx@...utronix.de>, <mingo@...hat.com>, <bp@...en8.de>, <hpa@...or.com>, <x86@...nel.org>, <jgross@...e.com>, <linux-pm@...r.kernel.org>, <linux-mm@...ck.org>, <kamatam@...zon.com>, <sstabellini@...nel.org>, <konrad.wilk@...cle.com>, <roger.pau@...rix.com>, <axboe@...nel.dk>, <davem@...emloft.net>, <rjw@...ysocki.net>, <len.brown@...el.com>, <pavel@....cz>, <peterz@...radead.org>, <eduval@...zon.com>, <sblbir@...zon.com>, <xen-devel@...ts.xenproject.org>, <vkuznets@...hat.com>, <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>, <dwmw@...zon.co.uk>, <benh@...nel.crashing.org>, <anchalag@...zon.com> Subject: Re: [PATCH v2 01/11] xen/manage: keep track of the on-going suspend mode 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@...zon.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@...zon.com> > > Signed-off-by: Munehisa Kamata <kamatam@...zon.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 > > >
Powered by blists - more mailing lists