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

["Orzel, Michal" <michal.orzel@xxxxxxx>]

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.

~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