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;
}
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
