Re: [PATCH v2 3/4] KVM: arm64: Map the hypervisor FF-A buffers on ffa init

[Will Deacon]

On Tue, Mar 04, 2025 at 05:38:02PM +0000, Sebastian Ene wrote:
> On Tue, Mar 04, 2025 at 01:56:35AM +0000, Will Deacon wrote:
> > On Tue, Mar 04, 2025 at 12:53:25AM +0000, Sebastian Ene wrote:
> > > On Mon, Mar 03, 2025 at 11:43:03PM +0000, Will Deacon wrote:
> > > > On Thu, Feb 27, 2025 at 06:17:48PM +0000, Sebastian Ene wrote:
> > > > > Map the hypervisor's buffers irrespective to the host and return
> > > > > a linux error code from the FF-A error code on failure. Remove
> > > > > the unmap ff-a buffers calls from the hypervisor as it will
> > > > > never be called.
> > > > > Prevent the host from using FF-A directly with Trustzone
> > > > > if the hypervisor could not map its own buffers.
> > > > > 
> > > > > Signed-off-by: Sebastian Ene <sebastianene@xxxxxxxxxx>
> > > > > ---
> > > > >  arch/arm64/kvm/hyp/nvhe/ffa.c | 46 +++++++++++++----------------------
> > > > >  1 file changed, 17 insertions(+), 29 deletions(-)
> > > > 
> > > > [...]
> > > > 
> > > > > @@ -861,6 +842,7 @@ int hyp_ffa_init(void *pages)
> > > > >  {
> > > > >  	struct arm_smccc_res res;
> > > > >  	void *tx, *rx;
> > > > > +	int ret;
> > > > >  
> > > > >  	if (kvm_host_psci_config.smccc_version < ARM_SMCCC_VERSION_1_2)
> > > > >  		return 0;
> > > > > @@ -911,5 +893,11 @@ int hyp_ffa_init(void *pages)
> > > > >  		.lock	= __HYP_SPIN_LOCK_UNLOCKED,
> > > > >  	};
> > > > >  
> > > > > +	/* Map our hypervisor buffers into the SPMD */
> > > > > +	ret = ffa_map_hyp_buffers();
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > 
> > > > Doesn't calling RXTX_MAP here undo the fix from c9c012625e12 ("KVM:
> > > > arm64: Trap FFA_VERSION host call in pKVM") where we want to allow for
> > > > the host to negotiate the version lazily?
> > > 
> > > We still have the same behaviour where we don't allow memory
> > > sharing to happen until the version is negotiated but this
> > > separates the hypervisor buffer mapping part from the host.
> > 
> > Sadly, the spec doesn't restrict this to the memory sharing calls:
> > 
> >   | [...] negotiation of the version must happen before an invocation of
> >   | any other FF-A ABI
> > 
> 
> We do that, as the hypervisor negotiates its own version in
> hyp_ffa_init.

hyp_ffa_init() only issues FFA_VERSION afaict, which is the one call
that you're allowed to make during negotiation. So the existing code is
fine.

> I think the host shouldn't be allowed to overwrite the
> hyp_ffa_version obtained from _init, this feels wrong as you
> can have a driver that forcefully downgrades the hypervisor to an old
> version.

I think that's also fine. The FFA code in the hypervisor exists solely
to proxy requests from the host; it's not used for anything else and so,
from the host's persective, FFA should behave identically to the case in
which the proxy is not present (e.g. if we were just using VHE). That
means that we're doing the right thing by deferring to the host for
version negotation.

Are you saying there's a bug in the current code if the host negotiates
the downgrade?

> We need to do three things, Sudeep & Will please correct me if I am
> wrong, but this is how I see it:
> 
> - the hypervisor should act as a separate entity (it has a different ID and
> in the current implementation we don't do a distinction between host/hyp) and
> it should be able to lock its own version from init.

I strongly disagree with that. The hypervisor isn't using FFA for
anything other than proxying the host and so we don't need to negotiate
a separate version.

What would we gain by doing this? Is there a bug with what we're doing
at the moment?

> - keep a separate version negotiated for the host
> - trap FFA_ID_GET from the host and return ID=1 because
>   currently we forward the call to the TZ and it returns the same ID
>   as the (hypervisor == 0).

Why is this beneficial? It just looks like complexity at EL2 for no gain
to me, but maybe I'm missing something.

Will