Re: [PATCH 2/2] kvmtool: assume dead vcpus are paused too

Re: [PATCH 2/2] kvmtool: assume dead vcpus are paused too

[Will Deacon]

Hi Sasha,

[adding the kvm list; not sure why it was dropped]

On Wed, Oct 28, 2015 at 01:34:25PM +0000, Will Deacon wrote:
> On Wed, Oct 28, 2015 at 09:00:07AM -0400, Sasha Levin wrote:
> > On 10/28/2015 08:27 AM, Will Deacon wrote:
> > > On Tue, Oct 27, 2015 at 10:08:44PM -0400, Sasha Levin wrote:
> > >> > On 10/27/2015 12:08 PM, Will Deacon wrote:
> > >>>> > >>  	for (i = 0; i < kvm->nrcpus; i++)
> > >>>>> > >> > -		pthread_kill(kvm->cpus[i]->thread, SIGKVMPAUSE);
> > >>>>> > >> > +		if (kvm->cpus[i]->is_running)
> > >>>>> > >> > +			pthread_kill(kvm->cpus[i]->thread, SIGKVMPAUSE);
> > >>>>> > >> > +		else
> > >>>>> > >> > +			paused_vcpus++;
> > >>> > > What serialises the test of kvm->cpus[i]->is_running against a concurrent
> > >>> > > SIGKVMEXIT?
> > >> > 
> > >> > If there's a pause signal pending we'd still end up marking it as paused
> > >> > and do the whole process even though the vcpu is actually dead, so while
> > >> > we can race with SIGKVMEXIT, it'll just mean we'd go through the whole
> > >> > pausing procedure even though the vcpu is dead.
> > >> > 
> > >> > Or did you mean something else?
> > > I couldn't convince myself there was an issue here (hence the question ;),
> > > but I was wondering about something like:
> > > 
> > >   1. The VCPUn thread takes SIGKVMEXIT (e.g. due to kvm_cpu__reboot)
> > >   2. The IPC thread handles a pause event and reads kvm->cpus[n]->is_running
> > >      as true
> > >   3. VCPUn sets kvm->cpus[n]->is_running to false
> > >   4. VCPUn exits
> > >   5. The IPC thread trues to pthread_kill on an exited thread
> > > 
> > > am I missing some obvious synchronisation here?
> > 
> > Can we take two signals in parallel? (I'm really not sure). If yes, than
> > what you've described is a problem (and has been for a while). If not,
> > then no :)
> 
> Regardless, isn't the pause event coming from polling the IPC file
> descriptor as opposed to a signal?

It looks like lkvm is currently SEGVing on exit due to the original
issue. Digging deeper, it looks like we should be taking the pause_lock
for the SIGKVMEXIT case too, so that we can serialise access to is_running.

What do you think about the patch below?

Will

--->8

diff --git a/hw/i8042.c b/hw/i8042.c
index 3801e20a675d..288b7d1108ac 100644
--- a/hw/i8042.c
+++ b/hw/i8042.c
@@ -163,7 +163,7 @@ static void kbd_write_command(struct kvm *kvm, u8 val)
 		state.mode &= ~MODE_DISABLE_AUX;
 		break;
 	case I8042_CMD_SYSTEM_RESET:
-		kvm_cpu__reboot(kvm);
+		kvm__reboot(kvm);
 		break;
 	default:
 		break;
diff --git a/include/kvm/kvm-cpu.h b/include/kvm/kvm-cpu.h
index aa0cb543f8fb..c4c9cca449eb 100644
--- a/include/kvm/kvm-cpu.h
+++ b/include/kvm/kvm-cpu.h
@@ -12,7 +12,6 @@ void kvm_cpu__reset_vcpu(struct kvm_cpu *vcpu);
 void kvm_cpu__setup_cpuid(struct kvm_cpu *vcpu);
 void kvm_cpu__enable_singlestep(struct kvm_cpu *vcpu);
 void kvm_cpu__run(struct kvm_cpu *vcpu);
-void kvm_cpu__reboot(struct kvm *kvm);
 int kvm_cpu__start(struct kvm_cpu *cpu);
 bool kvm_cpu__handle_exit(struct kvm_cpu *vcpu);
 int kvm_cpu__get_endianness(struct kvm_cpu *vcpu);
diff --git a/include/kvm/kvm.h b/include/kvm/kvm.h
index 37155dbc132b..a474dae6c2cf 100644
--- a/include/kvm/kvm.h
+++ b/include/kvm/kvm.h
@@ -93,6 +93,7 @@ int kvm__register_mmio(struct kvm *kvm, u64 phys_addr, u64 phys_addr_len, bool c
 		       void (*mmio_fn)(struct kvm_cpu *vcpu, u64 addr, u8 *data, u32 len, u8 is_write, void *ptr),
 			void *ptr);
 bool kvm__deregister_mmio(struct kvm *kvm, u64 phys_addr);
