1. Patchew
  2. Xen
  3. [PATCH v2 00/10] xen/arm: ffa: Improvements and fixes
  4. View patch

[PATCH v2 02/10] xen/arm: ffa: Rework feature discovery

Bertrand Marquis posted 10 patches 1 month ago
[PATCH v2 02/10] xen/arm: ffa: Rework feature discovery
Posted by Bertrand Marquis 1 month ago 
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;
+    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
Re: [PATCH v2 02/10] xen/arm: ffa: Rework feature discovery
Posted by Jens Wiklander 1 month ago 
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
>
Re: [PATCH v2 02/10] xen/arm: ffa: Rework feature discovery
Posted by Bertrand Marquis 4 weeks, 1 day ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.