[Xen-devel] [PATCH 1/3] x86emul: correct EFLAGS.TF handling

["Jan Beulich" <JBeulich@xxxxxxxx>]

For repeated string instructions we should not emulate multiple
iterations in one go when a single step trap needs injecting (which
needs to happen after every iteration).

For all non-branch instructions as well as not taken conditional
branches we additionally need to take DebugCtl.BTF into consideration.

And for mov-to/pop-into %ss there should be no #DB at all (EFLAGS.TF
remaining set means there'll be #DB after the next instruction).

Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -415,6 +415,8 @@ typedef union {
 #define MSR_SYSENTER_CS  0x00000174
 #define MSR_SYSENTER_ESP 0x00000175
 #define MSR_SYSENTER_EIP 0x00000176
+#define MSR_DEBUGCTL     0x000001d9
+#define DEBUGCTL_BTF     (1 << 1)
 #define MSR_EFER         0xc0000080
 #define MSR_STAR         0xc0000081
 #define MSR_LSTAR        0xc0000082
@@ -751,6 +753,8 @@ do {
     rc = ops->insn_fetch(x86_seg_cs, ip, NULL, 0, ctxt);                \
     if ( rc ) goto done;                                                \
     _regs.eip = ip;                                                     \
+    if ( _regs.eflags & EFLG_TF )                                       \
+        ctxt->retire.singlestep = true;                                 \
 } while (0)
 
 #define validate_far_branch(cs, ip) ({                                  \
@@ -767,6 +771,8 @@ do {
 #define commit_far_branch(cs, ip) ({                                    \
     validate_far_branch(cs, ip);                                        \
     _regs.eip = (ip);                                                   \
+    if ( _regs.eflags & EFLG_TF )                                       \
+        ctxt->retire.singlestep = true;                                 \
     ops->write_segment(x86_seg_cs, cs, ctxt);                           \
 })
 
@@ -948,6 +954,9 @@ static inline void put_loop_count(
         }                                                               \
         goto no_writeback;                                              \
     }                                                                   \
+    if ( max_reps > 1 && (_regs.eflags & EFLG_TF) &&                    \
+         !is_branch_step(ctxt, ops) )                                   \
+        max_reps = 1;                                                   \
     max_reps;                                                           \
 })
 
@@ -1637,6 +1646,16 @@ static bool is_aligned(enum x86_segment
     return !((reg.base + offs) & (size - 1));
 }
 
+static bool is_branch_step(struct x86_emulate_ctxt *ctxt,
+                           const struct x86_emulate_ops *ops)
+{
+    uint64_t debugctl;
+
+    return ops->read_msr &&
+           ops->read_msr(MSR_DEBUGCTL, &debugctl, ctxt) == X86EMUL_OKAY &&
+           (debugctl & DEBUGCTL_BTF);
+}
+
 static bool umip_active(struct x86_emulate_ctxt *ctxt,
                         const struct x86_emulate_ops *ops)
 {
@@ -3132,6 +3151,8 @@ x86_emulate(
             goto done;
 
         _regs.eip = imm1;
+        if ( _regs.eflags & EFLG_TF )
+            ctxt->retire.singlestep = true;
         break;
 
     case 0x9b:  /* wait/fwait */
@@ -4608,6 +4629,8 @@ x86_emulate(
              (rc = ops->write_segment(x86_seg_ss, &sreg, ctxt)) )
             goto done;
 
+        if ( ctxt->regs->eflags & EFLG_TF )
+            ctxt->retire.singlestep = true;
         break;
     }
 
@@ -4875,6 +4898,8 @@ x86_emulate(
             goto done;
         _regs.esp = lm ? msr_content : (uint32_t)msr_content;
 
+        if ( _regs.eflags & EFLG_TF )
+            ctxt->retire.singlestep = true;
         break;
     }
 
@@ -4914,6 +4939,9 @@ x86_emulate(
 
         _regs.eip = user64 ? _regs.edx : (uint32_t)_regs.edx;
         _regs.esp = user64 ? _regs.ecx : (uint32_t)_regs.ecx;
+
+        if ( _regs.eflags & EFLG_TF )
+            ctxt->retire.singlestep = true;
         break;
     }
 
@@ -5400,7 +5428,9 @@ x86_emulate(
         break;
 #endif
     default:
-        goto cannot_emulate;
+    cannot_emulate:
+        rc = X86EMUL_UNHANDLEABLE;
+        goto done;
     }
 
     switch ( dst.type )
@@ -5445,7 +5475,8 @@ x86_emulate(
         _regs.eip = (uint32_t)_regs.eip;
 
     /* Was singestepping active at the start of this instruction? */
-    if ( (rc == X86EMUL_OKAY) && (ctxt->regs->eflags & EFLG_TF) )
+    if ( (rc == X86EMUL_OKAY) && (ctxt->regs->eflags & EFLG_TF) &&
+         !is_branch_step(ctxt, ops) && !ctxt->retire.mov_ss )
         ctxt->retire.singlestep = true;
 
     *ctxt->regs = _regs;
@@ -5461,12 +5492,17 @@ x86_emulate(
  done:
     _put_fpu();
     put_stub(stub);
-    return rc;
 
- cannot_emulate:
-    _put_fpu();
-    put_stub(stub);
-    return X86EMUL_UNHANDLEABLE;
+    /*
+     * We may have set the single step flag ahead of the last possible point
+     * of failure (unavoidably with the current near CALL code flow, but also
+     * used on some far branch paths to keep the code simple), so to satisfy
+     * x86_emulate_wrapper()'s ASSERT() we may need to clear it here again.
+     */
+    if ( rc != X86EMUL_OKAY )
+        ctxt->retire.singlestep = false;
+
+    return rc;
 #undef state
 }

Attachment: x86emul-sstep-bstep.patch
Description: Text document

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel