* [PATCH v7 0/5] KVM: arm64: Support FF-A 1.2 and SEND_DIRECT2 ABI
@ 2025-07-01 22:06 Per Larsen via B4 Relay
2025-07-01 22:06 ` [PATCH v7 1/5] KVM: arm64: Correct return value on host version downgrade attempt Per Larsen via B4 Relay
` (4 more replies)
0 siblings, 5 replies; 20+ messages in thread
From: Per Larsen via B4 Relay @ 2025-07-01 22:06 UTC (permalink / raw)
To: Marc Zyngier, Oliver Upton, Joey Gouly, Suzuki K Poulose,
Zenghui Yu, Catalin Marinas, Will Deacon, Sudeep Holla
Cc: linux-arm-kernel, kvmarm, linux-kernel, ahomescu, armellel, arve,
ayrton, qperret, sebastianene, qwandor, Per Larsen
Hi,
The FF-A 1.2 specification introduces a new SEND_DIRECT2 ABI which
allows registers x4-x17 to be used for the message payload. This patch
set prevents the host from using a lower FF-A version than what has
already been negotiated with the hypervisor. This is necessary because
the hypervisor does not have the necessary compatibility paths to
translate from the hypervisor FF-A version to a previous version.
Support for FF-A 1.2 in the hypervisor is added as a precursor to the
addition of the FFA_MSG_SEND_DIRECT_REQ2 messaging interface. Notably,
this patch updates all smc calls to use SMCCC 1.2 as it makes it simpler
to support interfaces that need to accept more than 8 arguments and/or
return more than 4 values. The list of optional/unsupported interfaces
was updated to reflect additions in FF-A 1.2.
Tested by booting Android under QEMU and loading Trusty as the guest
VM and observing the SEND_DIRECT2 ABI being used successfully during
guest boot.
Changes in v7:
- 0/6: Revised cover letter slightly
- 4/6: ffa_call_supported: Clarify optional interfaces
hyp_ffa_post_init: Sanitize MBZ bits in x2 and x3
address nit: use GENMASK
- Link to v6: https://lore.kernel.org/r/20250627-virtio-msg-ffa-v6-0-8c02fd94edac@google.com
Changes since v5:
- 3/6 -> 2/5: Merged with parent patch as they were closely related
Don't rename variables from "res" to "regs" to avoid churn
Update and clarify reasoning for condition added to ffa_set_retval
Adopt style suggested by Marc when passing regs to arm_smccc_1_2_smc
Fix inadvertent duplicate statements in ffa_{map,unmap}_hyp_buffers
- Link to v5: https://lore.kernel.org/r/20250619-virtio-msg-ffa-v5-0-412c98558807@google.com
Changes since v4:
- 1/5 -> 1/6: Added Acked-by: Will Deacon <will@kernel.org>
Reworded patch subject and description
- 2/5: Dropped patch: zero x4-x7 in ffa_set_retval
- 2/6: New: use SMCCC 1.2 during hyp init
- 3/6: New: use SMCCC 1.2 in host FF-A handler
- 3/5 -> 4/6: No change; can revisit denylist design in later patchset
Added Acked-by: Will Deacon <will@kernel.org>
- 4/5 -> 5/6: No change
- 5/5 -> 6/6: No change
- Link to v4: https://lore.kernel.org/r/20250516-virtio-msg-ffa-v4-0-580ee70e5081@google.com
Changes since v3:
- all: Remove Signed-off-by: Per Larsen <perl@immunant.com>
- 2/5: Split out from 2/3, zero x4-x7 in set_ffa_retval
- 3/5: Split out from 2/3, Mark FFA_NOTIFICATION_* calls as
unsupported
- 2/3 -> 4/5: Drop ffa_get_hypervisor_version, access hyp_ffa_version
directly when version negotiation is complete.
- 3/3 -> 5/5: Call ffa_to_smccc_1_2_error directly in
do_do_ffa_direct_msg2
Push call to ffa_call_needs_smccc_1_2 lower down by adding
ffa_get_func_id and ffa_set_retval_smccc_1_x.
Drop ffa_to_smccc_1_2_regs and ffa_to_smccc_1_2_regs_prop
as they are no longer used.
- Link to v3: https://lore.kernel.org/r/20250513-virtio-msg-ffa-v3-0-d66c76ff1b2c@google.com
Changes since v2:
- 2/3: Removed files added by mistake.
Add and use ffa_get_hypervisor_version to access hyp_ffa_version
- 3/3: Use ffa_get_hypervisor_version to access hyp_ffa_version safely
- Link to v2: https://lore.kernel.org/r/20250508-virtio-msg-ffa-v2-0-ed84f8053965@google.com
Changes since v1:
- 1/3: Simplify commit message; drop long comment in do_ffa_version
- 2/3: Correct use of Co-developed-by: footer
s/arm_smccc_1_2_smc_fallback/arm_smccc_1_x_smc/
Always access hyp_ffa_version w/lock held
Remove superfluous comments in ffa_call_supported
Add and use FFA_FEAT_RXTX_MIN_SZ_MASK instead of constant
Add FFA_PARTITION_INFO_GET_REGS to calls that require SMCCC 1.2
- 3/3: Always access hyp_ffa_version w/lock held
Correct formatting
Thanks,
Per Larsen
--
2.49.0
---
Per Larsen (5):
KVM: arm64: Correct return value on host version downgrade attempt
KVM: arm64: Use SMCCC 1.2 for FF-A initialization and in host handler
KVM: arm64: Mark FFA_NOTIFICATION_* calls as unsupported
KVM: arm64: Bump the supported version of FF-A to 1.2
KVM: arm64: Support FFA_MSG_SEND_DIRECT_REQ2 in host handler
arch/arm64/kvm/hyp/nvhe/Makefile | 1 +
arch/arm64/kvm/hyp/nvhe/ffa.c | 244 +++++++++++++++++++++++++++------------
include/linux/arm_ffa.h | 3 +
3 files changed, 175 insertions(+), 73 deletions(-)
---
base-commit: 66701750d5565c574af42bef0b789ce0203e3071
change-id: 20250506-virtio-msg-ffa-22af72c92150
Best regards,
--
Per Larsen <perlarsen@google.com>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v7 1/5] KVM: arm64: Correct return value on host version downgrade attempt
2025-07-01 22:06 [PATCH v7 0/5] KVM: arm64: Support FF-A 1.2 and SEND_DIRECT2 ABI Per Larsen via B4 Relay
@ 2025-07-01 22:06 ` Per Larsen via B4 Relay
2025-07-01 22:06 ` [PATCH v7 2/5] KVM: arm64: Use SMCCC 1.2 for FF-A initialization and in host handler Per Larsen via B4 Relay
` (3 subsequent siblings)
4 siblings, 0 replies; 20+ messages in thread
From: Per Larsen via B4 Relay @ 2025-07-01 22:06 UTC (permalink / raw)
To: Marc Zyngier, Oliver Upton, Joey Gouly, Suzuki K Poulose,
Zenghui Yu, Catalin Marinas, Will Deacon, Sudeep Holla
Cc: linux-arm-kernel, kvmarm, linux-kernel, ahomescu, armellel, arve,
ayrton, qperret, sebastianene, qwandor, Per Larsen
From: Per Larsen <perlarsen@google.com>
Once the hypervisor negotiates the FF-A version with the host, it should
remain locked-in. However, it is possible to load FF-A as a module first
supporting version 1.1 and then 1.0.
Without this patch, the FF-A 1.0 driver will use 1.0 data structures to
make calls which the hypervisor will incorrectly interpret as 1.1 data
structures. With this patch, negotiation will fail.
This patch does not change existing functionality in the case where a
FF-A 1.2 driver is loaded after a 1.1 driver; the 1.2 driver will need
to use 1.1 in order to proceed.
Acked-by: Will Deacon <will@kernel.org>
Signed-off-by: Per Larsen <perlarsen@google.com>
---
arch/arm64/kvm/hyp/nvhe/ffa.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c
index 3369dd0c4009f84ad3cf9481c747bdc57a162370..2c199d40811efb5bfae199c4a67d8ae3d9307357 100644
--- a/arch/arm64/kvm/hyp/nvhe/ffa.c
+++ b/arch/arm64/kvm/hyp/nvhe/ffa.c
@@ -712,7 +712,10 @@ static void do_ffa_version(struct arm_smccc_res *res,
hyp_spin_lock(&version_lock);
if (has_version_negotiated) {
- res->a0 = hyp_ffa_version;
+ if (FFA_MINOR_VERSION(ffa_req_version) < FFA_MINOR_VERSION(hyp_ffa_version))
+ res->a0 = FFA_RET_NOT_SUPPORTED;
+ else
+ res->a0 = hyp_ffa_version;
goto unlock;
}
--
2.50.0.727.gbf7dc18ff4-goog
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v7 2/5] KVM: arm64: Use SMCCC 1.2 for FF-A initialization and in host handler
2025-07-01 22:06 [PATCH v7 0/5] KVM: arm64: Support FF-A 1.2 and SEND_DIRECT2 ABI Per Larsen via B4 Relay
2025-07-01 22:06 ` [PATCH v7 1/5] KVM: arm64: Correct return value on host version downgrade attempt Per Larsen via B4 Relay
@ 2025-07-01 22:06 ` Per Larsen via B4 Relay
2025-07-03 12:38 ` Marc Zyngier
2025-07-01 22:06 ` [PATCH v7 3/5] KVM: arm64: Mark FFA_NOTIFICATION_* calls as unsupported Per Larsen via B4 Relay
` (2 subsequent siblings)
4 siblings, 1 reply; 20+ messages in thread
From: Per Larsen via B4 Relay @ 2025-07-01 22:06 UTC (permalink / raw)
To: Marc Zyngier, Oliver Upton, Joey Gouly, Suzuki K Poulose,
Zenghui Yu, Catalin Marinas, Will Deacon, Sudeep Holla
Cc: linux-arm-kernel, kvmarm, linux-kernel, ahomescu, armellel, arve,
ayrton, qperret, sebastianene, qwandor, Per Larsen
From: Per Larsen <perlarsen@google.com>
SMCCC 1.1 and prior allows four registers to be sent back as a result
of an FF-A interface. SMCCC 1.2 increases the number of results that can
be sent back to 8 and 16 for 32-bit and 64-bit SMC/HVCs respectively.
FF-A 1.0 references SMCCC 1.2 (reference [4] on page xi) and FF-A 1.2
explicitly requires SMCCC 1.2 so it should be safe to use this version
unconditionally. Moreover, it is simpler to implement FF-A features
without having to worry about compatibility with SMCCC 1.1 and older.
Update the FF-A initialization and host handler code to use SMCCC 1.2.
Signed-off-by: Per Larsen <perlarsen@google.com>
---
arch/arm64/kvm/hyp/nvhe/Makefile | 1 +
arch/arm64/kvm/hyp/nvhe/ffa.c | 193 +++++++++++++++++++++++++--------------
2 files changed, 125 insertions(+), 69 deletions(-)
diff --git a/arch/arm64/kvm/hyp/nvhe/Makefile b/arch/arm64/kvm/hyp/nvhe/Makefile
index a76522d63c3e630795db5972a99abc3d24bc5e26..f859a8fb41a25effea1edd977bef889423153399 100644
--- a/arch/arm64/kvm/hyp/nvhe/Makefile
+++ b/arch/arm64/kvm/hyp/nvhe/Makefile
@@ -27,6 +27,7 @@ hyp-obj-y := timer-sr.o sysreg-sr.o debug-sr.o switch.o tlb.o hyp-init.o host.o
cache.o setup.o mm.o mem_protect.o sys_regs.o pkvm.o stacktrace.o ffa.o
hyp-obj-y += ../vgic-v3-sr.o ../aarch32.o ../vgic-v2-cpuif-proxy.o ../entry.o \
../fpsimd.o ../hyp-entry.o ../exception.o ../pgtable.o
+hyp-obj-y += ../../../kernel/smccc-call.o
hyp-obj-$(CONFIG_LIST_HARDENED) += list_debug.o
hyp-obj-y += $(lib-objs)
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;
+
+ /*
+ * 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;
+ }
}
static bool is_ffa_call(u64 func_id)
@@ -113,82 +145,92 @@ static bool is_ffa_call(u64 func_id)
static int ffa_map_hyp_buffers(u64 ffa_page_count)
{
- struct arm_smccc_res res;
+ struct arm_smccc_1_2_regs res;
- arm_smccc_1_1_smc(FFA_FN64_RXTX_MAP,
- hyp_virt_to_phys(hyp_buffers.tx),
- hyp_virt_to_phys(hyp_buffers.rx),
- ffa_page_count,
- 0, 0, 0, 0,
- &res);
+ arm_smccc_1_2_smc(&(struct arm_smccc_1_2_regs) {
+ .a0 = FFA_FN64_RXTX_MAP,
+ .a1 = hyp_virt_to_phys(hyp_buffers.tx),
+ .a2 = hyp_virt_to_phys(hyp_buffers.rx),
+ .a3 = ffa_page_count,
+ }, &res);
return res.a0 == FFA_SUCCESS ? FFA_RET_SUCCESS : res.a2;
}
static int ffa_unmap_hyp_buffers(void)
{
- struct arm_smccc_res res;
+ struct arm_smccc_1_2_regs res;
- arm_smccc_1_1_smc(FFA_RXTX_UNMAP,
- HOST_FFA_ID,
- 0, 0, 0, 0, 0, 0,
- &res);
+ arm_smccc_1_2_smc(&(struct arm_smccc_1_2_regs) {
+ .a0 = FFA_RXTX_UNMAP,
+ .a1 = HOST_FFA_ID,
+ }, &res);
return res.a0 == FFA_SUCCESS ? FFA_RET_SUCCESS : res.a2;
}
-static void ffa_mem_frag_tx(struct arm_smccc_res *res, u32 handle_lo,
+static void ffa_mem_frag_tx(struct arm_smccc_1_2_regs *res, u32 handle_lo,
u32 handle_hi, u32 fraglen, u32 endpoint_id)
{
- arm_smccc_1_1_smc(FFA_MEM_FRAG_TX,
- handle_lo, handle_hi, fraglen, endpoint_id,
- 0, 0, 0,
- res);
+ arm_smccc_1_2_smc(&(struct arm_smccc_1_2_regs) {
+ .a0 = FFA_MEM_FRAG_TX,
+ .a1 = handle_lo,
+ .a2 = handle_hi,
+ .a3 = fraglen,
+ .a4 = endpoint_id,
+ }, res);
}
-static void ffa_mem_frag_rx(struct arm_smccc_res *res, u32 handle_lo,
+static void ffa_mem_frag_rx(struct arm_smccc_1_2_regs *res, u32 handle_lo,
u32 handle_hi, u32 fragoff)
{
- arm_smccc_1_1_smc(FFA_MEM_FRAG_RX,
- handle_lo, handle_hi, fragoff, HOST_FFA_ID,
- 0, 0, 0,
- res);
+ arm_smccc_1_2_smc(&(struct arm_smccc_1_2_regs) {
+ .a0 = FFA_MEM_FRAG_RX,
+ .a1 = handle_lo,
+ .a2 = handle_hi,
+ .a3 = fragoff,
+ .a4 = HOST_FFA_ID,
+ }, res);
}
-static void ffa_mem_xfer(struct arm_smccc_res *res, u64 func_id, u32 len,
+static void ffa_mem_xfer(struct arm_smccc_1_2_regs *res, u64 func_id, u32 len,
u32 fraglen)
{
- arm_smccc_1_1_smc(func_id, len, fraglen,
- 0, 0, 0, 0, 0,
- res);
+ arm_smccc_1_2_smc(&(struct arm_smccc_1_2_regs) {
+ .a0 = func_id,
+ .a1 = len,
+ .a2 = fraglen,
+ }, res);
}
-static void ffa_mem_reclaim(struct arm_smccc_res *res, u32 handle_lo,
+static void ffa_mem_reclaim(struct arm_smccc_1_2_regs *res, u32 handle_lo,
u32 handle_hi, u32 flags)
{
- arm_smccc_1_1_smc(FFA_MEM_RECLAIM,
- handle_lo, handle_hi, flags,
- 0, 0, 0, 0,
- res);
+ arm_smccc_1_2_smc(&(struct arm_smccc_1_2_regs) {
+ .a0 = FFA_MEM_RECLAIM,
+ .a1 = handle_lo,
+ .a2 = handle_hi,
+ .a3 = flags,
+ }, res);
}
-static void ffa_retrieve_req(struct arm_smccc_res *res, u32 len)
+static void ffa_retrieve_req(struct arm_smccc_1_2_regs *res, u32 len)
{
- arm_smccc_1_1_smc(FFA_FN64_MEM_RETRIEVE_REQ,
- len, len,
- 0, 0, 0, 0, 0,
- res);
+ arm_smccc_1_2_smc(&(struct arm_smccc_1_2_regs) {
+ .a0 = FFA_FN64_MEM_RETRIEVE_REQ,
+ .a1 = len,
+ .a2 = len,
+ }, res);
}
-static void ffa_rx_release(struct arm_smccc_res *res)
+static void ffa_rx_release(struct arm_smccc_1_2_regs *res)
{
- arm_smccc_1_1_smc(FFA_RX_RELEASE,
- 0, 0,
- 0, 0, 0, 0, 0,
- res);
+ arm_smccc_1_2_smc(&(struct arm_smccc_1_2_regs) {
+ .a0 = FFA_RX_RELEASE,
+ }, res);
}
-static void do_ffa_rxtx_map(struct arm_smccc_res *res,
+static void do_ffa_rxtx_map(struct arm_smccc_1_2_regs *res,
struct kvm_cpu_context *ctxt)
{
DECLARE_REG(phys_addr_t, tx, ctxt, 1);
@@ -267,7 +309,7 @@ static void do_ffa_rxtx_map(struct arm_smccc_res *res,
goto out_unlock;
}
-static void do_ffa_rxtx_unmap(struct arm_smccc_res *res,
+static void do_ffa_rxtx_unmap(struct arm_smccc_1_2_regs *res,
struct kvm_cpu_context *ctxt)
{
DECLARE_REG(u32, id, ctxt, 1);
@@ -368,7 +410,7 @@ static int ffa_host_unshare_ranges(struct ffa_mem_region_addr_range *ranges,
return ret;
}
-static void do_ffa_mem_frag_tx(struct arm_smccc_res *res,
+static void do_ffa_mem_frag_tx(struct arm_smccc_1_2_regs *res,
struct kvm_cpu_context *ctxt)
{
DECLARE_REG(u32, handle_lo, ctxt, 1);
@@ -427,7 +469,7 @@ static void do_ffa_mem_frag_tx(struct arm_smccc_res *res,
}
static void __do_ffa_mem_xfer(const u64 func_id,
- struct arm_smccc_res *res,
+ struct arm_smccc_1_2_regs *res,
struct kvm_cpu_context *ctxt)
{
DECLARE_REG(u32, len, ctxt, 1);
@@ -521,7 +563,7 @@ static void __do_ffa_mem_xfer(const u64 func_id,
__do_ffa_mem_xfer((fid), (res), (ctxt)); \
} while (0);
-static void do_ffa_mem_reclaim(struct arm_smccc_res *res,
+static void do_ffa_mem_reclaim(struct arm_smccc_1_2_regs *res,
struct kvm_cpu_context *ctxt)
{
DECLARE_REG(u32, handle_lo, ctxt, 1);
@@ -634,7 +676,7 @@ static bool ffa_call_supported(u64 func_id)
return true;
}
-static bool do_ffa_features(struct arm_smccc_res *res,
+static bool do_ffa_features(struct arm_smccc_1_2_regs *res,
struct kvm_cpu_context *ctxt)
{
DECLARE_REG(u32, id, ctxt, 1);
@@ -666,17 +708,21 @@ static bool do_ffa_features(struct arm_smccc_res *res,
static int hyp_ffa_post_init(void)
{
size_t min_rxtx_sz;
- struct arm_smccc_res res;
+ struct arm_smccc_1_2_regs res;
- arm_smccc_1_1_smc(FFA_ID_GET, 0, 0, 0, 0, 0, 0, 0, &res);
+ arm_smccc_1_2_smc(&(struct arm_smccc_1_2_regs){
+ .a0 = FFA_ID_GET,
+ }, &res);
if (res.a0 != FFA_SUCCESS)
return -EOPNOTSUPP;
if (res.a2 != HOST_FFA_ID)
return -EINVAL;
- arm_smccc_1_1_smc(FFA_FEATURES, FFA_FN64_RXTX_MAP,
- 0, 0, 0, 0, 0, 0, &res);
+ arm_smccc_1_2_smc(&(struct arm_smccc_1_2_regs){
+ .a0 = FFA_FEATURES,
+ .a1 = FFA_FN64_RXTX_MAP,
+ }, &res);
if (res.a0 != FFA_SUCCESS)
return -EOPNOTSUPP;
@@ -700,7 +746,7 @@ static int hyp_ffa_post_init(void)
return 0;
}
-static void do_ffa_version(struct arm_smccc_res *res,
+static void do_ffa_version(struct arm_smccc_1_2_regs *res,
struct kvm_cpu_context *ctxt)
{
DECLARE_REG(u32, ffa_req_version, ctxt, 1);
@@ -724,9 +770,10 @@ static void do_ffa_version(struct arm_smccc_res *res,
* first if TEE supports it.
*/
if (FFA_MINOR_VERSION(ffa_req_version) < FFA_MINOR_VERSION(hyp_ffa_version)) {
- arm_smccc_1_1_smc(FFA_VERSION, ffa_req_version, 0,
- 0, 0, 0, 0, 0,
- res);
+ arm_smccc_1_2_smc(&(struct arm_smccc_1_2_regs) {
+ .a0 = FFA_VERSION,
+ .a1 = ffa_req_version,
+ }, res);
if (res->a0 == FFA_RET_NOT_SUPPORTED)
goto unlock;
@@ -743,7 +790,7 @@ static void do_ffa_version(struct arm_smccc_res *res,
hyp_spin_unlock(&version_lock);
}
-static void do_ffa_part_get(struct arm_smccc_res *res,
+static void do_ffa_part_get(struct arm_smccc_1_2_regs *res,
struct kvm_cpu_context *ctxt)
{
DECLARE_REG(u32, uuid0, ctxt, 1);
@@ -759,9 +806,14 @@ static void do_ffa_part_get(struct arm_smccc_res *res,
goto out_unlock;
}
- arm_smccc_1_1_smc(FFA_PARTITION_INFO_GET, uuid0, uuid1,
- uuid2, uuid3, flags, 0, 0,
- res);
+ arm_smccc_1_2_smc(&(struct arm_smccc_1_2_regs) {
+ .a0 = FFA_PARTITION_INFO_GET,
+ .a1 = uuid0,
+ .a2 = uuid1,
+ .a3 = uuid2,
+ .a4 = uuid3,
+ .a5 = flags,
+ }, res);
if (res->a0 != FFA_SUCCESS)
goto out_unlock;
@@ -794,7 +846,7 @@ static void do_ffa_part_get(struct arm_smccc_res *res,
bool kvm_host_ffa_handler(struct kvm_cpu_context *host_ctxt, u32 func_id)
{
- struct arm_smccc_res res;
+ struct arm_smccc_1_2_regs res;
/*
* There's no way we can tell what a non-standard SMC call might
@@ -863,13 +915,16 @@ bool kvm_host_ffa_handler(struct kvm_cpu_context *host_ctxt, u32 func_id)
int hyp_ffa_init(void *pages)
{
- struct arm_smccc_res res;
+ struct arm_smccc_1_2_regs res;
void *tx, *rx;
if (kvm_host_psci_config.smccc_version < ARM_SMCCC_VERSION_1_2)
return 0;
- arm_smccc_1_1_smc(FFA_VERSION, FFA_VERSION_1_1, 0, 0, 0, 0, 0, 0, &res);
+ arm_smccc_1_2_smc(&(struct arm_smccc_1_2_regs) {
+ .a0 = FFA_VERSION,
+ .a1 = FFA_VERSION_1_1,
+ }, &res);
if (res.a0 == FFA_RET_NOT_SUPPORTED)
return 0;
--
2.50.0.727.gbf7dc18ff4-goog
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v7 3/5] KVM: arm64: Mark FFA_NOTIFICATION_* calls as unsupported
2025-07-01 22:06 [PATCH v7 0/5] KVM: arm64: Support FF-A 1.2 and SEND_DIRECT2 ABI Per Larsen via B4 Relay
2025-07-01 22:06 ` [PATCH v7 1/5] KVM: arm64: Correct return value on host version downgrade attempt Per Larsen via B4 Relay
2025-07-01 22:06 ` [PATCH v7 2/5] KVM: arm64: Use SMCCC 1.2 for FF-A initialization and in host handler Per Larsen via B4 Relay
@ 2025-07-01 22:06 ` Per Larsen via B4 Relay
2025-07-01 22:06 ` [PATCH v7 4/5] KVM: arm64: Bump the supported version of FF-A to 1.2 Per Larsen via B4 Relay
2025-07-01 22:06 ` [PATCH v7 5/5] KVM: arm64: Support FFA_MSG_SEND_DIRECT_REQ2 in host handler Per Larsen via B4 Relay
4 siblings, 0 replies; 20+ messages in thread
From: Per Larsen via B4 Relay @ 2025-07-01 22:06 UTC (permalink / raw)
To: Marc Zyngier, Oliver Upton, Joey Gouly, Suzuki K Poulose,
Zenghui Yu, Catalin Marinas, Will Deacon, Sudeep Holla
Cc: linux-arm-kernel, kvmarm, linux-kernel, ahomescu, armellel, arve,
ayrton, qperret, sebastianene, qwandor, Per Larsen
From: Per Larsen <perlarsen@google.com>
Prevent FFA_NOTIFICATION_* interfaces from being passed through to TZ.
Acked-by: Will Deacon <will@kernel.org>
Signed-off-by: Per Larsen <perlarsen@google.com>
---
arch/arm64/kvm/hyp/nvhe/ffa.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c
index 65d241ba32403d014b43cc4ef4d5bf9693813809..5fd6474d96ae4b90d99796ee81bb36373219afc4 100644
--- a/arch/arm64/kvm/hyp/nvhe/ffa.c
+++ b/arch/arm64/kvm/hyp/nvhe/ffa.c
@@ -670,6 +670,14 @@ static bool ffa_call_supported(u64 func_id)
case FFA_RXTX_MAP:
case FFA_MEM_DONATE:
case FFA_MEM_RETRIEVE_REQ:
+ /* Optional notification interfaces added in FF-A 1.1 */
+ case FFA_NOTIFICATION_BITMAP_CREATE:
+ case FFA_NOTIFICATION_BITMAP_DESTROY:
+ case FFA_NOTIFICATION_BIND:
+ case FFA_NOTIFICATION_UNBIND:
+ case FFA_NOTIFICATION_SET:
+ case FFA_NOTIFICATION_GET:
+ case FFA_NOTIFICATION_INFO_GET:
return false;
}
--
2.50.0.727.gbf7dc18ff4-goog
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v7 4/5] KVM: arm64: Bump the supported version of FF-A to 1.2
2025-07-01 22:06 [PATCH v7 0/5] KVM: arm64: Support FF-A 1.2 and SEND_DIRECT2 ABI Per Larsen via B4 Relay
` (2 preceding siblings ...)
2025-07-01 22:06 ` [PATCH v7 3/5] KVM: arm64: Mark FFA_NOTIFICATION_* calls as unsupported Per Larsen via B4 Relay
@ 2025-07-01 22:06 ` Per Larsen via B4 Relay
2025-07-18 13:45 ` Will Deacon
2025-07-01 22:06 ` [PATCH v7 5/5] KVM: arm64: Support FFA_MSG_SEND_DIRECT_REQ2 in host handler Per Larsen via B4 Relay
4 siblings, 1 reply; 20+ messages in thread
From: Per Larsen via B4 Relay @ 2025-07-01 22:06 UTC (permalink / raw)
To: Marc Zyngier, Oliver Upton, Joey Gouly, Suzuki K Poulose,
Zenghui Yu, Catalin Marinas, Will Deacon, Sudeep Holla
Cc: linux-arm-kernel, kvmarm, linux-kernel, ahomescu, armellel, arve,
ayrton, qperret, sebastianene, qwandor, Per Larsen
From: Per Larsen <perlarsen@google.com>
FF-A version 1.2 introduces the DIRECT_REQ2 ABI. Bump the FF-A version
preferred by the hypervisor as a precursor to implementing the 1.2-only
FFA_MSG_SEND_DIRECT_REQ2 and FFA_MSG_SEND_RESP2 messaging interfaces.
We must also use SMCCC 1.2 for 64-bit SMCs if hypervisor negotiated FF-A
1.2, so ffa_set_retval is updated and a new function to call 64-bit smcs
using SMCCC 1.2 with fallback to SMCCC 1.1 is introduced.
Update ffa_call_supported to mark FF-A 1.2 interfaces as unsupported
lest they get forwarded.
Co-developed-by: Ayrton Munoz <ayrton@google.com>
Signed-off-by: Ayrton Munoz <ayrton@google.com>
Signed-off-by: Per Larsen <perlarsen@google.com>
---
arch/arm64/kvm/hyp/nvhe/ffa.c | 18 ++++++++++++++----
include/linux/arm_ffa.h | 1 +
2 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c
index 5fd6474d96ae4b90d99796ee81bb36373219afc4..79d834120a3f3d26e17e9170c60012b60c6f5a5e 100644
--- a/arch/arm64/kvm/hyp/nvhe/ffa.c
+++ b/arch/arm64/kvm/hyp/nvhe/ffa.c
@@ -678,6 +678,13 @@ static bool ffa_call_supported(u64 func_id)
case FFA_NOTIFICATION_SET:
case FFA_NOTIFICATION_GET:
case FFA_NOTIFICATION_INFO_GET:
+ /* Optional interfaces added in FF-A 1.2 */
+ case FFA_MSG_SEND_DIRECT_REQ2: /* Optional per 7.5.1 */
+ case FFA_MSG_SEND_DIRECT_RESP2: /* Optional per 7.5.1 */
+ case FFA_CONSOLE_LOG: /* Optional per 13.1: not in Table 13.1 */
+ case FFA_PARTITION_INFO_GET_REGS: /* Optional for virtual instances per 13.1 */
+ /* Unsupported interfaces added in FF-A 1.2 */
+ case FFA_EL3_INTR_HANDLE: /* Only valid for secure physical instances */
return false;
}
@@ -734,7 +741,10 @@ static int hyp_ffa_post_init(void)
if (res.a0 != FFA_SUCCESS)
return -EOPNOTSUPP;
- switch (res.a2) {
+ if ((res.a2 & GENMASK(15, 2)) != 0 || res.a3 != 0)
+ return -EINVAL;
+
+ switch (res.a2 & FFA_FEAT_RXTX_MIN_SZ_MASK) {
case FFA_FEAT_RXTX_MIN_SZ_4K:
min_rxtx_sz = SZ_4K;
break;
@@ -931,7 +941,7 @@ int hyp_ffa_init(void *pages)
arm_smccc_1_2_smc(&(struct arm_smccc_1_2_regs) {
.a0 = FFA_VERSION,
- .a1 = FFA_VERSION_1_1,
+ .a1 = FFA_VERSION_1_2,
}, &res);
if (res.a0 == FFA_RET_NOT_SUPPORTED)
return 0;
@@ -952,10 +962,10 @@ int hyp_ffa_init(void *pages)
if (FFA_MAJOR_VERSION(res.a0) != 1)
return -EOPNOTSUPP;
- if (FFA_MINOR_VERSION(res.a0) < FFA_MINOR_VERSION(FFA_VERSION_1_1))
+ if (FFA_MINOR_VERSION(res.a0) < FFA_MINOR_VERSION(FFA_VERSION_1_2))
hyp_ffa_version = res.a0;
else
- hyp_ffa_version = FFA_VERSION_1_1;
+ hyp_ffa_version = FFA_VERSION_1_2;
tx = pages;
pages += KVM_FFA_MBOX_NR_PAGES * PAGE_SIZE;
diff --git a/include/linux/arm_ffa.h b/include/linux/arm_ffa.h
index 5bded24dc24fea8cdcbe42bf79c7c025c3fa5f4b..2b760bede9c0a4bcb58bf697681c32e77c08a5d9 100644
--- a/include/linux/arm_ffa.h
+++ b/include/linux/arm_ffa.h
@@ -128,6 +128,7 @@
#define FFA_FEAT_RXTX_MIN_SZ_4K 0
#define FFA_FEAT_RXTX_MIN_SZ_64K 1
#define FFA_FEAT_RXTX_MIN_SZ_16K 2
+#define FFA_FEAT_RXTX_MIN_SZ_MASK GENMASK(1, 0)
/* FFA Bus/Device/Driver related */
struct ffa_device {
--
2.50.0.727.gbf7dc18ff4-goog
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v7 5/5] KVM: arm64: Support FFA_MSG_SEND_DIRECT_REQ2 in host handler
2025-07-01 22:06 [PATCH v7 0/5] KVM: arm64: Support FF-A 1.2 and SEND_DIRECT2 ABI Per Larsen via B4 Relay
` (3 preceding siblings ...)
2025-07-01 22:06 ` [PATCH v7 4/5] KVM: arm64: Bump the supported version of FF-A to 1.2 Per Larsen via B4 Relay
@ 2025-07-01 22:06 ` Per Larsen via B4 Relay
2025-07-18 13:53 ` Will Deacon
4 siblings, 1 reply; 20+ messages in thread
From: Per Larsen via B4 Relay @ 2025-07-01 22:06 UTC (permalink / raw)
To: Marc Zyngier, Oliver Upton, Joey Gouly, Suzuki K Poulose,
Zenghui Yu, Catalin Marinas, Will Deacon, Sudeep Holla
Cc: linux-arm-kernel, kvmarm, linux-kernel, ahomescu, armellel, arve,
ayrton, qperret, sebastianene, qwandor, Per Larsen
From: Per Larsen <perlarsen@google.com>
FF-A 1.2 adds the DIRECT_REQ2 messaging interface which is similar to
the existing FFA_MSG_SEND_DIRECT_{REQ,RESP} functions except that it
uses the SMC calling convention v1.2 which allows calls to use x4-x17 as
argument and return registers. Add support for FFA_MSG_SEND_DIRECT_REQ2
in the host ffa handler.
Signed-off-by: Per Larsen <perlarsen@google.com>
---
arch/arm64/kvm/hyp/nvhe/ffa.c | 24 +++++++++++++++++++++++-
include/linux/arm_ffa.h | 2 ++
2 files changed, 25 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c
index 79d834120a3f3d26e17e9170c60012b60c6f5a5e..21225988a9365219ccfd69e8e599d7403b5cdf05 100644
--- a/arch/arm64/kvm/hyp/nvhe/ffa.c
+++ b/arch/arm64/kvm/hyp/nvhe/ffa.c
@@ -679,7 +679,6 @@ static bool ffa_call_supported(u64 func_id)
case FFA_NOTIFICATION_GET:
case FFA_NOTIFICATION_INFO_GET:
/* Optional interfaces added in FF-A 1.2 */
- case FFA_MSG_SEND_DIRECT_REQ2: /* Optional per 7.5.1 */
case FFA_MSG_SEND_DIRECT_RESP2: /* Optional per 7.5.1 */
case FFA_CONSOLE_LOG: /* Optional per 13.1: not in Table 13.1 */
case FFA_PARTITION_INFO_GET_REGS: /* Optional for virtual instances per 13.1 */
@@ -862,6 +861,22 @@ static void do_ffa_part_get(struct arm_smccc_1_2_regs *res,
hyp_spin_unlock(&host_buffers.lock);
}
+static void do_ffa_direct_msg2(struct arm_smccc_1_2_regs *regs,
+ struct kvm_cpu_context *ctxt,
+ u64 vm_handle)
+{
+ DECLARE_REG(u32, endp, ctxt, 1);
+
+ struct arm_smccc_1_2_regs *args = (void *)&ctxt->regs.regs[0];
+
+ if (FIELD_GET(FFA_SRC_ENDPOINT_MASK, endp) != vm_handle) {
+ ffa_to_smccc_error(regs, FFA_RET_INVALID_PARAMETERS);
+ return;
+ }
+
+ arm_smccc_1_2_smc(args, regs);
+}
+
bool kvm_host_ffa_handler(struct kvm_cpu_context *host_ctxt, u32 func_id)
{
struct arm_smccc_1_2_regs res;
@@ -920,11 +935,18 @@ bool kvm_host_ffa_handler(struct kvm_cpu_context *host_ctxt, u32 func_id)
case FFA_PARTITION_INFO_GET:
do_ffa_part_get(&res, host_ctxt);
goto out_handled;
+ case FFA_MSG_SEND_DIRECT_REQ2:
+ if (hyp_ffa_version >= FFA_VERSION_1_2) {
+ do_ffa_direct_msg2(&res, host_ctxt, HOST_FFA_ID);
+ goto out_handled;
+ }
+ goto out_not_supported;
}
if (ffa_call_supported(func_id))
return false; /* Pass through */
+out_not_supported:
ffa_to_smccc_error(&res, FFA_RET_NOT_SUPPORTED);
out_handled:
ffa_set_retval(host_ctxt, &res);
diff --git a/include/linux/arm_ffa.h b/include/linux/arm_ffa.h
index 2b760bede9c0a4bcb58bf697681c32e77c08a5d9..db63a96190545c9f65943909de2d73cf8e7b9615 100644
--- a/include/linux/arm_ffa.h
+++ b/include/linux/arm_ffa.h
@@ -269,6 +269,8 @@ bool ffa_partition_check_property(struct ffa_device *dev, u32 property)
(ffa_partition_check_property(dev, FFA_PARTITION_DIRECT_REQ2_RECV) && \
!dev->mode_32bit)
+#define FFA_SRC_ENDPOINT_MASK GENMASK(31, 16)
+
/* For use with FFA_MSG_SEND_DIRECT_{REQ,RESP} which pass data via registers */
struct ffa_send_direct_data {
unsigned long data0; /* w3/x3 */
--
2.50.0.727.gbf7dc18ff4-goog
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v7 2/5] KVM: arm64: Use SMCCC 1.2 for FF-A initialization and in host handler
2025-07-01 22:06 ` [PATCH v7 2/5] KVM: arm64: Use SMCCC 1.2 for FF-A initialization and in host handler Per Larsen via B4 Relay
@ 2025-07-03 12:38 ` Marc Zyngier
2025-07-08 0:06 ` Per Larsen
0 siblings, 1 reply; 20+ messages in thread
From: Marc Zyngier @ 2025-07-03 12:38 UTC (permalink / raw)
To: perlarsen
Cc: Oliver Upton, Joey Gouly, Suzuki K Poulose, Zenghui Yu,
Catalin Marinas, Will Deacon, Sudeep Holla, linux-arm-kernel,
kvmarm, linux-kernel, ahomescu, armellel, arve, ayrton, qperret,
sebastianene, qwandor
On Tue, 01 Jul 2025 23:06:35 +0100,
Per Larsen via B4 Relay <devnull+perlarsen.google.com@kernel.org> wrote:
>
> From: Per Larsen <perlarsen@google.com>
>
> SMCCC 1.1 and prior allows four registers to be sent back as a result
> of an FF-A interface. SMCCC 1.2 increases the number of results that can
> be sent back to 8 and 16 for 32-bit and 64-bit SMC/HVCs respectively.
>
> FF-A 1.0 references SMCCC 1.2 (reference [4] on page xi) and FF-A 1.2
> explicitly requires SMCCC 1.2 so it should be safe to use this version
> unconditionally. Moreover, it is simpler to implement FF-A features
> without having to worry about compatibility with SMCCC 1.1 and older.
>
> Update the FF-A initialization and host handler code to use SMCCC 1.2.
>
> Signed-off-by: Per Larsen <perlarsen@google.com>
> ---
> arch/arm64/kvm/hyp/nvhe/Makefile | 1 +
> arch/arm64/kvm/hyp/nvhe/ffa.c | 193 +++++++++++++++++++++++++--------------
> 2 files changed, 125 insertions(+), 69 deletions(-)
>
> diff --git a/arch/arm64/kvm/hyp/nvhe/Makefile b/arch/arm64/kvm/hyp/nvhe/Makefile
> index a76522d63c3e630795db5972a99abc3d24bc5e26..f859a8fb41a25effea1edd977bef889423153399 100644
> --- a/arch/arm64/kvm/hyp/nvhe/Makefile
> +++ b/arch/arm64/kvm/hyp/nvhe/Makefile
> @@ -27,6 +27,7 @@ hyp-obj-y := timer-sr.o sysreg-sr.o debug-sr.o switch.o tlb.o hyp-init.o host.o
> cache.o setup.o mm.o mem_protect.o sys_regs.o pkvm.o stacktrace.o ffa.o
> hyp-obj-y += ../vgic-v3-sr.o ../aarch32.o ../vgic-v2-cpuif-proxy.o ../entry.o \
> ../fpsimd.o ../hyp-entry.o ../exception.o ../pgtable.o
> +hyp-obj-y += ../../../kernel/smccc-call.o
> hyp-obj-$(CONFIG_LIST_HARDENED) += list_debug.o
> hyp-obj-y += $(lib-objs)
>
> 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?
> +
> + /*
> + * 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.
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v7 2/5] KVM: arm64: Use SMCCC 1.2 for FF-A initialization and in host handler
2025-07-03 12:38 ` Marc Zyngier
@ 2025-07-08 0:06 ` Per Larsen
2025-07-18 13:37 ` Will Deacon
0 siblings, 1 reply; 20+ messages in thread
From: Per Larsen @ 2025-07-08 0:06 UTC (permalink / raw)
To: Marc Zyngier, perlarsen
Cc: Oliver Upton, Joey Gouly, Suzuki K Poulose, Zenghui Yu,
Catalin Marinas, Will Deacon, Sudeep Holla, linux-arm-kernel,
kvmarm, linux-kernel, ahomescu, armellel, arve, ayrton, qperret,
sebastianene, qwandor
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:
>>
>> From: Per Larsen <perlarsen@google.com>
>>
>> SMCCC 1.1 and prior allows four registers to be sent back as a result
>> of an FF-A interface. SMCCC 1.2 increases the number of results that can
>> be sent back to 8 and 16 for 32-bit and 64-bit SMC/HVCs respectively.
>>
>> FF-A 1.0 references SMCCC 1.2 (reference [4] on page xi) and FF-A 1.2
>> explicitly requires SMCCC 1.2 so it should be safe to use this version
>> unconditionally. Moreover, it is simpler to implement FF-A features
>> without having to worry about compatibility with SMCCC 1.1 and older.
>>
>> Update the FF-A initialization and host handler code to use SMCCC 1.2.
>>
>> Signed-off-by: Per Larsen <perlarsen@google.com>
>> ---
>> arch/arm64/kvm/hyp/nvhe/Makefile | 1 +
>> arch/arm64/kvm/hyp/nvhe/ffa.c | 193 +++++++++++++++++++++++++--------------
>> 2 files changed, 125 insertions(+), 69 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/hyp/nvhe/Makefile b/arch/arm64/kvm/hyp/nvhe/Makefile
>> index a76522d63c3e630795db5972a99abc3d24bc5e26..f859a8fb41a25effea1edd977bef889423153399 100644
>> --- a/arch/arm64/kvm/hyp/nvhe/Makefile
>> +++ b/arch/arm64/kvm/hyp/nvhe/Makefile
>> @@ -27,6 +27,7 @@ hyp-obj-y := timer-sr.o sysreg-sr.o debug-sr.o switch.o tlb.o hyp-init.o host.o
>> cache.o setup.o mm.o mem_protect.o sys_regs.o pkvm.o stacktrace.o ffa.o
>> hyp-obj-y += ../vgic-v3-sr.o ../aarch32.o ../vgic-v2-cpuif-proxy.o ../entry.o \
>> ../fpsimd.o ../hyp-entry.o ../exception.o ../pgtable.o
>> +hyp-obj-y += ../../../kernel/smccc-call.o
>> hyp-obj-$(CONFIG_LIST_HARDENED) += list_debug.o
>> hyp-obj-y += $(lib-objs)
>>
>> 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.>
>> +
>> + /*
>> + * 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.
The Trusty TEE OS calls FFA_MSG_WAIT here:
https://cs.android.com/android/platform/superproject/main/+/main:trusty/kernel/lib/sm/sm.c;l=844?q=sm.c&ss=android
and anticipates a direct request ABI as one of the potential replies.
Thanks,
Per
PS: I'm replying from perl@immunant.com as I cannot use
perlarsen@google.com to send replies to review comments.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v7 2/5] KVM: arm64: Use SMCCC 1.2 for FF-A initialization and in host handler
2025-07-08 0:06 ` Per Larsen
@ 2025-07-18 13:37 ` Will Deacon
2025-07-19 5:54 ` Per Larsen
0 siblings, 1 reply; 20+ messages in thread
From: Will Deacon @ 2025-07-18 13:37 UTC (permalink / raw)
To: Per Larsen
Cc: Marc Zyngier, perlarsen, Oliver Upton, Joey Gouly,
Suzuki K Poulose, Zenghui Yu, Catalin Marinas, Sudeep Holla,
linux-arm-kernel, kvmarm, linux-kernel, ahomescu, armellel, arve,
ayrton, qperret, sebastianene, qwandor
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:
> > > 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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v7 4/5] KVM: arm64: Bump the supported version of FF-A to 1.2
2025-07-01 22:06 ` [PATCH v7 4/5] KVM: arm64: Bump the supported version of FF-A to 1.2 Per Larsen via B4 Relay
@ 2025-07-18 13:45 ` Will Deacon
2025-07-31 7:56 ` Marc Zyngier
0 siblings, 1 reply; 20+ messages in thread
From: Will Deacon @ 2025-07-18 13:45 UTC (permalink / raw)
To: perlarsen
Cc: Marc Zyngier, Oliver Upton, Joey Gouly, Suzuki K Poulose,
Zenghui Yu, Catalin Marinas, Sudeep Holla, linux-arm-kernel,
kvmarm, linux-kernel, ahomescu, armellel, arve, ayrton, qperret,
sebastianene, qwandor
On Tue, Jul 01, 2025 at 10:06:37PM +0000, Per Larsen via B4 Relay wrote:
> From: Per Larsen <perlarsen@google.com>
>
> FF-A version 1.2 introduces the DIRECT_REQ2 ABI. Bump the FF-A version
> preferred by the hypervisor as a precursor to implementing the 1.2-only
> FFA_MSG_SEND_DIRECT_REQ2 and FFA_MSG_SEND_RESP2 messaging interfaces.
>
> We must also use SMCCC 1.2 for 64-bit SMCs if hypervisor negotiated FF-A
> 1.2, so ffa_set_retval is updated and a new function to call 64-bit smcs
> using SMCCC 1.2 with fallback to SMCCC 1.1 is introduced.
>
> Update ffa_call_supported to mark FF-A 1.2 interfaces as unsupported
> lest they get forwarded.
>
> Co-developed-by: Ayrton Munoz <ayrton@google.com>
> Signed-off-by: Ayrton Munoz <ayrton@google.com>
> Signed-off-by: Per Larsen <perlarsen@google.com>
> ---
> arch/arm64/kvm/hyp/nvhe/ffa.c | 18 ++++++++++++++----
> include/linux/arm_ffa.h | 1 +
> 2 files changed, 15 insertions(+), 4 deletions(-)
This patch needs to be split into smaller chunks as it's doing a number
of things in one go.
> diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c
> index 5fd6474d96ae4b90d99796ee81bb36373219afc4..79d834120a3f3d26e17e9170c60012b60c6f5a5e 100644
> --- a/arch/arm64/kvm/hyp/nvhe/ffa.c
> +++ b/arch/arm64/kvm/hyp/nvhe/ffa.c
> @@ -678,6 +678,13 @@ static bool ffa_call_supported(u64 func_id)
> case FFA_NOTIFICATION_SET:
> case FFA_NOTIFICATION_GET:
> case FFA_NOTIFICATION_INFO_GET:
> + /* Optional interfaces added in FF-A 1.2 */
> + case FFA_MSG_SEND_DIRECT_REQ2: /* Optional per 7.5.1 */
> + case FFA_MSG_SEND_DIRECT_RESP2: /* Optional per 7.5.1 */
> + case FFA_CONSOLE_LOG: /* Optional per 13.1: not in Table 13.1 */
> + case FFA_PARTITION_INFO_GET_REGS: /* Optional for virtual instances per 13.1 */
> + /* Unsupported interfaces added in FF-A 1.2 */
> + case FFA_EL3_INTR_HANDLE: /* Only valid for secure physical instances */
> return false;
> }
This could be a standalone change ^^^
>
> @@ -734,7 +741,10 @@ static int hyp_ffa_post_init(void)
> if (res.a0 != FFA_SUCCESS)
> return -EOPNOTSUPP;
>
> - switch (res.a2) {
> + if ((res.a2 & GENMASK(15, 2)) != 0 || res.a3 != 0)
> + return -EINVAL;
Why are you checking bits a2[15:2] and a3? The spec says they MBZ,
so we shouldn't care about enforcing that. In fact, adding the check
probably means we'll fail if those bits get allocated in future.
> +
> + switch (res.a2 & FFA_FEAT_RXTX_MIN_SZ_MASK) {
That makes sense, and can be its own patch.
> case FFA_FEAT_RXTX_MIN_SZ_4K:
> min_rxtx_sz = SZ_4K;
> break;
> @@ -931,7 +941,7 @@ int hyp_ffa_init(void *pages)
>
> arm_smccc_1_2_smc(&(struct arm_smccc_1_2_regs) {
> .a0 = FFA_VERSION,
> - .a1 = FFA_VERSION_1_1,
> + .a1 = FFA_VERSION_1_2,
>
> }, &res);
> if (res.a0 == FFA_RET_NOT_SUPPORTED)
> return 0;
> @@ -952,10 +962,10 @@ int hyp_ffa_init(void *pages)
> if (FFA_MAJOR_VERSION(res.a0) != 1)
> return -EOPNOTSUPP;
>
> - if (FFA_MINOR_VERSION(res.a0) < FFA_MINOR_VERSION(FFA_VERSION_1_1))
> + if (FFA_MINOR_VERSION(res.a0) < FFA_MINOR_VERSION(FFA_VERSION_1_2))
> hyp_ffa_version = res.a0;
> else
> - hyp_ffa_version = FFA_VERSION_1_1;
> + hyp_ffa_version = FFA_VERSION_1_2;
The move to v1.2 can also be its own patch. So you end up with three in
total.
Will
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v7 5/5] KVM: arm64: Support FFA_MSG_SEND_DIRECT_REQ2 in host handler
2025-07-01 22:06 ` [PATCH v7 5/5] KVM: arm64: Support FFA_MSG_SEND_DIRECT_REQ2 in host handler Per Larsen via B4 Relay
@ 2025-07-18 13:53 ` Will Deacon
2025-07-21 11:13 ` Arve Hjønnevåg
2025-07-21 22:43 ` Per Larsen
0 siblings, 2 replies; 20+ messages in thread
From: Will Deacon @ 2025-07-18 13:53 UTC (permalink / raw)
To: perlarsen
Cc: Marc Zyngier, Oliver Upton, Joey Gouly, Suzuki K Poulose,
Zenghui Yu, Catalin Marinas, Sudeep Holla, linux-arm-kernel,
kvmarm, linux-kernel, ahomescu, armellel, arve, ayrton, qperret,
sebastianene, qwandor
On Tue, Jul 01, 2025 at 10:06:38PM +0000, Per Larsen via B4 Relay wrote:
> From: Per Larsen <perlarsen@google.com>
>
> FF-A 1.2 adds the DIRECT_REQ2 messaging interface which is similar to
> the existing FFA_MSG_SEND_DIRECT_{REQ,RESP} functions except that it
> uses the SMC calling convention v1.2 which allows calls to use x4-x17 as
> argument and return registers. Add support for FFA_MSG_SEND_DIRECT_REQ2
> in the host ffa handler.
>
> Signed-off-by: Per Larsen <perlarsen@google.com>
> ---
> arch/arm64/kvm/hyp/nvhe/ffa.c | 24 +++++++++++++++++++++++-
> include/linux/arm_ffa.h | 2 ++
> 2 files changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c
> index 79d834120a3f3d26e17e9170c60012b60c6f5a5e..21225988a9365219ccfd69e8e599d7403b5cdf05 100644
> --- a/arch/arm64/kvm/hyp/nvhe/ffa.c
> +++ b/arch/arm64/kvm/hyp/nvhe/ffa.c
> @@ -679,7 +679,6 @@ static bool ffa_call_supported(u64 func_id)
> case FFA_NOTIFICATION_GET:
> case FFA_NOTIFICATION_INFO_GET:
> /* Optional interfaces added in FF-A 1.2 */
> - case FFA_MSG_SEND_DIRECT_REQ2: /* Optional per 7.5.1 */
I think that's the only change needed. In fact, maybe just don't add it
in the earlier patch?
> case FFA_MSG_SEND_DIRECT_RESP2: /* Optional per 7.5.1 */
> case FFA_CONSOLE_LOG: /* Optional per 13.1: not in Table 13.1 */
> case FFA_PARTITION_INFO_GET_REGS: /* Optional for virtual instances per 13.1 */
> @@ -862,6 +861,22 @@ static void do_ffa_part_get(struct arm_smccc_1_2_regs *res,
> hyp_spin_unlock(&host_buffers.lock);
> }
>
> +static void do_ffa_direct_msg2(struct arm_smccc_1_2_regs *regs,
> + struct kvm_cpu_context *ctxt,
> + u64 vm_handle)
> +{
> + DECLARE_REG(u32, endp, ctxt, 1);
> +
> + struct arm_smccc_1_2_regs *args = (void *)&ctxt->regs.regs[0];
> +
> + if (FIELD_GET(FFA_SRC_ENDPOINT_MASK, endp) != vm_handle) {
> + ffa_to_smccc_error(regs, FFA_RET_INVALID_PARAMETERS);
> + return;
> + }
Why do we care about checking the src id? We don't check that for
FFA_MSG_SEND_DIRECT_REQ and I don't think we need to care about it here
either.
Will
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v7 2/5] KVM: arm64: Use SMCCC 1.2 for FF-A initialization and in host handler
2025-07-18 13:37 ` Will Deacon
@ 2025-07-19 5:54 ` Per Larsen
2025-07-21 11:01 ` Arve Hjønnevåg
0 siblings, 1 reply; 20+ messages in thread
From: Per Larsen @ 2025-07-19 5:54 UTC (permalink / raw)
To: Will Deacon
Cc: Marc Zyngier, perlarsen, Oliver Upton, Joey Gouly,
Suzuki K Poulose, Zenghui Yu, Catalin Marinas, Sudeep Holla,
linux-arm-kernel, kvmarm, linux-kernel, ahomescu, armellel, arve,
ayrton, qperret, sebastianene, qwandor
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:
>>>> 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.
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.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v7 2/5] KVM: arm64: Use SMCCC 1.2 for FF-A initialization and in host handler
2025-07-19 5:54 ` Per Larsen
@ 2025-07-21 11:01 ` Arve Hjønnevåg
2025-07-22 0:20 ` Per Larsen
0 siblings, 1 reply; 20+ messages in thread
From: Arve Hjønnevåg @ 2025-07-21 11:01 UTC (permalink / raw)
To: Per Larsen
Cc: Will Deacon, Marc Zyngier, perlarsen, Oliver Upton, Joey Gouly,
Suzuki K Poulose, Zenghui Yu, Catalin Marinas, Sudeep Holla,
linux-arm-kernel, kvmarm, linux-kernel, ahomescu, armellel,
ayrton, qperret, sebastianene, qwandor
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:
> >>>> 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.
> 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.
The FF-A 1.3 ALP2 fixes this by adding a 64 bit FF-A run opcode, but
at the current patchstack only adds ff-a 1.2 support and the linux
ff-a driver does not yet support the new 1.3 ALP2 call flow either so
I think the current v7 patch here is the best option for now.
--
Arve Hjønnevåg
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v7 5/5] KVM: arm64: Support FFA_MSG_SEND_DIRECT_REQ2 in host handler
2025-07-18 13:53 ` Will Deacon
@ 2025-07-21 11:13 ` Arve Hjønnevåg
2025-07-21 22:43 ` Per Larsen
1 sibling, 0 replies; 20+ messages in thread
From: Arve Hjønnevåg @ 2025-07-21 11:13 UTC (permalink / raw)
To: Will Deacon
Cc: perlarsen, Marc Zyngier, Oliver Upton, Joey Gouly,
Suzuki K Poulose, Zenghui Yu, Catalin Marinas, Sudeep Holla,
linux-arm-kernel, kvmarm, linux-kernel, ahomescu, armellel,
ayrton, qperret, sebastianene, qwandor
On Fri, Jul 18, 2025 at 6:53 AM Will Deacon <will@kernel.org> wrote:
>
> On Tue, Jul 01, 2025 at 10:06:38PM +0000, Per Larsen via B4 Relay wrote:
> > From: Per Larsen <perlarsen@google.com>
> >
> > FF-A 1.2 adds the DIRECT_REQ2 messaging interface which is similar to
> > the existing FFA_MSG_SEND_DIRECT_{REQ,RESP} functions except that it
> > uses the SMC calling convention v1.2 which allows calls to use x4-x17 as
> > argument and return registers. Add support for FFA_MSG_SEND_DIRECT_REQ2
> > in the host ffa handler.
> >
> > Signed-off-by: Per Larsen <perlarsen@google.com>
> > ---
> > arch/arm64/kvm/hyp/nvhe/ffa.c | 24 +++++++++++++++++++++++-
> > include/linux/arm_ffa.h | 2 ++
> > 2 files changed, 25 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c
> > index 79d834120a3f3d26e17e9170c60012b60c6f5a5e..21225988a9365219ccfd69e8e599d7403b5cdf05 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/ffa.c
> > +++ b/arch/arm64/kvm/hyp/nvhe/ffa.c
> > @@ -679,7 +679,6 @@ static bool ffa_call_supported(u64 func_id)
> > case FFA_NOTIFICATION_GET:
> > case FFA_NOTIFICATION_INFO_GET:
> > /* Optional interfaces added in FF-A 1.2 */
> > - case FFA_MSG_SEND_DIRECT_REQ2: /* Optional per 7.5.1 */
>
> I think that's the only change needed. In fact, maybe just don't add it
> in the earlier patch?
>
> > case FFA_MSG_SEND_DIRECT_RESP2: /* Optional per 7.5.1 */
> > case FFA_CONSOLE_LOG: /* Optional per 13.1: not in Table 13.1 */
> > case FFA_PARTITION_INFO_GET_REGS: /* Optional for virtual instances per 13.1 */
> > @@ -862,6 +861,22 @@ static void do_ffa_part_get(struct arm_smccc_1_2_regs *res,
> > hyp_spin_unlock(&host_buffers.lock);
> > }
> >
> > +static void do_ffa_direct_msg2(struct arm_smccc_1_2_regs *regs,
> > + struct kvm_cpu_context *ctxt,
> > + u64 vm_handle)
> > +{
> > + DECLARE_REG(u32, endp, ctxt, 1);
> > +
> > + struct arm_smccc_1_2_regs *args = (void *)&ctxt->regs.regs[0];
> > +
> > + if (FIELD_GET(FFA_SRC_ENDPOINT_MASK, endp) != vm_handle) {
> > + ffa_to_smccc_error(regs, FFA_RET_INVALID_PARAMETERS);
> > + return;
> > + }
>
> Why do we care about checking the src id? We don't check that for
> FFA_MSG_SEND_DIRECT_REQ and I don't think we need to care about it here
> either.
>
> Will
I think not checking the src id for FFA_MSG_SEND_DIRECT_REQ is a bug
that should be fixed as well. The receiver expects the hypervisor to
have validated this. If the src id is not validated here then the host
can impersonate other VMs or even the hypervisor itself. This bug
might be minor at the moment since other VMs can't send messages at
the moment, but it is still a bug that needs to be fixed at some
point.
--
Arve Hjønnevåg
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v7 5/5] KVM: arm64: Support FFA_MSG_SEND_DIRECT_REQ2 in host handler
2025-07-18 13:53 ` Will Deacon
2025-07-21 11:13 ` Arve Hjønnevåg
@ 2025-07-21 22:43 ` Per Larsen
2025-07-22 15:03 ` Will Deacon
1 sibling, 1 reply; 20+ messages in thread
From: Per Larsen @ 2025-07-21 22:43 UTC (permalink / raw)
To: Will Deacon, perlarsen
Cc: Marc Zyngier, Oliver Upton, Joey Gouly, Suzuki K Poulose,
Zenghui Yu, Catalin Marinas, Sudeep Holla, linux-arm-kernel,
kvmarm, linux-kernel, ahomescu, armellel, arve, ayrton, qperret,
sebastianene, qwandor
On 7/18/25 6:53 AM, Will Deacon wrote:
> On Tue, Jul 01, 2025 at 10:06:38PM +0000, Per Larsen via B4 Relay wrote:
>> From: Per Larsen <perlarsen@google.com>
>>
>> FF-A 1.2 adds the DIRECT_REQ2 messaging interface which is similar to
>> the existing FFA_MSG_SEND_DIRECT_{REQ,RESP} functions except that it
>> uses the SMC calling convention v1.2 which allows calls to use x4-x17 as
>> argument and return registers. Add support for FFA_MSG_SEND_DIRECT_REQ2
>> in the host ffa handler.
>>
>> Signed-off-by: Per Larsen <perlarsen@google.com>
>> ---
>> arch/arm64/kvm/hyp/nvhe/ffa.c | 24 +++++++++++++++++++++++-
>> include/linux/arm_ffa.h | 2 ++
>> 2 files changed, 25 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c
>> index 79d834120a3f3d26e17e9170c60012b60c6f5a5e..21225988a9365219ccfd69e8e599d7403b5cdf05 100644
>> --- a/arch/arm64/kvm/hyp/nvhe/ffa.c
>> +++ b/arch/arm64/kvm/hyp/nvhe/ffa.c
>> @@ -679,7 +679,6 @@ static bool ffa_call_supported(u64 func_id)
>> case FFA_NOTIFICATION_GET:
>> case FFA_NOTIFICATION_INFO_GET:
>> /* Optional interfaces added in FF-A 1.2 */
>> - case FFA_MSG_SEND_DIRECT_REQ2: /* Optional per 7.5.1 */
>
> I think that's the only change needed. In fact, maybe just don't add it
> in the earlier patch?
>
>> case FFA_MSG_SEND_DIRECT_RESP2: /* Optional per 7.5.1 */
>> case FFA_CONSOLE_LOG: /* Optional per 13.1: not in Table 13.1 */
>> case FFA_PARTITION_INFO_GET_REGS: /* Optional for virtual instances per 13.1 */
>> @@ -862,6 +861,22 @@ static void do_ffa_part_get(struct arm_smccc_1_2_regs *res,
>> hyp_spin_unlock(&host_buffers.lock);
>> }
>>
>> +static void do_ffa_direct_msg2(struct arm_smccc_1_2_regs *regs,
>> + struct kvm_cpu_context *ctxt,
>> + u64 vm_handle)
>> +{
>> + DECLARE_REG(u32, endp, ctxt, 1);
>> +
>> + struct arm_smccc_1_2_regs *args = (void *)&ctxt->regs.regs[0];
>> +
>> + if (FIELD_GET(FFA_SRC_ENDPOINT_MASK, endp) != vm_handle) {
>> + ffa_to_smccc_error(regs, FFA_RET_INVALID_PARAMETERS);
>> + return;
>> + }
>
> Why do we care about checking the src id? We don't check that for
> FFA_MSG_SEND_DIRECT_REQ and I don't think we need to care about it here
> either.
FFA_MSG_SEND_DIRECT_REQ is handled by do_ffa_direct_msg [0] (in the
android common kernels, I'm not aware of efforts to upstream this).
I patterned the check in do_ffa_direct_msg2 off the checking done in
do_ffa_direct_msg. I pressume your reasoning is that this check can
never fail since we pass in HOST_FFA_ID in kvm_host_ffa_handler. My
thinking was that we do need to validate the source ID once we start
using this function for requests that come from a guest VM. I could
of course add the check in an android-specific patch, WDYT is best?
Also note that since do_ffa_direct_msg was switched to use SMCCC 1.2, I
think it can handle both FFA_MSG_SEND_DIRECT_REQ and
FFA_MSG_SEND_DIRECT_REQ2. If you agree, should we upstream
do_ffa_direct_msg and use it to handle both of these direct requests?
[0]
https://cs.android.com/android/kernel/superproject/+/common-android16-6.12:common/arch/arm64/kvm/hyp/nvhe/ffa.c;l=1446
Thanks,
Per
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v7 2/5] KVM: arm64: Use SMCCC 1.2 for FF-A initialization and in host handler
2025-07-21 11:01 ` Arve Hjønnevåg
@ 2025-07-22 0:20 ` Per Larsen
2025-07-22 15:55 ` Will Deacon
0 siblings, 1 reply; 20+ messages in thread
From: Per Larsen @ 2025-07-22 0:20 UTC (permalink / raw)
To: Arve Hjønnevåg
Cc: Will Deacon, Marc Zyngier, perlarsen, Oliver Upton, Joey Gouly,
Suzuki K Poulose, Zenghui Yu, Catalin Marinas, Sudeep Holla,
linux-arm-kernel, kvmarm, linux-kernel, ahomescu, armellel,
ayrton, qperret, sebastianene, qwandor
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:
>>>>>> 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.
>> 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.
>
> The FF-A 1.3 ALP2 fixes this by adding a 64 bit FF-A run opcode, but
> at the current patchstack only adds ff-a 1.2 support and the linux
> ff-a driver does not yet support the new 1.3 ALP2 call flow either so
> I think the current v7 patch here is the best option for now.
>
FFA_RUN is passed through to EL3 by kvm_host_ffa_handler so I'm not sure
there is a code path where func_id == FFA_RUN in set_ffa_retval.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v7 5/5] KVM: arm64: Support FFA_MSG_SEND_DIRECT_REQ2 in host handler
2025-07-21 22:43 ` Per Larsen
@ 2025-07-22 15:03 ` Will Deacon
0 siblings, 0 replies; 20+ messages in thread
From: Will Deacon @ 2025-07-22 15:03 UTC (permalink / raw)
To: Per Larsen
Cc: perlarsen, Marc Zyngier, Oliver Upton, Joey Gouly,
Suzuki K Poulose, Zenghui Yu, Catalin Marinas, Sudeep Holla,
linux-arm-kernel, kvmarm, linux-kernel, ahomescu, armellel, arve,
ayrton, qperret, sebastianene, qwandor
On Mon, Jul 21, 2025 at 03:43:42PM -0700, Per Larsen wrote:
>
>
> On 7/18/25 6:53 AM, Will Deacon wrote:
> > On Tue, Jul 01, 2025 at 10:06:38PM +0000, Per Larsen via B4 Relay wrote:
> > > From: Per Larsen <perlarsen@google.com>
> > >
> > > FF-A 1.2 adds the DIRECT_REQ2 messaging interface which is similar to
> > > the existing FFA_MSG_SEND_DIRECT_{REQ,RESP} functions except that it
> > > uses the SMC calling convention v1.2 which allows calls to use x4-x17 as
> > > argument and return registers. Add support for FFA_MSG_SEND_DIRECT_REQ2
> > > in the host ffa handler.
> > >
> > > Signed-off-by: Per Larsen <perlarsen@google.com>
> > > ---
> > > arch/arm64/kvm/hyp/nvhe/ffa.c | 24 +++++++++++++++++++++++-
> > > include/linux/arm_ffa.h | 2 ++
> > > 2 files changed, 25 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c
> > > index 79d834120a3f3d26e17e9170c60012b60c6f5a5e..21225988a9365219ccfd69e8e599d7403b5cdf05 100644
> > > --- a/arch/arm64/kvm/hyp/nvhe/ffa.c
> > > +++ b/arch/arm64/kvm/hyp/nvhe/ffa.c
> > > @@ -679,7 +679,6 @@ static bool ffa_call_supported(u64 func_id)
> > > case FFA_NOTIFICATION_GET:
> > > case FFA_NOTIFICATION_INFO_GET:
> > > /* Optional interfaces added in FF-A 1.2 */
> > > - case FFA_MSG_SEND_DIRECT_REQ2: /* Optional per 7.5.1 */
> >
> > I think that's the only change needed. In fact, maybe just don't add it
> > in the earlier patch?
> >
> > > case FFA_MSG_SEND_DIRECT_RESP2: /* Optional per 7.5.1 */
> > > case FFA_CONSOLE_LOG: /* Optional per 13.1: not in Table 13.1 */
> > > case FFA_PARTITION_INFO_GET_REGS: /* Optional for virtual instances per 13.1 */
> > > @@ -862,6 +861,22 @@ static void do_ffa_part_get(struct arm_smccc_1_2_regs *res,
> > > hyp_spin_unlock(&host_buffers.lock);
> > > }
> > > +static void do_ffa_direct_msg2(struct arm_smccc_1_2_regs *regs,
> > > + struct kvm_cpu_context *ctxt,
> > > + u64 vm_handle)
> > > +{
> > > + DECLARE_REG(u32, endp, ctxt, 1);
> > > +
> > > + struct arm_smccc_1_2_regs *args = (void *)&ctxt->regs.regs[0];
> > > +
> > > + if (FIELD_GET(FFA_SRC_ENDPOINT_MASK, endp) != vm_handle) {
> > > + ffa_to_smccc_error(regs, FFA_RET_INVALID_PARAMETERS);
> > > + return;
> > > + }
> >
> > Why do we care about checking the src id? We don't check that for
> > FFA_MSG_SEND_DIRECT_REQ and I don't think we need to care about it here
> > either.
> FFA_MSG_SEND_DIRECT_REQ is handled by do_ffa_direct_msg [0] (in the android
> common kernels, I'm not aware of efforts to upstream this).
>
> I patterned the check in do_ffa_direct_msg2 off the checking done in
> do_ffa_direct_msg. I pressume your reasoning is that this check can
> never fail since we pass in HOST_FFA_ID in kvm_host_ffa_handler. My
> thinking was that we do need to validate the source ID once we start
> using this function for requests that come from a guest VM. I could
> of course add the check in an android-specific patch, WDYT is best?
As long as upstream only has one ID for the whole of non-secure, I don't
think it makes sense to check it. So either we drop this patch or teach
upstream about different IDs, which is probably a separate series.
What I want to avoid is upstream becoming a frankenkernel comprised of
random parts of Android that don't make sense in isolation.
Will
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v7 2/5] KVM: arm64: Use SMCCC 1.2 for FF-A initialization and in host handler
2025-07-22 0:20 ` Per Larsen
@ 2025-07-22 15:55 ` Will Deacon
0 siblings, 0 replies; 20+ messages in thread
From: Will Deacon @ 2025-07-22 15:55 UTC (permalink / raw)
To: Per Larsen, sudeep.holla, mark.rutland
Cc: Arve Hjønnevåg, Marc Zyngier, perlarsen, Oliver Upton,
Joey Gouly, Suzuki K Poulose, Zenghui Yu, Catalin Marinas,
linux-arm-kernel, kvmarm, linux-kernel, ahomescu, armellel,
ayrton, qperret, sebastianene, qwandor
[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.
> > The FF-A 1.3 ALP2 fixes this by adding a 64 bit FF-A run opcode, but
> > at the current patchstack only adds ff-a 1.2 support and the linux
> > ff-a driver does not yet support the new 1.3 ALP2 call flow either so
> > I think the current v7 patch here is the best option for now.
> >
> FFA_RUN is passed through to EL3 by kvm_host_ffa_handler so I'm not sure
> there is a code path where func_id == FFA_RUN in set_ffa_retval.
That's actually a really interesting point...
The passthrough code in __kvm_hyp_host_forward_smc() assumes SMC64 and
populates/reloads X0-X17 across the SMC. If the firmware replies to an
SMC32 call with an SMC64, then so be it, but it's not the hypervisor's
job to fix that up and the same problem would presumably happen even if
the hypervisor wasn't present.
So perhaps there's an argument that the proxy should be consistent with
that behaviour, otherwise we end up with different semantics depending
on whether we chose to proxy the call or pass it through. That would
mean always populating X8-X17 in the return value, regardless of the
function ID or anything else.
The spec should still be fixed because the current wording makes very
little sense in this area, but in the meantime maybe it's best just to
pass the hot potato between the host and the firmware rather than try to
fix it up ourselves.
Marc -- what do you think? We're damned if we do, damned if we don't,
but there's something to be said for being consistent when we get into
this messy situation.
Will
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v7 4/5] KVM: arm64: Bump the supported version of FF-A to 1.2
2025-07-18 13:45 ` Will Deacon
@ 2025-07-31 7:56 ` Marc Zyngier
2025-08-05 14:49 ` Will Deacon
0 siblings, 1 reply; 20+ messages in thread
From: Marc Zyngier @ 2025-07-31 7:56 UTC (permalink / raw)
To: Will Deacon
Cc: perlarsen, Oliver Upton, Joey Gouly, Suzuki K Poulose, Zenghui Yu,
Catalin Marinas, Sudeep Holla, linux-arm-kernel, kvmarm,
linux-kernel, ahomescu, armellel, arve, ayrton, qperret,
sebastianene, qwandor
On Fri, 18 Jul 2025 14:45:17 +0100,
Will Deacon <will@kernel.org> wrote:
>
> On Tue, Jul 01, 2025 at 10:06:37PM +0000, Per Larsen via B4 Relay wrote:
> > From: Per Larsen <perlarsen@google.com>
> >
> > FF-A version 1.2 introduces the DIRECT_REQ2 ABI. Bump the FF-A version
> > preferred by the hypervisor as a precursor to implementing the 1.2-only
> > FFA_MSG_SEND_DIRECT_REQ2 and FFA_MSG_SEND_RESP2 messaging interfaces.
> >
> > We must also use SMCCC 1.2 for 64-bit SMCs if hypervisor negotiated FF-A
> > 1.2, so ffa_set_retval is updated and a new function to call 64-bit smcs
> > using SMCCC 1.2 with fallback to SMCCC 1.1 is introduced.
> >
> > Update ffa_call_supported to mark FF-A 1.2 interfaces as unsupported
> > lest they get forwarded.
> >
> > Co-developed-by: Ayrton Munoz <ayrton@google.com>
> > Signed-off-by: Ayrton Munoz <ayrton@google.com>
> > Signed-off-by: Per Larsen <perlarsen@google.com>
> > ---
> > arch/arm64/kvm/hyp/nvhe/ffa.c | 18 ++++++++++++++----
> > include/linux/arm_ffa.h | 1 +
> > 2 files changed, 15 insertions(+), 4 deletions(-)
>
[..]
Late catching up on this, as we seem to get a version a day, probably
in the hope that it will keep *something* away,...
> > @@ -734,7 +741,10 @@ static int hyp_ffa_post_init(void)
> > if (res.a0 != FFA_SUCCESS)
> > return -EOPNOTSUPP;
> >
> > - switch (res.a2) {
> > + if ((res.a2 & GENMASK(15, 2)) != 0 || res.a3 != 0)
> > + return -EINVAL;
>
> Why are you checking bits a2[15:2] and a3? The spec says they MBZ,
> so we shouldn't care about enforcing that. In fact, adding the check
> probably means we'll fail if those bits get allocated in future.
I have the exact opposite approach. If we don't check that they are 0
for v1.2 and previous versions, we won't be able to tell what they
mean when they are finally allocated to mean something in version
1.337.
Until we support such version, MBZ should be enforced, because we
otherwise don't understand what the "client" is trying to say. And we
don't understand, we're guaranteed to do the wrong thing.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v7 4/5] KVM: arm64: Bump the supported version of FF-A to 1.2
2025-07-31 7:56 ` Marc Zyngier
@ 2025-08-05 14:49 ` Will Deacon
0 siblings, 0 replies; 20+ messages in thread
From: Will Deacon @ 2025-08-05 14:49 UTC (permalink / raw)
To: Marc Zyngier
Cc: perlarsen, Oliver Upton, Joey Gouly, Suzuki K Poulose, Zenghui Yu,
Catalin Marinas, Sudeep Holla, linux-arm-kernel, kvmarm,
linux-kernel, ahomescu, armellel, arve, ayrton, qperret,
sebastianene, qwandor
Hey Marc,
(we discussed this very briefly offline but I wanted to reply for the
benefit of everybody else and also because I don't recall quite where we
ended up)
On Thu, Jul 31, 2025 at 08:56:59AM +0100, Marc Zyngier wrote:
> On Fri, 18 Jul 2025 14:45:17 +0100,
> Will Deacon <will@kernel.org> wrote:
> > On Tue, Jul 01, 2025 at 10:06:37PM +0000, Per Larsen via B4 Relay wrote:
> > > From: Per Larsen <perlarsen@google.com>
> > > @@ -734,7 +741,10 @@ static int hyp_ffa_post_init(void)
> > > if (res.a0 != FFA_SUCCESS)
> > > return -EOPNOTSUPP;
> > >
> > > - switch (res.a2) {
> > > + if ((res.a2 & GENMASK(15, 2)) != 0 || res.a3 != 0)
> > > + return -EINVAL;
> >
> > Why are you checking bits a2[15:2] and a3? The spec says they MBZ,
> > so we shouldn't care about enforcing that. In fact, adding the check
> > probably means we'll fail if those bits get allocated in future.
>
> I have the exact opposite approach. If we don't check that they are 0
> for v1.2 and previous versions, we won't be able to tell what they
> mean when they are finally allocated to mean something in version
> 1.337.
>
> Until we support such version, MBZ should be enforced, because we
> otherwise don't understand what the "client" is trying to say. And we
> don't understand, we're guaranteed to do the wrong thing.
We've lost a bunch of context in the diff here, but there are two
important things to keep in mind at this point:
1. We've negotiated a known version of FF-A, so it won't be v1.337 and
we _should_ be able rely on the spec authors not breaking stuff
retrospectively (famous last words...)
2. The response we're parsing here is something that has come back
from TZ after we (the hypervisor) have called FFA_FEATURES. If
those MBZ bits are non-zero, I think should just ignore them.
Cheers,
Will
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2025-08-05 16:59 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-01 22:06 [PATCH v7 0/5] KVM: arm64: Support FF-A 1.2 and SEND_DIRECT2 ABI Per Larsen via B4 Relay
2025-07-01 22:06 ` [PATCH v7 1/5] KVM: arm64: Correct return value on host version downgrade attempt Per Larsen via B4 Relay
2025-07-01 22:06 ` [PATCH v7 2/5] KVM: arm64: Use SMCCC 1.2 for FF-A initialization and in host handler Per Larsen via B4 Relay
2025-07-03 12:38 ` Marc Zyngier
2025-07-08 0:06 ` Per Larsen
2025-07-18 13:37 ` Will Deacon
2025-07-19 5:54 ` Per Larsen
2025-07-21 11:01 ` Arve Hjønnevåg
2025-07-22 0:20 ` Per Larsen
2025-07-22 15:55 ` Will Deacon
2025-07-01 22:06 ` [PATCH v7 3/5] KVM: arm64: Mark FFA_NOTIFICATION_* calls as unsupported Per Larsen via B4 Relay
2025-07-01 22:06 ` [PATCH v7 4/5] KVM: arm64: Bump the supported version of FF-A to 1.2 Per Larsen via B4 Relay
2025-07-18 13:45 ` Will Deacon
2025-07-31 7:56 ` Marc Zyngier
2025-08-05 14:49 ` Will Deacon
2025-07-01 22:06 ` [PATCH v7 5/5] KVM: arm64: Support FFA_MSG_SEND_DIRECT_REQ2 in host handler Per Larsen via B4 Relay
2025-07-18 13:53 ` Will Deacon
2025-07-21 11:13 ` Arve Hjønnevåg
2025-07-21 22:43 ` Per Larsen
2025-07-22 15:03 ` Will Deacon
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).