Re: [PATCH v2 01/10] xen/arm: ffa: Rework firmware discovery

[Jens Wiklander <jens.wiklander@xxxxxxxxxx>]

Hi Bertrand,

On Wed, Oct 16, 2024 at 10:32 AM Bertrand Marquis
<bertrand.marquis@xxxxxxx> wrote:
>
> Rework firmware discovery during probe:
> - move prints into the probe
> - rename ffa_version to ffa_fw_version as the variable identifies the
>   version of the firmware and not the one we support
> - add error prints when allocation fail during probe
>
> No functional changes.
>
> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> ---
> Changes in v2:
> - Fix error message when we fail to retrieve ffa_version
> - Move back printing the firmware version before checking supported
>   features
> - Use Warning instead of Info to inform user that FF-A is not supported
>   in firmware.
> ---
>  xen/arch/arm/tee/ffa.c | 44 ++++++++++++++++++++++++++----------------
>  1 file changed, 27 insertions(+), 17 deletions(-)
>
> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> index 022089278e1c..1cc4023135d5 100644
> --- a/xen/arch/arm/tee/ffa.c
> +++ b/xen/arch/arm/tee/ffa.c
> @@ -71,8 +71,8 @@
>
>  #include "ffa_private.h"
>
> -/* Negotiated FF-A version to use with the SPMC */
> -static uint32_t __ro_after_init ffa_version;
> +/* Negotiated FF-A version to use with the SPMC, 0 if not there or supported 
> */
> +static uint32_t __ro_after_init ffa_fw_version;
>
>
>  /*
> @@ -105,10 +105,7 @@ static bool ffa_get_version(uint32_t *vers)
>
>      arm_smccc_1_2_smc(&arg, &resp);
>      if ( resp.a0 == FFA_RET_NOT_SUPPORTED )
> -    {
> -        gprintk(XENLOG_ERR, "ffa: FFA_VERSION returned not supported\n");
>          return false;
> -    }
>
>      *vers = resp.a0;
>
> @@ -372,7 +369,7 @@ static int ffa_domain_init(struct domain *d)
>      struct ffa_ctx *ctx;
>      int ret;
>
> -    if ( !ffa_version )
> +    if ( !ffa_fw_version )
>          return -ENODEV;
>       /*
>        * We can't use that last possible domain ID or ffa_get_vm_id() would
> @@ -505,6 +502,9 @@ static bool ffa_probe(void)
>       */
>      BUILD_BUG_ON(PAGE_SIZE != FFA_PAGE_SIZE);
>
> +    printk(XENLOG_INFO "ARM FF-A Mediator version %u.%u\n",
> +           FFA_MY_VERSION_MAJOR, FFA_MY_VERSION_MINOR);
> +
>      /*
>       * psci_init_smccc() updates this value with what's reported by EL-3
>       * or secure world.
> @@ -514,22 +514,24 @@ static bool ffa_probe(void)
>          printk(XENLOG_ERR
>                 "ffa: unsupported SMCCC version %#x (need at least %#x)\n",
>                 smccc_ver, ARM_SMCCC_VERSION_1_2);
> -        return false;
> +        goto err_no_fw;
>      }
>
>      if ( !ffa_get_version(&vers) )
> -        return false;
> +    {
> +        gprintk(XENLOG_ERR, "Cannot retrieve the FFA version\n");
> +        goto err_no_fw;
> +    }
>
>      if ( vers < FFA_MIN_SPMC_VERSION || vers > FFA_MY_VERSION )
>      {
>          printk(XENLOG_ERR "ffa: Incompatible version %#x found\n", vers);
> -        return false;
> +        goto err_no_fw;
>      }
>
> -    major_vers = (vers >> FFA_VERSION_MAJOR_SHIFT) & FFA_VERSION_MAJOR_MASK;
> +    major_vers = (vers >> FFA_VERSION_MAJOR_SHIFT)
> +                 & FFA_VERSION_MAJOR_MASK;

Spurious change?

>      minor_vers = vers & FFA_VERSION_MINOR_MASK;
> -    printk(XENLOG_INFO "ARM FF-A Mediator version %u.%u\n",
> -           FFA_MY_VERSION_MAJOR, FFA_MY_VERSION_MINOR);

It's not a big deal, but isn't it useful to know which version we're
at? If it's too much with a separate line, how about adding "(our
version %u.u%)" at the end of the line below?

>      printk(XENLOG_INFO "ARM FF-A Firmware version %u.%u\n",
>             major_vers, minor_vers);
>
> @@ -546,12 +548,18 @@ static bool ffa_probe(void)
>           !check_mandatory_feature(FFA_MEM_SHARE_32) ||
>           !check_mandatory_feature(FFA_MEM_RECLAIM) ||
>           !check_mandatory_feature(FFA_MSG_SEND_DIRECT_REQ_32) )
> -        return false;
> +    {
> +        printk(XENLOG_ERR "ffa: Mandatory feature not supported by fw\n");
> +        goto err_no_fw;
> +    }
>
> -    if ( !ffa_rxtx_init() )
> -        return false;
> +    ffa_fw_version = vers;
>
> -    ffa_version = vers;
> +    if ( !ffa_rxtx_init() )
> +    {
> +        printk(XENLOG_ERR "ffa: Error during RXTX buffer init\n");

With this added, wouldn't it make sense to remove the error print in
ffa_rxtx_init()?

Cheers,
Jens

> +        goto err_no_fw;
> +    }
>
>      if ( !ffa_partinfo_init() )
>          goto err_rxtx_destroy;
> @@ -564,7 +572,9 @@ static bool ffa_probe(void)
>
>  err_rxtx_destroy:
>      ffa_rxtx_destroy();
> -    ffa_version = 0;
> +err_no_fw:
> +    ffa_fw_version = 0;
> +    printk(XENLOG_WARNING "ARM FF-A No firmware support\n");
>
>      return false;
>  }
> --
> 2.47.0
>