Re: [PATCH v2 06/10] xen/arm: ffa: Use bit 15 convention for SPs

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Jens,

> On 23 Oct 2024, at 15:15, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
> 
> Hi Bertrand,
> 
> On Wed, Oct 16, 2024 at 10:32 AM Bertrand Marquis
> <bertrand.marquis@xxxxxxx> wrote:
>> 
>> Make use and required to have bit 15 convention respected by secure
>> world (having bit 15 of IDs set for secure endpoints and non-set for
>> non-secure ones).
>> If any secure partition has an ID with bit 15 not set, it will not be
>> possible to contact or detect them.
>> Print an error log during probe for each secure endpoint detected with
>> bit 15 not set.
>> 
>> We are switching to this convention because Xen is currently not using
>> VMIDs with bit 15 set so we are sure that no VM will have it set (this
>> is ensured by BUILD_BUG_ON in case this becomes false in the future).
>> It is allowing to easily distinguish between secure and non-secure
>> endpoints, preventing the need to store a list of secure endpoint IDs in
>> Xen.
>> 
>> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
>> ---
>> Changes in v2:
>> - rebase
>> ---
>> xen/arch/arm/tee/ffa.c          | 22 +++++++++++---
>> xen/arch/arm/tee/ffa_partinfo.c | 54 +++++++++++++++++++++++++--------
>> xen/arch/arm/tee/ffa_private.h  |  7 +++++
>> xen/arch/arm/tee/ffa_shm.c      | 12 +++++++-
>> 4 files changed, 77 insertions(+), 18 deletions(-)
>> 
>> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
>> index 4b55e8b48f01..a292003ca9fe 100644
>> --- a/xen/arch/arm/tee/ffa.c
>> +++ b/xen/arch/arm/tee/ffa.c
>> @@ -195,6 +195,14 @@ static void handle_msg_send_direct_req(struct 
>> cpu_user_regs *regs, uint32_t fid)
>>         goto out;
>>     }
>> 
>> +    /* we do not support direct messages to VMs */
>> +    if ( !FFA_ID_IS_SECURE(src_dst & GENMASK(15,0)) )
>> +    {
>> +        resp.a0 = FFA_ERROR;
>> +        resp.a2 = FFA_RET_NOT_SUPPORTED;
>> +        goto out;
>> +    }
>> +
>>     arg.a1 = src_dst;
>>     arg.a2 = get_user_reg(regs, 2) & mask;
>>     arg.a3 = get_user_reg(regs, 3) & mask;
>> @@ -391,11 +399,15 @@ static int ffa_domain_init(struct domain *d)
>> 
>>     if ( !ffa_fw_version )
>>         return -ENODEV;
>> -     /*
>> -      * We can't use that last possible domain ID or ffa_get_vm_id() would
>> -      * cause an overflow.
>> -      */
>> -    if ( d->domain_id >= UINT16_MAX)
>> +    /*
>> +     * We are using the domain_id + 1 as the FF-A ID for VMs as FF-A ID 0 is
>> +     * reserved for the hypervisor and we only support secure endpoints 
>> using
>> +     * FF-A IDs with BIT 15 set to 1 so make sure those are not used by Xen.
>> +     */
>> +    BUILD_BUG_ON(DOMID_FIRST_RESERVED >= UINT16_MAX);
>> +    BUILD_BUG_ON((DOMID_MASK & BIT(15, U)) != 0);
>> +
>> +    if ( d->domain_id >= DOMID_FIRST_RESERVED )
>>         return -ERANGE;
>> 
>>     ctx = xzalloc(struct ffa_ctx);
>> diff --git a/xen/arch/arm/tee/ffa_partinfo.c 
>> b/xen/arch/arm/tee/ffa_partinfo.c
>> index 75a073d090e0..3cf801523296 100644
>> --- a/xen/arch/arm/tee/ffa_partinfo.c
>> +++ b/xen/arch/arm/tee/ffa_partinfo.c
>> @@ -169,14 +169,26 @@ void ffa_handle_partition_info_get(struct 
>> cpu_user_regs *regs)
>> 
>>     if ( ffa_sp_count > 0 )
>>     {
>> -        uint32_t n;
>> +        uint32_t n, real_num = ffa_sp_count;
> 
> Nit: how about n_limit instead of real_num?

Sure i will change that in v3.

> 
>>         void *src_buf = ffa_rx;
>> 
>>         /* copy the secure partitions info */
>> -        for ( n = 0; n < ffa_sp_count; n++ )
>> +        for ( n = 0; n < real_num; n++ )
>>         {
>> -            memcpy(dst_buf, src_buf, dst_size);
>> -            dst_buf += dst_size;
>> +            struct ffa_partition_info_1_1 *fpi = src_buf;
>> +
>> +            /* filter out SP not following bit 15 convention if any */
>> +            if ( FFA_ID_IS_SECURE(fpi->id) )
>> +            {
>> +                memcpy(dst_buf, src_buf, dst_size);
>> +                dst_buf += dst_size;
>> +            }
>> +            else
>> +            {
>> +                printk(XENLOG_INFO "ffa: sp id 0x%04x skipped, bit 15 is 
>> 0\n",
>> +                       fpi->id);
> 
> We have already logged this in init_subscribers() below. Is there a
> risk of flooding the logs with this?

You are right, having it printed once is enough.
I will remove this LOG.

Cheers
Bertrand

> 
> Cheers,
> Jens
> 
>> +                ffa_sp_count--;
>> +            }
>>             src_buf += src_size;
>>         }
>>     }
>> @@ -276,10 +288,25 @@ static bool init_subscribers(uint16_t count, uint32_t 
>> fpi_size)
>>     {
>>         fpi = ffa_rx + n * fpi_size;
>> 
>> -        if ( fpi->partition_properties & FFA_PART_PROP_NOTIF_CREATED )
>> -            subscr_vm_created_count++;
>> -        if ( fpi->partition_properties & FFA_PART_PROP_NOTIF_DESTROYED )
>> -            subscr_vm_destroyed_count++;
>> +        /*
>> +         * We need to have secure partitions using bit 15 set convention for
>> +         * secure partition IDs.
>> +         * Inform the user with a log and discard giving created or destroy
>> +         * event to those IDs.
>> +         */
>> +        if ( !FFA_ID_IS_SECURE(fpi->id) )
>> +        {
>> +            printk(XENLOG_ERR "ffa: Firmware is not using bit 15 convention 
>> for IDs !!\n"
>> +                              "ffa: Secure partition with id 0x%04x cannot 
>> be used\n",
>> +                              fpi->id);
>> +        }
>> +        else
>> +        {
>> +            if ( fpi->partition_properties & FFA_PART_PROP_NOTIF_CREATED )
>> +                subscr_vm_created_count++;
>> +            if ( fpi->partition_properties & FFA_PART_PROP_NOTIF_DESTROYED )
>> +                subscr_vm_destroyed_count++;
>> +        }
>>     }
>> 
>>     if ( subscr_vm_created_count )
>> @@ -299,10 +326,13 @@ static bool init_subscribers(uint16_t count, uint32_t 
>> fpi_size)
>>     {
>>         fpi = ffa_rx + n * fpi_size;
>> 
>> -        if ( fpi->partition_properties & FFA_PART_PROP_NOTIF_CREATED )
>> -            subscr_vm_created[c_pos++] = fpi->id;
>> -        if ( fpi->partition_properties & FFA_PART_PROP_NOTIF_DESTROYED )
>> -            subscr_vm_destroyed[d_pos++] = fpi->id;
>> +        if ( FFA_ID_IS_SECURE(fpi->id) )
>> +        {
>> +            if ( fpi->partition_properties & FFA_PART_PROP_NOTIF_CREATED )
>> +                subscr_vm_created[c_pos++] = fpi->id;
>> +            if ( fpi->partition_properties & FFA_PART_PROP_NOTIF_DESTROYED )
>> +                subscr_vm_destroyed[d_pos++] = fpi->id;
>> +        }
>>     }
>> 
>>     return true;
>> diff --git a/xen/arch/arm/tee/ffa_private.h b/xen/arch/arm/tee/ffa_private.h
>> index e5bc73f9039e..afe69b43dbef 100644
>> --- a/xen/arch/arm/tee/ffa_private.h
>> +++ b/xen/arch/arm/tee/ffa_private.h
>> @@ -108,6 +108,13 @@
>>  */
>> #define FFA_CTX_TEARDOWN_DELAY          SECONDS(1)
>> 
>> +/*
>> + * We rely on the convention suggested but not mandated by the FF-A
>> + * specification that secure world endpoint identifiers have the bit 15
>> + * set and normal world have it set to 0.
>> + */
>> +#define FFA_ID_IS_SECURE(id)    ((id) & BIT(15, U))
>> +
>> /* FF-A-1.1-REL0 section 10.9.2 Memory region handle, page 167 */
>> #define FFA_HANDLE_HYP_FLAG             BIT(63, ULL)
>> #define FFA_HANDLE_INVALID              0xffffffffffffffffULL
>> diff --git a/xen/arch/arm/tee/ffa_shm.c b/xen/arch/arm/tee/ffa_shm.c
>> index efa5b67db8e1..29675f9ba3f7 100644
>> --- a/xen/arch/arm/tee/ffa_shm.c
>> +++ b/xen/arch/arm/tee/ffa_shm.c
>> @@ -469,6 +469,7 @@ void ffa_handle_mem_share(struct cpu_user_regs *regs)
>>     int ret = FFA_RET_DENIED;
>>     uint32_t range_count;
>>     uint32_t region_offs;
>> +    uint16_t dst_id;
>> 
>>     if ( !ffa_fw_supports_fid(FFA_MEM_SHARE_64) )
>>     {
>> @@ -537,6 +538,15 @@ void ffa_handle_mem_share(struct cpu_user_regs *regs)
>>         goto out_unlock;
>> 
>>     mem_access = ctx->tx + trans.mem_access_offs;
>> +
>> +    dst_id = ACCESS_ONCE(mem_access->access_perm.endpoint_id);
>> +    if ( !FFA_ID_IS_SECURE(dst_id) )
>> +    {
>> +        /* we do not support sharing with VMs */
>> +        ret = FFA_RET_NOT_SUPPORTED;
>> +        goto out_unlock;
>> +    }
>> +
>>     if ( ACCESS_ONCE(mem_access->access_perm.perm) != FFA_MEM_ACC_RW )
>>     {
>>         ret = FFA_RET_NOT_SUPPORTED;
>> @@ -567,7 +577,7 @@ void ffa_handle_mem_share(struct cpu_user_regs *regs)
>>         goto out_unlock;
>>     }
>>     shm->sender_id = trans.sender_id;
>> -    shm->ep_id = ACCESS_ONCE(mem_access->access_perm.endpoint_id);
>> +    shm->ep_id = dst_id;
>> 
>>     /*
>>      * Check that the Composite memory region descriptor fits.
>> --
>> 2.47.0