diff mbox

[1/7] arm: traps: psci: use generic register accessors

Message ID 1502222922-25821-2-git-send-email-volodymyr_babchuk@epam.com (mailing list archive)
State New, archived
Headers show

Commit Message

Volodymyr Babchuk Aug. 8, 2017, 8:08 p.m. UTC 
From: Volodymyr Babchuk <vlad.babchuk@gmail.com>

There are standard functions set_user_reg() and get_user_reg(). We can
use them in PSCI_SET_RESULT()/PSCI_ARG() macroses instead of relying on
CONFIG_ARM_64 definition.

Signed-off-by: Volodymyr Babchuk <vlad.babchuk@gmail.com>
---
 xen/arch/arm/traps.c | 38 +++++++++++++++++---------------------
 1 file changed, 17 insertions(+), 21 deletions(-)

Comments

Andrew Cooper Aug. 8, 2017, 8:37 p.m. UTC | #1 
On 08/08/2017 21:08, Volodymyr Babchuk wrote:
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 6cf9ee7..ed78b36 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1449,13 +1449,12 @@ static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
>  }
>  #endif
>  
> +#define PSCI_SET_RESULT(reg, val) set_user_reg(reg, 0, val)
> +#define PSCI_ARG(reg,n) get_user_reg(reg, n)
> +
>  #ifdef CONFIG_ARM_64
> -#define PSCI_RESULT_REG(reg) (reg)->x0
> -#define PSCI_ARG(reg,n) (reg)->x##n
> -#define PSCI_ARG32(reg,n) (uint32_t)( (reg)->x##n & 0x00000000FFFFFFFF )
> +#define PSCI_ARG32(reg,n) (uint32_t)(get_user_reg(reg, n) & 0x00000000FFFFFFFF)

There is no need for the mask as well as the explicit (uint32_t) cast. 
I'd recommend dropping the mask entirely.

If you insist on keeping the mask, then it should be 0xffffffffu or
0x00000000ffffffffull to be compliant with the C standard (A pedantic
compiler will complain that the literal is out of range of int).

Also as you are changing all of these macros, it would be nice to apply
correct style to them, by inserting spaces after all the commas.

~Andrew

>  #else
> -#define PSCI_RESULT_REG(reg) (reg)->r0
> -#define PSCI_ARG(reg,n) (reg)->r##n
>  #define PSCI_ARG32(reg,n) PSCI_ARG(reg,n)
>  #endif
>  
>
Volodymyr Babchuk Aug. 8, 2017, 8:46 p.m. UTC | #2 
Hi Andrew

On 08.08.17 23:37, Andrew Cooper wrote:
> On 08/08/2017 21:08, Volodymyr Babchuk wrote:
>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> index 6cf9ee7..ed78b36 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -1449,13 +1449,12 @@ static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
>>   }
>>   #endif
>>   
>> +#define PSCI_SET_RESULT(reg, val) set_user_reg(reg, 0, val)
>> +#define PSCI_ARG(reg,n) get_user_reg(reg, n)
>> +
>>   #ifdef CONFIG_ARM_64
>> -#define PSCI_RESULT_REG(reg) (reg)->x0
>> -#define PSCI_ARG(reg,n) (reg)->x##n
>> -#define PSCI_ARG32(reg,n) (uint32_t)( (reg)->x##n & 0x00000000FFFFFFFF )
>> +#define PSCI_ARG32(reg,n) (uint32_t)(get_user_reg(reg, n) & 0x00000000FFFFFFFF)
> 
> There is no need for the mask as well as the explicit (uint32_t) cast.
> I'd recommend dropping the mask entirely.
Yes, I know this. But Julien asked me to keep this ([1])

> If you insist on keeping the mask, then it should be 0xffffffffu or
> 0x00000000ffffffffull to be compliant with the C standard (A pedantic
> compiler will complain that the literal is out of range of int).
> 
> Also as you are changing all of these macros, it would be nice to apply
> correct style to them, by inserting spaces after all the commas.
Yep, will do. Thanks.

> ~Andrew
> 
>>   #else
>> -#define PSCI_RESULT_REG(reg) (reg)->r0
>> -#define PSCI_ARG(reg,n) (reg)->r##n
>>   #define PSCI_ARG32(reg,n) PSCI_ARG(reg,n)
>>   #endif
>>   
>>
> 
[1] https://www.mail-archive.com/xen-devel@lists.xen.org/msg113198.html
Julien Grall Aug. 9, 2017, 9:52 a.m. UTC | #3 
On 08/08/17 21:37, Andrew Cooper wrote:
> On 08/08/2017 21:08, Volodymyr Babchuk wrote:
>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> index 6cf9ee7..ed78b36 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -1449,13 +1449,12 @@ static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
>>  }
>>  #endif
>>
>> +#define PSCI_SET_RESULT(reg, val) set_user_reg(reg, 0, val)
>> +#define PSCI_ARG(reg,n) get_user_reg(reg, n)
>> +
>>  #ifdef CONFIG_ARM_64
>> -#define PSCI_RESULT_REG(reg) (reg)->x0
>> -#define PSCI_ARG(reg,n) (reg)->x##n
>> -#define PSCI_ARG32(reg,n) (uint32_t)( (reg)->x##n & 0x00000000FFFFFFFF )
>> +#define PSCI_ARG32(reg,n) (uint32_t)(get_user_reg(reg, n) & 0x00000000FFFFFFFF)
>
> There is no need for the mask as well as the explicit (uint32_t) cast.
> I'd recommend dropping the mask entirely.

I want to avoid the implicit cast from 64-bit register to 32-bit that 
Volodymyr introduced in his series.

uint32_t pstate = get_user_reg(regs, 1);

IHMO this is a call to mistake. Another solution is to provide 3 helpers
	- get_user_reg32(...)
	- get_user_reg64(...)
	- get_user_reg(...) -> Return the full register (32-bit on ARM32, 
64-bit on ARM64).

This would at least document the return value of get_user_reg*.

Cheers,
Andrew Cooper Aug. 9, 2017, 11:12 a.m. UTC | #4 
On 09/08/17 10:52, Julien Grall wrote:
>
>
> On 08/08/17 21:37, Andrew Cooper wrote:
>> On 08/08/2017 21:08, Volodymyr Babchuk wrote:
>>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>>> index 6cf9ee7..ed78b36 100644
>>> --- a/xen/arch/arm/traps.c
>>> +++ b/xen/arch/arm/traps.c
>>> @@ -1449,13 +1449,12 @@ static void do_debug_trap(struct
>>> cpu_user_regs *regs, unsigned int code)
>>>  }
>>>  #endif
>>>
>>> +#define PSCI_SET_RESULT(reg, val) set_user_reg(reg, 0, val)
>>> +#define PSCI_ARG(reg,n) get_user_reg(reg, n)
>>> +
>>>  #ifdef CONFIG_ARM_64
>>> -#define PSCI_RESULT_REG(reg) (reg)->x0
>>> -#define PSCI_ARG(reg,n) (reg)->x##n
>>> -#define PSCI_ARG32(reg,n) (uint32_t)( (reg)->x##n &
>>> 0x00000000FFFFFFFF )
>>> +#define PSCI_ARG32(reg,n) (uint32_t)(get_user_reg(reg, n) &
>>> 0x00000000FFFFFFFF)
>>
>> There is no need for the mask as well as the explicit (uint32_t) cast.
>> I'd recommend dropping the mask entirely.
>
> I want to avoid the implicit cast from 64-bit register to 32-bit that
> Volodymyr introduced in his series.
>
> uint32_t pstate = get_user_reg(regs, 1);

This is how we'd expect code to look on the x86 side, but you are the
maintainer here, so have prerogative.

>
> IHMO this is a call to mistake. Another solution is to provide 3 helpers
>     - get_user_reg32(...)
>     - get_user_reg64(...)
>     - get_user_reg(...) -> Return the full register (32-bit on ARM32,
> 64-bit on ARM64).
>
> This would at least document the return value of get_user_reg*.

None of this is an explanation for having both an explicit uint32_t cast
and mask hidden inside PSCI_ARG32().  They are redundant.

~Andrew
Julien Grall Aug. 9, 2017, 11:43 a.m. UTC | #5 
Hi Andrew,

On 09/08/17 12:12, Andrew Cooper wrote:
> On 09/08/17 10:52, Julien Grall wrote:
>>
>>
>> On 08/08/17 21:37, Andrew Cooper wrote:
>>> On 08/08/2017 21:08, Volodymyr Babchuk wrote:
>>>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>>>> index 6cf9ee7..ed78b36 100644
>>>> --- a/xen/arch/arm/traps.c
>>>> +++ b/xen/arch/arm/traps.c
>>>> @@ -1449,13 +1449,12 @@ static void do_debug_trap(struct
>>>> cpu_user_regs *regs, unsigned int code)
>>>>  }
>>>>  #endif
>>>>
>>>> +#define PSCI_SET_RESULT(reg, val) set_user_reg(reg, 0, val)
>>>> +#define PSCI_ARG(reg,n) get_user_reg(reg, n)
>>>> +
>>>>  #ifdef CONFIG_ARM_64
>>>> -#define PSCI_RESULT_REG(reg) (reg)->x0
>>>> -#define PSCI_ARG(reg,n) (reg)->x##n
>>>> -#define PSCI_ARG32(reg,n) (uint32_t)( (reg)->x##n &
>>>> 0x00000000FFFFFFFF )
>>>> +#define PSCI_ARG32(reg,n) (uint32_t)(get_user_reg(reg, n) &
>>>> 0x00000000FFFFFFFF)
>>>
>>> There is no need for the mask as well as the explicit (uint32_t) cast.
>>> I'd recommend dropping the mask entirely.
>>
>> I want to avoid the implicit cast from 64-bit register to 32-bit that
>> Volodymyr introduced in his series.
>>
>> uint32_t pstate = get_user_reg(regs, 1);
>
> This is how we'd expect code to look on the x86 side, but you are the
> maintainer here, so have prerogative.

This is a bit error-prone. But less than passing directly the return as 
an argument. I.e

foo(get_user_reg(regs, 1))

And assuming foo will do the cast for you. My point of explicit size is 
avoid potential mis-usage in the code.

>
>>
>> IHMO this is a call to mistake. Another solution is to provide 3 helpers
>>     - get_user_reg32(...)
>>     - get_user_reg64(...)
>>     - get_user_reg(...) -> Return the full register (32-bit on ARM32,
>> 64-bit on ARM64).
>>
>> This would at least document the return value of get_user_reg*.
>
> None of this is an explanation for having both an explicit uint32_t cast
> and mask hidden inside PSCI_ARG32().  They are redundant.

If you are using a macro, you need to keep at least the cast to prevent 
a user misusing the variable. The other solution is a static inline as I 
suggested above.

Cheers,
diff mbox

Patch

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 6cf9ee7..ed78b36 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1449,13 +1449,12 @@  static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
 }
 #endif
 
+#define PSCI_SET_RESULT(reg, val) set_user_reg(reg, 0, val)
+#define PSCI_ARG(reg,n) get_user_reg(reg, n)
+
 #ifdef CONFIG_ARM_64
-#define PSCI_RESULT_REG(reg) (reg)->x0
-#define PSCI_ARG(reg,n) (reg)->x##n
-#define PSCI_ARG32(reg,n) (uint32_t)( (reg)->x##n & 0x00000000FFFFFFFF )
+#define PSCI_ARG32(reg,n) (uint32_t)(get_user_reg(reg, n) & 0x00000000FFFFFFFF)
 #else
-#define PSCI_RESULT_REG(reg) (reg)->r0
-#define PSCI_ARG(reg,n) (reg)->r##n
 #define PSCI_ARG32(reg,n) PSCI_ARG(reg,n)
 #endif
 
@@ -1470,14 +1469,14 @@  static void do_trap_psci(struct cpu_user_regs *regs)
     register_t fid = PSCI_ARG(regs,0);
 
     /* preloading in case psci_mode_check fails */
-    PSCI_RESULT_REG(regs) = PSCI_INVALID_PARAMETERS;
+    PSCI_SET_RESULT(regs, PSCI_INVALID_PARAMETERS);
     switch( fid )
     {
     case PSCI_cpu_off:
         {
             uint32_t pstate = PSCI_ARG32(regs,1);
             perfc_incr(vpsci_cpu_off);
-            PSCI_RESULT_REG(regs) = do_psci_cpu_off(pstate);
+            PSCI_SET_RESULT(regs, do_psci_cpu_off(pstate));
         }
         break;
     case PSCI_cpu_on:
@@ -1485,36 +1484,36 @@  static void do_trap_psci(struct cpu_user_regs *regs)
             uint32_t vcpuid = PSCI_ARG32(regs,1);
             register_t epoint = PSCI_ARG(regs,2);
             perfc_incr(vpsci_cpu_on);
-            PSCI_RESULT_REG(regs) = do_psci_cpu_on(vcpuid, epoint);
+            PSCI_SET_RESULT(regs, do_psci_cpu_on(vcpuid, epoint));
         }
         break;
     case PSCI_0_2_FN_PSCI_VERSION:
         perfc_incr(vpsci_version);
-        PSCI_RESULT_REG(regs) = do_psci_0_2_version();
+        PSCI_SET_RESULT(regs, do_psci_0_2_version());
         break;
     case PSCI_0_2_FN_CPU_OFF:
         perfc_incr(vpsci_cpu_off);
-        PSCI_RESULT_REG(regs) = do_psci_0_2_cpu_off();
+        PSCI_SET_RESULT(regs, do_psci_0_2_cpu_off());
         break;
     case PSCI_0_2_FN_MIGRATE_INFO_TYPE:
         perfc_incr(vpsci_migrate_info_type);
-        PSCI_RESULT_REG(regs) = do_psci_0_2_migrate_info_type();
+        PSCI_SET_RESULT(regs, do_psci_0_2_migrate_info_type());
         break;
     case PSCI_0_2_FN_MIGRATE_INFO_UP_CPU:
     case PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU:
         perfc_incr(vpsci_migrate_info_up_cpu);
         if ( psci_mode_check(current->domain, fid) )
-            PSCI_RESULT_REG(regs) = do_psci_0_2_migrate_info_up_cpu();
+            PSCI_SET_RESULT(regs, do_psci_0_2_migrate_info_up_cpu());
         break;
     case PSCI_0_2_FN_SYSTEM_OFF:
         perfc_incr(vpsci_system_off);
         do_psci_0_2_system_off();
-        PSCI_RESULT_REG(regs) = PSCI_INTERNAL_FAILURE;
+        PSCI_SET_RESULT(regs, PSCI_INTERNAL_FAILURE);
         break;
     case PSCI_0_2_FN_SYSTEM_RESET:
         perfc_incr(vpsci_system_reset);
         do_psci_0_2_system_reset();
-        PSCI_RESULT_REG(regs) = PSCI_INTERNAL_FAILURE;
+        PSCI_SET_RESULT(regs, PSCI_INTERNAL_FAILURE);
         break;
     case PSCI_0_2_FN_CPU_ON:
     case PSCI_0_2_FN64_CPU_ON:
@@ -1524,8 +1523,7 @@  static void do_trap_psci(struct cpu_user_regs *regs)
             register_t vcpuid = PSCI_ARG(regs,1);
             register_t epoint = PSCI_ARG(regs,2);
             register_t cid = PSCI_ARG(regs,3);
-            PSCI_RESULT_REG(regs) =
-                do_psci_0_2_cpu_on(vcpuid, epoint, cid);
+            PSCI_SET_RESULT(regs, do_psci_0_2_cpu_on(vcpuid, epoint, cid));
         }
         break;
     case PSCI_0_2_FN_CPU_SUSPEND:
@@ -1536,8 +1534,7 @@  static void do_trap_psci(struct cpu_user_regs *regs)
             uint32_t pstate = PSCI_ARG32(regs,1);
             register_t epoint = PSCI_ARG(regs,2);
             register_t cid = PSCI_ARG(regs,3);
-            PSCI_RESULT_REG(regs) =
-                do_psci_0_2_cpu_suspend(pstate, epoint, cid);
+            PSCI_SET_RESULT(regs, do_psci_0_2_cpu_suspend(pstate, epoint, cid));
         }
         break;
     case PSCI_0_2_FN_AFFINITY_INFO:
@@ -1547,8 +1544,7 @@  static void do_trap_psci(struct cpu_user_regs *regs)
         {
             register_t taff = PSCI_ARG(regs,1);
             uint32_t laff = PSCI_ARG32(regs,2);
-            PSCI_RESULT_REG(regs) =
-                do_psci_0_2_affinity_info(taff, laff);
+            PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff, laff));
         }
         break;
     case PSCI_0_2_FN_MIGRATE:
@@ -1557,7 +1553,7 @@  static void do_trap_psci(struct cpu_user_regs *regs)
         if ( psci_mode_check(current->domain, fid) )
         {
             uint32_t tcpu = PSCI_ARG32(regs,1);
-            PSCI_RESULT_REG(regs) = do_psci_0_2_migrate(tcpu);
+            PSCI_SET_RESULT(regs, do_psci_0_2_migrate(tcpu));
         }
         break;
     default: