| Message ID | 1390246471-25167-12-git-send-email-michael@walle.cc |
|---|---|
| State | New |
| Headers | show |
On 20 January 2014 19:34, Michael Walle <michael@walle.cc> wrote: > Instead of translating the instruction to a no-op, pause the VM and display > a message to the user. > > As a side effect, this also works for instructions where the operands are > only known at runtime. > > Signed-off-by: Michael Walle <michael@walle.cc> > --- > target-lm32/helper.h | 1 + > target-lm32/op_helper.c | 17 +++++++++ > target-lm32/translate.c | 91 +++++++++++++++++++++++++++++++---------------- > 3 files changed, 79 insertions(+), 30 deletions(-) > > diff --git a/target-lm32/helper.h b/target-lm32/helper.h > index ad44fdf..f4442e0 100644 > --- a/target-lm32/helper.h > +++ b/target-lm32/helper.h > @@ -13,5 +13,6 @@ DEF_HELPER_1(rcsr_im, i32, env) > DEF_HELPER_1(rcsr_ip, i32, env) > DEF_HELPER_1(rcsr_jtx, i32, env) > DEF_HELPER_1(rcsr_jrx, i32, env) > +DEF_HELPER_1(ill, void, env) > > #include "exec/def-helper.h" > diff --git a/target-lm32/op_helper.c b/target-lm32/op_helper.c > index 71f21d1..7189cb5 100644 > --- a/target-lm32/op_helper.c > +++ b/target-lm32/op_helper.c > @@ -8,6 +8,10 @@ > > #include "exec/softmmu_exec.h" > > +#ifndef CONFIG_USER_ONLY > +#include "sysemu/sysemu.h" > +#endif > + > #if !defined(CONFIG_USER_ONLY) > #define MMUSUFFIX _mmu > #define SHIFT 0 > @@ -39,6 +43,19 @@ void HELPER(hlt)(CPULM32State *env) > cpu_loop_exit(env); > } > > +void HELPER(ill)(CPULM32State *env) > +{ > +#ifndef CONFIG_USER_ONLY > + CPUState *cs = CPU(lm32_env_get_cpu(env)); > + fprintf(stderr, "VM paused due to illegal instruction. " > + "Connect a debugger or switch to the monitor console " > + "to find out more.\n"); > + qemu_system_vmstop_request(RUN_STATE_PAUSED); > + cs->halted = 1; > + raise_exception(env, EXCP_HALTED); > +#endif Not really convinced this is a great idea. "This one target CPU type does something that none of the others do" seems less than ideal for QEMU as a whole. > +} > + > void HELPER(wcsr_bp)(CPULM32State *env, uint32_t bp, uint32_t idx) > { > uint32_t addr = bp & ~1; > diff --git a/target-lm32/translate.c b/target-lm32/translate.c > index f20460a..43ea4e6 100644 > --- a/target-lm32/translate.c > +++ b/target-lm32/translate.c > @@ -122,6 +122,12 @@ static inline void t_gen_raise_exception(DisasContext *dc, uint32_t index) > tcg_temp_free_i32(tmp); > } > > +static inline void t_gen_illegal_insn(DisasContext *dc) > +{ > + tcg_gen_movi_tl(cpu_pc, dc->pc); > + gen_helper_ill(cpu_env); > +} > + > static void gen_goto_tb(DisasContext *dc, int n, target_ulong dest) > { > TranslationBlock *tb; > @@ -425,6 +431,7 @@ static void dec_divu(DisasContext *dc) > > if (!(dc->features & LM32_FEATURE_DIVIDE)) { > qemu_log_mask(LOG_GUEST_ERROR, "hardware divider is not available\n"); > + t_gen_illegal_insn(dc); > return; > } > > @@ -504,6 +511,7 @@ static void dec_modu(DisasContext *dc) > > if (!(dc->features & LM32_FEATURE_DIVIDE)) { > qemu_log_mask(LOG_GUEST_ERROR, "hardware divider is not available\n"); > + t_gen_illegal_insn(dc); > return; > } > > @@ -527,6 +535,7 @@ static void dec_mul(DisasContext *dc) > if (!(dc->features & LM32_FEATURE_MULTIPLY)) { > qemu_log_mask(LOG_GUEST_ERROR, > "hardware multiplier is not available\n"); > + t_gen_illegal_insn(dc); > return; > } > > @@ -595,17 +604,18 @@ static void dec_scall(DisasContext *dc) > LOG_DIS("scall\n"); > } else if (dc->imm5 == 2) { > LOG_DIS("break\n"); > - } else { > - qemu_log_mask(LOG_GUEST_ERROR, "invalid opcode @0x%x", dc->pc); > - return; > } > > if (dc->imm5 == 7) { > tcg_gen_movi_tl(cpu_pc, dc->pc); > t_gen_raise_exception(dc, EXCP_SYSTEMCALL); > - } else { > + } else if (dc->imm5 == 2) { > tcg_gen_movi_tl(cpu_pc, dc->pc); > t_gen_raise_exception(dc, EXCP_BREAKPOINT); > + } else { > + qemu_log_mask(LOG_GUEST_ERROR, "invalid opcode @0x%x", dc->pc); > + t_gen_illegal_insn(dc); > + return; > } This leaves this function with two consecutive identical if..elseif..else ladders: why not combine them together? (optionally, use switch(dc->imm5).) The rest looks OK. thanks -- PMM
Am Samstag, 1. Februar 2014, 19:06:40 schrieb Peter Maydell: > On 20 January 2014 19:34, Michael Walle <michael@walle.cc> wrote: > > Instead of translating the instruction to a no-op, pause the VM and > > display a message to the user. > > > > As a side effect, this also works for instructions where the operands are > > only known at runtime. > > > > Signed-off-by: Michael Walle <michael@walle.cc> > > --- > > > > target-lm32/helper.h | 1 + > > target-lm32/op_helper.c | 17 +++++++++ > > target-lm32/translate.c | 91 > > +++++++++++++++++++++++++++++++---------------- 3 files changed, 79 > > insertions(+), 30 deletions(-) > > > > diff --git a/target-lm32/helper.h b/target-lm32/helper.h > > index ad44fdf..f4442e0 100644 > > --- a/target-lm32/helper.h > > +++ b/target-lm32/helper.h > > @@ -13,5 +13,6 @@ DEF_HELPER_1(rcsr_im, i32, env) > > > > DEF_HELPER_1(rcsr_ip, i32, env) > > DEF_HELPER_1(rcsr_jtx, i32, env) > > DEF_HELPER_1(rcsr_jrx, i32, env) > > > > +DEF_HELPER_1(ill, void, env) > > > > #include "exec/def-helper.h" > > > > diff --git a/target-lm32/op_helper.c b/target-lm32/op_helper.c > > index 71f21d1..7189cb5 100644 > > --- a/target-lm32/op_helper.c > > +++ b/target-lm32/op_helper.c > > @@ -8,6 +8,10 @@ > > > > #include "exec/softmmu_exec.h" > > > > +#ifndef CONFIG_USER_ONLY > > +#include "sysemu/sysemu.h" > > +#endif > > + > > > > #if !defined(CONFIG_USER_ONLY) > > #define MMUSUFFIX _mmu > > #define SHIFT 0 > > > > @@ -39,6 +43,19 @@ void HELPER(hlt)(CPULM32State *env) > > > > cpu_loop_exit(env); > > > > } > > > > +void HELPER(ill)(CPULM32State *env) > > +{ > > +#ifndef CONFIG_USER_ONLY > > + CPUState *cs = CPU(lm32_env_get_cpu(env)); > > + fprintf(stderr, "VM paused due to illegal instruction. " > > + "Connect a debugger or switch to the monitor console " > > + "to find out more.\n"); > > + qemu_system_vmstop_request(RUN_STATE_PAUSED); > > + cs->halted = 1; > > + raise_exception(env, EXCP_HALTED); > > +#endif > > Not really convinced this is a great idea. "This one target CPU > type does something that none of the others do" seems less > than ideal for QEMU as a whole. this was discussed some time ago. unfortunately, i don't find the thread atm. the particular problem with the lm32 target is that there is no defined behaviour if some unsupported opcode is executed on the real hardware, esp. there are no exceptions if that happens. therefore the suggested behaviour was to just stop the emulation and give the developer the chance to look at it by connecting a debugger. keep in mind that the lm32 target is more a development aid, rather than a real virtualization target. -michael
diff --git a/target-lm32/helper.h b/target-lm32/helper.h index ad44fdf..f4442e0 100644 --- a/target-lm32/helper.h +++ b/target-lm32/helper.h @@ -13,5 +13,6 @@ DEF_HELPER_1(rcsr_im, i32, env) DEF_HELPER_1(rcsr_ip, i32, env) DEF_HELPER_1(rcsr_jtx, i32, env) DEF_HELPER_1(rcsr_jrx, i32, env) +DEF_HELPER_1(ill, void, env) #include "exec/def-helper.h" diff --git a/target-lm32/op_helper.c b/target-lm32/op_helper.c index 71f21d1..7189cb5 100644 --- a/target-lm32/op_helper.c +++ b/target-lm32/op_helper.c @@ -8,6 +8,10 @@ #include "exec/softmmu_exec.h" +#ifndef CONFIG_USER_ONLY +#include "sysemu/sysemu.h" +#endif + #if !defined(CONFIG_USER_ONLY) #define MMUSUFFIX _mmu #define SHIFT 0 @@ -39,6 +43,19 @@ void HELPER(hlt)(CPULM32State *env) cpu_loop_exit(env); } +void HELPER(ill)(CPULM32State *env) +{ +#ifndef CONFIG_USER_ONLY + CPUState *cs = CPU(lm32_env_get_cpu(env)); + fprintf(stderr, "VM paused due to illegal instruction. " + "Connect a debugger or switch to the monitor console " + "to find out more.\n"); + qemu_system_vmstop_request(RUN_STATE_PAUSED); + cs->halted = 1; + raise_exception(env, EXCP_HALTED); +#endif +} + void HELPER(wcsr_bp)(CPULM32State *env, uint32_t bp, uint32_t idx) { uint32_t addr = bp & ~1; diff --git a/target-lm32/translate.c b/target-lm32/translate.c index f20460a..43ea4e6 100644 --- a/target-lm32/translate.c +++ b/target-lm32/translate.c @@ -122,6 +122,12 @@ static inline void t_gen_raise_exception(DisasContext *dc, uint32_t index) tcg_temp_free_i32(tmp); } +static inline void t_gen_illegal_insn(DisasContext *dc) +{ + tcg_gen_movi_tl(cpu_pc, dc->pc); + gen_helper_ill(cpu_env); +} + static void gen_goto_tb(DisasContext *dc, int n, target_ulong dest) { TranslationBlock *tb; @@ -425,6 +431,7 @@ static void dec_divu(DisasContext *dc) if (!(dc->features & LM32_FEATURE_DIVIDE)) { qemu_log_mask(LOG_GUEST_ERROR, "hardware divider is not available\n"); + t_gen_illegal_insn(dc); return; } @@ -504,6 +511,7 @@ static void dec_modu(DisasContext *dc) if (!(dc->features & LM32_FEATURE_DIVIDE)) { qemu_log_mask(LOG_GUEST_ERROR, "hardware divider is not available\n"); + t_gen_illegal_insn(dc); return; } @@ -527,6 +535,7 @@ static void dec_mul(DisasContext *dc) if (!(dc->features & LM32_FEATURE_MULTIPLY)) { qemu_log_mask(LOG_GUEST_ERROR, "hardware multiplier is not available\n"); + t_gen_illegal_insn(dc); return; } @@ -595,17 +604,18 @@ static void dec_scall(DisasContext *dc) LOG_DIS("scall\n"); } else if (dc->imm5 == 2) { LOG_DIS("break\n"); - } else { - qemu_log_mask(LOG_GUEST_ERROR, "invalid opcode @0x%x", dc->pc); - return; } if (dc->imm5 == 7) { tcg_gen_movi_tl(cpu_pc, dc->pc); t_gen_raise_exception(dc, EXCP_SYSTEMCALL); - } else { + } else if (dc->imm5 == 2) { tcg_gen_movi_tl(cpu_pc, dc->pc); t_gen_raise_exception(dc, EXCP_BREAKPOINT); + } else { + qemu_log_mask(LOG_GUEST_ERROR, "invalid opcode @0x%x", dc->pc); + t_gen_illegal_insn(dc); + return; } } @@ -681,6 +691,7 @@ static void dec_sextb(DisasContext *dc) if (!(dc->features & LM32_FEATURE_SIGN_EXTEND)) { qemu_log_mask(LOG_GUEST_ERROR, "hardware sign extender is not available\n"); + t_gen_illegal_insn(dc); return; } @@ -694,6 +705,7 @@ static void dec_sexth(DisasContext *dc) if (!(dc->features & LM32_FEATURE_SIGN_EXTEND)) { qemu_log_mask(LOG_GUEST_ERROR, "hardware sign extender is not available\n"); + t_gen_illegal_insn(dc); return; } @@ -722,6 +734,7 @@ static void dec_sl(DisasContext *dc) if (!(dc->features & LM32_FEATURE_SHIFT)) { qemu_log_mask(LOG_GUEST_ERROR, "hardware shifter is not available\n"); + t_gen_illegal_insn(dc); return; } @@ -743,24 +756,32 @@ static void dec_sr(DisasContext *dc) LOG_DIS("sr r%d, r%d, r%d\n", dc->r2, dc->r0, dc->r1); } - if (!(dc->features & LM32_FEATURE_SHIFT)) { - if (dc->format == OP_FMT_RI) { - /* TODO: check r1 == 1 during runtime */ - } else { - if (dc->imm5 != 1) { - qemu_log_mask(LOG_GUEST_ERROR, - "hardware shifter is not available\n"); - return; - } - } - } - + /* The real CPU (w/o hardware shifter) only supports right shift by exactly + * one bit */ if (dc->format == OP_FMT_RI) { + if (!(dc->features & LM32_FEATURE_SHIFT) && (dc->imm5 != 1)) { + qemu_log_mask(LOG_GUEST_ERROR, + "hardware shifter is not available\n"); + t_gen_illegal_insn(dc); + return; + } tcg_gen_sari_tl(cpu_R[dc->r1], cpu_R[dc->r0], dc->imm5); } else { - TCGv t0 = tcg_temp_new(); + int l1 = gen_new_label(); + int l2 = gen_new_label(); + TCGv t0 = tcg_temp_local_new(); tcg_gen_andi_tl(t0, cpu_R[dc->r1], 0x1f); + + if (!(dc->features & LM32_FEATURE_SHIFT)) { + tcg_gen_brcondi_tl(TCG_COND_EQ, t0, 1, l1); + t_gen_illegal_insn(dc); + tcg_gen_br(l2); + } + + gen_set_label(l1); tcg_gen_sar_tl(cpu_R[dc->r2], cpu_R[dc->r0], t0); + gen_set_label(l2); + tcg_temp_free(t0); } } @@ -773,24 +794,30 @@ static void dec_sru(DisasContext *dc) LOG_DIS("sru r%d, r%d, r%d\n", dc->r2, dc->r0, dc->r1); } - if (!(dc->features & LM32_FEATURE_SHIFT)) { - if (dc->format == OP_FMT_RI) { - /* TODO: check r1 == 1 during runtime */ - } else { - if (dc->imm5 != 1) { - qemu_log_mask(LOG_GUEST_ERROR, - "hardware shifter is not available\n"); - return; - } - } - } - if (dc->format == OP_FMT_RI) { + if (!(dc->features & LM32_FEATURE_SHIFT) && (dc->imm5 != 1)) { + qemu_log_mask(LOG_GUEST_ERROR, + "hardware shifter is not available\n"); + t_gen_illegal_insn(dc); + return; + } tcg_gen_shri_tl(cpu_R[dc->r1], cpu_R[dc->r0], dc->imm5); } else { - TCGv t0 = tcg_temp_new(); + int l1 = gen_new_label(); + int l2 = gen_new_label(); + TCGv t0 = tcg_temp_local_new(); tcg_gen_andi_tl(t0, cpu_R[dc->r1], 0x1f); + + if (!(dc->features & LM32_FEATURE_SHIFT)) { + tcg_gen_brcondi_tl(TCG_COND_EQ, t0, 1, l1); + t_gen_illegal_insn(dc); + tcg_gen_br(l2); + } + + gen_set_label(l1); tcg_gen_shr_tl(cpu_R[dc->r2], cpu_R[dc->r0], t0); + gen_set_label(l2); + tcg_temp_free(t0); } } @@ -819,6 +846,7 @@ static void dec_user(DisasContext *dc) LOG_DIS("user"); qemu_log_mask(LOG_GUEST_ERROR, "user instruction undefined\n"); + t_gen_illegal_insn(dc); } static void dec_wcsr(DisasContext *dc) @@ -886,6 +914,7 @@ static void dec_wcsr(DisasContext *dc) if (dc->num_breakpoints <= no) { qemu_log_mask(LOG_GUEST_ERROR, "breakpoint #%i is not available\n", no); + t_gen_illegal_insn(dc); break; } gen_helper_wcsr_bp(cpu_env, cpu_R[dc->r1], tcg_const_i32(no)); @@ -898,6 +927,7 @@ static void dec_wcsr(DisasContext *dc) if (dc->num_watchpoints <= no) { qemu_log_mask(LOG_GUEST_ERROR, "watchpoint #%i is not available\n", no); + t_gen_illegal_insn(dc); break; } gen_helper_wcsr_wp(cpu_env, cpu_R[dc->r1], tcg_const_i32(no)); @@ -956,6 +986,7 @@ static void dec_xor(DisasContext *dc) static void dec_ill(DisasContext *dc) { qemu_log_mask(LOG_GUEST_ERROR, "invalid opcode 0x%02x\n", dc->opcode); + t_gen_illegal_insn(dc); } typedef void (*DecoderInfo)(DisasContext *dc);
Instead of translating the instruction to a no-op, pause the VM and display a message to the user. As a side effect, this also works for instructions where the operands are only known at runtime. Signed-off-by: Michael Walle <michael@walle.cc> --- target-lm32/helper.h | 1 + target-lm32/op_helper.c | 17 +++++++++ target-lm32/translate.c | 91 +++++++++++++++++++++++++++++++---------------- 3 files changed, 79 insertions(+), 30 deletions(-)