[PATCH 2/2 v2] xen/acpi: upload PM state from init-domain to Xen

[PATCH 2/2 v2] xen/acpi: upload PM state from init-domain to Xen

[Ankur Arora]

This was broken in commit cd979883b9ede90643e019f33cb317933eb867b4.
do_suspend (from xen/manage.c) and thus xen_resume_notifier never get
called on the initial-domain at resume (it is if running as guest.)

The rationale for the breaking change was that upload_pm_data()
potentially does blocking work in syscore_resume(). This patch
addresses the original issue by scheduling upload_pm_data() to
execute in workqueue context.

Changes in v2:
 - rebased to 4.11.0-rc2
 - addressed comments from Boris Ostrovsky

Cc: Stanislaw Gruszka <sgruszka@redhat.com>
Cc: stable@vger.kernel.org
Based-on-patch-by: Konrad Wilk <konrad.wilk@oracle.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
---
 drivers/xen/xen-acpi-processor.c | 34 ++++++++++++++++++++++++++--------
 1 file changed, 26 insertions(+), 8 deletions(-)

diff --git a/drivers/xen/xen-acpi-processor.c b/drivers/xen/xen-acpi-processor.c
index fac0d7b..23e391d 100644
--- a/drivers/xen/xen-acpi-processor.c
+++ b/drivers/xen/xen-acpi-processor.c
@@ -27,10 +27,10 @@
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/types.h>
+#include <linux/syscore_ops.h>
 #include <linux/acpi.h>
 #include <acpi/processor.h>
 #include <xen/xen.h>
-#include <xen/xen-ops.h>
 #include <xen/interface/platform.h>
 #include <asm/xen/hypercall.h>
 
@@ -466,15 +466,33 @@ static int xen_upload_processor_pm_data(void)
 	return rc;
 }
 
-static int xen_acpi_processor_resume(struct notifier_block *nb,
-				     unsigned long action, void *data)
+static void xen_acpi_processor_resume_worker(struct work_struct *dummy)
 {
+	int rc;
+
 	bitmap_zero(acpi_ids_done, nr_acpi_bits);
-	return xen_upload_processor_pm_data();
+
+	rc = xen_upload_processor_pm_data();
+	if (rc != 0)
+		pr_info("ACPI data upload failed, error = %d\n", rc);
+}
+
+static void xen_acpi_processor_resume(void)
+{
+	static DECLARE_WORK(wq, xen_acpi_processor_resume_worker);
+
+	/*
+	 * xen_upload_processor_pm_data() calls non-atomic code.
+	 * However, the context for xen_acpi_processor_resume is syscore
+	 * with only the boot CPU online and in an atomic context.
+	 *
+	 * So defer the upload for some point safer.
+	 */
+	schedule_work(&wq);
 }
 
-struct notifier_block xen_acpi_processor_resume_nb = {
-	.notifier_call = xen_acpi_processor_resume,
+static struct syscore_ops xap_syscore_ops = {
+	.resume	= xen_acpi_processor_resume,
 };
 
 static int __init xen_acpi_processor_init(void)
@@ -527,7 +545,7 @@ static int __init xen_acpi_processor_init(void)
 	if (rc)
 		goto err_unregister;
 
-	xen_resume_notifier_register(&xen_acpi_processor_resume_nb);
+	register_syscore_ops(&xap_syscore_ops);
 
 	return 0;
 err_unregister:
@@ -544,7 +562,7 @@ static void __exit xen_acpi_processor_exit(void)
 {
 	int i;
 
-	xen_resume_notifier_unregister(&xen_acpi_processor_resume_nb);
+	unregister_syscore_ops(&xap_syscore_ops);
 	kfree(acpi_ids_done);
 	kfree(acpi_id_present);
 	kfree(acpi_id_cst_present);
-- 
2.7.4

Re: [PATCH 2/2 v2] xen/acpi: upload PM state from init-domain to Xen

[Stanislaw Gruszka]

On Tue, Mar 21, 2017 at 03:43:38PM -0700, Ankur Arora wrote:
> This was broken in commit cd979883b9ede90643e019f33cb317933eb867b4.
> do_suspend (from xen/manage.c) and thus xen_resume_notifier never get
> called on the initial-domain at resume (it is if running as guest.)
> 
> The rationale for the breaking change was that upload_pm_data()
> potentially does blocking work in syscore_resume(). This patch
> addresses the original issue by scheduling upload_pm_data() to
> execute in workqueue context.

It is ok to do upload_pm_data() with delay i.e. after some other
resume actions are done and possibly xen-acpi-processor is in
running state ?

Stanislaw

Re: [PATCH 2/2 v2] xen/acpi: upload PM state from init-domain to Xen

[Ankur Arora]

On 2017-03-22 02:05 AM, Stanislaw Gruszka wrote:
> On Tue, Mar 21, 2017 at 03:43:38PM -0700, Ankur Arora wrote:
>> This was broken in commit cd979883b9ede90643e019f33cb317933eb867b4.
>> do_suspend (from xen/manage.c) and thus xen_resume_notifier never get
>> called on the initial-domain at resume (it is if running as guest.)
>>
>> The rationale for the breaking change was that upload_pm_data()
>> potentially does blocking work in syscore_resume(). This patch
>> addresses the original issue by scheduling upload_pm_data() to
>> execute in workqueue context.
>
> It is ok to do upload_pm_data() with delay i.e. after some other
> resume actions are done and possibly xen-acpi-processor is in
> running state ?
The state uploaded is ACPI P and C state from struct acpi_processor
which AFAICS is stable once inited so a delay would not lead to
invalid state.
The only concern would be the ACPI pCPU hotplug logic in
acpi_processor_add() which could add a new entry in
per_cpu(processors) but that also looks okay because either we
get a NULL or we get a pointer to an inited structure.

As for the hypervisor -- that falls back to more limited state after
resume (because some of this state is thrown away at suspend) and so
uses that until it gets the uploaded PM state from the initial-domain.

Ankur

>
> Stanislaw
>

Re: [PATCH 2/2 v2] xen/acpi: upload PM state from init-domain to Xen

[Stanislaw Gruszka]

On Wed, Mar 22, 2017 at 10:56:02AM -0700, Ankur Arora wrote:
> >It is ok to do upload_pm_data() with delay i.e. after some other
> >resume actions are done and possibly xen-acpi-processor is in
> >running state ?
> The state uploaded is ACPI P and C state from struct acpi_processor
> which AFAICS is stable once inited so a delay would not lead to
> invalid state.
> The only concern would be the ACPI pCPU hotplug logic in
> acpi_processor_add() which could add a new entry in
> per_cpu(processors) but that also looks okay because either we
> get a NULL or we get a pointer to an inited structure.
> 
> As for the hypervisor -- that falls back to more limited state after
> resume (because some of this state is thrown away at suspend) and so
> uses that until it gets the uploaded PM state from the initial-domain.

Patch looks good to me then.

Reviewed-by: Stanislaw Gruszka <sgruszka@redhat.com>

Re: [Xen-devel] [PATCH 0/2 v2] xen/acpi: upload PM state from init-domain to Xen

[Boris Ostrovsky]

On 03/21/2017 06:43 PM, Ankur Arora wrote:
> This patch series re-enables the upload of PM data from initial-domain
> to Xen. This was broken in commit cd979883b9ede90643e019f33cb317933eb867b4.
>
> The upload now happens post-resume in workqueue context. From the
> POV of Xen, the PM upload might be delayed a little but should be
> fine -- Xen falls-back on more limited P and C states.
>
> Tested C-state upload via mwait_idle=0.
>
> Changes in v2:
>  - rebased to 4.11.0-rc2
>  - addressed comments from Boris Ostrovsky
>
> Ankur Arora (2):
>   xen/acpi: Replace hard coded "ACPI0007"
>   xen/acpi: upload PM state from init-domain to Xen
>
>  drivers/xen/xen-acpi-processor.c | 36 +++++++++++++++++++++++++++---------
>  1 file changed, 27 insertions(+), 9 deletions(-)
>

Applied to for-linus-4.11b, with some modifications to commit message in
patch 2.

Next time please copy maintainers (Juergen and me), otherwise there is a
chance that we may miss this. I am also copying Konrad since since his
R-b was not given on the public list.

-boris

Re: [Xen-devel] [PATCH 0/2 v2] xen/acpi: upload PM state from init-domain to Xen

[Konrad Rzeszutek Wilk]

On Thu, Mar 23, 2017 at 12:45:51PM -0400, Boris Ostrovsky wrote:
> On 03/21/2017 06:43 PM, Ankur Arora wrote:
> > This patch series re-enables the upload of PM data from initial-domain
> > to Xen. This was broken in commit cd979883b9ede90643e019f33cb317933eb867b4.
> >
> > The upload now happens post-resume in workqueue context. From the
> > POV of Xen, the PM upload might be delayed a little but should be
> > fine -- Xen falls-back on more limited P and C states.
> >
> > Tested C-state upload via mwait_idle=0.
> >
> > Changes in v2:
> >  - rebased to 4.11.0-rc2
> >  - addressed comments from Boris Ostrovsky
> >
> > Ankur Arora (2):
> >   xen/acpi: Replace hard coded "ACPI0007"
> >   xen/acpi: upload PM state from init-domain to Xen
> >
> >  drivers/xen/xen-acpi-processor.c | 36 +++++++++++++++++++++++++++---------
> >  1 file changed, 27 insertions(+), 9 deletions(-)
> >
> 
> Applied to for-linus-4.11b, with some modifications to commit message in
> patch 2.
> 
> Next time please copy maintainers (Juergen and me), otherwise there is a
> chance that we may miss this. I am also copying Konrad since since his
> R-b was not given on the public list.

Whoops.

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
here 
> 
> -boris

[PATCH 2/2 v2] xen/acpi: upload PM state from init-domain to Xen

[Ankur Arora]

This was broken in commit cd979883b9ede90643e019f33cb317933eb867b4.
do_suspend (from xen/manage.c) and thus xen_resume_notifier never get
called on the initial-domain at resume (it is if running as guest.)

The rationale for the breaking change was that upload_pm_data()
potentially does blocking work in syscore_resume(). This patch
addresses the original issue by scheduling upload_pm_data() to
execute in workqueue context.

Changes in v2:
 - rebased to 4.11.0-rc2
 - addressed comments from Boris Ostrovsky

Cc: Stanislaw Gruszka <sgruszka@redhat.com>
Cc: stable@vger.kernel.org
Based-on-patch-by: Konrad Wilk <konrad.wilk@oracle.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
---
 drivers/xen/xen-acpi-processor.c | 34 ++++++++++++++++++++++++++--------
 1 file changed, 26 insertions(+), 8 deletions(-)

diff --git a/drivers/xen/xen-acpi-processor.c b/drivers/xen/xen-acpi-processor.c
index fac0d7b..23e391d 100644
--- a/drivers/xen/xen-acpi-processor.c
+++ b/drivers/xen/xen-acpi-processor.c
@@ -27,10 +27,10 @@
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/types.h>
+#include <linux/syscore_ops.h>
 #include <linux/acpi.h>
 #include <acpi/processor.h>
 #include <xen/xen.h>
-#include <xen/xen-ops.h>
 #include <xen/interface/platform.h>
 #include <asm/xen/hypercall.h>
 
@@ -466,15 +466,33 @@ static int xen_upload_processor_pm_data(void)
 	return rc;
 }
 
-static int xen_acpi_processor_resume(struct notifier_block *nb,
-				     unsigned long action, void *data)
+static void xen_acpi_processor_resume_worker(struct work_struct *dummy)
 {
+	int rc;
+
 	bitmap_zero(acpi_ids_done, nr_acpi_bits);
-	return xen_upload_processor_pm_data();
+
+	rc = xen_upload_processor_pm_data();
+	if (rc != 0)
+		pr_info("ACPI data upload failed, error = %d\n", rc);
+}
+
+static void xen_acpi_processor_resume(void)
+{
+	static DECLARE_WORK(wq, xen_acpi_processor_resume_worker);
+
+	/*
+	 * xen_upload_processor_pm_data() calls non-atomic code.
+	 * However, the context for xen_acpi_processor_resume is syscore
+	 * with only the boot CPU online and in an atomic context.
+	 *
+	 * So defer the upload for some point safer.
+	 */
+	schedule_work(&wq);
 }
 
-struct notifier_block xen_acpi_processor_resume_nb = {
-	.notifier_call = xen_acpi_processor_resume,
+static struct syscore_ops xap_syscore_ops = {
+	.resume	= xen_acpi_processor_resume,
 };
 
 static int __init xen_acpi_processor_init(void)
@@ -527,7 +545,7 @@ static int __init xen_acpi_processor_init(void)
 	if (rc)
 		goto err_unregister;
 
-	xen_resume_notifier_register(&xen_acpi_processor_resume_nb);
+	register_syscore_ops(&xap_syscore_ops);
 
 	return 0;
 err_unregister:
@@ -544,7 +562,7 @@ static void __exit xen_acpi_processor_exit(void)
 {
 	int i;
 
-	xen_resume_notifier_unregister(&xen_acpi_processor_resume_nb);
+	unregister_syscore_ops(&xap_syscore_ops);
 	kfree(acpi_ids_done);
 	kfree(acpi_id_present);
 	kfree(acpi_id_cst_present);
-- 
2.7.4