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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

On Fri, Jun 6, 2025 at 6:52 AM Mykola Kvach <xakep.amatop@xxxxxxxxx> 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.
>
> >
> > > +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.

Is that actually possible in this context? If suspend is successful, we pause
all present vCPUs (including the current one), and control is not returned to
the guest until either resume or an error occurs in this function. Since this
runs as part of a trap, the current function can't be preempted.

Also, from the use of _VPF_down (via is_vcpu_online) on Arm, it looks like
guest requests are what manipulate this bit, which further limits what can
happen concurrently.

Maybe I missed something—if so, please share some details on how this could
happen.

Note: It looks like the same behavior is present for VCPUOP_down as well.

>
> > 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