From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-14.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3CED7C63697 for ; Mon, 23 Nov 2020 17:23:01 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id A98C520724 for ; Mon, 23 Nov 2020 17:23:00 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="ln6HwgDV"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="Dh9eL4mE" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org A98C520724 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To:Subject:To:From: Message-ID:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=TQX6iBejploaTQNv3ZuoKFSwEUACU7IftgdzADnvmxo=; b=ln6HwgDVr/PscI/alvNdoW9LT bCKTaIBf1ZASXOJ9AAcKlzrJ+XsYeHN5YOcOlBaHHELjukLYItkfIgpVIjBJVs3lhu6AIhXaEPysi Xb5PQI7T71hI1BsnvnTH6jNBmhsivZA3YUqBfF4xOWxycZi2yPIDa/+Qj8/diF3RH2jpkrbB3wfMe nX8GURbPd0n6RXNd03jbdRES/tWFccoIws0NIIlBuIXGSY0xJMvMUM0gkc1VHr9c9UvO08ZCq2XGP PmLmPy4A4bSpwOAC9UWOVVpfbKcWrqkjAx2XhAyU66Q/Qk1YHR+yNWb+k7scxtgwRVU7uPM7HkVxR J6rM+oZiw==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1khFXy-0001ia-Q8; Mon, 23 Nov 2020 17:22:38 +0000 Received: from mail.kernel.org ([198.145.29.99]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1khFXw-0001i5-7t for linux-arm-kernel@lists.infradead.org; Mon, 23 Nov 2020 17:22:37 +0000 Received: from disco-boy.misterjones.org (disco-boy.misterjones.org [51.254.78.96]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id AF96420728; Mon, 23 Nov 2020 17:22:34 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1606152154; bh=8wEfC5w8dh4/BgvMmxnFJk49Pw9D7B7b3SRsuAKB1CA=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=Dh9eL4mEAzqVUyJpdosPigUkyRsdc/BQ32Q6qR9ScTB9wi0OJtUVPuLJ+BF/RVRUo pa3s2b8l6KimnttnT7V3tkOOClmJoFrYI81eHx7P7B5bw/1xpPNo4UVnirUrUkCXvy PoR3Aa9zG20MVRFndp49qqp9lcW2vm81Ko+Fnp58= Received: from 78.163-31-62.static.virginmediabusiness.co.uk ([62.31.163.78] helo=wait-a-minute.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94) (envelope-from ) id 1khFXs-00D134-DG; Mon, 23 Nov 2020 17:22:32 +0000 Date: Mon, 23 Nov 2020 17:22:30 +0000 Message-ID: <87ft5056p5.wl-maz@kernel.org> From: Marc Zyngier To: David Brazdil Subject: Re: [PATCH v2 20/24] kvm: arm64: Intercept host's CPU_SUSPEND PSCI SMCs In-Reply-To: <20201116204318.63987-21-dbrazdil@google.com> References: <20201116204318.63987-1-dbrazdil@google.com> <20201116204318.63987-21-dbrazdil@google.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 Emacs/27.1 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") X-SA-Exim-Connect-IP: 62.31.163.78 X-SA-Exim-Rcpt-To: dbrazdil@google.com, kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, james.morse@arm.com, julien.thierry.kdev@gmail.com, suzuki.poulose@arm.com, catalin.marinas@arm.com, will@kernel.org, dennis@kernel.org, tj@kernel.org, cl@linux.com, mark.rutland@arm.com, lorenzo.pieralisi@arm.com, qperret@google.com, ascull@google.com, qwandor@google.com, kernel-team@android.com, lorenzo.pieralisi@arm.com, sudeep.holla@arm.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201123_122236_423884_BD0201C5 X-CRM114-Status: GOOD ( 38.72 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Mark Rutland , kernel-team@android.com, Lorenzo Pieralisi , Andrew Walbran , Suzuki K Poulose , Catalin Marinas , Quentin Perret , linux-kernel@vger.kernel.org, James Morse , linux-arm-kernel@lists.infradead.org, Sudeep Holla , Tejun Heo , Dennis Zhou , Christoph Lameter , Will Deacon , kvmarm@lists.cs.columbia.edu, Julien Thierry , Andrew Scull Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Adding Lorenzo and Sudeep to this one in particular, as there is a bit of a corner case below. On Mon, 16 Nov 2020 20:43:14 +0000, David Brazdil wrote: > > Add a handler of CPU_SUSPEND host PSCI SMCs. The SMC can either enter > a sleep state indistinguishable from a WFI or a deeper sleep state that > behaves like a CPU_OFF+CPU_ON. > > The handler saves r0,pc of the host and makes the same call to EL3 with > the hyp CPU entry point. It either returns back to the handler and then > back to the host, or wakes up into the entry point and initializes EL2 > state before dropping back to EL1. > > There is a simple atomic lock around the reset state struct to protect > from races with CPU_ON. A well-behaved host should never run CPU_ON > against an already online core, and the kernel indeed does not allow > that, so if the core sees its reset state struct locked, it will return > a non-spec error code PENDING_ON. This protects the hypervisor state and "non-spec" as in "outside of the PSCI specification"? Err... > avoids the need for more complicated locking and/or tracking power state > of individual cores. > > Signed-off-by: David Brazdil > --- > arch/arm64/kvm/hyp/nvhe/psci-relay.c | 39 +++++++++++++++++++++++++++- > 1 file changed, 38 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/kvm/hyp/nvhe/psci-relay.c b/arch/arm64/kvm/hyp/nvhe/psci-relay.c > index 2daf52b59846..313ef42f0eab 100644 > --- a/arch/arm64/kvm/hyp/nvhe/psci-relay.c > +++ b/arch/arm64/kvm/hyp/nvhe/psci-relay.c > @@ -121,6 +121,39 @@ static void release_reset_state(struct kvm_host_psci_state *cpu_state) > atomic_set_release(&cpu_state->pending_on, 0); > } > > +static int psci_cpu_suspend(u64 func_id, struct kvm_cpu_context *host_ctxt) > +{ > + u64 power_state = host_ctxt->regs.regs[1]; > + unsigned long pc = host_ctxt->regs.regs[2]; > + unsigned long r0 = host_ctxt->regs.regs[3]; > + struct kvm_host_psci_state *cpu_state; > + struct kvm_nvhe_init_params *cpu_params; > + int ret; > + > + cpu_state = this_cpu_ptr(&kvm_host_psci_state); > + cpu_params = this_cpu_ptr(&kvm_init_params); > + > + /* > + * Lock the reset state struct. This fails if the host has concurrently > + * called CPU_ON with this CPU as target. The kernel keeps track of > + * online CPUs, so that should never happen. If it does anyway, return > + * a non-spec error. This avoids the need for spinlocks. > + */ > + if (!try_acquire_reset_state(cpu_state, pc, r0)) > + return PSCI_RET_ALREADY_ON; So that's the core of the problem. I'm definitely not keen on EL2 returning unspecified error codes. But there is something I don't get: If the CPU is currently booting (reset state is locked), it means that CPU hasn't reached the EL1 kernel yet. So how can this same CPU issue a CPU_SUSPEND from EL1? CPU_SUSPEND can't be called for a third party, only by a CPU for itself. It looks like this case cannot happen by construction. And if it happens, it looks like the only course of action should be to panic, as we have lost track of the running CPUs. Am I missing something obvious? > + > + /* > + * Will either return if shallow sleep state, or wake up into the entry > + * point if it is a deep sleep state. > + */ > + ret = psci_call(func_id, power_state, > + __hyp_pa(hyp_symbol_addr(__kvm_hyp_cpu_entry)), > + __hyp_pa(cpu_params)); > + > + release_reset_state(cpu_state); > + return ret; > +} > + > static int psci_cpu_on(u64 func_id, struct kvm_cpu_context *host_ctxt) > { > u64 mpidr = host_ctxt->regs.regs[1]; > @@ -178,7 +211,9 @@ asmlinkage void __noreturn __kvm_hyp_psci_cpu_entry(void) > > static unsigned long psci_0_1_handler(u64 func_id, struct kvm_cpu_context *host_ctxt) > { > - if (func_id == kvm_host_psci_function_id[PSCI_FN_CPU_OFF]) > + if (func_id == kvm_host_psci_function_id[PSCI_FN_CPU_SUSPEND]) > + return psci_cpu_suspend(func_id, host_ctxt); > + else if (func_id == kvm_host_psci_function_id[PSCI_FN_CPU_OFF]) > return psci_forward(host_ctxt); > else if (func_id == kvm_host_psci_function_id[PSCI_FN_CPU_ON]) > return psci_cpu_on(func_id, host_ctxt); > @@ -202,6 +237,8 @@ static unsigned long psci_0_2_handler(u64 func_id, struct kvm_cpu_context *host_ > case PSCI_0_2_FN_SYSTEM_RESET: > psci_forward_noreturn(host_ctxt); > unreachable(); > + case PSCI_0_2_FN64_CPU_SUSPEND: > + return psci_cpu_suspend(func_id, host_ctxt); > case PSCI_0_2_FN64_CPU_ON: > return psci_cpu_on(func_id, host_ctxt); > default: > -- > 2.29.2.299.gdc1121823c-goog > > Thanks, M. -- Without deviation from the norm, progress is not possible. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel