Re: [PATCH v8 1/4] xen/arm: Implement PSCI SYSTEM_SUSPEND call for guests

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.08.2025 21:39, Mykola Kvach wrote:
> From: Mykola Kvach <mykola_kvach@xxxxxxxx>
> 
> 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_kvach@xxxxxxxx>

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