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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Bertrand,

On Mon, Sep 28, 2026 at 7:20 PM Bertrand Marquis
<Bertrand.Marquis@xxxxxxx> wrote:
>
> 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 ?

You are right. I checked the original patch and the code it was
based on.

The barrier was added in the 2018 series. At that time, the Arm
CPU_DEAD callback queued _free_percpu_area() even during system
suspend. If resume reached init_percpu_area() before that callback
completed, it would return -EBUSY because the old area still
existed. This would trigger BUG_ON() in enable_nonboot_cpus().

The barrier ensured that the old area was freed before bringing
the CPU back up.

That reason no longer applies. The current code keeps the per-CPU
areas during suspend and reuses them on resume.

Also, since commit 540d4d60378c ("cpu: sync any remaining RCU
callbacks before CPU up/down"), cpu_up() already calls
rcu_barrier() through cpu_hotplug_begin(), before CPU_UP_PREPARE.

The separate barrier and its comment were carried over after these
changes. I will remove both in v13. This also removes the line
that needed wrapping.

Best regards,
Mykola