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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Mykola,

> On 27 Aug 2026, at 16:32, Mykola Kvach <mykola_kvach@xxxxxxxx> wrote:
> 
> 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>
> ---
> 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.

This line would need wrapping as it is over 80 chars.

> +     */
> +    rcu_barrier();

Could you clarify which per-CPU area needs freeing here?
  
system_state is SYS_STATE_suspend for every CPU taken down by
disable_nonboot_cpus(), so cpu_percpu_callback() does not queue
_free_percpu_area() for any of them.

So I do not quite get what your comment case actually is.
Could you explain ?

Cheers
Bertrand

> +    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;
> }
> 
> -- 
> 2.43.0
>