[Xen-devel] [PATCH v4 11/24] x86/emul: Implement singlestep as a retire flag

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

The behaviour of singlestep is to raise #DB after the instruction has been
completed, but implementing it with inject_hw_exception() causes x86_emulate()
to return X86EMUL_EXCEPTION, despite succesfully completing execution of the
instruction, including register writeback.

Instead, use a retire flag to indicate singlestep, which causes x86_emulate()
to return X86EMUL_OKAY.

Update all callers of x86_emulate() to use the new retire flag.  This fixes
the behaviour of singlestep for shadow pagetable updates and mmcfg/mmio_ro
intercepts, which previously discarded the exception.

With this change, all uses of X86EMUL_EXCEPTION from x86_emulate() are
believed to have strictly fault semantics.

Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
Reviewed-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
Acked-by: Tim Deegan <tim@xxxxxxx>
---
CC: Jan Beulich <JBeulich@xxxxxxxx>

v4:
 * s/is_hvm_vcpu/has_hvm_container_domain/
 * Adjust comments and entry condition into the PAE second-half case.
v3:
 * New
---
 xen/arch/x86/hvm/emulate.c             |  3 +++
 xen/arch/x86/mm.c                      | 11 ++++++++++-
 xen/arch/x86/mm/shadow/multi.c         | 35 ++++++++++++++++++++++++++++++----
 xen/arch/x86/x86_emulate/x86_emulate.c |  9 ++++-----
 xen/arch/x86/x86_emulate/x86_emulate.h |  6 ++++++
 5 files changed, 54 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index fe62500..91c79fa 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -1788,6 +1788,9 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt 
*hvmemul_ctxt,
     if ( rc != X86EMUL_OKAY )
         return rc;
 
