Re: [Xen-devel] [PATCH v2 1/4] arm: traps: psci: use generic register accessors

[Julien Grall <julien.grall@xxxxxxx>]

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@xxxxxxxx>
> ---
>  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,

--
Julien Grall

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