Message-ID: <20250513145642.GB9443@willie-the-truck>
Date: Tue, 13 May 2025 15:56:43 +0100
From: Will Deacon <will@...nel.org>
To: perlarsen@...gle.com
Cc: Marc Zyngier <maz@...nel.org>, Oliver Upton <oliver.upton@...ux.dev>,
	Joey Gouly <joey.gouly@....com>,
	Suzuki K Poulose <suzuki.poulose@....com>,
	Zenghui Yu <yuzenghui@...wei.com>,
	Catalin Marinas <catalin.marinas@....com>,
	Sudeep Holla <sudeep.holla@....com>,
	linux-arm-kernel@...ts.infradead.org, kvmarm@...ts.linux.dev,
	linux-kernel@...r.kernel.org, sebastianene@...gle.com,
	lpieralisi@...nel.org, arve@...roid.com, qwandor@...gle.com,
	kernel-team@...roid.com, armellel@...gle.com, perl@...unant.com,
	jean-philippe@...aro.org, ahomescu@...gle.com, tabba@...gle.com,
	qperret@...gle.com, james.morse@....com,
	Ayrton Munoz <ayrton@...gle.com>
Subject: Re: [PATCH v3 2/3] KVM: arm64: Bump the supported version of FF-A to
 1.2

On Tue, May 13, 2025 at 06:28:31AM +0000, Per Larsen via B4 Relay wrote:
> From: Per Larsen <perlarsen@...gle.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 deny-list in ffa_call_supported to mark FFA_NOTIFICATION_* and
> interfaces added in FF-A 1.2 as unsupported lest they get forwarded.
> 
> Co-developed-by: Ayrton Munoz <ayrton@...gle.com>
> Signed-off-by: Ayrton Munoz <ayrton@...gle.com>
> Signed-off-by: Per Larsen <perlarsen@...gle.com>
> Signed-off-by: Per Larsen <perl@...unant.com>
> ---
>  arch/arm64/kvm/hyp/include/nvhe/ffa.h |  1 +
>  arch/arm64/kvm/hyp/nvhe/Makefile      |  1 +
>  arch/arm64/kvm/hyp/nvhe/ffa.c         | 88 ++++++++++++++++++++++++++++++++---
>  include/linux/arm_ffa.h               |  1 +
>  4 files changed, 85 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/include/nvhe/ffa.h b/arch/arm64/kvm/hyp/include/nvhe/ffa.h
> index 146e0aebfa1c7c9834c75a9a29bf87eb6f94f436..02def6fe51f5079b12c168585e12f862211c4c91 100644
> --- a/arch/arm64/kvm/hyp/include/nvhe/ffa.h
> +++ b/arch/arm64/kvm/hyp/include/nvhe/ffa.h
> @@ -13,5 +13,6 @@
>  
>  int hyp_ffa_init(void *pages);
>  bool kvm_host_ffa_handler(struct kvm_cpu_context *host_ctxt, u32 func_id);
> +u32 ffa_get_hypervisor_version(void);

No need to expose this outside of ffa.c (i.e. we can make the function
definition 'static').

>  #endif /* __KVM_HYP_FFA_H */
> diff --git a/arch/arm64/kvm/hyp/nvhe/Makefile b/arch/arm64/kvm/hyp/nvhe/Makefile
> index b43426a493df5a388caa920e259cc8c54d118a1b..95404ff16dac0389f45a3ee2c13a93b3ebebaf6d 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..403fde6ca4d6ec49566ef60709cedbaef9f04592 100644
> --- a/arch/arm64/kvm/hyp/nvhe/ffa.c
> +++ b/arch/arm64/kvm/hyp/nvhe/ffa.c
> @@ -101,6 +101,55 @@ static void ffa_set_retval(struct kvm_cpu_context *ctxt,
>  	cpu_reg(ctxt, 1) = res->a1;
>  	cpu_reg(ctxt, 2) = res->a2;
>  	cpu_reg(ctxt, 3) = res->a3;
> +
> +	/*
> +	 * Other result registers must be zero per DEN0077A but SMC32/HVC32 must
> +	 * preserve x8-x30 per DEN0028.
> +	 */
> +	cpu_reg(ctxt, 4) = 0;
> +	cpu_reg(ctxt, 5) = 0;
> +	cpu_reg(ctxt, 6) = 0;
> +	cpu_reg(ctxt, 7) = 0;

I don't think it's safe to zero these registers unconditionally. AFAICT,
R4-R7 were only allocated as result registers in SMCCC v1.2 and the
definition of 'struct arm_smccc_res' reflects that. If we blindly zero
these registers in the calling CPU context, we could end up corrupting
state that the compiler expects the asm containing the SMC instruction
to preserve.

> @@ -628,6 +677,20 @@ 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:

Please can you send this part as a separate patch? I think we'll be
passing these FFA_NOTIFICATION_ calls straight through to TZ as it
stands, which could result in unpleasant behaviours and might be
something we want to plug in the stable kernels irrespective of support
for v1.2.

> +	/* Unimplemented interfaces added in FF-A 1.2 */
> +	case FFA_MSG_SEND_DIRECT_REQ2:
> +	case FFA_MSG_SEND_DIRECT_RESP2:
> +	case FFA_CONSOLE_LOG:
> +	case FFA_PARTITION_INFO_GET_REGS:
> +	case FFA_EL3_INTR_HANDLE:
>  		return false;
>  	}
>  
> @@ -680,7 +743,7 @@ static int hyp_ffa_post_init(void)
>  	if (res.a0 != FFA_SUCCESS)
>  		return -EOPNOTSUPP;
>  
> -	switch (res.a2) {
> +	switch (res.a2 & FFA_FEAT_RXTX_MIN_SZ_MASK) {
>  	case FFA_FEAT_RXTX_MIN_SZ_4K:
>  		min_rxtx_sz = SZ_4K;
>  		break;
> @@ -861,6 +924,18 @@ bool kvm_host_ffa_handler(struct kvm_cpu_context *host_ctxt, u32 func_id)
>  	return true;
>  }
>  
> +u32 ffa_get_hypervisor_version(void)

As above, this can be static.

> +{
> +	u32 version = 0;
> +
> +	hyp_spin_lock(&version_lock);
> +	if (has_version_negotiated)

This looks a bit weird to me. We should just be able to use
'hyp_ffa_version' directly because kvm_host_ffa_handler() checks
'has_version_negotiated' already (with smp_load_acquire()).

The weird case is when negotiation has _not_ completed:


        if (func_id != FFA_VERSION &&
            !smp_load_acquire(&has_version_negotiated)) {
                ffa_to_smccc_error(&res, FFA_RET_INVALID_PARAMETERS);
                goto out_handled;
        }

	...
out_handled:
	ffa_set_retval(host_ctxt, &res);


This is odd because we're now trying to return an error due to the
version not being negotiated, but we need the version to return the
error. How are we supposed to handle that per the spec?

Will

Powered by blists - more mailing lists