* [PATCH 1/7] target/microblaze: Rename DISAS_UPDATE to DISAS_EXIT
2020-09-02 16:35 [PATCH 0/7] target/microblaze improvements Richard Henderson
@ 2020-09-02 16:35 ` Richard Henderson
2020-09-02 16:35 ` [PATCH 2/7] target/microblaze: Introduce DISAS_EXIT_NEXT, DISAS_EXIT_JUMP Richard Henderson
` (5 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Richard Henderson @ 2020-09-02 16:35 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.
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Tested-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
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/7] target/microblaze: Introduce DISAS_EXIT_NEXT, DISAS_EXIT_JUMP
2020-09-02 16:35 [PATCH 0/7] target/microblaze improvements Richard Henderson
2020-09-02 16:35 ` [PATCH 1/7] target/microblaze: Rename DISAS_UPDATE to DISAS_EXIT Richard Henderson
@ 2020-09-02 16:35 ` Richard Henderson
2020-09-02 16:35 ` [PATCH 3/7] target/microblaze: Replace cpustate_changed with DISAS_EXIT_NEXT Richard Henderson
` (4 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Richard Henderson @ 2020-09-02 16:35 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.
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Tested-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
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/7] target/microblaze: Replace cpustate_changed with DISAS_EXIT_NEXT
2020-09-02 16:35 [PATCH 0/7] target/microblaze improvements Richard Henderson
2020-09-02 16:35 ` [PATCH 1/7] target/microblaze: Rename DISAS_UPDATE to DISAS_EXIT Richard Henderson
2020-09-02 16:35 ` [PATCH 2/7] target/microblaze: Introduce DISAS_EXIT_NEXT, DISAS_EXIT_JUMP Richard Henderson
@ 2020-09-02 16:35 ` Richard Henderson
2020-09-02 16:35 ` [PATCH 4/7] target/microblaze: Handle DISAS_EXIT_NEXT in delay slot Richard Henderson
` (3 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Richard Henderson @ 2020-09-02 16:35 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.
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Tested-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
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/7] target/microblaze: Handle DISAS_EXIT_NEXT in delay slot
2020-09-02 16:35 [PATCH 0/7] target/microblaze improvements Richard Henderson
` (2 preceding siblings ...)
2020-09-02 16:35 ` [PATCH 3/7] target/microblaze: Replace cpustate_changed with DISAS_EXIT_NEXT Richard Henderson
@ 2020-09-02 16:35 ` Richard Henderson
2020-09-02 16:35 ` [PATCH 5/7] target/microblaze: Force rtid, rted, rtbd to exit Richard Henderson
` (2 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Richard Henderson @ 2020-09-02 16:35 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.
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Tested-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
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..459b25f8b9 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 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/7] target/microblaze: Force rtid, rted, rtbd to exit
2020-09-02 16:35 [PATCH 0/7] target/microblaze improvements Richard Henderson
` (3 preceding siblings ...)
2020-09-02 16:35 ` [PATCH 4/7] target/microblaze: Handle DISAS_EXIT_NEXT in delay slot Richard Henderson
@ 2020-09-02 16:35 ` Richard Henderson
2020-09-02 16:35 ` [PATCH 6/7] target/microblaze: Use tcg_gen_lookup_and_goto_ptr Richard Henderson
2020-09-02 16:35 ` [PATCH 7/7] target/microblaze: Diagnose invalid insns in delay slots Richard Henderson
6 siblings, 0 replies; 10+ messages in thread
From: Richard Henderson @ 2020-09-02 16:35 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.
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Tested-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
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 459b25f8b9..a94f3e340e 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 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/7] target/microblaze: Use tcg_gen_lookup_and_goto_ptr
2020-09-02 16:35 [PATCH 0/7] target/microblaze improvements Richard Henderson
` (4 preceding siblings ...)
2020-09-02 16:35 ` [PATCH 5/7] target/microblaze: Force rtid, rted, rtbd to exit Richard Henderson
@ 2020-09-02 16:35 ` Richard Henderson
2020-09-02 16:35 ` [PATCH 7/7] target/microblaze: Diagnose invalid insns in delay slots Richard Henderson
6 siblings, 0 replies; 10+ messages in thread
From: Richard Henderson @ 2020-09-02 16:35 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.
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Tested-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
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 a94f3e340e..4416361d08 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* [PATCH 7/7] target/microblaze: Diagnose invalid insns in delay slots
2020-09-02 16:35 [PATCH 0/7] target/microblaze improvements Richard Henderson
` (5 preceding siblings ...)
2020-09-02 16:35 ` [PATCH 6/7] target/microblaze: Use tcg_gen_lookup_and_goto_ptr Richard Henderson
@ 2020-09-02 16:35 ` Richard Henderson
2020-09-02 17:13 ` Philippe Mathieu-Daudé
2020-09-02 18:03 ` Richard Henderson
6 siblings, 2 replies; 10+ messages in thread
From: Richard Henderson @ 2020-09-02 16:35 UTC (permalink / raw)
To: qemu-devel; +Cc: edgar.iglesias
These cases result in undefined and undocumented behaviour but the
behaviour is deterministic, i.e cores will not lock-up or expose
security issues. However, RTL will not raise exceptions either.
Therefore, log a GUEST_ERROR and treat these cases as nops, to
avoid corner cases which could put qemu into an invalid state.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/microblaze/translate.c | 47 +++++++++++++++++++++++++++++------
1 file changed, 40 insertions(+), 7 deletions(-)
diff --git a/target/microblaze/translate.c b/target/microblaze/translate.c
index 4416361d08..caa4831aed 100644
--- a/target/microblaze/translate.c
+++ b/target/microblaze/translate.c
@@ -179,6 +179,20 @@ static bool trap_userspace(DisasContext *dc, bool cond)
return cond_user;
}
+/*
+ * Return true, and log an error, if the current insn is
+ * within a delay slot.
+ */
+static bool invalid_delay_slot(DisasContext *dc, const char *insn_type)
+{
+ if (dc->tb_flags & D_FLAG) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "Invalid insn in delay slot: %s\n", insn_type);
+ return true;
+ }
+ return false;
+}
+
static TCGv_i32 reg_for_read(DisasContext *dc, int reg)
{
if (likely(reg != 0)) {
@@ -500,6 +514,9 @@ DO_TYPEA_CFG(idivu, use_div, true, gen_idivu)
static bool trans_imm(DisasContext *dc, arg_imm *arg)
{
+ if (invalid_delay_slot(dc, "imm")) {
+ return true;
+ }
dc->ext_imm = arg->imm << 16;
tcg_gen_movi_i32(cpu_imm, dc->ext_imm);
dc->tb_flags_to_set = IMM_FLAG;
@@ -1067,6 +1084,9 @@ static bool do_branch(DisasContext *dc, int dest_rb, int dest_imm,
{
uint32_t add_pc;
+ if (invalid_delay_slot(dc, "branch")) {
+ return true;
+ }
if (delay) {
setup_dslot(dc, dest_rb < 0);
}
@@ -1106,6 +1126,9 @@ static bool do_bcc(DisasContext *dc, int dest_rb, int dest_imm,
{
TCGv_i32 zero, next;
+ if (invalid_delay_slot(dc, "bcc")) {
+ return true;
+ }
if (delay) {
setup_dslot(dc, dest_rb < 0);
}
@@ -1158,6 +1181,10 @@ static bool trans_brk(DisasContext *dc, arg_typea_br *arg)
if (trap_userspace(dc, true)) {
return true;
}
+ if (invalid_delay_slot(dc, "brk")) {
+ return true;
+ }
+
tcg_gen_mov_i32(cpu_pc, reg_for_read(dc, arg->rb));
if (arg->rd) {
tcg_gen_movi_i32(cpu_R[arg->rd], dc->base.pc_next);
@@ -1176,6 +1203,10 @@ static bool trans_brki(DisasContext *dc, arg_typeb_br *arg)
if (trap_userspace(dc, imm != 0x8 && imm != 0x18)) {
return true;
}
+ if (invalid_delay_slot(dc, "brki")) {
+ return true;
+ }
+
tcg_gen_movi_i32(cpu_pc, imm);
if (arg->rd) {
tcg_gen_movi_i32(cpu_R[arg->rd], dc->base.pc_next);
@@ -1216,6 +1247,11 @@ static bool trans_mbar(DisasContext *dc, arg_mbar *arg)
{
int mbar_imm = arg->imm;
+ /* Note that mbar is a specialized branch instruction. */
+ if (invalid_delay_slot(dc, "mbar")) {
+ return true;
+ }
+
/* Data access memory barrier. */
if ((mbar_imm & 2) == 0) {
tcg_gen_mb(TCG_BAR_SC | TCG_MO_ALL);
@@ -1263,6 +1299,10 @@ static bool do_rts(DisasContext *dc, arg_typeb_bc *arg, int to_set)
if (trap_userspace(dc, to_set)) {
return true;
}
+ if (invalid_delay_slot(dc, "rts")) {
+ return true;
+ }
+
dc->tb_flags_to_set |= to_set;
setup_dslot(dc, true);
@@ -1695,7 +1735,6 @@ static void mb_tr_translate_insn(DisasContextBase *dcb, CPUState *cs)
if (dc->jmp_cond != TCG_COND_NEVER && !(dc->tb_flags & D_FLAG)) {
/*
* Finish any return-from branch.
- * TODO: Diagnose rtXd in delay slot of rtYd earlier.
*/
uint32_t rt_ibe = dc->tb_flags & (DRTI_FLAG | DRTB_FLAG | DRTE_FLAG);
if (unlikely(rt_ibe != 0)) {
@@ -1717,12 +1756,6 @@ static void mb_tr_translate_insn(DisasContextBase *dcb, CPUState *cs)
* 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.
--
2.25.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 7/7] target/microblaze: Diagnose invalid insns in delay slots
2020-09-02 16:35 ` [PATCH 7/7] target/microblaze: Diagnose invalid insns in delay slots Richard Henderson
@ 2020-09-02 17:13 ` Philippe Mathieu-Daudé
2020-09-02 18:03 ` Richard Henderson
1 sibling, 0 replies; 10+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-09-02 17:13 UTC (permalink / raw)
To: Richard Henderson, qemu-devel; +Cc: edgar.iglesias
On 9/2/20 6:35 PM, Richard Henderson wrote:
> These cases result in undefined and undocumented behaviour but the
> behaviour is deterministic, i.e cores will not lock-up or expose
> security issues. However, RTL will not raise exceptions either.
>
> Therefore, log a GUEST_ERROR and treat these cases as nops, to
> avoid corner cases which could put qemu into an invalid state.
Excellent! This is the same issue I have with TX39 MIPS cores :)
Thanks for the clean fix.
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> target/microblaze/translate.c | 47 +++++++++++++++++++++++++++++------
> 1 file changed, 40 insertions(+), 7 deletions(-)
>
> diff --git a/target/microblaze/translate.c b/target/microblaze/translate.c
> index 4416361d08..caa4831aed 100644
> --- a/target/microblaze/translate.c
> +++ b/target/microblaze/translate.c
> @@ -179,6 +179,20 @@ static bool trap_userspace(DisasContext *dc, bool cond)
> return cond_user;
> }
>
> +/*
> + * Return true, and log an error, if the current insn is
> + * within a delay slot.
> + */
> +static bool invalid_delay_slot(DisasContext *dc, const char *insn_type)
> +{
> + if (dc->tb_flags & D_FLAG) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "Invalid insn in delay slot: %s\n", insn_type);
> + return true;
> + }
> + return false;
> +}
> +
> static TCGv_i32 reg_for_read(DisasContext *dc, int reg)
> {
> if (likely(reg != 0)) {
> @@ -500,6 +514,9 @@ DO_TYPEA_CFG(idivu, use_div, true, gen_idivu)
>
> static bool trans_imm(DisasContext *dc, arg_imm *arg)
> {
> + if (invalid_delay_slot(dc, "imm")) {
> + return true;
> + }
> dc->ext_imm = arg->imm << 16;
> tcg_gen_movi_i32(cpu_imm, dc->ext_imm);
> dc->tb_flags_to_set = IMM_FLAG;
> @@ -1067,6 +1084,9 @@ static bool do_branch(DisasContext *dc, int dest_rb, int dest_imm,
> {
> uint32_t add_pc;
>
> + if (invalid_delay_slot(dc, "branch")) {
> + return true;
> + }
> if (delay) {
> setup_dslot(dc, dest_rb < 0);
> }
> @@ -1106,6 +1126,9 @@ static bool do_bcc(DisasContext *dc, int dest_rb, int dest_imm,
> {
> TCGv_i32 zero, next;
>
> + if (invalid_delay_slot(dc, "bcc")) {
> + return true;
> + }
> if (delay) {
> setup_dslot(dc, dest_rb < 0);
> }
> @@ -1158,6 +1181,10 @@ static bool trans_brk(DisasContext *dc, arg_typea_br *arg)
> if (trap_userspace(dc, true)) {
> return true;
> }
> + if (invalid_delay_slot(dc, "brk")) {
> + return true;
> + }
> +
> tcg_gen_mov_i32(cpu_pc, reg_for_read(dc, arg->rb));
> if (arg->rd) {
> tcg_gen_movi_i32(cpu_R[arg->rd], dc->base.pc_next);
> @@ -1176,6 +1203,10 @@ static bool trans_brki(DisasContext *dc, arg_typeb_br *arg)
> if (trap_userspace(dc, imm != 0x8 && imm != 0x18)) {
> return true;
> }
> + if (invalid_delay_slot(dc, "brki")) {
> + return true;
> + }
> +
> tcg_gen_movi_i32(cpu_pc, imm);
> if (arg->rd) {
> tcg_gen_movi_i32(cpu_R[arg->rd], dc->base.pc_next);
> @@ -1216,6 +1247,11 @@ static bool trans_mbar(DisasContext *dc, arg_mbar *arg)
> {
> int mbar_imm = arg->imm;
>
> + /* Note that mbar is a specialized branch instruction. */
> + if (invalid_delay_slot(dc, "mbar")) {
> + return true;
> + }
> +
> /* Data access memory barrier. */
> if ((mbar_imm & 2) == 0) {
> tcg_gen_mb(TCG_BAR_SC | TCG_MO_ALL);
> @@ -1263,6 +1299,10 @@ static bool do_rts(DisasContext *dc, arg_typeb_bc *arg, int to_set)
> if (trap_userspace(dc, to_set)) {
> return true;
> }
> + if (invalid_delay_slot(dc, "rts")) {
> + return true;
> + }
> +
> dc->tb_flags_to_set |= to_set;
> setup_dslot(dc, true);
>
> @@ -1695,7 +1735,6 @@ static void mb_tr_translate_insn(DisasContextBase *dcb, CPUState *cs)
> if (dc->jmp_cond != TCG_COND_NEVER && !(dc->tb_flags & D_FLAG)) {
> /*
> * Finish any return-from branch.
> - * TODO: Diagnose rtXd in delay slot of rtYd earlier.
> */
> uint32_t rt_ibe = dc->tb_flags & (DRTI_FLAG | DRTB_FLAG | DRTE_FLAG);
> if (unlikely(rt_ibe != 0)) {
> @@ -1717,12 +1756,6 @@ static void mb_tr_translate_insn(DisasContextBase *dcb, CPUState *cs)
> * 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.
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 7/7] target/microblaze: Diagnose invalid insns in delay slots
2020-09-02 16:35 ` [PATCH 7/7] target/microblaze: Diagnose invalid insns in delay slots Richard Henderson
2020-09-02 17:13 ` Philippe Mathieu-Daudé
@ 2020-09-02 18:03 ` Richard Henderson
1 sibling, 0 replies; 10+ messages in thread
From: Richard Henderson @ 2020-09-02 18:03 UTC (permalink / raw)
To: qemu-devel; +Cc: edgar.iglesias
On 9/2/20 9:35 AM, Richard Henderson wrote:
> +static bool invalid_delay_slot(DisasContext *dc, const char *insn_type)
> +{
> + if (dc->tb_flags & D_FLAG) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "Invalid insn in delay slot: %s\n", insn_type);
I should probably log the pc as well.
r~
^ permalink raw reply [flat|nested] 10+ messages in thread