[PATCH v3 0/3] x86/vmx: implement Bus Lock and VM Notify

[PATCH v3 0/3] x86/vmx: implement Bus Lock and VM Notify

[Roger Pau Monne]

Hello,

Following series implements support for bus lock and notify VM exit.

Patches are not really dependent, but I've developed them together by
virtue of both features being in Intel Instructions Set Extensions PR
Chapter 9.

Thanks, Roger.

Roger Pau Monne (3):
  x86/vmx: implement VMExit based guest Bus Lock detection
  x86/vmx: introduce helper to set VMX_INTR_SHADOW_NMI
  x86/vmx: implement Notify VM Exit

 docs/misc/xen-command-line.pandoc       | 11 ++++
 xen/arch/x86/hvm/vmx/vmcs.c             | 21 ++++++-
 xen/arch/x86/hvm/vmx/vmx.c              | 74 ++++++++++++++++++++++---
 xen/arch/x86/include/asm/hvm/vmx/vmcs.h |  7 +++
 xen/arch/x86/include/asm/hvm/vmx/vmx.h  | 11 ++++
 xen/arch/x86/include/asm/perfc_defn.h   |  5 +-
 6 files changed, 118 insertions(+), 11 deletions(-)

-- 
2.37.0

[PATCH v3 1/3] x86/vmx: implement VMExit based guest Bus Lock detection

[Roger Pau Monne]

Add support for enabling guest Bus Lock Detection on Intel systems.
Such detection works by triggering a vmexit, which ought to be enough
of a pause to prevent a guest from abusing of the Bus Lock.

Add an extra Xen perf counter to track the number of Bus Locks detected.
This is done because Bus Locks can also be reported by setting the bit
26 in the exit reason field, so also account for those.

Note EXIT_REASON_BUS_LOCK VMExits will always have bit 26 set in
exit_reason, and hence the performance counter doesn't need to be
increased for EXIT_REASON_BUS_LOCK handling.

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
Changes since v2:
 - Subject/commit log adjustments.
 - Simply logic given bit 26 will always be set.

Changes since v1:
 - Adjust commit message.
---
 xen/arch/x86/hvm/vmx/vmcs.c             |  4 +++-
 xen/arch/x86/hvm/vmx/vmx.c              | 14 ++++++++++++++
 xen/arch/x86/include/asm/hvm/vmx/vmcs.h |  3 +++
 xen/arch/x86/include/asm/hvm/vmx/vmx.h  |  2 ++
 xen/arch/x86/include/asm/perfc_defn.h   |  4 +++-
 5 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 56fed2db03..d388e6729c 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -209,6 +209,7 @@ static void __init vmx_display_features(void)
     P(cpu_has_vmx_virt_exceptions, "Virtualisation Exceptions");
     P(cpu_has_vmx_pml, "Page Modification Logging");
     P(cpu_has_vmx_tsc_scaling, "TSC Scaling");
+    P(cpu_has_vmx_bus_lock_detection, "Bus Lock Detection");
 #undef P
 
     if ( !printed )
@@ -318,7 +319,8 @@ static int vmx_init_vmcs_config(bool bsp)
                SECONDARY_EXEC_ENABLE_VM_FUNCTIONS |
                SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS |
                SECONDARY_EXEC_XSAVES |
-               SECONDARY_EXEC_TSC_SCALING);
+               SECONDARY_EXEC_TSC_SCALING |
+               SECONDARY_EXEC_BUS_LOCK_DETECTION);
         if ( _vmx_misc_cap & VMX_MISC_VMWRITE_ALL )
             opt |= SECONDARY_EXEC_ENABLE_VMCS_SHADOWING;
         if ( opt_vpid_enabled )
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index f08a00dcbb..853f3a9111 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -4065,6 +4065,11 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
 
     if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) )
         return vmx_failed_vmentry(exit_reason, regs);
+    if ( unlikely(exit_reason & VMX_EXIT_REASONS_BUS_LOCK) )
+    {
+        perfc_incr(buslock);
+        exit_reason &= ~VMX_EXIT_REASONS_BUS_LOCK;
+    }
 
     if ( v->arch.hvm.vmx.vmx_realmode )
     {
@@ -4561,6 +4566,15 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
         vmx_handle_descriptor_access(exit_reason);
         break;
 
+    case EXIT_REASON_BUS_LOCK:
+        /*
+         * Nothing to do: just taking a vmexit should be enough of a pause to
+         * prevent a VM from crippling the host with bus locks.  Note
+         * EXIT_REASON_BUS_LOCK will always have bit 26 set in exit_reason, and
+         * hence the perf counter is already increased.
+         */
+        break;
+
     case EXIT_REASON_VMX_PREEMPTION_TIMER_EXPIRED:
     case EXIT_REASON_INVPCID:
     /* fall through */
diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
index 9119aa8536..5d3edc1642 100644
--- a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
+++ b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
@@ -266,6 +266,7 @@ extern u32 vmx_vmentry_control;
 #define SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS   0x00040000
 #define SECONDARY_EXEC_XSAVES                   0x00100000
 #define SECONDARY_EXEC_TSC_SCALING              0x02000000
+#define SECONDARY_EXEC_BUS_LOCK_DETECTION       0x40000000
 extern u32 vmx_secondary_exec_control;
 
 #define VMX_EPT_EXEC_ONLY_SUPPORTED                         0x00000001
@@ -345,6 +346,8 @@ extern u64 vmx_ept_vpid_cap;
     (vmx_secondary_exec_control & SECONDARY_EXEC_XSAVES)
 #define cpu_has_vmx_tsc_scaling \
     (vmx_secondary_exec_control & SECONDARY_EXEC_TSC_SCALING)
+#define cpu_has_vmx_bus_lock_detection \
+    (vmx_secondary_exec_control & SECONDARY_EXEC_BUS_LOCK_DETECTION)
 
 #define VMCS_RID_TYPE_MASK              0x80000000
 
diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmx.h b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
index 8eedf59155..03995701a1 100644
--- a/xen/arch/x86/include/asm/hvm/vmx/vmx.h
+++ b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
@@ -159,6 +159,7 @@ static inline void pi_clear_sn(struct pi_desc *pi_desc)
  * Exit Reasons
  */
 #define VMX_EXIT_REASONS_FAILED_VMENTRY 0x80000000
+#define VMX_EXIT_REASONS_BUS_LOCK       (1u << 26)
 
 #define EXIT_REASON_EXCEPTION_NMI       0
 #define EXIT_REASON_EXTERNAL_INTERRUPT  1
@@ -219,6 +220,7 @@ static inline void pi_clear_sn(struct pi_desc *pi_desc)
 #define EXIT_REASON_PML_FULL            62
 #define EXIT_REASON_XSAVES              63
 #define EXIT_REASON_XRSTORS             64
+#define EXIT_REASON_BUS_LOCK            74
 /* Remember to also update VMX_PERF_EXIT_REASON_SIZE! */
 
 /*
diff --git a/xen/arch/x86/include/asm/perfc_defn.h b/xen/arch/x86/include/asm/perfc_defn.h
index b07063b7d8..d6eb661940 100644
--- a/xen/arch/x86/include/asm/perfc_defn.h
+++ b/xen/arch/x86/include/asm/perfc_defn.h
@@ -6,7 +6,7 @@ PERFCOUNTER_ARRAY(exceptions,           "exceptions", 32)
 
 #ifdef CONFIG_HVM
 
-#define VMX_PERF_EXIT_REASON_SIZE 65
+#define VMX_PERF_EXIT_REASON_SIZE 75
 #define VMEXIT_NPF_PERFC 143
 #define SVM_PERF_EXIT_REASON_SIZE (VMEXIT_NPF_PERFC + 1)
 PERFCOUNTER_ARRAY(vmexits,              "vmexits",
@@ -125,4 +125,6 @@ PERFCOUNTER(realmode_exits,      "vmexits from realmode")
 
 PERFCOUNTER(pauseloop_exits, "vmexits from Pause-Loop Detection")
 
+PERFCOUNTER(buslock, "Bus Locks Detected")
+
 /*#endif*/ /* __XEN_PERFC_DEFN_H__ */
-- 
2.37.0

[PATCH v3 2/3] x86/vmx: introduce helper to set VMX_INTR_SHADOW_NMI

[Roger Pau Monne]

Introduce a small helper to OR VMX_INTR_SHADOW_NMI in
GUEST_INTERRUPTIBILITY_INFO in order to help dealing with the NMI
unblocked by IRET case.  Replace the existing usage in handling
EXIT_REASON_EXCEPTION_NMI and also add such handling to EPT violations
and page-modification log-full events.

Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Changes since v1:
 - New in this version.
---
 xen/arch/x86/hvm/vmx/vmx.c             | 28 +++++++++++++++++++-------
 xen/arch/x86/include/asm/hvm/vmx/vmx.h |  3 +++
 2 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 853f3a9111..d69c02b00a 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3935,6 +3935,15 @@ static int vmx_handle_apic_write(void)
     return vlapic_apicv_write(current, exit_qualification & 0xfff);
 }
 
