[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[xen staging-4.18] x86/pv: Fix merging of new status bits into %dr6



commit cecee35dd426bb49daf0b58dcf6966024fdc0f0c
Author:     Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
AuthorDate: Tue Sep 24 14:53:59 2024 +0200
Commit:     Jan Beulich <jbeulich@xxxxxxxx>
CommitDate: Tue Sep 24 14:53:59 2024 +0200

    x86/pv: Fix merging of new status bits into %dr6
    
    All #DB exceptions result in an update of %dr6, but this isn't captured in
    Xen's handling, and is buggy just about everywhere.
    
    To begin resolving this issue, add a new pending_dbg field to x86_event
    (unioned with cr2 to avoid taking any extra space, adjusting users to avoid
    old-GCC bugs with anonymous unions), and introduce pv_inject_DB() to replace
    the current callers using pv_inject_hw_exception().
    
    Push the adjustment of v->arch.dr6 into pv_inject_event(), and use the new
    x86_merge_dr6() rather than the current incorrect logic.
    
    A key property is that pending_dbg is taken with positive polarity to deal
    with RTM/BLD sensibly.  Most callers pass in a constant, but callers passing
    in a hardware %dr6 value need to XOR the value with X86_DR6_DEFAULT to flip 
to
    positive polarity.
    
    This fixes the behaviour of the breakpoint status bits; that any left 
pending
    are generally discarded when a new #DB is raised.  In principle it would fix
    RTM/BLD too, except PV guests can't turn these capabilities on to start 
with.
    
    Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
    Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
    Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
    master commit: db39fa4b27ea470902d4625567cb6fa24030ddfa
    master date: 2024-08-21 23:59:19 +0100
---
 xen/arch/x86/include/asm/domain.h      | 18 ++++++++++++++++--
 xen/arch/x86/include/asm/hvm/hvm.h     |  3 ++-
 xen/arch/x86/pv/emul-priv-op.c         |  5 +----
 xen/arch/x86/pv/emulate.c              |  9 +++++++--
 xen/arch/x86/pv/ro-page-fault.c        |  2 +-
 xen/arch/x86/pv/traps.c                | 16 ++++++++++++----
 xen/arch/x86/traps.c                   |  2 +-
 xen/arch/x86/x86_emulate/x86_emulate.h |  5 ++++-
 8 files changed, 44 insertions(+), 16 deletions(-)

diff --git a/xen/arch/x86/include/asm/domain.h 
b/xen/arch/x86/include/asm/domain.h
index 237510fed3..53876472fe 100644
--- a/xen/arch/x86/include/asm/domain.h
+++ b/xen/arch/x86/include/asm/domain.h
@@ -732,15 +732,29 @@ static inline void pv_inject_hw_exception(unsigned int 
vector, int errcode)
     pv_inject_event(&event);
 }
 
+static inline void pv_inject_DB(unsigned long pending_dbg)
+{
+    struct x86_event event = {
+        .vector      = X86_EXC_DB,
+        .type        = X86_EVENTTYPE_HW_EXCEPTION,
+        .error_code  = X86_EVENT_NO_EC,
+    };
+
+    event.pending_dbg = pending_dbg;
+
+    pv_inject_event(&event);
+}
+
 static inline void pv_inject_page_fault(int errcode, unsigned long cr2)
 {
-    const struct x86_event event = {
+    struct x86_event event = {
         .vector = X86_EXC_PF,
         .type = X86_EVENTTYPE_HW_EXCEPTION,
         .error_code = errcode,
-        .cr2 = cr2,
     };
 
+    event.cr2 = cr2;
+
     pv_inject_event(&event);
 }
 
diff --git a/xen/arch/x86/include/asm/hvm/hvm.h 
b/xen/arch/x86/include/asm/hvm/hvm.h
index fd390cefe1..f15d38e8bf 100644
--- a/xen/arch/x86/include/asm/hvm/hvm.h
+++ b/xen/arch/x86/include/asm/hvm/hvm.h
@@ -532,9 +532,10 @@ static inline void hvm_inject_page_fault(int errcode, 
unsigned long cr2)
         .vector = X86_EXC_PF,
         .type = X86_EVENTTYPE_HW_EXCEPTION,
         .error_code = errcode,
-        .cr2 = cr2,
     };
 
+    event.cr2 = cr2;
+
     hvm_inject_event(&event);
 }
 
diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
index aa11ecadaa..15c83b9d23 100644
--- a/xen/arch/x86/pv/emul-priv-op.c
+++ b/xen/arch/x86/pv/emul-priv-op.c
@@ -1366,10 +1366,7 @@ int pv_emulate_privileged_op(struct cpu_user_regs *regs)
             ctxt.bpmatch |= DR_STEP;
 
         if ( ctxt.bpmatch )
