[v2] xen/arm: ffa: Harden notifications and enable VM-to-VM delivery

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

Posted by Bertrand Marquis 4 months, 2 weeks ago

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@arm.com>
---
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(-)

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

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

Posted by Jens Wiklander 4 months ago

Hi Bertrand,

On Wed, Apr 29, 2026 at 7:44 AM Bertrand Marquis
<bertrand.marquis@arm.com> 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@arm.com>
> ---
> 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@linaro.org>

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
>

[PATCH v2 1/6] xen/arm: ffa: Fix NPI injection when vcpu0 is offline
[PATCH v2 2/6] xen/arm: ffa: Track hypervisor notifications in a bitmap
[PATCH v2 3/6] xen/arm: ffa: Tighten notification parameter validation
[PATCH v2 4/6] xen/arm: ffa: Preserve secure notification state when polling SPMC
[PATCH v2 5/6] xen/arm: ffa: Track VM notification bindings locally
[PATCH v2 6/6] xen/arm: ffa: Deliver VM-to-VM notifications locally