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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Julien,

Thanks a lot for the review.

> On 22 Sep 2024, at 11:07, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi,
> 
> On 19/09/2024 14:19, Bertrand Marquis 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>
>> ---
>>  xen/arch/arm/tee/ffa.c | 61 +++++++++++++++++++++---------------------
>>  1 file changed, 30 insertions(+), 31 deletions(-)
>> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
>> index 7c84aa6aa43d..7ff2529b2055 100644
>> --- a/xen/arch/arm/tee/ffa.c
>> +++ b/xen/arch/arm/tee/ffa.c
>> @@ -74,6 +74,24 @@
>>  /* Negotiated FF-A version to use with the SPMC, 0 if not there or 
>> supported */
>>  static uint32_t __ro_after_init ffa_fw_version;
>>  +/* List of ABI we use from the firmware */
>> +static const uint32_t ffa_fw_feat_needed[] = {
>> +    FFA_VERSION,
>> +    FFA_FEATURES,
>> +    FFA_NOTIFICATION_BITMAP_CREATE,
>> +    FFA_NOTIFICATION_BITMAP_DESTROY,
>> +    FFA_PARTITION_INFO_GET,
>> +    FFA_NOTIFICATION_INFO_GET_64,
>> +    FFA_NOTIFICATION_GET,
>> +    FFA_RX_RELEASE,
>> +    FFA_RXTX_MAP_64,
>> +    FFA_RXTX_UNMAP,
>> +    FFA_MEM_SHARE_32,
>> +    FFA_MEM_SHARE_64,
>> +    FFA_MEM_RECLAIM,
>> +    FFA_MSG_SEND_DIRECT_REQ_32,
>> +    FFA_MSG_SEND_DIRECT_REQ_64,
>> +};
> 
> NIT: As we are creating an array, could be take the opportunity to provide a 
> name for each feature (we could use macro for that)? This would make easier 
> for the user to know which feature is missing.

In fact those are not "features" per say but ABI we need to use with the 
firmware so maybe i should first rename the variable to say abi instead of feat.

Now we could create some features out of those as in practice several ABIs are 
needed to be able to use one feature (for example notifications support require 
the INFO_GET and the GET).

In your mind you wanted to have something like:
"Version", FFA_VERSION
"Direct Messages", FFA_MSG_SEND_DIRECT_REQ_32,
"Direct Messages", FFA_MSG_SEND_DIRECT_REQ_64 

So that we could print a more meaningfull name or would you be ok with just 
printing "FFA_MSG_SEND_DIRECT_REQ_32" ?

> 
>>    /*
>>   * Our rx/tx buffers shared with the SPMC. FFA_RXTX_PAGE_COUNT is the
>> @@ -112,20 +130,9 @@ static bool ffa_get_version(uint32_t *vers)
>>      return true;
>>  }
>>  -static int32_t ffa_features(uint32_t id)
>> +static bool ffa_feature_supported(uint32_t id)
>>  {
>> -    return ffa_simple_call(FFA_FEATURES, id, 0, 0, 0);
>> -}
>> -
>> -static bool check_mandatory_feature(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)
>> @@ -529,24 +536,6 @@ static bool ffa_probe(void)
>>          goto err_no_fw;
>>      }
>>  -    /*
>> -     * At the moment domains must support the same features used by Xen.
>> -     * TODO: Rework the code to allow domain to use a subset of the
>> -     * features supported.
>> -     */
> 
> You removed this TODO but I don't think it was addressed. Can you clarify?

Right this should only be removed with the next patch when we add fine
granular call support. I will readd it here and remove it in the fine granular 
patch.

> 
>> -    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) )
>> -    {
>> -        printk(XENLOG_ERR "ffa: Mandatory feature not supported by fw\n");
>> -        goto err_no_fw;
>> -    }
>> -
>>      major_vers = (vers >> FFA_VERSION_MAJOR_SHIFT)
>>                   & FFA_VERSION_MAJOR_MASK;
>>      minor_vers = vers & FFA_VERSION_MINOR_MASK;
>> @@ -555,6 +544,16 @@ static bool ffa_probe(void)
>>        ffa_fw_version = vers;
>>  +    for ( int i = 0; i < ARRAY_SIZE(ffa_fw_feat_needed); i++ )
> 
> This is an index, so please use unsigned int (even though it should 
> technically be size_t).

Ack

> 
>> +    {
>> +        if ( !ffa_feature_supported(ffa_fw_feat_needed[i]) )
>> +        {
>> +            printk(XENLOG_INFO "ARM FF-A Firmware does not support 
>> 0x%08x\n",
>> +                   ffa_fw_feat_needed[i]);
>> +            goto err_no_fw;
>> +        }
>> +    }
>> +
>>      if ( !ffa_rxtx_init() )
>>      {
>>          printk(XENLOG_ERR "ffa: Error during RXTX buffer init\n");
> 
> Cheers,

Cheers
Bertrand

> 
> -- 
> Julien Grall