Re: [PATCH v2 4/6] xen/arm: ffa: Preserve secure notification state when polling SPMC

[Jens Wiklander <jens.wiklander@xxxxxxxxxx>]

Hi Bertrand,

On Wed, Apr 29, 2026 at 7:44 AM Bertrand Marquis
<bertrand.marquis@xxxxxxx> wrote:
>
> Secure pending state is latched when the SPMC raises the schedule
> receiver interrupt, but Xen currently clears that latch too aggressively.
> Guest FFA_NOTIFICATION_INFO_GET consumes secure_pending even though it
> only reports pending state, and secure FFA_NOTIFICATION_GET only clears
> the latch when both SP and SPM bitmaps are requested together. This can
> drop a pending indication before the receiver retrieves secure
> notifications, or keep INFO_GET reporting stale secure pending state
> after a successful GET.
>
> Keep secure_pending as a latched indication until secure notifications
> are actually retrieved. Guest FFA_NOTIFICATION_INFO_GET now reports the
> latched state without clearing it, while a successful secure
> FFA_NOTIFICATION_GET clears the latch regardless of which secure bitmap
> flags were requested. Also protect secure_pending with notif_lock,
> serialize SPMC INFO_GET polling behind notif_info_lock, and preserve the
> caller-visible INFO_GET success width.
>
> Functional impact: guest INFO_GET preserves the secure pending
> indication until secure notifications are retrieved, and successful
> secure GET clears the guest-visible pending latch.
>
> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> ---
> Changes since v1:
> - drop the defensive fw_notif_enabled guard in notif_sri_action()
> ---
>  xen/arch/arm/tee/ffa_notif.c | 51 ++++++++++++++++++++++--------------
>  1 file changed, 32 insertions(+), 19 deletions(-)

Looks good.
Reviewed-by: Jens Wiklander <jens.wiklander@xxxxxxxxxx>

Cheers,
Jens

>
> diff --git a/xen/arch/arm/tee/ffa_notif.c b/xen/arch/arm/tee/ffa_notif.c
> index 1260f98a77e9..e1cd852d1c53 100644
> --- a/xen/arch/arm/tee/ffa_notif.c
> +++ b/xen/arch/arm/tee/ffa_notif.c
> @@ -18,6 +18,7 @@
>
>  static bool __ro_after_init fw_notif_enabled;
>  static unsigned int __ro_after_init notif_sri_irq;
> +static DEFINE_SPINLOCK(notif_info_lock);
>
>  static bool inject_notif_pending(struct domain *d)
>  {
> @@ -111,6 +112,7 @@ void ffa_handle_notification_info_get(struct 
> cpu_user_regs *regs)
>  {
>      struct domain *d = current->domain;
>      struct ffa_ctx *ctx = d->arch.tee;
> +    uint32_t fid = get_user_reg(regs, 0);
>      bool notif_pending;
>
>      if ( !IS_ENABLED(CONFIG_FFA_VM_TO_VM) && !fw_notif_enabled )
> @@ -119,7 +121,10 @@ void ffa_handle_notification_info_get(struct 
> cpu_user_regs *regs)
>          return;
>      }
>
> -    notif_pending = test_and_clear_bool(ctx->notif.secure_pending);
> +    spin_lock(&ctx->notif.notif_lock);
> +    notif_pending = ctx->notif.secure_pending;
> +    spin_unlock(&ctx->notif.notif_lock);
> +
>      if ( IS_ENABLED(CONFIG_FFA_VM_TO_VM) )
>      {
>          notif_pending |= test_and_clear_bool(ctx->notif.vm_pending);
> @@ -133,7 +138,9 @@ void ffa_handle_notification_info_get(struct 
> cpu_user_regs *regs)
>      if ( notif_pending )
>      {
>          /* A pending global notification for the guest */
> -        ffa_set_regs(regs, FFA_SUCCESS_64, 0,
> +        ffa_set_regs(regs,
> +                     smccc_is_conv_64(fid) ? FFA_SUCCESS_64 : FFA_SUCCESS_32,
> +                     0,
>                       1U << FFA_NOTIF_INFO_GET_ID_COUNT_SHIFT, 
> ffa_get_vm_id(d),
>                       0, 0, 0, 0);
>      }
> @@ -156,6 +163,8 @@ void ffa_handle_notification_get(struct cpu_user_regs 
> *regs)
>      uint32_t w5 = 0;
>      uint32_t w6 = 0;
>      uint32_t w7 = 0;
> +    uint32_t secure_flags = flags & ( FFA_NOTIF_FLAG_BITMAP_SP |
> +                                      FFA_NOTIF_FLAG_BITMAP_SPM );
>
>      if ( !IS_ENABLED(CONFIG_FFA_VM_TO_VM) && !fw_notif_enabled )
>      {
> @@ -175,27 +184,16 @@ void ffa_handle_notification_get(struct cpu_user_regs 
> *regs)
>          return;
>      }
>
> -    if ( fw_notif_enabled && (flags & ( FFA_NOTIF_FLAG_BITMAP_SP |
> -                                        FFA_NOTIF_FLAG_BITMAP_SPM )) )
> +    if ( fw_notif_enabled && secure_flags )
>      {
>          struct arm_smccc_1_2_regs arg = {
>              .a0 = FFA_NOTIFICATION_GET,
>              .a1 = recv,
> -            .a2 = flags & ( FFA_NOTIF_FLAG_BITMAP_SP |
> -                            FFA_NOTIF_FLAG_BITMAP_SPM ),
> +            .a2 = secure_flags,
>          };
>          struct arm_smccc_1_2_regs resp;
>          int32_t e;
>
> -        /*
> -         * Clear secure pending if both FFA_NOTIF_FLAG_BITMAP_SP and
> -         * FFA_NOTIF_FLAG_BITMAP_SPM are set since secure world can't have
> -         * any more pending notifications.
> -         */
> -        if ( ( flags  & FFA_NOTIF_FLAG_BITMAP_SP ) &&
> -             ( flags & FFA_NOTIF_FLAG_BITMAP_SPM ) )
> -            ACCESS_ONCE(ctx->notif.secure_pending) = false;
> -
>          arm_smccc_1_2_smc(&arg, &resp);
>          e = ffa_get_ret_code(&resp);
>          if ( e )
> @@ -212,6 +210,10 @@ void ffa_handle_notification_get(struct cpu_user_regs 
> *regs)
>
>          if ( flags & FFA_NOTIF_FLAG_BITMAP_SPM )
>              w6 = resp.a6;
> +
> +        spin_lock(&ctx->notif.notif_lock);
> +        ctx->notif.secure_pending = false;
> +        spin_unlock(&ctx->notif.notif_lock);
>      }
>
>      if ( IS_ENABLED(CONFIG_FFA_VM_TO_VM) )
> @@ -356,7 +358,10 @@ static void notif_vm_pend_intr(uint16_t vm_id)
>       * guarantees that the data structure isn't freed while we're accessing
>       * it.
>       */
> -    ACCESS_ONCE(ctx->notif.secure_pending) = true;
> +    spin_lock(&ctx->notif.notif_lock);
> +    ctx->notif.secure_pending = true;
> +    spin_unlock(&ctx->notif.notif_lock);
> +
>      inject_notif_pending(d);
>
>  out_unlock:
> @@ -375,11 +380,15 @@ static void notif_sri_action(void *unused)
>      unsigned int n;
>      int32_t res;
>
> -    do {
> +    spin_lock(&notif_info_lock);
> +
> +    do
> +    {
>          arm_smccc_1_2_smc(&arg, &resp);
>          res = ffa_get_ret_code(&resp);
>          if ( res )
>          {
> +            spin_unlock(&notif_info_lock);
>              if ( res != FFA_RET_NO_DATA && printk_ratelimit() )
>                  printk(XENLOG_WARNING
>                         "ffa: notification info get failed: error %d\n", res);
> @@ -393,7 +402,7 @@ static void notif_sri_action(void *unused)
>          id_pos = 0;
>          for ( n = 0; n < list_count; n++ )
>          {
> -            unsigned int count = ((ids_count >> 2 * n) & 0x3) + 1;
> +            unsigned int count = ((ids_count >> (2 * n)) & 0x3) + 1;
>              uint16_t vm_id = get_id_from_resp(&resp, id_pos);
>
>              notif_vm_pend_intr(vm_id);
> @@ -401,7 +410,9 @@ static void notif_sri_action(void *unused)
>              id_pos += count;
>          }
>
> -    } while (resp.a2 & FFA_NOTIF_INFO_GET_MORE_FLAG);
> +    } while ( resp.a2 & FFA_NOTIF_INFO_GET_MORE_FLAG );
> +
> +    spin_unlock(&notif_info_lock);
>  }
>
>  static DECLARE_TASKLET(notif_sri_tasklet, notif_sri_action, NULL);
> @@ -489,6 +500,7 @@ int ffa_notif_domain_init(struct domain *d)
>
>      spin_lock_init(&ctx->notif.notif_lock);
>      ctx->notif.notif_irq_raised = false;
> +    ctx->notif.secure_pending = false;
>      ctx->notif.hyp_pending = 0;
>
>      if ( fw_notif_enabled )
> @@ -507,6 +519,7 @@ void ffa_notif_domain_destroy(struct domain *d)
>
>      spin_lock(&ctx->notif.notif_lock);
>      ctx->notif.notif_irq_raised = false;
> +    ctx->notif.secure_pending = false;
>      ctx->notif.hyp_pending = 0;
>      spin_unlock(&ctx->notif.notif_lock);
>
> --
> 2.53.0
>