From patchwork Tue Apr 19 17:37:13 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: James Morse X-Patchwork-Id: 8882351 Return-Path: X-Original-To: patchwork-linux-arm@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 0B7869F39A for ; Tue, 19 Apr 2016 17:40:51 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 23362202D1 for ; Tue, 19 Apr 2016 17:40:50 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 34C562026C for ; Tue, 19 Apr 2016 17:40:49 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1asZcJ-0001eR-Ps; Tue, 19 Apr 2016 17:39:15 +0000 Received: from foss.arm.com ([217.140.101.70]) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1asZcH-0001cL-Tx for linux-arm-kernel@lists.infradead.org; Tue, 19 Apr 2016 17:39:14 +0000 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 0D13928; Tue, 19 Apr 2016 10:37:37 -0700 (PDT) Received: from [10.1.209.158] (melchizedek.cambridge.arm.com [10.1.209.158]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 312A63F215; Tue, 19 Apr 2016 10:38:52 -0700 (PDT) Message-ID: <57166CC9.4030804@arm.com> Date: Tue, 19 Apr 2016 18:37:13 +0100 From: James Morse User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.6.0 MIME-Version: 1.0 To: Marc Zyngier , linux-arm-kernel@lists.infradead.org, AKASHI Takahiro Subject: Re: [PATCH v7 07/16] arm64: kvm: allows kvm cpu hotplug References: <1459529620-22150-1-git-send-email-james.morse@arm.com> <1459529620-22150-8-git-send-email-james.morse@arm.com> <571656E9.5050402@arm.com> In-Reply-To: <571656E9.5050402@arm.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20160419_103913_991458_05FA2D87 X-CRM114-Status: GOOD ( 19.63 ) X-Spam-Score: -7.9 (-------) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Geoff Levand , Catalin Marinas , Lorenzo Pieralisi , Will Deacon Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-5.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, 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 Marc, Takahiro, On 19/04/16 17:03, Marc Zyngier wrote: > On 01/04/16 17:53, James Morse wrote: >> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c >> index b5384311dec4..962904a443be 100644 >> --- a/arch/arm/kvm/arm.c >> +++ b/arch/arm/kvm/arm.c >> @@ -591,7 +587,13 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) >> /* >> * Re-check atomic conditions >> */ >> - if (signal_pending(current)) { >> + if (unlikely(!__this_cpu_read(kvm_arm_hardware_enabled))) { >> + /* cpu has been torn down */ >> + ret = 0; >> + run->exit_reason = KVM_EXIT_FAIL_ENTRY; >> + run->fail_entry.hardware_entry_failure_reason >> + = (u64)-ENOEXEC; > > This hunk makes me feel a bit uneasy. Having to check something that > critical on the entry path is at least a bit weird. If we've reset EL2 > already, it means that we must have forced an exit on the guest to do so. (To save anyone else digging: the story comes from v12 of the kexec copy of this patch [0]) > So why do we hand the control back to KVM (or anything else) once we've > nuked a CPU? I'd expect it to be put on some back-burner, never to > return in this lifetime... This looks like the normal reboot code being called in the middle of a running system. Kexec calls kernel_restart_prepare(), which kicks each reboot notifier, one of which is kvm_reboot(), which calls: > on_each_cpu(hardware_disable_nolock, NULL, 1); We have to give the CPU back as there may be other reboot notifiers, and kexec hasn't yet shuffled onto the boot cpu. How about moving this check into handle_exit()[1]? Currently this sees ARM_EXCEPTION_IRQ, and tries to re-enter the guest, we can test for kvm_rebooting, which is set by kvm's reboot notifier .... but this would still break if another vcpu wakes from cond_resched() and sprints towards __kvm_vcpu_run(). Can we move cond_resched() to immediately before handle_exit()? I can't see a reason why this doesn't happen on the normal reboot path, presumably we rely on user space to kill any running guests. It looks like x86 uses the extable to work around this, their vmx_vcpu_run() has: > __ex(ASM_VMX_VMLAUNCH) "\n\t" Where __ex ends up calling ____kvm_handle_fault_on_reboot(), with a nearby comment: > * Hardware virtualization extension instructions may fault if a > * reboot turns off virtualization while processes are running. > * Trap the fault and ignore the instruction if that happens. Takahiro, any ideas/wisdom on this? Thanks, James [0] http://lists.infradead.org/pipermail/kexec/2015-December/014953.html [1] Untested(!) alternative. ==================================================== ==================================================== diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c index 0e63047a9530..dfa3cc42ec89 100644 --- a/arch/arm/kvm/arm.c +++ b/arch/arm/kvm/arm.c @@ -562,11 +562,6 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct k vm_run *run) ret = 1; run->exit_reason = KVM_EXIT_UNKNOWN; while (ret > 0) { - /* - * Check conditions before entering the guest - */ - cond_resched(); - update_vttbr(vcpu->kvm); if (vcpu->arch.power_off || vcpu->arch.pause) @@ -662,6 +657,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kv m_run *run) preempt_enable(); + cond_resched(); + ret = handle_exit(vcpu, run, ret); } diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c index eba89e42f0ed..cc562d9ff905 100644 --- a/arch/arm64/kvm/handle_exit.c +++ b/arch/arm64/kvm/handle_exit.c @@ -170,6 +170,12 @@ int handle_exit(struct kvm_vcpu *vcpu, struct kvm_run *run, { exit_handle_fn exit_handler; + if (kvm_rebooting) { + run->exit_reason = KVM_EXIT_SYSTEM_EVENT; + run->fail_entry.hardware_entry_failure_reason = (u64)-ENOEXEC; + return 0; + } + switch (exception_index) { case ARM_EXCEPTION_IRQ: return 1;