qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/6] target/microblaze: Use tcg_gen_lookup_and_goto_ptr
@ 2020-08-31 18:40 Richard Henderson
  2020-08-31 18:40 ` [PATCH 1/6] target/microblaze: Rename DISAS_UPDATE to DISAS_EXIT Richard Henderson
                   ` (7 more replies)
  0 siblings, 8 replies; 10+ messages in thread
From: Richard Henderson @ 2020-08-31 18:40 UTC (permalink / raw)
  To: qemu-devel; +Cc: edgar.iglesias

Based-on: <20200831160601.833692-1-richard.henderson@linaro.org>
("[PULL 00/76] target/microblaze improvements")

Hello again, Edgar.

I had dropped the tcg_gen_lookup_and_goto_ptr patch from the
previous omnibus patch set, as you had reported lockups.

I have identified, by inspection, two cases in which we failed
to return to the main loop even though we should have:

(1) Return-from-exception type instructions.

I had missed these before because they hadn't set cpustate_changed.
This still worked fine because they are all indirect branches, and
had exited immediately.

Fixed by distinguishing these cases from normal indirect branches
before we start using lookup_and_goto_ptr.

(2) MTS in a branch delay slot.

We did not check dc->cpustate_changed before setting
dc->base.is_jmp to DISAS_JUMP, which lost the fact that we
need to return to the main loop.

This mostly works fine without lookup_and_goto_ptr, because
we either (a) finished an indirect branch and returned to the
main loop anyway or (b) we'd return to the main loop via some
subsequent indirect branch, which would happen "soon enough".

We should have been able to see soft-lockup with the existing
code in the case of a cpustate_changed in the delay slot of
a loop of direct branches that all use goto_tb.  E.g.

	brid	0
	 msrset MSR_IE

I.e. an immediate branch back to the same branch insn,
re-enabling interrupts in the delay slot.  Probably not
something that shows up in the wild.

----

Follow-up question: The manual says that several classes of
instructions are invalid in a branch delay slot, but does
not say what action is taken, if any.

Some of these invalid cases could leave qemu in an inconsistent
state.  Would it be legal for us to diagnose these cases with
trap_illegal?  If not, what *should* we be doing?  We could also
LOG_GUEST_ERROR for these either way.

I've added some TODO comments in these patches that are relevant.

Thanks,


r~


Richard Henderson (6):
  target/microblaze: Rename DISAS_UPDATE to DISAS_EXIT
  target/microblaze: Introduce DISAS_EXIT_NEXT, DISAS_EXIT_JUMP
  target/microblaze: Replace cpustate_changed with DISAS_EXIT_NEXT
  target/microblaze: Handle DISAS_EXIT_NEXT in delay slot
  target/microblaze: Force rtid, rted, rtbd to exit
  target/microblaze: Use tcg_gen_lookup_and_goto_ptr

 target/microblaze/translate.c | 128 ++++++++++++++++++++++------------
 1 file changed, 82 insertions(+), 46 deletions(-)

-- 
2.25.1



^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/6] target/microblaze: Rename DISAS_UPDATE to DISAS_EXIT
  2020-08-31 18:40 [PATCH 0/6] target/microblaze: Use tcg_gen_lookup_and_goto_ptr Richard Henderson
@ 2020-08-31 18:40 ` Richard Henderson
  2020-08-31 18:40 ` [PATCH 2/6] target/microblaze: Introduce DISAS_EXIT_NEXT, DISAS_EXIT_JUMP Richard Henderson
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 10+ messages in thread
From: Richard Henderson @ 2020-08-31 18:40 UTC (permalink / raw)
  To: qemu-devel; +Cc: edgar.iglesias

The name "update" suggests that something needs updating, but
this is not the case.  Use "exit" to emphasize that nothing
needs doing except to exit.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/microblaze/translate.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/target/microblaze/translate.c b/target/microblaze/translate.c
index a377818b5e..03fc653ce2 100644
--- a/target/microblaze/translate.c
+++ b/target/microblaze/translate.c
@@ -37,7 +37,7 @@
 
 /* is_jmp field values */
 #define DISAS_JUMP    DISAS_TARGET_0 /* only pc was modified dynamically */
-#define DISAS_UPDATE  DISAS_TARGET_1 /* cpu state was modified dynamically */
+#define DISAS_EXIT    DISAS_TARGET_1 /* all cpu state modified dynamically */
 
 static TCGv_i32 cpu_R[32];
 static TCGv_i32 cpu_pc;
@@ -1161,7 +1161,7 @@ static bool trans_brk(DisasContext *dc, arg_typea_br *arg)
     tcg_gen_ori_i32(cpu_msr, cpu_msr, MSR_BIP);
     tcg_gen_movi_tl(cpu_res_addr, -1);
 
-    dc->base.is_jmp = DISAS_UPDATE;
+    dc->base.is_jmp = DISAS_EXIT;
     return true;
 }
 
@@ -1202,7 +1202,7 @@ static bool trans_brki(DisasContext *dc, arg_typeb_br *arg)
                          ~(MSR_VMS | MSR_UMS | MSR_VM | MSR_UM));
     }
     tcg_gen_ori_i32(cpu_msr, cpu_msr, msr_to_set);
-    dc->base.is_jmp = DISAS_UPDATE;
+    dc->base.is_jmp = DISAS_EXIT;
 #endif
 
     return true;
@@ -1712,7 +1712,7 @@ static void mb_tr_translate_insn(DisasContextBase *dcb, CPUState *cs)
 
     /* Force an exit if the per-tb cpu state has changed.  */
     if (dc->base.is_jmp == DISAS_NEXT && dc->cpustate_changed) {
-        dc->base.is_jmp = DISAS_UPDATE;
+        dc->base.is_jmp = DISAS_EXIT;
         tcg_gen_movi_i32(cpu_pc, dc->base.pc_next);
     }
 }
@@ -1733,7 +1733,7 @@ static void mb_tr_tb_stop(DisasContextBase *dcb, CPUState *cs)
         gen_goto_tb(dc, 0, dc->base.pc_next);
         return;
 
-    case DISAS_UPDATE:
+    case DISAS_EXIT:
         if (unlikely(cs->singlestep_enabled)) {
             gen_raise_exception(dc, EXCP_DEBUG);
         } else {
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/6] target/microblaze: Introduce DISAS_EXIT_NEXT, DISAS_EXIT_JUMP
  2020-08-31 18:40 [PATCH 0/6] target/microblaze: Use tcg_gen_lookup_and_goto_ptr Richard Henderson
  2020-08-31 18:40 ` [PATCH 1/6] target/microblaze: Rename DISAS_UPDATE to DISAS_EXIT Richard Henderson
@ 2020-08-31 18:40 ` Richard Henderson
  2020-08-31 18:40 ` [PATCH 3/6] target/microblaze: Replace cpustate_changed with DISAS_EXIT_NEXT Richard Henderson
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 10+ messages in thread
From: Richard Henderson @ 2020-08-31 18:40 UTC (permalink / raw)
  To: qemu-devel; +Cc: edgar.iglesias

Like DISAS_EXIT, except we need to update cpu_pc,
either to pc_next or to btarget respectively.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/microblaze/translate.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/target/microblaze/translate.c b/target/microblaze/translate.c
index 03fc653ce2..0733728794 100644
--- a/target/microblaze/translate.c
+++ b/target/microblaze/translate.c
@@ -39,6 +39,11 @@
 #define DISAS_JUMP    DISAS_TARGET_0 /* only pc was modified dynamically */
 #define DISAS_EXIT    DISAS_TARGET_1 /* all cpu state modified dynamically */
 
+/* cpu state besides pc was modified dynamically; update pc to next */
+#define DISAS_EXIT_NEXT DISAS_TARGET_2
+/* cpu state besides pc was modified dynamically; update pc to btarget */
+#define DISAS_EXIT_JUMP DISAS_TARGET_3
+
 static TCGv_i32 cpu_R[32];
 static TCGv_i32 cpu_pc;
 static TCGv_i32 cpu_msr;
@@ -1712,8 +1717,7 @@ static void mb_tr_translate_insn(DisasContextBase *dcb, CPUState *cs)
 
     /* Force an exit if the per-tb cpu state has changed.  */
     if (dc->base.is_jmp == DISAS_NEXT && dc->cpustate_changed) {
-        dc->base.is_jmp = DISAS_EXIT;
-        tcg_gen_movi_i32(cpu_pc, dc->base.pc_next);
+        dc->base.is_jmp = DISAS_EXIT_NEXT;
     }
 }
 
@@ -1734,12 +1738,14 @@ static void mb_tr_tb_stop(DisasContextBase *dcb, CPUState *cs)
         return;
 
     case DISAS_EXIT:
-        if (unlikely(cs->singlestep_enabled)) {
-            gen_raise_exception(dc, EXCP_DEBUG);
-        } else {
-            tcg_gen_exit_tb(NULL, 0);
-        }
-        return;
+        break;
+    case DISAS_EXIT_NEXT:
+        tcg_gen_movi_i32(cpu_pc, dc->base.pc_next);
+        break;
+    case DISAS_EXIT_JUMP:
+        tcg_gen_mov_i32(cpu_pc, cpu_btarget);
+        tcg_gen_discard_i32(cpu_btarget);
+        break;
 
     case DISAS_JUMP:
         if (dc->jmp_dest != -1 && !cs->singlestep_enabled) {
@@ -1781,6 +1787,13 @@ static void mb_tr_tb_stop(DisasContextBase *dcb, CPUState *cs)
     default:
         g_assert_not_reached();
     }
+
+    /* Finish DISAS_EXIT_* */
+    if (unlikely(cs->singlestep_enabled)) {
+        gen_raise_exception(dc, EXCP_DEBUG);
+    } else {
+        tcg_gen_exit_tb(NULL, 0);
+    }
 }
 
 static void mb_tr_disas_log(const DisasContextBase *dcb, CPUState *cs)
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 3/6] target/microblaze: Replace cpustate_changed with DISAS_EXIT_NEXT
  2020-08-31 18:40 [PATCH 0/6] target/microblaze: Use tcg_gen_lookup_and_goto_ptr Richard Henderson
  2020-08-31 18:40 ` [PATCH 1/6] target/microblaze: Rename DISAS_UPDATE to DISAS_EXIT Richard Henderson
  2020-08-31 18:40 ` [PATCH 2/6] target/microblaze: Introduce DISAS_EXIT_NEXT, DISAS_EXIT_JUMP Richard Henderson
@ 2020-08-31 18:40 ` Richard Henderson
  2020-08-31 18:40 ` [PATCH 4/6] target/microblaze: Handle DISAS_EXIT_NEXT in delay slot Richard Henderson
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 10+ messages in thread
From: Richard Henderson @ 2020-08-31 18:40 UTC (permalink / raw)
  To: qemu-devel; +Cc: edgar.iglesias

Rather than look for the combination of DISAS_NEXT with a separate
variable, go ahead and set is_jmp to the desired state.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/microblaze/translate.c | 34 ++++++++++------------------------
 1 file changed, 10 insertions(+), 24 deletions(-)

diff --git a/target/microblaze/translate.c b/target/microblaze/translate.c
index 0733728794..9c52448c06 100644
--- a/target/microblaze/translate.c
+++ b/target/microblaze/translate.c
@@ -70,7 +70,6 @@ typedef struct DisasContext {
 
     /* Decoder.  */
     uint32_t ext_imm;
-    unsigned int cpustate_changed;
     unsigned int tb_flags;
     unsigned int tb_flags_to_set;
     int mem_index;
@@ -1255,7 +1254,7 @@ static bool trans_mbar(DisasContext *dc, arg_mbar *arg)
      *
      * Therefore, choose to end the TB always.
      */
-    dc->cpustate_changed = 1;
+    dc->base.is_jmp = DISAS_EXIT_NEXT;
     return true;
 }
 
@@ -1307,19 +1306,6 @@ static void msr_read(DisasContext *dc, TCGv_i32 d)
     tcg_temp_free_i32(t);
 }
 
-#ifndef CONFIG_USER_ONLY
-static void msr_write(DisasContext *dc, TCGv_i32 v)
-{
-    dc->cpustate_changed = 1;
-
-    /* Install MSR_C.  */
-    tcg_gen_extract_i32(cpu_msr_c, v, 2, 1);
-
-    /* Clear MSR_C and MSR_CC; MSR_PVR is not writable, and is always clear. */
-    tcg_gen_andi_i32(cpu_msr, v, ~(MSR_C | MSR_CC | MSR_PVR));
-}
-#endif
-
 static bool do_msrclrset(DisasContext *dc, arg_type_msr *arg, bool set)
 {
     uint32_t imm = arg->imm;
@@ -1352,7 +1338,7 @@ static bool do_msrclrset(DisasContext *dc, arg_type_msr *arg, bool set)
         } else {
             tcg_gen_andi_i32(cpu_msr, cpu_msr, ~imm);
         }
-        dc->cpustate_changed = 1;
+        dc->base.is_jmp = DISAS_EXIT_NEXT;
     }
     return true;
 }
@@ -1385,7 +1371,13 @@ static bool trans_mts(DisasContext *dc, arg_mts *arg)
     TCGv_i32 src = reg_for_read(dc, arg->ra);
     switch (arg->rs) {
     case SR_MSR:
-        msr_write(dc, src);
+        /* Install MSR_C.  */
+        tcg_gen_extract_i32(cpu_msr_c, src, 2, 1);
+        /*
+         * Clear MSR_C and MSR_CC;
+         * MSR_PVR is not writable, and is always clear.
+         */
+        tcg_gen_andi_i32(cpu_msr, src, ~(MSR_C | MSR_CC | MSR_PVR));
         break;
     case SR_FSR:
         tcg_gen_st_i32(src, cpu_env, offsetof(CPUMBState, fsr));
@@ -1417,7 +1409,7 @@ static bool trans_mts(DisasContext *dc, arg_mts *arg)
         qemu_log_mask(LOG_GUEST_ERROR, "Invalid mts reg 0x%x\n", arg->rs);
         return true;
     }
-    dc->cpustate_changed = 1;
+    dc->base.is_jmp = DISAS_EXIT_NEXT;
     return true;
 #endif
 }
@@ -1629,7 +1621,6 @@ static void mb_tr_init_disas_context(DisasContextBase *dcb, CPUState *cs)
 
     dc->cpu = cpu;
     dc->tb_flags = dc->base.tb->flags;
-    dc->cpustate_changed = 0;
     dc->ext_imm = dc->base.tb->cs_base;
     dc->r0 = NULL;
     dc->r0_set = false;
@@ -1714,11 +1705,6 @@ static void mb_tr_translate_insn(DisasContextBase *dcb, CPUState *cs)
         }
         dc->base.is_jmp = DISAS_JUMP;
     }
-
-    /* Force an exit if the per-tb cpu state has changed.  */
-    if (dc->base.is_jmp == DISAS_NEXT && dc->cpustate_changed) {
-        dc->base.is_jmp = DISAS_EXIT_NEXT;
-    }
 }
 
 static void mb_tr_tb_stop(DisasContextBase *dcb, CPUState *cs)
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 4/6] target/microblaze: Handle DISAS_EXIT_NEXT in delay slot
  2020-08-31 18:40 [PATCH 0/6] target/microblaze: Use tcg_gen_lookup_and_goto_ptr Richard Henderson
                   ` (2 preceding siblings ...)
  2020-08-31 18:40 ` [PATCH 3/6] target/microblaze: Replace cpustate_changed with DISAS_EXIT_NEXT Richard Henderson
@ 2020-08-31 18:40 ` Richard Henderson
  2020-09-01  7:02   ` Edgar E. Iglesias
  2020-08-31 18:40 ` [PATCH 5/6] target/microblaze: Force rtid, rted, rtbd to exit Richard Henderson
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 10+ messages in thread
From: Richard Henderson @ 2020-08-31 18:40 UTC (permalink / raw)
  To: qemu-devel; +Cc: edgar.iglesias

