kvm: handle host mode irqs 0
Subsystems (12)
PATCH v2 1/8 kvm: handle host mode irqs 1
In reply to Nicolai Stangevmx_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
@@ -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
PATCH v2 4/8 kvm: handle host mode irqs 4
In reply to Nicolai StangeAn 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
@@ -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
PATCH v2 3/8 kvm: handle host mode irqs 3
In reply to Nicolai StangeCurrently, 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
@@ -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 )¤t_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
PATCH v2 5/8 kvm: handle host mode irqs 5
In reply to Nicolai StangePart 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
@@ -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
@@ -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
PATCH v2 6/8 kvm: handle host mode irqs 6
In reply to Nicolai StangeThe 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.hand 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
PATCH v2 2/8 kvm: handle host mode irqs 2
In reply to Nicolai StangeThe 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
@@ -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
PATCH v2 7/8 kvm: handle host mode irqs 7
In reply to Nicolai StangeThe 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
@@ -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
@@ -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
@@ -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
PATCH v2 8/8 kvm: handle host mode irqs 8
In reply to Nicolai StangeFor 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
@@ -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
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
[MODERATED] Re: [PATCH v2 5/8] kvm: handle host mode irqs 5
In reply to Nicolai StangeQuoted 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_l1dPart 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_l1dinto x86' per-cpuirq_cpustat_tas suggested by Peter Zijlstra.Provide the helpers
kvm_set_cpu_l1tf_flush_l1d(),kvm_clear_cpu_l1tf_flush_l1d()andkvm_get_cpu_l1tf_flush_l1d(). Make them trivial resp. non-existent for !CONFIG_KVM_INTEL as appropriate.
Quoted 6 lines
Let
vmx_l1d_flush()handlekvm_cpu_l1tf_flush_l1din the same way as ->l1tf_flush_l1d.Suggested-by Paolo Bonzini <[email protected]>Suggested-by Peter Zijlstra <[email protected]>Signed-off-by Nicolai Stange <[email protected]>
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@@ -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@@ -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 ; }
[MODERATED] Re: [PATCH v2 4/8] kvm: handle host mode irqs 4
In reply to Nicolai StangeQuoted 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
u16An 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.Suggested-by Paolo Bonzini <[email protected]>Signed-off-by Nicolai Stange <[email protected]>--- arch/x86/include/asm/hardirq.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)arch/x86/include/asm/hardirq.h +0
-0
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 */
Peter Zijlstra
maintainer[MODERATED] Re: [PATCH v2 5/8] kvm: handle host mode irqs 5
In reply to Nicolai Stangearch/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.
[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
-0typedef struct { u16 __softirq_pending; +#if IS_ENABLED(CONFIG_KVM_INTEL) + bool kvm_cpu_l1tf_flush_l1d; +#endifNAK on using
_Boolin 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)
[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
Re: [PATCH v2 5/8] kvm: handle host mode irqs 5
In reply to Nicolai StangeQuoted 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
-0typedef struct { u16 __softirq_pending; +#if IS_ENABLED(CONFIG_KVM_INTEL) + bool kvm_cpu_l1tf_flush_l1d; +#endifNAK on using
_Boolin 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