| Message ID | dec1c02aa0142263796c53b8df917fa25e2d2675.1729066788.git.bertrand.marquis@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xen/arm: ffa: Improvements and fixes | expand |
Hi Bertrand, On Wed, Oct 16, 2024 at 10:32 AM Bertrand Marquis <bertrand.marquis@arm.com> 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@arm.com> > --- > 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 >
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; 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); + 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.
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@arm.com> --- 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(-)