[PATCH v7 2/5] KVM: arm64: Use SMCCC 1.2 for FF-A initialization and in host handler

[Will Deacon]

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 <devnull+perlarsen.google.com at kernel.org> 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:
> > 
> > <quote>
> > Registers W4-W7 must be preserved unless they contain results, as
> > specified in the function definition.
> > </quote>
> > 
> > On what grounds can you blindly change these registers?
> From DEN0077A 1.2 Section 11.2: Reserved parameter convention
> 
> <quote>
> 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.
> </quote>
> 
> 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