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

[Sebastian Ene <sebastianene@google.com>]

On Thu, May 30, 2024 at 02:17:34PM +0100, Vincent Donnefort wrote:

Hello Vincent,

> 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.

I agree the spec is a bit unclear in the ownership transfer of the buffer:
- it mentions that the producer owns it when it is empty (I think it is
  a separate state the ownership transfer than being "full"). Some FF-A
  ABIs do the ownership transfer when they are invoked, but not all of
  them and this is why we need calls like RX_RELEASE, to signal the
  availability of the buffer and to transfer the ownership from the
  Consumer to the Producer. Can we add this explanation as part of the
  commit message to justify why we need this ?

I think it is also worth mentioning that this is how TF-A (Trusted
Firmware) deals with the ownership transfer of the buffer.

> 
> 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.
> 
> 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).
> 

Thanks,
Seb


> Signed-off-by: Vincent Donnefort <vdonnefort@google.com>
> 
> diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c
> 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
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel