Re: [Xen-devel] [PATCH 3/7] xen/arm: Rework psr_mode_is_32bit()

[Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>]

Julien Grall writes:

> psr_mode_is_32bit() prototype does not match the rest of the helpers for
> the process state. Looking at the callers, most of them will access
> struct cpu_user_regs just for calling psr_mode_is_32bit().
>
> The macro is now reworked to take a struct cpu_user_regs in parameter.
> At the same time take the opportunity to switch to a static inline
> helper.
I'm a bit concerned about naming now. As psr_mode_is_32bit() is now have
no psr parameter, and ARM ARM uses term "state" instead of "mode", maybe
it is worth to rename this helper to something like "is_32bit_state"?

> Lastly, when compiled for 32-bit, Xen will only support 32-bit guest. So
> it is pointless to check whether the register state correspond to 64-bit
> or not.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/traps.c       | 28 ++++++++++++++--------------
>  xen/include/asm-arm/regs.h |  9 ++++++++-
>  2 files changed, 22 insertions(+), 15 deletions(-)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 111a2029e6..54e66a86d0 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -919,7 +919,7 @@ static void _show_registers(const struct cpu_user_regs *regs,
>  #ifdef CONFIG_ARM_64
>          else if ( is_64bit_domain(v->domain) )
>          {
> -            if ( psr_mode_is_32bit(regs->cpsr) )
> +            if ( psr_mode_is_32bit(regs) )
>              {
>                  BUG_ON(!usr_mode(regs));
>                  show_registers_32(regs, ctxt, guest_mode, v);
> @@ -1625,7 +1625,7 @@ int check_conditional_instr(struct cpu_user_regs *regs, const union hsr hsr)
>      {
>          unsigned long it;
>  
> -        BUG_ON( !psr_mode_is_32bit(regs->cpsr) || !(cpsr&PSR_THUMB) );
> +        BUG_ON( !psr_mode_is_32bit(regs) || !(cpsr & PSR_THUMB) );
>  
>          it = ( (cpsr >> (10-2)) & 0xfc) | ((cpsr >> 25) & 0x3 );
>  
> @@ -1650,7 +1650,7 @@ int check_conditional_instr(struct cpu_user_regs *regs, const union hsr hsr)
>  void advance_pc(struct cpu_user_regs *regs, const union hsr hsr)
>  {
>      unsigned long itbits, cond, cpsr = regs->cpsr;
> -    bool is_thumb = psr_mode_is_32bit(cpsr) && (cpsr & PSR_THUMB);
> +    bool is_thumb = psr_mode_is_32bit(regs) && (cpsr & PSR_THUMB);
>  
>      if ( is_thumb && (cpsr & PSR_IT_MASK) )
>      {
> @@ -2078,32 +2078,32 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
>          advance_pc(regs, hsr);
>          break;
>      case HSR_EC_CP15_32:
> -        GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
> +        GUEST_BUG_ON(!psr_mode_is_32bit(regs));
>          perfc_incr(trap_cp15_32);
>          do_cp15_32(regs, hsr);
>          break;
>      case HSR_EC_CP15_64:
> -        GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
> +        GUEST_BUG_ON(!psr_mode_is_32bit(regs));
>          perfc_incr(trap_cp15_64);
>          do_cp15_64(regs, hsr);
>          break;
>      case HSR_EC_CP14_32:
> -        GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
> +        GUEST_BUG_ON(!psr_mode_is_32bit(regs));
>          perfc_incr(trap_cp14_32);
>          do_cp14_32(regs, hsr);
>          break;
>      case HSR_EC_CP14_64:
> -        GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
> +        GUEST_BUG_ON(!psr_mode_is_32bit(regs));
>          perfc_incr(trap_cp14_64);
>          do_cp14_64(regs, hsr);
>          break;
>      case HSR_EC_CP14_DBG:
> -        GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
> +        GUEST_BUG_ON(!psr_mode_is_32bit(regs));
>          perfc_incr(trap_cp14_dbg);
>          do_cp14_dbg(regs, hsr);
>          break;
>      case HSR_EC_CP:
> -        GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
> +        GUEST_BUG_ON(!psr_mode_is_32bit(regs));
>          perfc_incr(trap_cp);
>          do_cp(regs, hsr);
>          break;
> @@ -2114,7 +2114,7 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
>           * ARMv7 (DDI 0406C.b): B1.14.8
>           * ARMv8 (DDI 0487A.d): D1-1501 Table D1-44
>           */
> -        GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
> +        GUEST_BUG_ON(!psr_mode_is_32bit(regs));
>          perfc_incr(trap_smc32);
>          do_trap_smc(regs, hsr);
>          break;
> @@ -2122,7 +2122,7 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
>      {
>          register_t nr;
>  
> -        GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
> +        GUEST_BUG_ON(!psr_mode_is_32bit(regs));
>          perfc_incr(trap_hvc32);
>  #ifndef NDEBUG
>          if ( (hsr.iss & 0xff00) == 0xff00 )
> @@ -2137,7 +2137,7 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
>      }
>  #ifdef CONFIG_ARM_64
>      case HSR_EC_HVC64:
> -        GUEST_BUG_ON(psr_mode_is_32bit(regs->cpsr));
> +        GUEST_BUG_ON(psr_mode_is_32bit(regs));
>          perfc_incr(trap_hvc64);
>  #ifndef NDEBUG
>          if ( (hsr.iss & 0xff00) == 0xff00 )
> @@ -2153,12 +2153,12 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
>           *
>           * ARMv8 (DDI 0487A.d): D1-1501 Table D1-44
>           */
> -        GUEST_BUG_ON(psr_mode_is_32bit(regs->cpsr));
> +        GUEST_BUG_ON(psr_mode_is_32bit(regs));
>          perfc_incr(trap_smc64);
>          do_trap_smc(regs, hsr);
>          break;
>      case HSR_EC_SYSREG:
> -        GUEST_BUG_ON(psr_mode_is_32bit(regs->cpsr));
> +        GUEST_BUG_ON(psr_mode_is_32bit(regs));
>          perfc_incr(trap_sysreg);
>          do_sysreg(regs, hsr);
>          break;
> diff --git a/xen/include/asm-arm/regs.h b/xen/include/asm-arm/regs.h
> index ddc6eba9ce..0e3e56b452 100644
> --- a/xen/include/asm-arm/regs.h
> +++ b/xen/include/asm-arm/regs.h
> @@ -13,7 +13,14 @@
>  
>  #define psr_mode(psr,m) (((psr) & PSR_MODE_MASK) == m)
>  
> -#define psr_mode_is_32bit(psr) !!((psr) & PSR_MODE_BIT)
> +static inline bool psr_mode_is_32bit(const struct cpu_user_regs *regs)
> +{
> +#ifdef CONFIG_ARM_32
> +    return true;
> +#else
> +    return !!(regs->cpsr & PSR_MODE_BIT);
> +#endif
> +}
>  
>  #define usr_mode(r)     psr_mode((r)->cpsr,PSR_MODE_USR)
>  #define fiq_mode(r)     psr_mode((r)->cpsr,PSR_MODE_FIQ)


-- 
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel