Re: [PATCH v5 07/12] xen/arm: Add support for system suspend triggered by hardware domain

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Volodymyr,

On Sat, Aug 23, 2025 at 4:00 AM Volodymyr Babchuk
<Volodymyr_Babchuk@xxxxxxxx> wrote:
>
> Hi Mykola,
>
> Mykola Kvach <xakep.amatop@xxxxxxxxx> writes:
>
> > From: Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>
> >
> > Trigger Xen suspend when the hardware domain initiates suspend via
> > SHUTDOWN_suspend. Redirect system suspend to CPU#0 to ensure the
> > suspend logic runs on the boot CPU, as required.
> >
> > Introduce full suspend/resume infrastructure gated by CONFIG_SYSTEM_SUSPEND,
> > including logic to:
> >  - disable and enable non-boot physical CPUs
> >  - freeze and thaw domains
> >  - suspend and resume the GIC, timer and console
> >  - maintain system state before and after suspend
> >
> > Remove the restriction in the PSCI interface preventing suspend from the
> > hardware domain.
> >
> > Select HAS_SYSTEM_SUSPEND for ARM_64.
> >
> > 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 v5:
> > - select HAS_SYSTEM_SUSPEND in ARM_64 instead of ARM
> > - check llc_coloring_enabled instead of LLC_COLORING during the selection
> >   of HAS_SYSTEM_SUSPEND config
> > - call host_system_suspend from guest PSCI system suspend instead of
> >   arch_domain_shutdown, reducing the complexity of the new code
> > - update some comments
> >
> > Changes introduced in V4:
> >   - drop code for saving and restoring VCPU context (for more information
> >     refer part 1 of patch series)
> >   - remove IOMMU suspend and resume calls until they will be implemented
> >   - move system suspend logic to arch_domain_shutdown, invoked from
> >     domain_shutdown
> >   - apply minor fixes such as renaming functions, updating comments, and
> >     modifying the commit message to reflect these changes
> >   - add console_end_sync to resume path after system suspend
> >
> > Changes introduced in V3:
> >   - merge changes from other commits into this patch (previously stashed):
> >     1) disable/enable non-boot physical CPUs and freeze/thaw domains during
> >        suspend/resume
> >     2) suspend/resume the timer, GIC, console, IOMMU, and hardware domain
> > ---
> >  xen/arch/arm/Kconfig               |   1 +
> >  xen/arch/arm/Makefile              |   1 +
> >  xen/arch/arm/include/asm/suspend.h |  22 +++++
> >  xen/arch/arm/suspend.c             | 151 +++++++++++++++++++++++++++++
> >  xen/arch/arm/vpsci.c               |  12 ++-
> >  xen/common/domain.c                |   4 +
> >  6 files changed, 190 insertions(+), 1 deletion(-)
> >  create mode 100644 xen/arch/arm/include/asm/suspend.h
> >  create mode 100644 xen/arch/arm/suspend.c
> >
> > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> > index a0c8160474..ccdbaa5bc3 100644
> > --- a/xen/arch/arm/Kconfig
> > +++ b/xen/arch/arm/Kconfig
> > @@ -8,6 +8,7 @@ config ARM_64
> >       depends on !ARM_32
> >       select 64BIT
> >       select HAS_FAST_MULTIPLY
> > +     select HAS_SYSTEM_SUSPEND if UNSUPPORTED
> >       select HAS_VPCI_GUEST_SUPPORT if PCI_PASSTHROUGH
> >
> >  config ARM
> > diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> > index f833cdf207..3f6247adee 100644
> > --- a/xen/arch/arm/Makefile
> > +++ b/xen/arch/arm/Makefile
> > @@ -51,6 +51,7 @@ obj-y += setup.o
> >  obj-y += shutdown.o
> >  obj-y += smp.o
> >  obj-y += smpboot.o
> > +obj-$(CONFIG_SYSTEM_SUSPEND) += suspend.o
> >  obj-$(CONFIG_SYSCTL) += sysctl.o
> >  obj-y += time.o
> >  obj-y += traps.o
> > diff --git a/xen/arch/arm/include/asm/suspend.h 
> > b/xen/arch/arm/include/asm/suspend.h
> > new file mode 100644
> > index 0000000000..78d0e2383b
> > --- /dev/null
> > +++ b/xen/arch/arm/include/asm/suspend.h
> > @@ -0,0 +1,22 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +
> > +#ifndef __ASM_ARM_SUSPEND_H__
> > +#define __ASM_ARM_SUSPEND_H__
> > +
> > +#ifdef CONFIG_SYSTEM_SUSPEND
> > +
> > +int host_system_suspend(void);
> > +
> > +#endif /* CONFIG_SYSTEM_SUSPEND */
> > +
> > +#endif /* __ASM_ARM_SUSPEND_H__ */
> > +
> > + /*
> > +  * Local variables:
> > +  * mode: C
> > +  * c-file-style: "BSD"
> > +  * c-basic-offset: 4
> > +  * tab-width: 4
> > +  * indent-tabs-mode: nil
> > +  * End:
> > +  */
> > diff --git a/xen/arch/arm/suspend.c b/xen/arch/arm/suspend.c
> > new file mode 100644
> > index 0000000000..abf4b928ce
> > --- /dev/null
> > +++ b/xen/arch/arm/suspend.c
> > @@ -0,0 +1,151 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +
> > +#include <xen/console.h>
> > +#include <xen/cpu.h>
> > +#include <xen/llc-coloring.h>
> > +#include <xen/sched.h>
> > +
> > +/*
> > + * TODO list:
> > + *  - Test system suspend with LLC_COLORING enabled and verify 
> > functionality
> > + *  - Implement IOMMU suspend/resume handlers and integrate them
> > + *    into the suspend/resume path (IPMMU and SMMU)
> > + *  - Enable "xl suspend" support on ARM architecture
> > + *  - Properly disable Xen timer watchdog from relevant services
> > + *  - Add suspend/resume CI test for ARM (QEMU if feasible)
> > + *  - Investigate feasibility and need for implementing system suspend on 
> > ARM32
> > + */
> > +
> > +/* Xen suspend. Note: data is not used (suspend is the suspend to RAM) */
> > +static long system_suspend(void *data)
> > +{
> > +    int status;
> > +    unsigned long flags;
> > +
> > +    BUG_ON(system_state != SYS_STATE_active);
> > +
> > +    system_state = SYS_STATE_suspend;
> > +    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. Tested on Xilinx Zynq Ultrascale+ MPSoC (including power down 
> > of
> > +     * each non-boot CPU).
>
> I don't think that the last part of the comment is relevant in upstream.

I’ll drop that part, as it’s not relevant for upstream.

>
> > +     */
> > +    status = disable_nonboot_cpus();
> > +    if ( status )
> > +    {
> > +        system_state = SYS_STATE_resume;
> > +        goto resume_nonboot_cpus;
> > +    }
> > +
> > +    time_suspend();
> > +
> > +    local_irq_save(flags);
> > +    status = gic_suspend();
> > +    if ( status )
> > +    {
> > +        system_state = SYS_STATE_resume;
> > +        goto resume_irqs;
> > +    }
> > +
> > +    printk("Xen suspending...\n");
> > +
> > +    console_start_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_console;
> > +    }
> > +
> > +    /*
> > +     * Enable identity mapping before entering suspend to simplify
> > +     * the resume path
> > +     */
> > +    update_boot_mapping(true);
> > +
>
> This puzzles me. I expected actually PSCI suspend call here.
>
> > +    system_state = SYS_STATE_resume;
> > +    update_boot_mapping(false);
> > +
> > + resume_console:
> > +    console_resume();
> > +    console_end_sync();
> > +
> > +    gic_resume();
> > +
> > + resume_irqs:
> > +    local_irq_restore(flags);
> > +    time_resume();
> > +
> > + 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;
> > +
> > +    /* The resume of hardware domain should always follow Xen's resume. */
> > +    domain_resume(hardware_domain);
> > +
> > +    printk("Resume (status %d)\n", status);
> > +    return status;
> > +}
> > +
> > +int host_system_suspend(void)
> > +{
> > +    int status;
> > +
> > +    /* TODO: drop check after verification that features can work together 
> > */
> > +    if ( llc_coloring_enabled )
> > +    {
> > +        dprintk(XENLOG_ERR,
> > +                "System suspend is not supported with LLC_COLORING 
> > enabled\n");
> > +        return -ENOSYS;
> > +    }
> > +
> > +    /*
> > +     * system_suspend should be called when Dom0 finalizes the suspend
> > +     * procedure from its boot core (VCPU#0). However, Dom0's VCPU#0 could
> > +     * be mapped to any PCPU (this function could be executed by any PCPU).
> > +     * The suspend procedure has to be finalized by the PCPU#0 (non-boot
> > +     * PCPUs will be disabled during the suspend).
> > +     */
> > +    status = continue_hypercall_on_cpu(0, system_suspend, NULL);
> > +
> > +    /*
> > +     * If an error happened, there is nothing that needs to be done here
> > +     * because the system_suspend always returns in fully functional state
> > +     * no matter what the outcome of suspend procedure is. If the system
> > +     * suspended successfully the function will return 0 after the resume.
> > +     * Otherwise, if an error is returned it means Xen did not suspended,
> > +     * but it is still in the same state as if the system_suspend was never
> > +     * called. We dump a debug message in case of an error for debugging/
> > +     * logging purpose.
> > +     */
> > +    if ( status )
> > +        dprintk(XENLOG_ERR, "Failed to suspend, errno=%d\n", status);
> > +
> > +    return status;
> > +}
> > +
> > +/*
> > + * Local variables:
> > + * mode: C
> > + * c-file-style: "BSD"
> > + * c-basic-offset: 4
> > + * indent-tabs-mode: nil
> > + * End:
> > + */
> > diff --git a/xen/arch/arm/vpsci.c b/xen/arch/arm/vpsci.c
> > index 67d369a8a2..757e719ea7 100644
> > --- a/xen/arch/arm/vpsci.c
> > +++ b/xen/arch/arm/vpsci.c
> > @@ -4,6 +4,7 @@
> >  #include <xen/types.h>
> >
> >  #include <asm/current.h>
> > +#include <asm/suspend.h>
> >  #include <asm/vgic.h>
> >  #include <asm/vpsci.h>
> >  #include <asm/event.h>
> > @@ -214,9 +215,10 @@ static int32_t do_psci_1_0_system_suspend(register_t 
> > epoint, register_t cid)
> >      struct vcpu *v;
> >      struct domain *d = current->domain;
> >
> > -    /* SYSTEM_SUSPEND is not supported for the hardware domain yet */
> > +#ifndef CONFIG_SYSTEM_SUSPEND
> >      if ( is_hardware_domain(d) )
> >          return PSCI_NOT_SUPPORTED;
> > +#endif
> >
> >      /* Ensure that all CPUs other than the calling one are offline */
> >      domain_lock(d);
> > @@ -234,6 +236,14 @@ static int32_t do_psci_1_0_system_suspend(register_t 
> > epoint, register_t cid)
> >      if ( rc )
> >          return PSCI_DENIED;
> >
> > +#ifdef CONFIG_SYSTEM_SUSPEND
> > +    if ( is_hardware_domain(d) && host_system_suspend() )
> > +    {
> > +        domain_resume_nopause(d);
> > +        return PSCI_DENIED;
> > +    }
> > +#endif
> > +
> >      rc = do_setup_vcpu_ctx(current, epoint, cid);
> >      if ( rc != PSCI_SUCCESS )
> >      {
> > diff --git a/xen/common/domain.c b/xen/common/domain.c
> > index c3609b0cb0..414a691242 100644
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -1311,7 +1311,11 @@ int domain_shutdown(struct domain *d, u8 reason)
> >          d->shutdown_code = reason;
> >      reason = d->shutdown_code;
> >
> > +#if defined(CONFIG_SYSTEM_SUSPEND) && defined(CONFIG_ARM)
> > +    if ( reason != SHUTDOWN_suspend && is_hardware_domain(d) )
> > +#else
> >      if ( is_hardware_domain(d) )
> > +#endif
> >          hwdom_shutdown(reason);
> >
> >      if ( d->is_shutting_down )
>
> --
> WBR, Volodymyr

Best regards,
Mykola