| Message ID | 1435931368-27730-1-git-send-email-rkrcmar@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/07/2015 15:49, Radim Kr?má? wrote: > fpu_activate is called outside of vcpu_load(), which means it should not > touch VMCS, but fpu_activate needs to. Avoid the call by moving it to a > point where we know that the guest needs eager FPU and VMCS is loaded. > > This will get rid of the following trace > > vmwrite error: reg 6800 value 0 (err 1) > [<ffffffff8162035b>] dump_stack+0x19/0x1b > [<ffffffffa046c701>] vmwrite_error+0x2c/0x2e [kvm_intel] > [<ffffffffa045f26f>] vmcs_writel+0x1f/0x30 [kvm_intel] > [<ffffffffa04617e5>] vmx_fpu_activate.part.61+0x45/0xb0 [kvm_intel] > [<ffffffffa0461865>] vmx_fpu_activate+0x15/0x20 [kvm_intel] > [<ffffffffa0560b91>] kvm_arch_vcpu_create+0x51/0x70 [kvm] > [<ffffffffa0548011>] kvm_vm_ioctl+0x1c1/0x760 [kvm] > [<ffffffff8118b55a>] ? handle_mm_fault+0x49a/0xec0 > [<ffffffff811e47d5>] do_vfs_ioctl+0x2e5/0x4c0 > [<ffffffff8127abbe>] ? file_has_perm+0xae/0xc0 > [<ffffffff811e4a51>] SyS_ioctl+0xa1/0xc0 > [<ffffffff81630949>] system_call_fastpath+0x16/0x1b > > (Note: we also unconditionally activate FPU in vmx_vcpu_reset(), so the > removed code added nothing.) > > Fixes: c447e76b4cab ("kvm/fpu: Enable eager restore kvm FPU for MPX") > Cc: <stable@vger.kernel.org> > Reported-by: Vlastimil Holer <vlastimil.holer@gmail.com> > Signed-off-by: Radim Kr?má? <rkrcmar@redhat.com> Andrey reported offlist that the bug went away by reverting 1cde293. So the patch would at least need a new commit message. :) I'm putting it on hold. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2015-07-03 17:12+0200, Paolo Bonzini: > On 03/07/2015 15:49, Radim Kr?má? wrote: >> fpu_activate is called outside of vcpu_load(), which means it should not >> touch VMCS, but fpu_activate needs to. Avoid the call by moving it to a >> point where we know that the guest needs eager FPU and VMCS is loaded. >> >> This will get rid of the following trace >> >> vmwrite error: reg 6800 value 0 (err 1) >> [<ffffffff8162035b>] dump_stack+0x19/0x1b >> [<ffffffffa046c701>] vmwrite_error+0x2c/0x2e [kvm_intel] >> [<ffffffffa045f26f>] vmcs_writel+0x1f/0x30 [kvm_intel] >> [<ffffffffa04617e5>] vmx_fpu_activate.part.61+0x45/0xb0 [kvm_intel] >> [<ffffffffa0461865>] vmx_fpu_activate+0x15/0x20 [kvm_intel] >> [<ffffffffa0560b91>] kvm_arch_vcpu_create+0x51/0x70 [kvm] >> [<ffffffffa0548011>] kvm_vm_ioctl+0x1c1/0x760 [kvm] >> [<ffffffff8118b55a>] ? handle_mm_fault+0x49a/0xec0 >> [<ffffffff811e47d5>] do_vfs_ioctl+0x2e5/0x4c0 >> [<ffffffff8127abbe>] ? file_has_perm+0xae/0xc0 >> [<ffffffff811e4a51>] SyS_ioctl+0xa1/0xc0 >> [<ffffffff81630949>] system_call_fastpath+0x16/0x1b >> >> (Note: we also unconditionally activate FPU in vmx_vcpu_reset(), so the >> removed code added nothing.) >> >> Fixes: c447e76b4cab ("kvm/fpu: Enable eager restore kvm FPU for MPX") >> Cc: <stable@vger.kernel.org> >> Reported-by: Vlastimil Holer <vlastimil.holer@gmail.com> >> Signed-off-by: Radim Kr?má? <rkrcmar@redhat.com> > > Andrey reported offlist that the bug went away by reverting 1cde293. So > the patch would at least need a new commit message. :) > > I'm putting it on hold. I think it's a different bug than the one Andrey reproduced (https://bugzilla.kernel.org/show_bug.cgi?id=100671). I'll send a v2 that cleans up the code and makes the commit message clearer, unless you find the reasoning below unsound. This bug is specific to 'kvm_arch_vcpu_create()' and Vlastimil Holer hit it on RHEL 7.2 (278.el7) kernel that didn't have 1cde2930e154 ("sched/preempt: Add static_key() to preempt_notifiers"). The commit message does not base on tracing (I haven't reproduced it), but I couldn't make sense out of this bug otherwise. I think it happens just because other VCPU preempted the new one between vmx_vcpu_put()+put_cpu() and the end of kvm_x86_ops->fpu_activate(), so vmwrite accessed different VMCS. The code in kvm_vm_ioctl_create_vcpu() that made me think so: vcpu = kvm_arch_vcpu_create(id) { vcpu = kvm_x86_ops->vcpu_create(kvm, id) { vmx = kmem_cache_zalloc(kvm_vcpu_cache, GFP_KERNEL); kvm_vcpu_init(&vmx->vcpu, kvm, id); vmx->loaded_vmcs = &vmx->vmcs01; vmx->loaded_vmcs->vmcs = alloc_vmcs(); loaded_vmcs_init(vmx->loaded_vmcs); // disabling preemption and activating VMCS cpu = get_cpu(); vmx_vcpu_load(&vmx->vcpu, cpu); vmx_vcpu_setup(vmx); // abandoning VMCS and enabling preemption vmx_vcpu_put(&vmx->vcpu); put_cpu(); return &vmx->vcpu; } // enabled preemption and undefined current VMCS kvm_x86_ops->fpu_activate(vcpu); return vcpu; } preempt_notifier_init(&vcpu->preempt_notifier, &kvm_preempt_ops); kvm_arch_vcpu_setup(vcpu) { vcpu_load(vcpu); ... } -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/07/2015 15:50, Radim Kr?má? wrote: >> Andrey reported offlist that the bug went away by reverting 1cde293. So >> the patch would at least need a new commit message. :) > > I think it's a different bug than the one Andrey reproduced > (https://bugzilla.kernel.org/show_bug.cgi?id=100671). > I'll send a v2 that cleans up the code and makes the commit message > clearer, unless you find the reasoning below unsound. Yes, the patch is okay. The problem is that kvm-arch_vcpu_create is called from a VM ioctl and thus is not between vcpu_load and vcpu_put. Thanks, I applied it. Paolo > This bug is specific to 'kvm_arch_vcpu_create()' and Vlastimil Holer hit > it on RHEL 7.2 (278.el7) kernel that didn't have 1cde2930e154 > ("sched/preempt: Add static_key() to preempt_notifiers"). > > The commit message does not base on tracing (I haven't reproduced it), > but I couldn't make sense out of this bug otherwise. > I think it happens just because other VCPU preempted the new one between > vmx_vcpu_put()+put_cpu() and the end of kvm_x86_ops->fpu_activate(), so > vmwrite accessed different VMCS. The code in kvm_vm_ioctl_create_vcpu() > that made me think so: > > vcpu = kvm_arch_vcpu_create(id) { > vcpu = kvm_x86_ops->vcpu_create(kvm, id) { > vmx = kmem_cache_zalloc(kvm_vcpu_cache, GFP_KERNEL); > kvm_vcpu_init(&vmx->vcpu, kvm, id); > vmx->loaded_vmcs = &vmx->vmcs01; > vmx->loaded_vmcs->vmcs = alloc_vmcs(); > loaded_vmcs_init(vmx->loaded_vmcs); > > // disabling preemption and activating VMCS > cpu = get_cpu(); > vmx_vcpu_load(&vmx->vcpu, cpu); > > vmx_vcpu_setup(vmx); > > // abandoning VMCS and enabling preemption > vmx_vcpu_put(&vmx->vcpu); > put_cpu(); > > return &vmx->vcpu; > } > > // enabled preemption and undefined current VMCS > kvm_x86_ops->fpu_activate(vcpu); > return vcpu; > } > > preempt_notifier_init(&vcpu->preempt_notifier, &kvm_preempt_ops); > kvm_arch_vcpu_setup(vcpu) { > vcpu_load(vcpu); > ... > } > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index 64dd46793099..2fbea2544f24 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -98,6 +98,8 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu) best->ebx = xstate_required_size(vcpu->arch.xcr0, true); vcpu->arch.eager_fpu = use_eager_fpu() || guest_cpuid_has_mpx(vcpu); + if (vcpu->arch.eager_fpu) + kvm_x86_ops->fpu_activate(vcpu); /* * The existing code assumes virtual address is 48-bit in the canonical diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index bbaf44e8f0d3..6bd19c7abc65 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -7315,11 +7315,6 @@ struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, vcpu = kvm_x86_ops->vcpu_create(kvm, id); - /* - * Activate fpu unconditionally in case the guest needs eager FPU. It will be - * deactivated soon if it doesn't. - */ - kvm_x86_ops->fpu_activate(vcpu); return vcpu; }
fpu_activate is called outside of vcpu_load(), which means it should not touch VMCS, but fpu_activate needs to. Avoid the call by moving it to a point where we know that the guest needs eager FPU and VMCS is loaded. This will get rid of the following trace vmwrite error: reg 6800 value 0 (err 1) [<ffffffff8162035b>] dump_stack+0x19/0x1b [<ffffffffa046c701>] vmwrite_error+0x2c/0x2e [kvm_intel] [<ffffffffa045f26f>] vmcs_writel+0x1f/0x30 [kvm_intel] [<ffffffffa04617e5>] vmx_fpu_activate.part.61+0x45/0xb0 [kvm_intel] [<ffffffffa0461865>] vmx_fpu_activate+0x15/0x20 [kvm_intel] [<ffffffffa0560b91>] kvm_arch_vcpu_create+0x51/0x70 [kvm] [<ffffffffa0548011>] kvm_vm_ioctl+0x1c1/0x760 [kvm] [<ffffffff8118b55a>] ? handle_mm_fault+0x49a/0xec0 [<ffffffff811e47d5>] do_vfs_ioctl+0x2e5/0x4c0 [<ffffffff8127abbe>] ? file_has_perm+0xae/0xc0 [<ffffffff811e4a51>] SyS_ioctl+0xa1/0xc0 [<ffffffff81630949>] system_call_fastpath+0x16/0x1b (Note: we also unconditionally activate FPU in vmx_vcpu_reset(), so the removed code added nothing.) Fixes: c447e76b4cab ("kvm/fpu: Enable eager restore kvm FPU for MPX") Cc: <stable@vger.kernel.org> Reported-by: Vlastimil Holer <vlastimil.holer@gmail.com> Signed-off-by: Radim Kr?má? <rkrcmar@redhat.com> --- I also thought about adding a sanity check when we enable eager FPU WARN_ON(kvm_check_request(KVM_REQ_DEACTIVATE_FPU, vcpu)); but it would be poinless now for two reasons * REQ_DEACTIVATE_FPU can't be set before vcpu_run loads FPU and CPUID shouldn't change after vcpu_run (userspace has bigger problems then) * guest MPX requires host support and those hosts should have eager FPU Let's be wild and employed! arch/x86/kvm/cpuid.c | 2 ++ arch/x86/kvm/x86.c | 5 ----- 2 files changed, 2 insertions(+), 5 deletions(-)