Re: [PATCH 2/6] xen/arm: ffa: Track hypervisor notifications in a bitmap

[Jens Wiklander <jens.wiklander@xxxxxxxxxx>]

Hi Bertrand,

On Thu, Apr 23, 2026 at 9:30 AM Bertrand Marquis
<Bertrand.Marquis@xxxxxxx> wrote:
>
> Hi Jens,
>
> Thanks a lot for the review.
>
> > On 22 Apr 2026, at 11:34, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
> >
> > Hi Bertrand,
> >
> > On Fri, Apr 17, 2026 at 3:41 PM Bertrand Marquis
> > <bertrand.marquis@xxxxxxx> wrote:
> >>
> >> Hypervisor notifications are currently tracked with a dedicated
> >> buff_full_pending boolean. That state only represents a single HYP
> >> notification bit and keeps HYP bitmap handling tied to single-purpose
> >> bookkeeping.
> >>
> >> Replace the boolean with a hypervisor notification bitmap protected by
> >> notif_lock. INFO_GET reports pending when the bitmap is non-zero, GET
> >> returns and clears the HYP bitmap under the lock, and RX-buffer-full
> >> sets FFA_NOTIF_RX_BUFFER_FULL in the bitmap instead of updating
> >> separate state.
> >>
> >> Initialize and clear the bitmap during domain lifecycle handling, and
> >> use ctx->ffa_id for bitmap create and destroy so the notification state
> >> stays tied to the cached FF-A endpoint ID.
> >>
> >> No functional changes.
> >>
> >> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> >> ---
> >> xen/arch/arm/tee/ffa_notif.c   | 46 ++++++++++++++++++++++++++--------
> >> xen/arch/arm/tee/ffa_private.h |  9 +++++--
> >> 2 files changed, 43 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/xen/arch/arm/tee/ffa_notif.c b/xen/arch/arm/tee/ffa_notif.c
> >> index 07bc5cb3a430..d15119409a25 100644
> >> --- a/xen/arch/arm/tee/ffa_notif.c
> >> +++ b/xen/arch/arm/tee/ffa_notif.c
> >> @@ -94,8 +94,15 @@ void ffa_handle_notification_info_get(struct 
> >> cpu_user_regs *regs)
> >>
> >>     notif_pending = test_and_clear_bool(ctx->notif.secure_pending);
> >>     if ( IS_ENABLED(CONFIG_FFA_VM_TO_VM) )
> >> +    {
> >>         notif_pending |= test_and_clear_bool(ctx->notif.vm_pending);
> >>
> >> +        spin_lock(&ctx->notif.notif_lock);
> >> +        if ( ctx->notif.hyp_pending )
> >> +            notif_pending = true;
> >> +        spin_unlock(&ctx->notif.notif_lock);
> >
> > Isn't this a functional change? Before this patch, we didn't consider
> > ctx->notif.buff_full_pending here. Am I missing something?
>
> We did consider it implicitly through vm_pending.
>
> This patch makes that cleaner by using hyp_pending for the Hypervisor
> framework notification itself. Previously, RX-buffer-full was made visible
> indirectly via vm_pending, and FFA_NOTIFICATION_INFO_GET
> cleared that summary state.
>
> As a result, the guest-visible pending indication could be lost before
> the Hypervisor notification was actually retrieved with
> FFA_NOTIFICATION_GET.
>
> With this change, the pending state is tracked in hyp_pending and is only
> cleared when the Hypervisor notifications are retrieved through
> FFA_NOTIFICATION_GET.
>
> I will reword the commit message to make that clearer.

Thanks

>
> >
> >> +    }
> >> +
> >>     if ( notif_pending )
> >>     {
> >>         /* A pending global notification for the guest */
> >> @@ -174,12 +181,17 @@ void ffa_handle_notification_get(struct 
> >> cpu_user_regs *regs)
> >>             w6 = resp.a6;
> >>     }
> >>
> >> -    if ( IS_ENABLED(CONFIG_FFA_VM_TO_VM) &&
> >> -          flags & FFA_NOTIF_FLAG_BITMAP_HYP &&
> >> -          test_and_clear_bool(ctx->notif.buff_full_pending) )
> >> +    if ( IS_ENABLED(CONFIG_FFA_VM_TO_VM) )
> >>     {
> >> -        ACCESS_ONCE(ctx->notif.vm_pending) = false;
> >> -        w7 = FFA_NOTIF_RX_BUFFER_FULL;
> >> +        spin_lock(&ctx->notif.notif_lock);
> >> +
> >> +        if ( (flags & FFA_NOTIF_FLAG_BITMAP_HYP) && 
> >> ctx->notif.hyp_pending )
> >> +        {
> >> +            w7 = ctx->notif.hyp_pending;
> >> +            ctx->notif.hyp_pending = 0;
> >> +        }
> >> +
> >> +        spin_unlock(&ctx->notif.notif_lock);
> >>     }
> >>
> >>     ffa_set_regs(regs, FFA_SUCCESS_32, 0, w2, w3, w4, w5, w6, w7);
> >> @@ -207,12 +219,17 @@ int32_t ffa_handle_notification_set(struct 
> >> cpu_user_regs *regs)
> >> void ffa_raise_rx_buffer_full(struct domain *d)
> >> {
> >>     struct ffa_ctx *ctx = d->arch.tee;
> >> +    uint32_t prev_bitmap;
> >>
> >>     if ( !ctx )
> >>         return;
> >>
> >> -    ACCESS_ONCE(ctx->notif.buff_full_pending) = true;
> >> -    if ( !test_and_set_bool(ctx->notif.vm_pending) )
> >> +    spin_lock(&ctx->notif.notif_lock);
> >> +    prev_bitmap = ctx->notif.hyp_pending;
> >> +    ctx->notif.hyp_pending |= FFA_NOTIF_RX_BUFFER_FULL;
> >> +    spin_unlock(&ctx->notif.notif_lock);
> >> +
> >> +    if ( !(prev_bitmap & FFA_NOTIF_RX_BUFFER_FULL) )
> >
> > Do we need to check for FFA_NOTIF_RX_BUFFER_FULL? Isn't !prev_bitmap
> > more accurate, if any other bit would ever be used in the bitmap?
>
>   I would keep the bit-specific check here, if that is OK with you.
>
>  This function is about raising the RX buffer full notification,
>  so I think it is clearer to check whether that bit was newly
>  pended rather than whether the whole Hypervisor bitmap
>  was previously empty.
>
> Using !prev_bitmap would also make the condition depend on
> unrelated Hypervisor notifications being pending.
>
> Tell me if you are ok with that.

I see your point, it's fine.

Cheers,
Jens

>
> Cheers
> Bertrand
>