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@arm.com>
---
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;
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);
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");
+ 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.0Hi Bertrand,
On Wed, Oct 16, 2024 at 10:32 AM Bertrand Marquis
<bertrand.marquis@arm.com> 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@arm.com>
> ---
> 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
>
Hi Jens,
> On 21 Oct 2024, at 19:03, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> Hi Bertrand,
>
> On Wed, Oct 16, 2024 at 10:32 AM Bertrand Marquis
> <bertrand.marquis@arm.com> 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@arm.com>
>> ---
>> 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?
Yes, I will fix that in next version.
>
>> 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?
This was moved up.
>
>> 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()?
Definitely, I missed that.
Will fix in v3.
Cheers
Bertrand
>
> 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
On Mon, Oct 21, 2024 at 7:03 PM Jens Wiklander
<jens.wiklander@linaro.org> wrote:
>
> Hi Bertrand,
>
> On Wed, Oct 16, 2024 at 10:32 AM Bertrand Marquis
> <bertrand.marquis@arm.com> 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@arm.com>
> > ---
> > 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?
Please ignore this comment, I missed this was moved up a few lines.
Thanks,
Jens
>
> > 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
> >
© 2016 - 2024 Red Hat, Inc.