diff mbox

[v2,6/9] target-i386: Handle I/O breakpoints

Message ID 1445012619-23772-7-git-send-email-ehabkost@redhat.com
State New
Headers show

Commit Message

Eduardo Habkost Oct. 16, 2015, 4:23 p.m. UTC 
From: Richard Henderson <rth@twiddle.net>

Signed-off-by: Richard Henderson <rth@twiddle.net>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v1 -> v2:
* Rebase on top of new version of "Re-introduce optimal breakpoint
  removal"
---
 target-i386/bpt_helper.c | 98 +++++++++++++++++++++++++++++++++++-------------
 target-i386/cpu.h        |  2 +
 target-i386/helper.h     |  1 +
 target-i386/translate.c  | 20 +++++++++-
 4 files changed, 93 insertions(+), 28 deletions(-)

Comments

Eduardo Habkost Oct. 19, 2015, 5:30 p.m. UTC | #1 
On Fri, Oct 16, 2015 at 01:23:36PM -0300, Eduardo Habkost wrote:
> From: Richard Henderson <rth@twiddle.net>
> 
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Changes v1 -> v2:
> * Rebase on top of new version of "Re-introduce optimal breakpoint
>   removal"
> ---
>  target-i386/bpt_helper.c | 98 +++++++++++++++++++++++++++++++++++-------------
>  target-i386/cpu.h        |  2 +
>  target-i386/helper.h     |  1 +
>  target-i386/translate.c  | 20 +++++++++-
>  4 files changed, 93 insertions(+), 28 deletions(-)
> 
> diff --git a/target-i386/bpt_helper.c b/target-i386/bpt_helper.c
> index 0563abd..00f3942 100644
> --- a/target-i386/bpt_helper.c
> +++ b/target-i386/bpt_helper.c
> @@ -49,60 +49,72 @@ static inline int hw_breakpoint_len(unsigned long dr7, int index)
>      return (len == 2) ? 8 : len + 1;
>  }
>  
> -static void hw_breakpoint_insert(CPUX86State *env, int index)
> +static int hw_breakpoint_insert(CPUX86State *env, int index)
>  {
>      CPUState *cs = CPU(x86_env_get_cpu(env));
> -    int type = 0, err = 0;
> +    target_ulong dr7 = env->dr[7];
> +    target_ulong drN = env->dr[index];
> +    int err = 0;
>  
> -    switch (hw_breakpoint_type(env->dr[7], index)) {
> +    switch (hw_breakpoint_type(dr7, index)) {
>      case DR7_TYPE_BP_INST:
> -        if (hw_breakpoint_enabled(env->dr[7], index)) {
> -            err = cpu_breakpoint_insert(cs, env->dr[index], BP_CPU,
> +        if (hw_breakpoint_enabled(dr7, index)) {
> +            err = cpu_breakpoint_insert(cs, drN, BP_CPU,
>                                          &env->cpu_breakpoint[index]);
>          }
>          break;
> -    case DR7_TYPE_DATA_WR:
> -        type = BP_CPU | BP_MEM_WRITE;
> -        break;
> +
>      case DR7_TYPE_IO_RW:
> -        /* No support for I/O watchpoints yet */
> +        /* Notice when we should enable calls to bpt_io.  */
> +        return (hw_breakpoint_enabled(env->dr[7], index)
> +                ? HF_IOBPT_MASK : 0);

checkpatch.pl error:

  ERROR: return is not a function, parentheses are not required
  #57: FILE: target-i386/bpt_helper.c:69:
  +        return (hw_breakpoint_enabled(env->dr[7], index)
  
  total: 1 errors, 0 warnings, 242 lines checked

I will fix it in v3.
Richard Henderson Oct. 19, 2015, 5:46 p.m. UTC | #2 
On 10/19/2015 07:30 AM, Eduardo Habkost wrote:
>> >+        /* Notice when we should enable calls to bpt_io.  */
>> >+        return (hw_breakpoint_enabled(env->dr[7], index)
>> >+                ? HF_IOBPT_MASK : 0);
> checkpatch.pl error:
>
>    ERROR: return is not a function, parentheses are not required
>    #57: FILE: target-i386/bpt_helper.c:69:
>    +        return (hw_breakpoint_enabled(env->dr[7], index)
>
>    total: 1 errors, 0 warnings, 242 lines checked
>
> I will fix it in v3.

In this case checkpatch is wrong, imo.  The parenthesis are not there to "make 
return a function", but to make the multi-line expression indent properly.


r~
Eduardo Habkost Oct. 19, 2015, 5:57 p.m. UTC | #3 
On Mon, Oct 19, 2015 at 07:46:51AM -1000, Richard Henderson wrote:
> On 10/19/2015 07:30 AM, Eduardo Habkost wrote:
> >>>+        /* Notice when we should enable calls to bpt_io.  */
> >>>+        return (hw_breakpoint_enabled(env->dr[7], index)
> >>>+                ? HF_IOBPT_MASK : 0);
> >checkpatch.pl error:
> >
> >   ERROR: return is not a function, parentheses are not required
> >   #57: FILE: target-i386/bpt_helper.c:69:
> >   +        return (hw_breakpoint_enabled(env->dr[7], index)
> >
> >   total: 1 errors, 0 warnings, 242 lines checked
> >
> >I will fix it in v3.
> 
> In this case checkpatch is wrong, imo.  The parenthesis are not there to
> "make return a function", but to make the multi-line expression indent
> properly.

I understand if one thinks the expression looks better with the parenthesis,
but I fail to see why they are needed to indent the expression properly.

For reference, this is the change I have made in v3:

    diff --git a/target-i386/bpt_helper.c b/target-i386/bpt_helper.c
    index 0fbdc03..dac1b1a 100644
    --- a/target-i386/bpt_helper.c
    +++ b/target-i386/bpt_helper.c
    @@ -66,8 +66,8 @@ static int hw_breakpoint_insert(CPUX86State *env, int index)
    
         case DR7_TYPE_IO_RW:
             /* Notice when we should enable calls to bpt_io.  */
    -        return (hw_breakpoint_enabled(env->dr[7], index)
    -                ? HF_IOBPT_MASK : 0);
    +        return hw_breakpoint_enabled(env->dr[7], index)
    +               ? HF_IOBPT_MASK : 0;
    
         case DR7_TYPE_DATA_WR:
             if (hw_breakpoint_enabled(dr7, index)) {
Paolo Bonzini Oct. 20, 2015, 12:04 p.m. UTC | #4 
On 19/10/2015 19:57, Eduardo Habkost wrote:
> On Mon, Oct 19, 2015 at 07:46:51AM -1000, Richard Henderson wrote:
>> On 10/19/2015 07:30 AM, Eduardo Habkost wrote:
>>>>> +        /* Notice when we should enable calls to bpt_io.  */
>>>>> +        return (hw_breakpoint_enabled(env->dr[7], index)
>>>>> +                ? HF_IOBPT_MASK : 0);
>>> checkpatch.pl error:
>>>
>>>   ERROR: return is not a function, parentheses are not required
>>>   #57: FILE: target-i386/bpt_helper.c:69:
>>>   +        return (hw_breakpoint_enabled(env->dr[7], index)
>>>
>>>   total: 1 errors, 0 warnings, 242 lines checked
>>>
>>> I will fix it in v3.
>>
>> In this case checkpatch is wrong, imo.  The parenthesis are not there to
>> "make return a function", but to make the multi-line expression indent
>> properly.
> 
> I understand if one thinks the expression looks better with the parenthesis,
> but I fail to see why they are needed to indent the expression properly.

Because Emacs indents this:

>     +        return hw_breakpoint_enabled(env->dr[7], index)
>     +               ? HF_IOBPT_MASK : 0;

with the ? under the second "r" of "return", while it indents this as
written:

>     -        return (hw_breakpoint_enabled(env->dr[7], index)
>     -                ? HF_IOBPT_MASK : 0);

Another random example:

static bool sdl_check_format(DisplayChangeListener *dcl,
                             pixman_format_code_t format)
{
    /*
     * We let SDL convert for us a few more formats than,
     * the native ones. Thes are the ones I have tested.
     */
    return (format == PIXMAN_x8r8g8b8 ||
            format == PIXMAN_b8g8r8x8 ||
            format == PIXMAN_x1r5g5b5 ||
            format == PIXMAN_r5g6b5);
}

There's no unanimity though, so your v3 is okay too.

Paolo
diff mbox

Patch

diff --git a/target-i386/bpt_helper.c b/target-i386/bpt_helper.c
index 0563abd..00f3942 100644
--- a/target-i386/bpt_helper.c
+++ b/target-i386/bpt_helper.c
@@ -49,60 +49,72 @@  static inline int hw_breakpoint_len(unsigned long dr7, int index)
     return (len == 2) ? 8 : len + 1;
 }
 
-static void hw_breakpoint_insert(CPUX86State *env, int index)
+static int hw_breakpoint_insert(CPUX86State *env, int index)
 {
     CPUState *cs = CPU(x86_env_get_cpu(env));
-    int type = 0, err = 0;
+    target_ulong dr7 = env->dr[7];
+    target_ulong drN = env->dr[index];
+    int err = 0;
 
-    switch (hw_breakpoint_type(env->dr[7], index)) {
+    switch (hw_breakpoint_type(dr7, index)) {
     case DR7_TYPE_BP_INST:
-        if (hw_breakpoint_enabled(env->dr[7], index)) {
-            err = cpu_breakpoint_insert(cs, env->dr[index], BP_CPU,
+        if (hw_breakpoint_enabled(dr7, index)) {
+            err = cpu_breakpoint_insert(cs, drN, BP_CPU,
                                         &env->cpu_breakpoint[index]);
         }
         break;
-    case DR7_TYPE_DATA_WR:
-        type = BP_CPU | BP_MEM_WRITE;
-        break;
+
     case DR7_TYPE_IO_RW:
-        /* No support for I/O watchpoints yet */
+        /* Notice when we should enable calls to bpt_io.  */
+        return (hw_breakpoint_enabled(env->dr[7], index)
+                ? HF_IOBPT_MASK : 0);
+
+    case DR7_TYPE_DATA_WR:
+        if (hw_breakpoint_enabled(dr7, index)) {
+            err = cpu_watchpoint_insert(cs, drN,
+                                        hw_breakpoint_len(dr7, index),
+                                        BP_CPU | BP_MEM_WRITE,
+                                        &env->cpu_watchpoint[index]);
+        }
         break;
+
     case DR7_TYPE_DATA_RW:
-        type = BP_CPU | BP_MEM_ACCESS;
+        if (hw_breakpoint_enabled(dr7, index)) {
+            err = cpu_watchpoint_insert(cs, drN,
+                                        hw_breakpoint_len(dr7, index),
+                                        BP_CPU | BP_MEM_ACCESS,
+                                        &env->cpu_watchpoint[index]);
+        }
         break;
     }
-
-    if (type != 0) {
-        err = cpu_watchpoint_insert(cs, env->dr[index],
-                                    hw_breakpoint_len(env->dr[7], index),
-                                    type, &env->cpu_watchpoint[index]);
-    }
-
     if (err) {
         env->cpu_breakpoint[index] = NULL;
     }
+    return 0;
 }
 
 static void hw_breakpoint_remove(CPUX86State *env, int index)
 {
-    CPUState *cs;
+    CPUState *cs = CPU(x86_env_get_cpu(env));
 
-    if (!env->cpu_breakpoint[index]) {
-        return;
-    }
-    cs = CPU(x86_env_get_cpu(env));
     switch (hw_breakpoint_type(env->dr[7], index)) {
     case DR7_TYPE_BP_INST:
-        if (hw_breakpoint_enabled(env->dr[7], index)) {
+        if (env->cpu_breakpoint[index]) {
             cpu_breakpoint_remove_by_ref(cs, env->cpu_breakpoint[index]);
+            env->cpu_breakpoint[index] = NULL;
         }
         break;
+
     case DR7_TYPE_DATA_WR:
     case DR7_TYPE_DATA_RW:
-        cpu_watchpoint_remove_by_ref(cs, env->cpu_watchpoint[index]);
+        if (env->cpu_breakpoint[index]) {
+            cpu_watchpoint_remove_by_ref(cs, env->cpu_watchpoint[index]);
+            env->cpu_breakpoint[index] = NULL;
+        }
         break;
+
     case DR7_TYPE_IO_RW:
-        /* No support for I/O watchpoints yet */
+        /* HF_IOBPT_MASK cleared elsewhere.  */
         break;
     }
 }
@@ -110,6 +122,7 @@  static void hw_breakpoint_remove(CPUX86State *env, int index)
 void cpu_x86_update_dr7(CPUX86State *env, uint32_t new_dr7)
 {
     target_ulong old_dr7 = env->dr[7];
+    int iobpt = 0;
     int i;
 
     /* If nothing is changing except the global/local enable bits,
@@ -128,7 +141,10 @@  void cpu_x86_update_dr7(CPUX86State *env, uint32_t new_dr7)
         env->dr[7] = new_dr7;
         for (i = 0; i < DR7_MAX_BP; i++) {
             if (mod & (2 << i * 2) && hw_breakpoint_enabled(new_dr7, i)) {
-                hw_breakpoint_insert(env, i);
+                iobpt |= hw_breakpoint_insert(env, i);
+            } else if (hw_breakpoint_type(new_dr7, i) == DR7_TYPE_IO_RW
+                       && hw_breakpoint_enabled(new_dr7, i)) {
+                iobpt |= HF_IOBPT_MASK;
             }
         }
     } else {
@@ -137,11 +153,12 @@  void cpu_x86_update_dr7(CPUX86State *env, uint32_t new_dr7)
         }
         env->dr[7] = new_dr7;
         for (i = 0; i < DR7_MAX_BP; i++) {
-            hw_breakpoint_insert(env, i);
+            iobpt |= hw_breakpoint_insert(env, i);
         }
     }
 
     env->dr[7] |= DR7_FIXED_1;
+    env->hflags = (env->hflags & ~HF_IOBPT_MASK) | iobpt;
 }
 
 static bool check_hw_breakpoints(CPUX86State *env, bool force_dr6_update)
@@ -243,3 +260,30 @@  void helper_movl_drN_T0(CPUX86State *env, int reg, target_ulong t0)
     }
 #endif
 }
+
+/* Check if Port I/O is trapped by a breakpoint.  */
+void helper_bpt_io(CPUX86State *env, uint32_t port,
+                   uint32_t size, target_ulong next_eip)
+{
+#ifndef CONFIG_USER_ONLY
+    target_ulong dr7 = env->dr[7];
+    int i, hit = 0;
+
+    for (i = 0; i < DR7_MAX_BP; ++i) {
+        if (hw_breakpoint_type(dr7, i) == DR7_TYPE_IO_RW
+            && hw_breakpoint_enabled(dr7, i)) {
+            int bpt_len = hw_breakpoint_len(dr7, i);
+            if (port + size - 1 >= env->dr[i]
+                && port <= env->dr[i] + bpt_len - 1) {
+                hit |= 1 << i;
+            }
+        }
+    }
+
+    if (hit) {
+        env->dr[6] = (env->dr[6] & ~0xf) | hit;
+        env->eip = next_eip;
+        raise_exception(env, EXCP01_DB);
+    }
+#endif
+}
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index c9e4edf..649d54a 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -155,6 +155,7 @@ 
 #define HF_SVMI_SHIFT       21 /* SVM intercepts are active */
 #define HF_OSFXSR_SHIFT     22 /* CR4.OSFXSR */
 #define HF_SMAP_SHIFT       23 /* CR4.SMAP */
+#define HF_IOBPT_SHIFT      24 /* an io breakpoint enabled */
 
 #define HF_CPL_MASK          (3 << HF_CPL_SHIFT)
 #define HF_SOFTMMU_MASK      (1 << HF_SOFTMMU_SHIFT)
@@ -178,6 +179,7 @@ 
 #define HF_SVMI_MASK         (1 << HF_SVMI_SHIFT)
 #define HF_OSFXSR_MASK       (1 << HF_OSFXSR_SHIFT)
 #define HF_SMAP_MASK         (1 << HF_SMAP_SHIFT)
+#define HF_IOBPT_MASK        (1 << HF_IOBPT_SHIFT)
 
 /* hflags2 */
 
diff --git a/target-i386/helper.h b/target-i386/helper.h
index 8454a04..e9858c0 100644
--- a/target-i386/helper.h
+++ b/target-i386/helper.h
@@ -92,6 +92,7 @@  DEF_HELPER_3(outw, void, env, i32, i32)
 DEF_HELPER_2(inw, tl, env, i32)
 DEF_HELPER_3(outl, void, env, i32, i32)
 DEF_HELPER_2(inl, tl, env, i32)
+DEF_HELPER_FLAGS_4(bpt_io, TCG_CALL_NO_WG, void, env, i32, i32, tl)
 
 DEF_HELPER_3(svm_check_intercept_param, void, env, i32, i64)
 DEF_HELPER_3(vmexit, void, env, i32, i64)
diff --git a/target-i386/translate.c b/target-i386/translate.c
index ef10e68..ceed4d1 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -1154,6 +1154,19 @@  static inline void gen_cmps(DisasContext *s, TCGMemOp ot)
     gen_op_add_reg_T0(s->aflag, R_EDI);
 }
 
+static void gen_bpt_io(DisasContext *s, TCGv_i32 t_port, int ot)
+{
+    if (s->flags & HF_IOBPT_MASK) {
+        TCGv_i32 t_size = tcg_const_i32(1 << ot);
+        TCGv t_next = tcg_const_tl(s->pc - s->cs_base);
+
+        gen_helper_bpt_io(cpu_env, t_port, t_size, t_next);
+        tcg_temp_free_i32(t_size);
+        tcg_temp_free(t_next);
+    }
+}
+
+
 static inline void gen_ins(DisasContext *s, TCGMemOp ot)
 {
     if (s->tb->cflags & CF_USE_ICOUNT) {
@@ -1170,6 +1183,7 @@  static inline void gen_ins(DisasContext *s, TCGMemOp ot)
     gen_op_st_v(s, ot, cpu_T[0], cpu_A0);
     gen_op_movl_T0_Dshift(ot);
     gen_op_add_reg_T0(s->aflag, R_EDI);
+    gen_bpt_io(s, cpu_tmp2_i32, ot);
     if (s->tb->cflags & CF_USE_ICOUNT) {
         gen_io_end();
     }
@@ -1187,9 +1201,9 @@  static inline void gen_outs(DisasContext *s, TCGMemOp ot)
     tcg_gen_andi_i32(cpu_tmp2_i32, cpu_tmp2_i32, 0xffff);
     tcg_gen_trunc_tl_i32(cpu_tmp3_i32, cpu_T[0]);
     gen_helper_out_func(ot, cpu_tmp2_i32, cpu_tmp3_i32);
-
     gen_op_movl_T0_Dshift(ot);
     gen_op_add_reg_T0(s->aflag, R_ESI);
+    gen_bpt_io(s, cpu_tmp2_i32, ot);
     if (s->tb->cflags & CF_USE_ICOUNT) {
         gen_io_end();
     }
@@ -6269,6 +6283,7 @@  static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
         tcg_gen_movi_i32(cpu_tmp2_i32, val);
         gen_helper_in_func(ot, cpu_T[1], cpu_tmp2_i32);
         gen_op_mov_reg_v(ot, R_EAX, cpu_T[1]);
+        gen_bpt_io(s, cpu_tmp2_i32, ot);
         if (s->tb->cflags & CF_USE_ICOUNT) {
             gen_io_end();
             gen_jmp(s, s->pc - s->cs_base);
@@ -6289,6 +6304,7 @@  static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
         tcg_gen_movi_i32(cpu_tmp2_i32, val);
         tcg_gen_trunc_tl_i32(cpu_tmp3_i32, cpu_T[1]);
         gen_helper_out_func(ot, cpu_tmp2_i32, cpu_tmp3_i32);
+        gen_bpt_io(s, cpu_tmp2_i32, ot);
         if (s->tb->cflags & CF_USE_ICOUNT) {
             gen_io_end();
             gen_jmp(s, s->pc - s->cs_base);
@@ -6306,6 +6322,7 @@  static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
         tcg_gen_trunc_tl_i32(cpu_tmp2_i32, cpu_T[0]);
         gen_helper_in_func(ot, cpu_T[1], cpu_tmp2_i32);
         gen_op_mov_reg_v(ot, R_EAX, cpu_T[1]);
+        gen_bpt_io(s, cpu_tmp2_i32, ot);
         if (s->tb->cflags & CF_USE_ICOUNT) {
             gen_io_end();
             gen_jmp(s, s->pc - s->cs_base);
@@ -6325,6 +6342,7 @@  static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
         tcg_gen_trunc_tl_i32(cpu_tmp2_i32, cpu_T[0]);
         tcg_gen_trunc_tl_i32(cpu_tmp3_i32, cpu_T[1]);
         gen_helper_out_func(ot, cpu_tmp2_i32, cpu_tmp3_i32);
+        gen_bpt_io(s, cpu_tmp2_i32, ot);
         if (s->tb->cflags & CF_USE_ICOUNT) {
             gen_io_end();
             gen_jmp(s, s->pc - s->cs_base);