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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Julien,

> On 24 Sep 2024, at 19:31, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Bertrand,
> 
> On 24/09/2024 09:16, Bertrand Marquis wrote:
>>> 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.
> 
> Thanks for the clarification! With that in mind...
> 
>> 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" ?
> 
> ... I was more thinking about printing which ABI is missing. This is more 
> helpful to the user than knowning which features will be missing.
> 
> This has also the advantage that we can use macro to generate the names.

Ok then i will build a table of strings of the ABI names and use that to say 
what ABI is not supported when there is one.

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall
>