+void kvm__reboot(struct kvm *kvm);
 void kvm__pause(struct kvm *kvm);
 void kvm__continue(struct kvm *kvm);
 void kvm__notify_paused(void);
diff --git a/kvm-cpu.c b/kvm-cpu.c
index ad4441b1d96c..3e2037e3ccb3 100644
--- a/kvm-cpu.c
+++ b/kvm-cpu.c
@@ -45,10 +45,8 @@ void kvm_cpu__run(struct kvm_cpu *vcpu)
 static void kvm_cpu_signal_handler(int signum)
 {
 	if (signum == SIGKVMEXIT) {
-		if (current_kvm_cpu && current_kvm_cpu->is_running) {
+		if (current_kvm_cpu && current_kvm_cpu->is_running)
 			current_kvm_cpu->is_running = false;
-			kvm__continue(current_kvm_cpu->kvm);
-		}
 	} else if (signum == SIGKVMPAUSE) {
 		current_kvm_cpu->paused = 1;
 	}
@@ -70,19 +68,6 @@ static void kvm_cpu__handle_coalesced_mmio(struct kvm_cpu *cpu)
 	}
 }
 
-void kvm_cpu__reboot(struct kvm *kvm)
-{
-	int i;
-
-	/* The kvm->cpus array contains a null pointer in the last location */
-	for (i = 0; ; i++) {
-		if (kvm->cpus[i])
-			pthread_kill(kvm->cpus[i]->thread, SIGKVMEXIT);
-		else
-			break;
-	}
-}
-
 int kvm_cpu__start(struct kvm_cpu *cpu)
 {
 	sigset_t sigset;
@@ -177,7 +162,7 @@ int kvm_cpu__start(struct kvm_cpu *cpu)
 				 * Ensure that all VCPUs are torn down,
 				 * regardless of which CPU generated the event.
 				 */
-				kvm_cpu__reboot(cpu->kvm);
+				kvm__reboot(cpu->kvm);
 				goto exit_kvm;
 			};
 			break;
diff --git a/kvm-ipc.c b/kvm-ipc.c
index 857b0dc943e5..1ef3c3e4c5bc 100644
--- a/kvm-ipc.c
+++ b/kvm-ipc.c
@@ -341,7 +341,7 @@ static void handle_stop(struct kvm *kvm, int fd, u32 type, u32 len, u8 *msg)
 	if (WARN_ON(type != KVM_IPC_STOP || len))
 		return;
 
-	kvm_cpu__reboot(kvm);
+	kvm__reboot(kvm);
 }
 
 /* Pause/resume the guest using SIGUSR2 */
diff --git a/kvm.c b/kvm.c
index 10ed2300ed71..db301a3ae0fc 100644
--- a/kvm.c
+++ b/kvm.c
@@ -428,6 +428,27 @@ void kvm__dump_mem(struct kvm *kvm, unsigned long addr, unsigned long size, int
 	}
 }
 
