Hi Bertrand,
On Thu, Oct 24, 2024 at 11:50 AM Bertrand Marquis
<bertrand.marq...@arm.com> wrote:
>
> Hi Jens,
>
> > On 24 Oct 2024, at 09:41, Jens Wiklander <jens.wiklan...@linaro.org> wrote:
> >
> > Hi Bertrand,
> >
> > On Wed, Oct 16, 2024 at 10:32 AM Bertrand Marquis
> > <bertrand.marq...@arm.com> 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.marq...@arm.com>
> >> ---
> >> 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
>
>
