Hi Sasha,
[adding the kvm list; not sure why it was dropped]
On Wed, Oct 28, 2015 at 01:34:25PM +, 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
