Re: [Qemu-devel] [PATCH 3/4] target-arm: Combine user-only and softmmu get/set_r13_banked()

["Edgar E. Iglesias" <edgar.iglesias@gmail.com>]

On Fri, Feb 12, 2016 at 04:12:18PM +0100, Edgar E. Iglesias wrote:
> On Thu, Feb 11, 2016 at 07:11:48PM +0000, Peter Maydell wrote:
> > The user-mode versions of get/set_r13_banked() exist just to assert
> > if they're ever called -- the translate time code should never
> > emit calls to them because SRS from user mode always UNDEF.
> > There's no code in the softmmu versions that can't compile in
> > CONFIG_USER_ONLY, so combine the two functions rather than
> > having completely split versions under ifdefs.
> > 
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> 
> Hi Peter,
> 
> Do we really need the assert?
> If we keep it, can't we have it for both -user and -softmmu (avoiding the ifdef)?

e.g:

assert(arm_current_el(env) != 0);

Allthough, I'd probably vote for removing it...

Cheers,
Edgar



> 
> Cheers,
> Edgar
> 
> 
> 
> > ---
> >  target-arm/op_helper.c | 25 ++++++-------------------
> >  1 file changed, 6 insertions(+), 19 deletions(-)
> > 
> > diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> > index 053e9b6..05f97a7 100644
> > --- a/target-arm/op_helper.c
> > +++ b/target-arm/op_helper.c
> > @@ -457,26 +457,11 @@ void HELPER(set_user_reg)(CPUARMState *env, uint32_t regno, uint32_t val)
> >      }
> >  }
> >  
> > -#if defined(CONFIG_USER_ONLY)
> > -void HELPER(set_r13_banked)(CPUARMState *env, uint32_t mode, uint32_t val)
> > -{
> > -    ARMCPU *cpu = arm_env_get_cpu(env);
> > -
> > -    cpu_abort(CPU(cpu), "banked r13 write\n");
> > -}
> > -
> > -uint32_t HELPER(get_r13_banked)(CPUARMState *env, uint32_t mode)
> > -{
> > -    ARMCPU *cpu = arm_env_get_cpu(env);
> > -
> > -    cpu_abort(CPU(cpu), "banked r13 read\n");
> > -    return 0;
> > -}
> > -
> > -#else
> > -
> >  void HELPER(set_r13_banked)(CPUARMState *env, uint32_t mode, uint32_t val)
> >  {
> > +#if defined(CONFIG_USER_ONLY)
> > +    g_assert_not_reached();
> > +#endif
> >      if ((env->uncached_cpsr & CPSR_M) == mode) {
> >          env->regs[13] = val;
> >      } else {
> > @@ -486,13 +471,15 @@ void HELPER(set_r13_banked)(CPUARMState *env, uint32_t mode, uint32_t val)
> >  
> >  uint32_t HELPER(get_r13_banked)(CPUARMState *env, uint32_t mode)
> >  {
> > +#if defined(CONFIG_USER_ONLY)
> > +    g_assert_not_reached();
> > +#endif
> >      if ((env->uncached_cpsr & CPSR_M) == mode) {
> >          return env->regs[13];
> >      } else {
> >          return env->banked_r13[bank_number(mode)];
> >      }
> >  }
> > -#endif
> >  
> >  void HELPER(access_check_cp_reg)(CPUARMState *env, void *rip, uint32_t syndrome,
> >                                   uint32_t isread)
> > -- 
> > 1.9.1
> >