|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 04/10] xen/arm: ffa: Fine granular call support
On Wed, Oct 23, 2024 at 11:58 AM Jens Wiklander
<jens.wiklander@xxxxxxxxxx> wrote:
>
> 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>
I'm sorry, I'm having second thoughts about this patch. I have two concerns:
1. Xen will complain at boot with XENLOG_INFO if an ABI function
listed in ffa_fw_abi_needed is missing. With the current list of ABI
functions that's somewhat OK since it was a cause of disabling FF-A
support before. But as the list grows it may become annoying or even
confusing since when Xen supports more features it may complain more
even if there is no regression compared to previous versions. If we
need to print anything perhaps XENLOG_DEBUG is better.
2. FFA_FEATURES may return success for features not supported by the
SPMC. How about only returning success for features in the
ffa_fw_abi_needed bitmap?
Cheers,
Jens
>
> 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 |