| Message ID | 6c841c341b7dc9e06eb1c02555e30b29bd400d20.1726676338.git.bertrand.marquis@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: ffa: Improvements and fixes | expand |
Hi, On 19/09/2024 14:19, Bertrand Marquis 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> > --- > xen/arch/arm/tee/ffa.c | 61 +++++++++++++++++++++--------------------- > 1 file changed, 30 insertions(+), 31 deletions(-) > > diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c > index 7c84aa6aa43d..7ff2529b2055 100644 > --- a/xen/arch/arm/tee/ffa.c > +++ b/xen/arch/arm/tee/ffa.c > @@ -74,6 +74,24 @@ > /* Negotiated FF-A version to use with the SPMC, 0 if not there or supported */ > static uint32_t __ro_after_init ffa_fw_version; > > +/* List of ABI we use from the firmware */ > +static const uint32_t ffa_fw_feat_needed[] = { > + FFA_VERSION, > + FFA_FEATURES, > + FFA_NOTIFICATION_BITMAP_CREATE, > + FFA_NOTIFICATION_BITMAP_DESTROY, > + FFA_PARTITION_INFO_GET, > + FFA_NOTIFICATION_INFO_GET_64, > + FFA_NOTIFICATION_GET, > + FFA_RX_RELEASE, > + FFA_RXTX_MAP_64, > + FFA_RXTX_UNMAP, > + FFA_MEM_SHARE_32, > + FFA_MEM_SHARE_64, > + FFA_MEM_RECLAIM, > + FFA_MSG_SEND_DIRECT_REQ_32, > + FFA_MSG_SEND_DIRECT_REQ_64, > +}; NIT: As we are creating an array, could be take the opportunity to provide a name for each feature (we could use macro for that)? This would make easier for the user to know which feature is missing. > > /* > * Our rx/tx buffers shared with the SPMC. FFA_RXTX_PAGE_COUNT is the > @@ -112,20 +130,9 @@ static bool ffa_get_version(uint32_t *vers) > return true; > } > > -static int32_t ffa_features(uint32_t id) > +static bool ffa_feature_supported(uint32_t id) > { > - return ffa_simple_call(FFA_FEATURES, id, 0, 0, 0); > -} > - > -static bool check_mandatory_feature(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) > @@ -529,24 +536,6 @@ static bool ffa_probe(void) > goto err_no_fw; > } > > - /* > - * 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 > - * features supported. > - */ You removed this TODO but I don't think it was addressed. Can you clarify? > - 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) ) > - { > - printk(XENLOG_ERR "ffa: Mandatory feature not supported by fw\n"); > - goto err_no_fw; > - } > - > major_vers = (vers >> FFA_VERSION_MAJOR_SHIFT) > & FFA_VERSION_MAJOR_MASK; > minor_vers = vers & FFA_VERSION_MINOR_MASK; > @@ -555,6 +544,16 @@ static bool ffa_probe(void) > > ffa_fw_version = vers; > > + for ( int i = 0; i < ARRAY_SIZE(ffa_fw_feat_needed); i++ ) This is an index, so please use unsigned int (even though it should technically be size_t). > + { > + if ( !ffa_feature_supported(ffa_fw_feat_needed[i]) ) > + { > + printk(XENLOG_INFO "ARM FF-A Firmware does not support 0x%08x\n", > + ffa_fw_feat_needed[i]); > + goto err_no_fw; > + } > + } > + > if ( !ffa_rxtx_init() ) > { > printk(XENLOG_ERR "ffa: Error during RXTX buffer init\n"); Cheers,
Hi Julien, Thanks a lot for the review. > On 22 Sep 2024, at 11:07, Julien Grall <julien@xen.org> wrote: > > Hi, > > On 19/09/2024 14:19, Bertrand Marquis 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> >> --- >> xen/arch/arm/tee/ffa.c | 61 +++++++++++++++++++++--------------------- >> 1 file changed, 30 insertions(+), 31 deletions(-) >> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c >> index 7c84aa6aa43d..7ff2529b2055 100644 >> --- a/xen/arch/arm/tee/ffa.c >> +++ b/xen/arch/arm/tee/ffa.c >> @@ -74,6 +74,24 @@ >> /* Negotiated FF-A version to use with the SPMC, 0 if not there or supported */ >> static uint32_t __ro_after_init ffa_fw_version; >> +/* List of ABI we use from the firmware */ >> +static const uint32_t ffa_fw_feat_needed[] = { >> + FFA_VERSION, >> + FFA_FEATURES, >> + FFA_NOTIFICATION_BITMAP_CREATE, >> + FFA_NOTIFICATION_BITMAP_DESTROY, >> + FFA_PARTITION_INFO_GET, >> + FFA_NOTIFICATION_INFO_GET_64, >> + FFA_NOTIFICATION_GET, >> + FFA_RX_RELEASE, >> + FFA_RXTX_MAP_64, >> + FFA_RXTX_UNMAP, >> + FFA_MEM_SHARE_32, >> + FFA_MEM_SHARE_64, >> + FFA_MEM_RECLAIM, >> + FFA_MSG_SEND_DIRECT_REQ_32, >> + FFA_MSG_SEND_DIRECT_REQ_64, >> +}; > > NIT: As we are creating an array, could be take the opportunity to provide a name for each feature (we could use macro for that)? This would make easier for the user to know which feature is missing. In fact those are not "features" per say but ABI we need to use with the firmware so maybe i should first rename the variable to say abi instead of feat. Now we could create some features out of those as in practice several ABIs are needed to be able to use one feature (for example notifications support require the INFO_GET and the GET). In your mind you wanted to have something like: "Version", FFA_VERSION "Direct Messages", FFA_MSG_SEND_DIRECT_REQ_32, "Direct Messages", FFA_MSG_SEND_DIRECT_REQ_64 So that we could print a more meaningfull name or would you be ok with just printing "FFA_MSG_SEND_DIRECT_REQ_32" ? > >> /* >> * Our rx/tx buffers shared with the SPMC. FFA_RXTX_PAGE_COUNT is the >> @@ -112,20 +130,9 @@ static bool ffa_get_version(uint32_t *vers) >> return true; >> } >> -static int32_t ffa_features(uint32_t id) >> +static bool ffa_feature_supported(uint32_t id) >> { >> - return ffa_simple_call(FFA_FEATURES, id, 0, 0, 0); >> -} >> - >> -static bool check_mandatory_feature(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) >> @@ -529,24 +536,6 @@ static bool ffa_probe(void) >> goto err_no_fw; >> } >> - /* >> - * 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 >> - * features supported. >> - */ > > You removed this TODO but I don't think it was addressed. Can you clarify? Right this should only be removed with the next patch when we add fine granular call support. I will readd it here and remove it in the fine granular patch. > >> - 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) ) >> - { >> - printk(XENLOG_ERR "ffa: Mandatory feature not supported by fw\n"); >> - goto err_no_fw; >> - } >> - >> major_vers = (vers >> FFA_VERSION_MAJOR_SHIFT) >> & FFA_VERSION_MAJOR_MASK; >> minor_vers = vers & FFA_VERSION_MINOR_MASK; >> @@ -555,6 +544,16 @@ static bool ffa_probe(void) >> ffa_fw_version = vers; >> + for ( int i = 0; i < ARRAY_SIZE(ffa_fw_feat_needed); i++ ) > > This is an index, so please use unsigned int (even though it should technically be size_t). Ack > >> + { >> + if ( !ffa_feature_supported(ffa_fw_feat_needed[i]) ) >> + { >> + printk(XENLOG_INFO "ARM FF-A Firmware does not support 0x%08x\n", >> + ffa_fw_feat_needed[i]); >> + goto err_no_fw; >> + } >> + } >> + >> if ( !ffa_rxtx_init() ) >> { >> printk(XENLOG_ERR "ffa: Error during RXTX buffer init\n"); > > Cheers, Cheers Bertrand > > -- > Julien Grall
Hi Bertrand, On 24/09/2024 09:16, Bertrand Marquis wrote: >> On 22 Sep 2024, at 11:07, Julien Grall <julien@xen.org> wrote: >> >> Hi, >> >> On 19/09/2024 14:19, Bertrand Marquis 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> >>> --- >>> xen/arch/arm/tee/ffa.c | 61 +++++++++++++++++++++--------------------- >>> 1 file changed, 30 insertions(+), 31 deletions(-) >>> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c >>> index 7c84aa6aa43d..7ff2529b2055 100644 >>> --- a/xen/arch/arm/tee/ffa.c >>> +++ b/xen/arch/arm/tee/ffa.c >>> @@ -74,6 +74,24 @@ >>> /* Negotiated FF-A version to use with the SPMC, 0 if not there or supported */ >>> static uint32_t __ro_after_init ffa_fw_version; >>> +/* List of ABI we use from the firmware */ >>> +static const uint32_t ffa_fw_feat_needed[] = { >>> + FFA_VERSION, >>> + FFA_FEATURES, >>> + FFA_NOTIFICATION_BITMAP_CREATE, >>> + FFA_NOTIFICATION_BITMAP_DESTROY, >>> + FFA_PARTITION_INFO_GET, >>> + FFA_NOTIFICATION_INFO_GET_64, >>> + FFA_NOTIFICATION_GET, >>> + FFA_RX_RELEASE, >>> + FFA_RXTX_MAP_64, >>> + FFA_RXTX_UNMAP, >>> + FFA_MEM_SHARE_32, >>> + FFA_MEM_SHARE_64, >>> + FFA_MEM_RECLAIM, >>> + FFA_MSG_SEND_DIRECT_REQ_32, >>> + FFA_MSG_SEND_DIRECT_REQ_64, >>> +}; >> >> NIT: As we are creating an array, could be take the opportunity to provide a name for each feature (we could use macro for that)? This would make easier for the user to know which feature is missing. > > In fact those are not "features" per say but ABI we need to use with the firmware so maybe i should first rename the variable to say abi instead of feat. Thanks for the clarification! With that in mind... > > Now we could create some features out of those as in practice several ABIs are needed to be able to use one feature (for example notifications support require the INFO_GET and the GET). > > In your mind you wanted to have something like: > "Version", FFA_VERSION > "Direct Messages", FFA_MSG_SEND_DIRECT_REQ_32, > "Direct Messages", FFA_MSG_SEND_DIRECT_REQ_64 > > So that we could print a more meaningfull name or would you be ok with just printing "FFA_MSG_SEND_DIRECT_REQ_32" ? ... I was more thinking about printing which ABI is missing. This is more helpful to the user than knowning which features will be missing. This has also the advantage that we can use macro to generate the names. Cheers,
Hi Julien, > On 24 Sep 2024, at 19:31, Julien Grall <julien@xen.org> wrote: > > Hi Bertrand, > > On 24/09/2024 09:16, Bertrand Marquis wrote: >>> On 22 Sep 2024, at 11:07, Julien Grall <julien@xen.org> wrote: >>> >>> Hi, >>> >>> On 19/09/2024 14:19, Bertrand Marquis 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> >>>> --- >>>> xen/arch/arm/tee/ffa.c | 61 +++++++++++++++++++++--------------------- >>>> 1 file changed, 30 insertions(+), 31 deletions(-) >>>> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c >>>> index 7c84aa6aa43d..7ff2529b2055 100644 >>>> --- a/xen/arch/arm/tee/ffa.c >>>> +++ b/xen/arch/arm/tee/ffa.c >>>> @@ -74,6 +74,24 @@ >>>> /* Negotiated FF-A version to use with the SPMC, 0 if not there or supported */ >>>> static uint32_t __ro_after_init ffa_fw_version; >>>> +/* List of ABI we use from the firmware */ >>>> +static const uint32_t ffa_fw_feat_needed[] = { >>>> + FFA_VERSION, >>>> + FFA_FEATURES, >>>> + FFA_NOTIFICATION_BITMAP_CREATE, >>>> + FFA_NOTIFICATION_BITMAP_DESTROY, >>>> + FFA_PARTITION_INFO_GET, >>>> + FFA_NOTIFICATION_INFO_GET_64, >>>> + FFA_NOTIFICATION_GET, >>>> + FFA_RX_RELEASE, >>>> + FFA_RXTX_MAP_64, >>>> + FFA_RXTX_UNMAP, >>>> + FFA_MEM_SHARE_32, >>>> + FFA_MEM_SHARE_64, >>>> + FFA_MEM_RECLAIM, >>>> + FFA_MSG_SEND_DIRECT_REQ_32, >>>> + FFA_MSG_SEND_DIRECT_REQ_64, >>>> +}; >>> >>> NIT: As we are creating an array, could be take the opportunity to provide a name for each feature (we could use macro for that)? This would make easier for the user to know which feature is missing. >> In fact those are not "features" per say but ABI we need to use with the firmware so maybe i should first rename the variable to say abi instead of feat. > > Thanks for the clarification! With that in mind... > >> Now we could create some features out of those as in practice several ABIs are needed to be able to use one feature (for example notifications support require the INFO_GET and the GET). >> In your mind you wanted to have something like: >> "Version", FFA_VERSION >> "Direct Messages", FFA_MSG_SEND_DIRECT_REQ_32, >> "Direct Messages", FFA_MSG_SEND_DIRECT_REQ_64 >> So that we could print a more meaningfull name or would you be ok with just printing "FFA_MSG_SEND_DIRECT_REQ_32" ? > > ... I was more thinking about printing which ABI is missing. This is more helpful to the user than knowning which features will be missing. > > This has also the advantage that we can use macro to generate the names. Ok then i will build a table of strings of the ABI names and use that to say what ABI is not supported when there is one. Cheers Bertrand > > Cheers, > > -- > Julien Grall >
diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c index 7c84aa6aa43d..7ff2529b2055 100644 --- a/xen/arch/arm/tee/ffa.c +++ b/xen/arch/arm/tee/ffa.c @@ -74,6 +74,24 @@ /* Negotiated FF-A version to use with the SPMC, 0 if not there or supported */ static uint32_t __ro_after_init ffa_fw_version; +/* List of ABI we use from the firmware */ +static const uint32_t ffa_fw_feat_needed[] = { + FFA_VERSION, + FFA_FEATURES, + FFA_NOTIFICATION_BITMAP_CREATE, + FFA_NOTIFICATION_BITMAP_DESTROY, + FFA_PARTITION_INFO_GET, + FFA_NOTIFICATION_INFO_GET_64, + FFA_NOTIFICATION_GET, + FFA_RX_RELEASE, + FFA_RXTX_MAP_64, + FFA_RXTX_UNMAP, + FFA_MEM_SHARE_32, + FFA_MEM_SHARE_64, + FFA_MEM_RECLAIM, + FFA_MSG_SEND_DIRECT_REQ_32, + FFA_MSG_SEND_DIRECT_REQ_64, +}; /* * Our rx/tx buffers shared with the SPMC. FFA_RXTX_PAGE_COUNT is the @@ -112,20 +130,9 @@ static bool ffa_get_version(uint32_t *vers) return true; } -static int32_t ffa_features(uint32_t id) +static bool ffa_feature_supported(uint32_t id) { - return ffa_simple_call(FFA_FEATURES, id, 0, 0, 0); -} - -static bool check_mandatory_feature(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) @@ -529,24 +536,6 @@ static bool ffa_probe(void) goto err_no_fw; } - /* - * 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 - * 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) ) - { - printk(XENLOG_ERR "ffa: Mandatory feature not supported by fw\n"); - goto err_no_fw; - } - major_vers = (vers >> FFA_VERSION_MAJOR_SHIFT) & FFA_VERSION_MAJOR_MASK; minor_vers = vers & FFA_VERSION_MINOR_MASK; @@ -555,6 +544,16 @@ static bool ffa_probe(void) ffa_fw_version = vers; + for ( int i = 0; i < ARRAY_SIZE(ffa_fw_feat_needed); i++ ) + { + if ( !ffa_feature_supported(ffa_fw_feat_needed[i]) ) + { + printk(XENLOG_INFO "ARM FF-A Firmware does not support 0x%08x\n", + ffa_fw_feat_needed[i]); + goto err_no_fw; + } + } + if ( !ffa_rxtx_init() ) { printk(XENLOG_ERR "ffa: Error during RXTX buffer init\n");
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> --- xen/arch/arm/tee/ffa.c | 61 +++++++++++++++++++++--------------------- 1 file changed, 30 insertions(+), 31 deletions(-)