Re: [PATCH 6/6] xen/arm: ffa: Deliver VM-to-VM notifications locally

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Jens,

> On 22 Apr 2026, at 16:04, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
> 
> Hi Bertrand,
> 
> On Fri, Apr 17, 2026 at 3:41 PM Bertrand Marquis
> <bertrand.marquis@xxxxxxx> wrote:
>> 
>> VM notification binding and pending tracking exist for non-secure
>> endpoints, but FFA_NOTIFICATION_SET still only forwards secure
>> destinations to the SPMC. Non-secure VMs therefore cannot receive
>> notifications from other VMs. Local NPI delivery also needs explicit
>> re-arm tracking so repeated raises are not lost while the interrupt is
>> already pending.
>> 
>> Add a local VM notification delivery path for non-secure destinations.
>> notification_set_vm() resolves the destination endpoint, verifies that
>> every requested bit is bound to the sender, sets the receiver's
>> vm_pending bitmap under notif_lock, and raises an NPI only when the
>> receiver transitions from no local pending notifications to some.
>> 
>> Track whether a local NPI is already armed with notif_irq_raised, clear
>> that state once both VM and hypervisor pending bitmaps are drained, and
>> roll back newly-added VM pending bits if no destination vCPU is online.
>> Also expose firmware notification availability so FFA_FEATURES only
>> advertises notification support when it is actually provided by the
>> firmware or by CONFIG_FFA_VM_TO_VM.
>> 
>> Functional impact: when CONFIG_FFA_VM_TO_VM is enabled, non-secure
>> FFA_NOTIFICATION_SET delivers VM-to-VM notifications locally and keeps
>> NPI delivery reliable across repeated raises.
>> 
>> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
>> ---
>> xen/arch/arm/tee/ffa.c         |  24 +++++--
>> xen/arch/arm/tee/ffa_notif.c   | 126 +++++++++++++++++++++++++++++++--
>> xen/arch/arm/tee/ffa_private.h |  11 ++-
>> 3 files changed, 147 insertions(+), 14 deletions(-)
>> 
>> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
>> index 1fe33f26454a..7fe021049cba 100644
>> --- a/xen/arch/arm/tee/ffa.c
>> +++ b/xen/arch/arm/tee/ffa.c
>> @@ -39,8 +39,13 @@
>>  * o FFA_MSG_SEND_DIRECT_REQ:
>>  *   - only supported from a VM to an SP
>>  * o FFA_NOTIFICATION_*:
>> + *   - only supported when firmware notifications are enabled or VM-to-VM
>> + *     support is built in
>>  *   - only supports global notifications, that is, per vCPU notifications
>> - *     are not supported
>> + *     are not supported and secure per-vCPU notification information is
>> + *     not forwarded
>> + *   - the source endpoint ID reported for a notification may no longer
>> + *     exist by the time the receiver consumes it
>>  *   - doesn't support signalling the secondary scheduler of pending
>>  *     notification for secure partitions
>>  *   - doesn't support notifications for Xen itself
>> @@ -245,6 +250,8 @@ static void handle_features(struct cpu_user_regs *regs)
>>     uint32_t a1 = get_user_reg(regs, 1);
>>     struct domain *d = current->domain;
>>     struct ffa_ctx *ctx = d->arch.tee;
>> +    bool notif_supported = IS_ENABLED(CONFIG_FFA_VM_TO_VM) ||
>> +                           ffa_notif_fw_enabled();
>> 
>>     /*
>>      * FFA_FEATURES defines w2 as input properties only for specific
>> @@ -343,10 +350,16 @@ static void handle_features(struct cpu_user_regs *regs)
>> 
>>         break;
>>     case FFA_FEATURE_NOTIF_PEND_INTR:
>> -        ffa_set_regs_success(regs, GUEST_FFA_NOTIF_PEND_INTR_ID, 0);
>> +        if ( notif_supported )
>> +            ffa_set_regs_success(regs, GUEST_FFA_NOTIF_PEND_INTR_ID, 0);
>> +        else
>> +            ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
>>         break;
>>     case FFA_FEATURE_SCHEDULE_RECV_INTR:
>> -        ffa_set_regs_success(regs, GUEST_FFA_SCHEDULE_RECV_INTR_ID, 0);
>> +        if ( notif_supported )
>> +            ffa_set_regs_success(regs, GUEST_FFA_SCHEDULE_RECV_INTR_ID, 0);
>> +        else
>> +            ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
>>         break;
>>     case FFA_PARTITION_INFO_GET_REGS:
>>         if ( ACCESS_ONCE(ctx->guest_vers) >= FFA_VERSION_1_2 )
>> @@ -361,7 +374,10 @@ static void handle_features(struct cpu_user_regs *regs)
>>     case FFA_NOTIFICATION_SET:
>>     case FFA_NOTIFICATION_INFO_GET_32:
>>     case FFA_NOTIFICATION_INFO_GET_64:
>> -        ffa_set_regs_success(regs, 0, 0);
>> +        if ( notif_supported )
>> +            ffa_set_regs_success(regs, 0, 0);
>> +        else
>> +            ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
>>         break;
>>     default:
>>         ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
>> diff --git a/xen/arch/arm/tee/ffa_notif.c b/xen/arch/arm/tee/ffa_notif.c
>> index 4def701f0130..e77321244926 100644
>> --- a/xen/arch/arm/tee/ffa_notif.c
>> +++ b/xen/arch/arm/tee/ffa_notif.c
>> @@ -20,7 +20,12 @@ static bool __ro_after_init fw_notif_enabled;
>> static unsigned int __ro_after_init notif_sri_irq;
>> static DEFINE_SPINLOCK(notif_info_lock);
>> 
>> -static void inject_notif_pending(struct domain *d)
>> +bool ffa_notif_fw_enabled(void)
>> +{
>> +    return fw_notif_enabled;
>> +}
>> +
>> +static bool inject_notif_pending(struct domain *d)
>> {
>>     struct vcpu *v;
>> 
>> @@ -34,13 +39,15 @@ static void inject_notif_pending(struct domain *d)
>>         if ( is_vcpu_online(v) )
>>         {
>>             vgic_inject_irq(d, v, GUEST_FFA_NOTIF_PEND_INTR_ID, true);
>> -            return;
>> +            return true;
>>         }
>>     }
>> 
>>     if ( printk_ratelimit() )
>>         printk(XENLOG_G_DEBUG "%pd: ffa: can't inject NPI, all vCPUs 
>> offline\n",
>>                d);
>> +
>> +    return false;
>> }
>> 
>> static int32_t ffa_notif_parse_params(uint16_t dom_id, uint16_t caller_id,
>> @@ -104,6 +111,73 @@ out_unlock:
>>     return ret;
>> }
>> 
>> +/*
>> + * Deliver a VM-to-VM notification. ctx->notif.notif_lock protects
>> + * vm_bind/vm_pending so callers must not hold it already.
>> + */
>> +static int32_t notification_set_vm(uint16_t dst_id, uint16_t src_id,
>> +                                   uint32_t flags, uint64_t bitmap)
>> +{
>> +    struct domain *dst_d;
>> +    struct ffa_ctx *dst_ctx;
>> +    unsigned int id;
>> +    int32_t ret;
>> +    uint64_t prev_bitmap = 0;
>> +    uint64_t new_bitmap;
>> +    bool inject = false;
>> +
>> +    if ( flags )
>> +        return FFA_RET_INVALID_PARAMETERS;
>> +
>> +    ret = ffa_endpoint_domain_lookup(dst_id, &dst_d, &dst_ctx);
>> +    if ( ret )
>> +        return ret;
>> +
>> +    ret = FFA_RET_OK;
>> +
>> +    spin_lock(&dst_ctx->notif.notif_lock);
>> +
>> +    for ( id = 0; id < FFA_NUM_VM_NOTIF; id++ )
>> +    {
>> +        if ( !(bitmap & BIT(id, ULL)) )
>> +            continue;
>> +
>> +        if ( dst_ctx->notif.vm_bind[id] != src_id )
>> +        {
>> +            ret = FFA_RET_DENIED;
>> +            goto out_unlock;
>> +        }
>> +    }
>> +
>> +    prev_bitmap = dst_ctx->notif.vm_pending;
>> +    dst_ctx->notif.vm_pending |= bitmap;
>> +    if ( !dst_ctx->notif.notif_irq_raised &&
>> +         (dst_ctx->notif.vm_pending || dst_ctx->notif.hyp_pending) )
>> +    {
>> +        dst_ctx->notif.notif_irq_raised = true;
>> +        inject = true;
>> +    }
>> +
>> +out_unlock:
>> +    spin_unlock(&dst_ctx->notif.notif_lock);
>> +
>> +    new_bitmap = bitmap & ~prev_bitmap;
>> +    if ( ret == FFA_RET_OK && inject && new_bitmap &&
>> +         !inject_notif_pending(dst_d) )
>> +    {
>> +        spin_lock(&dst_ctx->notif.notif_lock);
>> +        dst_ctx->notif.vm_pending &= ~new_bitmap;
> 
> There's a window above when dst_ctx->notif.notif_lock is unlocked.
> What if another CPU has modified dst_ctx->notif.vm_pending during that
> window?

You are right, there is a race in that window. Thanks for the finding :-)

I think the simplest fix is to keep notif_lock held across the injection
attempt, so we do not expose partially updated notification state while the
interrupt delivery is still in progress.

I do not think we should roll the pending bits back on injection failure,
though. In practice the cases where we fail to inject are cases where the
target cannot currently take the notification anyway, and dropping the
pending state would be worse than keeping it pending and not marking the IRQ
as raised.

So my plan would be to keep the pending state, only mark notif_irq_raised
when we actually inject, and keep the existing debug print for the cases we
can detect.

If that approach looks OK to you, I will update this patch accordingly.

The same issue exists in patch 2 for the RX-buffer-full path, so I will fix
it there as well for the same reason.

Cheers
Bertrand