Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220707125329.378277-1-jaz@semihalf.com> <20220707125329.378277-2-jaz@semihalf.com>
In-Reply-To: <20220707125329.378277-2-jaz@semihalf.com>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Tue, 19 Jul 2022 20:08:59 +0200
Message-ID: <CAJZ5v0gdCN3P52ko44LQMqWJvDArHxZ7p4aSiQamML7aG_kRAA@mail.gmail.com>
Subject: Re: [RFC PATCH 1/2] suspend: extend S2Idle ops by new notify handler
To:     Grzegorz Jaszczyk <jaz@semihalf.com>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        dmy@semihalf.com, Mario Limonciello <mario.limonciello@amd.com>,
        Sean Christopherson <seanjc@google.com>, dbehr@google.com,
        upstream@semihalf.com, zide.chen@intel.corp-partner.google.com,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Len Brown <lenb@kernel.org>,
        Hans de Goede <hdegoede@redhat.com>,
        Mark Gross <markgross@kernel.org>, Pavel Machek <pavel@ucw.cz>,
        Mika Westerberg <mika.westerberg@linux.intel.com>,
        Sachi King <nakato@nakato.io>,
        "open list:ACPI" <linux-acpi@vger.kernel.org>,
        "open list:X86 PLATFORM DRIVERS" 
        <platform-driver-x86@vger.kernel.org>,
        "open list:HIBERNATION (aka Software Suspend, aka swsusp)" 
        <linux-pm@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Jul 7, 2022 at 2:56 PM Grzegorz Jaszczyk <jaz@semihalf.com> 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 <jaz@semihalf.com>
> ---
>  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
>