[PATCH]: pointer to vmcs getting lost

[PATCH]: pointer to vmcs getting lost

[Jesse]

Greetings,

I noticed a race condition when running two guests simultaneously and 
debugging both guests (on 64-bit intel cpus). Periodically I would get 
errors from the vmread, vmwrite, or vmresume instructions. Some research 
revealed that these errors were being caused by having an invalid vmcs 
loaded. Further, I found that the vmcs is a per_cpu variable, which I 
believe means that any reference to it is invalid after a context 
switch. (Corrections appreciated). This means that the vmcs must be 
reloaded each time the process is switched to. The patch below fixed the 
problem for me.

This patch does three things.
1. Extends the critical section in __vcpu_run to include the handling of 
vmexits, where many of the vmread/writes occur.
2. Perform a vcpu_load after we enter the critical section, and after we 
return from kvm_resched.
3. Move the call to kvm_guest_debug_pre into the critical section 
(because it calls vmread/write).

I hope you find this useful. I am not on list, so please CC me on replies.

~Jesse Dutton

 

diff -ruNa kvm-72/kernel/x86.c kvm-72-changed/kernel/x86.c
--- kvm-72/kernel/x86.c	2008-07-27 06:20:10.000000000 -0700
+++ kvm-72-changed/kernel/x86.c	2008-07-31 15:25:25.000000000 -0700
@@ -2845,8 +2845,6 @@
 	vapic_enter(vcpu);
 
 preempted:
-	if (vcpu->guest_debug.enabled)
-		kvm_x86_ops->guest_debug_pre(vcpu);
 
 again:
 	if (vcpu->requests)
@@ -2878,7 +2876,12 @@
 	clear_bit(KVM_REQ_PENDING_TIMER, &vcpu->requests);
 	kvm_inject_pending_timer_irqs(vcpu);
 
+	vcpu_put(vcpu);
 	preempt_disable();
+	vcpu_load(vcpu);
+
+	if (vcpu->guest_debug.enabled)
+		kvm_x86_ops->guest_debug_pre(vcpu);
 
 	kvm_x86_ops->prepare_guest_switch(vcpu);
 	kvm_load_guest_fpu(vcpu);
@@ -2941,7 +2944,6 @@
 
 	kvm_guest_exit();
 
-	preempt_enable();
 
 	down_read(&vcpu->kvm->slots_lock);
 
@@ -2960,6 +2962,8 @@
 
 	r = kvm_x86_ops->handle_exit(kvm_run, vcpu);
 
+	preempt_enable();
+
 	if (r > 0) {
 		if (dm_request_for_irq_injection(vcpu, kvm_run)) {
 			r = -EINTR;
@@ -2974,7 +2978,9 @@
 out:
 	up_read(&vcpu->kvm->slots_lock);
 	if (r > 0) {
+		vcpu_put(vcpu);
 		kvm_resched(vcpu);
+		vcpu_load(vcpu);
 		down_read(&vcpu->kvm->slots_lock);
 		goto preempted;
 	}

Re: [PATCH]: pointer to vmcs getting lost

[Marcelo Tosatti]

Hi Jesse,

On Fri, Aug 01, 2008 at 03:18:52PM -0700, Jesse wrote:
> Greetings,
>
> I noticed a race condition when running two guests simultaneously and  
> debugging both guests (on 64-bit intel cpus). Periodically I would get  
> errors from the vmread, vmwrite, or vmresume instructions. Some research  
> revealed that these errors were being caused by having an invalid vmcs  
> loaded. Further, I found that the vmcs is a per_cpu variable, which I  
> believe means that any reference to it is invalid after a context  
> switch. (Corrections appreciated). This means that the vmcs must be  
> reloaded each time the process is switched to. 

The preempt notifiers will do that for you.

> The patch below fixed the  
> problem for me.
>
> This patch does three things.
> 1. Extends the critical section in __vcpu_run to include the handling of  
> vmexits, where many of the vmread/writes occur.
> 2. Perform a vcpu_load after we enter the critical section, and after we  
> return from kvm_resched.
> 3. Move the call to kvm_guest_debug_pre into the critical section  
> (because it calls vmread/write).

Wouldnt it suffice to move ->guest_debug_pre into the non preemptable
section? http://article.gmane.org/gmane.comp.emulators.kvm.devel/20244 

I haven't tested that patch though.

Re: [PATCH]: pointer to vmcs getting lost

[Jesse]

Thanks for the feedback. Comments inline.

Marcelo Tosatti wrote:
> Hi Jesse,
>
> On Fri, Aug 01, 2008 at 03:18:52PM -0700, Jesse wrote:
>   
>> Greetings,
>>
>> I noticed a race condition when running two guests simultaneously and  
>> debugging both guests (on 64-bit intel cpus). Periodically I would get  
>> errors from the vmread, vmwrite, or vmresume instructions. Some research  
>> revealed that these errors were being caused by having an invalid vmcs  
>> loaded. Further, I found that the vmcs is a per_cpu variable, which I  
>> believe means that any reference to it is invalid after a context  
>> switch. (Corrections appreciated). This means that the vmcs must be  
>> reloaded each time the process is switched to. 
>>     
>
> The preempt notifiers will do that for you.
>   
Right, but they won't call VMPTRLD. For some reason this matters (for 
intel chips), even if the variable ends up back in the same place, as 
far as I can tell.
>   
>> The patch below fixed the  
>> problem for me.
>>
>> This patch does three things.
>> 1. Extends the critical section in __vcpu_run to include the handling of  
>> vmexits, where many of the vmread/writes occur.
>> 2. Perform a vcpu_load after we enter the critical section, and after we  
>> return from kvm_resched.
>> 3. Move the call to kvm_guest_debug_pre into the critical section  
>> (because it calls vmread/write).
>>     
>
> Wouldnt it suffice to move ->guest_debug_pre into the non preemptable
> section? http://article.gmane.org/gmane.comp.emulators.kvm.devel/20244 
>   
Excellent. I hadn't seen that patch yet. However, many of the 
vmreads/vmwrites that failed in my testing were in the exit handlers. 
And a calling VMPTRLD (in vcpu_load) explicitly on entering the critical 
section secures any other vmcs concurrency problems.
> I haven't tested that patch though.
>
>   

Re: [PATCH]: pointer to vmcs getting lost

[Marcelo Tosatti]

On Fri, Aug 01, 2008 at 04:36:03PM -0700, Jesse wrote:
> Thanks for the feedback. Comments inline.
>
> Marcelo Tosatti wrote:
>> Hi Jesse,
>>
>> On Fri, Aug 01, 2008 at 03:18:52PM -0700, Jesse wrote:
>>   
>>> Greetings,
>>>
>>> I noticed a race condition when running two guests simultaneously and 
>>>  debugging both guests (on 64-bit intel cpus). Periodically I would 
>>> get  errors from the vmread, vmwrite, or vmresume instructions. Some 
>>> research  revealed that these errors were being caused by having an 
>>> invalid vmcs  loaded. Further, I found that the vmcs is a per_cpu 
>>> variable, which I  believe means that any reference to it is invalid 
>>> after a context  switch. (Corrections appreciated). This means that 
>>> the vmcs must be  reloaded each time the process is switched to.     
>>
>> The preempt notifiers will do that for you.
>>   
> Right, but they won't call VMPTRLD. For some reason this matters (for  
> intel chips), even if the variable ends up back in the same place, as  
> far as I can tell.

They will: kvm_sched_in -> kvm_arch_vcpu_load -> vmx_vcpu_load:

    struct vcpu_vmx *vmx = to_vmx(vcpu)
    
    if (per_cpu(current_vmcs, cpu) != vmx->vmcs) {
        ...

>>> The patch below fixed the  problem for me.
>>>
>>> This patch does three things.
>>> 1. Extends the critical section in __vcpu_run to include the handling 
>>> of  vmexits, where  -many of the vmread/writes occur.
>>> 2. Perform a vcpu_load after we enter the critical section, and after 
>>> we  return from kvm_resched.
>>> 3. Move the call to kvm_guest_debug_pre into the critical section   
>>> (because it calls vmread/write).
>>>     
>>
>> Wouldnt it suffice to move ->guest_debug_pre into the non preemptable
>> section? http://article.gmane.org/gmane.comp.emulators.kvm.devel/20244  
>>   
> Excellent. I hadn't seen that patch yet. However, many of the  
> vmreads/vmwrites that failed in my testing were in the exit handlers.  
> And a calling VMPTRLD (in vcpu_load) explicitly on entering the critical  
> section secures any other vmcs concurrency problems.

Do you have preempt notifiers enabled in your host kernel?

Re: [PATCH]: pointer to vmcs getting lost

[Jesse]

Marcelo Tosatti wrote:
> Do you have preempt notifiers enabled in your host kernel?
>   

No. And it looks like this is the real issue then. (Thank you for 
reading between the lines and figuring out what was really wrong).

I compile the module apart from the kernel, so kvm support was not 
enabled when compiling the kernel. In 2.6.24, the PREEMPT_NOTIFIER 
option is not user selectable in make config, but is selected for you 
when kvm support is selected. IMO, this makes building modules apart 
from kernel source unadvised; however, there seems to be no alternative 
with the pace of kvm development relative to the kernel release schedule.

Re: [PATCH]: pointer to vmcs getting lost

[Avi Kivity]

Jesse wrote:
> Marcelo Tosatti wrote:
>> Do you have preempt notifiers enabled in your host kernel?
>>   
> 
> No. And it looks like this is the real issue then. (Thank you for 
> reading between the lines and figuring out what was really wrong).
> 
> I compile the module apart from the kernel, so kvm support was not 
> enabled when compiling the kernel. In 2.6.24, the PREEMPT_NOTIFIER 
> option is not user selectable in make config, but is selected for you 
> when kvm support is selected. IMO, this makes building modules apart 
> from kernel source unadvised; however, there seems to be no alternative 
> with the pace of kvm development relative to the kernel release schedule.

If the host kernel doesn't have preempt notifiers enabled, then the kvm 
external module emulates preempt notifiers.  This emulation uses the 
debug registers, and you mention that you are debugging guests, so that 
may be the source of the problem.