Hi Jens,
> On 11 Feb 2026, at 08:39, Jens Wiklander <[email protected]> wrote:
>
> Hi Bertrand,
>
> On Tue, Feb 3, 2026 at 6:38 PM Bertrand Marquis
> <[email protected]> wrote:
>>
>> rxtx_unmap() checks RX ownership without holding the RX/TX locks and
>> only enforces the ownership rule when FFA_RX_ACQUIRE is supported. This
>> allows a vCPU to acquire RX between the check and unmap, and it lets
>> RXTX_UNMAP proceed while RX is owned when buffers are not forwarded to
>> firmware.
>>
>> Hold rx_lock/tx_lock across the ownership check and unmap, and deny
>> RXTX_UNMAP whenever RX is owned, independent of RX_ACQUIRE support. For
>> teardown, release RX ownership under the same lock window; use
>> FFA_RX_RELEASE directly because rx_lock is held, and clear the local
>> flag when the firmware path is unavailable.
>>
>> Functional impact: RXTX_UNMAP now reliably returns DENIED while RX is
>> owned, and teardown releases/clears ownership without a race.
>>
>> Signed-off-by: Bertrand Marquis <[email protected]>
>> ---
>> xen/arch/arm/tee/ffa_rxtx.c | 36 +++++++++++++++++++++++++++++++++---
>> 1 file changed, 33 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/arm/tee/ffa_rxtx.c b/xen/arch/arm/tee/ffa_rxtx.c
>> index eff95a7955d7..450ce102cbdc 100644
>> --- a/xen/arch/arm/tee/ffa_rxtx.c
>> +++ b/xen/arch/arm/tee/ffa_rxtx.c
>> @@ -220,7 +220,7 @@ err_unlock_rxtx:
>> return ret;
>> }
>>
>> -static int32_t rxtx_unmap(struct domain *d)
>> +static int32_t rxtx_unmap(struct domain *d, bool teardown)
>> {
>> struct ffa_ctx *ctx = d->arch.tee;
>> int32_t ret = FFA_RET_OK;
>> @@ -234,6 +234,36 @@ static int32_t rxtx_unmap(struct domain *d)
>> goto err_unlock_rxtx;
>> }
>>
>> + if ( !ctx->rx_is_free )
>> + {
>> + if ( teardown )
>> + {
>> + if ( ffa_fw_supports_fid(FFA_RX_ACQUIRE) )
>> + {
>> + int32_t rel_ret;
>> +
>> + /* Can't use ffa_rx_release() while holding rx_lock. */
>> + rel_ret = ffa_simple_call(FFA_RX_RELEASE, ctx->ffa_id,
>> + 0, 0, 0);
>> + if ( rel_ret )
>> + gdprintk(XENLOG_DEBUG,
>> + "ffa: RX release during teardown failed: %d\n",
>> + rel_ret);
>> + else
>> + ctx->rx_is_free = true;
>
> I don't see why this assignment is needed, or the one just below.
True, in the teardown case we do not care at all.
I will remove those 2.
>
>> + }
>> + else
>> + ctx->rx_is_free = true;
>> + }
>> + else
>> + {
>> + gdprintk(XENLOG_DEBUG,
>> + "ffa: RXTX_UNMAP denied, RX buffer owned by VM\n");
>> + ret = FFA_RET_DENIED;
>> + goto err_unlock_rxtx;
>> + }
>> + }
>> +
>> if ( ffa_fw_supports_fid(FFA_RX_ACQUIRE) )
>> {
>> ret = ffa_rxtx_unmap(ffa_get_vm_id(d));
>> @@ -261,7 +291,7 @@ err_unlock_rxtx:
>>
>> int32_t ffa_handle_rxtx_unmap(void)
>> {
>> - return rxtx_unmap(current->domain);
>> + return rxtx_unmap(current->domain, false);
>> }
>>
>> int32_t ffa_rx_acquire(struct ffa_ctx *ctx, void **buf, size_t *buf_size)
>> @@ -369,7 +399,7 @@ int32_t ffa_rxtx_domain_init(struct domain *d)
>>
>> void ffa_rxtx_domain_destroy(struct domain *d)
>> {
>> - rxtx_unmap(d);
>> + rxtx_unmap(d, true);
>
> How about adding a /* teardown */ just after true as a reminder of
> what true is supposed to represent.
>
> Adding such comments isn't very common, but we're doing it at a few
> places in the source tree, and I think it helps when reading the code.
Agree i will add /* teardown */ just after true here.
Cheers
Bertrand
