* [request for stable inclusion][Patch 3.4.y] x86, kvm: use kernel_fpu_begin/end() in kvm_load/put_guest_fpu()
@ 2015-07-29 11:03 Rui Xiang
2015-07-29 11:03 ` [PATCH 3.4.y] x86, kvm: fix kvm's usage of kernel_fpu_begin/end() Rui Xiang
2015-07-31 3:03 ` [request for stable inclusion][Patch 3.4.y] x86, kvm: use kernel_fpu_begin/end() in kvm_load/put_guest_fpu() Rui Xiang
0 siblings, 2 replies; 6+ messages in thread
From: Rui Xiang @ 2015-07-29 11:03 UTC (permalink / raw)
To: Zefan Li, H. Peter Anvin, Suresh Siddha, Suresh Siddha
Cc: stable, huangzhichao, Rui Xiang
Hi Zefan, Suresh or Peter,
9c1c3fac53378c9782c18f80107965578d7b7167
x86, kvm: use kernel_fpu_begin/end() in kvm_load/put_guest_fpu()
We never cause the "Device Not Available" exception in kernel mode,
but without this patch, the orign unlazy_fpu() do stts(), the following
fpu_restore_checking() will trigger a DNA(Device Not Available), this
may corrupt the user process fpu. And an crash happened through a fault
injection in our scenario.
b1a74bf8212367be2b1d6685c11a84e056eaaaf1
x86, kvm: fix kvm's usage of kernel_fpu_begin/end()
The commit b1a74bf82123 fix a bug introduced by comit 9c1c3fac5337
(x86, kvm: use kernel_fpu_begin/end() in kvm_load/put_guest_fpu())
They look applicable to stable-3.4.y. The first one can be applied
cleanly, the other need some adjustments to backport.
The following patche is the backport for b1a74bf82123.
And what do you think?
Thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 3.4.y] x86, kvm: fix kvm's usage of kernel_fpu_begin/end()
2015-07-29 11:03 [request for stable inclusion][Patch 3.4.y] x86, kvm: use kernel_fpu_begin/end() in kvm_load/put_guest_fpu() Rui Xiang
@ 2015-07-29 11:03 ` Rui Xiang
2015-07-31 3:03 ` Rui Xiang
2015-07-31 3:03 ` [request for stable inclusion][Patch 3.4.y] x86, kvm: use kernel_fpu_begin/end() in kvm_load/put_guest_fpu() Rui Xiang
1 sibling, 1 reply; 6+ messages in thread
From: Rui Xiang @ 2015-07-29 11:03 UTC (permalink / raw)
To: Zefan Li, H. Peter Anvin, Suresh Siddha, Suresh Siddha
Cc: stable, huangzhichao, Avi Kivity, Rui Xiang
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);
--
1.8.2.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 3.4.y] x86, kvm: fix kvm's usage of kernel_fpu_begin/end()
2015-07-29 11:03 ` [PATCH 3.4.y] x86, kvm: fix kvm's usage of kernel_fpu_begin/end() Rui Xiang
@ 2015-07-31 3:03 ` Rui Xiang
0 siblings, 0 replies; 6+ messages in thread
From: Rui Xiang @ 2015-07-31 3:03 UTC (permalink / raw)
To: Zefan Li, H. Peter Anvin, Suresh Siddha, Suresh Siddha,
Paolo Bonzini, kvm
Cc: stable, huangzhichao
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);
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [request for stable inclusion][Patch 3.4.y] x86, kvm: use kernel_fpu_begin/end() in kvm_load/put_guest_fpu()
2015-07-29 11:03 [request for stable inclusion][Patch 3.4.y] x86, kvm: use kernel_fpu_begin/end() in kvm_load/put_guest_fpu() Rui Xiang
2015-07-29 11:03 ` [PATCH 3.4.y] x86, kvm: fix kvm's usage of kernel_fpu_begin/end() Rui Xiang
@ 2015-07-31 3:03 ` Rui Xiang
2015-07-31 3:52 ` Greg KH
1 sibling, 1 reply; 6+ messages in thread
From: Rui Xiang @ 2015-07-31 3:03 UTC (permalink / raw)
To: Zefan Li, H. Peter Anvin, Suresh Siddha, Suresh Siddha,
Paolo Bonzini, kvm
Cc: stable, huangzhichao
ping....
On 2015/7/29 19:03, Rui Xiang wrote:
> Hi Zefan, Suresh or Peter,
>
> 9c1c3fac53378c9782c18f80107965578d7b7167
> x86, kvm: use kernel_fpu_begin/end() in kvm_load/put_guest_fpu()
>
> We never cause the "Device Not Available" exception in kernel mode,
> but without this patch, the orign unlazy_fpu() do stts(), the following
> fpu_restore_checking() will trigger a DNA(Device Not Available), this
> may corrupt the user process fpu. And an crash happened through a fault
> injection in our scenario.
>
> b1a74bf8212367be2b1d6685c11a84e056eaaaf1
> x86, kvm: fix kvm's usage of kernel_fpu_begin/end()
>
> The commit b1a74bf82123 fix a bug introduced by comit 9c1c3fac5337
> (x86, kvm: use kernel_fpu_begin/end() in kvm_load/put_guest_fpu())
>
> They look applicable to stable-3.4.y. The first one can be applied
> cleanly, the other need some adjustments to backport.
> The following patche is the backport for b1a74bf82123.
>
> And what do you think?
>
> Thanks.
>
>
>
> .
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [request for stable inclusion][Patch 3.4.y] x86, kvm: use kernel_fpu_begin/end() in kvm_load/put_guest_fpu()
2015-07-31 3:03 ` [request for stable inclusion][Patch 3.4.y] x86, kvm: use kernel_fpu_begin/end() in kvm_load/put_guest_fpu() Rui Xiang
@ 2015-07-31 3:52 ` Greg KH
0 siblings, 0 replies; 6+ messages in thread
From: Greg KH @ 2015-07-31 3:52 UTC (permalink / raw)
To: Rui Xiang
Cc: Zefan Li, H. Peter Anvin, Suresh Siddha, Suresh Siddha,
Paolo Bonzini, kvm, stable, huangzhichao
On Fri, Jul 31, 2015 at 11:03:48AM +0800, Rui Xiang wrote:
> ping....
>
> On 2015/7/29 19:03, Rui Xiang wrote:
2 days ago? For a very old and slowly updated kernel release? Hah, be
happy if you get a response in a month...
^ permalink raw reply [flat|nested] 6+ messages in thread
* [request for stable inclusion][Patch 3.4.y] x86, kvm: use kernel_fpu_begin/end() in kvm_load/put_guest_fpu()
@ 2015-07-29 10:59 Rui Xiang
0 siblings, 0 replies; 6+ messages in thread
From: Rui Xiang @ 2015-07-29 10:59 UTC (permalink / raw)
To: Zefan Li, H. Peter Anvin, Suresh Siddha; +Cc: stable, huangzhichao, Rui Xiang
9c1c3fac53378c9782c18f80107965578d7b7167
x86, kvm: use kernel_fpu_begin/end() in kvm_load/put_guest_fpu()
We never cause the "Device Not Available" exception in kernel mode,
but without this patch, the orign unlazy_fpu() do stts(), the following
fpu_restore_checking() will trigger a DNA(Device Not Available), this
may corrupt the user process fpu. And an crash happened through a fault
injection in our scenario.
b1a74bf8212367be2b1d6685c11a84e056eaaaf1
x86, kvm: fix kvm's usage of kernel_fpu_begin/end()
The commit b1a74bf82123 fix a bug introduced by comit 9c1c3fac5337
(x86, kvm: use kernel_fpu_begin/end() in kvm_load/put_guest_fpu())
They look applicable to stable-3.4.y. The first one can be applied
cleanly, the other need some adjustments to backport.
The following patche is the backport for b1a74bf82123.
And what do you think?
Thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-07-31 3:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-07-29 11:03 [request for stable inclusion][Patch 3.4.y] x86, kvm: use kernel_fpu_begin/end() in kvm_load/put_guest_fpu() Rui Xiang
2015-07-29 11:03 ` [PATCH 3.4.y] x86, kvm: fix kvm's usage of kernel_fpu_begin/end() Rui Xiang
2015-07-31 3:03 ` Rui Xiang
2015-07-31 3:03 ` [request for stable inclusion][Patch 3.4.y] x86, kvm: use kernel_fpu_begin/end() in kvm_load/put_guest_fpu() Rui Xiang
2015-07-31 3:52 ` Greg KH
-- strict thread matches above, loose matches on Subject: below --
2015-07-29 10:59 Rui Xiang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).