Re: [PATCH 3.4.y] x86, kvm: fix kvm's usage of kernel_fpu_begin/end()

[Rui Xiang <rui.xiang@huawei.com>]

ping...

On 2015/7/29 19:03, Rui Xiang wrote:
> From: Suresh Siddha <suresh.b.siddha@intel.com>
> 
> commit b1a74bf8212367be2b1d6685c11a84e056eaaaf1 upstream.
> 
> Preemption is disabled between kernel_fpu_begin/end() and as such
> it is not a good idea to use these routines in kvm_load/put_guest_fpu()
> which can be very far apart.
> 
> kvm_load/put_guest_fpu() routines are already called with
> preemption disabled and KVM already uses the preempt notifier to save
> the guest fpu state using kvm_put_guest_fpu().
> 
> So introduce __kernel_fpu_begin/end() routines which don't touch
> preemption and use them instead of kernel_fpu_begin/end()
> for KVM's use model of saving/restoring guest FPU state.
> 
> Also with this change (and with eagerFPU model), fix the host cr0.TS vm-exit
> state in the case of VMX. For eagerFPU case, host cr0.TS is always clear.
> So no need to worry about it. For the traditional lazyFPU restore case,
> change the cr0.TS bit for the host state during vm-exit to be always clear
> and cr0.TS bit is set in the __vmx_load_host_state() when the FPU
> (guest FPU or the host task's FPU) state is not active. This ensures
> that the host/guest FPU state is properly saved, restored
> during context-switch and with interrupts (using irq_fpu_usable()) not
> stomping on the active FPU state.
> 
> Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
> Link: http://lkml.kernel.org/r/1348164109.26695.338.camel@sbsiddha-desk.sc.intel.com
> Cc: Avi Kivity <avi@redhat.com>
> Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>
> [xr: Backported to 3.4: Adjust context]
> Signed-off-by: Rui Xiang <rui.xiang@huawei.com>
> ---
>  arch/x86/include/asm/i387.h | 28 ++++++++++++++++++++++++++--
>  arch/x86/kernel/i387.c      | 13 +++++--------
>  arch/x86/kvm/vmx.c          | 10 +++++++---
>  arch/x86/kvm/x86.c          |  4 ++--
>  4 files changed, 40 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/x86/include/asm/i387.h b/arch/x86/include/asm/i387.h
> index 257d9cc..1262fb6 100644
> --- a/arch/x86/include/asm/i387.h
> +++ b/arch/x86/include/asm/i387.h
> @@ -23,8 +23,32 @@ extern int dump_fpu(struct pt_regs *, struct user_i387_struct *);
>  extern void math_state_restore(void);
>  
>  extern bool irq_fpu_usable(void);
> -extern void kernel_fpu_begin(void);
> -extern void kernel_fpu_end(void);
> +
> +/*
> + * Careful: __kernel_fpu_begin/end() must be called with preempt disabled
> + * and they don't touch the preempt state on their own.
> + * If you enable preemption after __kernel_fpu_begin(), preempt notifier
> + * should call the __kernel_fpu_end() to prevent the kernel/user FPU
> + * state from getting corrupted. KVM for example uses this model.
> + *
> + * All other cases use kernel_fpu_begin/end() which disable preemption
> + * during kernel FPU usage.
> + */
> +extern void __kernel_fpu_begin(void);
> +extern void __kernel_fpu_end(void);
> +
> +static inline void kernel_fpu_begin(void)
> +{
> +	WARN_ON_ONCE(!irq_fpu_usable());
> +	preempt_disable();
> +	__kernel_fpu_begin();
> +}
> +
> +static inline void kernel_fpu_end(void)
> +{
> +	__kernel_fpu_end();
> +	preempt_enable();
> +}
>  
>  /*
>   * Some instructions like VIA's padlock instructions generate a spurious
> diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
> index 6610e81..7aa728d 100644
> --- a/arch/x86/kernel/i387.c
> +++ b/arch/x86/kernel/i387.c
> @@ -77,29 +77,26 @@ bool irq_fpu_usable(void)
>  }
>  EXPORT_SYMBOL(irq_fpu_usable);
>  
> -void kernel_fpu_begin(void)
> +void __kernel_fpu_begin(void)
>  {
>  	struct task_struct *me = current;
>  
> -	WARN_ON_ONCE(!irq_fpu_usable());
> -	preempt_disable();
>  	if (__thread_has_fpu(me)) {
>  		__save_init_fpu(me);
>  		__thread_clear_has_fpu(me);
> -		/* We do 'stts()' in kernel_fpu_end() */
> +		/* We do 'stts()' in __kernel_fpu_end() */
>  	} else {
>  		percpu_write(fpu_owner_task, NULL);
>  		clts();
>  	}
>  }
> -EXPORT_SYMBOL(kernel_fpu_begin);
> +EXPORT_SYMBOL(__kernel_fpu_begin);
>  
> -void kernel_fpu_end(void)
> +void __kernel_fpu_end(void)
>  {
>  	stts();
> -	preempt_enable();
>  }
> -EXPORT_SYMBOL(kernel_fpu_end);
> +EXPORT_SYMBOL(__kernel_fpu_end);
>  
>  void unlazy_fpu(struct task_struct *tsk)
>  {
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 2eb4e5a..4ad0d71 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -1455,8 +1455,12 @@ static void __vmx_load_host_state(struct vcpu_vmx *vmx)
>  #ifdef CONFIG_X86_64
>  	wrmsrl(MSR_KERNEL_GS_BASE, vmx->msr_host_kernel_gs_base);
>  #endif
> -	if (user_has_fpu())
> -		clts();
> +	/*
> +	 * If the FPU is not active (through the host task or
> +	 * the guest vcpu), then restore the cr0.TS bit.
> +	 */
> +	if (!user_has_fpu() && !vmx->vcpu.guest_fpu_loaded)
> +		stts();
>  	load_gdt(&__get_cpu_var(host_gdt));
>  }
>  
> @@ -3633,7 +3637,7 @@ static void vmx_set_constant_host_state(struct vcpu_vmx *vmx)
>  	struct desc_ptr dt;
>  	unsigned long cr4;
>  
> -	vmcs_writel(HOST_CR0, read_cr0() | X86_CR0_TS);  /* 22.2.3 */
> +	vmcs_writel(HOST_CR0, read_cr0() & ~X86_CR0_TS);  /* 22.2.3 */
>  	vmcs_writel(HOST_CR3, read_cr3());  /* 22.2.3  FIXME: shadow tables */
>  
>  	/* Save the most likely value for this task's CR4 in the VMCS. */
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 55ee4ca..4ad2b7b 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5907,7 +5907,7 @@ void kvm_load_guest_fpu(struct kvm_vcpu *vcpu)
>  	 */
>  	kvm_put_guest_xcr0(vcpu);
>  	vcpu->guest_fpu_loaded = 1;
> -	kernel_fpu_begin();
> +	__kernel_fpu_begin();
>  	fpu_restore_checking(&vcpu->arch.guest_fpu);
>  	trace_kvm_fpu(1);
>  }
> @@ -5921,7 +5921,7 @@ void kvm_put_guest_fpu(struct kvm_vcpu *vcpu)
>  
>  	vcpu->guest_fpu_loaded = 0;
>  	fpu_save_init(&vcpu->arch.guest_fpu);
> -	kernel_fpu_end();
> +	__kernel_fpu_end();
>  	++vcpu->stat.fpu_reload;
>  	kvm_make_request(KVM_REQ_DEACTIVATE_FPU, vcpu);
>  	trace_kvm_fpu(0);
>