[RFC 1/8]KVM: fix bugs in kvm sched integration patch

[RFC 1/8]KVM: fix bugs in kvm sched integration patch

[Shaohua Li]

fix some bugs in kvm-sch patch.
1. vmcs_readl/vmcs_writel are called with preempt enabled
2. preempt_count check doesn't make sense with preempt disabled
3. vmx_cpu_run doesn't handle error correctly and kvm_mmu_reload might
sleep with mutex changes, so I move it above.

Signed-off-by: Shaohua Li <shaohua.li@intel.com>
---
 drivers/kvm/vmx.c |   12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

Index: linux/drivers/kvm/vmx.c
===================================================================
--- linux.orig/drivers/kvm/vmx.c	2007-07-18 16:11:22.000000000 +0800
+++ linux/drivers/kvm/vmx.c	2007-07-20 14:56:45.000000000 +0800
@@ -179,7 +179,6 @@ static unsigned long vmcs_readl(unsigned
 {
 	unsigned long value;
 
-	WARN_ON(raw_smp_processor_id() != current->kvm_vcpu->cpu);
 	asm volatile (ASM_VMX_VMREAD_RDX_RAX
 		      : "=a"(value) : "d"(field) : "cc");
 	return value;
@@ -215,7 +214,6 @@ static void vmcs_writel(unsigned long fi
 {
 	u8 error;
 
-	WARN_ON(raw_smp_processor_id() != current->kvm_vcpu->cpu);
 	asm volatile (ASM_VMX_VMWRITE_RAX_RDX "; setna %0"
 		       : "=q"(error) : "a"(value), "d"(field) : "cc" );
 	if (unlikely(error))
@@ -384,7 +382,6 @@ static void vmx_vcpu_load(struct kvm_vcp
 	u64 phys_addr = __pa(vcpu->vmcs);
 	u64 tsc_this, delta;
 
-	WARN_ON(!preempt_count());
 	if (vcpu->cpu != cpu)
 		vcpu_clear(vcpu);
 
@@ -427,7 +424,6 @@ static void vmx_vcpu_load(struct kvm_vcp
 
 static void vmx_vcpu_put(struct kvm_vcpu *vcpu)
 {
-	WARN_ON(!preempt_count());
 	vmx_load_host_state(vcpu);
 	kvm_put_guest_fpu(vcpu);
 }
@@ -2001,6 +1997,10 @@ preempted:
 		kvm_guest_debug_pre(vcpu);
 
 again:
+	r = kvm_mmu_reload(vcpu);
+	if (unlikely(r))
+		goto out;
+
 	preempt_disable();
 
 	if (!vcpu->mmio_read_completed)
@@ -2009,10 +2009,6 @@ again:
 	vmx_save_host_state(vcpu);
 	kvm_load_guest_fpu(vcpu);
 
-	r = kvm_mmu_reload(vcpu);
-	if (unlikely(r))
-		goto out;
-
 	/*
 	 * Loading guest fpu may have cleared host cr0.ts
 	 */

[RFC 1/8]KVM: fix bugs in kvm sched integration patch

[Shaohua Li]

fix some bugs in kvm-sch patch.
1. vmcs_readl/vmcs_writel are called with preempt enabled
2. preempt_count check doesn't make sense with preempt disabled
3. vmx_cpu_run doesn't handle error correctly and kvm_mmu_reload might
sleep with mutex changes, so I move it above.

Signed-off-by: Shaohua Li <shaohua.li-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/kvm/vmx.c |   12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

Index: linux/drivers/kvm/vmx.c
===================================================================
--- linux.orig/drivers/kvm/vmx.c	2007-07-18 16:11:22.000000000 +0800
+++ linux/drivers/kvm/vmx.c	2007-07-20 14:56:45.000000000 +0800
@@ -179,7 +179,6 @@ static unsigned long vmcs_readl(unsigned
 {
 	unsigned long value;
 
-	WARN_ON(raw_smp_processor_id() != current->kvm_vcpu->cpu);
 	asm volatile (ASM_VMX_VMREAD_RDX_RAX
 		      : "=a"(value) : "d"(field) : "cc");
 	return value;
@@ -215,7 +214,6 @@ static void vmcs_writel(unsigned long fi
 {
 	u8 error;
 
-	WARN_ON(raw_smp_processor_id() != current->kvm_vcpu->cpu);
 	asm volatile (ASM_VMX_VMWRITE_RAX_RDX "; setna %0"
 		       : "=q"(error) : "a"(value), "d"(field) : "cc" );
 	if (unlikely(error))
@@ -384,7 +382,6 @@ static void vmx_vcpu_load(struct kvm_vcp
 	u64 phys_addr = __pa(vcpu->vmcs);
 	u64 tsc_this, delta;
 
-	WARN_ON(!preempt_count());
 	if (vcpu->cpu != cpu)
 		vcpu_clear(vcpu);
 
@@ -427,7 +424,6 @@ static void vmx_vcpu_load(struct kvm_vcp
 
 static void vmx_vcpu_put(struct kvm_vcpu *vcpu)
 {
-	WARN_ON(!preempt_count());
 	vmx_load_host_state(vcpu);
 	kvm_put_guest_fpu(vcpu);
 }
@@ -2001,6 +1997,10 @@ preempted:
 		kvm_guest_debug_pre(vcpu);
 
 again:
+	r = kvm_mmu_reload(vcpu);
+	if (unlikely(r))
+		goto out;
+
 	preempt_disable();
 
 	if (!vcpu->mmio_read_completed)
@@ -2009,10 +2009,6 @@ again:
 	vmx_save_host_state(vcpu);
 	kvm_load_guest_fpu(vcpu);
 
-	r = kvm_mmu_reload(vcpu);
-	if (unlikely(r))
-		goto out;
-
 	/*
 	 * Loading guest fpu may have cleared host cr0.ts
 	 */

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/

Re: [RFC 1/8]KVM: fix bugs in kvm sched integration patch

[Avi Kivity]

Shaohua Li wrote:
> fix some bugs in kvm-sch patch.
>   

There is now a 'preempt-hooks' branch on kvm.git with the preempt-hooks 
work.  I'll continually update and rebase it against master.

> 1. vmcs_readl/vmcs_writel are called with preempt enabled
>   

Why is that bad?

> 2. preempt_count check doesn't make sense with preempt disabled
>   

That is already fixed in the branch.

> 3. vmx_cpu_run doesn't handle error correctly and kvm_mmu_reload might
> sleep with mutex changes, so I move it above.
>   

Yes.

> @@ -2001,6 +1997,10 @@ preempted:
>  		kvm_guest_debug_pre(vcpu);
>  
>  again:
> +	r = kvm_mmu_reload(vcpu);
> +	if (unlikely(r))
> +		goto out;
> +
>  	preempt_disable();
>  
>  	if (!vcpu->mmio_read_completed)
> @@ -2009,10 +2009,6 @@ again:
>  	vmx_save_host_state(vcpu);
>  	kvm_load_guest_fpu(vcpu);
>  
> -	r = kvm_mmu_reload(vcpu);
> -	if (unlikely(r))
> -		goto out;
> -
>  	/*
>  	 * Loading guest fpu may have cleared host cr0.ts
>  	 */
>   

I'll add this (and the spinlock->mutex conversion) to the branch.

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

Re: [RFC 1/8]KVM: fix bugs in kvm sched integration patch

[Avi Kivity]

Shaohua Li wrote:
> fix some bugs in kvm-sch patch.
>   

There is now a 'preempt-hooks' branch on kvm.git with the preempt-hooks 
work.  I'll continually update and rebase it against master.

> 1. vmcs_readl/vmcs_writel are called with preempt enabled
>   

Why is that bad?

> 2. preempt_count check doesn't make sense with preempt disabled
>   

That is already fixed in the branch.

> 3. vmx_cpu_run doesn't handle error correctly and kvm_mmu_reload might
> sleep with mutex changes, so I move it above.
>   

Yes.

> @@ -2001,6 +1997,10 @@ preempted:
>  		kvm_guest_debug_pre(vcpu);
>  
>  again:
> +	r = kvm_mmu_reload(vcpu);
> +	if (unlikely(r))
> +		goto out;
> +
>  	preempt_disable();
>  
>  	if (!vcpu->mmio_read_completed)
> @@ -2009,10 +2009,6 @@ again:
>  	vmx_save_host_state(vcpu);
>  	kvm_load_guest_fpu(vcpu);
>  
> -	r = kvm_mmu_reload(vcpu);
> -	if (unlikely(r))
> -		goto out;
> -
>  	/*
>  	 * Loading guest fpu may have cleared host cr0.ts
>  	 */
>   

I'll add this (and the spinlock->mutex conversion) to the branch.

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


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/

Re: [RFC 1/8]KVM: fix bugs in kvm sched integration patch

[Shaohua Li]

On Mon, 2007-07-23 at 18:46 +0800, Avi Kivity wrote:
> Shaohua Li wrote:
> > fix some bugs in kvm-sch patch.
> >  
> 
> There is now a 'preempt-hooks' branch on kvm.git with the
> preempt-hooks
> work.  I'll continually update and rebase it against master.
> 
> > 1. vmcs_readl/vmcs_writel are called with preempt enabled
> >  
> 
> Why is that bad?
1. raw_smp_processor_id()
2. migrate to other cpu
3. current->kvm_vcpu->cpu != the cpu id of step 1.
you will see the warning.

Thanks,
Shaohua

Re: [RFC 1/8]KVM: fix bugs in kvm sched integration patch

[Shaohua Li]

On Mon, 2007-07-23 at 18:46 +0800, Avi Kivity wrote:
> Shaohua Li wrote:
> > fix some bugs in kvm-sch patch.
> >  
> 
> There is now a 'preempt-hooks' branch on kvm.git with the
> preempt-hooks
> work.  I'll continually update and rebase it against master.
> 
> > 1. vmcs_readl/vmcs_writel are called with preempt enabled
> >  
> 
> Why is that bad?
1. raw_smp_processor_id()
2. migrate to other cpu
3. current->kvm_vcpu->cpu != the cpu id of step 1.
you will see the warning.

Thanks,
Shaohua

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/

Re: [RFC 1/8]KVM: fix bugs in kvm sched integration patch

[Avi Kivity]

Shaohua Li wrote:
>>
>>> 1. vmcs_readl/vmcs_writel are called with preempt enabled
>>>  
>>>       
>> Why is that bad?
>>     
> 1. raw_smp_processor_id()
> 2. migrate to other cpu
> 3. current->kvm_vcpu->cpu != the cpu id of step 1.
> you will see the warning.
>   

Ah, that code is gone from preempt-hooks, hence I didn't understand you.

The current version of the patchset does not change
vmcs_readl/vmcs_writel.  So I think everything is safe.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [RFC 1/8]KVM: fix bugs in kvm sched integration patch

[Avi Kivity]

Shaohua Li wrote:
>>
>>> 1. vmcs_readl/vmcs_writel are called with preempt enabled
>>>  
>>>       
>> Why is that bad?
>>     
> 1. raw_smp_processor_id()
> 2. migrate to other cpu
> 3. current->kvm_vcpu->cpu != the cpu id of step 1.
> you will see the warning.
>   

Ah, that code is gone from preempt-hooks, hence I didn't understand you.

The current version of the patchset does not change
vmcs_readl/vmcs_writel.  So I think everything is safe.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/