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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Julien,

> On 22 Sep 2024, at 11:00, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Bertrand,
> 
> On 19/09/2024 14:19, Bertrand Marquis 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>
>> ---
>>  xen/arch/arm/tee/ffa.c | 52 +++++++++++++++++++++++++-----------------
>>  1 file changed, 31 insertions(+), 21 deletions(-)
>> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
>> index 022089278e1c..7c84aa6aa43d 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,25 +514,21 @@ 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, "ffa: FFA_VERSION returned not supported\n");
> 
> This error message relies on the implementation of ffa_get_version(). It made 
> sense in the previous placement, but here, it seems a little bit odd. So if 
> you want to move the error message, then I think it should be reworded to be 
> more generic.
> 
> Maybe: "Cannot retrieve the FFA version".

Ack

> 
>> +        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;
>> -    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);
> 
> I kind of understand why we are moving the Medatior version early but...
> 
>> -    printk(XENLOG_INFO "ARM FF-A Firmware version %u.%u\n",
>> -           major_vers, minor_vers);
> 
> ... I am not sure why we would move this print later. Wouldn't this be useful 
> to know if there is a missing feature?

True I will move it back up.

>> -
>>      /*
>>       * 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
>> @@ -546,12 +542,24 @@ 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;
>> +    major_vers = (vers >> FFA_VERSION_MAJOR_SHIFT)
>> +                 & FFA_VERSION_MAJOR_MASK;
>> +    minor_vers = vers & FFA_VERSION_MINOR_MASK;
>> +    printk(XENLOG_INFO "ARM FF-A Firmware version %u.%u\n",
>> +           major_vers, minor_vers);
>> +
>> +    ffa_fw_version = vers;
>>  -    ffa_version = vers;
>> +    if ( !ffa_rxtx_init() )
>> +    {
>> +        printk(XENLOG_ERR "ffa: Error during RXTX buffer init\n");
>> +        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_INFO "ARM FF-A No firmware support\n");
> 
> I am guessing if we are trying to probe FFA, then most likely the user 
> expected to use it. So shouldn't this be a XENLOG_WARN?

Ack.

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall