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

[Jens Wiklander <jens.wiklander@xxxxxxxxxx>]

Hi Bertrand,

On Thu, Apr 23, 2026 at 10:17 AM Bertrand Marquis
<Bertrand.Marquis@xxxxxxx> wrote:
>
> 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.

Sounds good.

Cheers,
Jens