Received-SPF: pass (google.com: domain of linux-kernel+bounces-167893-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Fri, 3 May 2024 17:21:14 +0100
From: Will Deacon <will@kernel.org>
To: Sebastian Ene <sebastianene@google.com>
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: <20240503162114.GA18789@willie-the-truck>
References: <20240418163025.1193763-2-sebastianene@google.com>
 <20240418163025.1193763-3-sebastianene@google.com>
 <20240503143937.GA18656@willie-the-truck>
 <ZjUCyGoptCcIoGpU@google.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZjUCyGoptCcIoGpU@google.com>
User-Agent: Mutt/1.10.1 (2018-07-13)

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 <sebastianene@google.com>
> > > ---
> > >  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?

> > > @@ -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.

> > > +	/*
> > > +	 * 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)
  2. Secure implements 1.2 and so returns 1.2 but remains in 1.0
     compatability mode for the data structure formats.
  3. The host issues FFA_VERSION(1.1)
  4. The code above then issues FFA_VERSION(1.1) to Secure and it
     switches over to the 1.1 data structures

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.

Will