On Wed, Nov 19, 2025 at 1:07 PM Orzel, Michal <[email protected]> wrote: > > > > On 19/11/2025 12:00, Mykola Kvach wrote: > > Hi Michal, > > > > Thanks for your review. > > > > On Wed, Nov 19, 2025 at 10:48 AM Orzel, Michal <[email protected]> wrote: > >> > >> > >> > >> On 18/11/2025 16:37, Mykola Kvach wrote: > >>> From: Mykola Kvach <[email protected]> > >>> > >>> Add support for the PSCI SYSTEM_SUSPEND function in the vPSCI interface, > >>> allowing guests to request suspend via the PSCI v1.0+ SYSTEM_SUSPEND call > >>> (both 32-bit and 64-bit variants). > >>> > >>> Implementation details: > >>> - Add SYSTEM_SUSPEND function IDs to PSCI definitions > >>> - Trap and handle SYSTEM_SUSPEND in vPSCI > >>> - Allow only non-hardware domains to invoke SYSTEM_SUSPEND; return > >>> PSCI_NOT_SUPPORTED for the hardware domain to avoid halting the system > >>> in hwdom_shutdown() via domain_shutdown > >>> - Require all secondary VCPUs of the calling domain to be offline before > >>> suspend, as mandated by the PSCI specification > >>> > >>> The arch_domain_resume() function is an architecture-specific hook that is > >>> invoked during domain resume to perform any necessary setup or restoration > >>> steps required by the platform. > >>> > >>> The new vpsci_vcpu_up_prepare() helper is called on the resume path to > >>> set up > >>> the vCPU context (such as entry point, some system regs and context ID) > >>> before > >>> resuming a suspended guest. This keeps ARM/vPSCI-specific logic out of > >>> common > >>> code and avoids intrusive changes to the generic resume flow. > >>> > >>> 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 <[email protected]> > >>> --- > >>> Changes in V14: > >>> - move arch_domain_resume to a separate header > >>> - avoid usage of typeof for context struct > >>> - moved error printing from domain_resume to arch_domain_resume > >>> in order to simplify common parts of code > >>> - minor changes after review > >>> --- > >>> xen/arch/arm/domain.c | 41 +++++++++ > >>> xen/arch/arm/include/asm/domain.h | 2 + > >>> xen/arch/arm/include/asm/perfc_defn.h | 1 + > >>> xen/arch/arm/include/asm/psci.h | 2 + > >>> xen/arch/arm/include/asm/suspend.h | 24 ++++++ > >>> xen/arch/arm/include/asm/vpsci.h | 5 +- > >>> xen/arch/arm/vpsci.c | 116 +++++++++++++++++++++----- > >>> xen/common/domain.c | 5 ++ > >>> xen/include/xen/suspend.h | 15 ++++ > >>> 9 files changed, 189 insertions(+), 22 deletions(-) > >>> create mode 100644 xen/arch/arm/include/asm/suspend.h > >>> create mode 100644 xen/include/xen/suspend.h > >>> > >>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c > >>> index e36719bce4..3c0170480b 100644 > >>> --- a/xen/arch/arm/domain.c > >>> +++ b/xen/arch/arm/domain.c > >>> @@ -12,6 +12,8 @@ > >>> #include <xen/softirq.h> > >>> #include <xen/wait.h> > >>> > >>> +#include <public/sched.h> > >>> + > >>> #include <asm/arm64/sve.h> > >>> #include <asm/cpuerrata.h> > >>> #include <asm/cpufeature.h> > >>> @@ -24,10 +26,12 @@ > >>> #include <asm/platform.h> > >>> #include <asm/procinfo.h> > >>> #include <asm/regs.h> > >>> +#include <asm/suspend.h> > >>> #include <asm/firmware/sci.h> > >>> #include <asm/tee/tee.h> > >>> #include <asm/vfp.h> > >>> #include <asm/vgic.h> > >>> +#include <asm/vpsci.h> > >>> #include <asm/vtimer.h> > >>> > >>> #include "vpci.h" > >>> @@ -885,6 +889,43 @@ void arch_domain_creation_finished(struct domain *d) > >>> p2m_domain_creation_finished(d); > >>> } > >>> > >>> +int arch_domain_resume(struct domain *d) > >>> +{ > >>> + int rc; > >>> + struct resume_info *ctx = &d->arch.resume_ctx; > >>> + > >>> + if ( !d->is_shutting_down || d->shutdown_code != SHUTDOWN_suspend ) > >>> + { > >>> + dprintk(XENLOG_WARNING, > >>> + "%pd: Invalid domain state for resume: > >>> is_shutting_down=%d, shutdown_code=%d\n", > >> These are bool and uint, so %u should be used. > > > > ack > > > >> > >>> + d, d->is_shutting_down, d->shutdown_code); > >>> + return -EINVAL; > >>> + } > >>> + > >>> + /* > >>> + * It is still possible to call domain_shutdown() with a suspend > >>> reason > >>> + * via some hypercalls, such as SCHEDOP_shutdown or > >>> SCHEDOP_remote_shutdown. > >>> + * In these cases, the resume context will be empty. > >>> + * This is not expected to cause any issues, so we just warn about > >>> the > >>> + * situation and return without error, allowing the existing logic to > >>> + * proceed as expected. > >>> + */ > >>> + if ( !ctx->wake_cpu ) > >>> + { > >>> + dprintk(XENLOG_INFO, "%pd: Wake CPU pointer context was not > >>> provided\n", > >>> + d); > >>> + return 0; > >>> + } > >>> + > >>> + rc = vpsci_vcpu_up_prepare(ctx->wake_cpu , ctx->ep, ctx->cid); > >>> + if ( rc ) > >>> + printk("%pd: Failed to resume domain (ret %d)\n", d, rc); > >>> + > >>> + memset(ctx, 0, sizeof(*ctx)); > >>> + > >>> + return rc; > >>> +} > >>> + > >>> static int is_guest_pv32_psr(uint32_t psr) > >>> { > >>> switch (psr & PSR_MODE_MASK) > >>> diff --git a/xen/arch/arm/include/asm/domain.h > >>> b/xen/arch/arm/include/asm/domain.h > >>> index af3e168374..e637cb4de0 100644 > >>> --- a/xen/arch/arm/include/asm/domain.h > >>> +++ b/xen/arch/arm/include/asm/domain.h > >>> @@ -5,6 +5,7 @@ > >>> #include <xen/timer.h> > >>> #include <asm/page.h> > >>> #include <asm/p2m.h> > >>> +#include <asm/suspend.h> > >>> #include <asm/vfp.h> > >>> #include <asm/mmio.h> > >>> #include <asm/gic.h> > >>> @@ -126,6 +127,7 @@ struct arch_domain > >>> void *sci_data; > >>> #endif > >>> > >>> + struct resume_info resume_ctx; > >>> } __cacheline_aligned; > >>> > >>> struct arch_vcpu > >>> diff --git a/xen/arch/arm/include/asm/perfc_defn.h > >>> b/xen/arch/arm/include/asm/perfc_defn.h > >>> index effd25b69e..8dfcac7e3b 100644 > >>> --- a/xen/arch/arm/include/asm/perfc_defn.h > >>> +++ b/xen/arch/arm/include/asm/perfc_defn.h > >>> @@ -33,6 +33,7 @@ PERFCOUNTER(vpsci_system_reset, "vpsci: > >>> system_reset") > >>> PERFCOUNTER(vpsci_cpu_suspend, "vpsci: cpu_suspend") > >>> PERFCOUNTER(vpsci_cpu_affinity_info, "vpsci: cpu_affinity_info") > >>> PERFCOUNTER(vpsci_features, "vpsci: features") > >>> +PERFCOUNTER(vpsci_system_suspend, "vpsci: system_suspend") > >>> > >>> PERFCOUNTER(vcpu_kick, "vcpu: notify other vcpu") > >>> > >>> diff --git a/xen/arch/arm/include/asm/psci.h > >>> b/xen/arch/arm/include/asm/psci.h > >>> index 4780972621..48a93e6b79 100644 > >>> --- a/xen/arch/arm/include/asm/psci.h > >>> +++ b/xen/arch/arm/include/asm/psci.h > >>> @@ -47,10 +47,12 @@ void call_psci_system_reset(void); > >>> #define PSCI_0_2_FN32_SYSTEM_OFF PSCI_0_2_FN32(8) > >>> #define PSCI_0_2_FN32_SYSTEM_RESET PSCI_0_2_FN32(9) > >>> #define PSCI_1_0_FN32_PSCI_FEATURES PSCI_0_2_FN32(10) > >>> +#define PSCI_1_0_FN32_SYSTEM_SUSPEND PSCI_0_2_FN32(14) > >>> > >>> #define PSCI_0_2_FN64_CPU_SUSPEND PSCI_0_2_FN64(1) > >>> #define PSCI_0_2_FN64_CPU_ON PSCI_0_2_FN64(3) > >>> #define PSCI_0_2_FN64_AFFINITY_INFO PSCI_0_2_FN64(4) > >>> +#define PSCI_1_0_FN64_SYSTEM_SUSPEND PSCI_0_2_FN64(14) > >>> > >>> /* PSCI v0.2 affinity level state returned by AFFINITY_INFO */ > >>> #define PSCI_0_2_AFFINITY_LEVEL_ON 0 > >>> diff --git a/xen/arch/arm/include/asm/suspend.h > >>> b/xen/arch/arm/include/asm/suspend.h > >>> new file mode 100644 > >>> index 0000000000..b991a94d5a > >>> --- /dev/null > >>> +++ b/xen/arch/arm/include/asm/suspend.h > >>> @@ -0,0 +1,24 @@ > >>> +/* SPDX-License-Identifier: GPL-2.0-only */ > >>> + > >>> +#ifndef __ARM_SUSPEND_H__ > >> There should be no double underscores in guards (see examples in > >> CODING_STYLE) > > > > I had initially followed the style used by some of the existing headers > > in this directory, which still use guards with double underscores. > > > > I'll remove underscores in the next series. > > > >>> +#define __ARM_SUSPEND_H__ > >>> + > >>> +struct resume_info { > >>> + register_t ep; > >>> + register_t cid; > >>> + struct vcpu *wake_cpu; > >>> +}; > >>> + > >>> +int arch_domain_resume(struct domain *d); > >> If possible, headers should be standalone, meaning you should include > >> necessary > >> headers or forward declare what's missing. > > > > Thanks for the comment. > > > > My initial intention was to avoid adding new dependencies from asm > > headers to higher-level Xen headers, so I did not include e.g. > > <xen/sched.h> directly here. In this header we only need pointers to > > struct domain and struct vcpu, we never dereference them, so forward > > declarations would be sufficient to make it standalone. > > > > I also noticed that some existing asm headers in this directory do > > include higher-level Xen headers, so both patterns exist in the tree. > > > > If you prefer, I can either: > > - add forward declarations > > > > struct domain; > > struct vcpu; > > > > at the top of this header and keep the full includes in the .c > > files that actually dereference these types, or > > > > - include the appropriate Xen header(s) directly in this header. > > > > Personally I slightly prefer the forward-declaration approach to keep > > this header lightweight and avoid tightening the layering, but I am > > happy to follow your preference. > My preference is also to forward declare these structs. > > > > >> > >>> + > >>> +#endif /* __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/include/asm/vpsci.h > >>> b/xen/arch/arm/include/asm/vpsci.h > >>> index 0cca5e6830..d790ab3715 100644 > >>> --- a/xen/arch/arm/include/asm/vpsci.h > >>> +++ b/xen/arch/arm/include/asm/vpsci.h > >>> @@ -23,12 +23,15 @@ > >>> #include <asm/psci.h> > >>> > >>> /* Number of function implemented by virtual PSCI (only 0.2 or later) */ > >>> -#define VPSCI_NR_FUNCS 12 > >>> +#define VPSCI_NR_FUNCS 14 > >>> > >>> /* Functions handle PSCI calls from the guests */ > >>> bool do_vpsci_0_1_call(struct cpu_user_regs *regs, uint32_t fid); > >>> bool do_vpsci_0_2_call(struct cpu_user_regs *regs, uint32_t fid); > >>> > >>> +int vpsci_vcpu_up_prepare(struct vcpu *v, register_t entry_point, > >>> + register_t context_id); > >>> + > >>> #endif /* __ASM_VPSCI_H__ */ > >>> > >>> /* > >>> diff --git a/xen/arch/arm/vpsci.c b/xen/arch/arm/vpsci.c > >>> index 7ba9ccd94b..22c3a5f544 100644 > >>> --- a/xen/arch/arm/vpsci.c > >>> +++ b/xen/arch/arm/vpsci.c > >>> @@ -10,32 +10,16 @@ > >>> > >>> #include <public/sched.h> > >>> > >>> -static int do_common_cpu_on(register_t target_cpu, register_t > >>> entry_point, > >>> - register_t context_id) > >>> +int vpsci_vcpu_up_prepare(struct vcpu *v, register_t entry_point, > >>> + register_t context_id) > >> NIT: incorrect parameter alignment > > > > ack > > > >> > >>> { > >>> - struct vcpu *v; > >>> - struct domain *d = current->domain; > >>> - struct vcpu_guest_context *ctxt; > >>> int rc; > >>> + struct domain *d = v->domain; > >>> bool is_thumb = entry_point & 1; > >>> - register_t vcpuid; > >>> - > >>> - vcpuid = vaffinity_to_vcpuid(target_cpu); > >>> - > >>> - if ( (v = domain_vcpu(d, vcpuid)) == NULL ) > >>> - return PSCI_INVALID_PARAMETERS; > >>> - > >>> - /* THUMB set is not allowed with 64-bit domain */ > >>> - if ( is_64bit_domain(d) && is_thumb ) > >>> - return PSCI_INVALID_ADDRESS; > >>> - > >>> - if ( !test_bit(_VPF_down, &v->pause_flags) ) > >>> - return PSCI_ALREADY_ON; > >>> + struct vcpu_guest_context *ctxt; > >>> > >>> if ( (ctxt = alloc_vcpu_guest_context()) == NULL ) > >>> - return PSCI_DENIED; > >>> - > >>> - vgic_clear_pending_irqs(v); > >>> + return -ENOMEM; > >>> > >>> memset(ctxt, 0, sizeof(*ctxt)); > >>> ctxt->user_regs.pc64 = (u64) entry_point; > >>> @@ -76,8 +60,37 @@ static int do_common_cpu_on(register_t target_cpu, > >>> register_t entry_point, > >>> free_vcpu_guest_context(ctxt); > >>> > >>> if ( rc < 0 ) > >>> + return rc; > >>> + > >>> + return 0; > >>> +} > >>> + > >>> +static int do_common_cpu_on(register_t target_cpu, register_t > >>> entry_point, > >>> + register_t context_id) > >>> +{ > >>> + struct vcpu *v; > >>> + struct domain *d = current->domain; > >>> + int rc; > >>> + bool is_thumb = entry_point & 1; > >>> + register_t vcpuid; > >>> + > >>> + vcpuid = vaffinity_to_vcpuid(target_cpu); > >>> + > >>> + if ( (v = domain_vcpu(d, vcpuid)) == NULL ) > >>> + return PSCI_INVALID_PARAMETERS; > >>> + > >>> + /* THUMB set is not allowed with 64-bit domain */ > >>> + if ( is_64bit_domain(d) && is_thumb ) > >>> + return PSCI_INVALID_ADDRESS; > >>> + > >>> + if ( !test_bit(_VPF_down, &v->pause_flags) ) > >>> + return PSCI_ALREADY_ON; > >>> + > >>> + rc = vpsci_vcpu_up_prepare(v, entry_point, context_id); > >>> + if ( rc ) > >>> return PSCI_DENIED; > >>> > >>> + vgic_clear_pending_irqs(v); > >>> vcpu_wake(v); > >>> > >>> return PSCI_SUCCESS; > >>> @@ -197,6 +210,48 @@ static void do_psci_0_2_system_reset(void) > >>> domain_shutdown(d,SHUTDOWN_reboot); > >>> } > >>> > >>> +static int32_t do_psci_1_0_system_suspend(register_t epoint, register_t > >>> cid) > >>> +{ > >>> + int32_t rc; > >>> + struct vcpu *v; > >>> + struct domain *d = current->domain; > >>> + bool is_thumb = epoint & 1; > >>> + > >>> + /* THUMB set is not allowed with 64-bit domain */ > >>> + 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) ) > >>> + return PSCI_NOT_SUPPORTED; > >>> + > >>> + /* Ensure that all CPUs other than the calling one are offline */ > >>> + domain_lock(d); > >>> + for_each_vcpu ( d, v ) > >>> + { > >>> + if ( v != current && is_vcpu_online(v) ) > >>> + { > >>> + domain_unlock(d); > >>> + return PSCI_DENIED; > >>> + } > >>> + } > >>> + domain_unlock(d); > >>> + > >>> + rc = domain_shutdown(d, SHUTDOWN_suspend); > >>> + if ( rc ) > >>> + return PSCI_DENIED; > >>> + > >>> + d->arch.resume_ctx.ep = epoint; > >>> + d->arch.resume_ctx.cid = cid; > >>> + d->arch.resume_ctx.wake_cpu = current; > >>> + > >>> + gprintk(XENLOG_DEBUG, > >>> + "SYSTEM_SUSPEND requested, epoint=0x%"PRIregister", > >>> cid=0x%"PRIregister"\n", > >> NIT: %# is preffered over 0x%. > > > > ack > > > >> > >>> + epoint, cid); > >>> + > >>> + return rc; > >>> +} > >>> + > >>> static int32_t do_psci_1_0_features(uint32_t psci_func_id) > >>> { > >>> /* /!\ Ordered by function ID and not name */ > >>> @@ -214,6 +269,8 @@ static int32_t do_psci_1_0_features(uint32_t > >>> psci_func_id) > >>> case PSCI_0_2_FN32_SYSTEM_OFF: > >>> case PSCI_0_2_FN32_SYSTEM_RESET: > >>> case PSCI_1_0_FN32_PSCI_FEATURES: > >>> + case PSCI_1_0_FN32_SYSTEM_SUSPEND: > >>> + case PSCI_1_0_FN64_SYSTEM_SUSPEND: > >>> case ARM_SMCCC_VERSION_FID: > >>> return 0; > >>> default: > >>> @@ -344,6 +401,23 @@ bool do_vpsci_0_2_call(struct cpu_user_regs *regs, > >>> uint32_t fid) > >>> return true; > >>> } > >>> > >>> + case PSCI_1_0_FN32_SYSTEM_SUSPEND: > >>> + case PSCI_1_0_FN64_SYSTEM_SUSPEND: > >>> + { > >>> + register_t epoint = PSCI_ARG(regs, 1); > >>> + register_t cid = PSCI_ARG(regs, 2); > >>> + > >>> + if ( fid == PSCI_1_0_FN32_SYSTEM_SUSPEND ) > >>> + { > >>> + epoint &= GENMASK(31, 0); > >>> + cid &= GENMASK(31, 0); > >>> + } > >>> + > >>> + perfc_incr(vpsci_system_suspend); > >>> + PSCI_SET_RESULT(regs, do_psci_1_0_system_suspend(epoint, cid)); > >>> + return true; > >>> + } > >>> + > >>> default: > >>> return false; > >>> } > >>> diff --git a/xen/common/domain.c b/xen/common/domain.c > >>> index 775c339285..6410d32970 100644 > >>> --- a/xen/common/domain.c > >>> +++ b/xen/common/domain.c > >>> @@ -26,6 +26,7 @@ > >>> #include <xen/hypercall.h> > >>> #include <xen/delay.h> > >>> #include <xen/shutdown.h> > >>> +#include <xen/suspend.h> > >>> #include <xen/percpu.h> > >>> #include <xen/multicall.h> > >>> #include <xen/rcupdate.h> > >>> @@ -1363,6 +1364,9 @@ void domain_resume(struct domain *d) > >>> > >>> spin_lock(&d->shutdown_lock); > >>> > >>> + if ( arch_domain_resume(d) ) > >>> + goto fail; > >>> + > >>> d->is_shutting_down = d->is_shut_down = 0; > >>> d->shutdown_code = SHUTDOWN_CODE_INVALID; > >>> > >>> @@ -1373,6 +1377,7 @@ void domain_resume(struct domain *d) > >>> v->paused_for_shutdown = 0; > >>> } > >>> > >>> + fail: > >>> spin_unlock(&d->shutdown_lock); > >>> > >>> domain_unpause(d); > >>> diff --git a/xen/include/xen/suspend.h b/xen/include/xen/suspend.h > >>> new file mode 100644 > >>> index 0000000000..53f75fd6ad > >>> --- /dev/null > >>> +++ b/xen/include/xen/suspend.h > >>> @@ -0,0 +1,15 @@ > >>> +/* SPDX-License-Identifier: GPL-2.0-only */ > >>> +#ifndef __XEN_SUSPEND_H__ > >> There should be no double underscores in guards > > > > I initially followed the style used by the existing headers in this > > directory, which still have include guards with double underscores. > > > > You are right that this does not match CODING_STYLE examples. > > I will update this header to use a proper guard name. > > > >> > >>> +#define __XEN_SUSPEND_H__ > >>> + > >>> +#if __has_include(<asm/suspend.h>) > >>> +#include <asm/suspend.h> > >>> +#else > >>> +static inline int arch_domain_resume(struct domain *d) > >>> +{ > >>> + return 0; > >>> +} > >>> +#endif > >>> + > >> Stray whiteline? > > > > will drop > > > >> > >>> + > >>> +#endif /* __XEN_SUSPEND_H__ */ > >> Missing emacs block? > > > > It is permitted but isn't necessary as far as know, > > but if it needed here per your opinion I'll add it > > just let me know > On Arm we usually require them and that's the overall preference I would say. > > > > >> > >> Did you run MISRA scan on this patch? > > > > Yes, I ran it with: > > > > ./xen/scripts/xen-analysis.py \ > > --run-cppcheck --cppcheck-misra --cppcheck-html -- \ > > XEN_TARGET_ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- > > > > The analysis did not report any new MISRA violations in the code > > touched by this patch. > That's only cppcheck scan which is rather poor in finding violations. ECLAIR > scan is done through Gitlab CI and this one is what we rely on for taking the > series in.
Thanks for the clarification. Is it possible to run the ECLAIR MISRA scan locally, or is it only available via the project Gitlab CI instance? If there is a way to run it on a developer machine, I would be happy to set it up and check this series with the same configuration. > > ~Michal > Best regards, Mykola
