| Message ID | 8cc7abbd82b43aa595ddc2dce3495663dfc413e0.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: > > 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@arm.com> > --- > 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 >
Hi Jens, > On 22 Oct 2024, at 08:30, 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: >> >> 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@arm.com> >> --- >> 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. Works for me. Will fix in v3. Cheers Bertrand > > 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
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; + 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;
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@arm.com> --- 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(-)