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 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 749B9C83F1A for ; Fri, 18 Jul 2025 13:45:03 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=BOT9PzsR3/SwNUZlr6pAl3obJXDpAkRcFnZfw12R6F4=; b=QtdS8tyEpJEaBVi08gOL5L4cxL YUrC09pZG8s2QnE7aQ28144dagQbDO6D5iGg138SuognlMFbgDYVMkp3SXmIxrwBPaYbQ+1XfZwdy gBXrqR1ThfOMxMuzMPeA3OIPZM6b45JCRy1f7W3xWW4CVJvRLlfNSpNNH2AWctwekC/Qf7gPnNYXT AdsZbGMLTH2eV1YN3OJ+2Os1kHtsumgrcw4vT6oiU2k0uRy+Am53EyQmAiMy+gIw3QUBe5GLVz3i1 lX1aKd9k1TJzjNohxmSnUfX/v2F0u9GcItdB5wghtzpf4z370AZ5c+aCKMFthqLJsId+FclvLng+Q N6YsmKtA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98.2 #2 (Red Hat Linux)) id 1uclOS-0000000Citb-2kvM; Fri, 18 Jul 2025 13:44:56 +0000 Received: from sea.source.kernel.org ([2600:3c0a:e001:78e:0:1991:8:25]) by bombadil.infradead.org with esmtps (Exim 4.98.2 #2 (Red Hat Linux)) id 1uclH7-0000000Chxn-2m7V for linux-arm-kernel@lists.infradead.org; Fri, 18 Jul 2025 13:37:22 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by sea.source.kernel.org (Postfix) with ESMTP id 5FD35433A4; Fri, 18 Jul 2025 13:37:21 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 14FB1C4CEEB; Fri, 18 Jul 2025 13:37:17 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1752845841; bh=4gYK7cAt8c0eJOPvaPS4yKEvnYcbVrFkdFhehLWxsik=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=Yy8qNN24agcDyfnirinDCL8kW7X+EzPdoiBa/bKjTRmdTumHRqgxlq98W1ZY9Lqqp BXRCWHbuo/mPUK1rrExsvfoJk7+cLFTCsO5hvY5XicL6wgeSN0lHUjcUZDxHt/jJ+G ikoPOXq+obE1oTE44wqcKgR3kbJP3XhaXkO6AokdrxUywUg/GlKuRNkLpE6k8mWuay z/Xeoq8r/oezbD3wLUOMIgpPcTCeYaqIyloN9DCat7hju8q6/XvrRm2mk9loVJx5gw tYPcY1areNux5tMVYVKowc7jJ12vSCEu9jHb/ArpmyAeafi1KBW1VgaAS85YgZT1LV HY/OUHeipGcAg== Date: Fri, 18 Jul 2025 14:37:14 +0100 From: Will Deacon To: Per Larsen Cc: Marc Zyngier , perlarsen@google.com, Oliver Upton , Joey Gouly , Suzuki K Poulose , Zenghui Yu , Catalin Marinas , Sudeep Holla , linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev, linux-kernel@vger.kernel.org, ahomescu@google.com, armellel@google.com, arve@android.com, ayrton@google.com, qperret@google.com, sebastianene@google.com, qwandor@google.com Subject: Re: [PATCH v7 2/5] KVM: arm64: Use SMCCC 1.2 for FF-A initialization and in host handler Message-ID: References: <20250701-virtio-msg-ffa-v7-0-995afc3d385e@google.com> <20250701-virtio-msg-ffa-v7-2-995afc3d385e@google.com> <8634bdbgaz.wl-maz@kernel.org> <3b10fa81-bfdd-4325-a330-c79df2e21621@immunant.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <3b10fa81-bfdd-4325-a330-c79df2e21621@immunant.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20250718_063721_743500_69BE7905 X-CRM114-Status: GOOD ( 39.74 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Mon, Jul 07, 2025 at 05:06:23PM -0700, Per Larsen wrote: > On 7/3/25 5:38 AM, Marc Zyngier wrote: > > On Tue, 01 Jul 2025 23:06:35 +0100, > > Per Larsen via B4 Relay wrote: > > > diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c > > > index 2c199d40811efb5bfae199c4a67d8ae3d9307357..65d241ba32403d014b43cc4ef4d5bf9693813809 100644 > > > --- a/arch/arm64/kvm/hyp/nvhe/ffa.c > > > +++ b/arch/arm64/kvm/hyp/nvhe/ffa.c > > > @@ -71,36 +71,68 @@ static u32 hyp_ffa_version; > > > static bool has_version_negotiated; > > > static hyp_spinlock_t version_lock; > > > -static void ffa_to_smccc_error(struct arm_smccc_res *res, u64 ffa_errno) > > > +static void ffa_to_smccc_error(struct arm_smccc_1_2_regs *res, u64 ffa_errno) > > > { > > > - *res = (struct arm_smccc_res) { > > > + *res = (struct arm_smccc_1_2_regs) { > > > .a0 = FFA_ERROR, > > > .a2 = ffa_errno, > > > }; > > > } > > > -static void ffa_to_smccc_res_prop(struct arm_smccc_res *res, int ret, u64 prop) > > > +static void ffa_to_smccc_res_prop(struct arm_smccc_1_2_regs *res, int ret, u64 prop) > > > { > > > if (ret == FFA_RET_SUCCESS) { > > > - *res = (struct arm_smccc_res) { .a0 = FFA_SUCCESS, > > > - .a2 = prop }; > > > + *res = (struct arm_smccc_1_2_regs) { .a0 = FFA_SUCCESS, > > > + .a2 = prop }; > > > } else { > > > ffa_to_smccc_error(res, ret); > > > } > > > } > > > -static void ffa_to_smccc_res(struct arm_smccc_res *res, int ret) > > > +static void ffa_to_smccc_res(struct arm_smccc_1_2_regs *res, int ret) > > > { > > > ffa_to_smccc_res_prop(res, ret, 0); > > > } > > > static void ffa_set_retval(struct kvm_cpu_context *ctxt, > > > - struct arm_smccc_res *res) > > > + struct arm_smccc_1_2_regs *res) > > > { > > > + DECLARE_REG(u64, func_id, ctxt, 0); > > > cpu_reg(ctxt, 0) = res->a0; > > > cpu_reg(ctxt, 1) = res->a1; > > > cpu_reg(ctxt, 2) = res->a2; > > > cpu_reg(ctxt, 3) = res->a3; > > > + cpu_reg(ctxt, 4) = res->a4; > > > + cpu_reg(ctxt, 5) = res->a5; > > > + cpu_reg(ctxt, 6) = res->a6; > > > + cpu_reg(ctxt, 7) = res->a7; > > > > From DEN0028G 2.6: > > > > > > Registers W4-W7 must be preserved unless they contain results, as > > specified in the function definition. > > > > > > On what grounds can you blindly change these registers? > From DEN0077A 1.2 Section 11.2: Reserved parameter convention > > > Unused parameter registers in FF-A ABIs are reserved for future use by the > Framework. > > [...] > > The caller is expected to write zeroes to these registers. The callee > ignores the values in these registers. > > > My read is that, as long as we are writing zeroes into reserved registers > (which I believe we are), we comply with DEN0026G 2.6.> Right, the specs make this far more difficult to decipher than necessary but I think SMCCC defers to the definition of the specific call being made and then FF-A is basically saying that unused argument registers are always zeroed. Rather than have the EL2 proxy treat each call differently based on the used argument registers, we can rely on EL3 doing the right thing and blindly copy everything back, which is what you've done. So I think that's ok. > > > + > > > + /* > > > + * DEN0028C 2.6: SMC32/HVC32 call from aarch64 must preserve x8-x30. > > > + * > > > + * The most straightforward approach is to look at the function ID > > > + * sent by the caller. However, the caller could send FFA_MSG_WAIT > > > + * which is a 32-bit interface but the reply could very well be 64-bit > > > + * such as FFA_FN64_MSG_SEND_DIRECT_REQ or FFA_MSG_SEND_DIRECT_REQ2. > > > + * > > > + * Instead, we could look at the function ID in the response (a0) but > > > + * that doesn't work either as FFA_VERSION responses put the version > > > + * number (or error code) in w0. > > > + * > > > + * Set x8-x17 iff response contains 64-bit function ID in a0. > > > + */ > > > + if (func_id != FFA_VERSION && ARM_SMCCC_IS_64(res->a0)) { > > > + cpu_reg(ctxt, 8) = res->a8; > > > + cpu_reg(ctxt, 9) = res->a9; > > > + cpu_reg(ctxt, 10) = res->a10; > > > + cpu_reg(ctxt, 11) = res->a11; > > > + cpu_reg(ctxt, 12) = res->a12; > > > + cpu_reg(ctxt, 13) = res->a13; > > > + cpu_reg(ctxt, 14) = res->a14; > > > + cpu_reg(ctxt, 15) = res->a15; > > > + cpu_reg(ctxt, 16) = res->a16; > > > + cpu_reg(ctxt, 17) = res->a17; > > > + } > > > } > > > > I don't see how that can ever work. > > > > Irrespective of how FFA_MSG_WAIT actually works (and I couldn't find > > anything in the spec that supports the above), the requester will > > fully expect its registers to be preserved based on the initial > > function type, and that alone. No ifs, no buts. > > > > If what you describe can happen (please provide a convincing example), > > then the spec is doomed. > > DEN0077A 1.2 Section 8.2 (Runtime Model for FFA_RUN) and 8.3 (Runtime Model > for Direct request ABIs) contains Figures 8.1 and 8.2. Each figure shows > transitions between states "waiting", "blocked", "running", and "preempted". > Key to my understanding is that the waiting state in Figure 8.1 and Figure > 8.2 is the exact same state. This appears to be the case per Section 4.10. > > So we have to consider the ways to get in and out of the waiting state by > joining the state machines in Figures 8.1 and 8.2. Figure 8.1 has an edge > between "running" and "waiting" caused by FFA_MSG_WAIT. Figure 8.2 has an > edge between "waiting" and "running" caused by a "Direct request ABI". > > Direct request ABIs include FFA_MSG_SEND_DIRECT_REQ2 which is why the FF-A > 1.2 spec, in my read, permits the response to a 32-bit FFA_MSG_WAIT call can > be a 64-bit FFA_MSG_SEND_DIRECT_REQ2 reply. That seems bonkers to me and I agree with Marc's assessment above. The SMCCC is crystal clear that a caller using the SM32/HVC32 calling convention can rely on x8-x30 (as well as the stack pointers) being preserved. If FF-A has a problem with that then it needs to be fixed there and not bodged in Linux. Setting that aside, I'm still not sure I follow this part of your check: if (... && ARM_SMCCC_IS_64(res->a0)) won't res->a0 contain something like FFA_SUCCESS? The FFA spec states: FFA_SUCCESS64, is used only if any result register encodes a 64-bit parameter. which doesn't really seem related to whether or not the initial call was using SMC32 or SMC64. What am I missing? Will