diff mbox

[v2,1/4] arm: traps: psci: use generic register accessors

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

Commit Message

Volodymyr Babchuk June 22, 2017, 4:24 p.m. UTC 
There are standard functions set_user_reg() and get_user_reg(). Use
them instead of PSCI_RESULT_REG()/PSCI_ARG() macros.

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
 xen/arch/arm/traps.c | 68 ++++++++++++++++++++++------------------------------
 1 file changed, 29 insertions(+), 39 deletions(-)

Comments

Julien Grall June 30, 2017, 10:37 a.m. UTC | #1 
Hi Volodymyr,

On 22/06/17 17:24, Volodymyr Babchuk wrote:
> There are standard functions set_user_reg() and get_user_reg(). Use
> them instead of PSCI_RESULT_REG()/PSCI_ARG() macros.

Whilst I agree the use of {set,get}_user_reg(), I think we should 
abstract the call to make the code more readable.

>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
>  xen/arch/arm/traps.c | 68 ++++++++++++++++++++++------------------------------
>  1 file changed, 29 insertions(+), 39 deletions(-)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 6cf9ee7..2054c69 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1449,16 +1449,6 @@ static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
>  }
>  #endif
>
> -#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 )
> -#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
> -
>  /* helper function for checking arm mode 32/64 bit */
>  static inline int psci_mode_check(struct domain *d, register_t fid)
>  {
> @@ -1467,65 +1457,65 @@ static inline int psci_mode_check(struct domain *d, register_t fid)
>
>  static void do_trap_psci(struct cpu_user_regs *regs)
>  {
> -    register_t fid = PSCI_ARG(regs,0);
> +    register_t fid = get_user_reg(regs,0);
>
>      /* preloading in case psci_mode_check fails */
> -    PSCI_RESULT_REG(regs) = PSCI_INVALID_PARAMETERS;
> +    set_user_reg(regs, 0, PSCI_INVALID_PARAMETERS);

For instance, it is not clear for the user that register 0 is the result 
register. I would prefer if you introduce PSCI_SET_RESULT_REG(...).

>      switch( fid )
>      {
>      case PSCI_cpu_off:
>          {
> -            uint32_t pstate = PSCI_ARG32(regs,1);
> +            uint32_t pstate = get_user_reg(regs, 1);

PSCI_ARG32 is masking the top 32-bit of 64-bit register. Now, you are 
implicitly masking them. Which is much less obvious to read and error-prone.

I would much prefer is you re-implement PSCI_ARG32 in term of 
get_user_reg(...).

Cheers,
diff mbox

Patch

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 6cf9ee7..2054c69 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1449,16 +1449,6 @@  static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
 }
 #endif
 
-#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 )
-#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
-
 /* helper function for checking arm mode 32/64 bit */
 static inline int psci_mode_check(struct domain *d, register_t fid)
 {
@@ -1467,65 +1457,65 @@  static inline int psci_mode_check(struct domain *d, register_t fid)
 
 static void do_trap_psci(struct cpu_user_regs *regs)
 {
-    register_t fid = PSCI_ARG(regs,0);
+    register_t fid = get_user_reg(regs,0);
 
     /* preloading in case psci_mode_check fails */
-    PSCI_RESULT_REG(regs) = PSCI_INVALID_PARAMETERS;
+    set_user_reg(regs, 0, PSCI_INVALID_PARAMETERS);
     switch( fid )
     {
     case PSCI_cpu_off:
         {
-            uint32_t pstate = PSCI_ARG32(regs,1);
+            uint32_t pstate = get_user_reg(regs, 1);
             perfc_incr(vpsci_cpu_off);
-            PSCI_RESULT_REG(regs) = do_psci_cpu_off(pstate);
+            set_user_reg(regs, 0, do_psci_cpu_off(pstate));
         }
         break;
     case PSCI_cpu_on:
         {
-            uint32_t vcpuid = PSCI_ARG32(regs,1);
-            register_t epoint = PSCI_ARG(regs,2);
+            uint32_t vcpuid =  get_user_reg(regs, 1);
+            register_t epoint = get_user_reg(regs, 2);
             perfc_incr(vpsci_cpu_on);
-            PSCI_RESULT_REG(regs) = do_psci_cpu_on(vcpuid, epoint);
+            set_user_reg(regs, 0, 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();
+        set_user_reg(regs, 0, 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();
+        set_user_reg(regs, 0, 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();
+        set_user_reg(regs, 0, 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();
+            set_user_reg(regs, 0, 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;
+        set_user_reg(regs, 0, 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;
+        set_user_reg(regs, 0, PSCI_INTERNAL_FAILURE);
         break;
     case PSCI_0_2_FN_CPU_ON:
     case PSCI_0_2_FN64_CPU_ON:
         perfc_incr(vpsci_cpu_on);
         if ( psci_mode_check(current->domain, fid) )
         {
-            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);
+            register_t vcpuid = get_user_reg(regs, 1);
+            register_t epoint = get_user_reg(regs, 2);
+            register_t cid = get_user_reg(regs, 3);
+            set_user_reg(regs, 0,
+                         do_psci_0_2_cpu_on(vcpuid, epoint, cid));
         }
         break;
     case PSCI_0_2_FN_CPU_SUSPEND:
@@ -1533,11 +1523,11 @@  static void do_trap_psci(struct cpu_user_regs *regs)
         perfc_incr(vpsci_cpu_suspend);
         if ( psci_mode_check(current->domain, fid) )
         {
-            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);
+            uint32_t pstate = get_user_reg(regs, 1);
+            register_t epoint = get_user_reg(regs, 2);
+            register_t cid = get_user_reg(regs, 3);
+            set_user_reg(regs, 0,
+                         do_psci_0_2_cpu_suspend(pstate, epoint, cid));
         }
         break;
     case PSCI_0_2_FN_AFFINITY_INFO:
@@ -1545,10 +1535,10 @@  static void do_trap_psci(struct cpu_user_regs *regs)
         perfc_incr(vpsci_cpu_affinity_info);
         if ( psci_mode_check(current->domain, fid) )
         {
-            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);
+            register_t taff = get_user_reg(regs, 1);
+            uint32_t laff = get_user_reg(regs, 2);
+            set_user_reg(regs, 0,
+                         do_psci_0_2_affinity_info(taff, laff));
         }
         break;
     case PSCI_0_2_FN_MIGRATE:
@@ -1556,8 +1546,8 @@  static void do_trap_psci(struct cpu_user_regs *regs)
         perfc_incr(vpsci_cpu_migrate);
         if ( psci_mode_check(current->domain, fid) )
         {
-            uint32_t tcpu = PSCI_ARG32(regs,1);
-            PSCI_RESULT_REG(regs) = do_psci_0_2_migrate(tcpu);
+            uint32_t tcpu = get_user_reg(regs, 1);
+            set_user_reg(regs, 0, do_psci_0_2_migrate(tcpu));
         }
         break;
     default: