Re: [PATCH v12 13/13] xen/arm: Add host system suspend backend

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hi Mykola,

Mykola Kvach <mykola_kvach@xxxxxxxx> writes:

> From: Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>
>
> Add the Xen-wide suspend/resume backend used after a control-domain
> vPSCI SYSTEM_SUSPEND request has been accepted. The vPSCI policy,
> runtime driver blockers and control-domain sequencing checks are handled
> by the preceding commit; this change adds the code that actually drives
> the host suspend attempt.
>
> The backend runs from a tasklet scheduled on pCPU0, because non-boot CPUs
> are disabled during suspend. It freezes domains, disables the scheduler
> and then disables non-boot CPUs.
>
> Host-side suspend participants are handled in phases. IOMMU and console
> state are suspended first. Local IRQs are then disabled before suspending
> timer and GIC state. On resume or failure, the completed suspend phases
> are unwound in reverse: GIC and timer state are restored while IRQs are
> still disabled, local IRQs are restored, and then console and IOMMU state
> are restored.
>
> On boot, init_ttbr is normally initialized during secondary CPU hotplug.
> On uniprocessor systems this can leave init_ttbr uninitialized, so set it
> from the boot CPU before entering suspend.
>
> Note: the code is behind CONFIG_HAS_SYSTEM_SUSPEND, which is currently
> only selected when UNSUPPORTED is set and MPU is not set.
>
> Signed-off-by: Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>
> Signed-off-by: Saeed Nowshadi <saeed.nowshadi@xxxxxxxxxx>
> Signed-off-by: Mykyta Poturai <mykyta_poturai@xxxxxxxx>
> Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>

Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>

