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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Michal,

On Fri, Jun 13, 2025 at 10:53 AM Orzel, Michal <michal.orzel@xxxxxxx> wrote:
>
>
>
> On 06/06/2025 05:52, Mykola Kvach wrote:
> > 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.
> My understanding of the spec is that SYSTEM_SUSPEND is equivalent to 
> CPU_SUSPEND
> *for the deepest possible powerdown* state. CPU_SUSPEND can be implemented as
> standby or powerdown, but the SYSTEM_SUSPEND only mentions powerdown state
> (which is the true deepest state). Therefore I don't think standby could apply
> to SYSTEM_SUSPEND and we could simply ignore the entry point address passed 
> by OS.

Thank you for your feedback.

I agree with your and Julien's suggestions.
I will revert the behavior to the previous implementation, as proposed.

Best regards,
Mykola

>
> ~Michal
>
> >
> >>
> >>> +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
>