|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 06/10] xen/arm: ffa: Use bit 15 convention for SPs
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?
> 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?
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
>
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |