x86/vmx: Partially revert "x86/vmx: implement Notify VM Exit"

Commit Message

Andrew Cooper Jan. 18, 2023, 7:36 p.m. UTC

The original patch tried to do two things - implement VMNotify, and
re-optimise VT-x to not intercept #DB/#AC by default.

The second part is buggy in multiple ways.  Both GDBSX and Introspection need
to conditionally intercept #DB, which was not accounted for.  Also, #DB
interception has nothing at all to do with cpu_has_monitor_trap_flag.

Revert the second half, leaving #DB/#AC intercepted unilaterally, but with
VMNotify active by default when available.

Fixes: 573279cde1c4 ("x86/vmx: implement Notify VM Exit")
Signed-off-by: Andrew Cooper <[e-mail]>
---
CC: Jan Beulich <[e-mail]>
CC: Roger Pau Monné <[e-mail]>
CC: Wei Liu <[e-mail]>
CC: Kevin Tian <[e-mail]>

 #DB/#AC are not fastpaths in the slightest.  This perf optimisation can be
reworked at some point later with rather more care and testing.

It's *really* not as urgent as getting VMNotify active by default.
---
 xen/arch/x86/hvm/vmx/vmcs.c | 11 ++---------
 xen/arch/x86/hvm/vmx/vmx.c  | 16 ++--------------
 2 files changed, 4 insertions(+), 23 deletions(-)

Comments

Tian, Kevin Jan. 19, 2023, 5:52 a.m. UTC | #1

> From: Andrew Cooper <[e-mail]>
> Sent: Thursday, January 19, 2023 3:37 AM
> 
> The original patch tried to do two things - implement VMNotify, and
> re-optimise VT-x to not intercept #DB/#AC by default.
> 
> The second part is buggy in multiple ways.  Both GDBSX and Introspection
> need
> to conditionally intercept #DB, which was not accounted for.  Also, #DB
> interception has nothing at all to do with cpu_has_monitor_trap_flag.
> 
> Revert the second half, leaving #DB/#AC intercepted unilaterally, but with
> VMNotify active by default when available.
> 
> Fixes: 573279cde1c4 ("x86/vmx: implement Notify VM Exit")
> Signed-off-by: Andrew Cooper <[e-mail]>

Reviewed-by: Kevin Tian <[e-mail]>

Patch

diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 8992f4e0aeb2..7d8bfeb53982 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -1296,17 +1296,10 @@  static int construct_vmcs(struct vcpu *v)
     v->arch.hvm.vmx.exception_bitmap = HVM_TRAP_MASK
               | (paging_mode_hap(d) ? 0 : (1U << TRAP_page_fault))
               | (v->arch.fully_eager_fpu ? 0 : (1U << TRAP_no_device));
+
     if ( cpu_has_vmx_notify_vm_exiting )
-    {
         __vmwrite(NOTIFY_WINDOW, vm_notify_window);
-        /*
-         * Disable #AC and #DB interception: by using VM Notify Xen is
-         * guaranteed to get a VM exit even if the guest manages to lock the
-         * CPU.
-         */
-        v->arch.hvm.vmx.exception_bitmap &= ~((1U << TRAP_debug) |
-                                              (1U << TRAP_alignment_check));
-    }
+
     vmx_update_exception_bitmap(v);
 
     v->arch.hvm.guest_cr[0] = X86_CR0_PE | X86_CR0_ET;
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 15a07933ee5d..2e2ab0ac0e26 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1564,19 +1564,10 @@  static void cf_check vmx_update_host_cr3(struct vcpu *v)
 
 void vmx_update_debug_state(struct vcpu *v)
 {
-    unsigned int mask = 1u << TRAP_int3;
-
-    if ( !cpu_has_monitor_trap_flag && cpu_has_vmx_notify_vm_exiting )
-        /*
-         * Only allow toggling TRAP_debug if notify VM exit is enabled, as
-         * unconditionally setting TRAP_debug is part of the XSA-156 fix.
-         */
-        mask |= 1u << TRAP_debug;
-
     if ( v->arch.hvm.debug_state_latch )
-        v->arch.hvm.vmx.exception_bitmap |= mask;
+        v->arch.hvm.vmx.exception_bitmap |= 1U << TRAP_int3;
     else
-        v->arch.hvm.vmx.exception_bitmap &= ~mask;
+        v->arch.hvm.vmx.exception_bitmap &= ~(1U << TRAP_int3);
 
     vmx_vmcs_enter(v);
     vmx_update_exception_bitmap(v);
@@ -4192,9 +4183,6 @@  void vmx_vmexit_handler(struct cpu_user_regs *regs)
         switch ( vector )
         {
         case TRAP_debug:
-            if ( cpu_has_monitor_trap_flag && cpu_has_vmx_notify_vm_exiting )
-                goto exit_and_crash;
-
             /*
              * Updates DR6 where debugger can peek (See 3B 23.2.1,
              * Table 23-1, "Exit Qualification for Debug Exceptions").