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

[Paolo Bonzini <pbonzini@redhat.com>]

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) {
+                        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)