Re: [Qemu-devel] [PATCH] i386: fix breakpoints handling in icount mode

[Jan Kiszka]

On 2015-01-12 09:55, Paolo Bonzini wrote:
> On 12/01/2015 09:30, Jan Kiszka wrote:
>> I think this would only cure a symptom, but it doesn't explain why we
>> now hit cpu_handle_guest_debug which we do not before the patch:
> 
> That means we now exit with EXCP_DEBUG and we didn't before?
> 
> Something like this would be a more complete fix (it works if you have
> both gdb and CPU breakpoints), but I'm not sure if it's also a band-aid
> for the symptoms.
> 
> diff --git a/cpu-exec.c b/cpu-exec.c
> index a4f0eff..56139ac 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -302,7 +302,7 @@ static inline TranslationBlock *tb_find_fast(CPUArchState 
> *env)
>      return tb;
>  }
>  
> -static void cpu_handle_debug_exception(CPUArchState *env)
> +static int cpu_handle_debug_exception(CPUArchState *env)
>  {
>      CPUState *cpu = ENV_GET_CPU(env);
>      CPUClass *cc = CPU_GET_CLASS(cpu);
> @@ -314,7 +314,7 @@ static void cpu_handle_debug_exception(CPUArchState *env)
>          }
>      }
>  
> -    cc->debug_excp_handler(cpu);
> +    return cc->debug_excp_handler(cpu);
>  }
>  
>  /* main execution loop */
> @@ -375,12 +375,15 @@ int cpu_exec(CPUArchState *env)
>              if (cpu->exception_index >= 0) {
>                  if (cpu->exception_index >= EXCP_INTERRUPT) {
>                      /* exit request from the cpu execution loop */
> -                    ret = cpu->exception_index;
> -                    if (ret == EXCP_DEBUG) {
> -                        cpu_handle_debug_exception(env);
> +                    if (cpu->exception_index == EXCP_DEBUG) {
> +                        ret = cpu_handle_debug_exception(env);
> +                    } else {
> +                        ret = cpu->exception_index;
> +                    }
> +                    if (ret >= 0) {

This condition is always true for both 0 and EXCP_DEBUG.

> +                        cpu->exception_index = -1;
> +                        break;
>                      }
> -                    cpu->exception_index = -1;
> -                    break;
>                  } else {
>  #if defined(CONFIG_USER_ONLY)
>                      /* if user mode only, we simulate a fake exception
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 2098f1c..c1d6c20 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -95,7 +95,8 @@ struct TranslationBlock;
>   * @get_phys_page_debug: Callback for obtaining a physical address.
>   * @gdb_read_register: Callback for letting GDB read a register.
>   * @gdb_write_register: Callback for letting GDB write a register.
> - * @debug_excp_handler: Callback for handling debug exceptions.
> + * @debug_excp_handler: Callback for handling debug exceptions.  Should
> + * return either #EXCP_DEBUG or zero.
>   * @vmsd: State description for migration.
>   * @gdb_num_core_regs: Number of core registers accessible to GDB.
>   * @gdb_core_xml_file: File name for core registers GDB XML description.
> @@ -140,7 +141,7 @@ typedef struct CPUClass {
>      hwaddr (*get_phys_page_debug)(CPUState *cpu, vaddr addr);
>      int (*gdb_read_register)(CPUState *cpu, uint8_t *buf, int reg);
>      int (*gdb_write_register)(CPUState *cpu, uint8_t *buf, int reg);
> -    void (*debug_excp_handler)(CPUState *cpu);
> +    int (*debug_excp_handler)(CPUState *cpu);
>  
>      int (*write_elf64_note)(WriteCoreDumpFunction f, CPUState *cpu,
>                              int cpuid, void *opaque);
> diff --git a/qom/cpu.c b/qom/cpu.c
> index 9c68fa4..e86fec5 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -193,6 +193,11 @@ static bool cpu_common_virtio_is_big_endian(CPUState 
> *cpu)
>      return target_words_bigendian();
>  }
>  
> +static int cpu_common_debug_excp_handler(CPUState *cpu)
> +{
> +    return EXCP_DEBUG;
> +}
> +
>  static void cpu_common_noop(CPUState *cpu)
>  {
>  }
> @@ -340,7 +345,7 @@ static void cpu_class_init(ObjectClass *klass, void *data)
>      k->gdb_read_register = cpu_common_gdb_read_register;
>      k->gdb_write_register = cpu_common_gdb_write_register;
>      k->virtio_is_big_endian = cpu_common_virtio_is_big_endian;
> -    k->debug_excp_handler = cpu_common_noop;
> +    k->debug_excp_handler = cpu_common_debug_excp_handler;
>      k->cpu_exec_enter = cpu_common_noop;
>      k->cpu_exec_exit = cpu_common_noop;
>      k->cpu_exec_interrupt = cpu_common_exec_interrupt;
> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> index 2bed914..40b7f79 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -732,7 +732,7 @@ static bool check_breakpoints(ARMCPU *cpu)
>      return false;
>  }
>  
> -void arm_debug_excp_handler(CPUState *cs)
> +int arm_debug_excp_handler(CPUState *cs)
>  {
>      /* Called by core code when a watchpoint or breakpoint fires;
>       * need to check which one and raise the appropriate exception.
> @@ -756,9 +756,9 @@ void arm_debug_excp_handler(CPUState *cs)
>                  }
>                  env->exception.vaddress = wp_hit->hitaddr;
>                  raise_exception(env, EXCP_DATA_ABORT);
> -            } else {
> -                cpu_resume_from_signal(cs, NULL);
> +                return 0;
>              }
> +            cpu_resume_from_signal(cs, NULL);
>          }
>      } else {
>          if (check_breakpoints(cpu)) {
> @@ -771,8 +771,10 @@ void arm_debug_excp_handler(CPUState *cs)
>              }
>              /* FAR is UNKNOWN, so doesn't need setting */
>              raise_exception(env, EXCP_PREFETCH_ABORT);
> +            return 0;
>          }
>      }
> +    return EXCP_DEBUG;
>  }
>  
>  /* ??? Flag setting arithmetic is awkward because we need to do comparisons.
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index 4f1ddf7..a313424 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -1004,17 +1004,19 @@ bool check_hw_breakpoints(CPUX86State *env, bool 
> force_dr6_update)
>      return hit_enabled;
>  }
>  
> -void breakpoint_handler(CPUState *cs)
> +int breakpoint_handler(CPUState *cs)
>  {
>      X86CPU *cpu = X86_CPU(cs);
>      CPUX86State *env = &cpu->env;
>      CPUBreakpoint *bp;
> +    int ret = EXCP_DEBUG;
>  
>      if (cs->watchpoint_hit) {
>          if (cs->watchpoint_hit->flags & BP_CPU) {
>              cs->watchpoint_hit = NULL;
>              if (check_hw_breakpoints(env, false)) {
>                  raise_exception(env, EXCP01_DB);
> +                ret = 0;
>              } else {
>                  cpu_resume_from_signal(cs, NULL);
>              }
> @@ -1025,11 +1027,13 @@ void breakpoint_handler(CPUState *cs)
>                  if (bp->flags & BP_CPU) {
>                      check_hw_breakpoints(env, true);
>                      raise_exception(env, EXCP01_DB);
> +                    ret = 0;
>                  }
>                  break;
>              }
>          }
>      }
> +    return ret;
>  }
>  
>  typedef struct MCEInjectionParams {
> diff --git a/target-lm32/helper.c b/target-lm32/helper.c
> index 7a41f29..088d3fa 100644
> --- a/target-lm32/helper.c
> +++ b/target-lm32/helper.c
> @@ -125,17 +125,19 @@ static bool check_watchpoints(CPULM32State *env)
>      return false;
>  }
>  
> -void lm32_debug_excp_handler(CPUState *cs)
> +int lm32_debug_excp_handler(CPUState *cs)
>  {
>      LM32CPU *cpu = LM32_CPU(cs);
>      CPULM32State *env = &cpu->env;
>      CPUBreakpoint *bp;
> +    int ret = EXCP_DEBUG;
>  
>      if (cs->watchpoint_hit) {
>          if (cs->watchpoint_hit->flags & BP_CPU) {
>              cs->watchpoint_hit = NULL;
>              if (check_watchpoints(env)) {
>                  raise_exception(env, EXCP_WATCHPOINT);
> +                ret = 0;
>              } else {
>                  cpu_resume_from_signal(cs, NULL);
>              }
> @@ -145,11 +147,13 @@ void lm32_debug_excp_handler(CPUState *cs)
>              if (bp->pc == env->pc) {
>                  if (bp->flags & BP_CPU) {
>                      raise_exception(env, EXCP_BREAKPOINT);
> +                    ret = 0;
>                  }
>                  break;
>              }
>          }
>      }
> +    return ret;
>  }
>  
>  void lm32_cpu_do_interrupt(CPUState *cs)
> diff --git a/target-xtensa/helper.c b/target-xtensa/helper.c
> index d84d259..52f50a2 100644
> --- a/target-xtensa/helper.c
> +++ b/target-xtensa/helper.c
> @@ -79,7 +79,7 @@ static uint32_t check_hw_breakpoints(CPUXtensaState *env)
>      return 0;
>  }
>  
> -void xtensa_breakpoint_handler(CPUState *cs)
> +int xtensa_breakpoint_handler(CPUState *cs)
>  {
>      XtensaCPU *cpu = XTENSA_CPU(cs);
>      CPUXtensaState *env = &cpu->env;
> @@ -92,10 +92,12 @@ void xtensa_breakpoint_handler(CPUState *cs)
>              cause = check_hw_breakpoints(env);
>              if (cause) {
>                  debug_exception_env(env, cause);
> +                return 0;
>              }
>              cpu_resume_from_signal(cs, NULL);
>          }
>      }
> +    return EXCP_DEBUG;
>  }
>  
>  XtensaCPU *cpu_xtensa_init(const char *cpu_model)
> 
> 

Lost track of this: This doesn't build (EXCP_DEBUG not available to
qom/cpu.c, wrong breakpoint_handler prototype) and then, when the build
issues are fixed, it doesn't work.

Playing a bit with the code, I found out that this cures the issue:

diff --git a/target-i386/translate.c b/target-i386/translate.c
index 305ce50..57b607d 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -8006,6 +8006,7 @@ static inline void gen_intermediate_code_internal(X86CPU 
*cpu,
                 if (bp->pc == pc_ptr &&
                     !((bp->flags & BP_CPU) && (tb->flags & HF_RF_MASK))) {
                     gen_debug(dc, pc_ptr - dc->cs_base);
+                    pc_ptr = disas_insn(env, dc, pc_ptr);
                     goto done_generating;
                 }
             }

pc_ptr is used at the end of the function to calculate the tb size. I
suspect that the difference prevents that the breakpoint event is
associated with the stored location. Can someone explain this more
properly? Then I would happily pass patch credits.

Jan

signature.asc
Description: OpenPGP digital signature