Re: [PATCH v12 12/13] xen/arm: Add vPSCI SYSTEM_SUSPEND policy

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Bertrand,

Thank you for the review.

On Mon, Sep 28, 2026 at 7:20 PM Bertrand Marquis
<Bertrand.Marquis@xxxxxxx> wrote:
>
> Hi Mykola,
>
> > On 27 Aug 2026, at 16:32, Mykola Kvach <mykola_kvach@xxxxxxxx> wrote:
> >
> > Introduce CONFIG_HAS_HWDOM_SYSTEM_SUSPEND as an architecture-selected
> > capability for platforms where the hardware domain can be parked with
> > SHUTDOWN_suspend without calling hwdom_shutdown().
> >
> > Expose PSCI SYSTEM_SUSPEND as a vPSCI operation for all domains. For
> > non-control domains, including the hardware domain when it is not acting
> > as a control domain, the call is handled as a guest/domain suspend request
> > and parks the domain in SHUTDOWN_suspend.
> >
> > Control domains need additional sequencing because their SYSTEM_SUSPEND
> > request is used to coordinate host-wide suspend. A non-last awake control
> > domain may be parked in SHUTDOWN_suspend without requiring the host
> > suspend path to be available. The last awake control domain is treated as
> > the point where the request becomes a host-suspend request, and it may
> > only proceed when all non-control domains are already in SHUTDOWN_suspend
> > and the host suspend path is available.
> >
> > Keep the control-domain sequencing and domain-readiness checks out of
> > PSCI_FEATURES. They are per-attempt runtime conditions rather than stable
> > PSCI function availability. Advertise SYSTEM_SUSPEND as implemented by
> > vPSCI and report attempt-time policy failures as PSCI_DENIED.
> >
> > Select HAS_HWDOM_SYSTEM_SUSPEND independently from CONFIG_SYSTEM_SUSPEND
> > so that SHUTDOWN_suspend from the hardware domain can be treated as a
> > domain suspend state rather than as a hardware-domain initiated host
> > shutdown. This does not by itself imply that host-wide suspend is
> > available.
> >
> > Add host_system_suspend_allowed() to combine the host PSCI SYSTEM_SUSPEND
> > capability with runtime blockers reported by Xen-owned subsystems. Add
> > runtime blockers for registered serial, IOMMU, GIC and SMMUv3 MSI IRQ
> > paths lacking suspend/resume support. These blockers are runtime based,
> > so they only apply to drivers or paths that Xen actually uses on the
> > platform. For SMMUv3, the blocker applies only when Xen actually uses the
> > MSI IRQ path, since resume does not restore the SMMU *_IRQ_CFGn MSI
> > registers yet.
> >
> > Add a struct domain forward declaration to xen/suspend.h so the generic
> > header can expose arch_domain_resume() without requiring a full domain.h
> > include.
> >
> > Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>
> > Reviewed-by: Oleksandr Tyshchenko <Oleksandr_Tyshchenko@xxxxxxxx>
> > ---
> > Changes in V12:
> > - handle missing is_shut_down, change checking to call of
> >  domain_shutdown_completed
> >
> > Changes in V11:
> > - Mark host_system_suspend_runtime_allowed as __ro_after_init.
> > - Avoid printing the SMMUv3 MSI IRQ host suspend blocker more than once
> >  when multiple SMMUv3 instances use MSIs.
> > - Wrap the Arm IOMMU host suspend blocker in CONFIG_SYSTEM_SUSPEND to make
> >  its policy-only use explicit.
> >
> > Changes in V10:
> > - Return PSCI_DENIED rather than PSCI_NOT_SUPPORTED when the last awake
> >  control domain cannot proceed to host suspend, keeping PSCI_FEATURES
> >  stable once SYSTEM_SUSPEND is advertised.
> > - Shorten SYSTEM_SUSPEND blocker messages and use %pd when logging the
> >  control domain.
> > - Mark serial_suspend_available as __ro_after_init.
> > - Mention the struct domain forward declaration added to xen/suspend.h.
> >
> > Changes in V9:
> > - Select HAS_HWDOM_SYSTEM_SUSPEND independently from CONFIG_SYSTEM_SUSPEND
> >  so that hardware-domain SHUTDOWN_suspend support is not tied to
> >  host-wide system suspend availability.
> > - Add runtime host suspend blockers for Xen-owned subsystems lacking
> >  suspend/resume support.
> > - Keep vPSCI SYSTEM_SUSPEND advertised through PSCI_FEATURES and enforce
> >  control-domain sequencing in the call handler.
> > ---
> > xen/arch/arm/Kconfig                  |   1 +
> > xen/arch/arm/gic.c                    |   6 ++
> > xen/arch/arm/include/asm/psci.h       |   3 +
> > xen/arch/arm/include/asm/suspend.h    |  10 ++-
> > xen/arch/arm/psci.c                   |   7 ++
> > xen/arch/arm/suspend.c                |  40 +++++++++
> > xen/arch/arm/vpsci.c                  | 114 +++++++++++++++++++++++---
> > xen/common/Kconfig                    |   3 +
> > xen/common/domain.c                   |   7 +-
> > xen/drivers/char/serial.c             |  12 +++
> > xen/drivers/passthrough/arm/iommu.c   |   6 ++
> > xen/drivers/passthrough/arm/smmu-v3.c |   9 ++
> > xen/include/xen/serial.h              |   1 +
> > xen/include/xen/suspend.h             |   2 +
> > 14 files changed, 208 insertions(+), 13 deletions(-)
> >
> > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> > index 843a43897e..9027aa17eb 100644
> > --- a/xen/arch/arm/Kconfig
> > +++ b/xen/arch/arm/Kconfig
> > @@ -19,6 +19,7 @@ config ARM
> > select HAS_ALTERNATIVE if HAS_VMAP
> > select HAS_DEVICE_TREE_DISCOVERY
> > select HAS_DOM0LESS
> > + select HAS_HWDOM_SYSTEM_SUSPEND if !MPU
> > select HAS_GRANT_CACHE_FLUSH if GRANT_TABLE
> > select HAS_STACK_PROTECTOR
> > select HAS_STATIC_MEMORY
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index ffc11f36a1..0695474432 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -26,6 +26,7 @@
> > #include <asm/device.h>
> > #include <asm/io.h>
> > #include <asm/gic.h>
> > +#include <asm/suspend.h>
> > #include <asm/vgic.h>
> > #include <asm/acpi.h>
> >
> > @@ -44,6 +45,11 @@ static void __init __maybe_unused build_assertions(void)
> > void register_gic_ops(const struct gic_hw_operations *ops)
> > {
> >     gic_hw_ops = ops;
> > +
> > +#ifdef CONFIG_SYSTEM_SUSPEND
> > +    if ( !ops->suspend || !ops->resume )
> > +        host_system_suspend_disable("GIC driver lacks suspend support");
> > +#endif
> > }
> >
> > static void clear_cpu_lr_mask(void)
> > diff --git a/xen/arch/arm/include/asm/psci.h 
> > b/xen/arch/arm/include/asm/psci.h
> > index bb3c73496e..142fa1bfe5 100644
> > --- a/xen/arch/arm/include/asm/psci.h
> > +++ b/xen/arch/arm/include/asm/psci.h
> > @@ -24,6 +24,9 @@ void call_psci_cpu_off(void);
> > void call_psci_system_off(void);
> > void call_psci_system_reset(void);
> > int call_psci_system_suspend(void);
> > +#ifdef CONFIG_SYSTEM_SUSPEND
> > +bool psci_system_suspend_allowed(void);
> > +#endif
> >
> > /* Range of allocated PSCI function numbers */
> > #define PSCI_FNUM_MIN_VALUE                 _AC(0,U)
> > diff --git a/xen/arch/arm/include/asm/suspend.h 
> > b/xen/arch/arm/include/asm/suspend.h
> > index c848fc6340..50dc6e9fdf 100644
> > --- a/xen/arch/arm/include/asm/suspend.h
> > +++ b/xen/arch/arm/include/asm/suspend.h
> > @@ -39,7 +39,15 @@ extern struct resume_cpu_context resume_cpu_context;
> >
> > int prepare_resume_ctx(void);
> > void hyp_resume(void);
> > -#endif /* CONFIG_SYSTEM_SUSPEND */
> > +bool host_system_suspend_allowed(void);
> > +void host_system_suspend_disable(const char *reason);
> > +
> > +#else /* !CONFIG_SYSTEM_SUSPEND */
> > +
> > +static inline bool host_system_suspend_allowed(void) { return false; }
> > +static inline void host_system_suspend_disable(const char *reason) {}
> > +
> > +#endif
> >
> > #endif /* ARM_SUSPEND_H */
> >
> > diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
> > index e05dae1133..e9d78668fd 100644
> > --- a/xen/arch/arm/psci.c
> > +++ b/xen/arch/arm/psci.c
> > @@ -41,6 +41,13 @@ static bool __ro_after_init has_psci_system_suspend;
> >
> > #define PSCI_RET(res)   ((int32_t)(res).a0)
> >
> > +#ifdef CONFIG_SYSTEM_SUSPEND
> > +bool psci_system_suspend_allowed(void)
> > +{
> > +    return has_psci_system_suspend;
> > +}
> > +#endif
> > +
> > int call_psci_cpu_on(int cpu)
> > {
> >     struct arm_smccc_res res;
> > diff --git a/xen/arch/arm/suspend.c b/xen/arch/arm/suspend.c
> > index 6ea4a0f9cc..c7c26bcf03 100644
> > --- a/xen/arch/arm/suspend.c
> > +++ b/xen/arch/arm/suspend.c
> > @@ -1,9 +1,49 @@
> > /* SPDX-License-Identifier: GPL-2.0-only */
> >
> > +#include <asm/psci.h>
> > #include <asm/suspend.h>
> >
> > +#include <xen/lib.h>
> > +#include <xen/serial.h>
> > +
> > struct resume_cpu_context resume_cpu_context;
> >
> > +/*
> > + * Non-PSCI infrastructure can make host suspend impossible even when the 
> > PSCI
> > + * SYSTEM_SUSPEND conduit is present, e.g. when a Xen-owned driver has no 
> > valid
> > + * suspend/resume path.
> > + *
> > + * This gate is checked only when the last awake control domain attempts to
> > + * turn a guest SYSTEM_SUSPEND request into a host-suspend request.
> > + */
> > +static bool __ro_after_init host_system_suspend_runtime_allowed = true;
> > +
> > +static bool host_serial_suspend_allowed(void)
> > +{
> > +    if ( serial_suspend_supported() )
> > +        return true;
> > +
> > +    printk_once(XENLOG_INFO
> > +                "Host SYSTEM_SUSPEND blocked: serial unsupported\n");
> > +
> > +    return false;
> > +}
> > +
> > +bool host_system_suspend_allowed(void)
> > +{
> > +    return psci_system_suspend_allowed() &&
> > +           host_serial_suspend_allowed() &&
> > +           host_system_suspend_runtime_allowed;
> > +}
> > +
> > +void host_system_suspend_disable(const char *reason)
> > +{
> > +    host_system_suspend_runtime_allowed = false;
> > +
> > +    printk(XENLOG_INFO "Host SYSTEM_SUSPEND blocked: %s\n",
> > +           reason ? reason : "unsupported suspend/resume path");
> > +}
> > +
> > /*
> >  * Local variables:
> >  * mode: C
> > diff --git a/xen/arch/arm/vpsci.c b/xen/arch/arm/vpsci.c
> > index ac6af6118f..a41355d75d 100644
> > --- a/xen/arch/arm/vpsci.c
> > +++ b/xen/arch/arm/vpsci.c
> > @@ -5,6 +5,7 @@
> >
> > #include <asm/current.h>
> > #include <asm/domain.h>
> > +#include <asm/suspend.h>
> > #include <asm/vgic.h>
> > #include <asm/vpsci.h>
> > #include <asm/event.h>
> > @@ -219,6 +220,89 @@ static void do_psci_0_2_system_reset(void)
> >     domain_shutdown(d,SHUTDOWN_reboot);
> > }
> >
> > +/*
> > + * Serialise SYSTEM_SUSPEND policy decisions with the domain suspend 
> > transition,
> > + * so multiple control domains cannot all observe each other as still 
> > awake.
> > + */
> > +static DEFINE_SPINLOCK(vpsci_system_suspend_lock);
> > +
> > +static bool domain_in_suspend_state(struct domain *d)
> > +{
> > +    bool suspended;
> > +
> > +    spin_lock(&d->shutdown_lock);
> > +    suspended = domain_shutdown_completed(d) && (d->shutdown_code == 
> > SHUTDOWN_suspend);
>
> This lines is over 80 chars and should be broken down.

Ack.

>
> > +    spin_unlock(&d->shutdown_lock);
> > +
> > +    return suspended;
> > +}
> > +
> > +static int32_t domain_psci_system_suspend_policy(struct domain *d)
> > +{
> > +    struct domain *other;
> > +    bool last_awake_control_domain = true;
> > +    bool awake_non_control_domain = false;
> > +
> > +    /* Only control domains participate in sequencing policy. */
> > +    if ( !is_control_domain(d) )
> > +        return 0;
>
> I am wondering what would happen in a dom0less setup with no control
> domain if the domains request SYSTEM_SUSPEND.
>
> Could you explain how they would be resumed?
> Should we deny SYSTEM_SUSPEND requests in this case?

In the current implementation, a domain in SHUTDOWN_suspend needs
an explicit XEN_DOMCTL_resumedomain request to resume, for example
through "xl resume". A guest interrupt does not resume it.

This series relies on a control domain to coordinate resume,
including the order of backend and frontend domains. It does not
provide autonomous guest wakeup in a dom0less setup without a
control domain.

A separate policy could support dom0less systems with independent
guests. Each guest could resume on its own wake interrupt. Xen
could enter host suspend once all guests have completed suspend
and the platform is ready, then resume on a platform-supported
wake event. Such a mode could coexist with managed resume, but
it is not implemented in this series.

So yes, in v13 I will return PSCI_DENIED when a non-control domain
requests SYSTEM_SUSPEND and no control domain exists.
PSCI_FEATURES will remain unchanged.

This only checks that a control domain exists. It does not check
whether that domain has permission to resume this guest or will
remain available (it may be crashed, dying, or suspended).
The actual resumedomain request is still checked by XSM.

Best regards,
Mykola