[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [RFC PATCH 4/4] xen/arm: ffa: Enable VM to VM without firmware
Hi Bertrand, On Mon, Nov 4, 2024 at 8:27 AM Bertrand Marquis <Bertrand.Marquis@xxxxxxx> wrote: > > Hi Jens, > > > On 1 Nov 2024, at 11:44, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote: > > > > Hi Bertrand, > > > > On Wed, Oct 16, 2024 at 11:22 AM Bertrand Marquis > > <bertrand.marquis@xxxxxxx> wrote: > >> > >> When VM to VM support is activated and there is no suitable FF-A support > >> in the firmware, enable FF-A support for VMs to allow using it for VM to > >> VM communications. > >> If there is Optee running in the secure world and using the non FF-A > >> communication system, having CONFIG_FFA_VM_TO_VM could be non functional > >> (if optee is probed first) or Optee could be non functional (if FF-A is > >> probed first) so it is not recommended to activate the configuration > >> option for such systems. > >> > >> To make buffer full notification work between VMs when there is not > >> firmware, rework the notification handling and modify the global flag to > >> only be used as check for firmware notification support instead. > >> > >> Modify part_info_get to return the list of VMs when there is no firmware > >> support. > >> > >> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx> > >> --- > >> xen/arch/arm/tee/ffa.c | 11 +++ > >> xen/arch/arm/tee/ffa_notif.c | 118 ++++++++++++++++---------------- > >> xen/arch/arm/tee/ffa_partinfo.c | 2 + > >> 3 files changed, 73 insertions(+), 58 deletions(-) > > > > I think it is desirable or at least a good goal to be able to have all > > TEE configurations enabled at compile time. > > > > For optee_probe() to succeed, a DT node with compatible > > "linaro,optee-tz" must be present, and a trusted OS responding with > > the UUID used by OP-TEE. False positives can be ruled out unless the > > system is grossly misconfigured and shouldn't be used. > > > > If we could make the probe order deterministic with OP-TEE before > > FF-A, we should be OK. If there is an odd system with OP-TEE SMC ABI > > in the secure world that wants to use FF-A VM to VM, removing the > > "linaro,optee-tz" compatible node from DT is enough to disable > > optee_probe() without recompiling Xen. > > I do agree with the deterministic argument but I am not sure having > the order forced is the right solution. CONFIG_FFA_VM_TO_VM ensures that FF-A probing always succeeds and claims the TEE configuration. Logically, this should be last after all other probing has been tried since it disables further probing. > > Maybe we could use a command line argument so that one could > select explicitly the tee: > tee=ffa / tee=optee > > If we could prevent to modify the device tree that will probably make > things easier. > > What do you think ? That works for me. Cheers, Jens > > Cheers > Bertrand > > > > > Cheers, > > Jens > > > >> > >> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c > >> index 21d41b452dc9..6d427864f3da 100644 > >> --- a/xen/arch/arm/tee/ffa.c > >> +++ b/xen/arch/arm/tee/ffa.c > >> @@ -324,8 +324,11 @@ static int ffa_domain_init(struct domain *d) > >> struct ffa_ctx *ctx; > >> int ret; > >> > >> +#ifndef CONFIG_FFA_VM_TO_VM > >> if ( !ffa_fw_version ) > >> return -ENODEV; > >> +#endif > >> + > >> /* > >> * 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 > >> @@ -549,7 +552,15 @@ err_no_fw: > >> bitmap_zero(ffa_fw_abi_supported, FFA_ABI_BITMAP_SIZE); > >> printk(XENLOG_WARNING "ARM FF-A No firmware support\n"); > >> > >> +#ifdef CONFIG_FFA_VM_TO_VM > >> + INIT_LIST_HEAD(&ffa_teardown_head); > >> + init_timer(&ffa_teardown_timer, ffa_teardown_timer_callback, NULL, 0); > >> + > >> + printk(XENLOG_INFO "ARM FF-A only available between VMs\n"); > >> + return true; > >> +#else > >> return false; > >> +#endif > >> } > >> > >> 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 052b3e364a70..f2c87d1320de 100644 > >> --- a/xen/arch/arm/tee/ffa_notif.c > >> +++ b/xen/arch/arm/tee/ffa_notif.c > >> @@ -16,7 +16,7 @@ > >> > >> #include "ffa_private.h" > >> > >> -static bool __ro_after_init notif_enabled; > >> +static bool __ro_after_init fw_notif_enabled; > >> static unsigned int __ro_after_init notif_sri_irq; > >> > >> int ffa_handle_notification_bind(struct cpu_user_regs *regs) > >> @@ -27,21 +27,17 @@ int ffa_handle_notification_bind(struct cpu_user_regs > >> *regs) > >> uint32_t bitmap_lo = get_user_reg(regs, 3); > >> uint32_t bitmap_hi = get_user_reg(regs, 4); > >> > >> - if ( !notif_enabled ) > >> - return FFA_RET_NOT_SUPPORTED; > >> - > >> if ( (src_dst & 0xFFFFU) != ffa_get_vm_id(d) ) > >> return FFA_RET_INVALID_PARAMETERS; > >> > >> if ( flags ) /* Only global notifications are supported */ > >> return FFA_RET_DENIED; > >> > >> - /* > >> - * We only support notifications from SP so no need to check the > >> sender > >> - * endpoint ID, the SPMC will take care of that for us. > >> - */ > >> - return ffa_simple_call(FFA_NOTIFICATION_BIND, src_dst, flags, > >> bitmap_hi, > >> - bitmap_lo); > >> + if ( FFA_ID_IS_SECURE(src_dst>>16) && fw_notif_enabled ) > >> + return ffa_simple_call(FFA_NOTIFICATION_BIND, src_dst, flags, > >> + bitmap_hi, bitmap_lo); > >> + > >> + return FFA_RET_NOT_SUPPORTED; > >> } > >> > >> int ffa_handle_notification_unbind(struct cpu_user_regs *regs) > >> @@ -51,32 +47,36 @@ int ffa_handle_notification_unbind(struct > >> cpu_user_regs *regs) > >> uint32_t bitmap_lo = get_user_reg(regs, 3); > >> uint32_t bitmap_hi = get_user_reg(regs, 4); > >> > >> - if ( !notif_enabled ) > >> - return FFA_RET_NOT_SUPPORTED; > >> - > >> if ( (src_dst & 0xFFFFU) != ffa_get_vm_id(d) ) > >> return FFA_RET_INVALID_PARAMETERS; > >> > >> - /* > >> - * We only support notifications from SP so no need to check the > >> - * destination endpoint ID, the SPMC will take care of that for us. > >> - */ > >> - return ffa_simple_call(FFA_NOTIFICATION_UNBIND, src_dst, 0, > >> bitmap_hi, > >> - bitmap_lo); > >> + if ( FFA_ID_IS_SECURE(src_dst>>16) && fw_notif_enabled ) > >> + return ffa_simple_call(FFA_NOTIFICATION_UNBIND, src_dst, 0, > >> bitmap_hi, > >> + bitmap_lo); > >> + > >> + return FFA_RET_NOT_SUPPORTED; > >> } > >> > >> void ffa_handle_notification_info_get(struct cpu_user_regs *regs) > >> { > >> struct domain *d = current->domain; > >> struct ffa_ctx *ctx = d->arch.tee; > >> + bool notif_pending = false; > >> > >> - if ( !notif_enabled ) > >> +#ifndef CONFIG_FFA_VM_TO_VM > >> + if ( !fw_notif_enabled ) > >> { > >> ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED); > >> return; > >> } > >> +#endif > >> > >> - if ( test_and_clear_bool(ctx->notif.secure_pending) ) > >> + notif_pending = ctx->notif.secure_pending; > >> +#ifdef CONFIG_FFA_VM_TO_VM > >> + notif_pending |= ctx->notif.buff_full_pending; > >> +#endif > >> + > >> + if ( notif_pending ) > >> { > >> /* A pending global notification for the guest */ > >> ffa_set_regs(regs, FFA_SUCCESS_64, 0, > >> @@ -103,11 +103,13 @@ void ffa_handle_notification_get(struct > >> cpu_user_regs *regs) > >> uint32_t w6 = 0; > >> uint32_t w7 = 0; > >> > >> - if ( !notif_enabled ) > >> +#ifndef CONFIG_FFA_VM_TO_VM > >> + if ( !fw_notif_enabled ) > >> { > >> ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED); > >> return; > >> } > >> +#endif > >> > >> if ( (recv & 0xFFFFU) != ffa_get_vm_id(d) ) > >> { > >> @@ -115,7 +117,8 @@ void ffa_handle_notification_get(struct cpu_user_regs > >> *regs) > >> return; > >> } > >> > >> - if ( flags & ( FFA_NOTIF_FLAG_BITMAP_SP | FFA_NOTIF_FLAG_BITMAP_SPM ) > >> ) > >> + if ( fw_notif_enabled && (flags & ( FFA_NOTIF_FLAG_BITMAP_SP > >> + | FFA_NOTIF_FLAG_BITMAP_SPM )) ) > >> { > >> struct arm_smccc_1_2_regs arg = { > >> .a0 = FFA_NOTIFICATION_GET, > >> @@ -170,15 +173,14 @@ int ffa_handle_notification_set(struct cpu_user_regs > >> *regs) > >> uint32_t bitmap_lo = get_user_reg(regs, 3); > >> uint32_t bitmap_hi = get_user_reg(regs, 4); > >> > >> - if ( !notif_enabled ) > >> - return FFA_RET_NOT_SUPPORTED; > >> - > >> if ( (src_dst >> 16) != ffa_get_vm_id(d) ) > >> return FFA_RET_INVALID_PARAMETERS; > >> > >> - /* Let the SPMC check the destination of the notification */ > >> - return ffa_simple_call(FFA_NOTIFICATION_SET, src_dst, flags, > >> bitmap_lo, > >> - bitmap_hi); > >> + if ( FFA_ID_IS_SECURE(src_dst>>16) && fw_notif_enabled ) > >> + return ffa_simple_call(FFA_NOTIFICATION_SET, src_dst, flags, > >> bitmap_lo, > >> + bitmap_hi); > >> + > >> + return FFA_RET_NOT_SUPPORTED; > >> } > >> > >> #ifdef CONFIG_FFA_VM_TO_VM > >> @@ -190,7 +192,7 @@ void ffa_raise_rx_buffer_full(struct domain *d) > >> return; > >> > >> if ( !test_and_set_bool(ctx->notif.buff_full_pending) ) > >> - vgic_inject_irq(d, d->vcpu[0], notif_sri_irq, true); > >> + vgic_inject_irq(d, d->vcpu[0], GUEST_FFA_NOTIF_PEND_INTR_ID, > >> true); > >> } > >> #endif > >> > >> @@ -363,7 +365,7 @@ void ffa_notif_init_interrupt(void) > >> { > >> int ret; > >> > >> - if ( notif_enabled && notif_sri_irq < NR_GIC_SGI ) > >> + if ( fw_notif_enabled && notif_sri_irq < NR_GIC_SGI ) > >> { > >> /* > >> * An error here is unlikely since the primary CPU has already > >> @@ -394,47 +396,47 @@ void ffa_notif_init(void) > >> 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; > >> - > >> - irq = resp.a2; > >> - notif_sri_irq = irq; > >> - if ( irq >= NR_GIC_SGI ) > >> - irq_set_type(irq, IRQ_TYPE_EDGE_RISING); > >> - ret = request_irq(irq, 0, notif_irq_handler, "FF-A notif", NULL); > >> - if ( ret ) > >> + 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) ) > >> { > >> - printk(XENLOG_ERR "ffa: request_irq irq %u failed: error %d\n", > >> - irq, ret); > >> - return; > >> - } > >> + arm_smccc_1_2_smc(&arg, &resp); > >> + if ( resp.a0 != FFA_SUCCESS_32 ) > >> + return; > >> > >> - notif_enabled = true; > >> + irq = resp.a2; > >> + notif_sri_irq = irq; > >> + if ( irq >= NR_GIC_SGI ) > >> + irq_set_type(irq, IRQ_TYPE_EDGE_RISING); > >> + ret = request_irq(irq, 0, notif_irq_handler, "FF-A notif", NULL); > >> + if ( ret ) > >> + { > >> + printk(XENLOG_ERR "ffa: request_irq irq %u failed: error > >> %d\n", > >> + irq, ret); > >> + return; > >> + } > >> + fw_notif_enabled = true; > >> + } > >> } > >> > >> int ffa_notif_domain_init(struct domain *d) > >> { > >> int32_t res; > >> > >> - if ( !notif_enabled ) > >> - return 0; > >> + if ( fw_notif_enabled ) > >> + { > >> > >> - res = ffa_notification_bitmap_create(ffa_get_vm_id(d), d->max_vcpus); > >> - if ( res ) > >> - return -ENOMEM; > >> + res = ffa_notification_bitmap_create(ffa_get_vm_id(d), > >> d->max_vcpus); > >> + if ( res ) > >> + return -ENOMEM; > >> + } > >> > >> return 0; > >> } > >> > >> void ffa_notif_domain_destroy(struct domain *d) > >> { > >> - if ( notif_enabled ) > >> + if ( fw_notif_enabled ) > >> ffa_notification_bitmap_destroy(ffa_get_vm_id(d)); > >> } > >> diff --git a/xen/arch/arm/tee/ffa_partinfo.c > >> b/xen/arch/arm/tee/ffa_partinfo.c > >> index d699a267cc76..2e09440fe6c2 100644 > >> --- a/xen/arch/arm/tee/ffa_partinfo.c > >> +++ b/xen/arch/arm/tee/ffa_partinfo.c > >> @@ -128,12 +128,14 @@ void ffa_handle_partition_info_get(struct > >> cpu_user_regs *regs) > >> goto out; > >> } > >> > >> +#ifndef CONFIG_FFA_VM_TO_VM > >> if ( !ffa_fw_supports_fid(FFA_PARTITION_INFO_GET) ) > >> { > >> /* Just give an empty partition list to the caller */ > >> ret = FFA_RET_OK; > >> goto out; > >> } > >> +#endif > >> > >> ret = ffa_rx_acquire(d); > >> if ( ret != FFA_RET_OK ) > >> -- > >> 2.47.0 > >
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |