|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 04/10] xen/arm: ffa: Fine granular call support
Hi Bertrand,
On Wed, Oct 16, 2024 at 10:32 AM Bertrand Marquis
<bertrand.marquis@xxxxxxx> wrote:
>
> Create a bitmap to store which feature is supported or not by the
> firmware and use it to filter which calls are done to the firmware.
>
> While there reoder ABI definition by numbers to easily find the min and
> max ones.
>
> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> ---
> Changes in v2:
> - rename fw_feat to abi and macros to FFA_ABI to be coherent with the
> abi needed change done before
> - rework the macros to be simpler by directly defining MIN and MAX using
> only Function ids
> - check that requested function ids do not go over the bitmap size in
> ffa_fw_supports_fid
> - add an ASSERT to make sure that we do not try to set bits outside of
> the bitmap
> - turn off FF-A if there is not firmware support and adapt the commit
> message to reflect this
> - add a compile time check that FFA_ABI_MIN < FFA_ABI_MAX
> - remove spurious line removal
> - restore proper cleanup of rxtx init in case of error
> - reorder ABI by numbers
> ---
> xen/arch/arm/tee/ffa.c | 28 +++++++++++++++---------
> xen/arch/arm/tee/ffa_notif.c | 7 ++++++
> xen/arch/arm/tee/ffa_partinfo.c | 30 +++++++++++++++++++++++++-
> xen/arch/arm/tee/ffa_private.h | 38 ++++++++++++++++++++++++++++-----
> xen/arch/arm/tee/ffa_rxtx.c | 4 ++++
> xen/arch/arm/tee/ffa_shm.c | 12 +++++++++++
> 6 files changed, 103 insertions(+), 16 deletions(-)
Looks good.
Reviewed-by: Jens Wiklander <jens.wiklander@xxxxxxxxxx>
Cheers,
Jens
>
> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> index 1ee6b2895e92..267d4435ac08 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_abi_supported, FFA_ABI_BITMAP_SIZE);
>
> struct ffa_fw_abi {
> const uint32_t id;
> @@ -177,6 +180,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) )
> {
> @@ -577,19 +587,16 @@ static bool ffa_probe(void)
> else
> ffa_fw_version = vers;
>
> - /*
> - * 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.
> - */
> for ( unsigned int i = 0; i < ARRAY_SIZE(ffa_fw_abi_needed); i++ )
> {
> - if ( !ffa_abi_supported(ffa_fw_abi_needed[i].id) )
> - {
> + ASSERT(FFA_ABI_BITNUM(ffa_fw_abi_needed[i].id) <
> FFA_ABI_BITMAP_SIZE);
> +
> + if ( ffa_abi_supported(ffa_fw_abi_needed[i].id) )
> + set_bit(FFA_ABI_BITNUM(ffa_fw_abi_needed[i].id),
> + ffa_fw_abi_supported);
> + else
> printk(XENLOG_INFO "ARM FF-A Firmware does not support %s\n",
> ffa_fw_abi_needed[i].name);
> - goto err_no_fw;
> - }
> }
>
> if ( !ffa_rxtx_init() )
> @@ -611,6 +618,7 @@ err_rxtx_destroy:
> ffa_rxtx_destroy();
> err_no_fw:
> ffa_fw_version = 0;
> + bitmap_zero(ffa_fw_abi_supported, FFA_ABI_BITMAP_SIZE);
> printk(XENLOG_WARNING "ARM FF-A No firmware support\n");
>
> return false;
> 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..99c48f0e5c05 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 )
> {
> @@ -313,6 +338,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 045d9c4a0b56..85eb61c13464 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
> @@ -201,18 +202,17 @@
> #define FFA_INTERRUPT 0x84000062U
> #define FFA_VERSION 0x84000063U
> #define FFA_FEATURES 0x84000064U
> -#define FFA_RX_ACQUIRE 0x84000084U
> #define FFA_RX_RELEASE 0x84000065U
> #define FFA_RXTX_MAP_32 0x84000066U
> #define FFA_RXTX_MAP_64 0xC4000066U
> #define FFA_RXTX_UNMAP 0x84000067U
> #define FFA_PARTITION_INFO_GET 0x84000068U
> #define FFA_ID_GET 0x84000069U
> -#define FFA_SPM_ID_GET 0x84000085U
> +#define FFA_MSG_POLL 0x8400006AU
> #define FFA_MSG_WAIT 0x8400006BU
> #define FFA_MSG_YIELD 0x8400006CU
> #define FFA_RUN 0x8400006DU
> -#define FFA_MSG_SEND2 0x84000086U
> +#define FFA_MSG_SEND 0x8400006EU
> #define FFA_MSG_SEND_DIRECT_REQ_32 0x8400006FU
> #define FFA_MSG_SEND_DIRECT_REQ_64 0xC400006FU
> #define FFA_MSG_SEND_DIRECT_RESP_32 0x84000070U
> @@ -230,8 +230,6 @@
> #define FFA_MEM_RECLAIM 0x84000077U
> #define FFA_MEM_FRAG_RX 0x8400007AU
> #define FFA_MEM_FRAG_TX 0x8400007BU
> -#define FFA_MSG_SEND 0x8400006EU
> -#define FFA_MSG_POLL 0x8400006AU
> #define FFA_NOTIFICATION_BITMAP_CREATE 0x8400007DU
> #define FFA_NOTIFICATION_BITMAP_DESTROY 0x8400007EU
> #define FFA_NOTIFICATION_BIND 0x8400007FU
> @@ -240,6 +238,25 @@
> #define FFA_NOTIFICATION_GET 0x84000082U
> #define FFA_NOTIFICATION_INFO_GET_32 0x84000083U
> #define FFA_NOTIFICATION_INFO_GET_64 0xC4000083U
> +#define FFA_RX_ACQUIRE 0x84000084U
> +#define FFA_SPM_ID_GET 0x84000085U
> +#define FFA_MSG_SEND2 0x84000086U
> +
> +/**
> + * 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_ABI_ID(id) ((id) & ARM_SMCCC_FUNC_MASK)
> +#define FFA_ABI_CONV(id) (((id) >> ARM_SMCCC_CONV_SHIFT) & BIT(0,U))
> +
> +#define FFA_ABI_MIN FFA_ABI_ID(FFA_ERROR)
> +#define FFA_ABI_MAX FFA_ABI_ID(FFA_MSG_SEND2)
> +
> +#define FFA_ABI_BITMAP_SIZE (2 * (FFA_ABI_MAX - FFA_ABI_MIN + 1))
> +#define FFA_ABI_BITNUM(id) ((FFA_ABI_ID(id) - FFA_ABI_MIN) << 1 | \
> + FFA_ABI_CONV(id))
>
> struct ffa_ctx_notif {
> bool enabled;
> @@ -289,6 +306,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_abi_supported, FFA_ABI_BITMAP_SIZE);
>
> bool ffa_shm_domain_destroy(struct domain *d);
> void ffa_handle_mem_share(struct cpu_user_regs *regs);
> @@ -401,4 +420,13 @@ 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)
> +{
> + BUILD_BUG_ON(FFA_ABI_MIN > FFA_ABI_MAX);
> +
> + if ( FFA_ABI_BITNUM(fid) > FFA_ABI_BITMAP_SIZE)
> + return false;
> + return test_bit(FFA_ABI_BITNUM(fid), ffa_fw_abi_supported);
> +}
> +
> #endif /*__FFA_PRIVATE_H__*/
> diff --git a/xen/arch/arm/tee/ffa_rxtx.c b/xen/arch/arm/tee/ffa_rxtx.c
> index 661764052e67..b6931c855779 100644
> --- a/xen/arch/arm/tee/ffa_rxtx.c
> +++ b/xen/arch/arm/tee/ffa_rxtx.c
> @@ -193,6 +193,10 @@ 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;
> 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 )
> --
> 2.47.0
>
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |