Hi Jan,
On Fri, Aug 8, 2025 at 9:33 AM Jan Beulich <jbeul...@suse.com> wrote:
>
> 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.
Thank you for reviewing this patch series.
All your comments have been addressed in the next version of this patch
series, V9.
Best regards,
Mykola
>
> Jan
