Re: [PATCH 12/12] xen/arm: ffa: Add message parameter diagnostics

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Jens,

> On 11 Feb 2026, at 14:36, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
> 
> Hi Bertrand,
> 
> On Wed, Feb 11, 2026 at 12:48 PM Bertrand Marquis
> <Bertrand.Marquis@xxxxxxx> wrote:
>> 
>> Hi Jens,
>> 
>>> On 11 Feb 2026, at 11:16, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
>>> 
>>> Hi Bertrand,
>>> 
>>> On Tue, Feb 3, 2026 at 6:38 PM Bertrand Marquis
>>> <bertrand.marquis@xxxxxxx> wrote:
>>>> 
>>>> MSG_SEND2 and direct request validation failures are silent, making it
>>>> hard to diagnose invalid IDs, oversized messages, or unsupported
>>>> destination types.
>>>> 
>>>> Add debug logs for parameter validation failures:
>>>> - invalid endpoint IDs
>>>> - oversized messages
>>>> - unsupported destination types
>>>> - invalid sender/receiver combinations
>>>> - ratelimit MSG_SEND2 busy failures to avoid log flooding
>>>> 
>>>> No functional changes.
>>>> 
>>>> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
>>>> ---
>>>> xen/arch/arm/tee/ffa_msg.c | 45 ++++++++++++++++++++++++++++++++++++++
>>>> 1 file changed, 45 insertions(+)
>>>> 
>>>> diff --git a/xen/arch/arm/tee/ffa_msg.c b/xen/arch/arm/tee/ffa_msg.c
>>>> index 928f269f6c3a..cc273c9a8e80 100644
>>>> --- a/xen/arch/arm/tee/ffa_msg.c
>>>> +++ b/xen/arch/arm/tee/ffa_msg.c
>>>> @@ -4,6 +4,7 @@
>>>> */
>>>> 
>>>> #include <xen/const.h>
>>>> +#include <xen/lib.h>
>>>> #include <xen/sizes.h>
>>>> #include <xen/types.h>
>>>> 
>>>> @@ -100,6 +101,7 @@ void ffa_handle_msg_send_direct_req(struct 
>>>> cpu_user_regs *regs, uint32_t fid)
>>>>    if ( !ffa_fw_supports_fid(fid) )
>>>>    {
>>>>        ret = FFA_RET_NOT_SUPPORTED;
>>>> +        gdprintk(XENLOG_DEBUG, "ffa: direct req fid %#x not supported\n", 
>>>> fid);
>>>>        goto out;
>>>>    }
>>>> 
>>>> @@ -108,6 +110,9 @@ void ffa_handle_msg_send_direct_req(struct 
>>>> cpu_user_regs *regs, uint32_t fid)
>>>>         (src_dst & GENMASK(15,0)) == ffa_get_vm_id(d) )
>>>>    {
>>>>        ret = FFA_RET_INVALID_PARAMETERS;
>>>> +        gdprintk(XENLOG_DEBUG,
>>>> +                 "ffa: direct req invalid src/dst %#x\n",
>>>> +                 (uint32_t)src_dst);
>>>>        goto out;
>>>>    }
>>>> 
>>>> @@ -115,6 +120,9 @@ void ffa_handle_msg_send_direct_req(struct 
>>>> cpu_user_regs *regs, uint32_t fid)
>>>>    if ( !FFA_ID_IS_SECURE(src_dst & GENMASK(15,0)) )
>>>>    {
>>>>        ret = FFA_RET_NOT_SUPPORTED;
>>>> +        gdprintk(XENLOG_DEBUG,
>>>> +                 "ffa: direct req to non-secure dst %#x\n",
>>>> +                 (uint32_t)(src_dst & GENMASK(15, 0)));
>>>>        goto out;
>>>>    }
>>>> 
>>>> @@ -166,7 +174,12 @@ static int32_t ffa_msg_send2_vm(uint16_t dst_id, 
>>>> const void *src_buf,
>>>>    /* This is also checking that dest is not src */
>>>>    ret = ffa_endpoint_domain_lookup(dst_id, &dst_d, &dst_ctx);
>>>>    if ( ret )
>>>> +    {
>>>> +        gdprintk(XENLOG_DEBUG,
>>>> +                 "ffa: msg_send2 lookup failed: dst %#x ret %d\n",
>>>> +                 dst_id, ret);
>>>>        return ret;
>>>> +    }
>>>> 
>>>>    /* This also checks that destination has set a Rx buffer */
>>>>    ret = ffa_rx_acquire(dst_ctx , &rx_buf, &rx_size);
>>>> @@ -199,6 +212,12 @@ static int32_t ffa_msg_send2_vm(uint16_t dst_id, 
>>>> const void *src_buf,
>>>>    /* receiver rx buffer will be released by the receiver*/
>>>> 
>>>> out_unlock:
>>>> +    if ( ret )
>>>> +    {
>>>> +        if ( ret != FFA_RET_BUSY || printk_ratelimit() )
>>> 
>>> Shouldn't this be && ?
>> 
>> The intent here is to log all non BUSY errors but only log BUSY when 
>> ratelimit allows
>> to reduce the amount of logs in case of someone polling when the receiver RX 
>> buffer
>> is busy.
>> 
>> Maybe I should add a comment to make that clearer ?

I will add a comment in v2.

Cheers
Bertrand

> 
> Yes, please.
> 
> Cheers,
> Jens
> 
>> 
>> Cheers
>> Bertrand
>> 
>>> 
>>> Cheers,
>>> Jens
>>> 
>>>> +            gdprintk(XENLOG_DEBUG, "ffa: msg_send2 to %#x failed: %d\n",
>>>> +                     dst_id, ret);
>>>> +    }
>>>>    rcu_unlock_domain(dst_d);
>>>>    if ( !ret )
>>>>        ffa_raise_rx_buffer_full(dst_d);
>>>> @@ -226,7 +245,11 @@ int32_t ffa_handle_msg_send2(struct cpu_user_regs 
>>>> *regs)
>>>> 
>>>>    ret = ffa_tx_acquire(src_ctx, &tx_buf, &tx_size);
>>>>    if ( ret != FFA_RET_OK )
>>>> +    {
>>>> +        gdprintk(XENLOG_DEBUG,
>>>> +                 "ffa: msg_send2 TX acquire failed: %d\n", ret);
>>>>        return ret;
>>>> +    }
>>>> 
>>>>    /* create a copy of the message header */
>>>>    memcpy(&src_msg, tx_buf, sizeof(src_msg));
>>>> @@ -238,6 +261,9 @@ int32_t ffa_handle_msg_send2(struct cpu_user_regs 
>>>> *regs)
>>>>         dst_id == ffa_get_vm_id(src_d) )
>>>>    {
>>>>        ret = FFA_RET_INVALID_PARAMETERS;
>>>> +        gdprintk(XENLOG_DEBUG,
>>>> +                 "ffa: msg_send2 invalid src/dst src %#x dst %#x\n",
>>>> +                 src_id, dst_id);
>>>>        goto out;
>>>>    }
>>>> 
>>>> @@ -246,6 +272,9 @@ int32_t ffa_handle_msg_send2(struct cpu_user_regs 
>>>> *regs)
>>>>        if (src_msg.msg_offset < sizeof(struct ffa_part_msg_rxtx_1_1))
>>>>        {
>>>>            ret = FFA_RET_INVALID_PARAMETERS;
>>>> +            gdprintk(XENLOG_DEBUG,
>>>> +                     "ffa: msg_send2 invalid msg_offset %u (v1.1)\n",
>>>> +                     src_msg.msg_offset);
>>>>            goto out;
>>>>        }
>>>>        /* Set uuid to Nil UUID for v1.1 guests */
>>>> @@ -255,6 +284,9 @@ int32_t ffa_handle_msg_send2(struct cpu_user_regs 
>>>> *regs)
>>>>    else if ( src_msg.msg_offset < sizeof(struct ffa_part_msg_rxtx_1_2) )
>>>>    {
>>>>        ret = FFA_RET_INVALID_PARAMETERS;
>>>> +        gdprintk(XENLOG_DEBUG,
>>>> +                 "ffa: msg_send2 invalid msg_offset %u (v1.2)\n",
>>>> +                 src_msg.msg_offset);
>>>>        goto out;
>>>>    }
>>>> 
>>>> @@ -263,6 +295,9 @@ int32_t ffa_handle_msg_send2(struct cpu_user_regs 
>>>> *regs)
>>>>            src_msg.msg_size > (tx_size - src_msg.msg_offset) )
>>>>    {
>>>>        ret = FFA_RET_INVALID_PARAMETERS;
>>>> +        gdprintk(XENLOG_DEBUG,
>>>> +                 "ffa: msg_send2 invalid msg_size %u offset %u tx %zu\n",
>>>> +                 src_msg.msg_size, src_msg.msg_offset, tx_size);
>>>>        goto out;
>>>>    }
>>>> 
>>>> @@ -272,6 +307,8 @@ int32_t ffa_handle_msg_send2(struct cpu_user_regs 
>>>> *regs)
>>>>        if ( !ffa_fw_supports_fid(FFA_MSG_SEND2) )
>>>>        {
>>>>            ret = FFA_RET_NOT_SUPPORTED;
>>>> +            gdprintk(XENLOG_DEBUG,
>>>> +                     "ffa: msg_send2 to SP not supported\n");
>>>>            goto out;
>>>>        }
>>>>        /*
>>>> @@ -288,6 +325,8 @@ int32_t ffa_handle_msg_send2(struct cpu_user_regs 
>>>> *regs)
>>>> 
>>>>        ret = ffa_simple_call(FFA_MSG_SEND2,
>>>>                              ((uint32_t)ffa_get_vm_id(src_d)) << 16, 0, 0, 
>>>> 0);
>>>> +        if ( ret )
>>>> +            gdprintk(XENLOG_DEBUG, "ffa: msg_send2 to SP failed: %d\n", 
>>>> ret);
>>>>    }
>>>>    else if ( IS_ENABLED(CONFIG_FFA_VM_TO_VM) )
>>>>    {
>>>> @@ -295,7 +334,11 @@ int32_t ffa_handle_msg_send2(struct cpu_user_regs 
>>>> *regs)
>>>>        ret = ffa_msg_send2_vm(dst_id, tx_buf, &src_msg);
>>>>    }
>>>>    else
>>>> +    {
>>>>        ret = FFA_RET_INVALID_PARAMETERS;
>>>> +        gdprintk(XENLOG_DEBUG,
>>>> +                 "ffa: msg_send2 to VM disabled (dst %#x)\n", dst_id);
>>>> +    }
>>>> 
>>>> out:
>>>>    ffa_tx_release(src_ctx);
>>>> @@ -311,6 +354,7 @@ void ffa_handle_run(struct cpu_user_regs *regs, 
>>>> uint32_t fid)
>>>>    if ( !ffa_fw_supports_fid(fid) )
>>>>    {
>>>>        ret = FFA_RET_NOT_SUPPORTED;
>>>> +        gdprintk(XENLOG_DEBUG, "ffa: run fid %#x not supported\n", fid);
>>>>        goto out;
>>>>    }
>>>> 
>>>> @@ -322,6 +366,7 @@ void ffa_handle_run(struct cpu_user_regs *regs, 
>>>> uint32_t fid)
>>>>    if ( !FFA_ID_IS_SECURE(dst >> 16) )
>>>>    {
>>>>        ret = FFA_RET_NOT_SUPPORTED;
>>>> +        gdprintk(XENLOG_DEBUG, "ffa: run to non-secure dst %#x\n", dst);
>>>>        goto out;
>>>>    }
>>>> 
>>>> --
>>>> 2.50.1 (Apple Git-155)