[v7,19/20] target/arm: Rebuild hflags for M-profile.

Message ID 20191017185110.539-20-richard.henderson@linaro.org
State New
Headers show
Series
  • target/arm: Reduce overhead of cpu_get_tb_cpu_state
Related show

Commit Message

Richard Henderson Oct. 17, 2019, 6:51 p.m.
Continue setting, but not relying upon, env->hflags.

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

---
v7: Add rebuilds for v7m_msr and nvic_writel to v7m.ccr.
---
 hw/intc/armv7m_nvic.c  | 1 +
 target/arm/m_helper.c  | 6 ++++++
 target/arm/translate.c | 5 ++++-
 3 files changed, 11 insertions(+), 1 deletion(-)

-- 
2.17.1

Comments

Peter Maydell Oct. 18, 2019, 12:25 p.m. | #1
On Thu, 17 Oct 2019 at 19:51, Richard Henderson
<richard.henderson@linaro.org> wrote:
>

> Continue setting, but not relying upon, env->hflags.

>

> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

> ---

> v7: Add rebuilds for v7m_msr and nvic_writel to v7m.ccr.

> ---

>  hw/intc/armv7m_nvic.c  | 1 +

>  target/arm/m_helper.c  | 6 ++++++

>  target/arm/translate.c | 5 ++++-

>  3 files changed, 11 insertions(+), 1 deletion(-)

>

> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c

> index 8e93e51e81..a3993e7b72 100644

> --- a/hw/intc/armv7m_nvic.c

> +++ b/hw/intc/armv7m_nvic.c

> @@ -1604,6 +1604,7 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value,

>          }

>

>          cpu->env.v7m.ccr[attrs.secure] = value;

> +        arm_rebuild_hflags(&cpu->env);

>          break;

>      case 0xd24: /* System Handler Control and State (SHCSR) */

>          if (!arm_feature(&cpu->env, ARM_FEATURE_V7)) {


This seems fragile -- we have to remember to add in
a call to arm_rebuild_hflags() for every individual
case of a memory-mapped system register that we choose
to cache in tb flags. It doesn't seem consistent with
the choice for A-profile to rebuild hflags for pretty
much any sysreg write. Maybe it would be better to just
always rebuild hflags at the end of nvic_sysreg_write() ?

thanks
-- PMM
Richard Henderson Oct. 18, 2019, 2:31 p.m. | #2
On 10/18/19 5:25 AM, Peter Maydell wrote:
> On Thu, 17 Oct 2019 at 19:51, Richard Henderson

> <richard.henderson@linaro.org> wrote:

>>

>> Continue setting, but not relying upon, env->hflags.

>>

>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

>> ---

>> v7: Add rebuilds for v7m_msr and nvic_writel to v7m.ccr.

>> ---

>>  hw/intc/armv7m_nvic.c  | 1 +

>>  target/arm/m_helper.c  | 6 ++++++

>>  target/arm/translate.c | 5 ++++-

>>  3 files changed, 11 insertions(+), 1 deletion(-)

>>

>> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c

>> index 8e93e51e81..a3993e7b72 100644

>> --- a/hw/intc/armv7m_nvic.c

>> +++ b/hw/intc/armv7m_nvic.c

>> @@ -1604,6 +1604,7 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value,

>>          }

>>

>>          cpu->env.v7m.ccr[attrs.secure] = value;

>> +        arm_rebuild_hflags(&cpu->env);

>>          break;

>>      case 0xd24: /* System Handler Control and State (SHCSR) */

>>          if (!arm_feature(&cpu->env, ARM_FEATURE_V7)) {

> 

> This seems fragile -- we have to remember to add in

> a call to arm_rebuild_hflags() for every individual

> case of a memory-mapped system register that we choose

> to cache in tb flags. It doesn't seem consistent with

> the choice for A-profile to rebuild hflags for pretty

> much any sysreg write. Maybe it would be better to just

> always rebuild hflags at the end of nvic_sysreg_write() ?


I thought about that, but there were too many returns out of the middle.  I
suppose a wrapper function would take care of that.


r~
Peter Maydell Oct. 18, 2019, 2:52 p.m. | #3
On Fri, 18 Oct 2019 at 15:31, Richard Henderson
<richard.henderson@linaro.org> wrote:
>

> On 10/18/19 5:25 AM, Peter Maydell wrote:

> > This seems fragile -- we have to remember to add in

> > a call to arm_rebuild_hflags() for every individual

> > case of a memory-mapped system register that we choose

> > to cache in tb flags. It doesn't seem consistent with

> > the choice for A-profile to rebuild hflags for pretty

> > much any sysreg write. Maybe it would be better to just

> > always rebuild hflags at the end of nvic_sysreg_write() ?

>

> I thought about that, but there were too many returns out of the middle.  I

> suppose a wrapper function would take care of that.


I hadn't noticed the early returns from nvic_sysreg_write().
You could just turn all the 'return MEMTX_OK's into a
goto exit_ok and have that do the update.

thanks
-- PMM
Peter Maydell Oct. 18, 2019, 2:55 p.m. | #4
On Fri, 18 Oct 2019 at 15:52, Peter Maydell <peter.maydell@linaro.org> wrote:
>

> On Fri, 18 Oct 2019 at 15:31, Richard Henderson

> <richard.henderson@linaro.org> wrote:

> >

> > On 10/18/19 5:25 AM, Peter Maydell wrote:

> > > This seems fragile -- we have to remember to add in

> > > a call to arm_rebuild_hflags() for every individual

> > > case of a memory-mapped system register that we choose

> > > to cache in tb flags. It doesn't seem consistent with

> > > the choice for A-profile to rebuild hflags for pretty

> > > much any sysreg write. Maybe it would be better to just

> > > always rebuild hflags at the end of nvic_sysreg_write() ?

> >

> > I thought about that, but there were too many returns out of the middle.  I

> > suppose a wrapper function would take care of that.

>

> I hadn't noticed the early returns from nvic_sysreg_write().

> You could just turn all the 'return MEMTX_OK's into a

> goto exit_ok and have that do the update.


PS: just for clarity, nvic_sysreg_write(), not nvic_writel().

thanks
-- PMM

Patch

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 8e93e51e81..a3993e7b72 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -1604,6 +1604,7 @@  static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value,
         }
 
         cpu->env.v7m.ccr[attrs.secure] = value;
+        arm_rebuild_hflags(&cpu->env);
         break;
     case 0xd24: /* System Handler Control and State (SHCSR) */
         if (!arm_feature(&cpu->env, ARM_FEATURE_V7)) {
diff --git a/target/arm/m_helper.c b/target/arm/m_helper.c
index 27cd2f3f96..f2512e448e 100644
--- a/target/arm/m_helper.c
+++ b/target/arm/m_helper.c
@@ -494,6 +494,7 @@  void HELPER(v7m_bxns)(CPUARMState *env, uint32_t dest)
     switch_v7m_security_state(env, dest & 1);
     env->thumb = 1;
     env->regs[15] = dest & ~1;
+    arm_rebuild_hflags(env);
 }
 
 void HELPER(v7m_blxns)(CPUARMState *env, uint32_t dest)
@@ -555,6 +556,7 @@  void HELPER(v7m_blxns)(CPUARMState *env, uint32_t dest)
     switch_v7m_security_state(env, 0);
     env->thumb = 1;
     env->regs[15] = dest;
+    arm_rebuild_hflags(env);
 }
 
 static uint32_t *get_v7m_sp_ptr(CPUARMState *env, bool secure, bool threadmode,
@@ -895,6 +897,7 @@  static void v7m_exception_taken(ARMCPU *cpu, uint32_t lr, bool dotailchain,
     env->regs[14] = lr;
     env->regs[15] = addr & 0xfffffffe;
     env->thumb = addr & 1;
+    arm_rebuild_hflags(env);
 }
 
 static void v7m_update_fpccr(CPUARMState *env, uint32_t frameptr,
@@ -1765,6 +1768,7 @@  static void do_v7m_exception_exit(ARMCPU *cpu)
 
     /* Otherwise, we have a successful exception exit. */
     arm_clear_exclusive(env);
+    arm_rebuild_hflags(env);
     qemu_log_mask(CPU_LOG_INT, "...successful exception return\n");
 }
 
@@ -1837,6 +1841,7 @@  static bool do_v7m_function_return(ARMCPU *cpu)
     xpsr_write(env, 0, XPSR_IT);
     env->thumb = newpc & 1;
     env->regs[15] = newpc & ~1;
+    arm_rebuild_hflags(env);
 
     qemu_log_mask(CPU_LOG_INT, "...function return successful\n");
     return true;
@@ -1959,6 +1964,7 @@  static bool v7m_handle_execute_nsc(ARMCPU *cpu)
     switch_v7m_security_state(env, true);
     xpsr_write(env, 0, XPSR_IT);
     env->regs[15] += 4;
+    arm_rebuild_hflags(env);
     return true;
 
 gen_invep:
diff --git a/target/arm/translate.c b/target/arm/translate.c
index cb47cd9744..b3720cd59b 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -8325,7 +8325,7 @@  static bool trans_MRS_v7m(DisasContext *s, arg_MRS_v7m *a)
 
 static bool trans_MSR_v7m(DisasContext *s, arg_MSR_v7m *a)
 {
-    TCGv_i32 addr, reg;
+    TCGv_i32 addr, reg, el;
 
     if (!arm_dc_feature(s, ARM_FEATURE_M)) {
         return false;
@@ -8335,6 +8335,9 @@  static bool trans_MSR_v7m(DisasContext *s, arg_MSR_v7m *a)
     gen_helper_v7m_msr(cpu_env, addr, reg);
     tcg_temp_free_i32(addr);
     tcg_temp_free_i32(reg);
+    el = tcg_const_i32(s->current_el);
+    gen_helper_rebuild_hflags_m32(cpu_env, el);
+    tcg_temp_free_i32(el);
     gen_lookup_tb(s);
     return true;
 }