| Message ID | b23ad93b876267fb48a5a398e394e60fdf52d33d.1729066788.git.bertrand.marquis@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xen/arm: ffa: Improvements and fixes | expand |
Hi Bertrand, On Wed, Oct 16, 2024 at 10:32 AM Bertrand Marquis <bertrand.marquis@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.marquis@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. 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 >
Hi Jens, > On 24 Oct 2024, at 09:41, Jens Wiklander <jens.wiklander@linaro.org> wrote: > > Hi Bertrand, > > On Wed, Oct 16, 2024 at 10:32 AM Bertrand Marquis > <bertrand.marquis@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.marquis@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 ? 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
Hi Bertrand, On Thu, Oct 24, 2024 at 11:50 AM Bertrand Marquis <Bertrand.Marquis@arm.com> wrote: > > Hi Jens, > > > On 24 Oct 2024, at 09:41, Jens Wiklander <jens.wiklander@linaro.org> wrote: > > > > Hi Bertrand, > > > > On Wed, Oct 16, 2024 at 10:32 AM Bertrand Marquis > > <bertrand.marquis@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.marquis@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 > >
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)); - 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.
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@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(-)