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

[Julien Grall <julien@xxxxxxx>]

Hi Mykola,

On 06/06/2025 09:26, Mykola Kvach wrote:
> 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.
AFAIU, this code is called before suspend is completed. At that point 
you don't know yet the state of the vCPUs. They may be scheduled on a 
different pCPU. At which point, they can issue PSCI_CPU_ON.
> 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.
I don't see how this help. See why above. So you will want to at least 
pause the domain before checking if all the vCPUs are offline.
> Note: It looks like the same behavior is present for VCPUOP_down as well.

From a brief look, I agree the same behavior is present there.

Cheers,

--
Julien Grall