-        {
-            curr->arch.dr6 |= ctxt.bpmatch | DR_STATUS_RESERVED_ONE;
-            pv_inject_hw_exception(X86_EXC_DB, X86_EVENT_NO_EC);
-        }
+            pv_inject_DB(ctxt.bpmatch);
 
         /* fall through */
     case X86EMUL_RETRY:
diff --git a/xen/arch/x86/pv/emulate.c b/xen/arch/x86/pv/emulate.c
index e7a1c0a2cc..8c44dea123 100644
--- a/xen/arch/x86/pv/emulate.c
+++ b/xen/arch/x86/pv/emulate.c
@@ -71,10 +71,15 @@ void pv_emul_instruction_done(struct cpu_user_regs *regs, 
unsigned long rip)
 {
     regs->rip = rip;
     regs->eflags &= ~X86_EFLAGS_RF;
+
     if ( regs->eflags & X86_EFLAGS_TF )
     {
-        current->arch.dr6 |= DR_STEP | DR_STATUS_RESERVED_ONE;
-        pv_inject_hw_exception(X86_EXC_DB, X86_EVENT_NO_EC);
+        /*
+         * TODO: this should generally use TF from the start of the
+         * instruction.  It's only a latent bug for now, as this path isn't
+         * used for any instruction which modifies eflags.
+         */
+        pv_inject_DB(X86_DR6_BS);
     }
 }
 
diff --git a/xen/arch/x86/pv/ro-page-fault.c b/xen/arch/x86/pv/ro-page-fault.c
index cad28ef928..d0fe07e3a1 100644
--- a/xen/arch/x86/pv/ro-page-fault.c
+++ b/xen/arch/x86/pv/ro-page-fault.c
@@ -390,7 +390,7 @@ int pv_ro_page_fault(unsigned long addr, struct 
cpu_user_regs *regs)
         /* Fallthrough */
     case X86EMUL_OKAY:
         if ( ctxt.retire.singlestep )
-            pv_inject_hw_exception(X86_EXC_DB, X86_EVENT_NO_EC);
+            pv_inject_DB(X86_DR6_BS);
 
         /* Fallthrough */
     case X86EMUL_RETRY:
diff --git a/xen/arch/x86/pv/traps.c b/xen/arch/x86/pv/traps.c
index 240d1a2db7..3d499eb3b0 100644
--- a/xen/arch/x86/pv/traps.c
+++ b/xen/arch/x86/pv/traps.c
@@ -12,6 +12,7 @@
 #include <xen/lib.h>
 #include <xen/softirq.h>
 
+#include <asm/debugreg.h>
 #include <asm/pv/trace.h>
 #include <asm/shared.h>
 #include <asm/traps.h>
@@ -50,9 +51,9 @@ void pv_inject_event(const struct x86_event *event)
     tb->cs    = ti->cs;
     tb->eip   = ti->address;
 
-    if ( event->type == X86_EVENTTYPE_HW_EXCEPTION &&
-         vector == X86_EXC_PF )
+    switch ( vector | -(event->type == X86_EVENTTYPE_SW_INTERRUPT) )
     {
+    case X86_EXC_PF:
         curr->arch.pv.ctrlreg[2] = event->cr2;
         arch_set_cr2(curr, event->cr2);
 
@@ -62,9 +63,16 @@ void pv_inject_event(const struct x86_event *event)
             error_code |= PFEC_user_mode;
 
         trace_pv_page_fault(event->cr2, error_code);
-    }
-    else
+        break;
+
+    case X86_EXC_DB:
+        curr->arch.dr6 = x86_merge_dr6(curr->domain->arch.cpu_policy,
+                                       curr->arch.dr6, event->pending_dbg);
+        fallthrough;
+    default:
         trace_pv_trap(vector, regs->rip, use_error_code, error_code);
+        break;
+    }
 
     if ( use_error_code )
     {
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 9d72ebce55..abd3019976 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -2064,7 +2064,7 @@ void do_debug(struct cpu_user_regs *regs)
         return;
     }
 
-    pv_inject_hw_exception(X86_EXC_DB, X86_EVENT_NO_EC);
+    pv_inject_DB(0 /* N/A, already merged */);
 }
 
 /* SAF-1-safe */
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h 
b/xen/arch/x86/x86_emulate/x86_emulate.h
index 698750267a..e348e3c1d3 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.h
+++ b/xen/arch/x86/x86_emulate/x86_emulate.h
@@ -78,7 +78,10 @@ struct x86_event {
     uint8_t       type;         /* X86_EVENTTYPE_* */
     uint8_t       insn_len;     /* Instruction length */
     int32_t       error_code;   /* X86_EVENT_NO_EC if n/a */
-    unsigned long cr2;          /* Only for X86_EXC_PF h/w exception */
+    union {
+        unsigned long cr2;         /* #PF */
+        unsigned long pending_dbg; /* #DB (new DR6 bits, positive polarity) */
+    };
 };
 
 /*
--
generated by git-patchbot for /home/xen/git/xen.git#staging-4.18



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.