[PATCH v12 0/4] Enable guest suspend/resume support on ARM via vPSCI

[PATCH v12 0/4] Enable guest suspend/resume support on ARM via vPSCI

[Mykola Kvach]

From: Mykola Kvach <mykola_kvach@epam.com>

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.1754943874.git.mykola._5Fkvach@epam.com/
---

Changes in v12:
- Use the input vCPU from vpsci_vcpu_up_prepare function argument instead of current.
- Add a check for the wake_cpu pointer on resume.
- Call arch_domain_resume() under shutdown_lock.
- Drop redundant vgic_clear_pending_irqs() call from vpsci_vcpu_up_prepare().

Changes in V11:
- introduce arch_domain_resume() and vpsci_vcpu_up_prepare(), which are now
called on the resume path to avoid extra modifications to common code.
The vCPU context is now updated during domain resume.

Changes in V10:
- An ARM-specific helper (domain_resume_nopause_helper)
- An isb() is added before p2m_save_state
- The is_64bit_domain check is dropped when masking the upper part of entry
  point and cid for SMC32 SYSTEM_SUSPEND PSCI calls
- Status of vPSCI SYSTEM_SUSPEND changed from "Experimental" to "Tech Preview"

Changes in V9:
- no functional changes
- enhance commit message and add extra comment to the code after review

Changes in V8:
- GIC and virtual timer context saved when the domain suspends
- rework locking
- minor changes after code review

Changes in V7:
- add proper locking
- minor changes after code review

Main changes in V6:
- Skip execution of ctxt_switch_from for VCPUs in paused domains.
- Implement domain_resume_nopause
- Add a helper to determine if a VCPU is in suspended domain.
- Ignore upper 32 bits of arguments for 64-bit domains calling SMC32 SYSTEM_SUSPEND.
- Macro renamed from LIBXL_HAVE_NO_SUSPEND_RESUME to LIBXL_HAVE_NO_SUSPEND for clarity.
- Documentation now focuses only on the SYSTEM_SUSPEND function, with improved wording and structure.
- Changelog and commit messages refined for clarity and to remove redundant explanations.

Main change in V5:
  - Reverted the logic related to suspending domains. Instead of the standby
    mode introduced in v4, domains now resume execution at the point provided
    during suspend

The rest of the minor changes are described in the changelog of each commit.

Previous versions of this patch series:
  V1: https://lists.xenproject.org/archives/html/xen-devel/2018-11/msg01093.html
  V2: https://marc.info/?l=xen-devel&m=166514782207736&w=2
  V3: https://lists.xenproject.org/archives/html/xen-devel/2025-03/msg00168.html

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                          |   2 +
 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                 |  37 ++++++++
 xen/arch/arm/include/asm/domain.h     |   6 ++
 xen/arch/arm/include/asm/perfc_defn.h |   1 +
 xen/arch/arm/include/asm/psci.h       |   2 +
 xen/arch/arm/include/asm/vpsci.h      |   5 +-
 xen/arch/arm/vpsci.c                  | 116 +++++++++++++++++++++-----
 xen/arch/ppc/stubs.c                  |   5 ++
 xen/arch/riscv/stubs.c                |   5 ++
 xen/arch/x86/domain.c                 |   5 ++
 xen/common/domain.c                   |   9 ++
 xen/include/xen/domain.h              |   2 +
 19 files changed, 188 insertions(+), 37 deletions(-)

-- 
2.48.1

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

[Mykola Kvach]

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 v12:
- Use the input vCPU from vpsci_vcpu_up_prepare function argument instead of current.
- Add a check for the wake_cpu pointer on resume.
- Call arch_domain_resume() under shutdown_lock.
- Drop redundant vgic_clear_pending_irqs() call from vpsci_vcpu_up_prepare().
- Cosmetic fixes.

Changes in V11:
- introduce arch_domain_resume() and vpsci_vcpu_up_prepare(), which are now
called on the resume path to avoid extra modifications to common code.
The vCPU context is now updated during domain resume.

Changes in V10:
- small changes to the commit message reflect updates introduced in this
  version of the patch.
- Comments are improved, clarified, and expanded, especially regarding PSCI
  requirements and context handling.
- An ARM-specific helper (domain_resume_nopause_helper)
- gprintk() and PRIregister are used for logging in vPSCI code.
- An isb() is added before p2m_save_state
- The is_64bit_domain check is dropped when masking the upper part of entry
  point and cid for SMC32 SYSTEM_SUSPEND PSCI calls

Changes in V9:
- no functional changes
- cosmetic chnages after review
- enhance commit message and add extra comment to the code after review

Changes in V8:
- GIC and virtual timer context must be saved when the domain suspends
- rework locking
- minor changes after code review

Changes in V7:
- add proper locking
- minor changes after code review

Changes in V6:
- skip execution of ctxt_switch_from for vcpu that is in paused domain
- add implementation of domain_resume without domain_pause
- add helper function to determine if vcpu is suspended or not
- ignore upper 32 bits of argument values when the domain is 64-bit
  and calls the SMC32 SYSTEM_SUSPEND function
- cosmetic changes after review

Changes in V5:
- don't use standby mode, restore execution in a provided by guest point
- move checking that all CPUs, except current one, are offline to after
  pausing the vCPUs
- provide ret status from arch_domain_shutdown and handle it in
  domain_shutdown
- adjust VPSCI_NR_FUNCS to reflect the number of newly added PSCI functions

Changes in V4:
Dropped all changes related to watchdog, domain is marked as shutting
down in domain_shutdown and watchdog timeout handler won't trigger
because of it.

Previous versions included code to manage Xen watchdog timers during suspend,
but this was removed. When a guest OS starts the Xen watchdog (either via the
kernel driver or xenwatchdogd), it is responsible for managing that state
across suspend/resume. On Linux, the Xen kernel driver properly stops the
watchdog during suspend. However, when xenwatchdogd is used instead, suspend
handling is incomplete, potentially leading to watchdog-triggered resets on
resume. Xen leaves watchdog handling to the guest OS and its services.

Dropped all changes related to VCPU context, because instead domain_shutdown
is used, so we don't need any extra changes for suspending domain.

Changes in V3:
Dropped all domain flags and related code (which touched common functions like
vcpu_unblock), keeping only the necessary changes for Xen suspend/resume, i.e.
suspend/resume is now fully supported only for the hardware domain.
Proper support for domU suspend/resume will be added in a future patch.
This patch does not yet include VCPU context reset or domain context
restoration in VCPU.
---
 xen/arch/arm/domain.c                 |  37 ++++++++
 xen/arch/arm/include/asm/domain.h     |   6 ++
 xen/arch/arm/include/asm/perfc_defn.h |   1 +
 xen/arch/arm/include/asm/psci.h       |   2 +
 xen/arch/arm/include/asm/vpsci.h      |   5 +-
 xen/arch/arm/vpsci.c                  | 116 +++++++++++++++++++++-----
 xen/arch/ppc/stubs.c                  |   5 ++
 xen/arch/riscv/stubs.c                |   5 ++
 xen/arch/x86/domain.c                 |   5 ++
 xen/common/domain.c                   |   9 ++
 xen/include/xen/domain.h              |   2 +
 11 files changed, 171 insertions(+), 22 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 863ae18157..7d7358abe5 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>
@@ -27,6 +29,7 @@
 #include <asm/tee/tee.h>
 #include <asm/vfp.h>
 #include <asm/vgic.h>
+#include <asm/vpsci.h>
 #include <asm/vtimer.h>
 
 #include "vpci.h"
@@ -880,6 +883,40 @@ void arch_domain_creation_finished(struct domain *d)
     p2m_domain_creation_finished(d);
 }
 
+int arch_domain_resume(struct domain *d)
+{
+    int rc;
+    typeof(d->arch.resume_ctx) *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_WARNING, "%pd: Invalid wake CPU pointer for resume\n",
+                d);
+        return 0;
+    }
+
+    rc = vpsci_vcpu_up_prepare(ctx->wake_cpu , ctx->ep, ctx->cid);
+    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 a3487ca713..68185fc4d6 100644
--- a/xen/arch/arm/include/asm/domain.h
+++ b/xen/arch/arm/include/asm/domain.h
@@ -121,6 +121,12 @@ struct arch_domain
     void *tee;
 #endif
 
+    struct resume_info {
+        register_t ep;
+        register_t cid;
+        struct vcpu *wake_cpu;
+    } 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/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/arch/ppc/stubs.c b/xen/arch/ppc/stubs.c
index bdaf474c5c..0db0627b5c 100644
--- a/xen/arch/ppc/stubs.c
+++ b/xen/arch/ppc/stubs.c
@@ -224,6 +224,11 @@ void arch_domain_creation_finished(struct domain *d)
     BUG_ON("unimplemented");
 }
 
+int arch_domain_resume(struct domain *d)
+{
+    return 0;
+}
+
 int arch_set_info_guest(struct vcpu *v, vcpu_guest_context_u c)
 {
     BUG_ON("unimplemented");
diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c
index 1a8c86cd8d..52532ae14d 100644
--- a/xen/arch/riscv/stubs.c
+++ b/xen/arch/riscv/stubs.c
@@ -198,6 +198,11 @@ void arch_domain_creation_finished(struct domain *d)
     BUG_ON("unimplemented");
 }
 
+int arch_domain_resume(struct domain *d)
+{
+    return 0;
+}
+
 int arch_set_info_guest(struct vcpu *v, vcpu_guest_context_u c)
 {
     BUG_ON("unimplemented");
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 19fd86ce88..94a06bc697 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1138,6 +1138,11 @@ void arch_domain_creation_finished(struct domain *d)
         hvm_domain_creation_finished(d);
 }
 
+int arch_domain_resume(struct domain *d)
+{
+    return 0;
+}
+
 #ifdef CONFIG_COMPAT
 #define xen_vcpu_guest_context vcpu_guest_context
 #define fpu_ctxt fpu_ctxt.x
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 104e917f07..667017c5e1 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1352,6 +1352,7 @@ int domain_shutdown(struct domain *d, u8 reason)
 void domain_resume(struct domain *d)
 {
     struct vcpu *v;
+    int rc;
 
     /*
      * Some code paths assume that shutdown status does not get reset under
@@ -1361,6 +1362,13 @@ void domain_resume(struct domain *d)
 
     spin_lock(&d->shutdown_lock);
 
+    rc = arch_domain_resume(d);
+    if ( rc )
+    {
+        printk("%pd: Failed to resume domain (ret %d)\n", d, rc);
+        goto fail;
+    }
+
     d->is_shutting_down = d->is_shut_down = 0;
     d->shutdown_code = SHUTDOWN_CODE_INVALID;
 
@@ -1371,6 +1379,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/domain.h b/xen/include/xen/domain.h
index e10baf2615..5f77ffadf1 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -109,6 +109,8 @@ int arch_domain_soft_reset(struct domain *d);
 
 void arch_domain_creation_finished(struct domain *d);
 
+int arch_domain_resume(struct domain *d);
+
 void arch_p2m_set_access_required(struct domain *d, bool access_required);
 
 int arch_set_info_guest(struct vcpu *v, vcpu_guest_context_u c);
-- 
2.48.1

[PATCH v12 2/4] tools/xl: Allow compilation of 'xl resume' command on Arm

[Mykola Kvach]

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 v7:
- dropped renaming of LIBXL_HAVE_NO_SUSPEND_RESUME macro

Changes in v6:
- Renamed macro from LIBXL_HAVE_NO_SUSPEND_RESUME to LIBXL_HAVE_NO_SUSPEND
  to better reflect the scope of this change
- Applied cosmetic changes based on review feedback
---
 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 185f74d8a8..b204fc5e2e 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -1140,7 +1140,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 45745f0dbb..9233b73f85 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.48.1

[PATCH v12 3/4] SUPPORT.md: Document PSCI SYSTEM_SUSPEND support for guests

[Mykola Kvach]

From: Mykola Kvach <mykola_kvach@epam.com>

Add a new entry under the "Virtual Hardware, QEMU" section documenting
support for the optional PSCI SYSTEM_SUSPEND function exposed to guests.

This function is available via the virtual PSCI (vPSCI) 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>
---
Changes in V10:
- Status of vPSCI SYSTEM_SUSPEND changed from "Experimental" to
  "Tech Preview"

Changes in v6:
- Dropped the generic guest PSCI support entry (merged in a separate patch)
- This patch now documents only the SYSTEM_SUSPEND optional function
- Reworded commit message to match the final form after rebase

Changes in v5:
- Dropped ARM/PSCI entry: this refers to internal use of PSCI SMC calls,
  which is not relevant for SUPPORT.md
- Added a dedicated entry for PSCI SYSTEM_SUSPEND instead of generic guest
  PSCI info; guest PSCI support was documented in a separate patch
---
 SUPPORT.md | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/SUPPORT.md b/SUPPORT.md
index 6a82a92189..0ce0903cb1 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.48.1

[PATCH v12 4/4] CHANGELOG: Document guest suspend/resume to RAM support on Arm

[Mykola Kvach]

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>
Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Chnages in v6:
- removed reduntand explanation that thi support added for
  both arm32 and arm64.

Changes in v5:
- adjustments to the commit title and message
- expanded the changelog entry to include more context about
  suspend/resume support introduced in this patch series
---
 CHANGELOG.md | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index cd34ea87b8..7a75bd37cb 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -36,6 +36,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 
  - On Arm:
     - Ability to enable stack protector
+    - Support for guest suspend and resume to/from RAM via vPSCI.
+      Applies only to non-hardware domain guests.
 
 ### Removed
  - On x86:
-- 
2.48.1

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

[Volodymyr Babchuk]

Hi Mykola,

Mykola Kvach <xakep.amatop@gmail.com> writes:

> 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>

Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

> ---
> Changes in v12:
> - Use the input vCPU from vpsci_vcpu_up_prepare function argument instead of current.
> - Add a check for the wake_cpu pointer on resume.
> - Call arch_domain_resume() under shutdown_lock.
> - Drop redundant vgic_clear_pending_irqs() call from vpsci_vcpu_up_prepare().
> - Cosmetic fixes.
>
> Changes in V11:
> - introduce arch_domain_resume() and vpsci_vcpu_up_prepare(), which are now
> called on the resume path to avoid extra modifications to common code.
> The vCPU context is now updated during domain resume.
>
> Changes in V10:
> - small changes to the commit message reflect updates introduced in this
>   version of the patch.
> - Comments are improved, clarified, and expanded, especially regarding PSCI
>   requirements and context handling.
> - An ARM-specific helper (domain_resume_nopause_helper)
> - gprintk() and PRIregister are used for logging in vPSCI code.
> - An isb() is added before p2m_save_state
> - The is_64bit_domain check is dropped when masking the upper part of entry
>   point and cid for SMC32 SYSTEM_SUSPEND PSCI calls
>
> Changes in V9:
> - no functional changes
> - cosmetic chnages after review
> - enhance commit message and add extra comment to the code after review
>
> Changes in V8:
> - GIC and virtual timer context must be saved when the domain suspends
> - rework locking
> - minor changes after code review
>
> Changes in V7:
> - add proper locking
> - minor changes after code review
>
> Changes in V6:
> - skip execution of ctxt_switch_from for vcpu that is in paused domain
> - add implementation of domain_resume without domain_pause
> - add helper function to determine if vcpu is suspended or not
> - ignore upper 32 bits of argument values when the domain is 64-bit
>   and calls the SMC32 SYSTEM_SUSPEND function
> - cosmetic changes after review
>
> Changes in V5:
> - don't use standby mode, restore execution in a provided by guest point
> - move checking that all CPUs, except current one, are offline to after
>   pausing the vCPUs
> - provide ret status from arch_domain_shutdown and handle it in
>   domain_shutdown
> - adjust VPSCI_NR_FUNCS to reflect the number of newly added PSCI functions
>
> Changes in V4:
> Dropped all changes related to watchdog, domain is marked as shutting
> down in domain_shutdown and watchdog timeout handler won't trigger
> because of it.
>
> Previous versions included code to manage Xen watchdog timers during suspend,
> but this was removed. When a guest OS starts the Xen watchdog (either via the
> kernel driver or xenwatchdogd), it is responsible for managing that state
> across suspend/resume. On Linux, the Xen kernel driver properly stops the
> watchdog during suspend. However, when xenwatchdogd is used instead, suspend
> handling is incomplete, potentially leading to watchdog-triggered resets on
> resume. Xen leaves watchdog handling to the guest OS and its services.
>
> Dropped all changes related to VCPU context, because instead domain_shutdown
> is used, so we don't need any extra changes for suspending domain.
>
> Changes in V3:
> Dropped all domain flags and related code (which touched common functions like
> vcpu_unblock), keeping only the necessary changes for Xen suspend/resume, i.e.
> suspend/resume is now fully supported only for the hardware domain.
> Proper support for domU suspend/resume will be added in a future patch.
> This patch does not yet include VCPU context reset or domain context
> restoration in VCPU.
> ---
>  xen/arch/arm/domain.c                 |  37 ++++++++
>  xen/arch/arm/include/asm/domain.h     |   6 ++
>  xen/arch/arm/include/asm/perfc_defn.h |   1 +
>  xen/arch/arm/include/asm/psci.h       |   2 +
>  xen/arch/arm/include/asm/vpsci.h      |   5 +-
>  xen/arch/arm/vpsci.c                  | 116 +++++++++++++++++++++-----
>  xen/arch/ppc/stubs.c                  |   5 ++
>  xen/arch/riscv/stubs.c                |   5 ++
>  xen/arch/x86/domain.c                 |   5 ++
>  xen/common/domain.c                   |   9 ++
>  xen/include/xen/domain.h              |   2 +
>  11 files changed, 171 insertions(+), 22 deletions(-)
>
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 863ae18157..7d7358abe5 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>
> @@ -27,6 +29,7 @@
>  #include <asm/tee/tee.h>
>  #include <asm/vfp.h>
>  #include <asm/vgic.h>
> +#include <asm/vpsci.h>
>  #include <asm/vtimer.h>
>  
>  #include "vpci.h"
> @@ -880,6 +883,40 @@ void arch_domain_creation_finished(struct domain *d)
>      p2m_domain_creation_finished(d);
>  }
>  
> +int arch_domain_resume(struct domain *d)
> +{
> +    int rc;
> +    typeof(d->arch.resume_ctx) *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_WARNING, "%pd: Invalid wake CPU pointer for resume\n",
> +                d);
> +        return 0;
> +    }
> +
> +    rc = vpsci_vcpu_up_prepare(ctx->wake_cpu , ctx->ep, ctx->cid);
> +    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 a3487ca713..68185fc4d6 100644
> --- a/xen/arch/arm/include/asm/domain.h
> +++ b/xen/arch/arm/include/asm/domain.h
> @@ -121,6 +121,12 @@ struct arch_domain
>      void *tee;
>  #endif
>  
> +    struct resume_info {
> +        register_t ep;
> +        register_t cid;
> +        struct vcpu *wake_cpu;
> +    } 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/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/arch/ppc/stubs.c b/xen/arch/ppc/stubs.c
> index bdaf474c5c..0db0627b5c 100644
> --- a/xen/arch/ppc/stubs.c
> +++ b/xen/arch/ppc/stubs.c
> @@ -224,6 +224,11 @@ void arch_domain_creation_finished(struct domain *d)
>      BUG_ON("unimplemented");
>  }
>  
> +int arch_domain_resume(struct domain *d)
> +{
> +    return 0;
> +}
> +
>  int arch_set_info_guest(struct vcpu *v, vcpu_guest_context_u c)
>  {
>      BUG_ON("unimplemented");
> diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c
> index 1a8c86cd8d..52532ae14d 100644
> --- a/xen/arch/riscv/stubs.c
> +++ b/xen/arch/riscv/stubs.c
> @@ -198,6 +198,11 @@ void arch_domain_creation_finished(struct domain *d)
>      BUG_ON("unimplemented");
>  }
>  
> +int arch_domain_resume(struct domain *d)
> +{
> +    return 0;
> +}
> +
>  int arch_set_info_guest(struct vcpu *v, vcpu_guest_context_u c)
>  {
>      BUG_ON("unimplemented");
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 19fd86ce88..94a06bc697 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1138,6 +1138,11 @@ void arch_domain_creation_finished(struct domain *d)
>          hvm_domain_creation_finished(d);
>  }
>  
> +int arch_domain_resume(struct domain *d)
> +{
> +    return 0;
> +}
> +
>  #ifdef CONFIG_COMPAT
>  #define xen_vcpu_guest_context vcpu_guest_context
>  #define fpu_ctxt fpu_ctxt.x
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 104e917f07..667017c5e1 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -1352,6 +1352,7 @@ int domain_shutdown(struct domain *d, u8 reason)
>  void domain_resume(struct domain *d)
>  {
>      struct vcpu *v;
> +    int rc;
>  
>      /*
>       * Some code paths assume that shutdown status does not get reset under
> @@ -1361,6 +1362,13 @@ void domain_resume(struct domain *d)
>  
>      spin_lock(&d->shutdown_lock);
>  
> +    rc = arch_domain_resume(d);
> +    if ( rc )
> +    {
> +        printk("%pd: Failed to resume domain (ret %d)\n", d, rc);
> +        goto fail;
> +    }
> +
>      d->is_shutting_down = d->is_shut_down = 0;
>      d->shutdown_code = SHUTDOWN_CODE_INVALID;
>  
> @@ -1371,6 +1379,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/domain.h b/xen/include/xen/domain.h
> index e10baf2615..5f77ffadf1 100644
> --- a/xen/include/xen/domain.h
> +++ b/xen/include/xen/domain.h
> @@ -109,6 +109,8 @@ int arch_domain_soft_reset(struct domain *d);
>  
>  void arch_domain_creation_finished(struct domain *d);
>  
> +int arch_domain_resume(struct domain *d);
> +
>  void arch_p2m_set_access_required(struct domain *d, bool access_required);
>  
>  int arch_set_info_guest(struct vcpu *v, vcpu_guest_context_u c);

-- 
WBR, Volodymyr

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

[Jan Beulich]

On 31.08.2025 00:10, Mykola Kvach wrote:
> --- a/xen/arch/ppc/stubs.c
> +++ b/xen/arch/ppc/stubs.c
> @@ -224,6 +224,11 @@ void arch_domain_creation_finished(struct domain *d)
>      BUG_ON("unimplemented");
>  }
>  
> +int arch_domain_resume(struct domain *d)
> +{
> +    return 0;
> +}
> +
>  int arch_set_info_guest(struct vcpu *v, vcpu_guest_context_u c)
>  {
>      BUG_ON("unimplemented");
> diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c
> index 1a8c86cd8d..52532ae14d 100644
> --- a/xen/arch/riscv/stubs.c
> +++ b/xen/arch/riscv/stubs.c
> @@ -198,6 +198,11 @@ void arch_domain_creation_finished(struct domain *d)
>      BUG_ON("unimplemented");
>  }
>  
> +int arch_domain_resume(struct domain *d)
> +{
> +    return 0;
> +}
> +
>  int arch_set_info_guest(struct vcpu *v, vcpu_guest_context_u c)
>  {
>      BUG_ON("unimplemented");
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 19fd86ce88..94a06bc697 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1138,6 +1138,11 @@ void arch_domain_creation_finished(struct domain *d)
>          hvm_domain_creation_finished(d);
>  }
>  
> +int arch_domain_resume(struct domain *d)
> +{
> +    return 0;
> +}
> +
>  #ifdef CONFIG_COMPAT
>  #define xen_vcpu_guest_context vcpu_guest_context
>  #define fpu_ctxt fpu_ctxt.x

I definitely don't like this redundancy, and even less so that you introduce out-
of-line calls.

> --- a/xen/include/xen/domain.h
> +++ b/xen/include/xen/domain.h
> @@ -109,6 +109,8 @@ int arch_domain_soft_reset(struct domain *d);
>  
>  void arch_domain_creation_finished(struct domain *d);
>  
> +int arch_domain_resume(struct domain *d);

I think this wants to move to a per-arch header, presence of which is checked by
has_include(), with an inline fallback define once centrally here.

Jan

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

[Mykola Kvach]

Hi Jan,

On Mon, Sep 1, 2025 at 5:29 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 31.08.2025 00:10, Mykola Kvach wrote:
> > --- a/xen/arch/ppc/stubs.c
> > +++ b/xen/arch/ppc/stubs.c
> > @@ -224,6 +224,11 @@ void arch_domain_creation_finished(struct domain *d)
> >      BUG_ON("unimplemented");
> >  }
> >
> > +int arch_domain_resume(struct domain *d)
> > +{
> > +    return 0;
> > +}
> > +
> >  int arch_set_info_guest(struct vcpu *v, vcpu_guest_context_u c)
> >  {
> >      BUG_ON("unimplemented");
> > diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c
> > index 1a8c86cd8d..52532ae14d 100644
> > --- a/xen/arch/riscv/stubs.c
> > +++ b/xen/arch/riscv/stubs.c
> > @@ -198,6 +198,11 @@ void arch_domain_creation_finished(struct domain *d)
> >      BUG_ON("unimplemented");
> >  }
> >
> > +int arch_domain_resume(struct domain *d)
> > +{
> > +    return 0;
> > +}
> > +
> >  int arch_set_info_guest(struct vcpu *v, vcpu_guest_context_u c)
> >  {
> >      BUG_ON("unimplemented");
> > diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> > index 19fd86ce88..94a06bc697 100644
> > --- a/xen/arch/x86/domain.c
> > +++ b/xen/arch/x86/domain.c
> > @@ -1138,6 +1138,11 @@ void arch_domain_creation_finished(struct domain *d)
> >          hvm_domain_creation_finished(d);
> >  }
> >
> > +int arch_domain_resume(struct domain *d)
> > +{
> > +    return 0;
> > +}
> > +
> >  #ifdef CONFIG_COMPAT
> >  #define xen_vcpu_guest_context vcpu_guest_context
> >  #define fpu_ctxt fpu_ctxt.x
>
> I definitely don't like this redundancy, and even less so that you introduce out-
> of-line calls.

Thank you for your feedback.
I followed the existing pattern used in other architecture stubs.

>
> > --- a/xen/include/xen/domain.h
> > +++ b/xen/include/xen/domain.h
> > @@ -109,6 +109,8 @@ int arch_domain_soft_reset(struct domain *d);
> >
> >  void arch_domain_creation_finished(struct domain *d);
> >
> > +int arch_domain_resume(struct domain *d);
>
> I think this wants to move to a per-arch header, presence of which is checked by
> has_include(), with an inline fallback define once centrally here.

Would it be acceptable to use a weak function as the default
implementation instead? This way, architectures needing a real
implementation could override it, while others would use the weak
default.

>
> Jan

Best regards,
Mykola

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

[Mykola Kvach]

On Mon, Sep 1, 2025 at 8:02 PM Mykola Kvach <xakep.amatop@gmail.com> wrote:
>
> Hi Jan,
>
> On Mon, Sep 1, 2025 at 5:29 PM Jan Beulich <jbeulich@suse.com> wrote:
> >
> > On 31.08.2025 00:10, Mykola Kvach wrote:
> > > --- a/xen/arch/ppc/stubs.c
> > > +++ b/xen/arch/ppc/stubs.c
> > > @@ -224,6 +224,11 @@ void arch_domain_creation_finished(struct domain *d)
> > >      BUG_ON("unimplemented");
> > >  }
> > >
> > > +int arch_domain_resume(struct domain *d)
> > > +{
> > > +    return 0;
> > > +}
> > > +
> > >  int arch_set_info_guest(struct vcpu *v, vcpu_guest_context_u c)
> > >  {
> > >      BUG_ON("unimplemented");
> > > diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c
> > > index 1a8c86cd8d..52532ae14d 100644
> > > --- a/xen/arch/riscv/stubs.c
> > > +++ b/xen/arch/riscv/stubs.c
> > > @@ -198,6 +198,11 @@ void arch_domain_creation_finished(struct domain *d)
> > >      BUG_ON("unimplemented");
> > >  }
> > >
> > > +int arch_domain_resume(struct domain *d)
> > > +{
> > > +    return 0;
> > > +}
> > > +
> > >  int arch_set_info_guest(struct vcpu *v, vcpu_guest_context_u c)
> > >  {
> > >      BUG_ON("unimplemented");
> > > diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> > > index 19fd86ce88..94a06bc697 100644
> > > --- a/xen/arch/x86/domain.c
> > > +++ b/xen/arch/x86/domain.c
> > > @@ -1138,6 +1138,11 @@ void arch_domain_creation_finished(struct domain *d)
> > >          hvm_domain_creation_finished(d);
> > >  }
> > >
> > > +int arch_domain_resume(struct domain *d)
> > > +{
> > > +    return 0;
> > > +}
> > > +
> > >  #ifdef CONFIG_COMPAT
> > >  #define xen_vcpu_guest_context vcpu_guest_context
> > >  #define fpu_ctxt fpu_ctxt.x
> >
> > I definitely don't like this redundancy, and even less so that you introduce out-
> > of-line calls.
>
> Thank you for your feedback.
> I followed the existing pattern used in other architecture stubs.

... while I understand your concern about redundancy and out-of-line
calls, I would appreciate more specific technical reasoning for why
this approach is undesirable.
Code review is most effective when it is based on objective criteria
and project guidelines, rather than personal preferences.
This helps contributors understand the rationale and make improvements
that benefit the whole project.

>
> >
> > > --- a/xen/include/xen/domain.h
> > > +++ b/xen/include/xen/domain.h
> > > @@ -109,6 +109,8 @@ int arch_domain_soft_reset(struct domain *d);
> > >
> > >  void arch_domain_creation_finished(struct domain *d);
> > >
> > > +int arch_domain_resume(struct domain *d);
> >
> > I think this wants to move to a per-arch header, presence of which is checked by
> > has_include(), with an inline fallback define once centrally here.
>
> Would it be acceptable to use a weak function as the default
> implementation instead? This way, architectures needing a real
> implementation could override it, while others would use the weak
> default.
>
> >
> > Jan
>
> Best regards,
> Mykola

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

[Mykola Kvach]

On Mon, Sep 1, 2025 at 8:02 PM Mykola Kvach <xakep.amatop@gmail.com> wrote:
>
> Hi Jan,
>
> On Mon, Sep 1, 2025 at 5:29 PM Jan Beulich <jbeulich@suse.com> wrote:
> >
> > On 31.08.2025 00:10, Mykola Kvach wrote:
> > > --- a/xen/arch/ppc/stubs.c
> > > +++ b/xen/arch/ppc/stubs.c
> > > @@ -224,6 +224,11 @@ void arch_domain_creation_finished(struct domain *d)
> > >      BUG_ON("unimplemented");
> > >  }
> > >
> > > +int arch_domain_resume(struct domain *d)
> > > +{
> > > +    return 0;
> > > +}
> > > +
> > >  int arch_set_info_guest(struct vcpu *v, vcpu_guest_context_u c)
> > >  {
> > >      BUG_ON("unimplemented");
> > > diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c
> > > index 1a8c86cd8d..52532ae14d 100644
> > > --- a/xen/arch/riscv/stubs.c
> > > +++ b/xen/arch/riscv/stubs.c
> > > @@ -198,6 +198,11 @@ void arch_domain_creation_finished(struct domain *d)
> > >      BUG_ON("unimplemented");
> > >  }
> > >
> > > +int arch_domain_resume(struct domain *d)
> > > +{
> > > +    return 0;
> > > +}
> > > +
> > >  int arch_set_info_guest(struct vcpu *v, vcpu_guest_context_u c)
> > >  {
> > >      BUG_ON("unimplemented");
> > > diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> > > index 19fd86ce88..94a06bc697 100644
> > > --- a/xen/arch/x86/domain.c
> > > +++ b/xen/arch/x86/domain.c
> > > @@ -1138,6 +1138,11 @@ void arch_domain_creation_finished(struct domain *d)
> > >          hvm_domain_creation_finished(d);
> > >  }
> > >
> > > +int arch_domain_resume(struct domain *d)
> > > +{
> > > +    return 0;
> > > +}
> > > +
> > >  #ifdef CONFIG_COMPAT
> > >  #define xen_vcpu_guest_context vcpu_guest_context
> > >  #define fpu_ctxt fpu_ctxt.x
> >
> > I definitely don't like this redundancy, and even less so that you introduce out-
> > of-line calls.
>
> Thank you for your feedback.
> I followed the existing pattern used in other architecture stubs.
>
> >
> > > --- a/xen/include/xen/domain.h
> > > +++ b/xen/include/xen/domain.h
> > > @@ -109,6 +109,8 @@ int arch_domain_soft_reset(struct domain *d);
> > >
> > >  void arch_domain_creation_finished(struct domain *d);
> > >
> > > +int arch_domain_resume(struct domain *d);
> >
> > I think this wants to move to a per-arch header, presence of which is checked by
> > has_include(), with an inline fallback define once centrally here.
>
> Would it be acceptable to use a weak function as the default
> implementation instead? This way, architectures needing a real
> implementation could override it, while others would use the weak
> default.

AFAIU, both your proposal and mine would violate MISRA C Dir 1.1 and
Rule 1.1 (also Rule 1.2 but it is acceptable). According to these
requirements, any use of compiler extensions should be documented and
understood. In the context of the Xen hypervisor, such extensions must
be listed in "docs/misra/C-language-toolchain.rst" as required by our
project guidelines.

>
> >
> > Jan
>
> Best regards,
> Mykola

Best regards,
Mykola

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

[Jan Beulich]

On 02.09.2025 08:29, Mykola Kvach wrote:
> On Mon, Sep 1, 2025 at 8:02 PM Mykola Kvach <xakep.amatop@gmail.com> wrote:
>>
>> Hi Jan,
>>
>> On Mon, Sep 1, 2025 at 5:29 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>
>>> On 31.08.2025 00:10, Mykola Kvach wrote:
>>>> --- a/xen/arch/ppc/stubs.c
>>>> +++ b/xen/arch/ppc/stubs.c
>>>> @@ -224,6 +224,11 @@ void arch_domain_creation_finished(struct domain *d)
>>>>      BUG_ON("unimplemented");
>>>>  }
>>>>
>>>> +int arch_domain_resume(struct domain *d)
>>>> +{
>>>> +    return 0;
>>>> +}
>>>> +
>>>>  int arch_set_info_guest(struct vcpu *v, vcpu_guest_context_u c)
>>>>  {
>>>>      BUG_ON("unimplemented");
>>>> diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c
>>>> index 1a8c86cd8d..52532ae14d 100644
>>>> --- a/xen/arch/riscv/stubs.c
>>>> +++ b/xen/arch/riscv/stubs.c
>>>> @@ -198,6 +198,11 @@ void arch_domain_creation_finished(struct domain *d)
>>>>      BUG_ON("unimplemented");
>>>>  }
>>>>
>>>> +int arch_domain_resume(struct domain *d)
>>>> +{
>>>> +    return 0;
>>>> +}
>>>> +
>>>>  int arch_set_info_guest(struct vcpu *v, vcpu_guest_context_u c)
>>>>  {
>>>>      BUG_ON("unimplemented");
>>>> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
>>>> index 19fd86ce88..94a06bc697 100644
>>>> --- a/xen/arch/x86/domain.c
>>>> +++ b/xen/arch/x86/domain.c
>>>> @@ -1138,6 +1138,11 @@ void arch_domain_creation_finished(struct domain *d)
>>>>          hvm_domain_creation_finished(d);
>>>>  }
>>>>
>>>> +int arch_domain_resume(struct domain *d)
>>>> +{
>>>> +    return 0;
>>>> +}
>>>> +
>>>>  #ifdef CONFIG_COMPAT
>>>>  #define xen_vcpu_guest_context vcpu_guest_context
>>>>  #define fpu_ctxt fpu_ctxt.x
>>>
>>> I definitely don't like this redundancy, and even less so that you introduce out-
>>> of-line calls.
>>
>> Thank you for your feedback.
>> I followed the existing pattern used in other architecture stubs.
>>
>>>
>>>> --- a/xen/include/xen/domain.h
>>>> +++ b/xen/include/xen/domain.h
>>>> @@ -109,6 +109,8 @@ int arch_domain_soft_reset(struct domain *d);
>>>>
>>>>  void arch_domain_creation_finished(struct domain *d);
>>>>
>>>> +int arch_domain_resume(struct domain *d);
>>>
>>> I think this wants to move to a per-arch header, presence of which is checked by
>>> has_include(), with an inline fallback define once centrally here.
>>
>> Would it be acceptable to use a weak function as the default
>> implementation instead? This way, architectures needing a real
>> implementation could override it, while others would use the weak
>> default.

Besides this not addressing my out-of-line concern, we avoided the use
of weak symbols so far. While I don't recall specific details, iirc
this was somewhat related to Linux at some point deciding to reduce
(eventually eliminate?) the use of weak symbols.

> AFAIU, both your proposal and mine would violate MISRA C Dir 1.1 and
> Rule 1.1 (also Rule 1.2 but it is acceptable). According to these
> requirements, any use of compiler extensions should be documented and
> understood. In the context of the Xen hypervisor, such extensions must
> be listed in "docs/misra/C-language-toolchain.rst" as required by our
> project guidelines.

Just to mention that we use has_include() already, and that there are
two uses of __weak in livepatch code (which I would prefer not to use as
justification that further use of weak symbols is okay, as they're in
macros used in livepatches only, not in core Xen).

Jan

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

[Jan Beulich]

On 01.09.2025 19:17, Mykola Kvach wrote:
> On Mon, Sep 1, 2025 at 8:02 PM Mykola Kvach <xakep.amatop@gmail.com> wrote:
>> On Mon, Sep 1, 2025 at 5:29 PM Jan Beulich <jbeulich@suse.com> wrote:
>>> On 31.08.2025 00:10, Mykola Kvach wrote:
>>>> --- a/xen/arch/ppc/stubs.c
>>>> +++ b/xen/arch/ppc/stubs.c
>>>> @@ -224,6 +224,11 @@ void arch_domain_creation_finished(struct domain *d)
>>>>      BUG_ON("unimplemented");
>>>>  }
>>>>
>>>> +int arch_domain_resume(struct domain *d)
>>>> +{
>>>> +    return 0;
>>>> +}
>>>> +
>>>>  int arch_set_info_guest(struct vcpu *v, vcpu_guest_context_u c)
>>>>  {
>>>>      BUG_ON("unimplemented");
>>>> diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c
>>>> index 1a8c86cd8d..52532ae14d 100644
>>>> --- a/xen/arch/riscv/stubs.c
>>>> +++ b/xen/arch/riscv/stubs.c
>>>> @@ -198,6 +198,11 @@ void arch_domain_creation_finished(struct domain *d)
>>>>      BUG_ON("unimplemented");
>>>>  }
>>>>
>>>> +int arch_domain_resume(struct domain *d)
>>>> +{
>>>> +    return 0;
>>>> +}
>>>> +
>>>>  int arch_set_info_guest(struct vcpu *v, vcpu_guest_context_u c)
>>>>  {
>>>>      BUG_ON("unimplemented");
>>>> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
>>>> index 19fd86ce88..94a06bc697 100644
>>>> --- a/xen/arch/x86/domain.c
>>>> +++ b/xen/arch/x86/domain.c
>>>> @@ -1138,6 +1138,11 @@ void arch_domain_creation_finished(struct domain *d)
>>>>          hvm_domain_creation_finished(d);
>>>>  }
>>>>
>>>> +int arch_domain_resume(struct domain *d)
>>>> +{
>>>> +    return 0;
>>>> +}
>>>> +
>>>>  #ifdef CONFIG_COMPAT
>>>>  #define xen_vcpu_guest_context vcpu_guest_context
>>>>  #define fpu_ctxt fpu_ctxt.x
>>>
>>> I definitely don't like this redundancy, and even less so that you introduce out-
>>> of-line calls.
>>
>> Thank you for your feedback.
>> I followed the existing pattern used in other architecture stubs.
> 
> ... while I understand your concern about redundancy and out-of-line
> calls, I would appreciate more specific technical reasoning for why
> this approach is undesirable.

Out of line functions, even if as simple as the example above, have a
code size and performance effect; effectively empty inline functions
can typically be eliminated altogether by the compiler, including the
checking of their "return" values. While the impact may be low, any
such instance can later be used as motivation / justification to
introduce further instances (much like you did in to your earlier
reply, still in context above). And the sum of them then may not be
"low impact" anymore.

Furthermore we're already moving towards wider use of has_include().

> Code review is most effective when it is based on objective criteria
> and project guidelines, rather than personal preferences.

And what did you derive from that my comment was purely based on a
personal preference? Plus even if it were (often I would indicate so),
that's imo still okay, as in many case maintainer preferences also
matter (e.g. if only for a more consistent overall code base).

> This helps contributors understand the rationale and make improvements
> that benefit the whole project.

While content-wise I agree, considering the amount of work I put into
doing reviews, I still view this sort of "education" as pretty close
to an offense. Plus did you consider how well it would scale if in
every review all sorts of extra justification would need giving? I
don't really like to put things this way, but I would really recommend
you first start doing perhaps dozens of reviews a week before judging
on whether any particular review gave you enough background info.

Jan

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

[Mykola Kvach]

On Tue, Sep 2, 2025 at 10:13 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 01.09.2025 19:17, Mykola Kvach wrote:
> > On Mon, Sep 1, 2025 at 8:02 PM Mykola Kvach <xakep.amatop@gmail.com> wrote:
> >> On Mon, Sep 1, 2025 at 5:29 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>> On 31.08.2025 00:10, Mykola Kvach wrote:
> >>>> --- a/xen/arch/ppc/stubs.c
> >>>> +++ b/xen/arch/ppc/stubs.c
> >>>> @@ -224,6 +224,11 @@ void arch_domain_creation_finished(struct domain *d)
> >>>>      BUG_ON("unimplemented");
> >>>>  }
> >>>>
> >>>> +int arch_domain_resume(struct domain *d)
> >>>> +{
> >>>> +    return 0;
> >>>> +}
> >>>> +
> >>>>  int arch_set_info_guest(struct vcpu *v, vcpu_guest_context_u c)
> >>>>  {
> >>>>      BUG_ON("unimplemented");
> >>>> diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c
> >>>> index 1a8c86cd8d..52532ae14d 100644
> >>>> --- a/xen/arch/riscv/stubs.c
> >>>> +++ b/xen/arch/riscv/stubs.c
> >>>> @@ -198,6 +198,11 @@ void arch_domain_creation_finished(struct domain *d)
> >>>>      BUG_ON("unimplemented");
> >>>>  }
> >>>>
> >>>> +int arch_domain_resume(struct domain *d)
> >>>> +{
> >>>> +    return 0;
> >>>> +}
> >>>> +
> >>>>  int arch_set_info_guest(struct vcpu *v, vcpu_guest_context_u c)
> >>>>  {
> >>>>      BUG_ON("unimplemented");
> >>>> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> >>>> index 19fd86ce88..94a06bc697 100644
> >>>> --- a/xen/arch/x86/domain.c
> >>>> +++ b/xen/arch/x86/domain.c
> >>>> @@ -1138,6 +1138,11 @@ void arch_domain_creation_finished(struct domain *d)
> >>>>          hvm_domain_creation_finished(d);
> >>>>  }
> >>>>
> >>>> +int arch_domain_resume(struct domain *d)
> >>>> +{
> >>>> +    return 0;
> >>>> +}
> >>>> +
> >>>>  #ifdef CONFIG_COMPAT
> >>>>  #define xen_vcpu_guest_context vcpu_guest_context
> >>>>  #define fpu_ctxt fpu_ctxt.x
> >>>
> >>> I definitely don't like this redundancy, and even less so that you introduce out-
> >>> of-line calls.
> >>
> >> Thank you for your feedback.
> >> I followed the existing pattern used in other architecture stubs.
> >
> > ... while I understand your concern about redundancy and out-of-line
> > calls, I would appreciate more specific technical reasoning for why
> > this approach is undesirable.
>
> Out of line functions, even if as simple as the example above, have a
> code size and performance effect; effectively empty inline functions
> can typically be eliminated altogether by the compiler, including the
> checking of their "return" values. While the impact may be low, any
> such instance can later be used as motivation / justification to
> introduce further instances (much like you did in to your earlier
> reply, still in context above). And the sum of them then may not be
> "low impact" anymore.

Thank you for your detailed explanation. As I mentioned earlier,
I understand why it’s preferable to avoid out-of-line implementations,
and I appreciate your technical reasoning...

>
> Furthermore we're already moving towards wider use of has_include().
>
> > Code review is most effective when it is based on objective criteria
> > and project guidelines, rather than personal preferences.
>
> And what did you derive from that my comment was purely based on a
> personal preference? Plus even if it were (often I would indicate so),
> that's imo still okay, as in many case maintainer preferences also
> matter (e.g. if only for a more consistent overall code base).
>
> > This helps contributors understand the rationale and make improvements
> > that benefit the whole project.
>
> While content-wise I agree, considering the amount of work I put into
> doing reviews, I still view this sort of "education" as pretty close
> to an offense. Plus did you consider how well it would scale if in
> every review all sorts of extra justification would need giving? I
> don't really like to put things this way, but I would really recommend
> you first start doing perhaps dozens of reviews a week before judging
> on whether any particular review gave you enough background info.

... I want to emphasize that I respect your work and the significant effort
you put into reviews. My intention is not to question your expertise,
but to highlight the importance of consistency for contributors.

Since the current codebase already uses this approach in multiple places,
contributors may get mixed signals when similar patterns are sometimes
accepted and sometimes discouraged. Clearer project-wide guidance, or even
small clarifications in coding style, would make it easier for contributors
to align with maintainers’ expectations.

I will adjust my patch accordingly and use has_include as you suggested.

Thanks again for your guidance and for the effort you put into reviews.

>
> Jan

Best regards,
Mykola

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

[Jan Beulich]

On 02.09.2025 10:42, Mykola Kvach wrote:
> Since the current codebase already uses this approach in multiple places,
> contributors may get mixed signals when similar patterns are sometimes
> accepted and sometimes discouraged. Clearer project-wide guidance, or even
> small clarifications in coding style, would make it easier for contributors
> to align with maintainers’ expectations.
> 
> I will adjust my patch accordingly and use has_include as you suggested.

One other thing to mention: I know time is somewhat tight, but you shouldn't
be too quick either - another maintainer may have a different view, after all.

Jan