diff mbox series

KVM: arm64: FFA: Release hyp rx buffer

Message ID 20240530131734.2724454-1-vdonnefort@google.com (mailing list archive)
State New, archived
Headers show
Series KVM: arm64: FFA: Release hyp rx buffer | expand

Commit Message

Vincent Donnefort May 30, 2024, 1:17 p.m. UTC 
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.

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>


base-commit: 6d69b6c12fce479fde7bc06f686212451688a102

Comments

Vincent Donnefort May 31, 2024, 5:25 p.m. UTC | #1 
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
>
Sebastian Ene June 11, 2024, 1:56 p.m. UTC | #2 
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
>
Sudeep Holla June 11, 2024, 3:47 p.m. UTC | #3 
On Fri, May 31, 2024 at 06:25:46PM +0100, Vincent Donnefort wrote:
> 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?
>

Yes, it is kind of implicit.

Under section "Transmission of transaction descriptor in fragments"
subsection "Description", you can see the below statement(just to avoid spec
document version confusion)

"A Receiver must request the Sender to transmit the next fragment through an
invocation of the FFA_MEM_FRAG_RX ABI".

By that it means the receiver(e.g. SPM) has received the current fragment
sent by the sender via FFA_MEM_FRAG_TX(or specific mem API for first fragment)
and it is ready to receive the next when it invokes FFA_MEM_FRAG_RX.

If you think anything can be improved, happy to take it spec authors. I may
be missing to see something obvious.

--
Regards,
Sudeep
Sudeep Holla June 11, 2024, 4:17 p.m. UTC | #4 
On Tue, Jun 11, 2024 at 01:56:46PM +0000, Sebastian Ene wrote:
> 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

I prefer to drop all these section and spec version details as it may be
stale info in few months time. Better to just refer the section by name
if you are referring non release versions of the document.

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

I think above statement might add more confusion than clarity IMO.

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

This part looks good and can be added. But IMO, not a must, I am fine
either way.

Sorry I don't know how else to improve the commit message.

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

This section clarifies the need for this patch IMO. So I am fine with it
as along as the above info is present.

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

I have responded to this separately, hope that makes sense. You can even
drop this section. I am fine either way.

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

The change itself looks good to me.

Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

--
Regards,
Sudeep
diff mbox series

Patch

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