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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Julien,

On Sun, Aug 24, 2025 at 12:26 AM Julien Grall <julien@xxxxxxx> wrote:
>
> Hi Mykola,
>
> On 18/08/2025 09:49, Mykola Kvach wrote:
> > From: Mykola Kvach <mykola_kvach@xxxxxxxx>
> >
> > This patch adds support for the PSCI SYSTEM_SUSPEND function in the vPSCI
> > (virtual PSCI) interface, allowing guests to request suspend via the PSCI
> > v1.0 SYSTEM_SUSPEND call (both 32-bit and 64-bit variants).
> >
> > The implementation:
> > - Adds SYSTEM_SUSPEND function IDs to PSCI definitions
> > - Implements trapping and handling of SYSTEM_SUSPEND in vPSCI
> > - Allows only non-hardware domains to invoke SYSTEM_SUSPEND; for the
> >    hardware domain, PSCI_NOT_SUPPORTED is returned to avoid halting the
> >    system in hwdom_shutdown() called from domain_shutdown
> > - Ensures all secondary VCPUs of the calling domain are offline before
> >    allowing suspend due to PSCI spec
> >
> > GIC and virtual timer context must be saved when the domain suspends.
>
> Can you expand a bit more on this? Is this a requirement from the Arm
> Arm? If so, please specify the specification so we can easily check.
>
> > This is done by moving the respective code in ctxt_switch_from
> > before the return that happens if the domain suspended.
>
>  From the commit message, it is unclear why we want to skip the save part.

I'll add extra information to the commit message.

>
> [...]
>
> > ---
> >   xen/arch/arm/domain.c                 |  17 +++--
> >   xen/arch/arm/include/asm/perfc_defn.h |   1 +
> >   xen/arch/arm/include/asm/psci.h       |   2 +
> >   xen/arch/arm/include/asm/vpsci.h      |   2 +-
> >   xen/arch/arm/vpsci.c                  | 101 ++++++++++++++++++++++----
> >   xen/common/domain.c                   |  31 ++++++--
> >   xen/include/xen/sched.h               |   6 ++
> >   7 files changed, 131 insertions(+), 29 deletions(-)
> >
> > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> > index 310c578909..9e9649c4e2 100644
> > --- a/xen/arch/arm/domain.c
> > +++ b/xen/arch/arm/domain.c
> > @@ -90,6 +90,16 @@ static void ctxt_switch_from(struct vcpu *p)
> >       if ( is_idle_vcpu(p) )
> >           return;
> >
>
> It would be good to have a comment explaining what (and why) we need to
> save only partially the state while the VCPU is suspended.

Actually, nothing bad happens if we save everything except SCTLR_EL1,
which is modified in PSCI SYSTEM_SUSPEND. The only downside might be
losing some CPU cycles.

If we talk just about GIC and the Arch timer state, the relevant guidance
can be found in the Power State Coordination Interface
(DEN0022F.b, 6.8 Preserving the execution context).

Even if the guest saves the context it has access to and restores it on
resume, we only need to save the transient state. Looks like we can
do nothing with the arch timer here. And gic save function can be called
from PSCI SYSTEM_SUSPEND.

>
> > +    /* Arch timer */
> > +    p->arch.cntkctl = READ_SYSREG(CNTKCTL_EL1);
> > +    virt_timer_save(p);
> > +
> > +    /* VGIC */
> > +    gic_save_state(p);
> > +
> > +    if ( test_bit(_VPF_suspended, &p->pause_flags) )
> > +        return;
>
> Can you explain why we don't need an isb()?
>
> > +
> >       p2m_save_state(p);
> At least part of p2m_save_state() can't be skipped because it is
> applying a workaround on platform where AT mistakenly speculate.

Thank you for pointing that out, we need it here.

>
> [...]
>
> >
> > +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;
> > +
> > +    /* 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;
> > +
> > +    rc = do_setup_vcpu_ctx(current, epoint, cid);
> > +    if ( rc != PSCI_SUCCESS )
> > +    {
> > +        domain_resume_nopause(d);
> > +        return rc;
> > +    }
> > +
> > +    set_bit(_VPF_suspended, &current->pause_flags);
> > +
> > +    dprintk(XENLOG_DEBUG,
>
> I think you should use gdprintk() which will print the domain for you as
> well as appropriately ratelimit the message.
>
> That said, I would consider using gprintk() so the message can also be
> printed in a debug build.

Got it, I will use gprintk() instead as suggested. Thank you for the
recommendation.

>
> > +            "Dom %u: SYSTEM_SUSPEND requested, epoint=%#lx, cid=%#lx\n",
>
> For both of them you technically want to use PRIregister rather than lx.

Got it, I will use PRIregister instead as suggested. Thank you for the
recommendation.

>
> Lastly, we prefer to use %pd when printing a domain. The argument is
> 'd'. But this should not be necessary if you use gprintk().

Ok.

>
>
> > +            d->domain_id, 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 +267,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 +399,24 @@ 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 ( is_64bit_domain(current->domain) &&
>
> Shouldn't this be removed so the check also apply for 32-bit domain on
> 64-bit system?

AFAIK, this question was already addressed in a previous version of
this patch series:
https://patchew.org/Xen/cover.1751020456.git.mykola._5Fkvach@xxxxxxxx/072270e0940b6bcc2743d56a336363f4719ba60a.1751020456.git.mykola._5Fkvach@xxxxxxxx/#7070f416-119c-49f8-acd0-82c6e31f0fc6@xxxxxxx

>
> > +             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 5241a1629e..624c3e2c27 100644
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -1343,16 +1343,13 @@ int domain_shutdown(struct domain *d, u8 reason)
> >       return 0;
> >   }
> >
> > -void domain_resume(struct domain *d)
> > +#ifndef CONFIG_ARM
> > +static
> > +#endif
>
> I am not sure who suggests this but personally, I dislike using
> CONFIG_<ARCH> in common code. I also think this is worse for hiding the
> "static" keyword.
>
> For the latter, I think it would be better to provide a separate helper
> that can be #ifdef.

Will do. I will provide a separate helper as suggested.


>
> [...]
>
> Cheers,
>
> --
> Julien Grall
>

Best regards,
Mykola