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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Jens,

> On 24 Oct 2024, at 10:15, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
> 
> 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.

This is only printed at boot and in the worst case it would list all needed ABI.
If the list printed becomes big, it probably means that almost nothing is
possible to do which might be interesting for the user.
Only seeing this information with debug prints might lead into normal users
not understanding why communication with secure world are not working
without having a reason.
I would expect that the most common case will be for the list of printed
entries to be limited (right now it only prints something for 64bit sharing 
which
should be solved in Hafnium).
As Xen is already quite verbose in INFO mode during boot and this is not
a runtime print, I think it is ok.

> 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?

This would be a reinterpretation of the specification and could create
issues in some cases (some ABIs might be supported by Xen but not
by the SPMC and still work correctly this way) and even more when
we will have VM to VM.
The specification is saying that we should return what we support and
not what is supported by the SPMC. Filtering based on what is supported
by the SPMC and what will still work if not supported by the SPMC and
what we do not support even if it is supported by the SPMC might become
quickly very complex.

What do you think we would gain from doing what you suggest instead of
what we have right now ?

Cheers
Bertrand

> 
> 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