+static void undo_nmis_unblocked_by_iret(void)
+{
+    unsigned long guest_info;
+
+    __vmread(GUEST_INTERRUPTIBILITY_INFO, &guest_info);
+    __vmwrite(GUEST_INTERRUPTIBILITY_INFO,
+              guest_info | VMX_INTR_SHADOW_NMI);
+}
+
 void vmx_vmexit_handler(struct cpu_user_regs *regs)
 {
     unsigned long exit_qualification, exit_reason, idtv_info, intr_info = 0;
@@ -4134,13 +4143,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
         if ( unlikely(intr_info & INTR_INFO_NMI_UNBLOCKED_BY_IRET) &&
              !(idtv_info & INTR_INFO_VALID_MASK) &&
              (vector != TRAP_double_fault) )
-        {
-            unsigned long guest_info;
-
-            __vmread(GUEST_INTERRUPTIBILITY_INFO, &guest_info);
-            __vmwrite(GUEST_INTERRUPTIBILITY_INFO,
-                      guest_info | VMX_INTR_SHADOW_NMI);
-        }
+            undo_nmis_unblocked_by_iret();
 
         perfc_incra(cause_vector, vector);
 
@@ -4506,6 +4509,11 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
 
         __vmread(GUEST_PHYSICAL_ADDRESS, &gpa);
         __vmread(EXIT_QUALIFICATION, &exit_qualification);
+
+        if ( unlikely(exit_qualification & INTR_INFO_NMI_UNBLOCKED_BY_IRET) &&
+             !(idtv_info & INTR_INFO_VALID_MASK) )
+            undo_nmis_unblocked_by_iret();
+
         ept_handle_violation(exit_qualification, gpa);
         break;
     }
@@ -4550,6 +4558,12 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
         break;
 
     case EXIT_REASON_PML_FULL:
+        __vmread(EXIT_QUALIFICATION, &exit_qualification);
+
+        if ( unlikely(exit_qualification & INTR_INFO_NMI_UNBLOCKED_BY_IRET) &&
+             !(idtv_info & INTR_INFO_VALID_MASK) )
+            undo_nmis_unblocked_by_iret();
+
         vmx_vcpu_flush_pml_buffer(v);
         break;
 
diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmx.h b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
index 03995701a1..bc0caad6fb 100644
--- a/xen/arch/x86/include/asm/hvm/vmx/vmx.h
+++ b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
@@ -225,6 +225,9 @@ static inline void pi_clear_sn(struct pi_desc *pi_desc)
 
 /*
  * Interruption-information format
+ *
+ * Note INTR_INFO_NMI_UNBLOCKED_BY_IRET is also used with Exit Qualification
+ * field under some circumstances.
  */
 #define INTR_INFO_VECTOR_MASK           0xff            /* 7:0 */
 #define INTR_INFO_INTR_TYPE_MASK        0x700           /* 10:8 */
-- 
2.37.0

[PATCH v3 3/3] x86/vmx: implement Notify VM Exit

[Roger Pau Monne]

Under certain conditions guests can get the CPU stuck in an unbounded
loop without the possibility of an interrupt window to occur on
instruction boundary.  This was the case with the scenarios described
in XSA-156.

Make use of the Notify VM Exit mechanism, that will trigger a VM Exit
if no interrupt window occurs for a specified amount of time.  Note
that using the Notify VM Exit avoids having to trap #AC and #DB
exceptions, as Xen is guaranteed to get a VM Exit even if the guest
puts the CPU in a loop without an interrupt window, as such disable
the intercepts if the feature is available and enabled.

Setting the notify VM exit window to 0 is safe because there's a
threshold added by the hardware in order to have a sane window value.

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v2:
 - Move up the logic to set the exception bitmap in construct_vmcs().
 - Change perfcounter description.

Changes since v1:
 - Properly update debug state when using notify VM exit.
 - Reword commit message.
---
Intel is working on getting the ISE updated to note that 0 is always a
safe value to use because the hardware will add an internal threshold
to assert so:

https://lore.kernel.org/xen-devel/3c64db19-00fe-05bf-ac4d-6ef4201b6aa0@intel.com/
---
 docs/misc/xen-command-line.pandoc       | 11 +++++++++
 xen/arch/x86/hvm/vmx/vmcs.c             | 17 +++++++++++++
 xen/arch/x86/hvm/vmx/vmx.c              | 32 +++++++++++++++++++++++--
 xen/arch/x86/include/asm/hvm/vmx/vmcs.h |  4 ++++
 xen/arch/x86/include/asm/hvm/vmx/vmx.h  |  6 +++++
 xen/arch/x86/include/asm/perfc_defn.h   |  3 ++-
 6 files changed, 70 insertions(+), 3 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index da18172e50..272be0e79f 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -2566,6 +2566,17 @@ guest will notify Xen that it has failed to acquire a spinlock.
 <major>, <minor> and <build> must be integers. The values will be
 encoded in guest CPUID 0x40000002 if viridian enlightenments are enabled.
 
+### vm-notify-window (Intel)
+> `= <integer>`
+
+> Default: `0`
+
+Specify the value of the VM Notify window used to detect locked VMs. Set to -1
+to disable the feature.  Value is in units of crystal clock cycles.
+
+Note the hardware might add a threshold to the provided value in order to make
+it safe, and hence using 0 is fine.
+
 ### vpid (Intel)
 > `= <boolean>`
 
diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index d388e6729c..4985d25b85 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -67,6 +67,9 @@ integer_param("ple_gap", ple_gap);
 static unsigned int __read_mostly ple_window = 4096;
 integer_param("ple_window", ple_window);
 
+static unsigned int __ro_after_init vm_notify_window;
+integer_param("vm-notify-window", vm_notify_window);
+
 static bool __read_mostly opt_ept_pml = true;
 static s8 __read_mostly opt_ept_ad = -1;
 int8_t __read_mostly opt_ept_exec_sp = -1;
@@ -210,6 +213,7 @@ static void __init vmx_display_features(void)
     P(cpu_has_vmx_pml, "Page Modification Logging");
     P(cpu_has_vmx_tsc_scaling, "TSC Scaling");
     P(cpu_has_vmx_bus_lock_detection, "Bus Lock Detection");
+    P(cpu_has_vmx_notify_vm_exiting, "Notify VM Exit");
 #undef P
 
     if ( !printed )
@@ -329,6 +333,8 @@ static int vmx_init_vmcs_config(bool bsp)
             opt |= SECONDARY_EXEC_UNRESTRICTED_GUEST;
         if ( opt_ept_pml )
             opt |= SECONDARY_EXEC_ENABLE_PML;
+        if ( vm_notify_window != ~0u )
+            opt |= SECONDARY_EXEC_NOTIFY_VM_EXITING;
 
         /*
          * "APIC Register Virtualization" and "Virtual Interrupt Delivery"
@@ -1290,6 +1296,17 @@ 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 d69c02b00a..b372cde33e 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1419,10 +1419,19 @@ 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 |= 1U << TRAP_int3;
+        v->arch.hvm.vmx.exception_bitmap |= mask;
     else
-        v->arch.hvm.vmx.exception_bitmap &= ~(1U << TRAP_int3);
+        v->arch.hvm.vmx.exception_bitmap &= ~mask;
 
     vmx_vmcs_enter(v);
     vmx_update_exception_bitmap(v);
@@ -4150,6 +4159,9 @@ 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").
@@ -4589,6 +4601,22 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
          */
         break;
 
+    case EXIT_REASON_NOTIFY:
+        __vmread(EXIT_QUALIFICATION, &exit_qualification);
+
+        if ( exit_qualification & NOTIFY_VM_CONTEXT_INVALID )
+        {
+            perfc_incr(vmnotify_crash);
+            gprintk(XENLOG_ERR, "invalid VM context after notify vmexit\n");
+            domain_crash(v->domain);
+            break;
+        }
+
+        if ( unlikely(exit_qualification & INTR_INFO_NMI_UNBLOCKED_BY_IRET) )
+            undo_nmis_unblocked_by_iret();
+
+        break;
+
     case EXIT_REASON_VMX_PREEMPTION_TIMER_EXPIRED:
     case EXIT_REASON_INVPCID:
     /* fall through */
diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
index 5d3edc1642..0961eabf3f 100644
--- a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
+++ b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
@@ -267,6 +267,7 @@ extern u32 vmx_vmentry_control;
 #define SECONDARY_EXEC_XSAVES                   0x00100000
 #define SECONDARY_EXEC_TSC_SCALING              0x02000000
 #define SECONDARY_EXEC_BUS_LOCK_DETECTION       0x40000000
+#define SECONDARY_EXEC_NOTIFY_VM_EXITING        0x80000000
 extern u32 vmx_secondary_exec_control;
 
 #define VMX_EPT_EXEC_ONLY_SUPPORTED                         0x00000001
@@ -348,6 +349,8 @@ extern u64 vmx_ept_vpid_cap;
     (vmx_secondary_exec_control & SECONDARY_EXEC_TSC_SCALING)
 #define cpu_has_vmx_bus_lock_detection \
     (vmx_secondary_exec_control & SECONDARY_EXEC_BUS_LOCK_DETECTION)
+#define cpu_has_vmx_notify_vm_exiting \
+    (vmx_secondary_exec_control & SECONDARY_EXEC_NOTIFY_VM_EXITING)
 
 #define VMCS_RID_TYPE_MASK              0x80000000
 
@@ -455,6 +458,7 @@ enum vmcs_field {
     SECONDARY_VM_EXEC_CONTROL       = 0x0000401e,
     PLE_GAP                         = 0x00004020,
     PLE_WINDOW                      = 0x00004022,
+    NOTIFY_WINDOW                   = 0x00004024,
     VM_INSTRUCTION_ERROR            = 0x00004400,
     VM_EXIT_REASON                  = 0x00004402,
     VM_EXIT_INTR_INFO               = 0x00004404,
diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmx.h b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
index bc0caad6fb..e429de8541 100644
--- a/xen/arch/x86/include/asm/hvm/vmx/vmx.h
+++ b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
@@ -221,6 +221,7 @@ static inline void pi_clear_sn(struct pi_desc *pi_desc)
 #define EXIT_REASON_XSAVES              63
 #define EXIT_REASON_XRSTORS             64
 #define EXIT_REASON_BUS_LOCK            74
+#define EXIT_REASON_NOTIFY              75
 /* Remember to also update VMX_PERF_EXIT_REASON_SIZE! */
 
 /*
@@ -236,6 +237,11 @@ static inline void pi_clear_sn(struct pi_desc *pi_desc)
 #define INTR_INFO_VALID_MASK            0x80000000      /* 31 */
 #define INTR_INFO_RESVD_BITS_MASK       0x7ffff000
 
+/*
+ * Exit Qualifications for NOTIFY VM EXIT
+ */
+#define NOTIFY_VM_CONTEXT_INVALID       1u
+
 /*
  * Exit Qualifications for MOV for Control Register Access
  */
diff --git a/xen/arch/x86/include/asm/perfc_defn.h b/xen/arch/x86/include/asm/perfc_defn.h
index d6eb661940..a710fba8a8 100644
--- a/xen/arch/x86/include/asm/perfc_defn.h
+++ b/xen/arch/x86/include/asm/perfc_defn.h
@@ -6,7 +6,7 @@ PERFCOUNTER_ARRAY(exceptions,           "exceptions", 32)
 
 #ifdef CONFIG_HVM
 
-#define VMX_PERF_EXIT_REASON_SIZE 75
+#define VMX_PERF_EXIT_REASON_SIZE 76
 #define VMEXIT_NPF_PERFC 143
 #define SVM_PERF_EXIT_REASON_SIZE (VMEXIT_NPF_PERFC + 1)
 PERFCOUNTER_ARRAY(vmexits,              "vmexits",
@@ -126,5 +126,6 @@ PERFCOUNTER(realmode_exits,      "vmexits from realmode")
 PERFCOUNTER(pauseloop_exits, "vmexits from Pause-Loop Detection")
 
 PERFCOUNTER(buslock, "Bus Locks Detected")
+PERFCOUNTER(vmnotify_crash, "domain crashes by Notify VM Exit")
 
 /*#endif*/ /* __XEN_PERFC_DEFN_H__ */
-- 
2.37.0

Re: [PATCH v3 1/3] x86/vmx: implement VMExit based guest Bus Lock detection

[Jan Beulich]

On 01.07.2022 15:16, Roger Pau Monne wrote:
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -4065,6 +4065,11 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>  
>      if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) )
>          return vmx_failed_vmentry(exit_reason, regs);
> +    if ( unlikely(exit_reason & VMX_EXIT_REASONS_BUS_LOCK) )
> +    {
> +        perfc_incr(buslock);
> +        exit_reason &= ~VMX_EXIT_REASONS_BUS_LOCK;
> +    }

