On 02.07.18 17:44, speck for Thomas Gleixner wrote:
> Subject: [patch V5 05/10] x86/KVM/VMX: Add L1D flush logic
> From: Paolo Bonzini pbonzini@redhat.com
> 
> Add the logic for flushing L1D on VMENTER. The flush depends on the static
> key being enabled and the new l1tf_flush_l1d flag being set.
> 
> The flags is set:
>  - Always, if the flush module parameter is 'always'
> 
>  - Conditionally at:
>    - Entry to vcpu_run(), i.e. after executing user space
> 
>    - From the sched_in notifier, i.e. when switching to a vCPU thread.
> 
>    - From vmexit handlers which are considered unsafe, i.e. where
>      sensitive data can be brought into L1D:
> 
>      - The emulator, which could be a good target for other speculative
>        execution-based threats,
> 
>      - The MMU, which can bring host page tables in the L1 cache.
>      
>      - External interrupts
> 
>      - Nested operations that require the MMU (see above). That is
>        vmptrld, vmptrst, vmclear,vmwrite,vmread.
> 
>      - When handling invept,invvpid
> 
> [ tglx: Split out from combo patch and reduced to a single flag ]
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
> v4: Fix from Thomas using static key
> v5: Split it out from the big combo patch
>     Simplify the flags logic
> 
>  arch/x86/include/asm/kvm_host.h |    4 ++++
>  arch/x86/kvm/mmu.c              |    1 +
>  arch/x86/kvm/vmx.c              |   22 +++++++++++++++++++++-
>  arch/x86/kvm/x86.c              |    7 +++++++
>  4 files changed, 33 insertions(+), 1 deletion(-)
> 
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -713,6 +713,9 @@ struct kvm_vcpu_arch {
>  
>  	/* be preempted when it's in kernel-mode(cpl=0) */
>  	bool preempted_in_kernel;
> +
> +	/* Flush the L1 Data cache for L1TF mitigation on VMENTER */
> +	bool l1tf_flush_l1d;
>  };
>  
>  struct kvm_lpage_info {
> @@ -881,6 +884,7 @@ struct kvm_vcpu_stat {
>  	u64 signal_exits;
>  	u64 irq_window_exits;
>  	u64 nmi_window_exits;
> +	u64 l1d_flush;
>  	u64 halt_exits;
>  	u64 halt_successful_poll;
>  	u64 halt_attempted_poll;
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -3840,6 +3840,7 @@ int kvm_handle_page_fault(struct kvm_vcp
>  {
>  	int r = 1;
>  
> +	vcpu->arch.l1tf_flush_l1d = true;
>  	switch (vcpu->arch.apf.host_apf_reason) {
>  	default:
>  		trace_kvm_page_fault(fault_address, error_code);
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -9576,9 +9576,20 @@ static int vmx_handle_exit(struct kvm_vc
>  #define L1D_CACHE_ORDER 4
>  static void *vmx_l1d_flush_pages;
>  
> -static void __maybe_unused vmx_l1d_flush(void)
> +static void vmx_l1d_flush(struct kvm_vcpu *vcpu)
>  {
>  	int size = PAGE_SIZE << L1D_CACHE_ORDER;
> +	bool always;
> +
> +	/*
> +	 * If the mitigation mode is 'flush always', keep the flush bit
> +	 * set, otherwise clear it. It gets set again either from
> +	 * vcpu_run() or from one of the unsafe VMEXIT handlers.
> +	 */
> +	always = vmentry_l1d_flush == VMENTER_L1D_FLUSH_ALWAYS;
> +	vcpu->arch.l1tf_flush_l1d = always;
> +
> +	vcpu->stat.l1d_flush++;
>  
>  	if (static_cpu_has(X86_FEATURE_FLUSH_L1D)) {
>  		wrmsrl(MSR_IA32_FLUSH_CMD, L1D_FLUSH);
> @@ -9847,6 +9858,7 @@ static void vmx_handle_external_intr(str
>  			[ss]"i"(__KERNEL_DS),
>  			[cs]"i"(__KERNEL_CS)
>  			);
> +		vcpu->arch.l1tf_flush_l1d = true;

This means we flush when an external interrupt is the trap reason, but
we may as well get the interrupt only after interrupts are enabled again
from vcpu_enter_guest(), no? How is an interrupt executing "normally"
any different from an interrupt that causes a #vmexit?

Or in other words: Is it worth it to have anything but always/never as
an option? Has anyone done benchmarks?


Alex