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

[Julien Grall <julien@xxxxxxx>]

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.
> +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.
Did you add this check just to follow the specification, or is there any 
other problem in Xen?
> +            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.
Cheers,

--
Julien Grall