KVM: fix rcu usage warning in kvm_arch_vcpu_ioctl_set_sregs()
PATCH v2 1/3 KVM: fix rcu usage warning in kvm_arch_vcpu_ioctl_set_sregs()
Fix:
[ 1001.499596] =================================================== [ 1001.499599] [ INFO: suspicious rcu_dereference_check() usage. ] [ 1001.499601] --------------------------------------------------- [ 1001.499604] include/linux/kvm_host.h:301 invoked rcu_dereference_check() without protection! ...... [ 1001.499636] Pid: 6035, comm: qemu-system-x86 Not tainted 2.6.37-rc6+ #62 [ 1001.499638] Call Trace: [ 1001.499644] [<ffffffff81076cf6>] lockdep_rcu_dereference+0x9d/0xa5 [ 1001.499653] [<ffffffffa02567a6>] gfn_to_memslot+0x8d/0xc8 [kvm] [ 1001.499661] [<ffffffffa0256824>] gfn_to_hva+0x16/0x3f [kvm] [ 1001.499669] [<ffffffffa02568f5>] kvm_read_guest_page+0x1e/0x5e [kvm] [ 1001.499681] [<ffffffffa026c24a>] kvm_read_guest_page_mmu+0x53/0x5e [kvm] [ 1001.499699] [<ffffffffa026c294>] load_pdptrs+0x3f/0x9c [kvm] [ 1001.499705] [<ffffffffa00d825e>] ? vmx_set_cr0+0x507/0x517 [kvm_intel] [ 1001.499717] [<ffffffffa026ca56>] kvm_arch_vcpu_ioctl_set_sregs+0x1f3/0x3c0 [kvm] [ 1001.499727] [<ffffffffa025a410>] kvm_vcpu_ioctl+0x6a5/0xbc5 [kvm]
--- arch/x86/kvm/x86.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)
arch/x86/kvm/x86.c +4 -1
@@ -5571,7 +5571,7 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs) {int mmu_reset_needed =0 ; -int pending_vec, max_bits; +int pending_vec, max_bits, idx;struct desc_ptr dt; dt.size = sregs->idt.limit;
@@ -5600,10 +5600,13 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu, kvm_x86_ops->set_cr4 (vcpu, sregs->cr4);if (sregs->cr4 &X86_CR4_OSXSAVE )update_cpuid (vcpu); + + idx =srcu_read_lock (&vcpu->kvm->srcu);if (!is_long_mode (vcpu) &&is_pae (vcpu)) {load_pdptrs (vcpu, vcpu->arch.walk_mmu,kvm_read_cr3 (vcpu)); mmu_reset_needed =1 ; } +srcu_read_unlock (&vcpu->kvm->srcu, idx);if (mmu_reset_needed)kvm_mmu_reset_context (vcpu);
Signature · 1 line
1.7.3.4
PATCH v2 2/3 KVM: send IPI to vcpu only when it's in guest mode
We can interrupt the vcpu only when it's running in guest mode to reduce IPI
It looks like only ia64 and x86 need to send IPI to other vcpus, so i only add the implementation of 'vcpu->guest_mode' in ia64, but i don't know ia64 well, please point out the right way for me if the implementation is incorrect
Also reorganize struct kvm_vcpu to make ->guest_mode and ->requests in the same cache line explicitly.
--- arch/ia64/kvm/kvm-ia64.c | 2 ++ arch/x86/kvm/x86.c | 6 +++++- include/linux/kvm_host.h | 9 +++++---- virt/kvm/kvm_main.c | 7 ++++++- 4 files changed, 18 insertions(+), 6 deletions(-)
arch/ia64/kvm/kvm-ia64.c +2 -0
@@ -662,6 +662,7 @@ again: goto vcpu_run_fail;srcu_read_unlock (&vcpu->kvm->srcu, idx); +atomic_set (&vcpu->guest_mode,1 );kvm_guest_enter ();/*
@@ -683,6 +684,7 @@ again:*/ barrier ();kvm_guest_exit (); +atomic_set (&vcpu->guest_mode,0 );preempt_enable (); idx =srcu_read_lock (&vcpu->kvm->srcu);
arch/x86/kvm/x86.c +5 -1
@@ -5214,7 +5214,11 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)kvm_load_guest_xcr0 (vcpu);atomic_set (&vcpu->guest_mode,1 ); -smp_wmb (); + +/* We should set ->guest_mode before check ->requests, +* see the comment in make_all_cpus_request. +*/ +smp_mb ();local_irq_disable ();
include/linux/kvm_host.h +5 -4
@@ -103,14 +103,15 @@ struct kvm_vcpu {#ifdef CONFIG_PREEMPT_NOTIFIERS struct preempt_notifier preempt_notifier;#endif +int cpu;int vcpu_id; -struct mutex mutex; -int cpu; +int srcu_idx;atomic_t guest_mode; -struct kvm_run *run;unsigned long requests;unsigned long guest_debug; -int srcu_idx; + +struct mutex mutex; +struct kvm_run *run;int fpu_active;int guest_fpu_loaded, guest_xcr0_loaded;
virt/kvm/kvm_main.c +6 -1
@@ -153,7 +153,12 @@ static bool make_all_cpus_request(struct kvm *kvm, unsigned int req)if (kvm_make_check_request (req, vcpu))continue ; cpu = vcpu->cpu; -if (cpus !=NULL && cpu !=-1 && cpu != me) + +/* Set ->requests bit before we read ->guest_mode */ +smp_mb (); + +if (cpus !=NULL && cpu !=-1 && cpu != me && +atomic_read (&vcpu->guest_mode))cpumask_set_cpu (cpu, cpus); }if (unlikely (cpus ==NULL ))
Signature · 1 line
1.7.3.4
Re: [PATCH v2 2/3] KVM: send IPI to vcpu only when it's in guest mode
Quoted 26 lines · Xiao Guangrong
We can interrupt the vcpu only when it's running in guest mode to reduce IPI
It looks like only ia64 and x86 need to send IPI to other vcpus, so i only add the implementation of '
vcpu->guest_mode' in ia64, but i don't know ia64 well, please point out the right way for me if the implementation is incorrectAlso reorganize struct
kvm_vcputo make ->guest_mode and ->requests in the same cache line explicitly.Signed-off-by Xiao Guangrong <[email protected]>--- arch/ia64/kvm/kvm-ia64.c | 2 ++ arch/x86/kvm/x86.c | 6 +++++- include/linux/kvm_host.h | 9 +++++---- virt/kvm/kvm_main.c | 7 ++++++- 4 files changed, 18 insertions(+), 6 deletions(-)virt/kvm/kvm_main.c +0
-0@@ -153,7 +153,12 @@ static bool make_all_cpus_request(struct kvm *kvm, unsigned int req)if (kvm_make_check_request (req, vcpu))
virt/kvm/kvm_main.c static bool make_all_cpus_request(struct kvm *kvm, unsigned int req) · line 162
Quoted 10 lines
virt/kvm/kvm_main.c +6
-1continue ; cpu = vcpu->cpu; -if (cpus !=NULL && cpu !=-1 && cpu != me) + +/* Set ->requests bit before we read ->guest_mode */ +smp_mb (); + +if (cpus !=NULL && cpu !=-1 && cpu != me && +atomic_read (&vcpu->guest_mode))cpumask_set_cpu (cpu, cpus);
Don't think this is safe, since guest_mode does not imply that a vcpu has received the IPI, only that IPI has been sent (see kvm_vcpu_kick).
And make_all_cpus_request must guarantee all target vcpus are out of guest mode before it returns.
Re: [PATCH v2 2/3] KVM: send IPI to vcpu only when it's in guest mode
Quoted 38 lines · Marcelo Tosatti
Quoted 20 lines · Xiao Guangrong
We can interrupt the vcpu only when it's running in guest mode to reduce IPI
It looks like only ia64 and x86 need to send IPI to other vcpus, so i only add the implementation of '
vcpu->guest_mode' in ia64, but i don't know ia64 well, please point out the right way for me if the implementation is incorrectAlso reorganize struct
kvm_vcputo make ->guest_mode and ->requests in the same cache line explicitly.Signed-off-by Xiao Guangrong<[email protected]>---
arch/ia64/kvm/kvm-ia64.c | 2 ++ arch/x86/kvm/x86.c | 6 +++++- include/linux/kvm_host.h | 9 +++++---- virt/kvm/kvm_main.c | 7 ++++++- 4 files changed, 18 insertions(+), 6 deletions(-)Quoted 16 lines
diff --git
a/virt/kvm/kvm_main.cb/virt/kvm/kvm_main.c
index b1b6cbb..a475264 100644
---a/virt/kvm/kvm_main.c
+++b/virt/kvm/kvm_main.c
@@ -153,7 +153,12 @@ static boolmake_all_cpus_request(struct kvm *kvm, unsigned int req)
if (kvm_make_check_request(req, vcpu))
continue;
cpu =vcpu->cpu;
- if (cpus != NULL&& cpu != -1&& cpu != me)
+
+ /* Set ->requests bit before we read ->guest_mode */
+smp_mb();
+
+ if (cpus != NULL&& cpu != -1&& cpu != me&&
+atomic_read(&vcpu->guest_mode))
cpumask_set_cpu(cpu, cpus);
Quoted 6 lines
Don't think this is safe, since
guest_modedoes not imply that a vcpu has received the IPI, only that IPI has been sent (seekvm_vcpu_kick).And
make_all_cpus_requestmust guarantee all target vcpus are out of guest mode before it returns.
Good catch. We could remove the atomic_xchg() in kvm_vcpu_kick(), but need to think of any drawbacks to that.
Maybe we could replace guest_mode with a three value variable: in guest, outside guest, and transitioning out of guest. So kvm_vcpu_kick() avoids the IPI on the last two, while make_all_cpus_request() only avoids an IPI if outside guest mode.
Signature · 1 line
error compiling committee.c: too many arguments to function
PATCH v2 3/3 KVM: make make_all_cpus_request() lockless
In reply to Xiao GuangrongNow, we have 'vcpu->guest_mode' to judge whether need to send ipi to other cpus, this way is very exact, so checking request bit is needless, then we can drop the spinlock let it's collateral
--- include/linux/kvm_host.h | 6 ------ virt/kvm/kvm_main.c | 11 ++++------- 2 files changed, 4 insertions(+), 13 deletions(-)
include/linux/kvm_host.h +0 -6
@@ -213,7 +213,6 @@ struct kvm_memslots {struct kvm {spinlock_t mmu_lock; -raw_spinlock_t requests_lock;struct mutex slots_lock;struct mm_struct *mm;/* userspace tied to this vm */ struct kvm_memslots *memslots;
@@ -720,11 +719,6 @@ static inline void kvm_make_request(int req, struct kvm_vcpu *vcpu)set_bit (req, &vcpu->requests); } -static inline bool kvm_make_check_request (int req,struct kvm_vcpu *vcpu) -{ -return test_and_set_bit (req, &vcpu->requests); -} -static inline bool kvm_check_request (int req,struct kvm_vcpu *vcpu) {if (test_bit (req, &vcpu->requests)) {
virt/kvm/kvm_main.c +4 -7
@@ -147,16 +147,14 @@ static bool make_all_cpus_request(struct kvm *kvm, unsigned int req)zalloc_cpumask_var (&cpus,GFP_ATOMIC ); -raw_spin_lock (&kvm->requests_lock); - me =smp_processor_id (); + me =get_cpu ();kvm_for_each_vcpu (i, vcpu, kvm) { -if (kvm_make_check_request (req, vcpu)) -continue ; - cpu = vcpu->cpu; +kvm_make_request (req, vcpu);/* Set ->requests bit before we read ->guest_mode */ smp_mb (); + cpu = vcpu->cpu;if (cpus !=NULL && cpu !=-1 && cpu != me &&atomic_read (&vcpu->guest_mode))cpumask_set_cpu (cpu, cpus);
@@ -167,7 +165,7 @@ static bool make_all_cpus_request(struct kvm *kvm, unsigned int req)smp_call_function_many (cpus, ack_flush,NULL ,1 ); else called = false; -raw_spin_unlock (&kvm->requests_lock); +put_cpu ();free_cpumask_var (cpus);return called; }
@@ -433,7 +431,6 @@ static struct kvm *kvm_create_vm(void) kvm->mm = current->mm;atomic_inc (&kvm->mm->mm_count);spin_lock_init (&kvm->mmu_lock); -raw_spin_lock_init (&kvm->requests_lock);kvm_eventfd_init (kvm);mutex_init (&kvm->lock);mutex_init (&kvm->irq_lock);
Signature · 1 line
1.7.3.4