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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi, @Julien Grall

On Wed, Jun 4, 2025 at 2:00 AM Julien Grall <julien@xxxxxxx> wrote:
>
> Hi Mykola,
>
> On 27/05/2025 10:18, 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
> > - Treats suspend as a "standby" operation: the domain is shut down with
> >    SHUTDOWN_suspend, and resumes execution at the instruction following
> >    the call
>
> Looking at the specification, I am still not convinced you can implement
> PSCI SUSPEND as a NOP. For instance, in the section "5.1.19
> SYSTEM_SUSPEND", the wording implies the call cannot return when it is
> successul.
>
> I understand that 5.20.2 ("Caller reponsabilities" for SYSTEM_SUSPEND)
> suggests the caller should apply all the rules from 5.4 ("Caller
> responsabilties" for CPU_SUSPEND), but it is also mentioned that
> SYSTEM_SUSPEND behave as the deepest power down state.
>
> So I don't think standby is an option. I would like an opinion from the
> other maintainers.

Sure, let's discuss this with the others.

>
> > +static int32_t do_psci_1_0_system_suspend(register_t epoint, register_t 
> > cid)
>  > +{> +    struct vcpu *v;
> > +    struct domain *d = current->domain;
> > +
> > +    /* Drop this check once SYSTEM_SUSPEND is supported in hardware domain 
> > */
> > +    if ( is_hardware_domain(d) )
> > +        return PSCI_NOT_SUPPORTED;
> > +
> > +    /* Ensure that all CPUs other than the calling one are offline */
> > +    for_each_vcpu ( d, v )
> > +    {
> > +        if ( v != current && is_vcpu_online(v) )
>
> I think this is racy because you can still turn on a vCPU afterwards
> from a vCPU you haven't checked.
>

I'll think about how to protect against such cases.
Thank you for pointing that out.

> Did you add this check just to follow the specification, or is there any
> other problem in Xen?

Yes, it's just to comply with the specification — at least,
I've never seen PSCI_DENIED triggered because of this check.
It's a leftover from a previous patch series.

>
> > +            return PSCI_DENIED;
>  > +    }> +
> > +    /*
> > +     * System suspend requests are treated as performing standby
> > +     * as this simplifies Xen implementation.
> > +     *
> > +     * Arm Power State Coordination Interface (DEN0022F.b)
>
> This comment is a bit too verbose. There is no need to copy/paste the
> specification. You can just write a couple of sentence with link to the
> specification.

Got it, I'll revise the comment accordingly.

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

Best regards,
Mykola