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

[Jens Wiklander <jens.wiklander@xxxxxxxxxx>]

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.

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
>