diff mbox series

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

Message ID 20220701131648.34292-2-roger.pau@citrix.com (mailing list archive)
State New, archived
Headers show
Series x86/vmx: implement Bus Lock and VM Notify | expand

Commit Message

Roger Pau Monné July 1, 2022, 1:16 p.m. UTC 
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(-)

Comments

Jan Beulich July 4, 2022, 9:27 a.m. UTC | #1 
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
Roger Pau Monné July 4, 2022, 10:07 a.m. UTC | #2 
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.
Tian, Kevin July 19, 2022, 7:26 a.m. UTC | #3 
> 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?
Tian, Kevin July 19, 2022, 7:31 a.m. UTC | #4 
> 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.
>
Roger Pau Monné Dec. 13, 2022, 11:58 a.m. UTC | #5 
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.
diff mbox series

Patch

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__ */