| Message ID | 457020b684e87ee9ad9488a4ec0d17ade90fdea1.1729066788.git.bertrand.marquis@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xen/arm: ffa: Improvements and fixes | expand |
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? > 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 >
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 > >
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
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; }
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(-)