Re: [PATCH v2 4/4] KVM: arm64: Release the ownership of the hyp rx buffer to Trustzone

[Sudeep Holla]

On Wed, Mar 05, 2025 at 12:45:23AM +0000, Will Deacon wrote:
> On Thu, Feb 27, 2025 at 06:17:49PM +0000, Sebastian Ene wrote:
> > Introduce the release FF-A call to notify Trustzone that the hypervisor
> > has finished copying the data from the buffer shared with Trustzone to
> > the non-secure partition.
> > 
> > Reported-by: Andrei Homescu <ahomescu@xxxxxxxxxx>
> > Signed-off-by: Sebastian Ene <sebastianene@xxxxxxxxxx>
> > ---
> >  arch/arm64/kvm/hyp/nvhe/ffa.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c
> > index 861f24de97cb..7da0203f1ee9 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/ffa.c
> > +++ b/arch/arm64/kvm/hyp/nvhe/ffa.c
> > @@ -725,6 +725,7 @@ static void do_ffa_part_get(struct arm_smccc_res *res,
> >  	DECLARE_REG(u32, uuid3, ctxt, 4);
> >  	DECLARE_REG(u32, flags, ctxt, 5);
> >  	u32 count, partition_sz, copy_sz;
> > +	struct arm_smccc_res _res;
> >  
> >  	hyp_spin_lock(&host_buffers.lock);
> >  	if (!host_buffers.rx) {
> > @@ -741,7 +742,7 @@ static void do_ffa_part_get(struct arm_smccc_res *res,
> >  
> >  	count = res->a2;
> >  	if (!count)
> > -		goto out_unlock;
> > +		goto release_rx;
> >  
> >  	if (hyp_ffa_version > FFA_VERSION_1_0) {
> >  		/* Get the number of partitions deployed in the system */
> > @@ -757,10 +758,12 @@ static void do_ffa_part_get(struct arm_smccc_res *res,
> >  	copy_sz = partition_sz * count;
> >  	if (copy_sz > KVM_FFA_MBOX_NR_PAGES * PAGE_SIZE) {
> >  		ffa_to_smccc_res(res, FFA_RET_ABORTED);
> > -		goto out_unlock;
> > +		goto release_rx;
> >  	}
> >  
> >  	memcpy(host_buffers.rx, hyp_buffers.rx, copy_sz);
> > +release_rx:
> > +	ffa_rx_release(&_res);
> 
> Hmm, the FFA spec is characteristically unclear as to whether or not we
> need to release the rx buffer in the case that the flags indicate use of
> the rx buffer but the returned partition count is 0.
> 
> Sudeep -- do you know what we should be doing in that case?
> 

We need to call RX_RELEASE here. I went back to the spec to confirm the
same again.

v1.2 EAC0 spec Section 7.2.2.4.2 Transfer of buffer ownership
(Or just look for the section title in any version of the spec)
"
2. Ownership transfer for the RX buffer takes place as follows.
    2. For a framework message,
       1. Completion of the FFA_PARTITION_INFO_GET ABI transfers the ownership
       of the caller’s RX buffer from the Producer to the Consumer.
3. For both types of messages, an invocation of the following FF-A ABIs
    transfers the ownership from the Consumer to the Producer.
       1. FFA_MSG_WAIT ...
       2. FFA_RX_RELEASE.
"

Hope that helps, can dig deeper if there are any ambiguities around this.

-- 
Regards,
Sudeep