Re: [Xen-devel] [PATCH v4 01/11] arm: traps: use generic register accessors in the PSCI code

[Julien Grall]

Hi Volodymyr,

On 21/08/17 21:27, Volodymyr Babchuk wrote:

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


s/macroses/macros/ I think.


CONFIG_ARM_64 definition.

Signed-off-by: Volodymyr Babchuk 
---

* Added space into reg,n
* Used 32-bit constant tin PSCI_ARG32

---
xen/arch/arm/traps.c | 40 ++--
 1 file changed, 18 insertions(+), 22 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 13efb58..66f12cb 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1450,14 +1450,13 @@ 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 & 0x )
+#define PSCI_ARG32(reg, n) (uint32_t)(get_user_reg(reg, n) & 0x)


Please drop the mask, the cast is sufficient.


 #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)
+#define PSCI_ARG32(reg, n) PSCI_ARG(reg, n)


Please mention in the commit message that you also modify the coding style.

Cheers,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [PATCH v4 01/11] arm: traps: use generic register accessors in the PSCI code

[Volodymyr Babchuk]

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 
---

* Added space into reg,n
* Used 32-bit constant tin PSCI_ARG32

---
xen/arch/arm/traps.c | 40 ++--
 1 file changed, 18 insertions(+), 22 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 13efb58..66f12cb 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1450,14 +1450,13 @@ 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 & 0x )
+#define PSCI_ARG32(reg, n) (uint32_t)(get_user_reg(reg, n) & 0x)
 #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)
+#define PSCI_ARG32(reg, n) PSCI_ARG(reg, n)
 #endif
 
 /* helper function for checking arm mode 32/64 bit */
@@ -1471,14 +1470,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:
@@ -1486,36 +1485,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:
@@ -1525,8 +1524,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:
@@ -1537,8 +1535,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:
@@ -1548,8 +1545,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);
+