Re: [PATCH v2] xen/arm: vpsci: ignore upper 32 bits for SMC32 PSCI arguments

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Mykola,

> On 23 Mar 2026, at 12:11, Mykola Kvach <xakep.amatop@xxxxxxxxx> wrote:
> 
> From: Mykola Kvach <mykola_kvach@xxxxxxxx>
> 
> SMCCC DEN0028G, section 3.1, states that for AArch64 SMC/HVC calls
> using Wn, only the least significant 32 bits are significant and the
> upper 32 bits must be ignored by the implementation.
> 
> So for SMC32 PSCI calls, Xen must not treat non-zero upper bits in the
> argument registers as an error. Instead, they should be discarded when
> decoding the arguments.
> 
> Arm ARM DDI 0487J.a (D1-5406) also notes that the upper 32 bits may be
> implementation defined when entering from AArch32. Xen zeros them on
> entry, but that guarantee is only relevant for 32-bit domains.
> 
> Update PSCI v0.2+ CPU_ON, CPU_SUSPEND and AFFINITY_INFO to read SMC32
> arguments via PSCI_ARG32(), while keeping the SMC64 handling unchanged.
> 
> No functional change is intended for PSCI 0.1.
> 
> Suggested-by: Julien Grall <julien@xxxxxxx>
> Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>

Reviewed-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>

PS: this could be treated as a bug fix but as no implementation we know of has
been falling into this error, i do not think it would be needed to backport 
this.

Cheers
Bertrand

> ---
> v2:
> - introduce PSCI_ARG_CONV() to centralize convention-dependent argument
>   decoding for PSCI v0.2+ calls;
> - use smccc_is_conv_64(fid) instead of open-coding per-call SMC32 checks;
> - keep PSCI 0.1 handling unchanged, except switch on the already-decoded
>   fid instead of re-reading x0/r0.
> 
> Link to discussion: 
> https://patchew.org/Xen/cover.1751020456.git.mykola._5Fkvach@xxxxxxxx/072270e0940b6bcc2743d56a336363f4719ba60a.1751020456.git.mykola._5Fkvach@xxxxxxxx/#7070f416-119c-49f8-acd0-82c6e31f0fc6@xxxxxxx
> ---
> xen/arch/arm/vpsci.c | 19 ++++++++++++-------
> 1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/arm/vpsci.c b/xen/arch/arm/vpsci.c
> index 7ba9ccd94b..65dea5cf6c 100644
> --- a/xen/arch/arm/vpsci.c
> +++ b/xen/arch/arm/vpsci.c
> @@ -230,13 +230,16 @@ static int32_t do_psci_1_0_features(uint32_t 
> psci_func_id)
> #define PSCI_ARG32(reg, n) PSCI_ARG(reg, n)
> #endif
> 
> +#define PSCI_ARG_CONV(reg, n, conv_64) \
> +    ((conv_64) ? PSCI_ARG(reg, n) : PSCI_ARG32(reg, n))
> +
> /*
>  * PSCI 0.1 calls. It will return false if the function ID is not
>  * handled.
>  */
> bool do_vpsci_0_1_call(struct cpu_user_regs *regs, uint32_t fid)
> {
> -    switch ( (uint32_t)get_user_reg(regs, 0) )
> +    switch ( fid )
>     {
>     case PSCI_cpu_off:
>     {
> @@ -271,6 +274,8 @@ bool do_vpsci_0_2_call(struct cpu_user_regs *regs, 
> uint32_t fid)
>      * adding/removing a function. SSSC_SMCCC_*_REVISION should be
>      * updated once per release.
>      */
> +    bool is_conv_64 = smccc_is_conv_64(fid);
> +
>     switch ( fid )
>     {
>     case PSCI_0_2_FN32_PSCI_VERSION:
> @@ -303,9 +308,9 @@ bool do_vpsci_0_2_call(struct cpu_user_regs *regs, 
> uint32_t fid)
>     case PSCI_0_2_FN32_CPU_ON:
>     case PSCI_0_2_FN64_CPU_ON:
>     {
> -        register_t vcpuid = PSCI_ARG(regs, 1);
> -        register_t epoint = PSCI_ARG(regs, 2);
> -        register_t cid = PSCI_ARG(regs, 3);
> +        register_t vcpuid = PSCI_ARG_CONV(regs, 1, is_conv_64);
> +        register_t epoint = PSCI_ARG_CONV(regs, 2, is_conv_64);
> +        register_t cid = PSCI_ARG_CONV(regs, 3, is_conv_64);
> 
>         perfc_incr(vpsci_cpu_on);
>         PSCI_SET_RESULT(regs, do_psci_0_2_cpu_on(vcpuid, epoint, cid));
> @@ -316,8 +321,8 @@ bool do_vpsci_0_2_call(struct cpu_user_regs *regs, 
> uint32_t fid)
>     case PSCI_0_2_FN64_CPU_SUSPEND:
>     {
>         uint32_t pstate = PSCI_ARG32(regs, 1);
> -        register_t epoint = PSCI_ARG(regs, 2);
> -        register_t cid = PSCI_ARG(regs, 3);
> +        register_t epoint = PSCI_ARG_CONV(regs, 2, is_conv_64);
> +        register_t cid = PSCI_ARG_CONV(regs, 3, is_conv_64);
> 
>         perfc_incr(vpsci_cpu_suspend);
>         PSCI_SET_RESULT(regs, do_psci_0_2_cpu_suspend(pstate, epoint, cid));
> @@ -327,7 +332,7 @@ bool do_vpsci_0_2_call(struct cpu_user_regs *regs, 
> uint32_t fid)
>     case PSCI_0_2_FN32_AFFINITY_INFO:
>     case PSCI_0_2_FN64_AFFINITY_INFO:
>     {
> -        register_t taff = PSCI_ARG(regs, 1);
> +        register_t taff = PSCI_ARG_CONV(regs, 1, is_conv_64);
>         uint32_t laff = PSCI_ARG32(regs, 2);
> 
>         perfc_incr(vpsci_cpu_affinity_info);
> -- 
> 2.43.0
>