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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Jan,

Thank you for the review.

On Wed, May 13, 2026 at 9:54 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 12.05.2026 19:07, Mykola Kvach wrote:
> > From: Mykola Kvach <mykola_kvach@xxxxxxxx>
> >
> > 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
> > enforce the sequencing policy in the call handler.
> >
> > 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.
> >
> > Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>
> > ---
> >  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   |   4 +
> >  xen/drivers/passthrough/arm/smmu-v3.c |   4 +
> >  xen/include/xen/serial.h              |   1 +
> >  xen/include/xen/suspend.h             |   2 +
> >  14 files changed, 201 insertions(+), 13 deletions(-)
> >
>
> Contrary to what the cover letter says, there's no revlog here.

Right, I should have added a short note for this patch as well.

This is a new patch in this version of the series, so there was no previous
version of this particular patch to compare against, but I agree that this
should still have been mentioned explicitly, e.g. as "New in v9". I will add
that in the next version.

>
> > --- 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 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 driver lacks 
> > suspend/resume support\n");
>
> Please try to keep log messages down to a reasonable size. In the case here,
> what value does "suspend/resume" add?

Fair point. The important part is that the serial driver cannot support the
host suspend path. I will shorten the message somehow.

>
> > +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;
> > +
> > +    rcu_read_lock(&domlist_read_lock);
> > +
> > +    for_each_domain ( other )
> > +    {
> > +        bool suspended;
> > +
> > +        if ( other == d )
> > +            continue;
> > +
> > +        suspended = domain_in_suspend_state(other);
> > +        if ( suspended )
> > +            continue;
> > +
> > +        if ( is_control_domain(other) )
> > +        {
> > +            last_awake_control_domain = false;
> > +            break;
> > +        }
> > +
> > +        awake_non_control_domain = true;
> > +    }
> > +
> > +    rcu_read_unlock(&domlist_read_lock);
> > +
> > +    /*
> > +     * Another control domain is still awake. This request is only the 
> > first
> > +     * phase of the sequencing: park this control domain and leave the host
> > +     * running. Host-wide suspend gates must not block this intermediate 
> > state.
> > +     */
> > +    if ( !last_awake_control_domain )
> > +        return 0;
> > +
> > +    /*
> > +     * This is the last awake control domain. It must not be parked unless 
> > the
> > +     * request can proceed as a host-suspend request; otherwise Xen would 
> > lose
> > +     * the last domain that can coordinate the system suspend.
> > +     */
> > +    if ( awake_non_control_domain )
> > +    {
> > +        printk(XENLOG_DEBUG
> > +               "SYSTEM_SUSPEND denied: last awake control domain dom%u 
> > requested host suspend while non-control domains are still awake\n",
> > +               d->domain_id);
>
> Same here, plus please use %pd.

Ack, I will shorten the message and use %pd.

>
> > --- a/xen/drivers/char/serial.c
> > +++ b/xen/drivers/char/serial.c
> > @@ -497,6 +497,8 @@ const struct vuart_info *serial_vuart_info(int idx)
> >
> >  #ifdef CONFIG_SYSTEM_SUSPEND
> >
> > +static bool __read_mostly serial_suspend_available = true;
>
> __ro_after_init?

Good catch, yes.
I will use __ro_after_init for this variable.

Best regards,
Mykola

>
> Jan