From patchwork Wed Nov 4 11:51:12 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Will Deacon X-Patchwork-Id: 7549151 Return-Path: X-Original-To: patchwork-kvm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 5B99E9F327 for ; Wed, 4 Nov 2015 11:51:19 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 34F14205EC for ; Wed, 4 Nov 2015 11:51:18 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id E8BD6205EB for ; Wed, 4 Nov 2015 11:51:16 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753667AbbKDLvO (ORCPT ); Wed, 4 Nov 2015 06:51:14 -0500 Received: from foss.arm.com ([217.140.101.70]:59902 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751402AbbKDLvN (ORCPT ); Wed, 4 Nov 2015 06:51:13 -0500 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id A77843A1; Wed, 4 Nov 2015 03:51:02 -0800 (PST) Received: from arm.com (usa-sjc-imap-foss1.foss.arm.com [10.72.51.249]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 966C63F50D; Wed, 4 Nov 2015 03:51:11 -0800 (PST) Date: Wed, 4 Nov 2015 11:51:12 +0000 From: Will Deacon To: Sasha Levin Cc: andre.przywara@arm.com, josh@joshtriplett.org, penberg@kernel.org, asias.hejun@gmail.com, kvm@vger.kernel.org Subject: Re: [PATCH 2/2] kvmtool: assume dead vcpus are paused too Message-ID: <20151104115112.GE5405@arm.com> References: <1445265650-25616-1-git-send-email-sasha.levin@oracle.com> <1445265650-25616-2-git-send-email-sasha.levin@oracle.com> <20151027160802.GD20208@arm.com> <56302E2C.2010207@oracle.com> <20151028122745.GC29512@arm.com> <5630C6D7.2020608@oracle.com> <20151028133425.GB18966@arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20151028133425.GB18966@arm.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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 --- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html 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; }