Re: [PATCH 04/10] xen/arm: ffa: Fine granular call support

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Julien,

> On 22 Sep 2024, at 15:03, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Bertrand,
> 
> On 19/09/2024 14:19, Bertrand Marquis wrote:
>> Create a bitmap to store which feature is supported or not by the
>> firmware and use it to filter which calls done to the firmware.
>> With this enabled. allow FF-A support to be activated for guest even if
> 
> Typo: s/./,/ I think.

Ack

> 
>> the firmware does not support it.
> 
> Can you explain why you want to do this?
> 
> The TEE mediator was designed to interpose with the HW. Without the HW it 
> doesn't entirely make sense to try to use it.
> 
> It would also not work if the host system is using OP-TEE and expose to some 
> VM FF-A. So it feels that the mediator may not be the right place to handle 
> this case.

As said in the answer to the thread on the subject, I will revert this change 
for now and only introduce this once we have VM to VM support.
The goal of this change was to ensure that we could support VM to VM case even 
if the firmware does not support FF-A but that only make sense with VM to VM 
support in.

> 
>> As a consequence, if the firmware is not there or not supported, we
>> return an empty list of partitions to VMs requesting it through
>> PARTINFO_GET ABI.
>> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
>> ---
>>  xen/arch/arm/tee/ffa.c          | 31 ++++++++++++++++++++-----------
>>  xen/arch/arm/tee/ffa_notif.c    |  7 +++++++
>>  xen/arch/arm/tee/ffa_partinfo.c | 31 +++++++++++++++++++++++++++++--
>>  xen/arch/arm/tee/ffa_private.h  | 28 ++++++++++++++++++++++++++++
>>  xen/arch/arm/tee/ffa_rxtx.c     | 13 ++++++-------
>>  xen/arch/arm/tee/ffa_shm.c      | 12 ++++++++++++
>>  6 files changed, 102 insertions(+), 20 deletions(-)
>> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
>> index 1f602f25d097..53960b146220 100644
>> --- a/xen/arch/arm/tee/ffa.c
>> +++ b/xen/arch/arm/tee/ffa.c
>> @@ -72,7 +72,10 @@
>>  #include "ffa_private.h"
>>    /* Negotiated FF-A version to use with the SPMC, 0 if not there or 
>> supported */
>> -static uint32_t __ro_after_init ffa_fw_version;
>> +uint32_t __ro_after_init ffa_fw_version;
>> +
>> +/* Features supported by the SPMC or secure world when present */
>> +DECLARE_BITMAP(ffa_fw_feat_supported, FEAT_FUNC_BITMAP_SIZE);
>>    /* List of ABI we use from the firmware */
>>  static const uint32_t ffa_fw_feat_needed[] = {
>> @@ -174,6 +177,13 @@ static void handle_msg_send_direct_req(struct 
>> cpu_user_regs *regs, uint32_t fid)
>>      else
>>          mask = GENMASK_ULL(31, 0);
>>  +    if ( !ffa_fw_supports_fid(fid) )
>> +    {
>> +        resp.a0 = FFA_ERROR;
>> +        resp.a2 = FFA_RET_NOT_SUPPORTED;
>> +        goto out;
>> +    }
>> +
>>      src_dst = get_user_reg(regs, 1);
>>      if ( (src_dst >> 16) != ffa_get_vm_id(d) )
>>      {
>> @@ -387,8 +397,6 @@ static int ffa_domain_init(struct domain *d)
>>      struct ffa_ctx *ctx;
>>      int ret;
>>  -    if ( !ffa_fw_version )
>> -        return -ENODEV;
> >        /*>         * We can't use that last possible domain ID or 
> > ffa_get_vm_id() would
>>        * cause an overflow.
>> @@ -523,6 +531,9 @@ static bool ffa_probe(void)
>>      printk(XENLOG_INFO "ARM FF-A Mediator version %u.%u\n",
>>             FFA_MY_VERSION_MAJOR, FFA_MY_VERSION_MINOR);
>>  +    INIT_LIST_HEAD(&ffa_teardown_head);
>> +    init_timer(&ffa_teardown_timer, ffa_teardown_timer_callback, NULL, 0);
>> +
>>      /*
>>       * psci_init_smccc() updates this value with what's reported by EL-3
>>       * or secure world.
>> @@ -568,12 +579,12 @@ static bool ffa_probe(void)
>>        for ( int i = 0; i < ARRAY_SIZE(ffa_fw_feat_needed); i++ )
>>      {
>> -        if ( !ffa_feature_supported(ffa_fw_feat_needed[i]) )
>> -        {
>> +        if ( ffa_feature_supported(ffa_fw_feat_needed[i]) )
>> +            set_bit(FEAT_FUNC_BITNUM(ffa_fw_feat_needed[i]),
>> +                    ffa_fw_feat_supported);
>> +        else
>>              printk(XENLOG_INFO "ARM FF-A Firmware does not support 
>> 0x%08x\n",
>> -                   ffa_fw_feat_needed[i]);
>> -            goto err_no_fw;
>> -        }
>> +                       ffa_fw_feat_needed[i]);
>>      }
>>        if ( !ffa_rxtx_init() )
>> @@ -586,8 +597,6 @@ static bool ffa_probe(void)
>>          goto err_rxtx_destroy;
>>        ffa_notif_init();
>> -    INIT_LIST_HEAD(&ffa_teardown_head);
>> -    init_timer(&ffa_teardown_timer, ffa_teardown_timer_callback, NULL, 0);
>>        return true;
>>  @@ -597,7 +606,7 @@ err_no_fw:
>>      ffa_fw_version = 0;
>>      printk(XENLOG_INFO "ARM FF-A No firmware support\n");
>>  -    return false;
>> +    return true;
> 
> So effectively now ffa_probe() will always return true. If we end up to probe 
> FF-A before OP-Tee, then we would always say "FF-A"  is the TEE mediator.

Ack, i will revert that for now.

> 
>>  }
>>    static const struct tee_mediator_ops ffa_ops =
>> diff --git a/xen/arch/arm/tee/ffa_notif.c b/xen/arch/arm/tee/ffa_notif.c
>> index 541e61d2f606..4b3e46318f4b 100644
>> --- a/xen/arch/arm/tee/ffa_notif.c
>> +++ b/xen/arch/arm/tee/ffa_notif.c
>> @@ -377,6 +377,13 @@ void ffa_notif_init(void)
>>      unsigned int irq;
>>      int ret;
>>  +    /* Only enable fw notification if all ABIs we need are supported */
>> +    if ( !(ffa_fw_supports_fid(FFA_NOTIFICATION_BITMAP_CREATE) &&
>> +           ffa_fw_supports_fid(FFA_NOTIFICATION_BITMAP_DESTROY) &&
>> +           ffa_fw_supports_fid(FFA_NOTIFICATION_GET) &&
>> +           ffa_fw_supports_fid(FFA_NOTIFICATION_INFO_GET_64)) )
>> +        return;
>> +
>>      arm_smccc_1_2_smc(&arg, &resp);
>>      if ( resp.a0 != FFA_SUCCESS_32 )
>>          return;
>> diff --git a/xen/arch/arm/tee/ffa_partinfo.c 
>> b/xen/arch/arm/tee/ffa_partinfo.c
>> index 93a03c6bc672..a42bd92ab8cf 100644
>> --- a/xen/arch/arm/tee/ffa_partinfo.c
>> +++ b/xen/arch/arm/tee/ffa_partinfo.c
>> @@ -77,7 +77,15 @@ int32_t ffa_handle_partition_info_get(uint32_t w1, 
>> uint32_t w2, uint32_t w3,
>>       */
>>      if ( w5 == FFA_PARTITION_INFO_GET_COUNT_FLAG &&
>>           ctx->guest_vers == FFA_VERSION_1_1 )
>> -        return ffa_partition_info_get(w1, w2, w3, w4, w5, count, fpi_size);
>> +    {
>> +        if ( ffa_fw_supports_fid(FFA_PARTITION_INFO_GET) )
>> +            return ffa_partition_info_get(w1, w2, w3, w4, w5, count, 
>> fpi_size);
>> +        else
>> +        {
>> +            *count = 0;
>> +            return FFA_RET_OK;
>> +        }
>> +    }
>>      if ( w5 )
>>          return FFA_RET_INVALID_PARAMETERS;
>>  @@ -87,6 +95,18 @@ int32_t ffa_handle_partition_info_get(uint32_t w1, 
>> uint32_t w2, uint32_t w3,
>>      if ( !spin_trylock(&ctx->rx_lock) )
>>          return FFA_RET_BUSY;
>>  +    if ( !ffa_fw_supports_fid(FFA_PARTITION_INFO_GET) )
>> +    {
>> +        if ( ctx->guest_vers == FFA_VERSION_1_0 )
>> +            *fpi_size = sizeof(struct ffa_partition_info_1_0);
>> +        else
>> +            *fpi_size = sizeof(struct ffa_partition_info_1_1);
>> +
>> +        *count = 0;
>> +        ret = FFA_RET_OK;
>> +        goto out;
>> +    }
>> +
>>      if ( !ctx->page_count || !ctx->rx_is_free )
>>          goto out;
>>      spin_lock(&ffa_rx_buffer_lock);
>> @@ -250,6 +270,11 @@ bool ffa_partinfo_init(void)
>>      uint32_t count;
>>      int e;
>>  +    if ( !ffa_fw_supports_fid(FFA_PARTITION_INFO_GET) ||
>> +         !ffa_fw_supports_fid(FFA_MSG_SEND_DIRECT_REQ_32) ||
>> +         !ffa_rx || !ffa_tx )
>> +        return false;
>> +
>>      e = ffa_partition_info_get(0, 0, 0, 0, 0, &count, &fpi_size);
>>      if ( e )
>>      {
>> @@ -267,7 +292,6 @@ bool ffa_partinfo_init(void)
>>    out:
>>      ffa_rx_release();
> > -
> 
> Spurious change?

Yes

> 
>>      return ret;
>>  }
>>  @@ -313,6 +337,9 @@ int ffa_partinfo_domain_init(struct domain *d)
>>      unsigned int n;
>>      int32_t res;
>>  +    if ( !ffa_fw_supports_fid(FFA_MSG_SEND_DIRECT_REQ_32) )
>> +        return 0;
>> +
>>      ctx->vm_destroy_bitmap = xzalloc_array(unsigned long, count);
>>      if ( !ctx->vm_destroy_bitmap )
>>          return -ENOMEM;
>> diff --git a/xen/arch/arm/tee/ffa_private.h b/xen/arch/arm/tee/ffa_private.h
>> index 7c6b06f686fc..d4dc9c8cd67b 100644
>> --- a/xen/arch/arm/tee/ffa_private.h
>> +++ b/xen/arch/arm/tee/ffa_private.h
>> @@ -14,6 +14,7 @@
>>  #include <xen/spinlock.h>
>>  #include <xen/sched.h>
>>  #include <xen/time.h>
>> +#include <xen/bitmap.h>
>>    /* Error codes */
>>  #define FFA_RET_OK                      0
>> @@ -238,6 +239,23 @@
>>  #define FFA_NOTIFICATION_INFO_GET_32    0x84000083U
>>  #define FFA_NOTIFICATION_INFO_GET_64    0xC4000083U
>>  +/**
>> + * Encoding of features supported or not by the fw in a bitmap:
>> + * - Function IDs are going from 0x60 to 0xFF
>> + * - A function can be supported in 32 and/or 64bit
>> + * The bitmap has one bit for each function in 32 and 64 bit.
>> + */
>> +#define FFA_FUNC_MIN           FFA_ERROR
>> +#define FFA_FUNC_MAX           FFA_NOTIFICATION_INFO_GET_64
> 
> These two defines confused me because FAA_ERROR is 0x84000060U and 
> FFA_NOTIFICATION_INFO_GET_64 is 0xC4000083U. I think it would be better if we 
> define them using FFA_FUNC_ID.
> 
> We also probably want to have a compiler time check that FFA_FUNC_MIN is < 
> FFA_FUNC_MAX.

Ack.

> 
>> +#define FFA_FUNC_ID(id)        ((id) & ARM_SMCCC_FUNC_MASK)
>> +#define FFA_FUNC_CONV(id)      (((id) >> ARM_SMCCC_CONV_SHIFT) & BIT(0,U))
>> +
>> +#define FEAT_FUNC_BITMAP_SIZE   (2 * (FFA_FUNC_ID(FFA_FUNC_MAX) - \
>> +                                    FFA_FUNC_ID(FFA_FUNC_MIN) + 1))
>> +#define FEAT_FUNC_BITNUM(id)    ((FFA_FUNC_ID(id) - \
>> +                                    FFA_FUNC_ID(FFA_FUNC_MIN)) << 1 | \
>> +                                    FFA_FUNC_CONV(id))
> 
> The code seem to make two assumptions:
>  1. id is a constant
>  2. id is always valid
> 
> I think it would be good to have a BUILD_BUG_ON(). This should avoid the two 
> assumptions.

Yes you are right.

I guess i could make a BUILD_BUG_ON(id > FFA_FUNC_MAX) but the macro will 
definitely look ugly.
I think i will switch to static inline functions doing the thing needed to have 
something cleaner.

> 
>> +
>>  struct ffa_ctx_notif {
>>      bool enabled;
>>  @@ -286,6 +304,8 @@ extern void *ffa_rx;
>>  extern void *ffa_tx;
>>  extern spinlock_t ffa_rx_buffer_lock;
>>  extern spinlock_t ffa_tx_buffer_lock;
>> +extern uint32_t __ro_after_init ffa_fw_version;
>> +extern DECLARE_BITMAP(ffa_fw_feat_supported, FEAT_FUNC_BITMAP_SIZE);
>>    bool ffa_shm_domain_destroy(struct domain *d);
>>  void ffa_handle_mem_share(struct cpu_user_regs *regs);
>> @@ -398,4 +418,12 @@ static inline int32_t ffa_rx_release(void)
>>      return ffa_simple_call(FFA_RX_RELEASE, 0, 0, 0, 0);
>>  }
>>  +static inline bool ffa_fw_supports_fid(uint32_t fid)
>> +{
>> +    if ( ffa_fw_version == 0 )
>> +        return false;
> 
> You could avoid this check if you ensure that ...
> 
> > +    else> +        return test_bit(FEAT_FUNC_BITNUM(fid), 
> > ffa_fw_feat_supported);
> 
> .. the bitmap is always zeroed if ffa_fw_version is 0. You also want to check 
> that the fid is valid (could be done at build time if fid is always a 
> constant).

Agree, the point here was to optimize a bit and prevent the bitmap check when 
possible but that is probably not worth it.
Will fix.

> 
>> +}
>> +
>>  #endif /*__FFA_PRIVATE_H__*/
>> diff --git a/xen/arch/arm/tee/ffa_rxtx.c b/xen/arch/arm/tee/ffa_rxtx.c
>> index 661764052e67..cb54c76911fd 100644
>> --- a/xen/arch/arm/tee/ffa_rxtx.c
>> +++ b/xen/arch/arm/tee/ffa_rxtx.c
>> @@ -193,24 +193,23 @@ bool ffa_rxtx_init(void)
>>  {
>>      int e;
>>  +    /* Firmware not there or not supporting */
>> +    if ( !ffa_fw_supports_fid(FFA_RXTX_MAP_64) )
>> +        return false;
>> +
>>      ffa_rx = alloc_xenheap_pages(get_order_from_pages(FFA_RXTX_PAGE_COUNT), 
>> 0);
>>      if ( !ffa_rx )
>>          return false;
>>        ffa_tx = 
>> alloc_xenheap_pages(get_order_from_pages(FFA_RXTX_PAGE_COUNT), 0);
>>      if ( !ffa_tx )
>> -        goto err;
>> +        return false;
>>        e = ffa_rxtx_map(__pa(ffa_tx), __pa(ffa_rx), FFA_RXTX_PAGE_COUNT);
>>      if ( e )
>>      {
>>          printk(XENLOG_ERR "ffa: Failed to map rxtx: error %d\n", e);
>> -        goto err;
>> +        return false;
>>      }
>>      return true;
>> -
>> -err:
>> -    ffa_rxtx_destroy();
> 
> This seems to be unrelated to the change. Can you explain why we don't need 
> to call ffa_rxtx_destroy()?

Yes this is definitely a mistake. I will fix that.

> 
>> -
>> -    return false;
> >   }> diff --git a/xen/arch/arm/tee/ffa_shm.c b/xen/arch/arm/tee/ffa_shm.c
>> index 370d83ec5cf8..efa5b67db8e1 100644
>> --- a/xen/arch/arm/tee/ffa_shm.c
>> +++ b/xen/arch/arm/tee/ffa_shm.c
>> @@ -149,6 +149,9 @@ static int32_t ffa_mem_share(uint32_t tot_len, uint32_t 
>> frag_len,
>>  static int32_t ffa_mem_reclaim(uint32_t handle_lo, uint32_t handle_hi,
>>                                 uint32_t flags)
>>  {
>> +    if ( !ffa_fw_supports_fid(FFA_MEM_RECLAIM) )
>> +        return FFA_RET_NOT_SUPPORTED;
>> +
>>      return ffa_simple_call(FFA_MEM_RECLAIM, handle_lo, handle_hi, flags, 0);
>>  }
>>  @@ -467,6 +470,12 @@ void ffa_handle_mem_share(struct cpu_user_regs *regs)
>>      uint32_t range_count;
>>      uint32_t region_offs;
>>  +    if ( !ffa_fw_supports_fid(FFA_MEM_SHARE_64) )
>> +    {
>> +        ret = FFA_RET_NOT_SUPPORTED;
>> +        goto out_set_ret;
>> +    }
>> +
>>      /*
>>       * We're only accepting memory transaction descriptors via the rx/tx
>>       * buffer.
>> @@ -621,6 +630,9 @@ int ffa_handle_mem_reclaim(uint64_t handle, uint32_t 
>> flags)
>>      register_t handle_lo;
>>      int ret;
>>  +    if ( !ffa_fw_supports_fid(FFA_MEM_RECLAIM) )
>> +        return FFA_RET_NOT_SUPPORTED;
>> +
>>      spin_lock(&ctx->lock);
>>      shm = find_shm_mem(ctx, handle);
>>      if ( shm )

Thanks again for the review.

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall