[PATCH v2 1/3] 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()

[Xiao Guangrong]

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]

Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
---
 arch/x86/kvm/x86.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index fa708c9..0b77e9c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -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);
-- 
1.7.3.4

[PATCH v2 2/3] KVM: send IPI to vcpu only when it's in guest mode

[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 incorrect

Also reorganize struct kvm_vcpu to make ->guest_mode and ->requests
in the same cache line explicitly.

Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
---
 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(-)

diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
index 70d224d..15c11b2 100644
--- a/arch/ia64/kvm/kvm-ia64.c
+++ b/arch/ia64/kvm/kvm-ia64.c
@@ -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);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 0b77e9c..f882158 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -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();
 
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index b5021db..5f5ef74 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -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;
diff --git a/virt/kvm/kvm_main.c b/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 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))
-- 
1.7.3.4

Re: [PATCH v2 2/3] KVM: send IPI to vcpu only when it's in guest mode

[Marcelo Tosatti]

On Fri, Jan 07, 2011 at 03:47:51PM +0800, Xiao Guangrong wrote:
> 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.
> 
> Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
> ---
>  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(-)
> 

> diff --git a/virt/kvm/kvm_main.c b/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 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);

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

[Avi Kivity]

On 01/07/2011 01:55 PM, Marcelo Tosatti wrote:
> On Fri, Jan 07, 2011 at 03:47:51PM +0800, Xiao Guangrong wrote:
> >  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.
> >
> >  Signed-off-by: Xiao Guangrong<xiaoguangrong@cn.fujitsu.com>
> >  ---
> >   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(-)
> >
>
> >  diff --git a/virt/kvm/kvm_main.c b/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 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);
>
> 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.
>

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.

-- 
error compiling committee.c: too many arguments to function

[PATCH v2 3/3] KVM: make make_all_cpus_request() lockless

[Xiao Guangrong]

Now, 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

Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
---
 include/linux/kvm_host.h |    6 ------
 virt/kvm/kvm_main.c      |   11 ++++-------
 2 files changed, 4 insertions(+), 13 deletions(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 5f5ef74..72b6dde 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -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)) {
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index a475264..1afcdcd 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -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);
-- 
1.7.3.4