It is legal to put an mts instruction into a delay slot.
We should continue to return to the main loop in that
case so that we recognize any pending interrupts.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/microblaze/translate.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/target/microblaze/translate.c b/target/microblaze/translate.c
index 9c52448c06..b116a0ce4f 100644
--- a/target/microblaze/translate.c
+++ b/target/microblaze/translate.c
@@ -1696,6 +1696,10 @@ static void mb_tr_translate_insn(DisasContextBase *dcb, CPUState *cs)
     dc->base.pc_next += 4;
 
     if (dc->jmp_cond != TCG_COND_NEVER && !(dc->tb_flags & D_FLAG)) {
+        /*
+         * Finish finish any return-from branch.
+         * TODO: Diagnose rtXd in delay slot of rtYd earlier.
+         */
         if (dc->tb_flags & DRTI_FLAG) {
             do_rti(dc);
         } else if (dc->tb_flags & DRTB_FLAG) {
@@ -1703,7 +1707,35 @@ static void mb_tr_translate_insn(DisasContextBase *dcb, CPUState *cs)
         } else if (dc->tb_flags & DRTE_FLAG) {
             do_rte(dc);
         }
-        dc->base.is_jmp = DISAS_JUMP;
+
+        /* Complete the branch, ending the TB. */
+        switch (dc->base.is_jmp) {
+        case DISAS_NORETURN:
+            /*
+             * E.g. illegal insn in a delay slot.  We've already exited
+             * and will handle D_FLAG in mb_cpu_do_interrupt.
+             */
+            break;
+        case DISAS_EXIT:
+            /*
+             * TODO: diagnose brk/brki in delay slot earlier.
+             * This would then fold into the illegal insn case above.
+             */
+            break;
+        case DISAS_NEXT:
+            /* Normal insn a delay slot.  */
+            dc->base.is_jmp = DISAS_JUMP;
+            break;
+        case DISAS_EXIT_NEXT:
+            /*
+             * E.g. mts insn in a delay slot.  Continue with btarget,
+             * but still return to the main loop.
+             */
+            dc->base.is_jmp = DISAS_EXIT_JUMP;
+            break;
+        default:
+            g_assert_not_reached();
+        }
     }
 }
 
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 5/6] target/microblaze: Force rtid, rted, rtbd to exit
  2020-08-31 18:40 [PATCH 0/6] target/microblaze: Use tcg_gen_lookup_and_goto_ptr Richard Henderson
                   ` (3 preceding siblings ...)
  2020-08-31 18:40 ` [PATCH 4/6] target/microblaze: Handle DISAS_EXIT_NEXT in delay slot Richard Henderson
@ 2020-08-31 18:40 ` Richard Henderson
  2020-08-31 18:40 ` [PATCH 6/6] target/microblaze: Use tcg_gen_lookup_and_goto_ptr Richard Henderson
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 10+ messages in thread
From: Richard Henderson @ 2020-08-31 18:40 UTC (permalink / raw)
  To: qemu-devel; +Cc: edgar.iglesias

These return-from-exception type instructions have modified
MSR to re-enable various forms of interrupt.  Force a return
to the main loop.

Consolidate the cleanup of tb_flags into mb_tr_translate_insn.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/microblaze/translate.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/target/microblaze/translate.c b/target/microblaze/translate.c
index b116a0ce4f..8d02b07b73 100644
--- a/target/microblaze/translate.c
+++ b/target/microblaze/translate.c
@@ -1518,7 +1518,6 @@ static void do_rti(DisasContext *dc)
     tcg_gen_or_i32(cpu_msr, cpu_msr, tmp);
 
     tcg_temp_free_i32(tmp);
-    dc->tb_flags &= ~DRTI_FLAG;
 }
 
 static void do_rtb(DisasContext *dc)
@@ -1531,7 +1530,6 @@ static void do_rtb(DisasContext *dc)
     tcg_gen_or_i32(cpu_msr, cpu_msr, tmp);
 
     tcg_temp_free_i32(tmp);
-    dc->tb_flags &= ~DRTB_FLAG;
 }
 
 static void do_rte(DisasContext *dc)
@@ -1545,7 +1543,6 @@ static void do_rte(DisasContext *dc)
     tcg_gen_or_i32(cpu_msr, cpu_msr, tmp);
 
     tcg_temp_free_i32(tmp);
-    dc->tb_flags &= ~DRTE_FLAG;
 }
 
 /* Insns connected to FSL or AXI stream attached devices.  */
@@ -1700,12 +1697,16 @@ static void mb_tr_translate_insn(DisasContextBase *dcb, CPUState *cs)
          * Finish finish any return-from branch.
          * TODO: Diagnose rtXd in delay slot of rtYd earlier.
          */
-        if (dc->tb_flags & DRTI_FLAG) {
-            do_rti(dc);
-        } else if (dc->tb_flags & DRTB_FLAG) {
-            do_rtb(dc);
-        } else if (dc->tb_flags & DRTE_FLAG) {
-            do_rte(dc);
+        uint32_t rt_ibe = dc->tb_flags & (DRTI_FLAG | DRTB_FLAG | DRTE_FLAG);
+        if (unlikely(rt_ibe != 0)) {
+            dc->tb_flags &= ~(DRTI_FLAG | DRTB_FLAG | DRTE_FLAG);
+            if (rt_ibe & DRTI_FLAG) {
+                do_rti(dc);
+            } else if (rt_ibe & DRTB_FLAG) {
+                do_rtb(dc);
+            } else {
+                do_rte(dc);
+            }
         }
 
         /* Complete the branch, ending the TB. */
@@ -1723,8 +1724,12 @@ static void mb_tr_translate_insn(DisasContextBase *dcb, CPUState *cs)
              */
             break;
         case DISAS_NEXT:
-            /* Normal insn a delay slot.  */
-            dc->base.is_jmp = DISAS_JUMP;
+            /*
+             * Normal insn a delay slot.
+             * However, the return-from-exception type insns should
+             * return to the main loop, as they have adjusted MSR.
+             */
+            dc->base.is_jmp = (rt_ibe ? DISAS_EXIT_JUMP : DISAS_JUMP);
             break;
         case DISAS_EXIT_NEXT:
             /*
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 6/6] target/microblaze: Use tcg_gen_lookup_and_goto_ptr
  2020-08-31 18:40 [PATCH 0/6] target/microblaze: Use tcg_gen_lookup_and_goto_ptr Richard Henderson
                   ` (4 preceding siblings ...)
  2020-08-31 18:40 ` [PATCH 5/6] target/microblaze: Force rtid, rted, rtbd to exit Richard Henderson
@ 2020-08-31 18:40 ` Richard Henderson
  2020-09-01  7:08 ` [PATCH 0/6] " Edgar E. Iglesias
  2020-09-01 10:00 ` Edgar E. Iglesias
  7 siblings, 0 replies; 10+ messages in thread
From: Richard Henderson @ 2020-08-31 18:40 UTC (permalink / raw)
  To: qemu-devel; +Cc: edgar.iglesias

Normal indirect jumps, or page-crossing direct jumps, can use
tcg_gen_lookup_and_goto_ptr to avoid returning to the main loop
simply to find an existing TB for the next pc.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/microblaze/translate.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/microblaze/translate.c b/target/microblaze/translate.c
index 8d02b07b73..2c166eeb92 100644
--- a/target/microblaze/translate.c
+++ b/target/microblaze/translate.c
@@ -147,7 +147,7 @@ static void gen_goto_tb(DisasContext *dc, int n, target_ulong dest)
         tcg_gen_exit_tb(dc->base.tb, n);
     } else {
         tcg_gen_movi_i32(cpu_pc, dest);
-        tcg_gen_exit_tb(NULL, 0);
+        tcg_gen_lookup_and_goto_ptr();
     }
     dc->base.is_jmp = DISAS_NORETURN;
 }
@@ -1803,7 +1803,7 @@ static void mb_tr_tb_stop(DisasContextBase *dcb, CPUState *cs)
         if (unlikely(cs->singlestep_enabled)) {
             gen_raise_exception(dc, EXCP_DEBUG);
         } else {
-            tcg_gen_exit_tb(NULL, 0);
+            tcg_gen_lookup_and_goto_ptr();
         }
         return;
 
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 4/6] target/microblaze: Handle DISAS_EXIT_NEXT in delay slot
  2020-08-31 18:40 ` [PATCH 4/6] target/microblaze: Handle DISAS_EXIT_NEXT in delay slot Richard Henderson
@ 2020-09-01  7:02   ` Edgar E. Iglesias
  0 siblings, 0 replies; 10+ messages in thread
From: Edgar E. Iglesias @ 2020-09-01  7:02 UTC (permalink / raw)
  To: Richard Henderson; +Cc: qemu-devel

On Mon, Aug 31, 2020 at 11:40:16AM -0700, Richard Henderson wrote:
> It is legal to put an mts instruction into a delay slot.
> We should continue to return to the main loop in that
> case so that we recognize any pending interrupts.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/microblaze/translate.c | 34 +++++++++++++++++++++++++++++++++-
>  1 file changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/target/microblaze/translate.c b/target/microblaze/translate.c
> index 9c52448c06..b116a0ce4f 100644
> --- a/target/microblaze/translate.c
> +++ b/target/microblaze/translate.c
> @@ -1696,6 +1696,10 @@ static void mb_tr_translate_insn(DisasContextBase *dcb, CPUState *cs)
>      dc->base.pc_next += 4;
>  
>      if (dc->jmp_cond != TCG_COND_NEVER && !(dc->tb_flags & D_FLAG)) {
> +        /*
> +         * Finish finish any return-from branch.

Typo, 2x finish.

With that fixed:
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 0/6] target/microblaze: Use tcg_gen_lookup_and_goto_ptr
  2020-08-31 18:40 [PATCH 0/6] target/microblaze: Use tcg_gen_lookup_and_goto_ptr Richard Henderson
                   ` (5 preceding siblings ...)
  2020-08-31 18:40 ` [PATCH 6/6] target/microblaze: Use tcg_gen_lookup_and_goto_ptr Richard Henderson
@ 2020-09-01  7:08 ` Edgar E. Iglesias
  2020-09-01 10:00 ` Edgar E. Iglesias
  7 siblings, 0 replies; 10+ messages in thread
From: Edgar E. Iglesias @ 2020-09-01  7:08 UTC (permalink / raw)
  To: Richard Henderson; +Cc: qemu-devel

On Mon, Aug 31, 2020 at 11:40:12AM -0700, Richard Henderson wrote:
> Based-on: <20200831160601.833692-1-richard.henderson@linaro.org>
> ("[PULL 00/76] target/microblaze improvements")
> 
> Hello again, Edgar.
> 
> I had dropped the tcg_gen_lookup_and_goto_ptr patch from the
> previous omnibus patch set, as you had reported lockups.
> 
> I have identified, by inspection, two cases in which we failed
> to return to the main loop even though we should have:
> 
> (1) Return-from-exception type instructions.
> 
> I had missed these before because they hadn't set cpustate_changed.
> This still worked fine because they are all indirect branches, and
> had exited immediately.
> 
> Fixed by distinguishing these cases from normal indirect branches
> before we start using lookup_and_goto_ptr.
> 
> (2) MTS in a branch delay slot.
> 
> We did not check dc->cpustate_changed before setting
> dc->base.is_jmp to DISAS_JUMP, which lost the fact that we
> need to return to the main loop.
> 
> This mostly works fine without lookup_and_goto_ptr, because
> we either (a) finished an indirect branch and returned to the
> main loop anyway or (b) we'd return to the main loop via some
> subsequent indirect branch, which would happen "soon enough".
> 
> We should have been able to see soft-lockup with the existing
> code in the case of a cpustate_changed in the delay slot of
> a loop of direct branches that all use goto_tb.  E.g.
> 
> 	brid	0
> 	 msrset MSR_IE
> 
> I.e. an immediate branch back to the same branch insn,
> re-enabling interrupts in the delay slot.  Probably not
> something that shows up in the wild.


Nice! Yes, this seems to fix the problem we ran into before.
Series looks good both in review and testing except for minor typo in a comment.

With the typo in patch #4 fixed:
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Tested-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>



> 
> ----
> 
> Follow-up question: The manual says that several classes of
> instructions are invalid in a branch delay slot, but does
> not say what action is taken, if any.
> 
> Some of these invalid cases could leave qemu in an inconsistent
> state.  Would it be legal for us to diagnose these cases with
> trap_illegal?  If not, what *should* we be doing?  We could also
> LOG_GUEST_ERROR for these either way.
> 
> I've added some TODO comments in these patches that are relevant.

Thanks, I'll try to dig out some details. A guest-error will likely
be needed anyway since some cores don't have exceptions enabled.
But we may want both.

Cheers,
Edgar


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 0/6] target/microblaze: Use tcg_gen_lookup_and_goto_ptr
  2020-08-31 18:40 [PATCH 0/6] target/microblaze: Use tcg_gen_lookup_and_goto_ptr Richard Henderson
                   ` (6 preceding siblings ...)
  2020-09-01  7:08 ` [PATCH 0/6] " Edgar E. Iglesias
@ 2020-09-01 10:00 ` Edgar E. Iglesias
  7 siblings, 0 replies; 10+ messages in thread
From: Edgar E. Iglesias @ 2020-09-01 10:00 UTC (permalink / raw)
  To: Richard Henderson; +Cc: qemu-devel

On Mon, Aug 31, 2020 at 11:40:12AM -0700, Richard Henderson wrote:
> Based-on: <20200831160601.833692-1-richard.henderson@linaro.org>
> ("[PULL 00/76] target/microblaze improvements")
> 
> Hello again, Edgar.
> 
> I had dropped the tcg_gen_lookup_and_goto_ptr patch from the
> previous omnibus patch set, as you had reported lockups.
> 
> I have identified, by inspection, two cases in which we failed
> to return to the main loop even though we should have:
> 
> (1) Return-from-exception type instructions.
> 
> I had missed these before because they hadn't set cpustate_changed.
> This still worked fine because they are all indirect branches, and
> had exited immediately.
> 
> Fixed by distinguishing these cases from normal indirect branches
> before we start using lookup_and_goto_ptr.
> 
> (2) MTS in a branch delay slot.
> 
> We did not check dc->cpustate_changed before setting
> dc->base.is_jmp to DISAS_JUMP, which lost the fact that we
> need to return to the main loop.
> 
> This mostly works fine without lookup_and_goto_ptr, because
> we either (a) finished an indirect branch and returned to the
> main loop anyway or (b) we'd return to the main loop via some
> subsequent indirect branch, which would happen "soon enough".
> 
> We should have been able to see soft-lockup with the existing
> code in the case of a cpustate_changed in the delay slot of
> a loop of direct branches that all use goto_tb.  E.g.
> 
> 	brid	0
> 	 msrset MSR_IE
> 
> I.e. an immediate branch back to the same branch insn,
> re-enabling interrupts in the delay slot.  Probably not
> something that shows up in the wild.
> 
> ----
> 
> Follow-up question: The manual says that several classes of
> instructions are invalid in a branch delay slot, but does
> not say what action is taken, if any.
> 
> Some of these invalid cases could leave qemu in an inconsistent
> state.  Would it be legal for us to diagnose these cases with
> trap_illegal?  If not, what *should* we be doing?  We could also
> LOG_GUEST_ERROR for these either way.

What I found out is that these result in undefined and undocumented
behaviour but that the behaviour is deterministic, i.e the cores
won't lock-up or exposed security issues or anything like that.
RTL will not raise exceptions on these either.

So I think LOG_GUEST_ERROR and treating as NOP is probably a good
approach. We can fix that in follow-up patches.

Cheers,
Edgar


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-09-01 10:09 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-08-31 18:40 [PATCH 0/6] target/microblaze: Use tcg_gen_lookup_and_goto_ptr Richard Henderson
2020-08-31 18:40 ` [PATCH 1/6] target/microblaze: Rename DISAS_UPDATE to DISAS_EXIT Richard Henderson
2020-08-31 18:40 ` [PATCH 2/6] target/microblaze: Introduce DISAS_EXIT_NEXT, DISAS_EXIT_JUMP Richard Henderson
2020-08-31 18:40 ` [PATCH 3/6] target/microblaze: Replace cpustate_changed with DISAS_EXIT_NEXT Richard Henderson
2020-08-31 18:40 ` [PATCH 4/6] target/microblaze: Handle DISAS_EXIT_NEXT in delay slot Richard Henderson
2020-09-01  7:02   ` Edgar E. Iglesias
2020-08-31 18:40 ` [PATCH 5/6] target/microblaze: Force rtid, rted, rtbd to exit Richard Henderson
2020-08-31 18:40 ` [PATCH 6/6] target/microblaze: Use tcg_gen_lookup_and_goto_ptr Richard Henderson
2020-09-01  7:08 ` [PATCH 0/6] " Edgar E. Iglesias
2020-09-01 10:00 ` Edgar E. Iglesias

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).