Skip to contentSkip to conversation

kvm: handle host mode irqs 0

PATCH Superseded Nicolai Stange 5 participants 15 messages Jul 29, 2018 – Aug 1, 2018

Subsystems (12)
DRM drivers x86 Intel i915 PCI +8 more subsystems

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore
#1 lore raw

PATCH v2 1/8 kvm: handle host mode irqs 1

In reply to Nicolai Stange

vmx_l1d_flush() gets invoked only if ->l1tf_flush_l1d is true. There's no point in setting ->l1tf_flush_l1d to true from there again.

Don't do that.

---
 arch/x86/kvm/vmx.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)
arch/x86/kvm/vmx.c +7 -7
index b4b8e8cb4a7e..695e161ffb36 100644
@@ -9691,15 +9691,15 @@ static void vmx_l1d_flush(struct kvm_vcpu *vcpu)
 	/*
 	 * This code is only executed when the the flush mode is 'cond' or
 	 * 'always'
-	 *
-	 * If 'flush always', keep the flush bit set, otherwise clear
-	 * it. The flush bit gets set again either from vcpu_run() or from
-	 * one of the unsafe VMEXIT handlers.
 	 */
-	if (static_branch_unlikely(&vmx_l1d_flush_always))
-		vcpu->arch.l1tf_flush_l1d = true;
-	else
+	if (!static_branch_unlikely(&vmx_l1d_flush_always)) {
+		/*
+		 * Clear the flush bit, it gets set again either from
+		 * vcpu_run() or from one of the unsafe VMEXIT
+		 * handlers.
+		 */
 		vcpu->arch.l1tf_flush_l1d = false;
+	}
 
 	vcpu->stat.l1d_flush++;
 
Signature · 1 line
2.13.7
#4 lore raw

PATCH v2 4/8 kvm: handle host mode irqs 4

In reply to Nicolai Stange

An upcoming patch will extend KVM's L1TF mitigation in conditional mode to also cover interrupts after VMEXITs. For tracking those, stores to a new per-cpu flag from interrupt handlers will become necessary.

In order to improve cache locality, this new flag will be added to x86's irq_cpustat_t.

Make some space available there by shrinking the ->softirq_pending bitfield from 32 to 16 bits: the number of bits actually used is only NR_SOFTIRQS == 10.

---
 arch/x86/include/asm/hardirq.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/x86/include/asm/hardirq.h +1 -1
index 740a428acf1e..cd43dffa6fb4 100644
@@ -6,7 +6,7 @@
 #include <linux/irq.h>
 
 typedef struct {
-	unsigned int __softirq_pending;
+	u16 __softirq_pending;
 	unsigned int __nmi_count;	/* arch dependent */
 #ifdef CONFIG_X86_LOCAL_APIC
 	unsigned int apic_timer_irqs;	/* arch dependent */
Signature · 1 line
2.13.7
#3 lore raw

PATCH v2 3/8 kvm: handle host mode irqs 3

In reply to Nicolai Stange

Currently, vmx_vcpu_run() checks if ->l1tf_flush_l1d is set and invokes vmx_l1d_flush() if so.

This test is unncessary for the "always flush L1d" mode.

Move the check to vmx_l1d_flush()'s conditional mode code path.

Notes:
- vmx_l1d_flush() is likely to get inlined anyway and thus, there's no extra function call.
- This change inverts the (static) branch prediction, but there hadn't been any explicit likely()/unlikely() annotations before and so I decided to leave it as is.

---
 arch/x86/kvm/vmx.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
arch/x86/kvm/vmx.c +6 -4
index 5139738cc5a9..ec955b870756 100644
@@ -9693,12 +9693,16 @@ static void vmx_l1d_flush(struct kvm_vcpu *vcpu)
 	 * 'always'
 	 */
 	if (static_branch_likely(&vmx_l1d_flush_cond)) {
+		bool flush_l1d = vcpu->arch.l1tf_flush_l1d;
+
 		/*
 		 * Clear the flush bit, it gets set again either from
 		 * vcpu_run() or from one of the unsafe VMEXIT
 		 * handlers.
 		 */
 		vcpu->arch.l1tf_flush_l1d = false;
+		if (!flush_l1d)
+			return;
 	}
 
 	vcpu->stat.l1d_flush++;
@@ -10228,10 +10232,8 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
 	evmcs_rsp = static_branch_unlikely(&enable_evmcs) ?
 		(unsigned long)&current_evmcs->host_rsp : 0;
 
-	if (static_branch_unlikely(&vmx_l1d_should_flush)) {
-		if (vcpu->arch.l1tf_flush_l1d)
-			vmx_l1d_flush(vcpu);
-	}
+	if (static_branch_unlikely(&vmx_l1d_should_flush))
+		vmx_l1d_flush(vcpu);
 
 	asm(
 		/* Store host registers */
Signature · 1 line
2.13.7
#6 lore raw

PATCH v2 5/8 kvm: handle host mode irqs 5

In reply to Nicolai Stange

Part of the L1TF mitigation for vmx includes flushing the L1d cache upon VMENTRY.

L1d flushes are costly and two modes of operations are provided to users:
"always" and the more selective "conditional" mode.

If operating in the latter, the cache would get flushed only if a host side code path considered unconfined had been traversed. "Unconfined" in this context means that it might have pulled in sensitive data like user data or kernel crypto keys.

The need for L1d flushes is tracked by means of the per-vcpu flag ->l1tf_flush_l1d. KVM exit handlers considered unconfined set it. A vmx_l1d_flush() subsequently invoked before the next VMENTER will conduct a L1d flush based on its value and reset that flag again.

Currently, interrupts delivered "normally" while in root operation between VMEXIT and VMENTER are not taken into account. Part of the reason is that these don't leave any traces and thus, the vmx code is unable to tell if any such has happened.

As proposed by Paolo Bonzini, prepare for tracking all interrupts by introducing a new per-cpu flag, "kvm_cpu_l1tf_flush_l1d". It will be in strong analogy to the per-vcpu ->l1tf_flush_l1d.

A later patch will make interrupt handlers set it.

For the sake of cache locality, group kvm_cpu_l1tf_flush_l1d into x86' per-cpu irq_cpustat_t as suggested by Peter Zijlstra.

Provide the helpers kvm_set_cpu_l1tf_flush_l1d(), kvm_clear_cpu_l1tf_flush_l1d() and kvm_get_cpu_l1tf_flush_l1d(). Make them trivial resp. non-existent for !CONFIG_KVM_INTEL as appropriate.

Let vmx_l1d_flush() handle kvm_cpu_l1tf_flush_l1d in the same way as ->l1tf_flush_l1d.

---
 arch/x86/include/asm/hardirq.h | 24 ++++++++++++++++++++++++
 arch/x86/kvm/vmx.c             | 17 +++++++++++++----
 2 files changed, 37 insertions(+), 4 deletions(-)
arch/x86/include/asm/hardirq.h +24 -0
index cd43dffa6fb4..f0a54e5c4f61 100644
@@ -7,6 +7,9 @@
 
 typedef struct {
 	u16 __softirq_pending;
+#if IS_ENABLED(CONFIG_KVM_INTEL)
+	bool kvm_cpu_l1tf_flush_l1d;
+#endif
 	unsigned int __nmi_count;	/* arch dependent */
 #ifdef CONFIG_X86_LOCAL_APIC
 	unsigned int apic_timer_irqs;	/* arch dependent */
@@ -58,4 +61,25 @@ extern u64 arch_irq_stat_cpu(unsigned int cpu);
 extern u64 arch_irq_stat(void);
 #define arch_irq_stat		arch_irq_stat
 
+
+#if IS_ENABLED(CONFIG_KVM_INTEL)
+static inline void kvm_set_cpu_l1tf_flush_l1d(void)
+{
+	__this_cpu_write(irq_stat.kvm_cpu_l1tf_flush_l1d, true);
+}
+
+static inline void kvm_clear_cpu_l1tf_flush_l1d(void)
+{
+	__this_cpu_write(irq_stat.kvm_cpu_l1tf_flush_l1d, false);
+}
+
+static inline bool kvm_get_cpu_l1tf_flush_l1d(void)
+{
+	return __this_cpu_read(irq_stat.kvm_cpu_l1tf_flush_l1d);
+}
+#else /* !IS_ENABLED(CONFIG_KVM_INTEL) */
+static inline void kvm_set_cpu_l1tf_flush_l1d(void)
+{}
+#endif /* IS_ENABLED(CONFIG_KVM_INTEL) */
+
 #endif /* _ASM_X86_HARDIRQ_H */
arch/x86/kvm/vmx.c +13 -4
index ec955b870756..33892ddd229f 100644
@@ -9693,14 +9693,23 @@ static void vmx_l1d_flush(struct kvm_vcpu *vcpu)
 	 * 'always'
 	 */
 	if (static_branch_likely(&vmx_l1d_flush_cond)) {
-		bool flush_l1d = vcpu->arch.l1tf_flush_l1d;
+		bool flush_l1d;
 
 		/*
-		 * Clear the flush bit, it gets set again either from
-		 * vcpu_run() or from one of the unsafe VMEXIT
-		 * handlers.
+		 * Clear the per-vcpu flush bit, it gets set again
+		 * either from vcpu_run() or from one of the unsafe
+		 * VMEXIT handlers.
 		 */
+		flush_l1d = vcpu->arch.l1tf_flush_l1d;
 		vcpu->arch.l1tf_flush_l1d = false;
+
+		/*
+		 * Clear the per-cpu flush bit, it gets set again from
+		 * the interrupt handlers.
+		 */
+		flush_l1d |= kvm_get_cpu_l1tf_flush_l1d();
+		kvm_clear_cpu_l1tf_flush_l1d();
+
 		if (!flush_l1d)
 			return;
 	}
Signature · 1 line
2.13.7
#12 lore raw

PATCH v2 6/8 kvm: handle host mode irqs 6

In reply to Nicolai Stange

The next patch in this series will have to make the definition of irq_cpustat_t available to entering_irq().

Inclusion of asm/hardirq.h into asm/apic.h would cause circular header dependencies like

asm/smp.h
asm/apic.h
asm/hardirq.h
linux/irq.h
linux/topology.h
linux/smp.h
asm/smp.h

or

linux/gfp.h

    linux/mmzone.h
      asm/mmzone.h
        asm/mmzone_64.h
          asm/smp.h
            asm/apic.h
              asm/hardirq.h
                linux/irq.h
                  linux/irqdesc.h
                    linux/kobject.h
                      linux/sysfs.h
                        linux/kernfs.h
                          linux/idr.h
                            linux/gfp.h

and others.

This causes compilation errors because of the header guards becoming effective in the second inclusion: symbols/macros that had been defined before wouldn't be available to intermediate headers in the #include chain anymore.

A possible workaround would be to move the definition of irq_cpustat_t into its own header and include that from both, asm/hardirq.h and asm/apic.h.

However, this wouldn't solve the real problem, namely asm/harirq.h unnecessarily pulling in all the linux/irq.h cruft: nothing in asm/hardirq.h itself requires it. Also, note that there are some other archs, like e.g. arm64, which don't have that #include in their asm/hardirq.h.

Remove the linux/irq.h #include from x86' asm/hardirq.h.

Fix resulting compilation errors by adding appropriate #includes to *.c files as needed.

Note that some of these *.c files could be cleaned up a bit wrt. to their set of #includes, but that should better be done from separate patches, if at all.

---
 arch/x86/include/asm/dmi.h                                   | 2 +-
 arch/x86/include/asm/hardirq.h                               | 1 -
 arch/x86/include/asm/kvm_host.h                              | 1 +
 arch/x86/kernel/apic/apic.c                                  | 1 +
 arch/x86/kernel/apic/io_apic.c                               | 1 +
 arch/x86/kernel/apic/msi.c                                   | 1 +
 arch/x86/kernel/apic/vector.c                                | 1 +
 arch/x86/kernel/fpu/core.c                                   | 1 +
 arch/x86/kernel/hpet.c                                       | 1 +
 arch/x86/kernel/i8259.c                                      | 1 +
 arch/x86/kernel/idt.c                                        | 1 +
 arch/x86/kernel/irq.c                                        | 1 +
 arch/x86/kernel/irq_32.c                                     | 1 +
 arch/x86/kernel/irq_64.c                                     | 1 +
 arch/x86/kernel/irqinit.c                                    | 1 +
 arch/x86/kernel/smpboot.c                                    | 1 +
 arch/x86/kernel/time.c                                       | 1 +
 arch/x86/mm/pti.c                                            | 1 +
 arch/x86/platform/intel-mid/device_libs/platform_mrfld_wdt.c | 1 +
 arch/x86/xen/enlighten.c                                     | 1 +
 drivers/gpu/drm/i915/i915_pmu.c                              | 1 +
 drivers/gpu/drm/i915/intel_lpe_audio.c                       | 1 +
 drivers/pci/controller/pci-hyperv.c                          | 1 +
 23 files changed, 22 insertions(+), 2 deletions(-)
arch/x86/include/asm/dmi.h +1 -1
index 0ab2ab27ad1f..b825cb201251 100644
@@ -4,8 +4,8 @@
 
 #include <linux/compiler.h>
 #include <linux/init.h>
+#include <linux/io.h>
 
-#include <asm/io.h>
 #include <asm/setup.h>
 
 static __always_inline __init void *dmi_alloc(unsigned len)
arch/x86/include/asm/hardirq.h +0 -1
index f0a54e5c4f61..35736bd3828c 100644
@@ -3,7 +3,6 @@
 #define _ASM_X86_HARDIRQ_H
 
 #include <linux/threads.h>
-#include <linux/irq.h>
 
 typedef struct {
 	u16 __softirq_pending;
arch/x86/include/asm/kvm_host.h +1 -0
index 57d418061c55..37749429afd9 100644
@@ -17,6 +17,7 @@
 #include <linux/tracepoint.h>
 #include <linux/cpumask.h>
 #include <linux/irq_work.h>
+#include <linux/irq.h>
 
 #include <linux/kvm.h>
 #include <linux/kvm_para.h>
arch/x86/kernel/apic/apic.c +1 -0
index 8703caa9d6db..f93a21b01072 100644
@@ -56,6 +56,7 @@
 #include <asm/hypervisor.h>
 #include <asm/cpu_device_id.h>
 #include <asm/intel-family.h>
+#include <asm/irq_regs.h>
 
 unsigned int num_processors;
 
arch/x86/kernel/apic/io_apic.c +1 -0
index 3982f79d2377..ff0d14cd9e82 100644
@@ -33,6 +33,7 @@
 
 #include <linux/mm.h>
 #include <linux/interrupt.h>
+#include <linux/irq.h>
 #include <linux/init.h>
 #include <linux/delay.h>
 #include <linux/sched.h>
arch/x86/kernel/apic/msi.c +1 -0
index ce503c99f5c4..72a94401f9e0 100644
@@ -12,6 +12,7 @@
  */
 #include <linux/mm.h>
 #include <linux/interrupt.h>
+#include <linux/irq.h>
 #include <linux/pci.h>
 #include <linux/dmar.h>
 #include <linux/hpet.h>
arch/x86/kernel/apic/vector.c +1 -0
index 35aaee4fc028..c9b773401fd8 100644
@@ -11,6 +11,7 @@
  * published by the Free Software Foundation.
  */
 #include <linux/interrupt.h>
+#include <linux/irq.h>
 #include <linux/seq_file.h>
 #include <linux/init.h>
 #include <linux/compiler.h>
arch/x86/kernel/fpu/core.c +1 -0
index f92a6593de1e..2ea85b32421a 100644
@@ -10,6 +10,7 @@
 #include <asm/fpu/signal.h>
 #include <asm/fpu/types.h>
 #include <asm/traps.h>
+#include <asm/irq_regs.h>
 
 #include <linux/hardirq.h>
 #include <linux/pkeys.h>
arch/x86/kernel/hpet.c +1 -0
index 346b24883911..b0acb22e5a46 100644
@@ -1,6 +1,7 @@
 #include <linux/clocksource.h>
 #include <linux/clockchips.h>
 #include <linux/interrupt.h>
+#include <linux/irq.h>
 #include <linux/export.h>
 #include <linux/delay.h>
 #include <linux/errno.h>
arch/x86/kernel/i8259.c +1 -0
index 86c4439f9d74..519649ddf100 100644
@@ -5,6 +5,7 @@
 #include <linux/sched.h>
 #include <linux/ioport.h>
 #include <linux/interrupt.h>
+#include <linux/irq.h>
 #include <linux/timex.h>
 #include <linux/random.h>
 #include <linux/init.h>
arch/x86/kernel/idt.c +1 -0
index 74383a3780dc..01adea278a71 100644
@@ -8,6 +8,7 @@
 #include <asm/traps.h>
 #include <asm/proto.h>
 #include <asm/desc.h>
+#include <asm/hw_irq.h>
 
 struct idt_data {
 	unsigned int	vector;
arch/x86/kernel/irq.c +1 -0
index 328d027d829d..59b5f2ea7c2f 100644
@@ -10,6 +10,7 @@
 #include <linux/ftrace.h>
 #include <linux/delay.h>
 #include <linux/export.h>
+#include <linux/irq.h>
 
 #include <asm/apic.h>
 #include <asm/io_apic.h>
arch/x86/kernel/irq_32.c +1 -0
index c1bdbd3d3232..95600a99ae93 100644
@@ -11,6 +11,7 @@
 
 #include <linux/seq_file.h>
 #include <linux/interrupt.h>
+#include <linux/irq.h>
 #include <linux/kernel_stat.h>
 #include <linux/notifier.h>
 #include <linux/cpu.h>
arch/x86/kernel/irq_64.c +1 -0
index d86e344f5b3d..0469cd078db1 100644
@@ -11,6 +11,7 @@
 
 #include <linux/kernel_stat.h>
 #include <linux/interrupt.h>
+#include <linux/irq.h>
 #include <linux/seq_file.h>
 #include <linux/delay.h>
 #include <linux/ftrace.h>
arch/x86/kernel/irqinit.c +1 -0
index 772196c1b8c4..a0693b71cfc1 100644
@@ -5,6 +5,7 @@
 #include <linux/sched.h>
 #include <linux/ioport.h>
 #include <linux/interrupt.h>
+#include <linux/irq.h>
 #include <linux/timex.h>
 #include <linux/random.h>
 #include <linux/kprobes.h>
arch/x86/kernel/smpboot.c +1 -0
index 7f7def989fb0..f5d30c68fd09 100644
@@ -80,6 +80,7 @@
 #include <asm/intel-family.h>
 #include <asm/cpu_device_id.h>
 #include <asm/spec-ctrl.h>
+#include <asm/hw_irq.h>
 
 /* representing HT siblings of each logical CPU */
 DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_sibling_map);
arch/x86/kernel/time.c +1 -0
index 774ebafa97c4..be01328eb755 100644
@@ -12,6 +12,7 @@
 
 #include <linux/clockchips.h>
 #include <linux/interrupt.h>
+#include <linux/irq.h>
 #include <linux/i8253.h>
 #include <linux/time.h>
 #include <linux/export.h>
arch/x86/mm/pti.c +1 -0
index 4d418e705878..fb752d9a3ce9 100644
@@ -45,6 +45,7 @@
 #include <asm/pgalloc.h>
 #include <asm/tlbflush.h>
 #include <asm/desc.h>
+#include <asm/sections.h>
 
 #undef pr_fmt
 #define pr_fmt(fmt)     "Kernel/User page tables isolation: " fmt
arch/x86/platform/intel-mid/device_libs/platform_mrfld_wdt.c +1 -0
index 4f5fa65a1011..2acd6be13375 100644
@@ -18,6 +18,7 @@
 #include <asm/intel-mid.h>
 #include <asm/intel_scu_ipc.h>
 #include <asm/io_apic.h>
+#include <asm/hw_irq.h>
 
 #define TANGIER_EXT_TIMER0_MSI 12
 
arch/x86/xen/enlighten.c +1 -0
index c9081c6671f0..df208af3cd74 100644
@@ -3,6 +3,7 @@
 #endif
 #include <linux/cpu.h>
 #include <linux/kexec.h>
+#include <linux/slab.h>
 
 #include <xen/features.h>
 #include <xen/page.h>
drivers/gpu/drm/i915/i915_pmu.c +1 -0
index dc87797db500..b50b74053664 100644
@@ -5,5 +5,6 @@
  */
 
+#include <linux/irq.h>
 #include "i915_pmu.h"
 #include "intel_ringbuffer.h"
 #include "i915_drv.h"
drivers/gpu/drm/i915/intel_lpe_audio.c +1 -0
index 6269750e2b54..b4941101f21a 100644
@@ -62,6 +62,7 @@
 
 #include <linux/acpi.h>
 #include <linux/device.h>
+#include <linux/irq.h>
 #include <linux/pci.h>
 #include <linux/pm_runtime.h>
 
drivers/pci/controller/pci-hyperv.c +1 -0
index 6cc5036ac83c..fadc305533d9 100644
@@ -45,6 +45,7 @@
 #include <linux/irqdomain.h>
 #include <asm/irqdomain.h>
 #include <asm/apic.h>
+#include <linux/irq.h>
 #include <linux/msi.h>
 #include <linux/hyperv.h>
 #include <linux/refcount.h>
Signature · 1 line
2.13.7
#2 lore raw

PATCH v2 2/8 kvm: handle host mode irqs 2

In reply to Nicolai Stange

The vmx_l1d_flush_always static key is only ever evaluated if vmx_l1d_should_flush is enabled. In that case however, there are only two L1d flushing modes possible: "always" and "conditional".

The "conditional" mode's implementation tends to require more sophisticated logic than the "always" mode.

Avoid inverted logic by replacing the 'vmx_l1d_flush_always' static key with a 'vmx_l1d_flush_cond' one.

There is no change in functionality.

---
 arch/x86/kvm/vmx.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
arch/x86/kvm/vmx.c +5 -5
index 695e161ffb36..5139738cc5a9 100644
@@ -189,7 +189,7 @@ module_param(ple_window_max, uint, 0444);
 extern const ulong vmx_return;
 
 static DEFINE_STATIC_KEY_FALSE(vmx_l1d_should_flush);
-static DEFINE_STATIC_KEY_FALSE(vmx_l1d_flush_always);
+static DEFINE_STATIC_KEY_FALSE(vmx_l1d_flush_cond);
 static DEFINE_MUTEX(vmx_l1d_flush_mutex);
 
 /* Storage for pre module init parameter parsing */
@@ -263,10 +263,10 @@ static int vmx_setup_l1d_flush(enum vmx_l1d_flush_state l1tf)
 	else
 		static_branch_disable(&vmx_l1d_should_flush);
 
-	if (l1tf == VMENTER_L1D_FLUSH_ALWAYS)
-		static_branch_enable(&vmx_l1d_flush_always);
+	if (l1tf == VMENTER_L1D_FLUSH_COND)
+		static_branch_enable(&vmx_l1d_flush_cond);
 	else
-		static_branch_disable(&vmx_l1d_flush_always);
+		static_branch_disable(&vmx_l1d_flush_cond);
 	return 0;
 }
 
@@ -9692,7 +9692,7 @@ static void vmx_l1d_flush(struct kvm_vcpu *vcpu)
 	 * This code is only executed when the the flush mode is 'cond' or
 	 * 'always'
 	 */
-	if (!static_branch_unlikely(&vmx_l1d_flush_always)) {
+	if (static_branch_likely(&vmx_l1d_flush_cond)) {
 		/*
 		 * Clear the flush bit, it gets set again either from
 		 * vcpu_run() or from one of the unsafe VMEXIT
Signature · 1 line
2.13.7
#13 lore raw

PATCH v2 7/8 kvm: handle host mode irqs 7

In reply to Nicolai Stange

The last missing piece to having vmx_l1d_flush() take interrupts after VMEXIT into account is to set the kvm_cpu_l1tf_flush_l1d per-cpu flag on irq entry.

Issue calls to kvm_set_cpu_l1tf_flush_l1d() from entering_irq(), ipi_entering_ack_irq(), smp_reschedule_interrupt() and uv_bau_message_interrupt().

---
 arch/x86/include/asm/apic.h   | 3 +++
 arch/x86/kernel/smp.c         | 1 +
 arch/x86/platform/uv/tlb_uv.c | 1 +
 3 files changed, 5 insertions(+)
arch/x86/include/asm/apic.h +3 -0
index 9362a3aae927..130e81e10fc7 100644
@@ -10,6 +10,7 @@
 #include <asm/fixmap.h>
 #include <asm/mpspec.h>
 #include <asm/msr.h>
+#include <asm/hardirq.h>
 
 #define ARCH_APICTIMER_STOPS_ON_C3	1
 
@@ -514,6 +515,7 @@ extern void irq_exit(void);
 static inline void entering_irq(void)
 {
 	irq_enter();
+	kvm_set_cpu_l1tf_flush_l1d();
 }
 
 static inline void entering_ack_irq(void)
@@ -526,6 +528,7 @@ static inline void ipi_entering_ack_irq(void)
 {
 	irq_enter();
 	ack_APIC_irq();
+	kvm_set_cpu_l1tf_flush_l1d();
 }
 
 static inline void exiting_irq(void)
arch/x86/kernel/smp.c +1 -0
index 5c574dff4c1a..04adc8d60aed 100644
@@ -261,6 +261,7 @@ __visible void __irq_entry smp_reschedule_interrupt(struct pt_regs *regs)
 {
 	ack_APIC_irq();
 	inc_irq_stat(irq_resched_count);
+	kvm_set_cpu_l1tf_flush_l1d();
 
 	if (trace_resched_ipi_enabled()) {
 		/*
arch/x86/platform/uv/tlb_uv.c +1 -0
index ca446da48fd2..3866b96a7ee7 100644
@@ -1285,6 +1285,7 @@ void uv_bau_message_interrupt(struct pt_regs *regs)
 	struct msg_desc msgdesc;
 
 	ack_APIC_irq();
+	kvm_set_cpu_l1tf_flush_l1d();
 	time_start = get_cycles();
 
 	bcp = &per_cpu(bau_control, smp_processor_id());
Signature · 1 line
2.13.7
#14 lore raw

PATCH v2 8/8 kvm: handle host mode irqs 8

In reply to Nicolai Stange

For VMEXITs caused by external interrupts, vmx_handle_external_intr() indirectly calls into the interrupt handlers through the host's IDT.

It follows that these interrupts get accounted for in the kvm_cpu_l1tf_flush_l1d per-cpu flag.

The subsequently executed vmx_l1d_flush() will thus be aware that some interrupts have happened and conduct a L1d flush anyway.

Setting ->l1tf_flush_l1d from vmx_handle_external_intr() isn't needed anymore. Drop it.

---
 arch/x86/kvm/vmx.c | 1 -
 1 file changed, 1 deletion(-)
arch/x86/kvm/vmx.c +0 -1
index 33892ddd229f..c6594fe38437 100644
@@ -9983,7 +9983,6 @@ static void vmx_handle_external_intr(struct kvm_vcpu *vcpu)
 			[ss]"i"(__KERNEL_DS),
 			[cs]"i"(__KERNEL_CS)
 			);
-		vcpu->arch.l1tf_flush_l1d = true;
 	}
 }
 STACK_FRAME_NON_STANDARD(vmx_handle_external_intr);
Signature · 1 line
2.13.7
#0 lore raw

v2 cover kvm: handle host mode irqs 0

Changes to v1 (the initial RFC series):
turn the kernel_mode_irq_gen per-cpu counter into a latch flag (Paolo Bonzini). Group it into irq_cpustat_t for cache locality (Peter Zijlstra). Set it from C rather than asm entry code (Andi Kleen).

Details
- [1-3/8]: unchanged

- [4/8] ("x86/irq: demote irq_cpustat_t's ->__softirq_pending to u16"): new, prerequisite for [5/8].

- [5/8] ("x86/KVM/VMX: introduce per-host-cpu analogue of ->l1tf_flush_l1d"):
roughly corresponds to former [5/6] ("x86/KVM/VMX: flush L1d upon interrupt after VMEXIT"). Introduce the new latch flag rather than taking that kernel_mode_irq_gen snapshot.

- [6/8] ("x86: don't include linux/irq.h from asm/hardirq.h"): new, prerequisite for [7/8].
I tested ARCH=i386 and ARCH=x86_64 allmodconfig compilations. (For i386 I had to disable CONFIG_BPFILTER_UMH because of a cross compilation issue.)

- [7/8] ("x86: let interrupt handlers set kvm_cpu_l1tf_flush_l1d"):
roughly corresponds to former [4/6] ("x86: implement kernel mode irq tracking"). Replace that kernel_mode_irq_gen increment from entry code by setting the new latch flag from entering_irq() & Co.

- [8/8] ("x86/KVM/VMX: don't set ->l1tf_flush_l1d from vmx_handle_external_intr()"):
this is the original [6/6] with its changelog slightly adjusted.

Nicolai Stange (8):
x86/KVM/VMX: don't set ->l1tf_flush_l1d to true from vmx_l1d_flush() x86/KVM/VMX: replace 'vmx_l1d_flush_always' with 'vmx_l1d_flush_cond' x86/KVM/VMX: move the ->l1tf_flush_l1d test to vmx_l1d_flush() x86/irq: demote irq_cpustat_t's ->__softirq_pending to u16 x86/KVM/VMX: introduce per-host-cpu analogue of ->l1tf_flush_l1d
x86: don't include linux/irq.h from asm/hardirq.h
x86: let interrupt handlers set kvm_cpu_l1tf_flush_l1d x86/KVM/VMX: don't set ->l1tf_flush_l1d from
vmx_handle_external_intr()

 arch/x86/include/asm/apic.h                        |  3 ++
 arch/x86/include/asm/dmi.h                         |  2 +-
 arch/x86/include/asm/hardirq.h                     | 27 ++++++++++++--
 arch/x86/include/asm/kvm_host.h                    |  1 +
 arch/x86/kernel/apic/apic.c                        |  1 +
 arch/x86/kernel/apic/io_apic.c                     |  1 +
 arch/x86/kernel/apic/msi.c                         |  1 +
 arch/x86/kernel/apic/vector.c                      |  1 +
 arch/x86/kernel/fpu/core.c                         |  1 +
 arch/x86/kernel/hpet.c                             |  1 +
 arch/x86/kernel/i8259.c                            |  1 +
 arch/x86/kernel/idt.c                              |  1 +
 arch/x86/kernel/irq.c                              |  1 +
 arch/x86/kernel/irq_32.c                           |  1 +
 arch/x86/kernel/irq_64.c                           |  1 +
 arch/x86/kernel/irqinit.c                          |  1 +
 arch/x86/kernel/smp.c                              |  1 +
 arch/x86/kernel/smpboot.c                          |  1 +
 arch/x86/kernel/time.c                             |  1 +
 arch/x86/kvm/vmx.c                                 | 42 +++++++++++++---------
 arch/x86/mm/pti.c                                  |  1 +
 .../intel-mid/device_libs/platform_mrfld_wdt.c     |  1 +
 arch/x86/platform/uv/tlb_uv.c                      |  1 +
 arch/x86/xen/enlighten.c                           |  1 +
 drivers/gpu/drm/i915/i915_pmu.c                    |  1 +
 drivers/gpu/drm/i915/intel_lpe_audio.c             |  1 +
 drivers/pci/controller/pci-hyperv.c                |  1 +
 27 files changed, 78 insertions(+), 19 deletions(-)
Signature · 1 line
2.13.7
maintainer Reviewed-by on v2 5/8
#7 lore raw

[MODERATED] Re: [PATCH v2 5/8] kvm: handle host mode irqs 5

In reply to Nicolai Stange
Quoted 38 lines · speck for Nicolai Stange

From: Nicolai Stange <[email protected]>
Subject: [PATCH v2 5/8] x86/KVM/VMX: introduce per-host-cpu analogue of
->l1tf_flush_l1d

Part of the L1TF mitigation for vmx includes flushing the L1d cache upon VMENTRY.

L1d flushes are costly and two modes of operations are provided to users:
"always" and the more selective "conditional" mode.

If operating in the latter, the cache would get flushed only if a host side code path considered unconfined had been traversed. "Unconfined" in this context means that it might have pulled in sensitive data like user data or kernel crypto keys.

The need for L1d flushes is tracked by means of the per-vcpu flag ->l1tf_flush_l1d. KVM exit handlers considered unconfined set it. A vmx_l1d_flush() subsequently invoked before the next VMENTER will conduct a L1d flush based on its value and reset that flag again.

Currently, interrupts delivered "normally" while in root operation between VMEXIT and VMENTER are not taken into account. Part of the reason is that these don't leave any traces and thus, the vmx code is unable to tell if any such has happened.

As proposed by Paolo Bonzini, prepare for tracking all interrupts by introducing a new per-cpu flag, "kvm_cpu_l1tf_flush_l1d". It will be in strong analogy to the per-vcpu ->l1tf_flush_l1d.

A later patch will make interrupt handlers set it.

For the sake of cache locality, group kvm_cpu_l1tf_flush_l1d into x86' per-cpu irq_cpustat_t as suggested by Peter Zijlstra.

Provide the helpers kvm_set_cpu_l1tf_flush_l1d(), kvm_clear_cpu_l1tf_flush_l1d() and kvm_get_cpu_l1tf_flush_l1d(). Make them trivial resp. non-existent for !CONFIG_KVM_INTEL as appropriate.

Quoted 6 lines

Let vmx_l1d_flush() handle kvm_cpu_l1tf_flush_l1d in the same way as ->l1tf_flush_l1d.

Quoted 78 lines
---
 arch/x86/include/asm/hardirq.h | 24 ++++++++++++++++++++++++
 arch/x86/kvm/vmx.c             | 17 +++++++++++++----
 2 files changed, 37 insertions(+), 4 deletions(-)
arch/x86/include/asm/hardirq.h +24 -0
index cd43dffa6fb4..f0a54e5c4f61 100644
@@ -7,6 +7,9 @@
 
 typedef struct {
 	u16 __softirq_pending;
+#if IS_ENABLED(CONFIG_KVM_INTEL)
+	bool kvm_cpu_l1tf_flush_l1d;
+#endif
 	unsigned int __nmi_count;	/* arch dependent */
 #ifdef CONFIG_X86_LOCAL_APIC
 	unsigned int apic_timer_irqs;	/* arch dependent */
@@ -58,4 +61,25 @@ extern u64 arch_irq_stat_cpu(unsigned int cpu);
 extern u64 arch_irq_stat(void);
 #define arch_irq_stat		arch_irq_stat
 
+
+#if IS_ENABLED(CONFIG_KVM_INTEL)
+static inline void kvm_set_cpu_l1tf_flush_l1d(void)
+{
+	__this_cpu_write(irq_stat.kvm_cpu_l1tf_flush_l1d, true);
+}
+
+static inline void kvm_clear_cpu_l1tf_flush_l1d(void)
+{
+	__this_cpu_write(irq_stat.kvm_cpu_l1tf_flush_l1d, false);
+}
+
+static inline bool kvm_get_cpu_l1tf_flush_l1d(void)
+{
+	return __this_cpu_read(irq_stat.kvm_cpu_l1tf_flush_l1d);
+}
+#else /* !IS_ENABLED(CONFIG_KVM_INTEL) */
+static inline void kvm_set_cpu_l1tf_flush_l1d(void)
+{}
+#endif /* IS_ENABLED(CONFIG_KVM_INTEL) */
+
 #endif /* _ASM_X86_HARDIRQ_H */
arch/x86/kvm/vmx.c +13 -4
index ec955b870756..33892ddd229f 100644
@@ -9693,14 +9693,23 @@ static void vmx_l1d_flush(struct kvm_vcpu *vcpu)
 	 * 'always'
 	 */
 	if (static_branch_likely(&vmx_l1d_flush_cond)) {
-		bool flush_l1d = vcpu->arch.l1tf_flush_l1d;
+		bool flush_l1d;
 
 		/*
-		 * Clear the flush bit, it gets set again either from
-		 * vcpu_run() or from one of the unsafe VMEXIT
-		 * handlers.
+		 * Clear the per-vcpu flush bit, it gets set again
+		 * either from vcpu_run() or from one of the unsafe
+		 * VMEXIT handlers.
 		 */
+		flush_l1d = vcpu->arch.l1tf_flush_l1d;
 		vcpu->arch.l1tf_flush_l1d = false;
+
+		/*
+		 * Clear the per-cpu flush bit, it gets set again from
+		 * the interrupt handlers.
+		 */
+		flush_l1d |= kvm_get_cpu_l1tf_flush_l1d();
+		kvm_clear_cpu_l1tf_flush_l1d();
+
 		if (!flush_l1d)
 			return;
 	}
maintainer Reviewed-by on v2 4/8
#5 lore raw

[MODERATED] Re: [PATCH v2 4/8] kvm: handle host mode irqs 4

In reply to Nicolai Stange
Quoted 27 lines · speck for Nicolai Stange

From: Nicolai Stange <[email protected]>
Subject: [PATCH v2 4/8] x86/irq: demote irq_cpustat_t's ->__softirq_pending to
u16

An upcoming patch will extend KVM's L1TF mitigation in conditional mode to also cover interrupts after VMEXITs. For tracking those, stores to a new per-cpu flag from interrupt handlers will become necessary.

In order to improve cache locality, this new flag will be added to x86's irq_cpustat_t.

Make some space available there by shrinking the ->softirq_pending bitfield from 32 to 16 bits: the number of bits actually used is only NR_SOFTIRQS == 10.

---
 arch/x86/include/asm/hardirq.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/x86/include/asm/hardirq.h +0 -0
index 740a428acf1e..cd43dffa6fb4 100644

arch/x86/include/asm/hardirq.h line 12

Quoted 8 lines
arch/x86/include/asm/hardirq.h +1 -1
@@ -6,7 +6,7 @@
 #include <linux/irq.h>
 
 typedef struct {
-	unsigned int __softirq_pending;
+	u16 __softirq_pending;
 	unsigned int __nmi_count;	/* arch dependent */
 #ifdef CONFIG_X86_LOCAL_APIC
 	unsigned int apic_timer_irqs;	/* arch dependent */
maintainer
#8 lore raw

[MODERATED] Re: [PATCH v2 5/8] kvm: handle host mode irqs 5

In reply to Nicolai Stange

arch/x86/include/asm/hardirq.h line 12

Quoted 9 lines · speck for Nicolai Stange
arch/x86/include/asm/hardirq.h +3 -0
@@ -7,6 +7,9 @@
 
 typedef struct {
 	u16 __softirq_pending;
+#if IS_ENABLED(CONFIG_KVM_INTEL)
+	bool kvm_cpu_l1tf_flush_l1d;
+#endif

NAK on using _Bool in a composite type.

#9 lore raw

[MODERATED] Re: [PATCH v2 5/8] kvm: handle host mode irqs 5

Quoted 5 lines · speck for Peter Zijlstra
Quoted 4 lines · speck for Nicolai Stange
arch/x86/include/asm/hardirq.h +0 -0
@@ -7,6 +7,9 @@
 
Quoted 8 lines

arch/x86/include/asm/hardirq.h line 12

Quoted 5 lines
arch/x86/include/asm/hardirq.h +3 -0
 typedef struct {
 	u16 __softirq_pending;
+#if IS_ENABLED(CONFIG_KVM_INTEL)
+	bool kvm_cpu_l1tf_flush_l1d;
+#endif

NAK on using _Bool in a composite type.

Do I get it right that you'd prefer a bitfield?

If so, I chose a bool here because updating a bitfield would involve a rmw operation and we might want to make NMIs set that flag opportunistically in a later patch.

This series ignores NMIs for simplicity, because those issued after local_irq_disable() and before VMENTER would still get missed.

Or did I miss your point completely?

Thanks,

Nicolai

Signature · 2 lines
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton,
HRB 21284 (AG Nürnberg)
#10 lore raw

[MODERATED] [AUTOREPLY] Re: [PATCH v2 5/8] kvm: handle host mode irqs 5

I'm out of the office until Monday August 6, 2018.

Josh

#11 lore raw

Re: [PATCH v2 5/8] kvm: handle host mode irqs 5

In reply to Nicolai Stange
Quoted 18 lines · speck for Nicolai Stange
Quoted 5 lines · speck for Peter Zijlstra
Quoted 4 lines · speck for Nicolai Stange
arch/x86/include/asm/hardirq.h +0 -0
@@ -7,6 +7,9 @@
 
Quoted 8 lines

arch/x86/include/asm/hardirq.h line 12

Quoted 5 lines
arch/x86/include/asm/hardirq.h +3 -0
 typedef struct {
 	u16 __softirq_pending;
+#if IS_ENABLED(CONFIG_KVM_INTEL)
+	bool kvm_cpu_l1tf_flush_l1d;
+#endif

NAK on using _Bool in a composite type.

Do I get it right that you'd prefer a bitfield?

Quoted 8 lines

If so, I chose a bool here because updating a bitfield would involve a rmw operation and we might want to make NMIs set that flag opportunistically in a later patch.

This series ignores NMIs for simplicity, because those issued after local_irq_disable() and before VMENTER would still get missed.

Or did I miss your point completely?

Use an explicit type, because bool storage size is implementation defined. It's a byte on x86, but in general we avoid it. So just use an u16 or such which avoids the rmw as well.

Thanks.

tglx