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

[Jens Wiklander <jens.wiklander@xxxxxxxxxx>]

Hi Bertrand,

On Thu, Oct 24, 2024 at 11:50 AM Bertrand Marquis
<Bertrand.Marquis@xxxxxxx> wrote:
>
> Hi Jens,
>
> > On 24 Oct 2024, at 09:41, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
> >
> > 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.
> >
>
> You mean in the case where it failed during domain_init ?
>
> I can add the following comment:
>  Call bitmap_destroy even if bitmap create failed as the SPMC should return 
> an error that we will ignore
>
> Would that be ok ?

Yes, that's fine.

Cheers,
Jens

>
> Cheers
> Bertrand
>
>
> > 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
>
>