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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

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 ?

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