On 20 January 2014 19:34, Michael Walle <mich...@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 <mich...@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
