Re: [PATCH v2 02/10] xen/arm: ffa: Rework feature discovery

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Jens,

> On 22 Oct 2024, at 08:30, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
> 
> Hi Bertrand,
> 
> On Wed, Oct 16, 2024 at 10:32 AM Bertrand Marquis
> <bertrand.marquis@xxxxxxx> wrote:
>> 
>> Store the list of ABI we need in a list and go through the list instead
>> of having a list of conditions inside the code.
>> 
>> No functional change.
>> 
>> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
>> ---
>> Changes in v2:
>> - Store a string version of ABI needed from firmware and print the name
>>  of the ABI not supported instead of the id
>> - Restore comment with TODO which should not have been removed at this
>>  stage
>> - fix to unsigned int the index in the loop on the array
>> - use abi instead of feature in the names of the functions and variables
>>  as we are not checking features but abis
>> ---
>> xen/arch/arm/tee/ffa.c | 57 +++++++++++++++++++++++++-----------------
>> 1 file changed, 34 insertions(+), 23 deletions(-)
>> 
>> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
>> index 1cc4023135d5..dde932422ecf 100644
>> --- a/xen/arch/arm/tee/ffa.c
>> +++ b/xen/arch/arm/tee/ffa.c
>> @@ -74,6 +74,31 @@
>> /* Negotiated FF-A version to use with the SPMC, 0 if not there or supported 
>> */
>> static uint32_t __ro_after_init ffa_fw_version;
>> 
>> +struct ffa_fw_abi {
>> +    const uint32_t id;
> 
> I prefer removing the const attribute for this struct member since it
> doesn't add anything when the struct itself is const.

Works for me.
Will fix in v3.

Cheers
Bertrand

> 
> Cheers,
> Jens
> 
>> +    const char *name;
>> +};
>> +
>> +#define FW_ABI(abi) {abi,#abi}
>> +
>> +/* List of ABI we use from the firmware */
>> +static const struct ffa_fw_abi ffa_fw_abi_needed[] = {
>> +    FW_ABI(FFA_VERSION),
>> +    FW_ABI(FFA_FEATURES),
>> +    FW_ABI(FFA_NOTIFICATION_BITMAP_CREATE),
>> +    FW_ABI(FFA_NOTIFICATION_BITMAP_DESTROY),
>> +    FW_ABI(FFA_PARTITION_INFO_GET),
>> +    FW_ABI(FFA_NOTIFICATION_INFO_GET_64),
>> +    FW_ABI(FFA_NOTIFICATION_GET),
>> +    FW_ABI(FFA_RX_RELEASE),
>> +    FW_ABI(FFA_RXTX_MAP_64),
>> +    FW_ABI(FFA_RXTX_UNMAP),
>> +    FW_ABI(FFA_MEM_SHARE_32),
>> +    FW_ABI(FFA_MEM_SHARE_64),
>> +    FW_ABI(FFA_MEM_RECLAIM),
>> +    FW_ABI(FFA_MSG_SEND_DIRECT_REQ_32),
>> +    FW_ABI(FFA_MSG_SEND_DIRECT_REQ_64),
>> +};
>> 
>> /*
>>  * Our rx/tx buffers shared with the SPMC. FFA_RXTX_PAGE_COUNT is the
>> @@ -112,20 +137,9 @@ static bool ffa_get_version(uint32_t *vers)
>>     return true;
>> }
>> 
>> -static int32_t ffa_features(uint32_t id)
>> -{
>> -    return ffa_simple_call(FFA_FEATURES, id, 0, 0, 0);
>> -}
>> -
>> -static bool check_mandatory_feature(uint32_t id)
>> +static bool ffa_abi_supported(uint32_t id)
>> {
>> -    int32_t ret = ffa_features(id);
>> -
>> -    if ( ret )
>> -        printk(XENLOG_ERR "ffa: mandatory feature id %#x missing: error 
>> %d\n",
>> -               id, ret);
>> -
>> -    return !ret;
>> +    return !ffa_simple_call(FFA_FEATURES, id, 0, 0, 0);
>> }
>> 
>> static void handle_version(struct cpu_user_regs *regs)
>> @@ -540,17 +554,14 @@ static bool ffa_probe(void)
>>      * TODO: Rework the code to allow domain to use a subset of the
>>      * features supported.
>>      */
>> -    if ( !check_mandatory_feature(FFA_PARTITION_INFO_GET) ||
>> -         !check_mandatory_feature(FFA_RX_RELEASE) ||
>> -         !check_mandatory_feature(FFA_RXTX_MAP_64) ||
>> -         !check_mandatory_feature(FFA_MEM_SHARE_64) ||
>> -         !check_mandatory_feature(FFA_RXTX_UNMAP) ||
>> -         !check_mandatory_feature(FFA_MEM_SHARE_32) ||
>> -         !check_mandatory_feature(FFA_MEM_RECLAIM) ||
>> -         !check_mandatory_feature(FFA_MSG_SEND_DIRECT_REQ_32) )
>> +    for ( unsigned int i = 0; i < ARRAY_SIZE(ffa_fw_abi_needed); i++ )
>>     {
>> -        printk(XENLOG_ERR "ffa: Mandatory feature not supported by fw\n");
>> -        goto err_no_fw;
>> +        if ( !ffa_abi_supported(ffa_fw_abi_needed[i].id) )
>> +        {
>> +            printk(XENLOG_INFO "ARM FF-A Firmware does not support %s\n",
>> +                   ffa_fw_abi_needed[i].name);
>> +            goto err_no_fw;
>> +        }
>>     }
>> 
>>     ffa_fw_version = vers;
>> --
>> 2.47.0