[Xen-devel,1/6] xen/arm: smccc-1.1: Make return values unsigned long

Message ID 20180824165820.32620-2-julien.grall@arm.com
State Superseded
Headers show
Series
  • xen/arm: SMCCC fixup and improvement
Related show

Commit Message

Julien Grall Aug. 24, 2018, 4:58 p.m.
From: Marc Zyngier <marc.zyngier@arm.com>

An unfortunate consequence of having a strong typing for the input
values to the SMC call is that it also affects the type of the
return values, limiting r0 to 32 bits and r{1,2,3} to whatever
was passed as an input.

Let's turn everything into "unsigned long", which satisfies the
requirements of both architectures, and allows for the full
range of return values.

Reported-by: Stefano Stabellini <stefanos@xilinx.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/include/asm-arm/smccc.h | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

Comments

Volodymyr Babchuk Aug. 27, 2018, 2:03 p.m. | #1
Hi  Julien, Marc

On 24.08.18 19:58, Julien Grall wrote:
> From: Marc Zyngier <marc.zyngier@arm.com>
> 
> An unfortunate consequence of having a strong typing for the input
> values to the SMC call is that it also affects the type of the
> return values, limiting r0 to 32 bits and r{1,2,3} to whatever
> was passed as an input. >
> Let's turn everything into "unsigned long", which satisfies the
> requirements of both architectures, and allows for the full
> range of return values.
Maybe it better to use register_t then? By definition register_t has the 
same size as a CPU register and SMC uses CPU registers to pass 
parameters/return values.

> Reported-by: Stefano Stabellini <stefanos@xilinx.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>   xen/include/asm-arm/smccc.h | 22 +++++++++++-----------
>   1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
> index 74c13f8419..a31d67a1de 100644
> --- a/xen/include/asm-arm/smccc.h
> +++ b/xen/include/asm-arm/smccc.h
> @@ -119,35 +119,35 @@ struct arm_smccc_res {
>   
>   #define __declare_arg_0(a0, res)                        \
>       struct arm_smccc_res    *___res = res;              \
> -    register uin32_t        r0 asm("r0") = a0;          \
> +    register unsigned long  r0 asm("r0") = (uint32_t)a0;\
Do you really want to silently drop upper 32 bits of the argument?
I know, that SMCCC states that function id is a 32-bit value,
but I don't think that it is a good place to enforce this
behavior.
I think it is better to allow user to shoot in his leg in this case.

>       register unsigned long  r1 asm("r1");               \
>       register unsigned long  r2 asm("r2");               \
>       register unsigned long  r3 asm("r3")
>   
>   #define __declare_arg_1(a0, a1, res)                    \
>       struct arm_smccc_res    *___res = res;              \
> -    register uint32_t       r0 asm("r0") = a0;          \
> -    register typeof(a1)     r1 asm("r1") = a1;          \
> +    register unsigned long  r0 asm("r0") = (uint32_t)a0;\
> +    register unsigned long  r1 asm("r1") = a1;          \
>       register unsigned long  r2 asm("r2");               \
>       register unsigned long  r3 asm("r3")
>   
>   #define __declare_arg_2(a0, a1, a2, res)                \
>       struct arm_smccc_res    *___res = res;				\
> -    register u32            r0 asm("r0") = a0;          \
> -    register typeof(a1)     r1 asm("r1") = a1;          \
> -    register typeof(a2)     r2 asm("r2") = a2;          \
> +    register unsigned long  r0 asm("r0") = (uint32_t)a0;\
> +    register unsigned long  r1 asm("r1") = a1;          \
> +    register unsigned long  r2 asm("r2") = a2;          \
>       register unsigned long  r3 asm("r3")
>   
>   #define __declare_arg_3(a0, a1, a2, a3, res)            \
>       struct arm_smccc_res    *___res = res;              \
> -    register u32            r0 asm("r0") = a0;          \
> -    register typeof(a1)     r1 asm("r1") = a1;          \
> -    register typeof(a2)     r2 asm("r2") = a2;          \
> -    register typeof(a3)     r3 asm("r3") = a3
> +    register unsigned long  r0 asm("r0") = (uint32_t)a0;\
> +    register unsigned long  r1 asm("r1") = a1;          \
> +    register unsigned long  r2 asm("r2") = a2;          \
> +    register unsigned long  r3 asm("r3") = a3
>   
>   #define __declare_arg_4(a0, a1, a2, a3, a4, res)        \
>       __declare_arg_3(a0, a1, a2, a3, res);               \
> -    register typeof(a4) r4 asm("r4") = a4
> +    register unsigned long r4 asm("r4") = a4
>   
>   #define __declare_arg_5(a0, a1, a2, a3, a4, a5, res)    \
>       __declare_arg_4(a0, a1, a2, a3, a4, res);           \
>
Julien Grall Aug. 27, 2018, 3:23 p.m. | #2
On 27/08/2018 15:03, Volodymyr Babchuk wrote:
> Hi  Julien, Marc

Hi,

> On 24.08.18 19:58, Julien Grall wrote:
>> From: Marc Zyngier <marc.zyngier@arm.com>
>>
>> An unfortunate consequence of having a strong typing for the input
>> values to the SMC call is that it also affects the type of the
>> return values, limiting r0 to 32 bits and r{1,2,3} to whatever
>> was passed as an input. >
>> Let's turn everything into "unsigned long", which satisfies the
>> requirements of both architectures, and allows for the full
>> range of return values.
> Maybe it better to use register_t then? By definition register_t has the 
> same size as a CPU register and SMC uses CPU registers to pass 
> parameters/return values.

The code is based on Linux series (posted last Friday). I don't much 
want to diverge too much from it. So unsigned "unsigned long" here is ok.

> 
>> Reported-by: Stefano Stabellini <stefanos@xilinx.com>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> ---
>>   xen/include/asm-arm/smccc.h | 22 +++++++++++-----------
>>   1 file changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
>> index 74c13f8419..a31d67a1de 100644
>> --- a/xen/include/asm-arm/smccc.h
>> +++ b/xen/include/asm-arm/smccc.h
>> @@ -119,35 +119,35 @@ struct arm_smccc_res {
>>   #define __declare_arg_0(a0, res)                        \
>>       struct arm_smccc_res    *___res = res;              \
>> -    register uin32_t        r0 asm("r0") = a0;          \
>> +    register unsigned long  r0 asm("r0") = (uint32_t)a0;\
> Do you really want to silently drop upper 32 bits of the argument?
> I know, that SMCCC states that function id is a 32-bit value,
> but I don't think that it is a good place to enforce this
> behavior.
> I think it is better to allow user to shoot in his leg in this case.

I don't see any issue with casting here. This is what the SMCCC request 
for x0 and after all you silently drop upper 32-bit when using a static 
inline function...

Cheers,
Volodymyr Babchuk Aug. 27, 2018, 4:33 p.m. | #3
Julien,

On 27.08.18 18:23, Julien Grall wrote:
> 
> 
> On 27/08/2018 15:03, Volodymyr Babchuk wrote:
>> Hi  Julien, Marc
> 
> Hi,
> 
>> On 24.08.18 19:58, Julien Grall wrote:
>>> From: Marc Zyngier <marc.zyngier@arm.com>
>>>
>>> An unfortunate consequence of having a strong typing for the input
>>> values to the SMC call is that it also affects the type of the
>>> return values, limiting r0 to 32 bits and r{1,2,3} to whatever
>>> was passed as an input. >
>>> Let's turn everything into "unsigned long", which satisfies the
>>> requirements of both architectures, and allows for the full
>>> range of return values.
>> Maybe it better to use register_t then? By definition register_t has 
>> the same size as a CPU register and SMC uses CPU registers to pass 
>> parameters/return values.
> 
> The code is based on Linux series (posted last Friday). I don't much 
> want to diverge too much from it. So unsigned "unsigned long" here is ok.
> 
>>
>>> Reported-by: Stefano Stabellini <stefanos@xilinx.com>
>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>> ---
>>>   xen/include/asm-arm/smccc.h | 22 +++++++++++-----------
>>>   1 file changed, 11 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
>>> index 74c13f8419..a31d67a1de 100644
>>> --- a/xen/include/asm-arm/smccc.h
>>> +++ b/xen/include/asm-arm/smccc.h
>>> @@ -119,35 +119,35 @@ struct arm_smccc_res {
>>>   #define __declare_arg_0(a0, res)                        \
>>>       struct arm_smccc_res    *___res = res;              \
>>> -    register uin32_t        r0 asm("r0") = a0;          \
>>> +    register unsigned long  r0 asm("r0") = (uint32_t)a0;\
>> Do you really want to silently drop upper 32 bits of the argument?
>> I know, that SMCCC states that function id is a 32-bit value,
>> but I don't think that it is a good place to enforce this
>> behavior.
>> I think it is better to allow user to shoot in his leg in this case.
> 
> I don't see any issue with casting here. This is what the SMCCC request 
> for x0 and after all you silently drop upper 32-bit when using a static 
> inline function...
> 

Yes, you are right.

Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

Patch

diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
index 74c13f8419..a31d67a1de 100644
--- a/xen/include/asm-arm/smccc.h
+++ b/xen/include/asm-arm/smccc.h
@@ -119,35 +119,35 @@  struct arm_smccc_res {
 
 #define __declare_arg_0(a0, res)                        \
     struct arm_smccc_res    *___res = res;              \
-    register uin32_t        r0 asm("r0") = a0;          \
+    register unsigned long  r0 asm("r0") = (uint32_t)a0;\
     register unsigned long  r1 asm("r1");               \
     register unsigned long  r2 asm("r2");               \
     register unsigned long  r3 asm("r3")
 
 #define __declare_arg_1(a0, a1, res)                    \
     struct arm_smccc_res    *___res = res;              \
-    register uint32_t       r0 asm("r0") = a0;          \
-    register typeof(a1)     r1 asm("r1") = a1;          \
+    register unsigned long  r0 asm("r0") = (uint32_t)a0;\
+    register unsigned long  r1 asm("r1") = a1;          \
     register unsigned long  r2 asm("r2");               \
     register unsigned long  r3 asm("r3")
 
 #define __declare_arg_2(a0, a1, a2, res)                \
     struct arm_smccc_res    *___res = res;				\
-    register u32            r0 asm("r0") = a0;          \
-    register typeof(a1)     r1 asm("r1") = a1;          \
-    register typeof(a2)     r2 asm("r2") = a2;          \
+    register unsigned long  r0 asm("r0") = (uint32_t)a0;\
+    register unsigned long  r1 asm("r1") = a1;          \
+    register unsigned long  r2 asm("r2") = a2;          \
     register unsigned long  r3 asm("r3")
 
 #define __declare_arg_3(a0, a1, a2, a3, res)            \
     struct arm_smccc_res    *___res = res;              \
-    register u32            r0 asm("r0") = a0;          \
-    register typeof(a1)     r1 asm("r1") = a1;          \
-    register typeof(a2)     r2 asm("r2") = a2;          \
-    register typeof(a3)     r3 asm("r3") = a3
+    register unsigned long  r0 asm("r0") = (uint32_t)a0;\
+    register unsigned long  r1 asm("r1") = a1;          \
+    register unsigned long  r2 asm("r2") = a2;          \
+    register unsigned long  r3 asm("r3") = a3
 
 #define __declare_arg_4(a0, a1, a2, a3, a4, res)        \
     __declare_arg_3(a0, a1, a2, a3, res);               \
-    register typeof(a4) r4 asm("r4") = a4
+    register unsigned long r4 asm("r4") = a4
 
 #define __declare_arg_5(a0, a1, a2, a3, a4, a5, res)    \
     __declare_arg_4(a0, a1, a2, a3, a4, res);           \