Re: [PATCH] xen/arm: smccc: Preserve argument types in v1.1 SMC

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Jan,

> On 16 Feb 2026, at 10:21, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> On 16.02.2026 08:47, Bertrand Marquis wrote:
>> The SMCCC v1.1 inline helper currently forces a1-a4 into
>> unsigned long and uses in/out constraints for r0-r3. In
>> contrast, a5-a7 are passed with their original types via
>> read-only constraints. On arm64 this means a 32-bit signed
>> value in a1-a4 is converted to a 64-bit unsigned value, while
>> the same value in a5-a7 keeps its signed 32-bit form. For
>> example, a negative int in a2 is widened to unsigned long, but
>> a negative int in a5 is passed as a 32-bit signed value, so the
>> SMC sees different encodings depending on argument position.
>> 
>> Switch the helper to use typed input registers arg0-arg7
>> derived from the call arguments (keeping a0 cast to u32) and
>> separate output registers r0-r3. This preserves argument types
>> consistently across all positions. Argument evaluation order
>> is unchanged, so we do not reintroduce the issue fixed in
>> "e00dc325bd9e" ("xen/arm: smccc-1.1: Handle function result as
>> parameters").
>> 
>> This also aligns Xen's SMCCC parameter handling with Linux's type-
>> preserving behavior (same externally visible argument handling,
>> independent implementation) to avoid surprising differences
>> between a1-a4 and a5-a7.
>> 
>> Current callers (PSCI, SCMI, platform SMC pass-through, OP-TEE,
>> and exynos5) pass unsigned values; exynos5 passes an int CPU id
>> which should always be > 0.
> 
> Reported-by: Andrew ?

Ack, sorry forgot that.

> 
>> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
>> ---
>> xen/arch/arm/include/asm/smccc.h | 69 +++++++++++++-------------------
>> 1 file changed, 27 insertions(+), 42 deletions(-)
>> 
>> diff --git a/xen/arch/arm/include/asm/smccc.h 
>> b/xen/arch/arm/include/asm/smccc.h
>> index 441b3ab65dee..5b30dd57b69d 100644
>> --- a/xen/arch/arm/include/asm/smccc.h
>> +++ b/xen/arch/arm/include/asm/smccc.h
>> @@ -99,87 +99,68 @@ struct arm_smccc_res {
>> #define __count_args(...)                               \
>>     ___count_args(__VA_ARGS__, 7, 6, 5, 4, 3, 2, 1, 0)
>> 
>> -#define __constraint_write_0                        \
>> -    "+r" (r0), "=&r" (r1), "=&r" (r2), "=&r" (r3)
>> -#define __constraint_write_1                        \
>> -    "+r" (r0), "+r" (r1), "=&r" (r2), "=&r" (r3)
>> -#define __constraint_write_2                        \
>> -    "+r" (r0), "+r" (r1), "+r" (r2), "=&r" (r3)
>> -#define __constraint_write_3                        \
>> -    "+r" (r0), "+r" (r1), "+r" (r2), "+r" (r3)
>> -#define __constraint_write_4    __constraint_write_3
>> -#define __constraint_write_5    __constraint_write_4
>> -#define __constraint_write_6    __constraint_write_5
>> -#define __constraint_write_7    __constraint_write_6
>> -
>> -#define __constraint_read_0
>> -#define __constraint_read_1
>> -#define __constraint_read_2
>> -#define __constraint_read_3
>> -#define __constraint_read_4 "r" (r4)
>> -#define __constraint_read_5 __constraint_read_4, "r" (r5)
>> -#define __constraint_read_6 __constraint_read_5, "r" (r6)
>> -#define __constraint_read_7 __constraint_read_6, "r" (r7)
>> +#define __constraint_read_0 "r" (arg0)
>> +#define __constraint_read_1 __constraint_read_0, "r" (arg1)
>> +#define __constraint_read_2 __constraint_read_1, "r" (arg2)
>> +#define __constraint_read_3 __constraint_read_2, "r" (arg3)
>> +#define __constraint_read_4 __constraint_read_3, "r" (arg4)
>> +#define __constraint_read_5 __constraint_read_4, "r" (arg5)
>> +#define __constraint_read_6 __constraint_read_5, "r" (arg6)
>> +#define __constraint_read_7 __constraint_read_6, "r" (arg7)
>> 
>> #define __declare_arg_0(a0, res)                            \
>>     struct arm_smccc_res    *___res = (res);                \
>> -    register unsigned long  r0 ASM_REG(0) = (uint32_t)(a0); \
>> -    register unsigned long  r1 ASM_REG(1);                  \
>> -    register unsigned long  r2 ASM_REG(2);                  \
>> -    register unsigned long  r3 ASM_REG(3)
>> +    register unsigned long  arg0 ASM_REG(0) = (uint32_t)(a0)
>> 
>> #define __declare_arg_1(a0, a1, res)                        \
>>     typeof(a1) __a1 = (a1);                                 \
>>     struct arm_smccc_res    *___res = (res);                \
>> -    register unsigned long  r0 ASM_REG(0) = (uint32_t)(a0); \
>> -    register unsigned long  r1 ASM_REG(1) = __a1;           \
>> -    register unsigned long  r2 ASM_REG(2);                  \
>> -    register unsigned long  r3 ASM_REG(3)
>> +    register unsigned long  arg0 ASM_REG(0) = (uint32_t)(a0);\
>> +    register typeof(a1)     arg1 ASM_REG(1) = __a1
> 
> Is it intentional that you switch to typeof() rather than directly going
> to auto? This was it'll be more churn, aiui. And if deliberately going
> only half a step, perhaps worth saying so in the description?
> 

Yes it is because Andrew wants to rebase his serie on top of this
patch.

Cheers
Bertrand

> Jan