Skip to contentSkip to conversation

: pointer to vmcs getting lost

PATCH In review Jesse 3 participants 6 messages Aug 1, 2008 – Aug 11, 2008 KVM

lore
Share & tools
lore

Jesse

author
#0 lore raw

PATCH 1/1 : pointer to vmcs getting lost

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

kernel/x86.c +0 -0
diff -ruNa kvm-72/kernel/x86.c kvm-72-changed/kernel/x86.c
@@ -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;
	}
#1 lore raw

Hi Jesse,

Quoted 10 lines · 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 preempt notifiers will do that for you.

Quoted 10 lines

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.

author
#2 lore raw

Thanks for the feedback. Comments inline.

Quoted 11 lines · Marcelo Tosatti

Hi Jesse,

Quoted 4 lines · 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 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.

Quoted 12 lines
Quoted 8 lines

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.

Quoted 3 lines

I haven't tested that patch though.

#3 lore raw
Quoted 18 lines · Jesse

Thanks for the feedback. Comments inline.

Quoted 15 lines · Marcelo Tosatti

Hi Jesse,

Quoted 10 lines · 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.

Quoted 5 lines
Quoted 2 lines

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) {
        ...
Quoted 11 lines
Quoted 11 lines
Quoted 10 lines

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).

Quoted 7 lines
Quoted 3 lines

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?

author
#4 lore raw
Quoted 2 lines · Marcelo Tosatti

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.

#5 lore raw
Quoted 7 lines · Jesse
Quoted 2 lines · Marcelo Tosatti

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.