| Message ID | 1455217909-28317-4-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 11.02.2016 22:11, 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> Reviewed-by: Sergey Fedorov <serge.fdrv@gmail.com> > --- > 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)
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)? 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 >
On 12 February 2016 at 15:12, Edgar E. Iglesias <edgar.iglesias@gmail.com> 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)? I would be happy to entirely drop the assert, yes. thanks -- PMM
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 > >
On Fri, Feb 12, 2016 at 03:15:22PM +0000, Peter Maydell wrote: > On 12 February 2016 at 15:12, Edgar E. Iglesias > <edgar.iglesias@gmail.com> 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)? > > I would be happy to entirely drop the assert, yes. OK, thanks. With that change: Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
On 12.02.2016 18:16, Edgar E. Iglesias wrote: > On Fri, Feb 12, 2016 at 03:15:22PM +0000, Peter Maydell wrote: >> On 12 February 2016 at 15:12, Edgar E. Iglesias >> <edgar.iglesias@gmail.com> 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)? >> I would be happy to entirely drop the assert, yes. > OK, thanks. > > With that change: > Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > Yes, I also think it would be okay to drop that assert. Anyway: Reviewed-by: Sergey Fedorov <serge.fdrv@gmail.com>
On 12 February 2016 at 15:16, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > On Fri, Feb 12, 2016 at 03:15:22PM +0000, Peter Maydell wrote: >> On 12 February 2016 at 15:12, Edgar E. Iglesias >> <edgar.iglesias@gmail.com> 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)? >> >> I would be happy to entirely drop the assert, yes. > > OK, thanks. It turns out that the compiler was being clever and dropping all the code after the g_assert_not_reached(), which meant I didn't notice that bank_number() is only compiled in in the softmmu build. We should just move bank_number() into being a static inline in internals.h, I think -- it's pretty trivial. Will send that patch... thanks -- PMM
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)
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> --- target-arm/op_helper.c | 25 ++++++------------------- 1 file changed, 6 insertions(+), 19 deletions(-)