* [PATCH v14 0/4] Enable guest suspend/resume support on ARM via vPSCI
@ 2025-11-18 15:37 Mykola Kvach
2025-11-18 15:37 ` [PATCH v14 1/4] xen/arm: Implement PSCI SYSTEM_SUSPEND call for guests Mykola Kvach
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Mykola Kvach @ 2025-11-18 15:37 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk, Andrew Cooper, Anthony PERARD, Jan Beulich,
Roger Pau Monné, Juergen Gross, Oleksii Kurochko,
Community Manager
This patch series introduces the initial support for guest suspend
and resume on ARM platforms using the PSCI SYSTEM_SUSPEND interface. The main
goal is to allow ARM guests to request suspension using PSCI and be resumed
by the control domain (e.g., via "xl resume").
Background
The PSCI SYSTEM_SUSPEND call is part of the PSCI v1.0+ specification and is
used by guests to enter the deepest possible power state. On Xen/ARM, we
emulate this interface in the virtual PSCI (vPSCI) layer for guests.
This series includes:
1. A new vPSCI implementation of the PSCI SYSTEM_SUSPEND function for guests
2. Documentation updates to SUPPORT.md to reflect PSCI and vPSCI support status
3. Enabling "xl resume" command compilation for ARM, which was previously disabled
Usage
For Linux-based guests:
- Suspend can be triggered using: "echo mem > /sys/power/state" or "systemctl suspend"
- Resume can be performed from control domain using: "xl resume <domain>"
For more information, refer to the official Linux kernel documentation on power management.
Note that currently, SYSTEM_SUSPEND is supported only for guest domains (not for
the hardware domain).
---
This is the first part of previous patch series and originally consist only
with necessary changes needed for guest domain suspend.
The second part can be found here:
https://patchew.org/Xen/cover.1756763487.git.mykola._5Fkvach@epam.com/
---
Changes introduced in v14:
- move arch_domain_resume() to its own header, drop typeof usage for
the resume context, move resume error prints into arch_domain_resume(),
and address review nits
- move the suspend-to-RAM changelog entry to the 4.22 section
- commit message tweaks.
Mykola Kvach (4):
xen/arm: Implement PSCI SYSTEM_SUSPEND call for guests
tools/xl: Allow compilation of 'xl resume' command on Arm
SUPPORT.md: Document PSCI SYSTEM_SUSPEND support for guests
CHANGELOG: Document guest suspend/resume to RAM support on Arm
CHANGELOG.md | 4 +
SUPPORT.md | 5 +-
tools/include/libxl.h | 1 -
tools/xl/xl.h | 4 +-
tools/xl/xl_cmdtable.c | 4 +-
tools/xl/xl_migrate.c | 2 +-
tools/xl/xl_saverestore.c | 2 +-
tools/xl/xl_vmcontrol.c | 12 +--
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 ++++
17 files changed, 208 insertions(+), 37 deletions(-)
create mode 100644 xen/arch/arm/include/asm/suspend.h
create mode 100644 xen/include/xen/suspend.h
--
2.43.0
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v14 1/4] xen/arm: Implement PSCI SYSTEM_SUSPEND call for guests
2025-11-18 15:37 [PATCH v14 0/4] Enable guest suspend/resume support on ARM via vPSCI Mykola Kvach
@ 2025-11-18 15:37 ` Mykola Kvach
2025-11-19 8:47 ` Orzel, Michal
2025-11-18 15:37 ` [PATCH v14 2/4] tools/xl: Allow compilation of 'xl resume' command on Arm Mykola Kvach
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Mykola Kvach @ 2025-11-18 15:37 UTC (permalink / raw)
To: xen-devel
Cc: Mykola Kvach, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Michal Orzel, Volodymyr Babchuk, Andrew Cooper, Anthony PERARD,
Jan Beulich, Roger Pau Monné
From: Mykola Kvach <mykola_kvach@epam.com>
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 <mykola_kvach@epam.com>
---
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",
+ 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__
+#define __ARM_SUSPEND_H__
+
+struct resume_info {
+ register_t ep;
+ register_t cid;
+ struct vcpu *wake_cpu;
+};
+
+int arch_domain_resume(struct domain *d);
+
+#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)
{
- 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",
+ 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__
+#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
+
+
+#endif /* __XEN_SUSPEND_H__ */
--
2.43.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v14 2/4] tools/xl: Allow compilation of 'xl resume' command on Arm
2025-11-18 15:37 [PATCH v14 0/4] Enable guest suspend/resume support on ARM via vPSCI Mykola Kvach
2025-11-18 15:37 ` [PATCH v14 1/4] xen/arm: Implement PSCI SYSTEM_SUSPEND call for guests Mykola Kvach
@ 2025-11-18 15:37 ` Mykola Kvach
2025-11-18 15:37 ` [PATCH v14 3/4] SUPPORT.md: Document PSCI SYSTEM_SUSPEND support for guests Mykola Kvach
2025-11-18 15:37 ` [PATCH v14 4/4] CHANGELOG: Document guest suspend/resume to RAM support on Arm Mykola Kvach
3 siblings, 0 replies; 13+ messages in thread
From: Mykola Kvach @ 2025-11-18 15:37 UTC (permalink / raw)
To: xen-devel; +Cc: Mykola Kvach, Anthony PERARD, Juergen Gross
From: Mykola Kvach <mykola_kvach@epam.com>
The "xl resume" command was previously excluded from Arm builds because
system suspend/resume (e.g., SYSTEM_SUSPEND via vPSCI) was not
implemented. On x86, this command is used for resume.
This change enables compilation of `xl resume` on Arm regardless of the
underlying implementation status, making the tool available for testing
and future feature support. The relevant libxl infrastructure and handler
functions are already present and usable.
Note: This does not imply full system suspend/resume support on Arm.
The `xl suspend` command still does not work on Arm platforms.
Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
Acked-by: Anthony PERARD <anthony.perard@vates.tech>
---
Changes in v14:
- no chnages
---
tools/include/libxl.h | 1 -
tools/xl/xl.h | 4 ++--
tools/xl/xl_cmdtable.c | 4 ++--
tools/xl/xl_migrate.c | 2 +-
tools/xl/xl_saverestore.c | 2 +-
tools/xl/xl_vmcontrol.c | 12 ++++++------
6 files changed, 12 insertions(+), 13 deletions(-)
diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index bc35e412da..14b9e4a859 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -1145,7 +1145,6 @@ typedef struct libxl__ctx libxl_ctx;
* restoring or migrating a domain. In this case the related functions
* should be expected to return failure. That is:
* - libxl_domain_suspend
- * - libxl_domain_resume
* - libxl_domain_remus_start
*/
#if defined(__arm__) || defined(__aarch64__)
diff --git a/tools/xl/xl.h b/tools/xl/xl.h
index 9000df00de..63db30a6eb 100644
--- a/tools/xl/xl.h
+++ b/tools/xl/xl.h
@@ -65,7 +65,7 @@ static const char migrate_permission_to_go[]=
"domain is yours, you are cleared to unpause";
static const char migrate_report[]=
"my copy unpause results are as follows";
-#endif
+#endif /* !LIBXL_HAVE_NO_SUSPEND_RESUME */
/* followed by one byte:
* 0: everything went well, domain is running
@@ -130,8 +130,8 @@ int main_migrate_receive(int argc, char **argv);
int main_save(int argc, char **argv);
int main_migrate(int argc, char **argv);
int main_suspend(int argc, char **argv);
+#endif /* !LIBXL_HAVE_NO_SUSPEND_RESUME */
int main_resume(int argc, char **argv);
-#endif
int main_dump_core(int argc, char **argv);
int main_pause(int argc, char **argv);
int main_unpause(int argc, char **argv);
diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
index 06a0039718..bcb2d233cc 100644
--- a/tools/xl/xl_cmdtable.c
+++ b/tools/xl/xl_cmdtable.c
@@ -198,12 +198,12 @@ const struct cmd_spec cmd_table[] = {
"Suspend a domain to RAM",
"<Domain>",
},
+#endif /* !LIBXL_HAVE_NO_SUSPEND_RESUME */
{ "resume",
&main_resume, 0, 1,
"Resume a domain from RAM",
"<Domain>",
},
-#endif
{ "dump-core",
&main_dump_core, 0, 1,
"Core dump a domain",
@@ -548,7 +548,7 @@ const struct cmd_spec cmd_table[] = {
" checkpoint must be disabled.\n"
"-p Use COLO userspace proxy."
},
-#endif
+#endif /* !LIBXL_HAVE_NO_SUSPEND_RESUME */
{ "devd",
&main_devd, 0, 1,
"Daemon that listens for devices and launches backends",
diff --git a/tools/xl/xl_migrate.c b/tools/xl/xl_migrate.c
index b8594f44a5..4b4a379aa1 100644
--- a/tools/xl/xl_migrate.c
+++ b/tools/xl/xl_migrate.c
@@ -767,7 +767,7 @@ int main_remus(int argc, char **argv)
close(send_fd);
return EXIT_FAILURE;
}
-#endif
+#endif /* !LIBXL_HAVE_NO_SUSPEND_RESUME */
/*
diff --git a/tools/xl/xl_saverestore.c b/tools/xl/xl_saverestore.c
index 953d791d1a..747094ec7b 100644
--- a/tools/xl/xl_saverestore.c
+++ b/tools/xl/xl_saverestore.c
@@ -270,7 +270,7 @@ int main_save(int argc, char **argv)
return EXIT_SUCCESS;
}
-#endif /* LIBXL_HAVE_NO_SUSPEND_RESUME */
+#endif /* !LIBXL_HAVE_NO_SUSPEND_RESUME */
diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
index c813732838..93766f631b 100644
--- a/tools/xl/xl_vmcontrol.c
+++ b/tools/xl/xl_vmcontrol.c
@@ -38,11 +38,6 @@ static void suspend_domain(uint32_t domid)
libxl_domain_suspend_only(ctx, domid, NULL);
}
-static void resume_domain(uint32_t domid)
-{
- libxl_domain_resume(ctx, domid, 1, NULL);
-}
-
int main_suspend(int argc, char **argv)
{
int opt;
@@ -55,6 +50,12 @@ int main_suspend(int argc, char **argv)
return EXIT_SUCCESS;
}
+#endif /* !LIBXL_HAVE_NO_SUSPEND_RESUME */
+
+static void resume_domain(uint32_t domid)
+{
+ libxl_domain_resume(ctx, domid, 1, NULL);
+}
int main_resume(int argc, char **argv)
{
@@ -68,7 +69,6 @@ int main_resume(int argc, char **argv)
return EXIT_SUCCESS;
}
-#endif
static void pause_domain(uint32_t domid)
{
--
2.43.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v14 3/4] SUPPORT.md: Document PSCI SYSTEM_SUSPEND support for guests
2025-11-18 15:37 [PATCH v14 0/4] Enable guest suspend/resume support on ARM via vPSCI Mykola Kvach
2025-11-18 15:37 ` [PATCH v14 1/4] xen/arm: Implement PSCI SYSTEM_SUSPEND call for guests Mykola Kvach
2025-11-18 15:37 ` [PATCH v14 2/4] tools/xl: Allow compilation of 'xl resume' command on Arm Mykola Kvach
@ 2025-11-18 15:37 ` Mykola Kvach
2025-11-18 15:37 ` [PATCH v14 4/4] CHANGELOG: Document guest suspend/resume to RAM support on Arm Mykola Kvach
3 siblings, 0 replies; 13+ messages in thread
From: Mykola Kvach @ 2025-11-18 15:37 UTC (permalink / raw)
To: xen-devel
Cc: Mykola Kvach, Andrew Cooper, Anthony PERARD, Michal Orzel,
Jan Beulich, Julien Grall, Roger Pau Monné,
Stefano Stabellini, Julien Grall
From: Mykola Kvach <mykola_kvach@epam.com>
Add a new entry under the "Virtual Hardware, Hypervisor" section
documenting support for the optional PSCI SYSTEM_SUSPEND function
exposed to guests.
This function is available via the virtual PSCI interface and allows
guest domains (domUs) to initiate system suspend operations.
The feature is currently marked as "Tech Preview".
Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
Acked-by: Julien Grall <jgrall@amazon.com>
---
Changes in V14:
- fixes in commit message.
---
SUPPORT.md | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/SUPPORT.md b/SUPPORT.md
index d441bccf37..8e7ab7cb3e 100644
--- a/SUPPORT.md
+++ b/SUPPORT.md
@@ -962,8 +962,9 @@ Emulated PSCI interface exposed to guests. We support all mandatory
functions of PSCI 1.1. See below for the list of optional PSCI call
implemented and their status.
- Status, Mandatory: Supported
- Status, MIGRATE_INFO_TYPE: Supported
+ Status, Mandatory: Supported
+ Status, MIGRATE_INFO_TYPE: Supported
+ Status, SYSTEM_SUSPEND: Tech Preview
## Virtual Hardware, QEMU
--
2.43.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v14 4/4] CHANGELOG: Document guest suspend/resume to RAM support on Arm
2025-11-18 15:37 [PATCH v14 0/4] Enable guest suspend/resume support on ARM via vPSCI Mykola Kvach
` (2 preceding siblings ...)
2025-11-18 15:37 ` [PATCH v14 3/4] SUPPORT.md: Document PSCI SYSTEM_SUSPEND support for guests Mykola Kvach
@ 2025-11-18 15:37 ` Mykola Kvach
3 siblings, 0 replies; 13+ messages in thread
From: Mykola Kvach @ 2025-11-18 15:37 UTC (permalink / raw)
To: xen-devel; +Cc: Mykola Kvach, Oleksii Kurochko, Community Manager
From: Mykola Kvach <mykola_kvach@epam.com>
Mention the newly added support for guest suspend and resume to/from
RAM via vPSCI on Arm platforms.
This support is limited to non-hardware domain guests.
Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
---
Chnages in v14:
- moved s2ram changelog to 4.22 release section
---
CHANGELOG.md | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/CHANGELOG.md b/CHANGELOG.md
index c9932a2af0..fd4657ea42 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -10,6 +10,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
### Added
+ - On Arm:
+ - Support for guest suspend and resume to/from RAM via vPSCI.
+ Applies only to non-hardware domain guests.
+
### Removed
## [4.21.0 UNRELEASED](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=shortlog;h=staging) - TBD
--
2.43.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v14 1/4] xen/arm: Implement PSCI SYSTEM_SUSPEND call for guests
2025-11-18 15:37 ` [PATCH v14 1/4] xen/arm: Implement PSCI SYSTEM_SUSPEND call for guests Mykola Kvach
@ 2025-11-19 8:47 ` Orzel, Michal
2025-11-19 11:00 ` Mykola Kvach
2025-11-19 12:47 ` Mykola Kvach
0 siblings, 2 replies; 13+ messages in thread
From: Orzel, Michal @ 2025-11-19 8:47 UTC (permalink / raw)
To: Mykola Kvach, xen-devel
Cc: Mykola Kvach, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Andrew Cooper, Anthony PERARD, Jan Beulich,
Roger Pau Monné
On 18/11/2025 16:37, Mykola Kvach wrote:
> From: Mykola Kvach <mykola_kvach@epam.com>
>
> 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 <mykola_kvach@epam.com>
> ---
> 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.
> + 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)
> +#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.
> +
> +#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
> {
> - 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%.
> + 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
> +#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?
> +
> +#endif /* __XEN_SUSPEND_H__ */
Missing emacs block?
Did you run MISRA scan on this patch?
~Michal
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v14 1/4] xen/arm: Implement PSCI SYSTEM_SUSPEND call for guests
2025-11-19 8:47 ` Orzel, Michal
@ 2025-11-19 11:00 ` Mykola Kvach
2025-11-19 11:07 ` Nicola Vetrini
2025-11-19 11:07 ` Orzel, Michal
2025-11-19 12:47 ` Mykola Kvach
1 sibling, 2 replies; 13+ messages in thread
From: Mykola Kvach @ 2025-11-19 11:00 UTC (permalink / raw)
To: Orzel, Michal
Cc: xen-devel, Mykola Kvach, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk, Andrew Cooper,
Anthony PERARD, Jan Beulich, Roger Pau Monné
Hi Michal,
Thanks for your review.
On Wed, Nov 19, 2025 at 10:48 AM Orzel, Michal <michal.orzel@amd.com> wrote:
>
>
>
> On 18/11/2025 16:37, Mykola Kvach wrote:
> > From: Mykola Kvach <mykola_kvach@epam.com>
> >
> > 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 <mykola_kvach@epam.com>
> > ---
> > 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.
>
> > +
> > +#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
>
> 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.
>
> ~Michal
>
>
Best regards,
Mykola
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v14 1/4] xen/arm: Implement PSCI SYSTEM_SUSPEND call for guests
2025-11-19 11:00 ` Mykola Kvach
@ 2025-11-19 11:07 ` Nicola Vetrini
2025-11-19 11:07 ` Orzel, Michal
1 sibling, 0 replies; 13+ messages in thread
From: Nicola Vetrini @ 2025-11-19 11:07 UTC (permalink / raw)
To: Mykola Kvach
Cc: Orzel, Michal, xen-devel, Mykola Kvach, Stefano Stabellini,
Julien Grall, Bertrand Marquis, Volodymyr Babchuk, Andrew Cooper,
Anthony PERARD, Jan Beulich, Roger Pau Monné
On 2025-11-19 12:00, Mykola Kvach wrote:
> Hi Michal,
>
> Thanks for your review.
>
> On Wed, Nov 19, 2025 at 10:48 AM Orzel, Michal <michal.orzel@amd.com>
> wrote:
>>
>>
>>
>> On 18/11/2025 16:37, Mykola Kvach wrote:
>> > From: Mykola Kvach <mykola_kvach@epam.com>
>> >
>> > 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 <mykola_kvach@epam.com>
>> > ---
>> > 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.
>
>>
>> > +
>> > +#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
>
>>
>> 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.
>
This just runs cppcheck, which has many false negatives, but the
upstream analysis should be done using ECLAIR with GitLab pipelines
>>
>> ~Michal
>>
>>
>
> Best regards,
> Mykola
--
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v14 1/4] xen/arm: Implement PSCI SYSTEM_SUSPEND call for guests
2025-11-19 11:00 ` Mykola Kvach
2025-11-19 11:07 ` Nicola Vetrini
@ 2025-11-19 11:07 ` Orzel, Michal
2025-11-19 11:32 ` Mykola Kvach
1 sibling, 1 reply; 13+ messages in thread
From: Orzel, Michal @ 2025-11-19 11:07 UTC (permalink / raw)
To: Mykola Kvach
Cc: xen-devel, Mykola Kvach, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk, Andrew Cooper,
Anthony PERARD, Jan Beulich, Roger Pau Monné
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 <michal.orzel@amd.com> wrote:
>>
>>
>>
>> On 18/11/2025 16:37, Mykola Kvach wrote:
>>> From: Mykola Kvach <mykola_kvach@epam.com>
>>>
>>> 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 <mykola_kvach@epam.com>
>>> ---
>>> 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.
~Michal
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v14 1/4] xen/arm: Implement PSCI SYSTEM_SUSPEND call for guests
2025-11-19 11:07 ` Orzel, Michal
@ 2025-11-19 11:32 ` Mykola Kvach
2025-11-19 11:43 ` Orzel, Michal
0 siblings, 1 reply; 13+ messages in thread
From: Mykola Kvach @ 2025-11-19 11:32 UTC (permalink / raw)
To: Orzel, Michal
Cc: xen-devel, Mykola Kvach, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk, Andrew Cooper,
Anthony PERARD, Jan Beulich, Roger Pau Monné
On Wed, Nov 19, 2025 at 1:07 PM Orzel, Michal <michal.orzel@amd.com> 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 <michal.orzel@amd.com> wrote:
> >>
> >>
> >>
> >> On 18/11/2025 16:37, Mykola Kvach wrote:
> >>> From: Mykola Kvach <mykola_kvach@epam.com>
> >>>
> >>> 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 <mykola_kvach@epam.com>
> >>> ---
> >>> 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v14 1/4] xen/arm: Implement PSCI SYSTEM_SUSPEND call for guests
2025-11-19 11:32 ` Mykola Kvach
@ 2025-11-19 11:43 ` Orzel, Michal
2025-11-19 11:51 ` Mykola Kvach
0 siblings, 1 reply; 13+ messages in thread
From: Orzel, Michal @ 2025-11-19 11:43 UTC (permalink / raw)
To: Mykola Kvach
Cc: xen-devel, Mykola Kvach, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk, Andrew Cooper,
Anthony PERARD, Jan Beulich, Roger Pau Monné
On 19/11/2025 12:32, Mykola Kvach wrote:
> On Wed, Nov 19, 2025 at 1:07 PM Orzel, Michal <michal.orzel@amd.com> 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 <michal.orzel@amd.com> wrote:
>>>>
>>>>
>>>>
>>>> On 18/11/2025 16:37, Mykola Kvach wrote:
>>>>> From: Mykola Kvach <mykola_kvach@epam.com>
>>>>>
>>>>> 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 <mykola_kvach@epam.com>
>>>>> ---
>>>>> 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.
It's not possible to run it locally. But you can use your (if you don't have,
ask on Matrix) Xen fork under https://gitlab.com/xen-project/people to push a
branch and trigger the CI (ECLAIR scan needs to be manually executed from the
pipeline page).
~Michal
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v14 1/4] xen/arm: Implement PSCI SYSTEM_SUSPEND call for guests
2025-11-19 11:43 ` Orzel, Michal
@ 2025-11-19 11:51 ` Mykola Kvach
0 siblings, 0 replies; 13+ messages in thread
From: Mykola Kvach @ 2025-11-19 11:51 UTC (permalink / raw)
To: Orzel, Michal
Cc: xen-devel, Mykola Kvach, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk, Andrew Cooper,
Anthony PERARD, Jan Beulich, Roger Pau Monné
On Wed, Nov 19, 2025 at 1:43 PM Orzel, Michal <michal.orzel@amd.com> wrote:
>
>
>
> On 19/11/2025 12:32, Mykola Kvach wrote:
> > On Wed, Nov 19, 2025 at 1:07 PM Orzel, Michal <michal.orzel@amd.com> 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 <michal.orzel@amd.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 18/11/2025 16:37, Mykola Kvach wrote:
> >>>>> From: Mykola Kvach <mykola_kvach@epam.com>
> >>>>>
> >>>>> 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 <mykola_kvach@epam.com>
> >>>>> ---
> >>>>> 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.
> It's not possible to run it locally. But you can use your (if you don't have,
> ask on Matrix) Xen fork under https://gitlab.com/xen-project/people to push a
> branch and trigger the CI (ECLAIR scan needs to be manually executed from the
> pipeline page).
Got it, thanks.
~Mykola
>
> ~Michal
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v14 1/4] xen/arm: Implement PSCI SYSTEM_SUSPEND call for guests
2025-11-19 8:47 ` Orzel, Michal
2025-11-19 11:00 ` Mykola Kvach
@ 2025-11-19 12:47 ` Mykola Kvach
1 sibling, 0 replies; 13+ messages in thread
From: Mykola Kvach @ 2025-11-19 12:47 UTC (permalink / raw)
To: Orzel, Michal
Cc: xen-devel, Mykola Kvach, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk, Andrew Cooper,
Anthony PERARD, Jan Beulich, Roger Pau Monné
On Wed, Nov 19, 2025 at 10:48 AM Orzel, Michal <michal.orzel@amd.com> wrote:
>
>
>
> On 18/11/2025 16:37, Mykola Kvach wrote:
> > From: Mykola Kvach <mykola_kvach@epam.com>
> >
> > 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 <mykola_kvach@epam.com>
> > ---
> > 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.
>
> > + 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)
> > +#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.
>
> > +
> > +#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
>
> > {
> > - 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%.
>
> > + 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
>
> > +#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?
>
> > +
> > +#endif /* __XEN_SUSPEND_H__ */
> Missing emacs block?
>
> Did you run MISRA scan on this patch?
The ECLAIR MISRA scan was run via GitLab CI for this branch by my
colleague:
https://gitlab.com/xen-project/people/dimaprkp4k/xen/-/pipelines/2167389661
As far as I can see, there are no new MISRA violations reported for the
code touched by this patch.
~Mykola
>
> ~Michal
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2025-11-19 12:48 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-11-18 15:37 [PATCH v14 0/4] Enable guest suspend/resume support on ARM via vPSCI Mykola Kvach
2025-11-18 15:37 ` [PATCH v14 1/4] xen/arm: Implement PSCI SYSTEM_SUSPEND call for guests Mykola Kvach
2025-11-19 8:47 ` Orzel, Michal
2025-11-19 11:00 ` Mykola Kvach
2025-11-19 11:07 ` Nicola Vetrini
2025-11-19 11:07 ` Orzel, Michal
2025-11-19 11:32 ` Mykola Kvach
2025-11-19 11:43 ` Orzel, Michal
2025-11-19 11:51 ` Mykola Kvach
2025-11-19 12:47 ` Mykola Kvach
2025-11-18 15:37 ` [PATCH v14 2/4] tools/xl: Allow compilation of 'xl resume' command on Arm Mykola Kvach
2025-11-18 15:37 ` [PATCH v14 3/4] SUPPORT.md: Document PSCI SYSTEM_SUSPEND support for guests Mykola Kvach
2025-11-18 15:37 ` [PATCH v14 4/4] CHANGELOG: Document guest suspend/resume to RAM support on Arm Mykola Kvach
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.