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 11:40 AM Julien Grall <julien@xxxxxxx> wrote:
>
> 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.

Got it, thank you.

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