On 07.08.2025 21:39, Mykola Kvach wrote:
> From: Mykola Kvach <mykola_kv...@epam.com>
>
> This patch adds support for the PSCI SYSTEM_SUSPEND function in the vPSCI
> (virtual PSCI) interface, allowing guests to request suspend via the PSCI
> v1.0 SYSTEM_SUSPEND call (both 32-bit and 64-bit variants).
>
> The implementation:
> - Adds SYSTEM_SUSPEND function IDs to PSCI definitions
> - Implements trapping and handling of SYSTEM_SUSPEND in vPSCI
> - Allows only non-hardware domains to invoke SYSTEM_SUSPEND; for the
> hardware domain, PSCI_NOT_SUPPORTED is returned to avoid halting the
> system in hwdom_shutdown() called from domain_shutdown
> - Ensures all secondary VCPUs of the calling domain are offline before
> allowing suspend due to PSCI spec
>
> GIC and virtual timer context must be saved when the domain suspends.
> This is done by moving the respective code in ctxt_switch_from
> before the return that happens if the domain suspended.
>
> Usage:
>
> For Linux-based guests, suspend can be initiated with:
> echo mem > /sys/power/state
> or via:
> systemctl suspend
>
> Resuming the guest is performed from control domain using:
> xl resume <domain>
>
> Signed-off-by: Mykola Kvach <mykola_kv...@epam.com>
Nothing is being said on why domain_resume_nopause() would be needed. While this
may be entirely obvious to Arm people, the change is done in common code.
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -1343,16 +1343,13 @@ int domain_shutdown(struct domain *d, u8 reason)
> return 0;
> }
>
> -void domain_resume(struct domain *d)
> +#ifndef CONFIG_ARM
> +static
> +#endif
> +void domain_resume_nopause(struct domain *d)
> {
> struct vcpu *v;
>
> - /*
> - * Some code paths assume that shutdown status does not get reset under
> - * their feet (e.g., some assertions make this assumption).
> - */
> - domain_pause(d);
> -
> spin_lock(&d->shutdown_lock);
>
> d->is_shutting_down = d->is_shut_down = 0;
> @@ -1360,13 +1357,24 @@ void domain_resume(struct domain *d)
>
> for_each_vcpu ( d, v )
> {
> + clear_bit(_VPF_suspended, &v->pause_flags);
Similarly it's not becoming clear why unconditionally doing this here would
be correct (now and going forward). There are other calls to this function,
after all.
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -814,6 +814,9 @@ void domain_destroy(struct domain *d);
> int domain_kill(struct domain *d);
> int domain_shutdown(struct domain *d, u8 reason);
> void domain_resume(struct domain *d);
> +#ifdef CONFIG_ARM
> +void domain_resume_nopause(struct domain *d);
> +#endif
>
> int domain_soft_reset(struct domain *d, bool resuming);
>
> @@ -1010,6 +1013,9 @@ static inline struct domain *next_domain_in_cpupool(
> /* VCPU is parked. */
> #define _VPF_parked 8
> #define VPF_parked (1UL<<_VPF_parked)
> +/* VCPU is suspended. */
> +#define _VPF_suspended 9
> +#define VPF_suspended (1UL<<_VPF_suspended)
And then, even if it's "only" style: With how adjacent code is formatted,
how come there's no suitable blank padding here? If anything wants (and
really needs) doing differently from pre-existing code, then to have
blanks around the << as well.
Jan
