Re: [PATCH v8 13/13] xen/arm: Add support for system suspend triggered by hardware domain

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

Hi Mykola,

> diff --git a/xen/arch/arm/suspend.c b/xen/arch/arm/suspend.c
> index e38566b0b7..4d1289776b 100644
> --- a/xen/arch/arm/suspend.c
> +++ b/xen/arch/arm/suspend.c
> @@ -1,9 +1,190 @@
> /* SPDX-License-Identifier: GPL-2.0-only */
> 
> +#include <asm/psci.h>
> #include <asm/suspend.h>
> 
> +#include <public/sched.h>
> +#include <xen/console.h>
> +#include <xen/cpu.h>
> +#include <xen/errno.h>
> +#include <xen/iommu.h>
> +#include <xen/sched.h>
> +#include <xen/tasklet.h>
> +
> struct cpu_context cpu_context = {};
> 
> +static int can_system_suspend(void)
> +{
> +    int ret = 0;
> +    struct domain *d;
> +
> +    rcu_read_lock(&domlist_read_lock);
> +
> +    for_each_domain ( d )
> +    {
> +        bool domain_suspended;
> +
> +        spin_lock(&d->shutdown_lock);
> +        domain_suspended = d->is_shut_down &&
> +                           d->shutdown_code == SHUTDOWN_suspend;
> +        spin_unlock(&d->shutdown_lock);
> +
> +        if ( domain_suspended )
> +            continue;
> +
> +        printk(XENLOG_ERR
> +               "System suspend requires all domains to be shut down for 
> suspend (dom%d: isn't in suspend state)\n",

d->domain_id is unsigned if I’m not mistaken, it wants %u (typedef uint16_t 
domid_t;)

> +               d->domain_id);
> +
> +        ret = -EBUSY;
> +        break;
> +    }
> +
> +    rcu_read_unlock(&domlist_read_lock);
> +
> +    return ret;
> +}
> +
> +/* 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();
> +
> +    status = can_system_suspend();
> +    if ( status )
> +    {
> +        system_state = SYS_STATE_resume;
> +        goto resume_scheduler;

When we have an error and we get the resume_scheduler path, we apply back the
context of the guest saved previously in do_psci_1_0_system_suspend(), so am I
correct saying the guest won’t get any PSCI error back and we resume the guest
from the guest resume entrypoint?

In case, should we have a different path that returns a PSCI error (PSCI_*) 
into the guest
x0, and skips the context restore?

> +    }
> +
> +    /*
> +     * 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;
> +    }
> +
> +    time_suspend();
> +
> +    status = iommu_suspend();
> +    if ( status )
> +    {
> +        system_state = SYS_STATE_resume;
> +        goto resume_time;
> +    }
> +
> +    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_end_sync;
> +    }
> +
> +    local_irq_save(flags);
> +    status = gic_suspend();
> +    if ( status )
> +    {
> +        system_state = SYS_STATE_resume;
> +        goto resume_irqs;
> +    }
> +
> +    set_init_ttbr(xen_pgtable);
> +
> +    /*
> +     * Enable identity mapping before entering suspend to simplify
> +     * the resume path
> +     */
> +    update_boot_mapping(true);
> +
> +    if ( prepare_resume_ctx(&cpu_context) )
> +    {
> +        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;
> +    update_boot_mapping(false);
> +
> +    gic_resume();
> +
> + resume_irqs:
> +    local_irq_restore(flags);
> +
> +    console_resume();
> + resume_end_sync:
> +    console_end_sync();
> +
> +    iommu_resume();
> +
> + resume_time:
> +    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();
> +
> + resume_scheduler:
> +    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 bd87ec430d..8fb9172186 100644
> --- a/xen/arch/arm/vpsci.c
> +++ b/xen/arch/arm/vpsci.c
> @@ -5,6 +5,7 @@
> 
> #include <asm/current.h>
> #include <asm/domain.h>
> +#include <asm/suspend.h>
> #include <asm/vgic.h>
> #include <asm/vpsci.h>
> #include <asm/event.h>
> @@ -232,8 +233,7 @@ static int32_t do_psci_1_0_system_suspend(register_t 
> epoint, register_t cid)
>     if ( is_64bit_domain(d) && is_thumb )
>         return PSCI_INVALID_ADDRESS;
> 
> -    /* SYSTEM_SUSPEND is not supported for the hardware domain yet */
> -    if ( is_hardware_domain(d) )
> +    if ( !IS_ENABLED(CONFIG_SYSTEM_SUSPEND) && is_hardware_domain(d) )
>         return PSCI_NOT_SUPPORTED;
> 
>     /* Ensure that all CPUs other than the calling one are offline */
> @@ -266,6 +266,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 ( is_control_domain(d) )

Why is_control_domain() here and not is_hardware_domain() ?

> +        host_system_suspend(d);
> +
>     return rc;
> }
> 
> @@ -290,7 +293,10 @@ static int32_t do_psci_1_0_features(uint32_t 
> psci_func_id)
>         return 0;
>     case PSCI_1_0_FN32_SYSTEM_SUSPEND:
>     case PSCI_1_0_FN64_SYSTEM_SUSPEND:
> -        return is_hardware_domain(current->domain) ? PSCI_NOT_SUPPORTED : 0;
> +        if ( IS_ENABLED(CONFIG_SYSTEM_SUSPEND) ||
> +             !is_hardware_domain(current->domain) )

Should this have also the condition that “is hardware domain and psci_ver >= 
PSCI_VERSION(1, 0)”?
Otherwise if the host machine doestn’t support PSCI 1.0 we would return OK here 
but the call would
fail later in call_psci_system_suspend()? 

> +            return 0;
> +        fallthrough;
>     default:
>         return PSCI_NOT_SUPPORTED;
>     }
> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> index 0a20aa0a12..feb1336f46 100644
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -137,6 +137,9 @@ config HAS_EX_TABLE
> config HAS_FAST_MULTIPLY
> bool
> 
> +config HAS_HWDOM_SYSTEM_SUSPEND
> + bool
> +
> config HAS_IOPORTS
> bool
> 
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index bb9e210c28..d3edfb2a13 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -1375,6 +1375,11 @@ void __domain_crash(struct domain *d)
>     domain_shutdown(d, SHUTDOWN_crash);
> }
> 
> +static inline bool want_hwdom_shutdown(uint8_t reason)
> +{
> +    return !IS_ENABLED(CONFIG_HAS_HWDOM_SYSTEM_SUSPEND) ||
> +           reason != SHUTDOWN_suspend;
> +}
> 
> int domain_shutdown(struct domain *d, u8 reason)
> {
> @@ -1391,7 +1396,7 @@ int domain_shutdown(struct domain *d, u8 reason)
>         d->shutdown_code = reason;
>     reason = d->shutdown_code;
> 
> -    if ( is_hardware_domain(d) )
> +    if ( is_hardware_domain(d) && want_hwdom_shutdown(reason) )
>         hwdom_shutdown(reason);
> 
>     if ( d->is_shutting_down )
> diff --git a/xen/drivers/passthrough/arm/smmu.c 
> b/xen/drivers/passthrough/arm/smmu.c
> index 22d306d0cb..45f29ef8ec 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -2947,6 +2947,13 @@ static void arm_smmu_iommu_domain_teardown(struct 
> domain *d)
> xfree(xen_domain);
> }
> 
> +#ifdef CONFIG_SYSTEM_SUSPEND
> +static int arm_smmu_suspend(void)
> +{
> + return -ENOSYS;
> +}
> +#endif

Maybe we want to gate the feature also to !CONFIG_ARM_SMMU ? I would wait for 
the maintainers
view on this.

Cheers,
Luca