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

[Sudeep Holla <sudeep.holla@arm.com>]

On Tue, Jul 22, 2025 at 04:55:31PM +0100, Will Deacon wrote:
> [Sudeep & Mark to To:]
> 
> On Mon, Jul 21, 2025 at 05:20:01PM -0700, Per Larsen wrote:
> > On 7/21/25 4:01 AM, Arve Hjønnevåg wrote:
> > > On Fri, Jul 18, 2025 at 10:54 PM Per Larsen <perl@immunant.com> wrote:
> > > > On 7/18/25 6:37 AM, Will Deacon wrote:
> > > > > 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@kernel.org> wrote:
> > > > > > > > +  /*
> > > > > > > > +   * 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.
> > > > Ack. Patchset v8 addresses Marc's feedback by using the func_id detect
> > > > SMC64 instead.>
> > > > > 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?I agree that we cannot use res->a0 to distinguish SMC32/SMC64 for the
> > > > reason you stated.
> > > 
> > > I don't think using the function-id of the original call works
> > > correctly in this context though. If you look at
> > > drivers/firmware/arm_ffa/driver.c:ffa_msg_send_direct_req2 it has the
> > > same problem as the FFA_MSG_WAIT example in your comment. In the
> > > simple case it will use FFA_MSG_SEND_DIRECT_REQ2 for the call and
> > > FFA_MSG_SEND_DIRECT_RESP2 for the response, both 64 bit opcodes, and
> > > either approach here will have the same correct result. However if
> > > FFA_MSG_SEND_DIRECT_REQ2 responds with FFA_INTERRUPT or FFA_YIELD,
> > > then the driver will resume the call with FFA_RUN, a 32 bit opcode,
> > > and still expect a 64 bit FFA_MSG_SEND_DIRECT_RESP2 response with a
> > > full response in x4-17. If you use ARM_SMCCC_IS_64(func_id) here
> > > instead of ARM_SMCCC_IS_64(res->a0), then the part of response in
> > > x8-x17 will be lost.
> 
> Can't we just update the FF-A driver? This is clearly all the result of
> a half-baked spec...
> 
> Sudeep -- any chance you can add support for the hot-off-the-press
> 64-bit FFA_RUN call to the Linux driver, please? Without that, I don't
> see how the REQ2/RESP2 calls can work properly.
> 

Apologies for the delay in following up on this thread. Yes, we can certainly
add the 64-bit FFA_RUN, but we'll need to align with the spec authors first.
I'll follow up internally to ensure there's no conflict between how it's
defined in the spec and how it's used in the kernel.

It looks like the change in the spec is already WIP which I was made aware
just now, so I don't see any issue updating the driver.

-- 
Regards,
Sudeep