Currently the LPS0 prepare_late callback is aimed to run as the very
last thing before entering the S2Idle state from LPS0 perspective,
nevertheless between this call and the system actually entering the
S2Idle state there are several places where the suspension process could
be canceled.
In order to notify VMM about guest entering suspend, extend the S2Idle
ops by new notify callback, which will be really invoked as a very last
thing before guest actually enters S2Idle state.
Additionally extend the acpi_s2idle_dev_ops by notify() callback so
any driver can hook into it and allow to implement its own notification.
Taking advantage of e.g. existing acpi_s2idle_dev_ops's prepare/restore
hooks is not an option since it will not allow to prevent race
conditions:
- VM0 enters s2idle
- host notes about VM0 is in s2idle
- host continues with system suspension but in the meantime VM0 exits
s2idle and sends notification but it is already too late (VM could not
even send notification on time).
Introducing notify() as a very last step before the system enters S2Idle
together with an assumption that the VMM has control over guest
resumption allows preventing mentioned races.
Signed-off-by: Grzegorz Jaszczyk <[email protected]>
---
drivers/acpi/x86/s2idle.c | 11 +++++++++++
include/linux/acpi.h | 1 +
include/linux/suspend.h | 1 +
kernel/power/suspend.c | 4 ++++
4 files changed, 17 insertions(+)
diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
index 2963229062f8..d5aff194c736 100644
--- a/drivers/acpi/x86/s2idle.c
+++ b/drivers/acpi/x86/s2idle.c
@@ -520,10 +520,21 @@ void acpi_s2idle_restore_early(void)
lps0_dsm_func_mask, lps0_dsm_guid);
}
+static void acpi_s2idle_notify(void)
+{
+ struct acpi_s2idle_dev_ops *handler;
+
+ list_for_each_entry(handler, &lps0_s2idle_devops_head, list_node) {
+ if (handler->notify)
+ handler->notify();
+ }
+}
+
static const struct platform_s2idle_ops acpi_s2idle_ops_lps0 = {
.begin = acpi_s2idle_begin,
.prepare = acpi_s2idle_prepare,
.prepare_late = acpi_s2idle_prepare_late,
+ .notify = acpi_s2idle_notify,
.wake = acpi_s2idle_wake,
.restore_early = acpi_s2idle_restore_early,
.restore = acpi_s2idle_restore,
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 4f82a5bc6d98..b32c4baed99b 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1068,6 +1068,7 @@ struct acpi_s2idle_dev_ops {
struct list_head list_node;
void (*prepare)(void);
void (*restore)(void);
+ void (*notify)(void);
};
int acpi_register_lps0_dev(struct acpi_s2idle_dev_ops *arg);
void acpi_unregister_lps0_dev(struct acpi_s2idle_dev_ops *arg);
diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 70f2921e2e70..16ef7f9d9a03 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -191,6 +191,7 @@ struct platform_s2idle_ops {
int (*begin)(void);
int (*prepare)(void);
int (*prepare_late)(void);
+ void (*notify)(void);
bool (*wake)(void);
void (*restore_early)(void);
void (*restore)(void);
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 827075944d28..6ba211b94ed1 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -100,6 +100,10 @@ static void s2idle_enter(void)
/* Push all the CPUs into the idle loop. */
wake_up_all_idle_cpus();
+
+ if (s2idle_ops && s2idle_ops->notify)
+ s2idle_ops->notify();
+
/* Make the current CPU wait so it can enter the idle loop too. */
swait_event_exclusive(s2idle_wait_head,
s2idle_state == S2IDLE_STATE_WAKE);
--
2.37.0.rc0.161.g10f37bed90-goog
On Thu, Jul 7, 2022 at 2:56 PM Grzegorz Jaszczyk <[email protected]> wrote:
>
> Currently the LPS0 prepare_late callback is aimed to run as the very
> last thing before entering the S2Idle state from LPS0 perspective,
> nevertheless between this call and the system actually entering the
> S2Idle state there are several places where the suspension process could
> be canceled.
And why is this a problem?
The cancellation will occur only if there is a wakeup signal that
would otherwise cause one of the CPUs to exit the idle state. Such a
wakeup signal can appear after calling the new notifier as well, so
why does it make a difference?
> In order to notify VMM about guest entering suspend, extend the S2Idle
> ops by new notify callback, which will be really invoked as a very last
> thing before guest actually enters S2Idle state.
It is not guaranteed that "suspend" (defined as all CPUs entering idle
states) will be actually entered even after this "last step".
> Additionally extend the acpi_s2idle_dev_ops by notify() callback so
> any driver can hook into it and allow to implement its own notification.
>
> Taking advantage of e.g. existing acpi_s2idle_dev_ops's prepare/restore
> hooks is not an option since it will not allow to prevent race
> conditions:
> - VM0 enters s2idle
> - host notes about VM0 is in s2idle
> - host continues with system suspension but in the meantime VM0 exits
> s2idle and sends notification but it is already too late (VM could not
> even send notification on time).
Too late for what?
> Introducing notify() as a very last step before the system enters S2Idle
> together with an assumption that the VMM has control over guest
> resumption allows preventing mentioned races.
How does it do that?
It looks like you want suspend-to-idle to behave like S3 and it won't.
> Signed-off-by: Grzegorz Jaszczyk <[email protected]>
> ---
> drivers/acpi/x86/s2idle.c | 11 +++++++++++
> include/linux/acpi.h | 1 +
> include/linux/suspend.h | 1 +
> kernel/power/suspend.c | 4 ++++
> 4 files changed, 17 insertions(+)
>
> diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
> index 2963229062f8..d5aff194c736 100644
> --- a/drivers/acpi/x86/s2idle.c
> +++ b/drivers/acpi/x86/s2idle.c
> @@ -520,10 +520,21 @@ void acpi_s2idle_restore_early(void)
> lps0_dsm_func_mask, lps0_dsm_guid);
> }
>
> +static void acpi_s2idle_notify(void)
> +{
> + struct acpi_s2idle_dev_ops *handler;
> +
> + list_for_each_entry(handler, &lps0_s2idle_devops_head, list_node) {
> + if (handler->notify)
> + handler->notify();
> + }
> +}
> +
> static const struct platform_s2idle_ops acpi_s2idle_ops_lps0 = {
> .begin = acpi_s2idle_begin,
> .prepare = acpi_s2idle_prepare,
> .prepare_late = acpi_s2idle_prepare_late,
> + .notify = acpi_s2idle_notify,
> .wake = acpi_s2idle_wake,
> .restore_early = acpi_s2idle_restore_early,
> .restore = acpi_s2idle_restore,
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 4f82a5bc6d98..b32c4baed99b 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -1068,6 +1068,7 @@ struct acpi_s2idle_dev_ops {
> struct list_head list_node;
> void (*prepare)(void);
> void (*restore)(void);
> + void (*notify)(void);
> };
> int acpi_register_lps0_dev(struct acpi_s2idle_dev_ops *arg);
> void acpi_unregister_lps0_dev(struct acpi_s2idle_dev_ops *arg);
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index 70f2921e2e70..16ef7f9d9a03 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -191,6 +191,7 @@ struct platform_s2idle_ops {
> int (*begin)(void);
> int (*prepare)(void);
> int (*prepare_late)(void);
> + void (*notify)(void);
> bool (*wake)(void);
> void (*restore_early)(void);
> void (*restore)(void);
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index 827075944d28..6ba211b94ed1 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -100,6 +100,10 @@ static void s2idle_enter(void)
>
> /* Push all the CPUs into the idle loop. */
> wake_up_all_idle_cpus();
> +
> + if (s2idle_ops && s2idle_ops->notify)
> + s2idle_ops->notify();
> +
> /* Make the current CPU wait so it can enter the idle loop too. */
> swait_event_exclusive(s2idle_wait_head,
> s2idle_state == S2IDLE_STATE_WAKE);
> --
> 2.37.0.rc0.161.g10f37bed90-goog
>
wt., 19 lip 2022 o 20:09 Rafael J. Wysocki <[email protected]> napisał(a):
>
> On Thu, Jul 7, 2022 at 2:56 PM Grzegorz Jaszczyk <[email protected]> wrote:
> >
> > Currently the LPS0 prepare_late callback is aimed to run as the very
> > last thing before entering the S2Idle state from LPS0 perspective,
> > nevertheless between this call and the system actually entering the
> > S2Idle state there are several places where the suspension process could
> > be canceled.
>
> And why is this a problem?
>
> The cancellation will occur only if there is a wakeup signal that
> would otherwise cause one of the CPUs to exit the idle state. Such a
> wakeup signal can appear after calling the new notifier as well, so
> why does it make a difference?
It could also occur due to suspend_test. Additionally with new
notifier we could get notification when the system wakes up from
s2idle_loop and immediately goes to sleep again (due to e.g.
acpi_s2idle_wake condition not being met) - in this case relying on
prepare_late callback is not possible since it is not called in this
path.
>
> > In order to notify VMM about guest entering suspend, extend the S2Idle
> > ops by new notify callback, which will be really invoked as a very last
> > thing before guest actually enters S2Idle state.
>
> It is not guaranteed that "suspend" (defined as all CPUs entering idle
> states) will be actually entered even after this "last step".
Since this whole patchset is aimed at notifying the host about a guest
entering s2idle state, reaching this step can be considered as a
suspend "entry point" for VM IMO. It is because we are talking about
the vCPU not the real CPU. Therefore it seems to me, that even if some
other vCPUs could still get some wakeup signal they will not be able
to kick (through s2idle_wake->swake_up_one(&s2idle_wait_head);) the
original vCPU which entered s2idle_loop, triggered the new notifier
and is halted due to handling vCPU exit (and was about to trigger
swait_event_exclusive). So it will prevent the VM's resume process
from being started.
>
> > Additionally extend the acpi_s2idle_dev_ops by notify() callback so
> > any driver can hook into it and allow to implement its own notification.
> >
> > Taking advantage of e.g. existing acpi_s2idle_dev_ops's prepare/restore
> > hooks is not an option since it will not allow to prevent race
> > conditions:
> > - VM0 enters s2idle
> > - host notes about VM0 is in s2idle
> > - host continues with system suspension but in the meantime VM0 exits
> > s2idle and sends notification but it is already too late (VM could not
> > even send notification on time).
>
> Too late for what?
Too late to cancel the host suspend process, which thinks that the VM
is in s2idle state while it isn't.
>
> > Introducing notify() as a very last step before the system enters S2Idle
> > together with an assumption that the VMM has control over guest
> > resumption allows preventing mentioned races.
>
> How does it do that?
At the moment when VM triggers this new notifier we trap on MMIO
access and the VMM handles vCPU exit (so the vCPU is "halted").
Therefore the VMM could control when it finishes such handling and
releases the vCPU again.
Maybe adding some more context will be helpful. This patchset was
aimed for two different scenarios actually:
1) Host is about to enter the suspend state and needs first to suspend
VM with all pass-through devices. In this case the host waits for
s2idle notification from the guest and when it receives it, it
continues with its own suspend process.
2) Guest could be a "privileged" one (in terms of VMM) and when the
guest enters s2idle state it notifies the host, which in turn triggers
the suspend process of the host.
>
> It looks like you want suspend-to-idle to behave like S3 and it won't.
In a way, yes, we compensate for the lack of something like PM1_CNT to
trap on for detecting that the guest is suspending.
We could instead force the guest to use S3 but IMO it is undesirable,
since it generally does make a difference which suspend mode is used
in the guest, s2idle or S3, e.g some drivers check which suspend type
is used and based on that behaves differently during suspend. One of
the example is:
https://elixir.bootlin.com/linux/v5.18.12/source/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c#L2323
https://elixir.bootlin.com/linux/v5.18.12/source/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c#L1069
https://elixir.bootlin.com/linux/v5.18.12/source/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c#L583
Thank you,
Grzegorz
>
> > Signed-off-by: Grzegorz Jaszczyk <[email protected]>
> > ---
> > drivers/acpi/x86/s2idle.c | 11 +++++++++++
> > include/linux/acpi.h | 1 +
> > include/linux/suspend.h | 1 +
> > kernel/power/suspend.c | 4 ++++
> > 4 files changed, 17 insertions(+)
> >
> > diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
> > index 2963229062f8..d5aff194c736 100644
> > --- a/drivers/acpi/x86/s2idle.c
> > +++ b/drivers/acpi/x86/s2idle.c
> > @@ -520,10 +520,21 @@ void acpi_s2idle_restore_early(void)
> > lps0_dsm_func_mask, lps0_dsm_guid);
> > }
> >
> > +static void acpi_s2idle_notify(void)
> > +{
> > + struct acpi_s2idle_dev_ops *handler;
> > +
> > + list_for_each_entry(handler, &lps0_s2idle_devops_head, list_node) {
> > + if (handler->notify)
> > + handler->notify();
> > + }
> > +}
> > +
> > static const struct platform_s2idle_ops acpi_s2idle_ops_lps0 = {
> > .begin = acpi_s2idle_begin,
> > .prepare = acpi_s2idle_prepare,
> > .prepare_late = acpi_s2idle_prepare_late,
> > + .notify = acpi_s2idle_notify,
> > .wake = acpi_s2idle_wake,
> > .restore_early = acpi_s2idle_restore_early,
> > .restore = acpi_s2idle_restore,
> > diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> > index 4f82a5bc6d98..b32c4baed99b 100644
> > --- a/include/linux/acpi.h
> > +++ b/include/linux/acpi.h
> > @@ -1068,6 +1068,7 @@ struct acpi_s2idle_dev_ops {
> > struct list_head list_node;
> > void (*prepare)(void);
> > void (*restore)(void);
> > + void (*notify)(void);
> > };
> > int acpi_register_lps0_dev(struct acpi_s2idle_dev_ops *arg);
> > void acpi_unregister_lps0_dev(struct acpi_s2idle_dev_ops *arg);
> > diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> > index 70f2921e2e70..16ef7f9d9a03 100644
> > --- a/include/linux/suspend.h
> > +++ b/include/linux/suspend.h
> > @@ -191,6 +191,7 @@ struct platform_s2idle_ops {
> > int (*begin)(void);
> > int (*prepare)(void);
> > int (*prepare_late)(void);
> > + void (*notify)(void);
> > bool (*wake)(void);
> > void (*restore_early)(void);
> > void (*restore)(void);
> > diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> > index 827075944d28..6ba211b94ed1 100644
> > --- a/kernel/power/suspend.c
> > +++ b/kernel/power/suspend.c
> > @@ -100,6 +100,10 @@ static void s2idle_enter(void)
> >
> > /* Push all the CPUs into the idle loop. */
> > wake_up_all_idle_cpus();
> > +
> > + if (s2idle_ops && s2idle_ops->notify)
> > + s2idle_ops->notify();
> > +
> > /* Make the current CPU wait so it can enter the idle loop too. */
> > swait_event_exclusive(s2idle_wait_head,
> > s2idle_state == S2IDLE_STATE_WAKE);
> > --
> > 2.37.0.rc0.161.g10f37bed90-goog
> >
>> It looks like you want suspend-to-idle to behave like S3 and it won't.
>
> In a way, yes, we compensate for the lack of something like PM1_CNT to
> trap on for detecting that the guest is suspending.
> We could instead force the guest to use S3 but IMO it is undesirable,
> since it generally does make a difference which suspend mode is used
> in the guest, s2idle or S3, e.g some drivers check which suspend type
> is used and based on that behaves differently during suspend. One of
> the example is:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv5.18.12%2Fsource%2Fdrivers%2Fgpu%2Fdrm%2Famd%2Famdgpu%2Famdgpu_drv.c%23L2323&data=05%7C01%7Cmario.limonciello%40amd.com%7C7bdd972291324d03847e08da6a51ff4f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637939197694682503%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=5M1sn3iRybQzSFi3ojQ4YTJuW41DlgJNl5sxbWEvLBQ%3D&reserved=0
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv5.18.12%2Fsource%2Fdrivers%2Fgpu%2Fdrm%2Famd%2Famdgpu%2Famdgpu_acpi.c%23L1069&data=05%7C01%7Cmario.limonciello%40amd.com%7C7bdd972291324d03847e08da6a51ff4f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637939197694682503%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=fIrLmZAgpIRPYO4to4uYUoBSEWXmz1lr%2BTnR14kAfvM%3D&reserved=0
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv5.18.12%2Fsource%2Fdrivers%2Fgpu%2Fdrm%2Famd%2Famdgpu%2Famdgpu_gfx.c%23L583&data=05%7C01%7Cmario.limonciello%40amd.com%7C7bdd972291324d03847e08da6a51ff4f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637939197694682503%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=SNsbmpV4HrgA%2Bkff4JzRodNDzKvwM5tnkGDvrKO44dc%3D&reserved=0
>
Actually I recently was suggesting a change to add this detection to
another driver to set a policy and Rafael pushed back. He's actively
removing it from other places in the kernel.
For amdgpu stuff you pointed above, are you wanting to pass through the
PCIe GPU device to a guest and then suspend that guest? Or is this just
illustrative?
For a dGPU I would expect it works, but I don't think passing an APU's
GPU PCIe endpoint would functionally work (there were bugs reported on
this I recall).
That code path you point out only has special handling for APU when
headed to S0ix and that's because the GPU driver happens to be where the
control point is for some common silicon functions. If the bug I
mentioned about PCIe passthrough of the APU GPU endpoint to the guest is
fixed and the guest needs to do s0ix when the host doesn't we're going
to have other breakage to worry about because of that common silicon
functionality I mentioned.
śr., 20 lip 2022 o 17:22 Limonciello, Mario
<[email protected]> napisał(a):
>
> >> It looks like you want suspend-to-idle to behave like S3 and it won't.
> >
> > In a way, yes, we compensate for the lack of something like PM1_CNT to
> > trap on for detecting that the guest is suspending.
> > We could instead force the guest to use S3 but IMO it is undesirable,
> > since it generally does make a difference which suspend mode is used
> > in the guest, s2idle or S3, e.g some drivers check which suspend type
> > is used and based on that behaves differently during suspend. One of
> > the example is:
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv5.18.12%2Fsource%2Fdrivers%2Fgpu%2Fdrm%2Famd%2Famdgpu%2Famdgpu_drv.c%23L2323&data=05%7C01%7Cmario.limonciello%40amd.com%7C7bdd972291324d03847e08da6a51ff4f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637939197694682503%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=5M1sn3iRybQzSFi3ojQ4YTJuW41DlgJNl5sxbWEvLBQ%3D&reserved=0
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv5.18.12%2Fsource%2Fdrivers%2Fgpu%2Fdrm%2Famd%2Famdgpu%2Famdgpu_acpi.c%23L1069&data=05%7C01%7Cmario.limonciello%40amd.com%7C7bdd972291324d03847e08da6a51ff4f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637939197694682503%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=fIrLmZAgpIRPYO4to4uYUoBSEWXmz1lr%2BTnR14kAfvM%3D&reserved=0
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv5.18.12%2Fsource%2Fdrivers%2Fgpu%2Fdrm%2Famd%2Famdgpu%2Famdgpu_gfx.c%23L583&data=05%7C01%7Cmario.limonciello%40amd.com%7C7bdd972291324d03847e08da6a51ff4f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637939197694682503%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=SNsbmpV4HrgA%2Bkff4JzRodNDzKvwM5tnkGDvrKO44dc%3D&reserved=0
> >
>
> Actually I recently was suggesting a change to add this detection to
> another driver to set a policy and Rafael pushed back. He's actively
> removing it from other places in the kernel.
>
> For amdgpu stuff you pointed above, are you wanting to pass through the
> PCIe GPU device to a guest and then suspend that guest? Or is this just
> illustrative?
Just illustrative. I am not focused on amdgpu stuff right now.
Thank you,
Grzegorz
>
> For a dGPU I would expect it works, but I don't think passing an APU's
> GPU PCIe endpoint would functionally work (there were bugs reported on
> this I recall).
>
> That code path you point out only has special handling for APU when
> headed to S0ix and that's because the GPU driver happens to be where the
> control point is for some common silicon functions. If the bug I
> mentioned about PCIe passthrough of the APU GPU endpoint to the guest is
> fixed and the guest needs to do s0ix when the host doesn't we're going
> to have other breakage to worry about because of that common silicon
> functionality I mentioned.
Hi Rafael,
Could you please kindly comment on the above?
Thank you in advance,
Grzegorz
śr., 20 lip 2022 o 15:15 Grzegorz Jaszczyk <[email protected]> napisał(a):
>
> wt., 19 lip 2022 o 20:09 Rafael J. Wysocki <[email protected]> napisał(a):
> >
> > On Thu, Jul 7, 2022 at 2:56 PM Grzegorz Jaszczyk <[email protected]> wrote:
> > >
> > > Currently the LPS0 prepare_late callback is aimed to run as the very
> > > last thing before entering the S2Idle state from LPS0 perspective,
> > > nevertheless between this call and the system actually entering the
> > > S2Idle state there are several places where the suspension process could
> > > be canceled.
> >
> > And why is this a problem?
> >
> > The cancellation will occur only if there is a wakeup signal that
> > would otherwise cause one of the CPUs to exit the idle state. Such a
> > wakeup signal can appear after calling the new notifier as well, so
> > why does it make a difference?
>
> It could also occur due to suspend_test. Additionally with new
> notifier we could get notification when the system wakes up from
> s2idle_loop and immediately goes to sleep again (due to e.g.
> acpi_s2idle_wake condition not being met) - in this case relying on
> prepare_late callback is not possible since it is not called in this
> path.
>
> >
> > > In order to notify VMM about guest entering suspend, extend the S2Idle
> > > ops by new notify callback, which will be really invoked as a very last
> > > thing before guest actually enters S2Idle state.
> >
> > It is not guaranteed that "suspend" (defined as all CPUs entering idle
> > states) will be actually entered even after this "last step".
>
> Since this whole patchset is aimed at notifying the host about a guest
> entering s2idle state, reaching this step can be considered as a
> suspend "entry point" for VM IMO. It is because we are talking about
> the vCPU not the real CPU. Therefore it seems to me, that even if some
> other vCPUs could still get some wakeup signal they will not be able
> to kick (through s2idle_wake->swake_up_one(&s2idle_wait_head);) the
> original vCPU which entered s2idle_loop, triggered the new notifier
> and is halted due to handling vCPU exit (and was about to trigger
> swait_event_exclusive). So it will prevent the VM's resume process
> from being started.
>
> >
> > > Additionally extend the acpi_s2idle_dev_ops by notify() callback so
> > > any driver can hook into it and allow to implement its own notification.
> > >
> > > Taking advantage of e.g. existing acpi_s2idle_dev_ops's prepare/restore
> > > hooks is not an option since it will not allow to prevent race
> > > conditions:
> > > - VM0 enters s2idle
> > > - host notes about VM0 is in s2idle
> > > - host continues with system suspension but in the meantime VM0 exits
> > > s2idle and sends notification but it is already too late (VM could not
> > > even send notification on time).
> >
> > Too late for what?
>
> Too late to cancel the host suspend process, which thinks that the VM
> is in s2idle state while it isn't.
>
> >
> > > Introducing notify() as a very last step before the system enters S2Idle
> > > together with an assumption that the VMM has control over guest
> > > resumption allows preventing mentioned races.
> >
> > How does it do that?
>
> At the moment when VM triggers this new notifier we trap on MMIO
> access and the VMM handles vCPU exit (so the vCPU is "halted").
> Therefore the VMM could control when it finishes such handling and
> releases the vCPU again.
>
> Maybe adding some more context will be helpful. This patchset was
> aimed for two different scenarios actually:
> 1) Host is about to enter the suspend state and needs first to suspend
> VM with all pass-through devices. In this case the host waits for
> s2idle notification from the guest and when it receives it, it
> continues with its own suspend process.
> 2) Guest could be a "privileged" one (in terms of VMM) and when the
> guest enters s2idle state it notifies the host, which in turn triggers
> the suspend process of the host.
>
> >
> > It looks like you want suspend-to-idle to behave like S3 and it won't.
>
> In a way, yes, we compensate for the lack of something like PM1_CNT to
> trap on for detecting that the guest is suspending.
> We could instead force the guest to use S3 but IMO it is undesirable,
> since it generally does make a difference which suspend mode is used
> in the guest, s2idle or S3, e.g some drivers check which suspend type
> is used and based on that behaves differently during suspend. One of
> the example is:
> https://elixir.bootlin.com/linux/v5.18.12/source/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c#L2323
> https://elixir.bootlin.com/linux/v5.18.12/source/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c#L1069
> https://elixir.bootlin.com/linux/v5.18.12/source/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c#L583
>
> Thank you,
> Grzegorz
Hi Rafael,
Gentle ping
Best regards,
Grzegorz
pon., 22 sie 2022 o 11:26 Grzegorz Jaszczyk <[email protected]> napisał(a):
>
> Hi Rafael,
>
> Could you please kindly comment on the above?
>
> Thank you in advance,
> Grzegorz
>
> śr., 20 lip 2022 o 15:15 Grzegorz Jaszczyk <[email protected]> napisał(a):
> >
> > wt., 19 lip 2022 o 20:09 Rafael J. Wysocki <[email protected]> napisał(a):
> > >
> > > On Thu, Jul 7, 2022 at 2:56 PM Grzegorz Jaszczyk <[email protected]> wrote:
> > > >
> > > > Currently the LPS0 prepare_late callback is aimed to run as the very
> > > > last thing before entering the S2Idle state from LPS0 perspective,
> > > > nevertheless between this call and the system actually entering the
> > > > S2Idle state there are several places where the suspension process could
> > > > be canceled.
> > >
> > > And why is this a problem?
> > >
> > > The cancellation will occur only if there is a wakeup signal that
> > > would otherwise cause one of the CPUs to exit the idle state. Such a
> > > wakeup signal can appear after calling the new notifier as well, so
> > > why does it make a difference?
> >
> > It could also occur due to suspend_test. Additionally with new
> > notifier we could get notification when the system wakes up from
> > s2idle_loop and immediately goes to sleep again (due to e.g.
> > acpi_s2idle_wake condition not being met) - in this case relying on
> > prepare_late callback is not possible since it is not called in this
> > path.
> >
> > >
> > > > In order to notify VMM about guest entering suspend, extend the S2Idle
> > > > ops by new notify callback, which will be really invoked as a very last
> > > > thing before guest actually enters S2Idle state.
> > >
> > > It is not guaranteed that "suspend" (defined as all CPUs entering idle
> > > states) will be actually entered even after this "last step".
> >
> > Since this whole patchset is aimed at notifying the host about a guest
> > entering s2idle state, reaching this step can be considered as a
> > suspend "entry point" for VM IMO. It is because we are talking about
> > the vCPU not the real CPU. Therefore it seems to me, that even if some
> > other vCPUs could still get some wakeup signal they will not be able
> > to kick (through s2idle_wake->swake_up_one(&s2idle_wait_head);) the
> > original vCPU which entered s2idle_loop, triggered the new notifier
> > and is halted due to handling vCPU exit (and was about to trigger
> > swait_event_exclusive). So it will prevent the VM's resume process
> > from being started.
> >
> > >
> > > > Additionally extend the acpi_s2idle_dev_ops by notify() callback so
> > > > any driver can hook into it and allow to implement its own notification.
> > > >
> > > > Taking advantage of e.g. existing acpi_s2idle_dev_ops's prepare/restore
> > > > hooks is not an option since it will not allow to prevent race
> > > > conditions:
> > > > - VM0 enters s2idle
> > > > - host notes about VM0 is in s2idle
> > > > - host continues with system suspension but in the meantime VM0 exits
> > > > s2idle and sends notification but it is already too late (VM could not
> > > > even send notification on time).
> > >
> > > Too late for what?
> >
> > Too late to cancel the host suspend process, which thinks that the VM
> > is in s2idle state while it isn't.
> >
> > >
> > > > Introducing notify() as a very last step before the system enters S2Idle
> > > > together with an assumption that the VMM has control over guest
> > > > resumption allows preventing mentioned races.
> > >
> > > How does it do that?
> >
> > At the moment when VM triggers this new notifier we trap on MMIO
> > access and the VMM handles vCPU exit (so the vCPU is "halted").
> > Therefore the VMM could control when it finishes such handling and
> > releases the vCPU again.
> >
> > Maybe adding some more context will be helpful. This patchset was
> > aimed for two different scenarios actually:
> > 1) Host is about to enter the suspend state and needs first to suspend
> > VM with all pass-through devices. In this case the host waits for
> > s2idle notification from the guest and when it receives it, it
> > continues with its own suspend process.
> > 2) Guest could be a "privileged" one (in terms of VMM) and when the
> > guest enters s2idle state it notifies the host, which in turn triggers
> > the suspend process of the host.
> >
> > >
> > > It looks like you want suspend-to-idle to behave like S3 and it won't.
> >
> > In a way, yes, we compensate for the lack of something like PM1_CNT to
> > trap on for detecting that the guest is suspending.
> > We could instead force the guest to use S3 but IMO it is undesirable,
> > since it generally does make a difference which suspend mode is used
> > in the guest, s2idle or S3, e.g some drivers check which suspend type
> > is used and based on that behaves differently during suspend. One of
> > the example is:
> > https://elixir.bootlin.com/linux/v5.18.12/source/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c#L2323
> > https://elixir.bootlin.com/linux/v5.18.12/source/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c#L1069
> > https://elixir.bootlin.com/linux/v5.18.12/source/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c#L583
> >
> > Thank you,
> > Grzegorz
Hi Rafael,
Kindly reminder about this topic.
BTW I've noticed that in the meantime v. similar patch was merged but
aimed for debugging purposes [1] (it uses s/notify/check and invokes
the callback a bit earlier just before s2idle_entry).
Perhaps combining Mario's [1] with aligned to it patch #2 of this
series [2] could be used and accepted as s2idle notifications method?
[1] https://patchwork.kernel.org/project/linux-pm/patch/[email protected]
[2] https://patchwork.kernel.org/project/linux-pm/patch/[email protected]/
Best regards,
Grzegorz
pon., 12 wrz 2022 o 16:44 Grzegorz Jaszczyk <[email protected]> napisał(a):
>
> Hi Rafael,
>
> Gentle ping
>
> Best regards,
> Grzegorz
>
> pon., 22 sie 2022 o 11:26 Grzegorz Jaszczyk <[email protected]> napisał(a):
> >
> > Hi Rafael,
> >
> > Could you please kindly comment on the above?
> >
> > Thank you in advance,
> > Grzegorz
> >
> > śr., 20 lip 2022 o 15:15 Grzegorz Jaszczyk <[email protected]> napisał(a):
> > >
> > > wt., 19 lip 2022 o 20:09 Rafael J. Wysocki <[email protected]> napisał(a):
> > > >
> > > > On Thu, Jul 7, 2022 at 2:56 PM Grzegorz Jaszczyk <[email protected]> wrote:
> > > > >
> > > > > Currently the LPS0 prepare_late callback is aimed to run as the very
> > > > > last thing before entering the S2Idle state from LPS0 perspective,
> > > > > nevertheless between this call and the system actually entering the
> > > > > S2Idle state there are several places where the suspension process could
> > > > > be canceled.
> > > >
> > > > And why is this a problem?
> > > >
> > > > The cancellation will occur only if there is a wakeup signal that
> > > > would otherwise cause one of the CPUs to exit the idle state. Such a
> > > > wakeup signal can appear after calling the new notifier as well, so
> > > > why does it make a difference?
> > >
> > > It could also occur due to suspend_test. Additionally with new
> > > notifier we could get notification when the system wakes up from
> > > s2idle_loop and immediately goes to sleep again (due to e.g.
> > > acpi_s2idle_wake condition not being met) - in this case relying on
> > > prepare_late callback is not possible since it is not called in this
> > > path.
> > >
> > > >
> > > > > In order to notify VMM about guest entering suspend, extend the S2Idle
> > > > > ops by new notify callback, which will be really invoked as a very last
> > > > > thing before guest actually enters S2Idle state.
> > > >
> > > > It is not guaranteed that "suspend" (defined as all CPUs entering idle
> > > > states) will be actually entered even after this "last step".
> > >
> > > Since this whole patchset is aimed at notifying the host about a guest
> > > entering s2idle state, reaching this step can be considered as a
> > > suspend "entry point" for VM IMO. It is because we are talking about
> > > the vCPU not the real CPU. Therefore it seems to me, that even if some
> > > other vCPUs could still get some wakeup signal they will not be able
> > > to kick (through s2idle_wake->swake_up_one(&s2idle_wait_head);) the
> > > original vCPU which entered s2idle_loop, triggered the new notifier
> > > and is halted due to handling vCPU exit (and was about to trigger
> > > swait_event_exclusive). So it will prevent the VM's resume process
> > > from being started.
> > >
> > > >
> > > > > Additionally extend the acpi_s2idle_dev_ops by notify() callback so
> > > > > any driver can hook into it and allow to implement its own notification.
> > > > >
> > > > > Taking advantage of e.g. existing acpi_s2idle_dev_ops's prepare/restore
> > > > > hooks is not an option since it will not allow to prevent race
> > > > > conditions:
> > > > > - VM0 enters s2idle
> > > > > - host notes about VM0 is in s2idle
> > > > > - host continues with system suspension but in the meantime VM0 exits
> > > > > s2idle and sends notification but it is already too late (VM could not
> > > > > even send notification on time).
> > > >
> > > > Too late for what?
> > >
> > > Too late to cancel the host suspend process, which thinks that the VM
> > > is in s2idle state while it isn't.
> > >
> > > >
> > > > > Introducing notify() as a very last step before the system enters S2Idle
> > > > > together with an assumption that the VMM has control over guest
> > > > > resumption allows preventing mentioned races.
> > > >
> > > > How does it do that?
> > >
> > > At the moment when VM triggers this new notifier we trap on MMIO
> > > access and the VMM handles vCPU exit (so the vCPU is "halted").
> > > Therefore the VMM could control when it finishes such handling and
> > > releases the vCPU again.
> > >
> > > Maybe adding some more context will be helpful. This patchset was
> > > aimed for two different scenarios actually:
> > > 1) Host is about to enter the suspend state and needs first to suspend
> > > VM with all pass-through devices. In this case the host waits for
> > > s2idle notification from the guest and when it receives it, it
> > > continues with its own suspend process.
> > > 2) Guest could be a "privileged" one (in terms of VMM) and when the
> > > guest enters s2idle state it notifies the host, which in turn triggers
> > > the suspend process of the host.
> > >
> > > >
> > > > It looks like you want suspend-to-idle to behave like S3 and it won't.
> > >
> > > In a way, yes, we compensate for the lack of something like PM1_CNT to
> > > trap on for detecting that the guest is suspending.
> > > We could instead force the guest to use S3 but IMO it is undesirable,
> > > since it generally does make a difference which suspend mode is used
> > > in the guest, s2idle or S3, e.g some drivers check which suspend type
> > > is used and based on that behaves differently during suspend. One of
> > > the example is:
> > > https://elixir.bootlin.com/linux/v5.18.12/source/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c#L2323
> > > https://elixir.bootlin.com/linux/v5.18.12/source/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c#L1069
> > > https://elixir.bootlin.com/linux/v5.18.12/source/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c#L583
> > >
> > > Thank you,
> > > Grzegorz
On Wed, Nov 30, 2022 at 1:25 PM Grzegorz Jaszczyk <[email protected]> wrote:
>
> Hi Rafael,
>
> Kindly reminder about this topic.
Well, it's been a bit outdated since Mario's changes have been integrated.
> BTW I've noticed that in the meantime v. similar patch was merged but
> aimed for debugging purposes [1] (it uses s/notify/check and invokes
> the callback a bit earlier just before s2idle_entry).
> Perhaps combining Mario's [1] with aligned to it patch #2 of this
> series [2] could be used and accepted as s2idle notifications method?
Maybe it could.
Please send a new series based on top of the current mainline kernel
(6.2-rc7 as of yesterday) and we'll see.
> [1] https://patchwork.kernel.org/project/linux-pm/patch/[email protected]
> [2] https://patchwork.kernel.org/project/linux-pm/patch/[email protected]/
>
> pon., 12 wrz 2022 o 16:44 Grzegorz Jaszczyk <[email protected]> napisał(a):
> >
> > Hi Rafael,
> >
> > Gentle ping
> >
> > Best regards,
> > Grzegorz
> >
> > pon., 22 sie 2022 o 11:26 Grzegorz Jaszczyk <[email protected]> napisał(a):
> > >
> > > Hi Rafael,
> > >
> > > Could you please kindly comment on the above?
> > >
> > > Thank you in advance,
> > > Grzegorz
> > >
> > > śr., 20 lip 2022 o 15:15 Grzegorz Jaszczyk <[email protected]> napisał(a):
> > > >
> > > > wt., 19 lip 2022 o 20:09 Rafael J. Wysocki <[email protected]> napisał(a):
> > > > >
> > > > > On Thu, Jul 7, 2022 at 2:56 PM Grzegorz Jaszczyk <[email protected]> wrote:
> > > > > >
> > > > > > Currently the LPS0 prepare_late callback is aimed to run as the very
> > > > > > last thing before entering the S2Idle state from LPS0 perspective,
> > > > > > nevertheless between this call and the system actually entering the
> > > > > > S2Idle state there are several places where the suspension process could
> > > > > > be canceled.
> > > > >
> > > > > And why is this a problem?
> > > > >
> > > > > The cancellation will occur only if there is a wakeup signal that
> > > > > would otherwise cause one of the CPUs to exit the idle state. Such a
> > > > > wakeup signal can appear after calling the new notifier as well, so
> > > > > why does it make a difference?
> > > >
> > > > It could also occur due to suspend_test. Additionally with new
> > > > notifier we could get notification when the system wakes up from
> > > > s2idle_loop and immediately goes to sleep again (due to e.g.
> > > > acpi_s2idle_wake condition not being met) - in this case relying on
> > > > prepare_late callback is not possible since it is not called in this
> > > > path.
> > > >
> > > > >
> > > > > > In order to notify VMM about guest entering suspend, extend the S2Idle
> > > > > > ops by new notify callback, which will be really invoked as a very last
> > > > > > thing before guest actually enters S2Idle state.
> > > > >
> > > > > It is not guaranteed that "suspend" (defined as all CPUs entering idle
> > > > > states) will be actually entered even after this "last step".
> > > >
> > > > Since this whole patchset is aimed at notifying the host about a guest
> > > > entering s2idle state, reaching this step can be considered as a
> > > > suspend "entry point" for VM IMO. It is because we are talking about
> > > > the vCPU not the real CPU. Therefore it seems to me, that even if some
> > > > other vCPUs could still get some wakeup signal they will not be able
> > > > to kick (through s2idle_wake->swake_up_one(&s2idle_wait_head);) the
> > > > original vCPU which entered s2idle_loop, triggered the new notifier
> > > > and is halted due to handling vCPU exit (and was about to trigger
> > > > swait_event_exclusive). So it will prevent the VM's resume process
> > > > from being started.
> > > >
> > > > >
> > > > > > Additionally extend the acpi_s2idle_dev_ops by notify() callback so
> > > > > > any driver can hook into it and allow to implement its own notification.
> > > > > >
> > > > > > Taking advantage of e.g. existing acpi_s2idle_dev_ops's prepare/restore
> > > > > > hooks is not an option since it will not allow to prevent race
> > > > > > conditions:
> > > > > > - VM0 enters s2idle
> > > > > > - host notes about VM0 is in s2idle
> > > > > > - host continues with system suspension but in the meantime VM0 exits
> > > > > > s2idle and sends notification but it is already too late (VM could not
> > > > > > even send notification on time).
> > > > >
> > > > > Too late for what?
> > > >
> > > > Too late to cancel the host suspend process, which thinks that the VM
> > > > is in s2idle state while it isn't.
> > > >
> > > > >
> > > > > > Introducing notify() as a very last step before the system enters S2Idle
> > > > > > together with an assumption that the VMM has control over guest
> > > > > > resumption allows preventing mentioned races.
> > > > >
> > > > > How does it do that?
> > > >
> > > > At the moment when VM triggers this new notifier we trap on MMIO
> > > > access and the VMM handles vCPU exit (so the vCPU is "halted").
> > > > Therefore the VMM could control when it finishes such handling and
> > > > releases the vCPU again.
> > > >
> > > > Maybe adding some more context will be helpful. This patchset was
> > > > aimed for two different scenarios actually:
> > > > 1) Host is about to enter the suspend state and needs first to suspend
> > > > VM with all pass-through devices. In this case the host waits for
> > > > s2idle notification from the guest and when it receives it, it
> > > > continues with its own suspend process.
> > > > 2) Guest could be a "privileged" one (in terms of VMM) and when the
> > > > guest enters s2idle state it notifies the host, which in turn triggers
> > > > the suspend process of the host.
> > > >
> > > > >
> > > > > It looks like you want suspend-to-idle to behave like S3 and it won't.
> > > >
> > > > In a way, yes, we compensate for the lack of something like PM1_CNT to
> > > > trap on for detecting that the guest is suspending.
> > > > We could instead force the guest to use S3 but IMO it is undesirable,
> > > > since it generally does make a difference which suspend mode is used
> > > > in the guest, s2idle or S3, e.g some drivers check which suspend type
> > > > is used and based on that behaves differently during suspend. One of
> > > > the example is:
> > > > https://elixir.bootlin.com/linux/v5.18.12/source/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c#L2323
> > > > https://elixir.bootlin.com/linux/v5.18.12/source/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c#L1069
> > > > https://elixir.bootlin.com/linux/v5.18.12/source/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c#L583
> > > >
> > > > Thank you,
> > > > Grzegorz