Re: [PATCH] KVM: arm64: FFA: Release hyp rx buffer

[Vincent Donnefort <vdonnefort@google.com>]

Hi Sudeep,

On Thu, May 30, 2024 at 02:17:34PM +0100, Vincent Donnefort wrote:
> According to the FF-A spec (1.2 BET0 7.2.2.4.2), after a producer has
> written into a buffer, it is "full" and now owned by the consumer until
> it is read and "empty". This must be signaled by the consumer with the
> RX_RELEASE invocation.
> 
> It is clear from the spec (7.2.2.4.2), that MEM_RETRIEVE_RESP is
> transferring the ownership from producer (in our case SPM) to consumer
> (hypervisor). We must then subsequently invoke RX_RELEASE to transfer
> back the ownership (i.e. to let SPM mark the buffer as "empty").
> 
> It is less clear though what is happening with MEM_FRAG_TX. But this
> invocation, as a response to MEM_FRAG_RX writes into the same hypervisor
> RX buffer. Also this is matching the TF-A implementation where the RX
> buffer is marked "full" during a MEM_FRAG_RX.

Perhaps a change is needed here in the FF-A spec? Not only it seems strange that
there are no mention about MEM_FRAG_RX in the buffer ownership paragraph but
also this seems strange to have to RX_RELEASE: If one invoke MEM_FRAG_RX, that's
probably because the Rx buffer is ready to be read anyway?

> 
> Release the RX hypervisor buffer in those two cases. This will unblock
> later invocations using this buffer which would otherwise fail.
> (RETRIEVE_REQ, MEM_FRAG_RX and PARTITION_INFO_GET).
> 
> Signed-off-by: Vincent Donnefort <vdonnefort@google.com>
> 
> index 02746f9d0980..efb053af331c 100644
> --- a/arch/arm64/kvm/hyp/nvhe/ffa.c
> +++ b/arch/arm64/kvm/hyp/nvhe/ffa.c
> @@ -177,6 +177,14 @@ static void ffa_retrieve_req(struct arm_smccc_res *res, u32 len)
>  			  res);
>  }
>  
> +static void ffa_rx_release(struct arm_smccc_res *res)
> +{
> +	arm_smccc_1_1_smc(FFA_RX_RELEASE,
> +			  0, 0,
> +			  0, 0, 0, 0, 0,
> +			  res);
> +}
> +
>  static void do_ffa_rxtx_map(struct arm_smccc_res *res,
>  			    struct kvm_cpu_context *ctxt)
>  {
> @@ -543,16 +551,19 @@ static void do_ffa_mem_reclaim(struct arm_smccc_res *res,
>  	if (WARN_ON(offset > len ||
>  		    fraglen > KVM_FFA_MBOX_NR_PAGES * PAGE_SIZE)) {
>  		ret = FFA_RET_ABORTED;
> +		ffa_rx_release(res);
>  		goto out_unlock;
>  	}
>  
>  	if (len > ffa_desc_buf.len) {
>  		ret = FFA_RET_NO_MEMORY;
> +		ffa_rx_release(res);
>  		goto out_unlock;
>  	}
>  
>  	buf = ffa_desc_buf.buf;
>  	memcpy(buf, hyp_buffers.rx, fraglen);
> +	ffa_rx_release(res);
>  
>  	for (fragoff = fraglen; fragoff < len; fragoff += fraglen) {
>  		ffa_mem_frag_rx(res, handle_lo, handle_hi, fragoff);
> @@ -563,6 +574,7 @@ static void do_ffa_mem_reclaim(struct arm_smccc_res *res,
>  
>  		fraglen = res->a3;
>  		memcpy((void *)buf + fragoff, hyp_buffers.rx, fraglen);
> +		ffa_rx_release(res);
>  	}
>  
>  	ffa_mem_reclaim(res, handle_lo, handle_hi, flags);
> 
> base-commit: 6d69b6c12fce479fde7bc06f686212451688a102
> -- 
> 2.45.1.288.g0e0cd299f1-goog
>