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 V13:
- use __has_include plus static inline function instead
  of arch_domain_resume stubs

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/suspend.h    |  18 ++++
 xen/arch/arm/include/asm/vpsci.h      |   5 +-
 xen/arch/arm/vpsci.c                  | 116 +++++++++++++++++++++-----
 xen/common/domain.c                   |   9 ++
 xen/include/xen/domain.h              |  11 +++
 9 files changed, 183 insertions(+), 22 deletions(-)
 create mode 100644 xen/arch/arm/include/asm/suspend.h

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/suspend.h b/xen/arch/arm/include/asm/suspend.h
new file mode 100644
index 0000000000..c8ea84bd40
--- /dev/null
+++ b/xen/arch/arm/include/asm/suspend.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef __ASM_ARM_SUSPEND_H__
+#define __ASM_ARM_SUSPEND_H__
+
+int arch_domain_resume(struct domain *d);
+
+#endif /* __ASM_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 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..63b81f8fba 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -8,6 +8,10 @@
 
 #include <public/xen.h>
 
+#if __has_include(<asm/suspend.h>)
+#include <asm/suspend.h>
+#endif
+
 struct guest_area {
     struct page_info *pg;
     void *map;
@@ -109,6 +113,13 @@ int arch_domain_soft_reset(struct domain *d);
 
 void arch_domain_creation_finished(struct domain *d);
 
+#if !__has_include(<asm/suspend.h>)
+static inline int arch_domain_resume(struct domain *d)
+{
+    return 0;
+}
+#endif /* !__has_include(<asm/suspend.h>) */
+
 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

Hi Mykola,

On 02/09/2025 10:03, Mykola Kvach wrote:
> @@ -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;

I know this is v13, but I don't think we should use typeof() is in this 
context. "struct resume_info" is much shorter and help the review (I 
don't have to grep resume_ctx to figure out the type).

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

I think this odd to warn if something is expected. It would be best to 
use "XENLOG_INFO".

> +     */
> +    if ( !ctx->wake_cpu )
> +    {
> +        dprintk(XENLOG_WARNING, "%pd: Invalid wake CPU pointer for resume\n",

As you wrote above, there is nothing wrong. So "Invalid" is not correct. 
I think a better wording is "Wake CPU pointer context was not provided").

Also note that dprintk() will be a NOP in release build. I am guessing 
this is intended?

I will answer you Jan's comment separately. The rest looks good to me.

Cheers,

-- 
Julien Grall

Hi Julien,

Thank you for reviewing this patch series and for your valuable feedback.

On Sat, Sep 13, 2025 at 1:38 AM Julien Grall <julien@xen.org> wrote:
>
> Hi Mykola,
>
> On 02/09/2025 10:03, Mykola Kvach wrote:
> > @@ -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;
>
> I know this is v13, but I don't think we should use typeof() is in this
> context. "struct resume_info" is much shorter and help the review (I
> don't have to grep resume_ctx to figure out the type).

Ack

>
> > +
> > +    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.
>
> I think this odd to warn if something is expected. It would be best to
> use "XENLOG_INFO".

Ack.

>
> > +     */
> > +    if ( !ctx->wake_cpu )
> > +    {
> > +        dprintk(XENLOG_WARNING, "%pd: Invalid wake CPU pointer for resume\n",
>
> As you wrote above, there is nothing wrong. So "Invalid" is not correct.
> I think a better wording is "Wake CPU pointer context was not provided").

Thanks for the suggestion. I'll change the message.

>
> Also note that dprintk() will be a NOP in release build. I am guessing
> this is intended?

Yep. In any case, the status is checked after the call to arch_domain_resume
(see domain_resume), so we notify the user about the situation. Detailed
prints are only available in debug builds.

>
> I will answer you Jan's comment separately. The rest looks good to me.
>
> Cheers,
>
> --
> Julien Grall
>

Best regards,
Mykola

On 02.09.2025 11:03, Mykola Kvach wrote:
> ---
>  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/suspend.h    |  18 ++++
>  xen/arch/arm/include/asm/vpsci.h      |   5 +-
>  xen/arch/arm/vpsci.c                  | 116 +++++++++++++++++++++-----
>  xen/common/domain.c                   |   9 ++
>  xen/include/xen/domain.h              |  11 +++
>  9 files changed, 183 insertions(+), 22 deletions(-)
>  create mode 100644 xen/arch/arm/include/asm/suspend.h

Note, btw, how this way you won't need x86, ppc, and riscv acks.

> --- a/xen/include/xen/domain.h
> +++ b/xen/include/xen/domain.h
> @@ -8,6 +8,10 @@
>  
>  #include <public/xen.h>
>  
> +#if __has_include(<asm/suspend.h>)
> +#include <asm/suspend.h>
> +#endif
> +
>  struct guest_area {
>      struct page_info *pg;
>      void *map;
> @@ -109,6 +113,13 @@ int arch_domain_soft_reset(struct domain *d);
>  
>  void arch_domain_creation_finished(struct domain *d);
>  
> +#if !__has_include(<asm/suspend.h>)
> +static inline int arch_domain_resume(struct domain *d)
> +{
> +    return 0;
> +}
> +#endif /* !__has_include(<asm/suspend.h>) */
> +
>  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);

Imo it would be preferable to have such in a single #if/#else. There's nothing
wrong with an #include not sitting at the very top.

(Another question is whether to put this in xen/domain.h at all. There could
be a xen/suspend.h having - for now at least - just this and nothing else.)

Jan

On Tue, Sep 2, 2025 at 6:43 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 02.09.2025 11:03, Mykola Kvach wrote:
> > ---
> >  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/suspend.h    |  18 ++++
> >  xen/arch/arm/include/asm/vpsci.h      |   5 +-
> >  xen/arch/arm/vpsci.c                  | 116 +++++++++++++++++++++-----
> >  xen/common/domain.c                   |   9 ++
> >  xen/include/xen/domain.h              |  11 +++
> >  9 files changed, 183 insertions(+), 22 deletions(-)
> >  create mode 100644 xen/arch/arm/include/asm/suspend.h
>
> Note, btw, how this way you won't need x86, ppc, and riscv acks.

Thanks for suggesting this approach. Hopefully, the other stub functions
can be updated in a similar manner.

>
> > --- a/xen/include/xen/domain.h
> > +++ b/xen/include/xen/domain.h
> > @@ -8,6 +8,10 @@
> >
> >  #include <public/xen.h>
> >
> > +#if __has_include(<asm/suspend.h>)
> > +#include <asm/suspend.h>
> > +#endif
> > +
> >  struct guest_area {
> >      struct page_info *pg;
> >      void *map;
> > @@ -109,6 +113,13 @@ int arch_domain_soft_reset(struct domain *d);
> >
> >  void arch_domain_creation_finished(struct domain *d);
> >
> > +#if !__has_include(<asm/suspend.h>)
> > +static inline int arch_domain_resume(struct domain *d)
> > +{
> > +    return 0;
> > +}
> > +#endif /* !__has_include(<asm/suspend.h>) */
> > +
> >  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);
>
> Imo it would be preferable to have such in a single #if/#else. There's nothing
> wrong with an #include not sitting at the very top.

I understand that includes can be placed near where something from the
header is used. However, I find it more natural to keep them together
in a single location.

>
> (Another question is whether to put this in xen/domain.h at all. There could
> be a xen/suspend.h having - for now at least - just this and nothing else.)

With this approach, I don’t need to move the include to the middle of
the file.

>
> Jan

Best regards,
Mykola

Hi,

On 02/09/2025 18:55, Mykola Kvach wrote:
>>> --- a/xen/include/xen/domain.h
>>> +++ b/xen/include/xen/domain.h
>>> @@ -8,6 +8,10 @@
>>>
>>>   #include <public/xen.h>
>>>
>>> +#if __has_include(<asm/suspend.h>)
>>> +#include <asm/suspend.h>
>>> +#endif
>>> +
>>>   struct guest_area {
>>>       struct page_info *pg;
>>>       void *map;
>>> @@ -109,6 +113,13 @@ int arch_domain_soft_reset(struct domain *d);
>>>
>>>   void arch_domain_creation_finished(struct domain *d);
>>>
>>> +#if !__has_include(<asm/suspend.h>)
>>> +static inline int arch_domain_resume(struct domain *d)
>>> +{
>>> +    return 0;
>>> +}
>>> +#endif /* !__has_include(<asm/suspend.h>) */
>>> +
>>>   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);
>>
>> Imo it would be preferable to have such in a single #if/#else. There's nothing
>> wrong with an #include not sitting at the very top.
> 
> I understand that includes can be placed near where something from the
> header is used. However, I find it more natural to keep them together
> in a single location.

It is not always possible to keep all includes together. I also have a 
slight preference to Jan suggestion because we don't have multiple "#if 
__has_include(<asm/suspend.h>)" which I find rather ugly but necessary.

> 
>>
>> (Another question is whether to put this in xen/domain.h at all. There could
>> be a xen/suspend.h having - for now at least - just this and nothing else.)
> 
> With this approach, I don’t need to move the include to the middle of
> the file.

A new suspend.h file would also work for me.

Cheers,

-- 
Julien Grall

On Sat, Sep 13, 2025 at 1:43 AM Julien Grall <julien@xen.org> wrote:
>
> Hi,
>
> On 02/09/2025 18:55, Mykola Kvach wrote:
> >>> --- a/xen/include/xen/domain.h
> >>> +++ b/xen/include/xen/domain.h
> >>> @@ -8,6 +8,10 @@
> >>>
> >>>   #include <public/xen.h>
> >>>
> >>> +#if __has_include(<asm/suspend.h>)
> >>> +#include <asm/suspend.h>
> >>> +#endif
> >>> +
> >>>   struct guest_area {
> >>>       struct page_info *pg;
> >>>       void *map;
> >>> @@ -109,6 +113,13 @@ int arch_domain_soft_reset(struct domain *d);
> >>>
> >>>   void arch_domain_creation_finished(struct domain *d);
> >>>
> >>> +#if !__has_include(<asm/suspend.h>)
> >>> +static inline int arch_domain_resume(struct domain *d)
> >>> +{
> >>> +    return 0;
> >>> +}
> >>> +#endif /* !__has_include(<asm/suspend.h>) */
> >>> +
> >>>   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);
> >>
> >> Imo it would be preferable to have such in a single #if/#else. There's nothing
> >> wrong with an #include not sitting at the very top.
> >
> > I understand that includes can be placed near where something from the
> > header is used. However, I find it more natural to keep them together
> > in a single location.
>
> It is not always possible to keep all includes together. I also have a
> slight preference to Jan suggestion because we don't have multiple "#if
> __has_include(<asm/suspend.h>)" which I find rather ugly but necessary.
>
> >
> >>
> >> (Another question is whether to put this in xen/domain.h at all. There could
> >> be a xen/suspend.h having - for now at least - just this and nothing else.)
> >
> > With this approach, I don’t need to move the include to the middle of
> > the file.
>
> A new suspend.h file would also work for me.

Ack.

>
> Cheers,
>
> --
> Julien Grall
>

Best regards,
Mykola