To cover for the flag bit, don't you also need to mask it off in
nvmx_idtv_handling()? Or (didn't go into detail with checking whether
there aren't any counter indications) pass the exit reason there from
vmx_vmexit_handler(), instead of re-reading it from the VMCS?

Jan

Re: [PATCH v3 1/3] x86/vmx: implement VMExit based guest Bus Lock detection

[Roger Pau Monné]

On Mon, Jul 04, 2022 at 11:27:37AM +0200, Jan Beulich wrote:
> On 01.07.2022 15:16, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/hvm/vmx/vmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
> > @@ -4065,6 +4065,11 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
> >  
> >      if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) )
> >          return vmx_failed_vmentry(exit_reason, regs);
> > +    if ( unlikely(exit_reason & VMX_EXIT_REASONS_BUS_LOCK) )
> > +    {
> > +        perfc_incr(buslock);
> > +        exit_reason &= ~VMX_EXIT_REASONS_BUS_LOCK;
> > +    }
> 
> To cover for the flag bit, don't you also need to mask it off in
> nvmx_idtv_handling()? Or (didn't go into detail with checking whether
> there aren't any counter indications) pass the exit reason there from
> vmx_vmexit_handler(), instead of re-reading it from the VMCS?

This seem to be an existing issue with nvmx_idtv_handling(), as it
should use just the low 16bits to check against the VM Exit reason
codes.

I can send a pre-patch to fix it, could pass exit reason from
vmx_vmexit_handler(), but I would still need to cast to uint16_t for
comparing against exit reason codes, as there's a jump into the 'out'
label before VMX_EXIT_REASONS_BUS_LOCK is masked out.

I think there's a similar issue with nvmx_n2_vmexit_handler() that
doesn't cast the value to uint16_t and is called before
VMX_EXIT_REASONS_BUS_LOCK is removed from exit reason.

Thanks, Roger.

RE: [PATCH v3 1/3] x86/vmx: implement VMExit based guest Bus Lock detection

[Tian, Kevin]

> From: Roger Pau Monne <roger.pau@citrix.com>
> Sent: Friday, July 1, 2022 9:17 PM
> 
> @@ -4065,6 +4065,11 @@ void vmx_vmexit_handler(struct cpu_user_regs
> *regs)
> 
>      if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) )
>          return vmx_failed_vmentry(exit_reason, regs);

Add a blank line.

> +    if ( unlikely(exit_reason & VMX_EXIT_REASONS_BUS_LOCK) )
> +    {
> +        perfc_incr(buslock);
> +        exit_reason &= ~VMX_EXIT_REASONS_BUS_LOCK;
> +    }
> 
>      if ( v->arch.hvm.vmx.vmx_realmode )
>      {
> @@ -4561,6 +4566,15 @@ void vmx_vmexit_handler(struct cpu_user_regs
> *regs)
>          vmx_handle_descriptor_access(exit_reason);
>          break;
> 
> +    case EXIT_REASON_BUS_LOCK:
> +        /*
> +         * Nothing to do: just taking a vmexit should be enough of a pause to
> +         * prevent a VM from crippling the host with bus locks.  Note
> +         * EXIT_REASON_BUS_LOCK will always have bit 26 set in exit_reason,
> and
> +         * hence the perf counter is already increased.
> +         */
> +        break;
> +

Would it be helpful from diagnostic angle by throwing out a warning,
once per the culprit domain?

RE: [PATCH v3 1/3] x86/vmx: implement VMExit based guest Bus Lock detection

[Tian, Kevin]

> From: Roger Pau Monné <roger.pau@citrix.com>
> Sent: Monday, July 4, 2022 6:07 PM
> 
> On Mon, Jul 04, 2022 at 11:27:37AM +0200, Jan Beulich wrote:
> > On 01.07.2022 15:16, Roger Pau Monne wrote:
> > > --- a/xen/arch/x86/hvm/vmx/vmx.c
> > > +++ b/xen/arch/x86/hvm/vmx/vmx.c
> > > @@ -4065,6 +4065,11 @@ void vmx_vmexit_handler(struct
> cpu_user_regs *regs)
> > >
> > >      if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) )
> > >          return vmx_failed_vmentry(exit_reason, regs);
> > > +    if ( unlikely(exit_reason & VMX_EXIT_REASONS_BUS_LOCK) )
> > > +    {
> > > +        perfc_incr(buslock);
> > > +        exit_reason &= ~VMX_EXIT_REASONS_BUS_LOCK;
> > > +    }
> >
> > To cover for the flag bit, don't you also need to mask it off in
> > nvmx_idtv_handling()? Or (didn't go into detail with checking whether
> > there aren't any counter indications) pass the exit reason there from
> > vmx_vmexit_handler(), instead of re-reading it from the VMCS?
> 
> This seem to be an existing issue with nvmx_idtv_handling(), as it
> should use just the low 16bits to check against the VM Exit reason
> codes.
> 
> I can send a pre-patch to fix it, could pass exit reason from
> vmx_vmexit_handler(), but I would still need to cast to uint16_t for
> comparing against exit reason codes, as there's a jump into the 'out'
> label before VMX_EXIT_REASONS_BUS_LOCK is masked out.

or just masking out the bit in an earlier place which then also
covers nvmx_n2_vmexit_handler() below? There are a few other
goto's and return's before the point where that bit is currently
masked out. Having bus lock counted even in those failure paths
is also not a bad thing imho...

> 
> I think there's a similar issue with nvmx_n2_vmexit_handler() that
> doesn't cast the value to uint16_t and is called before
> VMX_EXIT_REASONS_BUS_LOCK is removed from exit reason.
> 

RE: [PATCH v3 2/3] x86/vmx: introduce helper to set VMX_INTR_SHADOW_NMI

[Tian, Kevin]

> From: Roger Pau Monne <roger.pau@citrix.com>
> Sent: Friday, July 1, 2022 9:17 PM
> 
> @@ -225,6 +225,9 @@ static inline void pi_clear_sn(struct pi_desc *pi_desc)
> 
>  /*
>   * Interruption-information format
> + *
> + * Note INTR_INFO_NMI_UNBLOCKED_BY_IRET is also used with Exit
> Qualification
> + * field under some circumstances.
>   */
>  #define INTR_INFO_VECTOR_MASK           0xff            /* 7:0 */
>  #define INTR_INFO_INTR_TYPE_MASK        0x700           /* 10:8 */

Overall this is good. But I wonder whether the readability is slightly better
by defining a dedicate flag bit for exit qualification with a clear comment
on which events it makes sense to...

RE: [PATCH v3 3/3] x86/vmx: implement Notify VM Exit

[Tian, Kevin]

> From: Roger Pau Monne <roger.pau@citrix.com>
> Sent: Friday, July 1, 2022 9:17 PM
> @@ -4589,6 +4601,22 @@ void vmx_vmexit_handler(struct cpu_user_regs
> *regs)
>           */
>          break;
> 
> +    case EXIT_REASON_NOTIFY:
> +        __vmread(EXIT_QUALIFICATION, &exit_qualification);
> +
> +        if ( exit_qualification & NOTIFY_VM_CONTEXT_INVALID )
> +        {

	if ( unlikely() )

Apart from that:

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

Re: [PATCH v3 1/3] x86/vmx: implement VMExit based guest Bus Lock detection

[Roger Pau Monné]

On Tue, Jul 19, 2022 at 07:26:08AM +0000, Tian, Kevin wrote:
> > From: Roger Pau Monne <roger.pau@citrix.com>
> > Sent: Friday, July 1, 2022 9:17 PM
> > 
> > @@ -4065,6 +4065,11 @@ void vmx_vmexit_handler(struct cpu_user_regs
> > *regs)
> > 
> >      if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) )
> >          return vmx_failed_vmentry(exit_reason, regs);
> 
> Add a blank line.
> 
> > +    if ( unlikely(exit_reason & VMX_EXIT_REASONS_BUS_LOCK) )
> > +    {
> > +        perfc_incr(buslock);
> > +        exit_reason &= ~VMX_EXIT_REASONS_BUS_LOCK;
> > +    }
> > 
> >      if ( v->arch.hvm.vmx.vmx_realmode )
> >      {
> > @@ -4561,6 +4566,15 @@ void vmx_vmexit_handler(struct cpu_user_regs
> > *regs)
> >          vmx_handle_descriptor_access(exit_reason);
> >          break;
> > 
> > +    case EXIT_REASON_BUS_LOCK:
> > +        /*
> > +         * Nothing to do: just taking a vmexit should be enough of a pause to
> > +         * prevent a VM from crippling the host with bus locks.  Note
> > +         * EXIT_REASON_BUS_LOCK will always have bit 26 set in exit_reason,
> > and
> > +         * hence the perf counter is already increased.
> > +         */
> > +        break;
> > +
> 
> Would it be helpful from diagnostic angle by throwing out a warning,
> once per the culprit domain?

Hm, not sure.  I've assumed that increasing the counter would be
enough, but that's not tied to a domain.

I will leave as-is unless someone else expresses interest in this (and
can also be added later if desired).

Thanks, Roger.

Re: [PATCH v3 2/3] x86/vmx: introduce helper to set VMX_INTR_SHADOW_NMI

[Roger Pau Monné]

On Tue, Jul 19, 2022 at 07:33:47AM +0000, Tian, Kevin wrote:
> > From: Roger Pau Monne <roger.pau@citrix.com>
> > Sent: Friday, July 1, 2022 9:17 PM
> > 
> > @@ -225,6 +225,9 @@ static inline void pi_clear_sn(struct pi_desc *pi_desc)
> > 
> >  /*
> >   * Interruption-information format
> > + *
> > + * Note INTR_INFO_NMI_UNBLOCKED_BY_IRET is also used with Exit
> > Qualification
> > + * field under some circumstances.
> >   */
> >  #define INTR_INFO_VECTOR_MASK           0xff            /* 7:0 */
> >  #define INTR_INFO_INTR_TYPE_MASK        0x700           /* 10:8 */
> 
> Overall this is good. But I wonder whether the readability is slightly better
> by defining a dedicate flag bit for exit qualification with a clear comment
> on which events it makes sense to...

I've expanded the comment to:

"Note INTR_INFO_NMI_UNBLOCKED_BY_IRET is also used with Exit Qualification
field for EPT violations, PML full and SPP-related event vmexits."

I leave the creation of the specific flag to a separate commit, to not
block the series on that.

Thanks, Roger.