qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PULL v4 11/12] target-lm32: stop VM on illegal or unkn


From: Peter Maydell
Subject: Re: [Qemu-devel] [PULL v4 11/12] target-lm32: stop VM on illegal or unknown instruction
Date: Sat, 1 Feb 2014 18:06:40 +0000

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]