+void kvm__reboot(struct kvm *kvm)
+{
+	int i;
+
+	/* Check if the guest is running */
+	if (!kvm->cpus[0] || kvm->cpus[0]->thread == 0)
+		return;
+
+	mutex_lock(&pause_lock);
+
+	/* The kvm->cpus array contains a null pointer in the last location */
+	for (i = 0; ; i++) {
+		if (kvm->cpus[i])
+			pthread_kill(kvm->cpus[i]->thread, SIGKVMEXIT);
+		else
+			break;
+	}
+
+	kvm__continue(kvm);
+}
+
 void kvm__pause(struct kvm *kvm)
 {
 	int i, paused_vcpus = 0;
@@ -441,8 +462,12 @@ void kvm__pause(struct kvm *kvm)
 	pause_event = eventfd(0, 0);
 	if (pause_event < 0)
 		die("Failed creating pause notification event");
-	for (i = 0; i < kvm->nrcpus; i++)
-		pthread_kill(kvm->cpus[i]->thread, SIGKVMPAUSE);
+	for (i = 0; i < kvm->nrcpus; i++) {
+		if (kvm->cpus[i]->is_running)
+			pthread_kill(kvm->cpus[i]->thread, SIGKVMPAUSE);
+		else
+			paused_vcpus++;
+	}
 
 	while (paused_vcpus < kvm->nrcpus) {
 		u64 cur_read;
diff --git a/powerpc/spapr_rtas.c b/powerpc/spapr_rtas.c
index 38d899c7f561..b898ff20ba5a 100644
--- a/powerpc/spapr_rtas.c
+++ b/powerpc/spapr_rtas.c
@@ -118,7 +118,7 @@ static void rtas_power_off(struct kvm_cpu *vcpu,
 		rtas_st(vcpu->kvm, rets, 0, -3);
 		return;
 	}
-	kvm_cpu__reboot(vcpu->kvm);
+	kvm__reboot(vcpu->kvm);
 }
 
 static void rtas_system_reboot(struct kvm_cpu *vcpu,
@@ -131,7 +131,7 @@ static void rtas_system_reboot(struct kvm_cpu *vcpu,
 	}
 
 	/* NB this actually halts the VM */
-	kvm_cpu__reboot(vcpu->kvm);
+	kvm__reboot(vcpu->kvm);
 }
 
 static void rtas_query_cpu_stopped_state(struct kvm_cpu *vcpu,
diff --git a/term.c b/term.c
index aebd174597b1..58f66a0c0ea5 100644
--- a/term.c
+++ b/term.c
@@ -37,7 +37,7 @@ int term_getc(struct kvm *kvm, int term)
 	if (term_got_escape) {
 		term_got_escape = false;
 		if (c == 'x')
-			kvm_cpu__reboot(kvm);
+			kvm__reboot(kvm);
 		if (c == term_escape_char)
 			return c;
 	}
diff --git a/ui/sdl.c b/ui/sdl.c
index f97a5112eca9..5035405bb488 100644
--- a/ui/sdl.c
+++ b/ui/sdl.c
@@ -266,7 +266,7 @@ static void *sdl__thread(void *p)
 	}
 exit:
 	done = true;
-	kvm_cpu__reboot(fb->kvm);
+	kvm__reboot(fb->kvm);
 
 	return NULL;
 }

Re: [PATCH 2/2] kvmtool: assume dead vcpus are paused too

[Sasha Levin]

On 11/04/2015 06:51 AM, Will Deacon wrote:
> +	mutex_lock(&pause_lock);
> +
> +	/* The kvm->cpus array contains a null pointer in the last location */
> +	for (i = 0; ; i++) {
> +		if (kvm->cpus[i])
> +			pthread_kill(kvm->cpus[i]->thread, SIGKVMEXIT);
> +		else
> +			break;
> +	}
> +
> +	kvm__continue(kvm);

In this scenario: if we grabbed pause_lock, signaled vcpu0 to exit, and it did
before we called kvm__continue(), we won't end up releasing pause_lock, which
might cause a lockup later, no?


Thanks,
Sasha

Re: [PATCH 2/2] kvmtool: assume dead vcpus are paused too

[Will Deacon]

On Wed, Nov 04, 2015 at 06:51:12PM -0500, Sasha Levin wrote:
> On 11/04/2015 06:51 AM, Will Deacon wrote:
> > +	mutex_lock(&pause_lock);
> > +
> > +	/* The kvm->cpus array contains a null pointer in the last location */
> > +	for (i = 0; ; i++) {
> > +		if (kvm->cpus[i])
> > +			pthread_kill(kvm->cpus[i]->thread, SIGKVMEXIT);
> > +		else
> > +			break;
> > +	}
> > +
> > +	kvm__continue(kvm);
> 
> In this scenario: if we grabbed pause_lock, signaled vcpu0 to exit, and it did
> before we called kvm__continue(), we won't end up releasing pause_lock, which
> might cause a lockup later, no?

Hmm, yeah, maybe that should be an explicit mutex_unlock rather than a
call to kvm__continue.

Will

Re: [PATCH 2/2] kvmtool: assume dead vcpus are paused too

[Sasha Levin]

On 11/05/2015 09:41 AM, Will Deacon wrote:
> On Wed, Nov 04, 2015 at 06:51:12PM -0500, Sasha Levin wrote:
>> > On 11/04/2015 06:51 AM, Will Deacon wrote:
>>> > > +	mutex_lock(&pause_lock);
>>> > > +
>>> > > +	/* The kvm->cpus array contains a null pointer in the last location */
>>> > > +	for (i = 0; ; i++) {
>>> > > +		if (kvm->cpus[i])
>>> > > +			pthread_kill(kvm->cpus[i]->thread, SIGKVMEXIT);
>>> > > +		else
>>> > > +			break;
>>> > > +	}
>>> > > +
>>> > > +	kvm__continue(kvm);
>> > 
>> > In this scenario: if we grabbed pause_lock, signaled vcpu0 to exit, and it did
>> > before we called kvm__continue(), we won't end up releasing pause_lock, which
>> > might cause a lockup later, no?
> Hmm, yeah, maybe that should be an explicit mutex_unlock rather than a
> call to kvm__continue.

Yeah, that should do the trick.


Thanks,
Sasha