+    if ( hvmemul_ctxt->ctxt.retire.singlestep )
+        hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
+
     new_intr_shadow = hvmemul_ctxt->intr_shadow;
 
     /* MOV-SS instruction toggles MOV-SS shadow, else we just clear it. */
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index b7c7122..231c7bf 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -5382,6 +5382,9 @@ int ptwr_do_page_fault(struct vcpu *v, unsigned long addr,
     if ( rc == X86EMUL_UNHANDLEABLE )
         goto bail;
 
+    if ( ptwr_ctxt.ctxt.retire.singlestep )
+        pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
+
     perfc_incr(ptwr_emulations);
     return EXCRET_fault_fixed;
 
@@ -5503,7 +5506,13 @@ int mmio_ro_do_page_fault(struct vcpu *v, unsigned long 
addr,
     else
         rc = x86_emulate(&ctxt, &mmio_ro_emulate_ops);
 
-    return rc != X86EMUL_UNHANDLEABLE ? EXCRET_fault_fixed : 0;
+    if ( rc == X86EMUL_UNHANDLEABLE )
+        return 0;
+
+    if ( ctxt.retire.singlestep )
+        pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
+
+    return EXCRET_fault_fixed;
 }
 
 void *alloc_xen_pagetable(void)
diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c
index 9ee48a8..eac2330 100644
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -3422,18 +3422,36 @@ static int sh_page_fault(struct vcpu *v,
         v->arch.paging.last_write_emul_ok = 0;
 #endif
 
+    if ( emul_ctxt.ctxt.retire.singlestep )
+    {
+        if ( has_hvm_container_domain(d) )
+            hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
+        else
+            pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
+    }
+
 #if GUEST_PAGING_LEVELS == 3 /* PAE guest */
-    if ( r == X86EMUL_OKAY ) {
+    /*
+     * If there are no pending actions, emulate up to four extra instructions
+     * in the hope of catching the "second half" of a 64-bit pagetable write.
+     */
+    if ( r == X86EMUL_OKAY && !emul_ctxt.ctxt.retire.raw )
+    {
         int i, emulation_count=0;
         this_cpu(trace_emulate_initial_va) = va;
-        /* Emulate up to four extra instructions in the hope of catching
-         * the "second half" of a 64-bit pagetable write. */
+
         for ( i = 0 ; i < 4 ; i++ )
         {
             shadow_continue_emulation(&emul_ctxt, regs);
             v->arch.paging.last_write_was_pt = 0;
             r = x86_emulate(&emul_ctxt.ctxt, emul_ops);
-            if ( r == X86EMUL_OKAY )
+
+            /*
+             * Only continue the search for the second half if there are no
+             * exceptions or pending actions.  Otherwise, give up and re-enter
+             * the guest.
+             */
+            if ( r == X86EMUL_OKAY && !emul_ctxt.ctxt.retire.raw )
             {
                 emulation_count++;
                 if ( v->arch.paging.last_write_was_pt )
@@ -3449,6 +3467,15 @@ static int sh_page_fault(struct vcpu *v,
             {
                 perfc_incr(shadow_em_ex_fail);
                 TRACE_SHADOW_PATH_FLAG(TRCE_SFLAG_EMULATION_LAST_FAILED);
+
+                if ( emul_ctxt.ctxt.retire.singlestep )
+                {
+                    if ( has_hvm_container_domain(d) )
+                        hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
+                    else
+                        pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
+                }
+
                 break; /* Don't emulate again if we failed! */
             }
         }
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c 
b/xen/arch/x86/x86_emulate/x86_emulate.c
index fa8d98f..7bc1cd9 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -2415,7 +2415,6 @@ x86_emulate(
     struct x86_emulate_state state;
     int rc;
     uint8_t b, d;
-    bool tf = ctxt->regs->eflags & EFLG_TF;
     struct operand src = { .reg = PTR_POISON };
     struct operand dst = { .reg = PTR_POISON };
     enum x86_swint_type swint_type;
@@ -5413,11 +5412,11 @@ x86_emulate(
     if ( !mode_64bit() )
         _regs.eip = (uint32_t)_regs.eip;
 
-    *ctxt->regs = _regs;
+    /* Was singestepping active at the start of this instruction? */
+    if ( (rc == X86EMUL_OKAY) && (ctxt->regs->eflags & EFLG_TF) )
+        ctxt->retire.singlestep = true;
 
-    /* Inject #DB if single-step tracing was enabled at instruction start. */
-    if ( tf && (rc == X86EMUL_OKAY) && ops->inject_hw_exception )
-        rc = ops->inject_hw_exception(EXC_DB, -1, ctxt) ? : X86EMUL_EXCEPTION;
+    *ctxt->regs = _regs;
 
  done:
     _put_fpu();
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h 
b/xen/arch/x86/x86_emulate/x86_emulate.h
index f4bcf36..5a4f9b7 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.h
+++ b/xen/arch/x86/x86_emulate/x86_emulate.h
@@ -483,6 +483,7 @@ struct x86_emulate_ctxt
             bool hlt:1;          /* Instruction HLTed. */
             bool mov_ss:1;       /* Instruction sets MOV-SS irq shadow. */
             bool sti:1;          /* Instruction sets STI irq shadow. */
+            bool singlestep:1;   /* Singlestepping was active. */
         };
     } retire;
 };
@@ -572,12 +573,17 @@ static inline int x86_emulate_wrapper(
     struct x86_emulate_ctxt *ctxt,
     const struct x86_emulate_ops *ops)
 {
+    unsigned long orig_eip = ctxt->regs->eip;
     int rc = x86_emulate(ctxt, ops);
 
     /* Retire flags should only be set for successful instruction emulation. */
     if ( rc != X86EMUL_OKAY )
         ASSERT(ctxt->retire.raw == 0);
 
+    /* All cases returning X86EMUL_EXCEPTION should have fault semantics. */
+    if ( rc == X86EMUL_EXCEPTION )
+        ASSERT(ctxt->regs->eip == orig_eip);
+
     return rc;
 }
 
-- 
2.1.4


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