Re: [PATCH v2 09/10] xen/arm: ffa: Remove per VM notif_enabled

[Jens Wiklander <jens.wiklander@xxxxxxxxxx>]

Hi Bertrand,

On Wed, Oct 16, 2024 at 10:32 AM Bertrand Marquis
<bertrand.marquis@xxxxxxx> wrote:
>
> Remove the per VM flag to store if notifications are enabled or not as
> the only case where they are not, if notifications are enabled globally,
> will make the VM creation fail.
> Also use the opportunity to always give the notifications interrupts IDs
> to VM. If the firmware does not support notifications, there won't be
> any generated and setting one will give back a NOT_SUPPORTED.
>
> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> ---
> Changes in v2:
> - rebase
> ---
>  xen/arch/arm/tee/ffa.c         | 17 +++--------------
>  xen/arch/arm/tee/ffa_notif.c   | 10 +---------
>  xen/arch/arm/tee/ffa_private.h |  2 --
>  3 files changed, 4 insertions(+), 25 deletions(-)
>
> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> index 72826b49d2aa..3a9525aa4598 100644
> --- a/xen/arch/arm/tee/ffa.c
> +++ b/xen/arch/arm/tee/ffa.c
> @@ -169,8 +169,6 @@ static void handle_version(struct cpu_user_regs *regs)
>
>  static void handle_features(struct cpu_user_regs *regs)
>  {
> -    struct domain *d = current->domain;
> -    struct ffa_ctx *ctx = d->arch.tee;
>      uint32_t a1 = get_user_reg(regs, 1);
>      unsigned int n;
>
> @@ -218,16 +216,10 @@ static void handle_features(struct cpu_user_regs *regs)
>          ffa_set_regs_success(regs, 0, 0);
>          break;
>      case FFA_FEATURE_NOTIF_PEND_INTR:
> -        if ( ctx->notif.enabled )
> -            ffa_set_regs_success(regs, GUEST_FFA_NOTIF_PEND_INTR_ID, 0);
> -        else
> -            ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
> +        ffa_set_regs_success(regs, GUEST_FFA_NOTIF_PEND_INTR_ID, 0);
>          break;
>      case FFA_FEATURE_SCHEDULE_RECV_INTR:
> -        if ( ctx->notif.enabled )
> -            ffa_set_regs_success(regs, GUEST_FFA_SCHEDULE_RECV_INTR_ID, 0);
> -        else
> -            ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
> +        ffa_set_regs_success(regs, GUEST_FFA_SCHEDULE_RECV_INTR_ID, 0);
>          break;
>
>      case FFA_NOTIFICATION_BIND:
> @@ -236,10 +228,7 @@ 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:
> -        if ( ctx->notif.enabled )
> -            ffa_set_regs_success(regs, 0, 0);
> -        else
> -            ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
> +        ffa_set_regs_success(regs, 0, 0);
>          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 4b3e46318f4b..3c6418e62e2b 100644
> --- a/xen/arch/arm/tee/ffa_notif.c
> +++ b/xen/arch/arm/tee/ffa_notif.c
> @@ -405,7 +405,6 @@ void ffa_notif_init(void)
>
>  int ffa_notif_domain_init(struct domain *d)
>  {
> -    struct ffa_ctx *ctx = d->arch.tee;
>      int32_t res;
>
>      if ( !notif_enabled )
> @@ -415,18 +414,11 @@ int ffa_notif_domain_init(struct domain *d)
>      if ( res )
>          return -ENOMEM;
>
> -    ctx->notif.enabled = true;
> -
>      return 0;
>  }
>
>  void ffa_notif_domain_destroy(struct domain *d)
>  {
> -    struct ffa_ctx *ctx = d->arch.tee;
> -
> -    if ( ctx->notif.enabled )
> -    {
> +    if ( notif_enabled )
>          ffa_notification_bitmap_destroy(ffa_get_vm_id(d));

This call may now be done even if there hasn't been a successful call
to ffa_notification_bitmap_create().
A comment mentioning this and that it's harmless (if we can be sure it
is) would be nice.

Cheers,
Jens

> -        ctx->notif.enabled = false;
> -    }
>  }
> diff --git a/xen/arch/arm/tee/ffa_private.h b/xen/arch/arm/tee/ffa_private.h
> index 02162e0ee4c7..973ee55be09b 100644
> --- a/xen/arch/arm/tee/ffa_private.h
> +++ b/xen/arch/arm/tee/ffa_private.h
> @@ -281,8 +281,6 @@ struct ffa_mem_region {
>  };
>
>  struct ffa_ctx_notif {
> -    bool enabled;
> -
>      /*
>       * True if domain is reported by FFA_NOTIFICATION_INFO_GET to have
>       * pending global notifications.
> --
> 2.47.0
>