|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [XEN PATCH v7 02/20] xen/arm: tee: add a primitive FF-A mediator
Hi, On Thu, Feb 23, 2023 at 3:47 PM Bertrand Marquis <Bertrand.Marquis@xxxxxxx> wrote: > > Hi Jens, > > > On 22 Feb 2023, at 16:32, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote: > > > > Adds a FF-A version 1.1 [1] mediator to communicate with a Secure > > Partition in secure world. > > > > This commit brings in only the parts needed to negotiate FF-A version > > number with guest and SPMC. > > > > [1] https://developer.arm.com/documentation/den0077/e > > Signed-off-by: Jens Wiklander <jens.wiklander@xxxxxxxxxx> > > Just some minor comments from my side after. > > > --- > > xen/arch/arm/include/asm/domain.h | 2 +- > > xen/arch/arm/include/asm/psci.h | 4 + > > xen/arch/arm/include/asm/tee/ffa.h | 35 +++++ > > xen/arch/arm/tee/Kconfig | 11 ++ > > xen/arch/arm/tee/Makefile | 1 + > > xen/arch/arm/tee/ffa.c | 217 +++++++++++++++++++++++++++++ > > xen/arch/arm/vsmc.c | 17 ++- > > xen/include/public/arch-arm.h | 1 + > > 8 files changed, 284 insertions(+), 4 deletions(-) > > create mode 100644 xen/arch/arm/include/asm/tee/ffa.h > > create mode 100644 xen/arch/arm/tee/ffa.c > > > > diff --git a/xen/arch/arm/include/asm/domain.h > > b/xen/arch/arm/include/asm/domain.h > > index 0e310601e846..754daa8efa04 100644 > > --- a/xen/arch/arm/include/asm/domain.h > > +++ b/xen/arch/arm/include/asm/domain.h > > @@ -110,7 +110,7 @@ struct arch_domain > > struct vpl011 vpl011; > > #endif > > > > -#ifdef CONFIG_TEE > > +#if defined(CONFIG_TEE) || defined(CONFIG_FFA) > > void *tee; > > #endif > > > > diff --git a/xen/arch/arm/include/asm/psci.h > > b/xen/arch/arm/include/asm/psci.h > > index 832f77afff3a..4780972621bb 100644 > > --- a/xen/arch/arm/include/asm/psci.h > > +++ b/xen/arch/arm/include/asm/psci.h > > @@ -24,6 +24,10 @@ void call_psci_cpu_off(void); > > void call_psci_system_off(void); > > void call_psci_system_reset(void); > > > > +/* Range of allocated PSCI function numbers */ > > +#define PSCI_FNUM_MIN_VALUE _AC(0,U) > > +#define PSCI_FNUM_MAX_VALUE _AC(0x1f,U) > > + > > /* PSCI v0.2 interface */ > > #define PSCI_0_2_FN32(nr) ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, > > \ > > ARM_SMCCC_CONV_32, > > \ > > diff --git a/xen/arch/arm/include/asm/tee/ffa.h > > b/xen/arch/arm/include/asm/tee/ffa.h > > new file mode 100644 > > index 000000000000..94960100718e > > --- /dev/null > > +++ b/xen/arch/arm/include/asm/tee/ffa.h > > @@ -0,0 +1,35 @@ > > +/* SPDX-License-Identifier: MIT */ > > Why is this file in MIT license ? > > Any chance to set it as BSD-2 or maybe no license at all (most private > headers do not have one) ? I'll change it to GPL-2.0-only. > > > +/* > > + * xen/arch/arm/include/asm/tee/ffa.h > > + * > > + * Arm Firmware Framework for ARMv8-A(FFA) mediator > > + * > > + * Copyright (C) 2023 Linaro Limited > > + */ > > + > > +#ifndef __ASM_ARM_TEE_FFA_H__ > > +#define __ASM_ARM_TEE_FFA_H__ > > + > > +#include <xen/const.h> > > +#include <xen/kconfig.h> > > + > > +#include <asm/smccc.h> > > +#include <asm/types.h> > > + > > +#define FFA_FNUM_MIN_VALUE _AC(0x60,U) > > +#define FFA_FNUM_MAX_VALUE _AC(0x86,U) > > + > > +static inline bool is_ffa_fid(uint32_t fid) > > +{ > > + uint32_t fn = fid & ARM_SMCCC_FUNC_MASK; > > + > > + return fn >= FFA_FNUM_MIN_VALUE && fn <= FFA_FNUM_MAX_VALUE; > > +} > > + > > +#ifdef CONFIG_FFA > > +#define FFA_NR_FUNCS (9 + (IS_ENABLED(CONFIG_ARM_64) ? 3 : 0) ) > > I am not quite sure of the usage of IS_ENABLED for a constant > > Maybe just doing: > #ifdef CONFIG_FFA > #ifdef CONFIG_ARM_64 > #define FFA_NR_FUNCS 12 > #else > #define FFA_NR_FUNCS 9 > #endif > #else > #define FFA_NR_FUNCS 0 > #endif OK, I'll change it. > > > +#else > > +#define FFA_NR_FUNCS 0 > > +#endif > > + > > +#endif /*__ASM_ARM_TEE_FFA_H__*/ > > diff --git a/xen/arch/arm/tee/Kconfig b/xen/arch/arm/tee/Kconfig > > index 392169b2559d..923f08ba8cb7 100644 > > --- a/xen/arch/arm/tee/Kconfig > > +++ b/xen/arch/arm/tee/Kconfig > > @@ -8,3 +8,14 @@ config OPTEE > > virtualization-enabled OP-TEE present. You can learn more > > about virtualization for OP-TEE at > > https://optee.readthedocs.io/architecture/virtualization.html > > + > > +config FFA > > + bool "Enable FF-A mediator support (UNSUPPORTED)" if UNSUPPORTED > > + default n > > + depends on ARM_64 > > + help > > + This option enables a minimal FF-A mediator. The mediator is > > + generic as it follows the FF-A specification [1], but it only > > + implements a small subset of the specification. > > + > > + [1] https://developer.arm.com/documentation/den0077/latest > > diff --git a/xen/arch/arm/tee/Makefile b/xen/arch/arm/tee/Makefile > > index 982c87968447..58a1015e40e0 100644 > > --- a/xen/arch/arm/tee/Makefile > > +++ b/xen/arch/arm/tee/Makefile > > @@ -1,2 +1,3 @@ > > +obj-$(CONFIG_FFA) += ffa.o > > obj-y += tee.o > > obj-$(CONFIG_OPTEE) += optee.o > > diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c > > new file mode 100644 > > index 000000000000..824153c9303a > > --- /dev/null > > +++ b/xen/arch/arm/tee/ffa.c > > @@ -0,0 +1,217 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > I think now you have to say either GPL-2.0-only or GPL-2.0-or-later OK, I'll change it to GPL-2.0-only > > > +/* > > + * xen/arch/arm/tee/ffa.c > > + * > > + * Arm Firmware Framework for ARMv8-A (FF-A) mediator > > + * > > + * Copyright (C) 2023 Linaro Limited > > + */ > > + > > +#include <xen/domain_page.h> > > +#include <xen/errno.h> > > +#include <xen/init.h> > > +#include <xen/lib.h> > > +#include <xen/sched.h> > > +#include <xen/types.h> > > +#include <xen/sizes.h> > > +#include <xen/bitops.h> > > Please try to put the headers in alphabetical order. > > > + > > +#include <asm/smccc.h> > > +#include <asm/event.h> > > +#include <asm/tee/tee.h> > > +#include <asm/tee/ffa.h> > > +#include <asm/regs.h> > > Same here I'll fix it. > > The rest looks ok to me. Thanks, Jens > > Cheers > Bertrand > > > + > > +/* Error codes */ > > +#define FFA_RET_OK 0 > > +#define FFA_RET_NOT_SUPPORTED -1 > > +#define FFA_RET_INVALID_PARAMETERS -2 > > +#define FFA_RET_NO_MEMORY -3 > > +#define FFA_RET_BUSY -4 > > +#define FFA_RET_INTERRUPTED -5 > > +#define FFA_RET_DENIED -6 > > +#define FFA_RET_RETRY -7 > > +#define FFA_RET_ABORTED -8 > > + > > +/* FFA_VERSION helpers */ > > +#define FFA_VERSION_MAJOR_SHIFT 16U > > +#define FFA_VERSION_MAJOR_MASK 0x7FFFU > > +#define FFA_VERSION_MINOR_SHIFT 0U > > +#define FFA_VERSION_MINOR_MASK 0xFFFFU > > +#define MAKE_FFA_VERSION(major, minor) \ > > + ((((major) & FFA_VERSION_MAJOR_MASK) << FFA_VERSION_MAJOR_SHIFT) | > > \ > > + ((minor) & FFA_VERSION_MINOR_MASK)) > > + > > +#define FFA_MIN_VERSION MAKE_FFA_VERSION(1, 0) > > +#define FFA_VERSION_1_0 MAKE_FFA_VERSION(1, 0) > > +#define FFA_VERSION_1_1 MAKE_FFA_VERSION(1, 1) > > + > > +/* > > + * This is the version we want to use in communication with guests and SPs. > > + * During negotiation with a guest or a SP we may need to lower it for > > + * that particular guest or SP. > > + */ > > +#define FFA_MY_VERSION_MAJOR 1U > > +#define FFA_MY_VERSION_MINOR 1U > > +#define FFA_MY_VERSION MAKE_FFA_VERSION(FFA_MY_VERSION_MAJOR, \ > > + FFA_MY_VERSION_MINOR) > > + > > +/* Function IDs */ > > +#define FFA_ERROR 0x84000060U > > +#define FFA_SUCCESS_32 0x84000061U > > +#define FFA_VERSION 0x84000063U > > + > > +struct ffa_ctx { > > + uint32_t guest_vers; > > +}; > > + > > +/* Negotiated FF-A version to use with the SPMC */ > > +static uint32_t ffa_version __ro_after_init; > > + > > +static bool ffa_get_version(uint32_t *vers) > > +{ > > + const struct arm_smccc_1_2_regs arg = { > > + .a0 = FFA_VERSION, > > + .a1 = FFA_MY_VERSION, > > + }; > > + struct arm_smccc_1_2_regs resp; > > + > > + arm_smccc_1_2_smc(&arg, &resp); > > + if ( resp.a0 == FFA_RET_NOT_SUPPORTED ) > > + { > > + gprintk(XENLOG_ERR, "ffa: FFA_VERSION returned not supported\n"); > > + return false; > > + } > > + > > + *vers = resp.a0; > > + > > + return true; > > +} > > + > > +static void set_regs(struct cpu_user_regs *regs, register_t v0, register_t > > v1, > > + register_t v2, register_t v3, register_t v4, > > register_t v5, > > + register_t v6, register_t v7) > > +{ > > + set_user_reg(regs, 0, v0); > > + set_user_reg(regs, 1, v1); > > + set_user_reg(regs, 2, v2); > > + set_user_reg(regs, 3, v3); > > + set_user_reg(regs, 4, v4); > > + set_user_reg(regs, 5, v5); > > + set_user_reg(regs, 6, v6); > > + set_user_reg(regs, 7, v7); > > +} > > + > > +static void handle_version(struct cpu_user_regs *regs) > > +{ > > + struct domain *d = current->domain; > > + struct ffa_ctx *ctx = d->arch.tee; > > + uint32_t vers = get_user_reg(regs, 1); > > + > > + if ( vers < FFA_VERSION_1_1 ) > > + vers = FFA_VERSION_1_0; > > + else > > + vers = FFA_VERSION_1_1; > > + > > + ctx->guest_vers = vers; > > + set_regs(regs, vers, 0, 0, 0, 0, 0, 0, 0); > > +} > > + > > +static bool ffa_handle_call(struct cpu_user_regs *regs) > > +{ > > + uint32_t fid = get_user_reg(regs, 0); > > + struct domain *d = current->domain; > > + struct ffa_ctx *ctx = d->arch.tee; > > + > > + if ( !ctx ) > > + return false; > > + > > + switch ( fid ) > > + { > > + case FFA_VERSION: > > + handle_version(regs); > > + return true; > > + > > + default: > > + gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid); > > + return false; > > + } > > +} > > + > > +static int ffa_domain_init(struct domain *d) > > +{ > > + struct ffa_ctx *ctx; > > + > > + if ( !ffa_version ) > > + return -ENODEV; > > + > > + ctx = xzalloc(struct ffa_ctx); > > + if ( !ctx ) > > + return -ENOMEM; > > + > > + d->arch.tee = ctx; > > + > > + return 0; > > +} > > + > > +/* This function is supposed to undo what ffa_domain_init() has done */ > > +static int ffa_relinquish_resources(struct domain *d) > > +{ > > + struct ffa_ctx *ctx = d->arch.tee; > > + > > + if ( !ctx ) > > + return 0; > > + > > + XFREE(d->arch.tee); > > + > > + return 0; > > +} > > + > > +static bool ffa_probe(void) > > +{ > > + uint32_t vers; > > + unsigned int major_vers; > > + unsigned int minor_vers; > > + > > + /* > > + * psci_init_smccc() updates this value with what's reported by EL-3 > > + * or secure world. > > + */ > > + if ( smccc_ver < ARM_SMCCC_VERSION_1_2 ) > > + { > > + printk(XENLOG_ERR > > + "ffa: unsupported SMCCC version %#x (need at least %#x)\n", > > + smccc_ver, ARM_SMCCC_VERSION_1_2); > > + return false; > > + } > > + > > + if ( !ffa_get_version(&vers) ) > > + return false; > > + > > + if ( vers < FFA_MIN_VERSION || vers > FFA_MY_VERSION ) > > + { > > + printk(XENLOG_ERR "ffa: Incompatible version %#x found\n", vers); > > + return false; > > + } > > + > > + major_vers = (vers >> FFA_VERSION_MAJOR_SHIFT) & > > FFA_VERSION_MAJOR_MASK; > > + minor_vers = vers & FFA_VERSION_MINOR_MASK; > > + printk(XENLOG_INFO "ARM FF-A Mediator version %u.%u\n", > > + FFA_MY_VERSION_MAJOR, FFA_MY_VERSION_MINOR); > > + printk(XENLOG_INFO "ARM FF-A Firmware version %u.%u\n", > > + major_vers, minor_vers); > > + > > + ffa_version = vers; > > + > > + return true; > > +} > > + > > +static const struct tee_mediator_ops ffa_ops = > > +{ > > + .probe = ffa_probe, > > + .domain_init = ffa_domain_init, > > + .relinquish_resources = ffa_relinquish_resources, > > + .handle_call = ffa_handle_call, > > +}; > > + > > +REGISTER_TEE_MEDIATOR(ffa, "FF-A", XEN_DOMCTL_CONFIG_TEE_FFA, &ffa_ops); > > diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c > > index cd68fa80e98a..7f2f5eb9ce3d 100644 > > --- a/xen/arch/arm/vsmc.c > > +++ b/xen/arch/arm/vsmc.c > > @@ -15,6 +15,7 @@ > > #include <asm/monitor.h> > > #include <asm/regs.h> > > #include <asm/smccc.h> > > +#include <asm/tee/ffa.h> > > #include <asm/tee/tee.h> > > #include <asm/traps.h> > > #include <asm/vpsci.h> > > @@ -24,7 +25,7 @@ > > #define XEN_SMCCC_FUNCTION_COUNT 3 > > > > /* Number of functions currently supported by Standard Service Service > > Calls. */ > > -#define SSSC_SMCCC_FUNCTION_COUNT (3 + VPSCI_NR_FUNCS) > > +#define SSSC_SMCCC_FUNCTION_COUNT (3 + VPSCI_NR_FUNCS + FFA_NR_FUNCS) > > > > static bool fill_uid(struct cpu_user_regs *regs, xen_uuid_t uuid) > > { > > @@ -188,13 +189,23 @@ static bool handle_existing_apis(struct cpu_user_regs > > *regs) > > return do_vpsci_0_1_call(regs, fid); > > } > > > > +static bool is_psci_fid(uint32_t fid) > > +{ > > + uint32_t fn = fid & ARM_SMCCC_FUNC_MASK; > > + > > + return fn >= PSCI_FNUM_MIN_VALUE && fn <= PSCI_FNUM_MAX_VALUE; > > +} > > + > > /* PSCI 0.2 interface and other Standard Secure Calls */ > > static bool handle_sssc(struct cpu_user_regs *regs) > > { > > uint32_t fid = (uint32_t)get_user_reg(regs, 0); > > > > - if ( do_vpsci_0_2_call(regs, fid) ) > > - return true; > > + if ( is_psci_fid(fid) ) > > + return do_vpsci_0_2_call(regs, fid); > > + > > + if ( is_ffa_fid(fid) ) > > + return tee_handle_call(regs); > > > > switch ( fid ) > > { > > diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h > > index 1528ced5097a..92aff923056a 100644 > > --- a/xen/include/public/arch-arm.h > > +++ b/xen/include/public/arch-arm.h > > @@ -296,6 +296,7 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_context_t); > > > > #define XEN_DOMCTL_CONFIG_TEE_NONE 0 > > #define XEN_DOMCTL_CONFIG_TEE_OPTEE 1 > > +#define XEN_DOMCTL_CONFIG_TEE_FFA 2 > > > > struct xen_arch_domainconfig { > > /* IN/OUT */ > > -- > > 2.34.1 > > >
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |