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

[Julien Grall <julien@xxxxxxx>]

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.
>   
>   /*
>    * 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?

> -    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).
> +    {
> +        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,

--
Julien Grall