[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 04/10] xen/arm: ffa: Fine granular call support
Hi, On Sun, Sep 22, 2024 at 3:04 PM 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. > > > 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. That's a good point, but all the FF-A handling should be in the same module since there's bound to be code and state to share. The problem is that FF-A tries to be a TEE mediator while it's about to become more than that. We can work around it by probing the OP-TEE mediator first, but it's adding another exception or special case. Would it make sense to move the FF-A mediator out of the TEE mediator framework and establish it separately? Cheers, Jens > > > > > 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. > > > } > > > > 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? > > > 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. > > > +#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. > > > + > > 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). > > > +} > > + > > #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()? > > > - > > - 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 ) > > Cheers, > > -- > Julien Grall > >
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |