[patch 0/2] fix migration/savevm with offline vcpus

[patch 0/2] fix migration/savevm with offline vcpus

[Marcelo Tosatti]

And also cleanup and standardize vcpu waking.

-- 

[patch 1/2] KVM: x86: do not entry guest mode if vcpu is not runnable

[Marcelo Tosatti]

[-- Attachment #1: vcpublock-caller-handles-mpstate --]
[-- Type: text/plain, Size: 3901 bytes --]

If a vcpu has been offlined, or not initialized at all, signals
requesting userspace work to be performed will result in KVM attempting
to re-entry guest mode.

Problem is that the in-kernel irqchip emulation happily executes HALTED
state vcpu's. This breaks "savevm" on Windows SMP installation (that
only boots up a single vcpu), for example.

Fix it by blocking halted vcpu's at kvm_arch_vcpu_ioctl_run(). 

Change the promotion from halted to running to happen in the vcpu
context. Use the information available in kvm_vcpu_block(), and the
current mpstate to make the decision:

- If there's an in-kernel timer or irq event the halted->running
promotion evaluation can be performed, no need for userspace assistance.

- If there's a signal, there's either userspace work to be performed
in the vcpu's context or irqchip emulation is in userspace.

This has the nice side effect of avoiding userspace exit in case 
of irq injection to a halted vcpu from the iothread.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: kvm/arch/x86/kvm/x86.c
===================================================================
--- kvm.orig/arch/x86/kvm/x86.c
+++ kvm/arch/x86/kvm/x86.c
@@ -2505,17 +2505,25 @@ void kvm_arch_exit(void)
 	kvm_mmu_module_exit();
 }
 
+static void kvm_vcpu_promote_runnable(struct kvm_vcpu *vcpu)
+{
+	if (vcpu->arch.mp_state == KVM_MP_STATE_HALTED)
+		vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
+}
+
 int kvm_emulate_halt(struct kvm_vcpu *vcpu)
 {
 	++vcpu->stat.halt_exits;
 	KVMTRACE_0D(HLT, vcpu, handler);
 	if (irqchip_in_kernel(vcpu->kvm)) {
+		int ret;
 		vcpu->arch.mp_state = KVM_MP_STATE_HALTED;
 		up_read(&vcpu->kvm->slots_lock);
-		kvm_vcpu_block(vcpu);
+		ret = kvm_vcpu_block(vcpu);
 		down_read(&vcpu->kvm->slots_lock);
-		if (vcpu->arch.mp_state != KVM_MP_STATE_RUNNABLE)
+		if (ret)
 			return -EINTR;
+		kvm_vcpu_promote_runnable(vcpu);
 		return 1;
 	} else {
 		vcpu->run->exit_reason = KVM_EXIT_HLT;
@@ -2978,10 +2986,12 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_v
 	if (vcpu->sigset_active)
 		sigprocmask(SIG_SETMASK, &vcpu->sigset, &sigsaved);
 
-	if (unlikely(vcpu->arch.mp_state == KVM_MP_STATE_UNINITIALIZED)) {
-		kvm_vcpu_block(vcpu);
-		r = -EAGAIN;
-		goto out;
+	if (unlikely(!kvm_arch_vcpu_runnable(vcpu))) {
+		if (kvm_vcpu_block(vcpu)) {
+			r = -EAGAIN;
+			goto out;
+		}
+		kvm_vcpu_promote_runnable(vcpu);
 	}
 
 	/* re-sync apic's tpr */
Index: kvm/include/linux/kvm_host.h
===================================================================
--- kvm.orig/include/linux/kvm_host.h
+++ kvm/include/linux/kvm_host.h
@@ -199,7 +199,7 @@ struct kvm_memory_slot *gfn_to_memslot(s
 int kvm_is_visible_gfn(struct kvm *kvm, gfn_t gfn);
 void mark_page_dirty(struct kvm *kvm, gfn_t gfn);
 
-void kvm_vcpu_block(struct kvm_vcpu *vcpu);
+int kvm_vcpu_block(struct kvm_vcpu *vcpu);
 void kvm_resched(struct kvm_vcpu *vcpu);
 void kvm_load_guest_fpu(struct kvm_vcpu *vcpu);
 void kvm_put_guest_fpu(struct kvm_vcpu *vcpu);
Index: kvm/virt/kvm/kvm_main.c
===================================================================
--- kvm.orig/virt/kvm/kvm_main.c
+++ kvm/virt/kvm/kvm_main.c
@@ -818,9 +818,10 @@ void mark_page_dirty(struct kvm *kvm, gf
 /*
  * The vCPU has executed a HLT instruction with in-kernel mode enabled.
  */
-void kvm_vcpu_block(struct kvm_vcpu *vcpu)
+int kvm_vcpu_block(struct kvm_vcpu *vcpu)
 {
 	DEFINE_WAIT(wait);
+	int ret = 0;
 
 	for (;;) {
 		prepare_to_wait(&vcpu->wq, &wait, TASK_INTERRUPTIBLE);
@@ -831,8 +832,10 @@ void kvm_vcpu_block(struct kvm_vcpu *vcp
 			break;
 		if (kvm_arch_vcpu_runnable(vcpu))
 			break;
-		if (signal_pending(current))
+		if (signal_pending(current)) {
+			ret = 1;
 			break;
+		}
 
 		vcpu_put(vcpu);
 		schedule();
@@ -840,6 +843,7 @@ void kvm_vcpu_block(struct kvm_vcpu *vcp
 	}
 
 	finish_wait(&vcpu->wq, &wait);
+	return ret;
 }
 
 void kvm_resched(struct kvm_vcpu *vcpu)

-- 

[patch 2/2] KVM: x86: standardize vcpu wakeup method for in-kernel irqchip

[Marcelo Tosatti]

[-- Attachment #1: cleanup-lapic-mpstate --]
[-- Type: text/plain, Size: 2295 bytes --]

Now that halted->runnable promotion is handled in the vcpu's context, cleanup 
wakers:

- If running outside of irq context, use kvm_vcpu_kick(vcpu)
- If running from irq context, wake_up_interruptible(vcpu->wq) (irq should 
trigger on target vcpu).

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: kvm/arch/x86/kvm/i8254.c
===================================================================
--- kvm.orig/arch/x86/kvm/i8254.c
+++ kvm/arch/x86/kvm/i8254.c
@@ -200,10 +200,8 @@ static int __pit_timer_fn(struct kvm_kpi
 
 	if (!atomic_inc_and_test(&pt->pending))
 		set_bit(KVM_REQ_PENDING_TIMER, &vcpu0->requests);
-	if (vcpu0 && waitqueue_active(&vcpu0->wq)) {
-		vcpu0->arch.mp_state = KVM_MP_STATE_RUNNABLE;
+	if (vcpu0 && waitqueue_active(&vcpu0->wq))
 		wake_up_interruptible(&vcpu0->wq);
-	}
 
 	pt->timer.expires = ktime_add_ns(pt->timer.expires, pt->period);
 	pt->scheduled = ktime_to_ns(pt->timer.expires);
Index: kvm/arch/x86/kvm/lapic.c
===================================================================
--- kvm.orig/arch/x86/kvm/lapic.c
+++ kvm/arch/x86/kvm/lapic.c
@@ -339,14 +339,7 @@ static int __apic_accept_irq(struct kvm_
 		} else
 			apic_clear_vector(vector, apic->regs + APIC_TMR);
 
-		if (vcpu->arch.mp_state == KVM_MP_STATE_RUNNABLE)
-			kvm_vcpu_kick(vcpu);
-		else if (vcpu->arch.mp_state == KVM_MP_STATE_HALTED) {
-			vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
-			if (waitqueue_active(&vcpu->wq))
-				wake_up_interruptible(&vcpu->wq);
-		}
-
+		kvm_vcpu_kick(vcpu);
 		result = (orig_irr == 0);
 		break;
 
@@ -384,8 +377,7 @@ static int __apic_accept_irq(struct kvm_
 		if (vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
 			vcpu->arch.sipi_vector = vector;
 			vcpu->arch.mp_state = KVM_MP_STATE_SIPI_RECEIVED;
-			if (waitqueue_active(&vcpu->wq))
-				wake_up_interruptible(&vcpu->wq);
+			kvm_vcpu_kick(vcpu);
 		}
 		break;
 
@@ -947,10 +939,9 @@ static int __apic_timer_fn(struct kvm_la
 
 	if(!atomic_inc_and_test(&apic->timer.pending))
 		set_bit(KVM_REQ_PENDING_TIMER, &apic->vcpu->requests);
-	if (waitqueue_active(q)) {
-		apic->vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
+	if (waitqueue_active(q))
 		wake_up_interruptible(q);
-	}
+
 	if (apic_lvtt_period(apic)) {
 		result = 1;
 		apic->timer.dev.expires = ktime_add_ns(

-- 

Re: [patch 1/2] KVM: x86: do not entry guest mode if vcpu is not runnable

[Marcelo Tosatti]

On Mon, Jul 21, 2008 at 11:38:56AM -0300, Marcelo Tosatti wrote:
> If a vcpu has been offlined, or not initialized at all, signals
> requesting userspace work to be performed will result in KVM attempting
> to re-entry guest mode.
> 
> Problem is that the in-kernel irqchip emulation happily executes HALTED
> state vcpu's. This breaks "savevm" on Windows SMP installation (that
> only boots up a single vcpu), for example.
> 
> Fix it by blocking halted vcpu's at kvm_arch_vcpu_ioctl_run(). 
> 
> Change the promotion from halted to running to happen in the vcpu
> context. Use the information available in kvm_vcpu_block(), and the
> current mpstate to make the decision:
> 
> - If there's an in-kernel timer or irq event the halted->running
> promotion evaluation can be performed, no need for userspace assistance.
> 
> - If there's a signal, there's either userspace work to be performed
> in the vcpu's context or irqchip emulation is in userspace.
> 
> This has the nice side effect of avoiding userspace exit in case 
> of irq injection to a halted vcpu from the iothread.
> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Thinko: reset the vcpu if SIPI received.

BTW, "INIT"/"SIPI" states are x86/IA64 specific MP protocol information,
aren't they? What are they doing in include/linux/ ?


Index: kvm/arch/x86/kvm/x86.c
===================================================================
--- kvm.orig/arch/x86/kvm/x86.c
+++ kvm/arch/x86/kvm/x86.c
@@ -2521,7 +2521,7 @@ int kvm_emulate_halt(struct kvm_vcpu *vc
 		up_read(&vcpu->kvm->slots_lock);
 		ret = kvm_vcpu_block(vcpu);
 		down_read(&vcpu->kvm->slots_lock);
-		if (ret)
+		if (ret || vcpu->arch.mp_state == KVM_MP_STATE_SIPI_RECEIVED)
 			return -EINTR;
 		kvm_vcpu_promote_runnable(vcpu);
 		return 1;

Re: [patch 1/2] KVM: x86: do not entry guest mode if vcpu is not runnable

[Avi Kivity]

Marcelo Tosatti wrote:
> If a vcpu has been offlined, or not initialized at all, signals
> requesting userspace work to be performed will result in KVM attempting
> to re-entry guest mode.
>
> Problem is that the in-kernel irqchip emulation happily executes HALTED
> state vcpu's. This breaks "savevm" on Windows SMP installation (that
> only boots up a single vcpu), for example.
>
> Fix it by blocking halted vcpu's at kvm_arch_vcpu_ioctl_run(). 
>
> Change the promotion from halted to running to happen in the vcpu
> context. Use the information available in kvm_vcpu_block(), and the
> current mpstate to make the decision:
>
> - If there's an in-kernel timer or irq event the halted->running
> promotion evaluation can be performed, no need for userspace assistance.
>
> - If there's a signal, there's either userspace work to be performed
> in the vcpu's context or irqchip emulation is in userspace.
>
> This has the nice side effect of avoiding userspace exit in case 
> of irq injection to a halted vcpu from the iothread.
>
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>
> Index: kvm/arch/x86/kvm/x86.c
> ===================================================================
> --- kvm.orig/arch/x86/kvm/x86.c
> +++ kvm/arch/x86/kvm/x86.c
> @@ -2505,17 +2505,25 @@ void kvm_arch_exit(void)
>  	kvm_mmu_module_exit();
>  }
>  
> +static void kvm_vcpu_promote_runnable(struct kvm_vcpu *vcpu)
> +{
> +	if (vcpu->arch.mp_state == KVM_MP_STATE_HALTED)
> +		vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
> +}
> +
>  int kvm_emulate_halt(struct kvm_vcpu *vcpu)
>  {
>  	++vcpu->stat.halt_exits;
>  	KVMTRACE_0D(HLT, vcpu, handler);
>  	if (irqchip_in_kernel(vcpu->kvm)) {
> +		int ret;
>   

Missing blank line.

> @@ -2978,10 +2986,12 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_v
>  	if (vcpu->sigset_active)
>  		sigprocmask(SIG_SETMASK, &vcpu->sigset, &sigsaved);
>  
> -	if (unlikely(vcpu->arch.mp_state == KVM_MP_STATE_UNINITIALIZED)) {
> -		kvm_vcpu_block(vcpu);
> -		r = -EAGAIN;
> -		goto out;
> +	if (unlikely(!kvm_arch_vcpu_runnable(vcpu))) {
> +		if (kvm_vcpu_block(vcpu)) {
> +			r = -EAGAIN;
> +			goto out;
> +		}
> +		kvm_vcpu_promote_runnable(vcpu);
>  	}
>   


Any reason this is not in __vcpu_run()?

Our main loop could look like

   while (no reason to stop)
         if (runnable)
              enter guest
         else
              block
         deal with aftermath

kvm_emulate_halt would then simply modify the mp state.

>  
>  	/* re-sync apic's tpr */
> Index: kvm/include/linux/kvm_host.h
> ===================================================================
> --- kvm.orig/include/linux/kvm_host.h
> +++ kvm/include/linux/kvm_host.h
> @@ -199,7 +199,7 @@ struct kvm_memory_slot *gfn_to_memslot(s
>  int kvm_is_visible_gfn(struct kvm *kvm, gfn_t gfn);
>  void mark_page_dirty(struct kvm *kvm, gfn_t gfn);
>  
> -void kvm_vcpu_block(struct kvm_vcpu *vcpu);
> +int kvm_vcpu_block(struct kvm_vcpu *vcpu);
>  void kvm_resched(struct kvm_vcpu *vcpu);
>  void kvm_load_guest_fpu(struct kvm_vcpu *vcpu);
>  void kvm_put_guest_fpu(struct kvm_vcpu *vcpu);
> Index: kvm/virt/kvm/kvm_main.c
> ===================================================================
> --- kvm.orig/virt/kvm/kvm_main.c
> +++ kvm/virt/kvm/kvm_main.c
> @@ -818,9 +818,10 @@ void mark_page_dirty(struct kvm *kvm, gf
>  /*
>   * The vCPU has executed a HLT instruction with in-kernel mode enabled.
>   */
> -void kvm_vcpu_block(struct kvm_vcpu *vcpu)
> +int kvm_vcpu_block(struct kvm_vcpu *vcpu)
>  {
>  	DEFINE_WAIT(wait);
> +	int ret = 0;
>  
>  	for (;;) {
>  		prepare_to_wait(&vcpu->wq, &wait, TASK_INTERRUPTIBLE);
> @@ -831,8 +832,10 @@ void kvm_vcpu_block(struct kvm_vcpu *vcp
>  			break;
>  		if (kvm_arch_vcpu_runnable(vcpu))
>  			break;
> -		if (signal_pending(current))
> +		if (signal_pending(current)) {
> +			ret = 1;
>  			break;
> +		}
>   

This is ambiguous.  Multiple exit conditions could be true at the same 
time (vcpu becomes runnable _and_ signal is pending), so you can't trust 
the return code.  It doesn't affect the usage in the rest of the patch 
(I think), but it is best to avoid such subtlety.

Can this be done by setting a KVM_REQ_UNHALT bit in vcpu->requests?

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