Hi Jens,
> On 24 Oct 2024, at 09:41, Jens Wiklander <[email protected]> wrote:
>
> Hi Bertrand,
>
> On Wed, Oct 16, 2024 at 10:32 AM Bertrand Marquis
> <[email protected]> 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 <[email protected]>
>> ---
>> 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
