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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

On Wed, Jul 2, 2025 at 3:28 PM Julien Grall <julien@xxxxxxx> wrote:
>
> 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.

got it

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

I mean — in that case, is it possible that the upper 32 bits are set to
non-zero values without it being an explicit error from the guest?

In my code, the macro used to extract 64-bit values (on 64-bit Xen, of
course) just copies values from the Xn registers directly.

According to the SMC Calling Convention specification:
"System Software on ARM Platforms" (ARM DEN 0028A), we must use Wn
for SMC32 parameters in AArch64.

AFAIK, writing to Wn zeroes the top 32 bits of Xn. So, if the guest
is properly using 32-bit values for arguments, the upper bits will already
be zeroed.

If the guest deliberately writes to the full Xn register --
perhaps to simplify handling of both SMC32 and SMC64, it may set
non-zero upper bits. In that case, I believe this is a guest error.


Finally, from the same document, section 3.1 "Register use in AArch64
SMC calls":

    The same architectural registers, R0–R7, are used for the two AArch64
    calling conventions, SMC32 and SMC64.
    The working size of the register is identified by its name:
        Xn: All 64 bits used.
        Wn: Lower 32 bits used, upper 32 bits are zero.

So, I think we should not ignore the upper 32 bits and should instead
treat non-zero values as an error when handling SMC32 calls from AArch64.
Alternatively, we could simply do nothing and expect the guest to follow
the SMC Calling Convention specification and fill the registers
accordingly.


Best regards,
Mykola

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