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 227EBC10F1A for ; Tue, 7 May 2024 09:17:46 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=BfgxXBsur2g4IzIumxQ8Upm5O0DnN6cO7on+kIkuHrk=; b=eVmZGZf94BlZ5w XR5gjnH+5TZlRAEKFX6Ht3QnoYxp27URkcpDk+r5wLRVp81N9aUjxGY5ZNBj9xO2rvrBp9Mb8TTUW D1oGJDlTrWFczjHSE7YnB997gqB6vLiuQMBQun7ifkcijxwiIcEEpQosqi9jDVUzV3CUMItuJbWSW YCvTPsIAO+sW1VqLRokHrtqGxrpsgDUCyvyVigcJQt1U2cW5VR4FWVbpeSiLDm6i2jiSh9CNKFOoV nZtgdCoZArJZghgYd6KbkFb3y+a9fM/WMhzO6R2BVQSLnoIppvssS3ycuLA1cPwp21g1oLt1JygDT A6Tjedi90UhO7YXtAJSw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1s4Gww-0000000AGUz-3Oa1; Tue, 07 May 2024 09:17:26 +0000 Received: from mail-wm1-x331.google.com ([2a00:1450:4864:20::331]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1s4Gwt-0000000AGSJ-1YGx for linux-arm-kernel@lists.infradead.org; Tue, 07 May 2024 09:17:25 +0000 Received: by mail-wm1-x331.google.com with SMTP id 5b1f17b1804b1-41a428374b9so39615e9.1 for ; Tue, 07 May 2024 02:17:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1715073438; x=1715678238; darn=lists.infradead.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=ANuDqw0S/PQRyRd8MNS09imR727D3AyKqo3jQsOx02k=; b=QGIGxU7QHVsdSqARCIp5BYWNf7jP0L7KlBdL8ygq3h5cp1aPVPOA0+iBT+0VggH4uF WGXZdZ/jLqLcSBXV3GjucUwr4J2NDLYzErKQIIKBd52j/LQ/xISryb+sVQnwxviKrSiq naFG7sm146xh/tKQ2m+B96x0uTufVTZmJBDhJEgS+TO9KeQoJATjsbGo+xf/iClXkRHn utbGX1RDYnesFsGHaU4mtplwp7eHIh4qYS9QeX9gDrjztJ4EANC1JUTjVHZ7RLvHOxfX JX+8ldOi7dT8IcjV4y/7bKN0tPnaaiCN2LX40bzE6hNtt3wpYpIFKfK0sJuEpKgWdUHQ WTjA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1715073438; x=1715678238; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=ANuDqw0S/PQRyRd8MNS09imR727D3AyKqo3jQsOx02k=; b=dDs0Dx3VkvXU2Ot+U7t44TLv9CbSlP8On7bZ4VVjhvAuKPu/cCytIKwCu+/gr7imdE mP7zB2JGo0PFpZq+TejvxHtsF56Vm5nsIyQjzaIZ8KnAKUSDOUPugQ34cobXfqJRKiu6 4n8+c5gyGVNyGlt3qsMwAmbHkwuBrjEn+WNT70JMG2Cbq7LdsC3I6UF+9eE69Y2YGuKA vhYMdStj/SxNlJMAZPVsmtzMdlOwtymPrY4Z7EqXXJP4WZ2mZv+ra+sHQIHTYFvhqGxE czrTy99CqNEAE0CbHURojkk27hDEQztiEZAkCPFQwszjMO3oswTrFreqMqEEM1JBoedk rvOA== X-Forwarded-Encrypted: i=1; AJvYcCWIAyeGPJ0ivtpFwBQgEyUtEsEbfuo/CJCfvzJriTxKJBB4fXZyi/XfnE3QWEHQTa2TKMM6ej02rvyP58Q/FV+q9BaXsrmUTQDGKYDoV/hBV7Wf4nU= X-Gm-Message-State: AOJu0YxfxhrApVdA1WVZqcnVOcLHOjZihRJWTjO8swAwY22iLlplzVrN 9lKL9TdiDY5KkLyR/8i+vL/pIE0nKyY+R4BZ7PIFPIi2aPQo0J9z/RaTMWORkQ== X-Google-Smtp-Source: AGHT+IFWa2DxPUzXtMTAIuzZ6k+JU9PX5c/LNCxxDXptEavvPHor3Swu8oygcQw0lxjLZwMh7NnFug== X-Received: by 2002:a05:600c:1c92:b0:41c:ab7:f9af with SMTP id 5b1f17b1804b1-41f3bdccb7dmr1044045e9.3.1715073438012; Tue, 07 May 2024 02:17:18 -0700 (PDT) Received: from google.com (216.131.76.34.bc.googleusercontent.com. [34.76.131.216]) by smtp.gmail.com with ESMTPSA id k5-20020a05600c1c8500b0041bab13cd74sm18931185wms.17.2024.05.07.02.17.17 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 07 May 2024 02:17:17 -0700 (PDT) Date: Tue, 7 May 2024 09:17:15 +0000 From: Sebastian Ene To: Will Deacon Cc: catalin.marinas@arm.com, james.morse@arm.com, jean-philippe@linaro.org, maz@kernel.org, oliver.upton@linux.dev, qperret@google.com, qwandor@google.com, sudeep.holla@arm.com, suzuki.poulose@arm.com, tabba@google.com, yuzenghui@huawei.com, lpieralisi@kernel.org, kvmarm@lists.linux.dev, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, kernel-team@android.com Subject: Re: [PATCH 1/4] KVM: arm64: Trap FFA_VERSION host call in pKVM Message-ID: References: <20240418163025.1193763-2-sebastianene@google.com> <20240418163025.1193763-3-sebastianene@google.com> <20240503143937.GA18656@willie-the-truck> <20240503162114.GA18789@willie-the-truck> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240503162114.GA18789@willie-the-truck> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240507_021723_489250_FCEAF738 X-CRM114-Status: GOOD ( 70.64 ) 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: , 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 On Fri, May 03, 2024 at 05:21:14PM +0100, Will Deacon wrote: > On Fri, May 03, 2024 at 03:29:12PM +0000, Sebastian Ene wrote: > > On Fri, May 03, 2024 at 03:39:38PM +0100, Will Deacon wrote: > > > On Thu, Apr 18, 2024 at 04:30:23PM +0000, Sebastian Ene wrote: > > > > The pKVM hypervisor initializes with FF-A version 1.0. Keep the > > > > supported version inside the host structure and prevent the host > > > > drivers from overwriting the FF-A version with an increased version. > > > > Without trapping the call, the host drivers can negotiate a higher > > > > version number with TEE which can result in a different memory layout > > > > described during the memory sharing calls. > > > > > > > > Signed-off-by: Sebastian Ene > > > > --- > > > > arch/arm64/kvm/hyp/nvhe/ffa.c | 43 ++++++++++++++++++++++++++++++++--- > > > > 1 file changed, 40 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c > > > > index 320f2eaa14a9..023712e8beeb 100644 > > > > --- a/arch/arm64/kvm/hyp/nvhe/ffa.c > > > > +++ b/arch/arm64/kvm/hyp/nvhe/ffa.c > > > > @@ -58,6 +58,7 @@ struct kvm_ffa_buffers { > > > > hyp_spinlock_t lock; > > > > void *tx; > > > > void *rx; > > > > + u32 ffa_version; > > > > }; > > > > > > Why should this be part of 'struct kvm_ffa_buffers'? The host, proxy and > > > Secure side will end up using the same version, so a simple global > > > variable would suffice, no? > > > > > I prefer keeping it here as we will have more clients in the future / > > different VMs and each one of them will have its own version and its own > > pair of buffers. > > We can add that when we need to. Let's keep it simple for now, as the > idea of the proxy having to support multiple versions of the spec at > once sounds terrifying to me. I don't think we're going to want to > re-marshall the data structures between the 1.0 and 1.1 formats, are we? > I don't think we increase the complexity of the code by keeping this argument in the structure. The code in nvhe/ffa.c supports marshalling the structure as of [this change](https://lore.kernel.org/r/20231005-ffa_v1-1_notif-v4-14-cddd3237809c@arm.com ) and that is why I was in favor of keeping the version where it belongs to. > > > > @@ -640,6 +641,39 @@ static bool do_ffa_features(struct arm_smccc_res *res, > > > > return true; > > > > } > > > > > > > > +static void do_ffa_version(struct arm_smccc_res *res, > > > > + struct kvm_cpu_context *ctxt) > > > > +{ > > > > + DECLARE_REG(u32, ffa_req_version, ctxt, 1); > > > > + u32 current_version; > > > > + > > > > + hyp_spin_lock(&host_buffers.lock); > > > > > > Why do you need to take the lock for this? > > > > > > > Because we interpret the host buffer content based on the version that we > > end up setting here and each time we are accessing these buffers we are > > protected by this lock. > > I think that's indicative of a broader issue, though, which is that you > allow for the version to be re-negotiated at runtime. The spec doesn't > allow that and I don't think we should either. > The spec talks about interopeartion in case two versions (x,y) and (a,b) want to talk: - given the pairs (x,y) and (a,b) x=major, y=minor if x == a and y > b the versions are incompatible until y downgrades its version such that y <= b. >From this I drew the conclusion that the spec allows the re-negotiation at runtime, please let me know if you see things differently. > > > > + /* > > > > + * If the client driver tries to downgrade the version, we need to ask > > > > + * first if TEE supports it. > > > > + */ > > > > + if (FFA_MINOR_VERSION(ffa_req_version) < FFA_MINOR_VERSION(current_version)) { > > > > > > Similarly here, I don't think 'current_version' is what we should expose. > > > Rather, we should be returning the highest version that the proxy > > > supports in the host, which is 1.0 at this point in the patch series. > > > > We already report the highest version that the proxy supports on line: > > `res->a0 = current_version;` > > > > 'current_version' is assigned during proxy initialization. > > This check allows us to downgrade the supported ffa_version if the Host > > requested it and only if TF-A supports it. > > I don't think we want the host negotiating directly with the Secure side, > though, do we? 'current_version' is initialised to whatever the Secure > side gives us, so if we had something like: > > 1. Proxy initialises, issues FFA_VERSION(1.0) This will save 1.0 in host_buffers.ffa_version > 2. Secure implements 1.2 and so returns 1.2 but remains in 1.0 > compatability mode for the data structure formats. Ack. > 3. The host issues FFA_VERSION(1.1) The call is trapped and we verify if the requested version (FFA_VERSION(1.1) is smaller than our current_version saved in step 1. Given that is not smaller we only reply with our current supported version which is FFA_VERSION(1.0) and we return to the host. > 4. The code above then issues FFA_VERSION(1.1) to Secure and it > switches over to the 1.1 data structures This was happening prior to my patch, so in a way this patch is a bugfix. We get this behavior without trapping and interpretting of the FFA_VERSION API. > > Did I get that right? > > I really think we need to settle on a single version for the host, > hypervisor and Secure and then stick with it following a single > negotiation stage. > > > > > + arm_smccc_1_1_smc(FFA_VERSION, ffa_req_version, 0, > > > > + 0, 0, 0, 0, 0, > > > > + res); > > > > > > Hmm, I'm struggling to see how this is supposed to work per the spec. > > > The FF-A spec says: > > > > > > | ... negotiation of the version must happen before an invocation of > > > | any other FF-A ABI. > > > > I think that is a bit vague in my opinion but what I get is that the first call > > executed should always be the get version ff-a call. > > > > > > > > and: > > > > > > | Once the caller invokes any FF-A ABI other than FFA_VERSION, the > > > | version negotiation phase is complete. > > > | > > > | Once an FF-A version has been negotiated between a caller and a > > > | callee, the version may not be changed for the lifetime of the > > > | calling component. The callee must treat the negotiated version as > > > | the only supported version for any subsequent interactions with the > > > | caller.> > > > So by the time we get here, we've already settled on our version with > > > the Secure side and the host cannot downgrade. > > > > At this stage I think the spec didn't take into account that there can be a hypervisor > > in between. > > Well, that's what the spec says and I think we need to follow it. I can > well imagine that the Secure side won't allow the version to be > re-negotiated on the fly and I don't think we'd want that in the proxy, > either. > > > > That's a bit rubbish if you ask me, but I think it means we'll have to > > > defer some of the proxy initialisation until the host calls FFA_VERSION, > > > at which point we'll need to negotiate a common version between the host, > > > the proxy and Secure. Once we've done that, our FFA_VERSION handler will > > > just return that negotiated version. > > > > We are already doing this when the ARM driver is built as an external > > module. If it is not as an external module and is builtin we have a > > bigger issue because it loads before pKVM at subsys_initcall. This means > > that we won't trap FFA_MAP* and other setup calls. > > Sorry, I don't follow. hyp_ffa_init() issues FFA_FEATURES immediately > after FFA_VERSION, so we terminate the negotiation right away. Sorry I confused you, I am afraid I was trying to desribe a different issue here which is related to how early the ARM FF-A driver initializes when is builtin - it is before the hypervisor proxy is installed. > > Will > > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com. > Thank you, Seb _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel