[PATCH 0/4] target/arm: Fix hflags mismatches for M-profile

[PATCH 0/4] target/arm: Fix hflags mismatches for M-profile

[Peter Maydell]

This patchset fixes three hflags-mismatch assertions I ran
into while doing some work on M-profile. The last patch
is just comment typos that I noticed in the process.

thanks
-- PMM

Peter Maydell (4):
  hw/intc/armv7m_nvic: Rebuild hflags on reset
  target/arm: Update hflags in trans_CPS_v7m()
  target/arm: Recalculate hflags correctly after writes to CONTROL
  target/arm: Fix some comment typos

 target/arm/helper.h    |  1 +
 hw/intc/armv7m_nvic.c  |  6 ++++++
 target/arm/helper.c    | 14 +++++++++++++-
 target/arm/translate.c | 14 ++++++++------
 4 files changed, 28 insertions(+), 7 deletions(-)

-- 
2.20.1

[PATCH 1/4] hw/intc/armv7m_nvic: Rebuild hflags on reset

[Peter Maydell]

Some of an M-profile CPU's cached hflags state depends on state that's
in our NVIC object. We already do an hflags rebuild when the NVIC
registers are written, but we also need to do this on NVIC reset,
because there's no guarantee that this will happen before the
CPU reset.

This fixes an assertion due to mismatched hflags which happens if
the CPU is reset from inside a HardFault handler.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/armv7m_nvic.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index a62587eb3f0..1ad35e55292 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -2593,6 +2593,12 @@ static void armv7m_nvic_reset(DeviceState *dev)
             s->itns[i] = true;
         }
     }
+
+    /*
+     * We updated state that affects the CPU's MMUidx and thus its hflags;
+     * and we can't guarantee that we run before the CPU reset function.
+     */
+    arm_rebuild_hflags(&s->cpu->env);
 }
 
 static void nvic_systick_trigger(void *opaque, int n, int level)
-- 
2.20.1

Re: [PATCH 1/4] hw/intc/armv7m_nvic: Rebuild hflags on reset

[Richard Henderson]

On 3/3/20 9:49 AM, Peter Maydell wrote:
> Some of an M-profile CPU's cached hflags state depends on state that's
> in our NVIC object. We already do an hflags rebuild when the NVIC
> registers are written, but we also need to do this on NVIC reset,
> because there's no guarantee that this will happen before the
> CPU reset.
> 
> This fixes an assertion due to mismatched hflags which happens if
> the CPU is reset from inside a HardFault handler.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/intc/armv7m_nvic.c | 6 ++++++
>  1 file changed, 6 insertions(+)

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


r~

[PATCH 2/4] target/arm: Update hflags in trans_CPS_v7m()

[Peter Maydell]

For M-profile CPUs, the FAULTMASK value affects the CPU's MMU index
(it changes the NegPri bit). We update the hflags after calls
to the v7m_msr helper in trans_MSR_v7m() but forgot to do so
in trans_CPS_v7m().

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/translate.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/target/arm/translate.c b/target/arm/translate.c
index 6259064ea7c..7f0154194cf 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -10590,7 +10590,7 @@ static bool trans_CPS(DisasContext *s, arg_CPS *a)
 
 static bool trans_CPS_v7m(DisasContext *s, arg_CPS_v7m *a)
 {
-    TCGv_i32 tmp, addr;
+    TCGv_i32 tmp, addr, el;
 
     if (!arm_dc_feature(s, ARM_FEATURE_M)) {
         return false;
@@ -10613,6 +10613,9 @@ static bool trans_CPS_v7m(DisasContext *s, arg_CPS_v7m *a)
         gen_helper_v7m_msr(cpu_env, addr, tmp);
         tcg_temp_free_i32(addr);
     }
+    el = tcg_const_i32(s->current_el);
+    gen_helper_rebuild_hflags_m32(cpu_env, el);
+    tcg_temp_free_i32(el);
     tcg_temp_free_i32(tmp);
     gen_lookup_tb(s);
     return true;
-- 
2.20.1

Re: [PATCH 2/4] target/arm: Update hflags in trans_CPS_v7m()

[Richard Henderson]

On 3/3/20 9:49 AM, Peter Maydell wrote:
> For M-profile CPUs, the FAULTMASK value affects the CPU's MMU index
> (it changes the NegPri bit). We update the hflags after calls
> to the v7m_msr helper in trans_MSR_v7m() but forgot to do so
> in trans_CPS_v7m().
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/translate.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

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


r~

[PATCH 3/4] target/arm: Recalculate hflags correctly after writes to CONTROL

[Peter Maydell]

A write to the CONTROL register can change our current EL (by
writing to the nPRIV bit). That means that we can't assume
that s->current_el is still valid in trans_MSR_v7m() when
we try to rebuild the hflags.

Add a new helper rebuild_hflags_m32_newel() which, like the
existing rebuild_hflags_a32_newel(), recalculates the current
EL from scratch, and use it in trans_MSR_v7m().

This fixes an assertion about an hflags mismatch when the
guest changes privilege by writing to CONTROL.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.h    |  1 +
 target/arm/helper.c    | 12 ++++++++++++
 target/arm/translate.c |  7 +++----
 3 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/target/arm/helper.h b/target/arm/helper.h
index fcbf5041213..a63fd5dfb73 100644
--- a/target/arm/helper.h
+++ b/target/arm/helper.h
@@ -90,6 +90,7 @@ DEF_HELPER_4(msr_banked, void, env, i32, i32, i32)
 DEF_HELPER_2(get_user_reg, i32, env, i32)
 DEF_HELPER_3(set_user_reg, void, env, i32, i32)
 
+DEF_HELPER_FLAGS_1(rebuild_hflags_m32_newel, TCG_CALL_NO_RWG, void, env)
 DEF_HELPER_FLAGS_2(rebuild_hflags_m32, TCG_CALL_NO_RWG, void, env, int)
 DEF_HELPER_FLAGS_1(rebuild_hflags_a32_newel, TCG_CALL_NO_RWG, void, env)
 DEF_HELPER_FLAGS_2(rebuild_hflags_a32, TCG_CALL_NO_RWG, void, env, int)
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 6be9ffa09ef..2eec812b80b 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -12325,6 +12325,18 @@ void arm_rebuild_hflags(CPUARMState *env)
     env->hflags = rebuild_hflags_internal(env);
 }
 
+/*
+ * If we have triggered a EL state change we can't rely on the
+ * translator having passed it to us, we need to recompute.
+ */
+void HELPER(rebuild_hflags_m32_newel)(CPUARMState *env)
+{
+    int el = arm_current_el(env);
+    int fp_el = fp_exception_el(env, el);
+    ARMMMUIdx mmu_idx = arm_mmu_idx_el(env, el);
+    env->hflags = rebuild_hflags_m32(env, fp_el, mmu_idx);
+}
+
 void HELPER(rebuild_hflags_m32)(CPUARMState *env, int el)
 {
     int fp_el = fp_exception_el(env, el);
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 7f0154194cf..4715ca0d2ad 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -8551,7 +8551,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, el;
+    TCGv_i32 addr, reg;
 
     if (!arm_dc_feature(s, ARM_FEATURE_M)) {
         return false;
@@ -8561,9 +8561,8 @@ 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);
+    /* If we wrote to CONTROL, the EL might have changed */
+    gen_helper_rebuild_hflags_m32_newel(cpu_env);
     gen_lookup_tb(s);
     return true;
 }
-- 
2.20.1

Re: [PATCH 3/4] target/arm: Recalculate hflags correctly after writes to CONTROL

[Richard Henderson]

On 3/3/20 9:49 AM, Peter Maydell wrote:
> A write to the CONTROL register can change our current EL (by
> writing to the nPRIV bit). That means that we can't assume
> that s->current_el is still valid in trans_MSR_v7m() when
> we try to rebuild the hflags.
> 
> Add a new helper rebuild_hflags_m32_newel() which, like the
> existing rebuild_hflags_a32_newel(), recalculates the current
> EL from scratch, and use it in trans_MSR_v7m().
> 
> This fixes an assertion about an hflags mismatch when the
> guest changes privilege by writing to CONTROL.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/helper.h    |  1 +
>  target/arm/helper.c    | 12 ++++++++++++
>  target/arm/translate.c |  7 +++----
>  3 files changed, 16 insertions(+), 4 deletions(-)

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


r~

[PATCH 4/4] target/arm: Fix some comment typos

[Peter Maydell]

Fix a couple of comment typos.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Noticed these while writing the other patches...
---
 target/arm/helper.c    | 2 +-
 target/arm/translate.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 2eec812b80b..3fa9c0cc861 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -12347,7 +12347,7 @@ void HELPER(rebuild_hflags_m32)(CPUARMState *env, int el)
 
 /*
  * If we have triggered a EL state change we can't rely on the
- * translator having passed it too us, we need to recompute.
+ * translator having passed it to us, we need to recompute.
  */
 void HELPER(rebuild_hflags_a32_newel)(CPUARMState *env)
 {
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 4715ca0d2ad..9f9f4e19e04 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -7296,7 +7296,7 @@ static int disas_coproc_insn(DisasContext *s, uint32_t insn)
 
         if (!isread && !(ri->type & ARM_CP_SUPPRESS_TB_END)) {
             /*
-             * A write to any coprocessor regiser that ends a TB
+             * A write to any coprocessor register that ends a TB
              * must rebuild the hflags for the next TB.
              */
             TCGv_i32 tcg_el = tcg_const_i32(s->current_el);
-- 
2.20.1

Re: [PATCH 4/4] target/arm: Fix some comment typos

[Richard Henderson]

On 3/3/20 9:49 AM, Peter Maydell wrote:
> Fix a couple of comment typos.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Noticed these while writing the other patches...
> ---
>  target/arm/helper.c    | 2 +-
>  target/arm/translate.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

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


r~