> ---
> Changes in V10:
> - Re-apply boot CPU local errata/workaround handling after SYSTEM_SUSPEND,
>   before resuming the rest of the host suspend path.
> - Move set_init_ttbr() declaration to asm/mmu/mm.h, since it is
>   MMU-specific.
>
> Changes in V9:
> - Split vPSCI availability policy, runtime host-suspend blockers and the
>   domain-readiness precheck into the preceding commit.
> - Trigger the host suspend backend from the control-domain SYSTEM_SUSPEND
>   path.
> - Reorder the host suspend/resume phases so the timer is suspended with
>   local IRQs disabled and local IRQs are restored after the GIC and timer
>   resume paths, before the console and IOMMU resume paths.
> - Move HAS_HWDOM_SYSTEM_SUSPEND and related logic to policy patch.
>
> Changes in V8:
> - Add a pre-suspend check in system_suspend() after scheduler_disable() to
>   require all domains to be in the shut down state with SHUTDOWN_suspend
>   before proceeding with the global suspend flow.
> - Drop the common-level depends on !ARM_64 || !SYSTEM_SUSPEND from
>   CONFIG_HAS_HWDOM_SHUTDOWN_ON_SUSPEND and model the ARM64 suspend case
>   with an arch-selected capability instead.
> - Rename CONFIG_HAS_HWDOM_SHUTDOWN_ON_SUSPEND to
>   CONFIG_HAS_HWDOM_SYSTEM_SUSPEND.
> - Rename need_hwdom_shutdown() to want_hwdom_shutdown().
>
> Changes in V7:
> - Control domain is responsible for host suspend.
> - Add an empty inline host_system_suspend() function when SYSTEM_SUSPEND
>   config is disabled.
> - Use IS_ENABLED() for config checking instead of #ifdef.
> - Replace #ifdef checks in domain_shutdown() with IS_ENABLED() to simplify
>   control flow.
> - Factor hardware domain shutdown condition into a helper
>   (need_hwdom_shutdown()) to avoid preprocessor directives inside the
>   function.
> - Squash with iommu suspend/resume commit.
> ---
>  xen/arch/arm/Kconfig                 |   1 +
>  xen/arch/arm/cpuerrata.c             |   7 +-
>  xen/arch/arm/include/asm/cpuerrata.h |   1 +
>  xen/arch/arm/include/asm/mmu/mm.h    |   2 +
>  xen/arch/arm/include/asm/suspend.h   |   2 +
>  xen/arch/arm/mmu/smpboot.c           |   2 +-
>  xen/arch/arm/suspend.c               | 156 +++++++++++++++++++++++++++
>  xen/arch/arm/vpsci.c                 |  10 +-
>  8 files changed, 177 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index 9027aa17eb..da1585ec50 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -9,6 +9,7 @@ config ARM_64
>       select 64BIT
>       select HAS_DOMAIN_TYPE
>       select HAS_FAST_MULTIPLY
> +     select HAS_SYSTEM_SUSPEND if !MPU && UNSUPPORTED
>       select HAS_VPCI_GUEST_SUPPORT if PCI_PASSTHROUGH
>  
>  config ARM
> diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
> index 3a32183618..e6499aaab3 100644
> --- a/xen/arch/arm/cpuerrata.c
> +++ b/xen/arch/arm/cpuerrata.c
> @@ -782,6 +782,11 @@ void check_local_cpu_errata(void)
>      update_cpu_capabilities(arm_errata, "enabled workaround for");
>  }
>  
> +int enable_local_cpu_errata_workarounds(void)
> +{
> +    return enable_nonboot_cpu_caps(arm_errata);
> +}
> +
>  void __init enable_errata_workarounds(void)
>  {
>      enable_cpu_capabilities(arm_errata);
> @@ -818,7 +823,7 @@ static int cpu_errata_callback(struct notifier_block *nfb,
>           * fixed to expect an error at CPU_STARTING phase.
>           */
>          ASSERT(system_state != SYS_STATE_boot);
> -        rc = enable_nonboot_cpu_caps(arm_errata);
> +        rc = enable_local_cpu_errata_workarounds();
>          break;
>      default:
>          break;
> diff --git a/xen/arch/arm/include/asm/cpuerrata.h 
> b/xen/arch/arm/include/asm/cpuerrata.h
> index 1799a16d7e..b93521326f 100644
> --- a/xen/arch/arm/include/asm/cpuerrata.h
> +++ b/xen/arch/arm/include/asm/cpuerrata.h
> @@ -5,6 +5,7 @@
>  #include <asm/alternative.h>
>  
>  void check_local_cpu_errata(void);
> +int enable_local_cpu_errata_workarounds(void);
>  void enable_errata_workarounds(void);
>  
>  #define CHECK_WORKAROUND_HELPER(erratum, feature, arch)         \
> diff --git a/xen/arch/arm/include/asm/mmu/mm.h 
> b/xen/arch/arm/include/asm/mmu/mm.h
> index 7f4d59137d..ee73a77777 100644
> --- a/xen/arch/arm/include/asm/mmu/mm.h
> +++ b/xen/arch/arm/include/asm/mmu/mm.h
> @@ -110,6 +110,8 @@ void dump_pt_walk(paddr_t ttbr, paddr_t addr,
>  extern void switch_ttbr(uint64_t ttbr);
>  extern void relocate_and_switch_ttbr(uint64_t ttbr);
>  
> +void set_init_ttbr(lpae_t *root);
> +
>  #endif /* __ARM_MMU_MM_H__ */
>  
>  /*
> diff --git a/xen/arch/arm/include/asm/suspend.h 
> b/xen/arch/arm/include/asm/suspend.h
> index 50dc6e9fdf..889a6509d9 100644
> --- a/xen/arch/arm/include/asm/suspend.h
> +++ b/xen/arch/arm/include/asm/suspend.h
> @@ -41,11 +41,13 @@ int prepare_resume_ctx(void);
>  void hyp_resume(void);
>  bool host_system_suspend_allowed(void);
>  void host_system_suspend_disable(const char *reason);
> +void host_system_suspend(struct domain *d);
>  
>  #else /* !CONFIG_SYSTEM_SUSPEND */
>  
>  static inline bool host_system_suspend_allowed(void) { return false; }
>  static inline void host_system_suspend_disable(const char *reason) {}
> +static inline void host_system_suspend(struct domain *d) {}
>  
>  #endif
>  
> diff --git a/xen/arch/arm/mmu/smpboot.c b/xen/arch/arm/mmu/smpboot.c
> index 37e91d72b7..ff508ecf40 100644
> --- a/xen/arch/arm/mmu/smpboot.c
> +++ b/xen/arch/arm/mmu/smpboot.c
> @@ -72,7 +72,7 @@ static void clear_boot_pagetables(void)
>      clear_table(boot_third);
>  }
>  
> -static void set_init_ttbr(lpae_t *root)
> +void set_init_ttbr(lpae_t *root)
>  {
>      /*
>       * init_ttbr is part of the identity mapping which is read-only. So
> diff --git a/xen/arch/arm/suspend.c b/xen/arch/arm/suspend.c
> index c7c26bcf03..3fe2ffa4fb 100644
> --- a/xen/arch/arm/suspend.c
> +++ b/xen/arch/arm/suspend.c
> @@ -1,10 +1,18 @@
>  /* SPDX-License-Identifier: GPL-2.0-only */
>  
> +#include <asm/cpuerrata.h>
> +#include <asm/cpufeature.h>
> +#include <asm/gic.h>
>  #include <asm/psci.h>
>  #include <asm/suspend.h>
>  
> +#include <xen/console.h>
> +#include <xen/cpu.h>
> +#include <xen/iommu.h>
>  #include <xen/lib.h>
> +#include <xen/sched.h>
>  #include <xen/serial.h>
> +#include <xen/tasklet.h>
>  
>  struct resume_cpu_context resume_cpu_context;
>  
> @@ -44,6 +52,154 @@ void host_system_suspend_disable(const char *reason)
>             reason ? reason : "unsupported suspend/resume path");
>  }
>  
> +/* Xen suspend. data identifies the domain that initiated suspend. */
> +static void system_suspend(void *data)
> +{
> +    int status;
> +    unsigned long flags;
> +    struct domain *d = (struct domain *)data;
> +
> +    BUG_ON(system_state != SYS_STATE_active);
> +
> +    system_state = SYS_STATE_suspend;
> +
> +    printk("Xen suspending...\n");
> +
> +    freeze_domains();
> +    scheduler_disable();
> +
> +    /*
> +     * Non-boot CPUs have to be disabled on suspend and enabled on resume
> +     * (hotplug-based mechanism). Disabling non-boot CPUs will lead to PSCI
> +     * CPU_OFF to be called by each non-boot CPU. Depending on the underlying
> +     * platform capabilities, this may lead to the physical powering down of
> +     * CPUs.
> +     */
> +    status = disable_nonboot_cpus();
> +    if ( status )
> +    {
> +        system_state = SYS_STATE_resume;
> +        goto resume_nonboot_cpus;
> +    }
> +
> +    console_start_sync();
> +    status = iommu_suspend();
> +    if ( status )
> +    {
> +        system_state = SYS_STATE_resume;
> +        goto resume_end_sync;
> +    }
> +
> +    status = console_suspend();
> +    if ( status )
> +    {
> +        dprintk(XENLOG_ERR, "Failed to suspend the console, err=%d\n", 
> status);
> +        system_state = SYS_STATE_resume;
> +        goto resume_iommu;
> +    }
> +
> +    local_irq_save(flags);
> +
> +    time_suspend();
> +
> +    status = gic_suspend();
> +    if ( status )
> +    {
> +        system_state = SYS_STATE_resume;
> +        goto resume_time;
> +    }
> +
> +    set_init_ttbr(xen_pgtable);
> +
> +    /*
> +     * Enable identity mapping before entering suspend to simplify
> +     * the resume path
> +     */
> +    update_boot_mapping(true);
> +
> +    if ( prepare_resume_ctx() )
> +    {
> +        status = call_psci_system_suspend();
> +        /*
> +         * If suspend is finalized properly by above system suspend PSCI 
> call,
> +         * the code below in this 'if' branch will never execute. Execution
> +         * will continue from hyp_resume which is the hypervisor's resume 
> point.
> +         * In hyp_resume CPU context will be restored and since 
> link-register is
> +         * restored as well, it will appear to return from 
> prepare_resume_ctx.
> +         * The difference in returning from prepare_resume_ctx on system 
> suspend
> +         * versus resume is in function's return value: on suspend, the 
> return
> +         * value is a non-zero value, on resume it is zero. That is why the
> +         * control flow will not re-enter this 'if' branch on resume.
> +         */
> +        if ( status )
> +            dprintk(XENLOG_WARNING, "PSCI system suspend failed, err=%d\n",
> +                    status);
> +
> +        system_state = SYS_STATE_resume;
> +    }
> +    else
> +    {
> +        system_state = SYS_STATE_resume;
> +
> +        /*
> +         * CPU0 resumes directly from hyp_resume(), bypassing the CPU hotplug
> +         * path that re-checks and re-enables errata workarounds for 
> secondary
> +         * CPUs.
> +         */
> +        check_local_cpu_errata();
> +        check_local_cpu_features();
> +        BUG_ON(enable_local_cpu_errata_workarounds());
> +    }
> +
> +    update_boot_mapping(false);
> +
> +    gic_resume();
> +
> + resume_time:
> +    time_resume();
> +
> +    local_irq_restore(flags);
> +
> +    console_resume();
> +
> + resume_iommu:
> +    iommu_resume();
> +
> + resume_end_sync:
> +    console_end_sync();
> +
> + resume_nonboot_cpus:
> +    /*
> +     * The rcu_barrier() has to be added to ensure that the per cpu area is
> +     * freed before a non-boot CPU tries to initialize it 
> (_free_percpu_area()
> +     * has to be called before the init_percpu_area()). This scenario occurs
> +     * when non-boot CPUs are hot-unplugged on suspend and hotplugged on 
> resume.
> +     */
> +    rcu_barrier();
> +    enable_nonboot_cpus();
> +
> +    scheduler_enable();
> +    thaw_domains();
> +
> +    system_state = SYS_STATE_active;
> +
> +    printk("Resume (status %d)\n", status);
> +
> +    domain_resume(d);
> +}
> +
> +static DECLARE_TASKLET(system_suspend_tasklet, system_suspend, NULL);
> +
> +void host_system_suspend(struct domain *d)
> +{
> +    system_suspend_tasklet.data = (void *)d;
> +    /*
> +     * The suspend procedure has to be finalized by the pCPU#0 (non-boot 
> pCPUs
> +     * will be disabled during the suspend).
> +     */
> +    tasklet_schedule_on_cpu(&system_suspend_tasklet, 0);
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/arm/vpsci.c b/xen/arch/arm/vpsci.c
> index a41355d75d..5134e75c24 100644
> --- a/xen/arch/arm/vpsci.c
> +++ b/xen/arch/arm/vpsci.c
> @@ -237,7 +237,8 @@ static bool domain_in_suspend_state(struct domain *d)
>      return suspended;
>  }
>  
> -static int32_t domain_psci_system_suspend_policy(struct domain *d)
> +static int32_t domain_psci_system_suspend_policy(struct domain *d,
> +                                                 bool *host_suspend)
>  {
>      struct domain *other;
>      bool last_awake_control_domain = true;
> @@ -300,6 +301,7 @@ static int32_t domain_psci_system_suspend_policy(struct 
> domain *d)
>      if ( !host_system_suspend_allowed() )
>          return PSCI_DENIED;
>  
> +    *host_suspend = true;
>      return 0;
>  }
>  
> @@ -310,6 +312,7 @@ static int32_t do_psci_1_0_system_suspend(register_t 
> epoint, register_t cid)
>      struct vcpu *v;
>      struct domain *d = current->domain;
>      bool is_thumb = epoint & 1;
> +    bool host_suspend = false;
>      struct resume_info *rctx = &d->arch.resume_ctx;
>  
>      /* THUMB set is not allowed with 64-bit domain */
> @@ -334,7 +337,7 @@ static int32_t do_psci_1_0_system_suspend(register_t 
> epoint, register_t cid)
>  
>      spin_lock(&vpsci_system_suspend_lock);
>  
> -    rc = domain_psci_system_suspend_policy(d);
> +    rc = domain_psci_system_suspend_policy(d, &host_suspend);
>      if ( !rc )
>      {
>          rc = domain_shutdown(d, SHUTDOWN_suspend);
> @@ -359,6 +362,9 @@ static int32_t do_psci_1_0_system_suspend(register_t 
> epoint, register_t cid)
>              "SYSTEM_SUSPEND requested, epoint=%#"PRIregister", 
> cid=%#"PRIregister"\n",
>              epoint, cid);
>  
> +    if ( host_suspend )
> +        host_system_suspend(d);
> +
>      return rc;
>  }

-- 
WBR, Volodymyr