Re: [PATCH 1/6] xen/arm: Fix evaluation of parameters for SMCCC calls

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Andrew,

> On 31 Aug 2026, at 14:19, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
> 
> Contrary to what was claimed in commit 67bcf5eae709 ("xen/arm: Simplify type
> handling for SMCCC declarations"), there is an important reason to retain the
> intermediate variable.  It is unsafe to have any logic between the assignment
> of the register variabes and the asm() block they're used in.
> 
> This logically reverts commit 67bcf5eae709 ("xen/arm: Simplify type handling
> for SMCCC declarations") while retaining the conversions from commit
> 7f15d5d13221 ("xen/treewide: More typeof() -> auto conversions").
> 
> Adjust __declare_arg_0() to match.  It happens to be safe because it's the
> first register expression once all macros are expanded, but it really should
> be consistent with the others.
> 
> Leave a comment explaining why they must be written like this.
> 
> Fixes: 67bcf5eae709 ("xen/arm: Simplify type handling for SMCCC declarations")
> Reported-by: Jan Setje-Eilers <Jan.SetjeEilers@xxxxxxxxxx>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Code looks good and my tests are passing:

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

Cheers
Bertrand

> ---
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: Julien Grall <julien@xxxxxxx>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> CC: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> CC: Michal Orzel <michal.orzel@xxxxxxx>
> CC: Jan Setje-Eilers <Jan.SetjeEilers@xxxxxxxxxx>
> ---
> xen/arch/arm/include/asm/smccc.h | 31 +++++++++++++++++++++++--------
> 1 file changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/include/asm/smccc.h 
> b/xen/arch/arm/include/asm/smccc.h
> index 62c6985e7315..53cdddb690b7 100644
> --- a/xen/arch/arm/include/asm/smccc.h
> +++ b/xen/arch/arm/include/asm/smccc.h
> @@ -108,37 +108,52 @@ struct arm_smccc_res {
> #define __constraint_read_6 __constraint_read_5, "r" (arg6)
> #define __constraint_read_7 __constraint_read_6, "r" (arg7)
> 
> +/*
> + * Macro arguments MUST be evaluated before being assigned to a register
> + * variable.
> + *
> + * This is manual register scheduling for the asm() statement, and any other
> + * logic to evaluate may clobber the already-scheduled registers.
> + */
> #define __declare_arg_0(a0, res)                            \
> +    auto __a0 = (uint32_t)(a0);                             \
>     struct arm_smccc_res    *___res = (res);                \
> -    register unsigned long  arg0 ASM_REG(0) = (uint32_t)(a0)
> +    register unsigned long  arg0 ASM_REG(0) = __a0
> 
> #define __declare_arg_1(a0, a1, res)                        \
> +    auto __a1 = (a1);                                       \
>     __declare_arg_0(a0, res);                               \
> -    register auto           arg1 ASM_REG(1) = (a1)
> +    register auto           arg1 ASM_REG(1) = __a1
> 
> #define __declare_arg_2(a0, a1, a2, res)                    \
> +    auto __a2 = (a2);                                       \
>     __declare_arg_1(a0, a1, res);                           \
> -    register auto           arg2 ASM_REG(2) = (a2)
> +    register auto           arg2 ASM_REG(2) = __a2
> 
> #define __declare_arg_3(a0, a1, a2, a3, res)                \
> +    auto __a3 = (a3);                                       \
>     __declare_arg_2(a0, a1, a2, res);                       \
> -    register auto           arg3 ASM_REG(3) = (a3)
> +    register auto           arg3 ASM_REG(3) = __a3
> 
> #define __declare_arg_4(a0, a1, a2, a3, a4, res)        \
> +    auto __a4 = (a4);                                   \
>     __declare_arg_3(a0, a1, a2, a3, res);               \
> -    register auto           arg4 ASM_REG(4) = (a4)
> +    register auto           arg4 ASM_REG(4) = __a4
> 
> #define __declare_arg_5(a0, a1, a2, a3, a4, a5, res)    \
> +    auto __a5 = (a5);                                   \
>     __declare_arg_4(a0, a1, a2, a3, a4, res);           \
> -    register auto           arg5 ASM_REG(5) = (a5)
> +    register auto           arg5 ASM_REG(5) = __a5
> 
> #define __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res)    \
> +    auto __a6 = (a6);                                       \
>     __declare_arg_5(a0, a1, a2, a3, a4, a5, res);           \
> -    register auto           arg6 ASM_REG(6) = (a6)
> +    register auto           arg6 ASM_REG(6) = __a6
> 
> #define __declare_arg_7(a0, a1, a2, a3, a4, a5, a6, a7, res)    \
> +    auto __a7 = (a7);                                           \
>     __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res);           \
> -    register auto           arg7 ASM_REG(7) = (a7)
> +    register auto           arg7 ASM_REG(7) = __a7
> 
> #define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__)
> #define __declare_args(count, ...)  ___declare_args(count, __VA_ARGS__)
> 
> base-commit: 79225a0c77e13b693b4d2b903a88289704b79db6
> -- 
> 2.39.5
>