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

[Julien Grall <julien@xxxxxxx>]

Hi,

On 02/07/2025 11:01, Mykola Kvach wrote:
> On Sat, Jun 28, 2025 at 9:17 PM Julien Grall <julien@xxxxxxx> wrote:
> > It would be better if we stash the value sand then update the registers.
> > Another possibility would be to entirely skip the save path for CPUs
> > that are turned off (after all this is a bit useless work...).
> Do you mean we should avoid calling ctxt_switch_from for a suspended
> domain?
This would be one way to handle it. I haven't looked in details whether 
there are any other implications.
[...]

> > > +{
> > > +    struct vcpu *v;
> > > +
> > > +    spin_lock(&d->shutdown_lock);
> > > +
> > > +    d->is_shutting_down = d->is_shut_down = 0;
> > > +    d->shutdown_code = SHUTDOWN_CODE_INVALID;
> > > +
> > > +    for_each_vcpu ( d, v )
> > > +    {
> > > +        if ( v->paused_for_shutdown )
> > > +            vcpu_unpause(v);
> > > +        v->paused_for_shutdown = 0;
> > > +    }
> > > +
> > > +    spin_unlock(&d->shutdown_lock);
> > > +}
> > > +
> > > +static int32_t do_psci_1_0_system_suspend(register_t epoint, register_t cid)
> > > +{
> > > +    int ret;
> > > +    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;
> > > +
> > > +    domain_shutdown(d, SHUTDOWN_suspend);
> > It would be good to add a comment explaining why you need to call
> > domain_shutdown() first. Otherwise, one could wonder whether we can get
> > rid of the complexity when a vCPU is still online.
> It's done first here because domain_shutdown() handles pausing of the
> vCPUs internally, and this allows us to securely check whether the vCPUs
> are online or not without interference from the guest.
>
> But you're probably right — a better solution might be to perform proper
> checking of which vCPUs are still online before calling it.
To clarify, I think this is right to call domain_shutdown() first to 
avoid any race when checking which vCPUs are still online (see the 
discussion we had in the previous version). My point is it would be good 
to document it in the code because this is not obvious.

> > > +        if ( v != current && is_vcpu_online(v) )
> > > +        {
> > > +            do_resume_on_error(d);
> > > +            return PSCI_DENIED;
> > > +        }
> > > +
> > > +    ret = do_setup_vcpu_ctx(current, epoint, cid);
> > > +    if ( ret != PSCI_SUCCESS )
> > > +        do_resume_on_error(d);
> > > +
> > > +    return ret;
> > > +}
> > > +
> > >    static int32_t do_psci_1_0_features(uint32_t psci_func_id)
> > >    {
> > >        /* /!\ Ordered by function ID and not name */
> > > @@ -214,6 +271,8 @@ static int32_t do_psci_1_0_features(uint32_t psci_func_id)
> > >        case PSCI_0_2_FN32_SYSTEM_OFF:
> > >        case PSCI_0_2_FN32_SYSTEM_RESET:
> > >        case PSCI_1_0_FN32_PSCI_FEATURES:
> > > +    case PSCI_1_0_FN32_SYSTEM_SUSPEND:
> > > +    case PSCI_1_0_FN64_SYSTEM_SUSPEND:
> > >        case ARM_SMCCC_VERSION_FID:
> > >            return 0;
> > >        default:
> > > @@ -344,6 +403,17 @@ bool do_vpsci_0_2_call(struct cpu_user_regs *regs, 
> > > uint32_t fid)
> > >            return true;
> > >        }
> > >
> > > +    case PSCI_1_0_FN32_SYSTEM_SUSPEND:
> > > +    case PSCI_1_0_FN64_SYSTEM_SUSPEND:
> > > +    {
> > > +        register_t epoint = PSCI_ARG(regs,1);
> > > +        register_t cid = PSCI_ARG(regs,2);
> > Coding style: For the two lines above, there is a missing space after ",".
> Sure, will fix — thanks for pointing that out.
>
> > Also, if a 64-bit domain is calling the 32-bit version, then we also
> > need to ignore the top 32-bits. AFAICT CPU_ON also have the same issue.
> > I am not going to ask fixing CPU_ON (it would be good though), but I
> > will at least ask we don't spread the mistake further.
> Maybe it would be better to return an error in this case?
Why should we return an error? This is valid for a 64-bit domain to use 
SMC32 convention.
> Should I also add checks for the case where the guest OS is 32-bit but
> tries to call the 64-bit version of SYSTEM_SUSPEND?
We already have this generic check at the beginning of vsmccc_handle_call().

Cheers,

--
Julien Grall