| Message ID | 1507748484-16871-3-git-send-email-volodymyr_babchuk@epam.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Volodymyr, On 11/10/17 20:01, Volodymyr Babchuk wrote: > This patch adds basic framework for TEE mediators. Guests can't talk > to TEE directly, we need some entity that will intercept request > and decide what to do with them. "TEE mediaor" is a such entity. s/mediaor/mediator/ > > This is how it works: user can build XEN with multiple TEE mediators > (see the next patches, where OP-TEE mediator is introduced). > TEE mediator register self with REGISTER_TEE_MEDIATOR() macro in the > same way, as device drivers use DT_DEVICE_START()/DT_DEVICE_END() > macros. > In runtime, during initialization, XEN issues standard SMC to read > TEE UID. Using this UID it selects and initializes one of built-in > mediators. Then generic vSMC handler will call selected mediator > when it intercept SMC that belongs to TEE OS or TEE application. As you may remember the discussion on the SMCCC support for guests, there are currently no way to know the SMCCC is present on the platform. I don't think you can rely on the platform support SMCC nor fully implementing it. This also bring the question of does every TEE are supporting SMCCC? > > Also, there are hooks for domain construction and destruction, so > TEE mediator can inform TEE about VM lifecycle. > > Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> > --- > MAINTAINERS | 5 ++ > xen/arch/arm/Kconfig | 10 ++++ > xen/arch/arm/Makefile | 1 + > xen/arch/arm/domain.c | 7 +++ > xen/arch/arm/setup.c | 4 ++ > xen/arch/arm/tee/Kconfig | 0 > xen/arch/arm/tee/Makefile | 1 + > xen/arch/arm/tee/tee.c | 134 ++++++++++++++++++++++++++++++++++++++++++++++ > xen/arch/arm/vsmc.c | 5 ++ > xen/arch/arm/xen.lds.S | 7 +++ > xen/include/asm-arm/tee.h | 79 +++++++++++++++++++++++++++ > 11 files changed, 253 insertions(+) > create mode 100644 xen/arch/arm/tee/Kconfig > create mode 100644 xen/arch/arm/tee/Makefile > create mode 100644 xen/arch/arm/tee/tee.c > create mode 100644 xen/include/asm-arm/tee.h > > diff --git a/MAINTAINERS b/MAINTAINERS > index 77b1e11..ede00c5 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -357,6 +357,11 @@ F: config/Stubdom.mk.in > F: m4/stubdom.m4 > F: stubdom/ > > +TEE MEDIATORS > +M: Volodymyr Babchuk <volodymyr_babchuk@epam.com> > +S: Supported > +F: xen/arch/arm/tee/* > + > TOOLSTACK > M: Ian Jackson <ian.jackson@eu.citrix.com> > M: Wei Liu <wei.liu2@citrix.com> > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig > index d46b98c..e1f112a 100644 > --- a/xen/arch/arm/Kconfig > +++ b/xen/arch/arm/Kconfig > @@ -50,6 +50,14 @@ config HAS_ITS > prompt "GICv3 ITS MSI controller support" if EXPERT = "y" > depends on HAS_GICV3 > > +config ARM_TEE The ARM in the title is a bit pointless. This Kconfig is only used for Arm architecture. > + bool "Enable TEE mediators support" > + default n > + depends on ARM No need for that. > + help > + This option enables generic TEE mediators support. It allows guests > + to access real TEE via one of TEE mediators implemented in XEN Missing full stop. > + > endmenu > > menu "ARM errata workaround via the alternative framework" > @@ -167,3 +175,5 @@ endmenu > source "common/Kconfig" > > source "drivers/Kconfig" > + > +source "arch/arm/tee/Kconfig" > diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile > index ede21fd..2710e0e 100644 > --- a/xen/arch/arm/Makefile > +++ b/xen/arch/arm/Makefile > @@ -3,6 +3,7 @@ subdir-$(CONFIG_ARM_64) += arm64 > subdir-y += platforms > subdir-$(CONFIG_ARM_64) += efi > subdir-$(CONFIG_ACPI) += acpi > +subdir-$(CONFIG_ARM_TEE) += tee > > obj-$(CONFIG_HAS_ALTERNATIVE) += alternative.o > obj-y += bootfdt.init.o > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c > index 784ae39..3290d39 100644 > --- a/xen/arch/arm/domain.c > +++ b/xen/arch/arm/domain.c > @@ -31,6 +31,7 @@ > #include <asm/platform.h> > #include <asm/procinfo.h> > #include <asm/regs.h> > +#include <asm/tee.h> > #include <asm/vfp.h> > #include <asm/vgic.h> > #include <asm/vtimer.h> > @@ -673,6 +674,9 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags, > if ( is_hardware_domain(d) && (rc = domain_vuart_init(d)) ) > goto fail; > > + /* Notify TEE that new domain was created */ > + tee_domain_create(d); I am not a big fan to see this in arch_domain_create until we see how this is going to fit with guest. For instance, will TEE be for every guests? What would be the other necessary information to configure it?... > + > return 0; > > fail: > @@ -684,6 +688,9 @@ fail: > > void arch_domain_destroy(struct domain *d) > { > + /* Notify TEE that domain is being destroyed */ > + tee_domain_destroy(d); > + > /* IOMMU page table is shared with P2M, always call > * iommu_domain_destroy() before p2m_teardown(). > */ > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > index 92f173b..8a4fcd8 100644 > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -47,6 +47,7 @@ > #include <asm/platform.h> > #include <asm/procinfo.h> > #include <asm/setup.h> > +#include <asm/tee.h> > #include <xsm/xsm.h> > #include <asm/acpi.h> > > @@ -846,6 +847,9 @@ void __init start_xen(unsigned long boot_phys_offset, > */ > apply_alternatives_all(); > > + /* Initialize TEE mediator */ > + tee_init(); > + > /* Create initial domain 0. */ > /* The vGIC for DOM0 is exactly emulating the hardware GIC */ > config.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE; > diff --git a/xen/arch/arm/tee/Kconfig b/xen/arch/arm/tee/Kconfig > new file mode 100644 > index 0000000..e69de29 > diff --git a/xen/arch/arm/tee/Makefile b/xen/arch/arm/tee/Makefile > new file mode 100644 > index 0000000..c54d479 > --- /dev/null > +++ b/xen/arch/arm/tee/Makefile > @@ -0,0 +1 @@ > +obj-y += tee.o > diff --git a/xen/arch/arm/tee/tee.c b/xen/arch/arm/tee/tee.c > new file mode 100644 > index 0000000..7f7a846 > --- /dev/null > +++ b/xen/arch/arm/tee/tee.c > @@ -0,0 +1,134 @@ > +/* > + * xen/arch/arm/tee/tee.c > + * > + * Generic part of TEE mediator subsystem > + * > + * Volodymyr Babchuk <volodymyr_babchuk@epam.com> > + * Copyright (c) 2017 EPAM Systems. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include <xen/types.h> > +#include <asm/smccc.h> > +#include <asm/tee.h> > + > +/* > + * According to ARM SMCCC (ARM DEN 0028B, page 17), service owner > + * for generic TEE queries is 63. > + */ > +#define TRUSTED_OS_GENERIC_API_OWNER 63 > + > +#define ARM_SMCCC_FUNC_GET_TEE_UID \ > + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \ > + ARM_SMCCC_CONV_32, \ > + TRUSTED_OS_GENERIC_API_OWNER, \ > + ARM_SMCCC_FUNC_CALL_UID) This likely needs to be defined in smccc as AFAIU it is part of the SMCCC. > + > +extern const struct tee_mediator_desc _steemediator[], _eteemediator[]; > +static const struct tee_mediator_ops *mediator_ops; > + > +/* Helper function to read UID returned by SMC */ > +static void parse_uid(const register_t regs[4], xen_uuid_t *uid) > +{ > + uint8_t *bytes = uid->a; > + int n; unsigned int please. > + > + /* > + * UID is returned in registers r0..r3, four bytes per register, > + * first byte is stored in low-order bits of a register. > + * (ARM DEN 0028B page 14) > + */ > + for (n = 0; n < 16; n++) for ( n = 0; ... ) > + bytes[n] = (uint8_t)(regs[n/4] >> ((n & 3) * 8)); > + > +} > + > +void tee_init(void) > +{ > + const struct tee_mediator_desc *desc; > + register_t resp[4]; > + xen_uuid_t tee_uid; > + int ret; > + > + /* Read UUID to determine which TEE is running */ You can't assume the platform is supporting SMCCC. See my comment at the beginning of the e-mail. > + call_smccc_smc(ARM_SMCCC_FUNC_GET_TEE_UID, 0, 0, 0, 0, 0, 0, 0, resp); > + if ( resp[0] == 0xFFFFFFFF ) { You likely want to use ARM_SMCCC_ERR_UNKOWN_FUNCTION here. > + printk("No TEE found\n"); > + return; > + } > + > + parse_uid(resp, &tee_uid); > + > + printk("TEE UID: %02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x\n", > + tee_uid.a[0 ], tee_uid.a[1 ], tee_uid.a[2 ], tee_uid.a[3 ], Please no space before ]. This is making more confusing to read. > + tee_uid.a[4 ], tee_uid.a[5 ], tee_uid.a[6 ], tee_uid.a[7 ], > + tee_uid.a[8 ], tee_uid.a[9 ], tee_uid.a[10], tee_uid.a[11], > + tee_uid.a[12], tee_uid.a[13], tee_uid.a[14], tee_uid.a[15]); > + > + for ( desc = _steemediator; desc != _eteemediator; desc++ ) { > + if ( memcmp(&desc->uid, &tee_uid, sizeof(xen_uuid_t)) == 0 ) > + { > + printk("Using TEE mediator for %sp\n", desc->name); > + mediator_ops = desc->ops; > + break; > + } } > + > + if ( !mediator_ops ) A warning here would be useful. > + return; > + > + ret = mediator_ops->init(); > + if ( ret ) > + { > + printk("TEE mediator failed to initialize :%d\n", ret); > + mediator_ops = NULL; > + } > +} > + > +bool tee_handle_smc(struct cpu_user_regs *regs) > +{ > + if ( !mediator_ops ) > + return false; > + > + return mediator_ops->handle_smc(regs); > +} > + > +void tee_domain_create(struct domain *d) > +{ > + if ( !mediator_ops ) > + return; > + > + return mediator_ops->domain_create(d); return here is not necessary. However, I am slightly surprised that tee_domain_create could never fail. > +} > + > +void tee_domain_destroy(struct domain *d) > +{ > + if ( !mediator_ops ) > + return; > + > + return mediator_ops->domain_destroy(d); Same here. > +} > + > +void tee_remove(void) What is this callback for? I don't see any use within this series. > +{ > + if ( !mediator_ops ) > + return; > + > + return mediator_ops->remove(); > +} > + > +/* > + * Local variables: > + * mode: C > + * c-file-style: "BSD" > + * c-basic-offset: 4 > + * indent-tabs-mode: nil > + * End: > + */ > diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c > index 7bd6008..186e34b 100644 > --- a/xen/arch/arm/vsmc.c > +++ b/xen/arch/arm/vsmc.c > @@ -22,6 +22,7 @@ > #include <asm/psci.h> > #include <asm/regs.h> > #include <asm/smccc.h> > +#include <asm/tee.h> > #include <asm/traps.h> > > /* Number of functions currently supported by Hypervisor Service. */ > @@ -288,6 +289,10 @@ static bool vsmccc_handle_call(struct cpu_user_regs *regs) > case ARM_SMCCC_OWNER_STANDARD: > handled = handle_sssc(regs); > break; > + case ARM_SMCCC_OWNER_TRUSTED_APP ... ARM_SMCCC_OWNER_TRUSTED_APP_END: > + case ARM_SMCCC_OWNER_TRUSTED_OS ... ARM_SMCCC_OWNER_TRUSTED_OS_END: > + handled = tee_handle_smc(regs); > + break; > } > } > > diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S > index c9b9546..b78b7f1 100644 > --- a/xen/arch/arm/xen.lds.S > +++ b/xen/arch/arm/xen.lds.S > @@ -126,6 +126,13 @@ SECTIONS > _aedevice = .; > } :text > > + . = ALIGN(8); > + .teemediator.info : { > + _steemediator = .; > + *(.teemediator.info) > + _eteemediator = .; > + } :text > + > . = ALIGN(PAGE_SIZE); /* Init code and data */ > __init_begin = .; > .init.text : { > diff --git a/xen/include/asm-arm/tee.h b/xen/include/asm-arm/tee.h > new file mode 100644 > index 0000000..7f500ac > --- /dev/null > +++ b/xen/include/asm-arm/tee.h > @@ -0,0 +1,79 @@ > +/* > + * xen/include/asm-arm/tee.h > + * > + * Generic part of TEE mediator subsystem > + * > + * Volodymyr Babchuk <volodymyr_babchuk@epam.com> > + * Copyright (c) 2017 EPAM Systems. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#ifndef __ARCH_ARM_TEE_TEE_H__ > +#define __ARCH_ARM_TEE_TEE_H__ > + > +#include <xen/lib.h> > +#include <xen/types.h> > +#include <asm/regs.h> > + > +#ifdef CONFIG_ARM_TEE > + > +struct tee_mediator_ops { Please add documentations on all the fields to explain their purposes. > + int (*init)(void); > + void (*domain_create)(struct domain *d); > + void (*domain_destroy)(struct domain *d); > + bool (*handle_smc)(struct cpu_user_regs *regs); > + void (*remove)(void); > +}; > + > +struct tee_mediator_desc { Ditto. > + const char *name; > + const xen_uuid_t uid; > + const struct tee_mediator_ops *ops; > +}; > + > +void tee_init(void); > +bool tee_handle_smc(struct cpu_user_regs *regs); > +void tee_domain_create(struct domain *d); > +void tee_domain_destroy(struct domain *d); > +void tee_remove(void); > + > +#define REGISTER_TEE_MEDIATOR(_name, _namestr, _uid, _ops) \ > +static const struct tee_mediator_desc __tee_desc_##_name __used \ > +__section(".teemediator.info") = { \ > + .name = _namestr, \ > + .uid = _uid, \ > + .ops = _ops \ > +} > + > +#else > + > +static inline void tee_init(void) {} > +static inline bool tee_handle_smc(struct cpu_user_regs *regs) > +{ > + return false; > +} > +static inline void tee_domain_create(struct domain *d) {} > +static inline tee_domain_destroy(struct domain *d) {} > +static inline tee_remove(void) {} > + > +#endif /* CONFIG_ARM_TEE */ > + > +#endif /* __ARCH_ARM_TEE_TEE_H__ */ > + > +/* > + * Local variables: > + * mode: C > + * c-file-style: "BSD" > + * c-basic-offset: 4 > + * indent-tabs-mode: nil > + * End: > + */ > Cheers,
On Mon, Oct 16, 2017 at 02:00:32PM +0100, Julien Grall wrote: > Hi Volodymyr, Hi Julien, [...] > >This is how it works: user can build XEN with multiple TEE mediators > >(see the next patches, where OP-TEE mediator is introduced). > >TEE mediator register self with REGISTER_TEE_MEDIATOR() macro in the > >same way, as device drivers use DT_DEVICE_START()/DT_DEVICE_END() > >macros. > >In runtime, during initialization, XEN issues standard SMC to read > >TEE UID. Using this UID it selects and initializes one of built-in > >mediators. Then generic vSMC handler will call selected mediator > >when it intercept SMC that belongs to TEE OS or TEE application. > > As you may remember the discussion on the SMCCC support for guests, there > are currently no way to know the SMCCC is present on the platform. Ah, yes. Device tree, then. > I don't think you can rely on the platform support SMCC nor fully > implementing it. This also bring the question of does every TEE are > supporting SMCCC? Honestly, I don't know. I suppose that there can be TEEs the are not compatible with SMCCC. Okay, instead of UID comparison in generic framework, I can introduce probe() functions for each mediator. On other hand, DT bindings, can work even better. How about this: TEE mediator registers with DT_DEVICE_START() as any other driver. If, during init() it finds compatible TEE, then TEE mediator registers itself in TEE framework. During registration it provides supported SMC fn ranges. This is for case when TEE is not SMCCC-compatible and uses function numbers outside ARM_SMCCC_OWNER_TRUSTED_OS range. > > > >Also, there are hooks for domain construction and destruction, so > >TEE mediator can inform TEE about VM lifecycle. > > > >Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> > >--- > > MAINTAINERS | 5 ++ > > xen/arch/arm/Kconfig | 10 ++++ > > xen/arch/arm/Makefile | 1 + > > xen/arch/arm/domain.c | 7 +++ > > xen/arch/arm/setup.c | 4 ++ > > xen/arch/arm/tee/Kconfig | 0 > > xen/arch/arm/tee/Makefile | 1 + > > xen/arch/arm/tee/tee.c | 134 ++++++++++++++++++++++++++++++++++++++++++++++ > > xen/arch/arm/vsmc.c | 5 ++ > > xen/arch/arm/xen.lds.S | 7 +++ > > xen/include/asm-arm/tee.h | 79 +++++++++++++++++++++++++++ > > 11 files changed, 253 insertions(+) > > create mode 100644 xen/arch/arm/tee/Kconfig > > create mode 100644 xen/arch/arm/tee/Makefile > > create mode 100644 xen/arch/arm/tee/tee.c > > create mode 100644 xen/include/asm-arm/tee.h > > > >diff --git a/MAINTAINERS b/MAINTAINERS > >index 77b1e11..ede00c5 100644 > >--- a/MAINTAINERS > >+++ b/MAINTAINERS > >@@ -357,6 +357,11 @@ F: config/Stubdom.mk.in > > F: m4/stubdom.m4 > > F: stubdom/ > >+TEE MEDIATORS > >+M: Volodymyr Babchuk <volodymyr_babchuk@epam.com> > >+S: Supported > >+F: xen/arch/arm/tee/* > >+ > > TOOLSTACK > > M: Ian Jackson <ian.jackson@eu.citrix.com> > > M: Wei Liu <wei.liu2@citrix.com> > >diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig > >index d46b98c..e1f112a 100644 > >--- a/xen/arch/arm/Kconfig > >+++ b/xen/arch/arm/Kconfig > >@@ -50,6 +50,14 @@ config HAS_ITS > > prompt "GICv3 ITS MSI controller support" if EXPERT = "y" > > depends on HAS_GICV3 > >+config ARM_TEE > > The ARM in the title is a bit pointless. This Kconfig is only used for Arm > architecture. Just plain TEE then? > >+ bool "Enable TEE mediators support" > >+ default n > >+ depends on ARM > > No need for that. Right. > >+ help > >+ This option enables generic TEE mediators support. It allows guests > >+ to access real TEE via one of TEE mediators implemented in XEN > > Missing full stop. > > >+ > > endmenu > > menu "ARM errata workaround via the alternative framework" > >@@ -167,3 +175,5 @@ endmenu > > source "common/Kconfig" > > source "drivers/Kconfig" > >+ > >+source "arch/arm/tee/Kconfig" > >diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile > >index ede21fd..2710e0e 100644 > >--- a/xen/arch/arm/Makefile > >+++ b/xen/arch/arm/Makefile > >@@ -3,6 +3,7 @@ subdir-$(CONFIG_ARM_64) += arm64 > > subdir-y += platforms > > subdir-$(CONFIG_ARM_64) += efi > > subdir-$(CONFIG_ACPI) += acpi > >+subdir-$(CONFIG_ARM_TEE) += tee > > obj-$(CONFIG_HAS_ALTERNATIVE) += alternative.o > > obj-y += bootfdt.init.o > >diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c > >index 784ae39..3290d39 100644 > >--- a/xen/arch/arm/domain.c > >+++ b/xen/arch/arm/domain.c > >@@ -31,6 +31,7 @@ > > #include <asm/platform.h> > > #include <asm/procinfo.h> > > #include <asm/regs.h> > >+#include <asm/tee.h> > > #include <asm/vfp.h> > > #include <asm/vgic.h> > > #include <asm/vtimer.h> > >@@ -673,6 +674,9 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags, > > if ( is_hardware_domain(d) && (rc = domain_vuart_init(d)) ) > > goto fail; > >+ /* Notify TEE that new domain was created */ > >+ tee_domain_create(d); > > I am not a big fan to see this in arch_domain_create until we see how this > is going to fit with guest. For instance, will TEE be for every guests? What > would be the other necessary information to configure it?... I think I'll call XSM in tee_domain_create() to check if this domain allowed to work with TEE. I can't imagine what additional information will be needed. This interface can be extended in the future, though. > >+ > > return 0; > > fail: > >@@ -684,6 +688,9 @@ fail: > > void arch_domain_destroy(struct domain *d) > > { > >+ /* Notify TEE that domain is being destroyed */ > >+ tee_domain_destroy(d); > >+ > > /* IOMMU page table is shared with P2M, always call > > * iommu_domain_destroy() before p2m_teardown(). > > */ > >diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > >index 92f173b..8a4fcd8 100644 > >--- a/xen/arch/arm/setup.c > >+++ b/xen/arch/arm/setup.c > >@@ -47,6 +47,7 @@ > > #include <asm/platform.h> > > #include <asm/procinfo.h> > > #include <asm/setup.h> > >+#include <asm/tee.h> > > #include <xsm/xsm.h> > > #include <asm/acpi.h> > >@@ -846,6 +847,9 @@ void __init start_xen(unsigned long boot_phys_offset, > > */ > > apply_alternatives_all(); > >+ /* Initialize TEE mediator */ > >+ tee_init(); > >+ > > /* Create initial domain 0. */ > > /* The vGIC for DOM0 is exactly emulating the hardware GIC */ > > config.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE; > >diff --git a/xen/arch/arm/tee/Kconfig b/xen/arch/arm/tee/Kconfig > >new file mode 100644 > >index 0000000..e69de29 > >diff --git a/xen/arch/arm/tee/Makefile b/xen/arch/arm/tee/Makefile > >new file mode 100644 > >index 0000000..c54d479 > >--- /dev/null > >+++ b/xen/arch/arm/tee/Makefile > >@@ -0,0 +1 @@ > >+obj-y += tee.o > >diff --git a/xen/arch/arm/tee/tee.c b/xen/arch/arm/tee/tee.c > >new file mode 100644 > >index 0000000..7f7a846 > >--- /dev/null > >+++ b/xen/arch/arm/tee/tee.c > >@@ -0,0 +1,134 @@ > >+/* > >+ * xen/arch/arm/tee/tee.c > >+ * > >+ * Generic part of TEE mediator subsystem > >+ * > >+ * Volodymyr Babchuk <volodymyr_babchuk@epam.com> > >+ * Copyright (c) 2017 EPAM Systems. > >+ * > >+ * This program is free software; you can redistribute it and/or modify > >+ * it under the terms of the GNU General Public License version 2 as > >+ * published by the Free Software Foundation. > >+ * > >+ * This program is distributed in the hope that it will be useful, > >+ * but WITHOUT ANY WARRANTY; without even the implied warranty of > >+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > >+ * GNU General Public License for more details. > >+ */ > >+ > >+#include <xen/types.h> > >+#include <asm/smccc.h> > >+#include <asm/tee.h> > >+ > >+/* > >+ * According to ARM SMCCC (ARM DEN 0028B, page 17), service owner > >+ * for generic TEE queries is 63. > >+ */ > >+#define TRUSTED_OS_GENERIC_API_OWNER 63 > >+ > >+#define ARM_SMCCC_FUNC_GET_TEE_UID \ > >+ ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \ > >+ ARM_SMCCC_CONV_32, \ > >+ TRUSTED_OS_GENERIC_API_OWNER, \ > >+ ARM_SMCCC_FUNC_CALL_UID) > > This likely needs to be defined in smccc as AFAIU it is part of the SMCCC. It only used there. I'm not sure if I should define it globally. > >+ > >+extern const struct tee_mediator_desc _steemediator[], _eteemediator[]; > >+static const struct tee_mediator_ops *mediator_ops; > >+ > >+/* Helper function to read UID returned by SMC */ > >+static void parse_uid(const register_t regs[4], xen_uuid_t *uid) > >+{ > >+ uint8_t *bytes = uid->a; > >+ int n; > > unsigned int please. > > >+ > >+ /* > >+ * UID is returned in registers r0..r3, four bytes per register, > >+ * first byte is stored in low-order bits of a register. > >+ * (ARM DEN 0028B page 14) > >+ */ > >+ for (n = 0; n < 16; n++) > > for ( n = 0; ... ) Yeah, sorry. It is hard to hold multiple coding styles in mind at the same time. I hope, Yurii will finish his coding style validator soon :) > >+ bytes[n] = (uint8_t)(regs[n/4] >> ((n & 3) * 8)); > >+ > >+} > >+ > >+void tee_init(void) > >+{ > >+ const struct tee_mediator_desc *desc; > >+ register_t resp[4]; > >+ xen_uuid_t tee_uid; > >+ int ret; > >+ > >+ /* Read UUID to determine which TEE is running */ > > You can't assume the platform is supporting SMCCC. See my comment at the > beginning of the e-mail. Yeah, looks like this part will be completely reworked. > >+ call_smccc_smc(ARM_SMCCC_FUNC_GET_TEE_UID, 0, 0, 0, 0, 0, 0, 0, resp); > >+ if ( resp[0] == 0xFFFFFFFF ) { > > You likely want to use ARM_SMCCC_ERR_UNKOWN_FUNCTION here. > > >+ printk("No TEE found\n"); > >+ return; > >+ } > >+ > >+ parse_uid(resp, &tee_uid); > >+ > >+ printk("TEE UID: %02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x\n", > >+ tee_uid.a[0 ], tee_uid.a[1 ], tee_uid.a[2 ], tee_uid.a[3 ], > > Please no space before ]. This is making more confusing to read. I put it for neat formatting. Probably, I can put double space after commas. Will be okay? > >+ tee_uid.a[4 ], tee_uid.a[5 ], tee_uid.a[6 ], tee_uid.a[7 ], > >+ tee_uid.a[8 ], tee_uid.a[9 ], tee_uid.a[10], tee_uid.a[11], > >+ tee_uid.a[12], tee_uid.a[13], tee_uid.a[14], tee_uid.a[15]); > >+ > >+ for ( desc = _steemediator; desc != _eteemediator; desc++ ) > > { > > >+ if ( memcmp(&desc->uid, &tee_uid, sizeof(xen_uuid_t)) == 0 ) > >+ { > >+ printk("Using TEE mediator for %sp\n", desc->name); > >+ mediator_ops = desc->ops; > >+ break; > >+ } > > } > > >+ > >+ if ( !mediator_ops ) > > A warning here would be useful. Why? Platform is not obligued to have any TEE. > >+ return; > >+ > >+ ret = mediator_ops->init(); > >+ if ( ret ) > >+ { > >+ printk("TEE mediator failed to initialize :%d\n", ret); > >+ mediator_ops = NULL; > >+ } > >+} > >+ > >+bool tee_handle_smc(struct cpu_user_regs *regs) > >+{ > >+ if ( !mediator_ops ) > >+ return false; > >+ > >+ return mediator_ops->handle_smc(regs); > >+} > >+ > >+void tee_domain_create(struct domain *d) > >+{ > >+ if ( !mediator_ops ) > >+ return; > >+ > >+ return mediator_ops->domain_create(d); > > return here is not necessary. However, I am slightly surprised that > tee_domain_create could never fail. Good question. Do you want to allow TEE to prevent domain creation? > >+} > >+ > >+void tee_domain_destroy(struct domain *d) > >+{ > >+ if ( !mediator_ops ) > >+ return; > >+ > >+ return mediator_ops->domain_destroy(d); > > Same here. > > >+} > >+ > >+void tee_remove(void) > > What is this callback for? I don't see any use within this series. Sorry, missed to call it. [...]
On 17/10/17 17:22, Volodymyr Babchuk wrote: > On Mon, Oct 16, 2017 at 02:00:32PM +0100, Julien Grall wrote: >>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig >>> index d46b98c..e1f112a 100644 >>> --- a/xen/arch/arm/Kconfig >>> +++ b/xen/arch/arm/Kconfig >>> @@ -50,6 +50,14 @@ config HAS_ITS >>> prompt "GICv3 ITS MSI controller support" if EXPERT = "y" >>> depends on HAS_GICV3 >>> +config ARM_TEE >> >> The ARM in the title is a bit pointless. This Kconfig is only used for Arm >> architecture. > Just plain TEE then? Yes please. > >>> + bool "Enable TEE mediators support" >>> + default n >>> + depends on ARM >> >> No need for that. > Right. > >>> + help >>> + This option enables generic TEE mediators support. It allows guests >>> + to access real TEE via one of TEE mediators implemented in XEN >> >> Missing full stop. >> >>> + >>> endmenu >>> menu "ARM errata workaround via the alternative framework" >>> @@ -167,3 +175,5 @@ endmenu >>> source "common/Kconfig" >>> source "drivers/Kconfig" >>> + >>> +source "arch/arm/tee/Kconfig" >>> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile >>> index ede21fd..2710e0e 100644 >>> --- a/xen/arch/arm/Makefile >>> +++ b/xen/arch/arm/Makefile >>> @@ -3,6 +3,7 @@ subdir-$(CONFIG_ARM_64) += arm64 >>> subdir-y += platforms >>> subdir-$(CONFIG_ARM_64) += efi >>> subdir-$(CONFIG_ACPI) += acpi >>> +subdir-$(CONFIG_ARM_TEE) += tee >>> obj-$(CONFIG_HAS_ALTERNATIVE) += alternative.o >>> obj-y += bootfdt.init.o >>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c >>> index 784ae39..3290d39 100644 >>> --- a/xen/arch/arm/domain.c >>> +++ b/xen/arch/arm/domain.c >>> @@ -31,6 +31,7 @@ >>> #include <asm/platform.h> >>> #include <asm/procinfo.h> >>> #include <asm/regs.h> >>> +#include <asm/tee.h> >>> #include <asm/vfp.h> >>> #include <asm/vgic.h> >>> #include <asm/vtimer.h> >>> @@ -673,6 +674,9 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags, >>> if ( is_hardware_domain(d) && (rc = domain_vuart_init(d)) ) >>> goto fail; >>> + /* Notify TEE that new domain was created */ >>> + tee_domain_create(d); >> >> I am not a big fan to see this in arch_domain_create until we see how this >> is going to fit with guest. For instance, will TEE be for every guests? What >> would be the other necessary information to configure it?... > I think I'll call XSM in tee_domain_create() to check if this domain allowed > to work with TEE. I can't imagine what additional information will be needed. > This interface can be extended in the future, though. You will never need to inform TEE that a new client (aka domain) is been created, nor allocated memory for the TEE at domain creation in Xen? [...] >>> diff --git a/xen/arch/arm/tee/tee.c b/xen/arch/arm/tee/tee.c >>> new file mode 100644 >>> index 0000000..7f7a846 >>> --- /dev/null >>> +++ b/xen/arch/arm/tee/tee.c >>> @@ -0,0 +1,134 @@ >>> +/* >>> + * xen/arch/arm/tee/tee.c >>> + * >>> + * Generic part of TEE mediator subsystem >>> + * >>> + * Volodymyr Babchuk <volodymyr_babchuk@epam.com> >>> + * Copyright (c) 2017 EPAM Systems. >>> + * >>> + * This program is free software; you can redistribute it and/or modify >>> + * it under the terms of the GNU General Public License version 2 as >>> + * published by the Free Software Foundation. >>> + * >>> + * This program is distributed in the hope that it will be useful, >>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >>> + * GNU General Public License for more details. >>> + */ >>> + >>> +#include <xen/types.h> >>> +#include <asm/smccc.h> >>> +#include <asm/tee.h> >>> + >>> +/* >>> + * According to ARM SMCCC (ARM DEN 0028B, page 17), service owner >>> + * for generic TEE queries is 63. >>> + */ >>> +#define TRUSTED_OS_GENERIC_API_OWNER 63 >>> + >>> +#define ARM_SMCCC_FUNC_GET_TEE_UID \ >>> + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \ >>> + ARM_SMCCC_CONV_32, \ >>> + TRUSTED_OS_GENERIC_API_OWNER, \ >>> + ARM_SMCCC_FUNC_CALL_UID) >> >> This likely needs to be defined in smccc as AFAIU it is part of the SMCCC. > It only used there. I'm not sure if I should define it globally. Maybe ARM_SMCCC_FUNC_GET_TEE_UID, but definitely TRUSTED_OS_GENERIC_API_OWNER should stick with the rest of the subsystem definition in smccc.h. [...] >>> + printk("No TEE found\n"); >>> + return; >>> + } >>> + >>> + parse_uid(resp, &tee_uid); >>> + >>> + printk("TEE UID: %02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x\n", >>> + tee_uid.a[0 ], tee_uid.a[1 ], tee_uid.a[2 ], tee_uid.a[3 ], >> >> Please no space before ]. This is making more confusing to read. > I put it for neat formatting. Probably, I can put double space after commas. > Will be okay? That is that really important to have them? I mean, ok it is not going to be neat but the format string is already ugly and it would not be too difficult to read the arguments. > >>> + tee_uid.a[4 ], tee_uid.a[5 ], tee_uid.a[6 ], tee_uid.a[7 ], >>> + tee_uid.a[8 ], tee_uid.a[9 ], tee_uid.a[10], tee_uid.a[11], >>> + tee_uid.a[12], tee_uid.a[13], tee_uid.a[14], tee_uid.a[15]); >>> + >>> + for ( desc = _steemediator; desc != _eteemediator; desc++ ) >> >> { >> >>> + if ( memcmp(&desc->uid, &tee_uid, sizeof(xen_uuid_t)) == 0 ) >>> + { >>> + printk("Using TEE mediator for %sp\n", desc->name); >>> + mediator_ops = desc->ops; >>> + break; >>> + } >> >> } >> >>> + >>> + if ( !mediator_ops ) >> >> A warning here would be useful. > Why? Platform is not obligued to have any TEE. What do you mean? You can only be here because the platform has TEE but Xen does not have any mediator. You actually print "no TEE found" a bit above. So why not printing for when Xen is unable to use it? > >>> + return; >>> + >>> + ret = mediator_ops->init(); >>> + if ( ret ) >>> + { >>> + printk("TEE mediator failed to initialize :%d\n", ret); >>> + mediator_ops = NULL; >>> + } >>> +} >>> + >>> +bool tee_handle_smc(struct cpu_user_regs *regs) >>> +{ >>> + if ( !mediator_ops ) >>> + return false; >>> + >>> + return mediator_ops->handle_smc(regs); >>> +} >>> + >>> +void tee_domain_create(struct domain *d) >>> +{ >>> + if ( !mediator_ops ) >>> + return; >>> + >>> + return mediator_ops->domain_create(d); >> >> return here is not necessary. However, I am slightly surprised that >> tee_domain_create could never fail. > Good question. Do you want to allow TEE to prevent domain creation? See my answer a bit above on the call. > >>> +} >>> + >>> +void tee_domain_destroy(struct domain *d) >>> +{ >>> + if ( !mediator_ops ) >>> + return; >>> + >>> + return mediator_ops->domain_destroy(d); >> >> Same here. >> >>> +} >>> + >>> +void tee_remove(void) >> >> What is this callback for? I don't see any use within this series. > Sorry, missed to call it. > > [...] > Cheers,
Hi Julien, On Tue, Oct 17, 2017 at 05:39:29PM +0100, Julien Grall wrote: Excuse me, looks like you skipped my thoughts about how to detect TEE if we are not sure, that we are running on SMCCC-capable platform. How do you think, is it appropriate to rely on DT? [...] > >>>@@ -673,6 +674,9 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags, > >>> if ( is_hardware_domain(d) && (rc = domain_vuart_init(d)) ) > >>> goto fail; > >>>+ /* Notify TEE that new domain was created */ > >>>+ tee_domain_create(d); > >> > >>I am not a big fan to see this in arch_domain_create until we see how this > >>is going to fit with guest. For instance, will TEE be for every guests? What > >>would be the other necessary information to configure it?... > >I think I'll call XSM in tee_domain_create() to check if this domain allowed > >to work with TEE. I can't imagine what additional information will be needed. > >This interface can be extended in the future, though. > > You will never need to inform TEE that a new client (aka domain) is been > created, nor allocated memory for the TEE at domain creation in Xen? Yes. You are right. But then there are another question: what to do if tee_domain_create() failed? Prevent domain creation? Or create domain anyways, but forbid it to call TEE? > [...] > > >>>diff --git a/xen/arch/arm/tee/tee.c b/xen/arch/arm/tee/tee.c > >>>new file mode 100644 > >>>index 0000000..7f7a846 > >>>--- /dev/null > >>>+++ b/xen/arch/arm/tee/tee.c > >>>@@ -0,0 +1,134 @@ > >>>+/* > >>>+ * xen/arch/arm/tee/tee.c > >>>+ * > >>>+ * Generic part of TEE mediator subsystem > >>>+ * > >>>+ * Volodymyr Babchuk <volodymyr_babchuk@epam.com> > >>>+ * Copyright (c) 2017 EPAM Systems. > >>>+ * > >>>+ * This program is free software; you can redistribute it and/or modify > >>>+ * it under the terms of the GNU General Public License version 2 as > >>>+ * published by the Free Software Foundation. > >>>+ * > >>>+ * This program is distributed in the hope that it will be useful, > >>>+ * but WITHOUT ANY WARRANTY; without even the implied warranty of > >>>+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > >>>+ * GNU General Public License for more details. > >>>+ */ > >>>+ > >>>+#include <xen/types.h> > >>>+#include <asm/smccc.h> > >>>+#include <asm/tee.h> > >>>+ > >>>+/* > >>>+ * According to ARM SMCCC (ARM DEN 0028B, page 17), service owner > >>>+ * for generic TEE queries is 63. > >>>+ */ > >>>+#define TRUSTED_OS_GENERIC_API_OWNER 63 > >>>+ > >>>+#define ARM_SMCCC_FUNC_GET_TEE_UID \ > >>>+ ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \ > >>>+ ARM_SMCCC_CONV_32, \ > >>>+ TRUSTED_OS_GENERIC_API_OWNER, \ > >>>+ ARM_SMCCC_FUNC_CALL_UID) > >> > >>This likely needs to be defined in smccc as AFAIU it is part of the SMCCC. > >It only used there. I'm not sure if I should define it globally. > > Maybe ARM_SMCCC_FUNC_GET_TEE_UID, but definitely > TRUSTED_OS_GENERIC_API_OWNER should stick with the rest of the subsystem > definition in smccc.h. Yes, will do in this way. > [...] > > >>>+ printk("No TEE found\n"); > >>>+ return; > >>>+ } > >>>+ > >>>+ parse_uid(resp, &tee_uid); > >>>+ > >>>+ printk("TEE UID: %02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x\n", > >>>+ tee_uid.a[0 ], tee_uid.a[1 ], tee_uid.a[2 ], tee_uid.a[3 ], > >> > >>Please no space before ]. This is making more confusing to read. > >I put it for neat formatting. Probably, I can put double space after commas. > >Will be okay? > > That is that really important to have them? I mean, ok it is not going to be > neat but the format string is already ugly and it would not be too difficult > to read the arguments. Dunno. As for me, it eases parsing. For example, it is easy to see that all indexes are correct. But if this is inappropriate, I can remove all extra spaces. > > > > >>>+ tee_uid.a[4 ], tee_uid.a[5 ], tee_uid.a[6 ], tee_uid.a[7 ], > >>>+ tee_uid.a[8 ], tee_uid.a[9 ], tee_uid.a[10], tee_uid.a[11], > >>>+ tee_uid.a[12], tee_uid.a[13], tee_uid.a[14], tee_uid.a[15]); > >>>+ > >>>+ for ( desc = _steemediator; desc != _eteemediator; desc++ ) > >> > >>{ > >> > >>>+ if ( memcmp(&desc->uid, &tee_uid, sizeof(xen_uuid_t)) == 0 ) > >>>+ { > >>>+ printk("Using TEE mediator for %sp\n", desc->name); > >>>+ mediator_ops = desc->ops; > >>>+ break; > >>>+ } > >> > >>} > >> > >>>+ > >>>+ if ( !mediator_ops ) > >> > >>A warning here would be useful. > >Why? Platform is not obligued to have any TEE. > > What do you mean? You can only be here because the platform has TEE but Xen > does not have any mediator. You actually print "no TEE found" a bit above. > So why not printing for when Xen is unable to use it? Ah, yes. This makes sense.
On 17/10/17 18:22, Volodymyr Babchuk wrote: > Hi Julien, > > On Tue, Oct 17, 2017 at 05:39:29PM +0100, Julien Grall wrote: > > Excuse me, looks like you skipped my thoughts about how to detect > TEE if we are not sure, that we are running on SMCCC-capable platform. > > How do you think, is it appropriate to rely on DT? I didn't have any opinion as long as it covers most of the TEE and does not make any assumption on the platform other than written in the ARM ARM. I have no idea how Linux is doing the detection, it might be worth for you to have a look there. > > [...] >>>>> @@ -673,6 +674,9 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags, >>>>> if ( is_hardware_domain(d) && (rc = domain_vuart_init(d)) ) >>>>> goto fail; >>>>> + /* Notify TEE that new domain was created */ >>>>> + tee_domain_create(d); >>>> >>>> I am not a big fan to see this in arch_domain_create until we see how this >>>> is going to fit with guest. For instance, will TEE be for every guests? What >>>> would be the other necessary information to configure it?... >>> I think I'll call XSM in tee_domain_create() to check if this domain allowed >>> to work with TEE. I can't imagine what additional information will be needed. >>> This interface can be extended in the future, though. >> >> You will never need to inform TEE that a new client (aka domain) is been >> created, nor allocated memory for the TEE at domain creation in Xen? > Yes. You are right. But then there are another question: what to do > if tee_domain_create() failed? Prevent domain creation? Or create > domain anyways, but forbid it to call TEE? I would expect TEE to not be enabled by default for guests. This would be user configurable. So if it requests TEE and it is not possible to instantiate one, then the domain creation should failed. > >> [...] >> >>>>> diff --git a/xen/arch/arm/tee/tee.c b/xen/arch/arm/tee/tee.c >>>>> new file mode 100644 >>>>> index 0000000..7f7a846 >>>>> --- /dev/null >>>>> +++ b/xen/arch/arm/tee/tee.c >>>>> @@ -0,0 +1,134 @@ >>>>> +/* >>>>> + * xen/arch/arm/tee/tee.c >>>>> + * >>>>> + * Generic part of TEE mediator subsystem >>>>> + * >>>>> + * Volodymyr Babchuk <volodymyr_babchuk@epam.com> >>>>> + * Copyright (c) 2017 EPAM Systems. >>>>> + * >>>>> + * This program is free software; you can redistribute it and/or modify >>>>> + * it under the terms of the GNU General Public License version 2 as >>>>> + * published by the Free Software Foundation. >>>>> + * >>>>> + * This program is distributed in the hope that it will be useful, >>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >>>>> + * GNU General Public License for more details. >>>>> + */ >>>>> + >>>>> +#include <xen/types.h> >>>>> +#include <asm/smccc.h> >>>>> +#include <asm/tee.h> >>>>> + >>>>> +/* >>>>> + * According to ARM SMCCC (ARM DEN 0028B, page 17), service owner >>>>> + * for generic TEE queries is 63. >>>>> + */ >>>>> +#define TRUSTED_OS_GENERIC_API_OWNER 63 >>>>> + >>>>> +#define ARM_SMCCC_FUNC_GET_TEE_UID \ >>>>> + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \ >>>>> + ARM_SMCCC_CONV_32, \ >>>>> + TRUSTED_OS_GENERIC_API_OWNER, \ >>>>> + ARM_SMCCC_FUNC_CALL_UID) >>>> >>>> This likely needs to be defined in smccc as AFAIU it is part of the SMCCC. >>> It only used there. I'm not sure if I should define it globally. >> >> Maybe ARM_SMCCC_FUNC_GET_TEE_UID, but definitely >> TRUSTED_OS_GENERIC_API_OWNER should stick with the rest of the subsystem >> definition in smccc.h. > Yes, will do in this way. > >> [...] >> >>>>> + printk("No TEE found\n"); >>>>> + return; >>>>> + } >>>>> + >>>>> + parse_uid(resp, &tee_uid); >>>>> + >>>>> + printk("TEE UID: %02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x\n", >>>>> + tee_uid.a[0 ], tee_uid.a[1 ], tee_uid.a[2 ], tee_uid.a[3 ], >>>> >>>> Please no space before ]. This is making more confusing to read. >>> I put it for neat formatting. Probably, I can put double space after commas. >>> Will be okay? >> >> That is that really important to have them? I mean, ok it is not going to be >> neat but the format string is already ugly and it would not be too difficult >> to read the arguments. > Dunno. As for me, it eases parsing. For example, it is easy to see that > all indexes are correct. But if this is inappropriate, I can remove all > extra spaces. Well, if you are going to do a different probe than via SMCCC, then this discussion is not necessary as the printk would likely disappear. >> >>> >>>>> + tee_uid.a[4 ], tee_uid.a[5 ], tee_uid.a[6 ], tee_uid.a[7 ], >>>>> + tee_uid.a[8 ], tee_uid.a[9 ], tee_uid.a[10], tee_uid.a[11], >>>>> + tee_uid.a[12], tee_uid.a[13], tee_uid.a[14], tee_uid.a[15]); >>>>> + >>>>> + for ( desc = _steemediator; desc != _eteemediator; desc++ ) >>>> >>>> { >>>> >>>>> + if ( memcmp(&desc->uid, &tee_uid, sizeof(xen_uuid_t)) == 0 ) >>>>> + { >>>>> + printk("Using TEE mediator for %sp\n", desc->name); >>>>> + mediator_ops = desc->ops; >>>>> + break; >>>>> + } >>>> >>>> } >>>> >>>>> + >>>>> + if ( !mediator_ops ) >>>> >>>> A warning here would be useful. >>> Why? Platform is not obligued to have any TEE. >> >> What do you mean? You can only be here because the platform has TEE but Xen >> does not have any mediator. You actually print "no TEE found" a bit above. >> So why not printing for when Xen is unable to use it? > Ah, yes. This makes sense. >
diff --git a/MAINTAINERS b/MAINTAINERS index 77b1e11..ede00c5 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -357,6 +357,11 @@ F: config/Stubdom.mk.in F: m4/stubdom.m4 F: stubdom/ +TEE MEDIATORS +M: Volodymyr Babchuk <volodymyr_babchuk@epam.com> +S: Supported +F: xen/arch/arm/tee/* + TOOLSTACK M: Ian Jackson <ian.jackson@eu.citrix.com> M: Wei Liu <wei.liu2@citrix.com> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig index d46b98c..e1f112a 100644 --- a/xen/arch/arm/Kconfig +++ b/xen/arch/arm/Kconfig @@ -50,6 +50,14 @@ config HAS_ITS prompt "GICv3 ITS MSI controller support" if EXPERT = "y" depends on HAS_GICV3 +config ARM_TEE + bool "Enable TEE mediators support" + default n + depends on ARM + help + This option enables generic TEE mediators support. It allows guests + to access real TEE via one of TEE mediators implemented in XEN + endmenu menu "ARM errata workaround via the alternative framework" @@ -167,3 +175,5 @@ endmenu source "common/Kconfig" source "drivers/Kconfig" + +source "arch/arm/tee/Kconfig" diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile index ede21fd..2710e0e 100644 --- a/xen/arch/arm/Makefile +++ b/xen/arch/arm/Makefile @@ -3,6 +3,7 @@ subdir-$(CONFIG_ARM_64) += arm64 subdir-y += platforms subdir-$(CONFIG_ARM_64) += efi subdir-$(CONFIG_ACPI) += acpi +subdir-$(CONFIG_ARM_TEE) += tee obj-$(CONFIG_HAS_ALTERNATIVE) += alternative.o obj-y += bootfdt.init.o diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index 784ae39..3290d39 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -31,6 +31,7 @@ #include <asm/platform.h> #include <asm/procinfo.h> #include <asm/regs.h> +#include <asm/tee.h> #include <asm/vfp.h> #include <asm/vgic.h> #include <asm/vtimer.h> @@ -673,6 +674,9 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags, if ( is_hardware_domain(d) && (rc = domain_vuart_init(d)) ) goto fail; + /* Notify TEE that new domain was created */ + tee_domain_create(d); + return 0; fail: @@ -684,6 +688,9 @@ fail: void arch_domain_destroy(struct domain *d) { + /* Notify TEE that domain is being destroyed */ + tee_domain_destroy(d); + /* IOMMU page table is shared with P2M, always call * iommu_domain_destroy() before p2m_teardown(). */ diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index 92f173b..8a4fcd8 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -47,6 +47,7 @@ #include <asm/platform.h> #include <asm/procinfo.h> #include <asm/setup.h> +#include <asm/tee.h> #include <xsm/xsm.h> #include <asm/acpi.h> @@ -846,6 +847,9 @@ void __init start_xen(unsigned long boot_phys_offset, */ apply_alternatives_all(); + /* Initialize TEE mediator */ + tee_init(); + /* Create initial domain 0. */ /* The vGIC for DOM0 is exactly emulating the hardware GIC */ config.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE; diff --git a/xen/arch/arm/tee/Kconfig b/xen/arch/arm/tee/Kconfig new file mode 100644 index 0000000..e69de29 diff --git a/xen/arch/arm/tee/Makefile b/xen/arch/arm/tee/Makefile new file mode 100644 index 0000000..c54d479 --- /dev/null +++ b/xen/arch/arm/tee/Makefile @@ -0,0 +1 @@ +obj-y += tee.o diff --git a/xen/arch/arm/tee/tee.c b/xen/arch/arm/tee/tee.c new file mode 100644 index 0000000..7f7a846 --- /dev/null +++ b/xen/arch/arm/tee/tee.c @@ -0,0 +1,134 @@ +/* + * xen/arch/arm/tee/tee.c + * + * Generic part of TEE mediator subsystem + * + * Volodymyr Babchuk <volodymyr_babchuk@epam.com> + * Copyright (c) 2017 EPAM Systems. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <xen/types.h> +#include <asm/smccc.h> +#include <asm/tee.h> + +/* + * According to ARM SMCCC (ARM DEN 0028B, page 17), service owner + * for generic TEE queries is 63. + */ +#define TRUSTED_OS_GENERIC_API_OWNER 63 + +#define ARM_SMCCC_FUNC_GET_TEE_UID \ + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \ + ARM_SMCCC_CONV_32, \ + TRUSTED_OS_GENERIC_API_OWNER, \ + ARM_SMCCC_FUNC_CALL_UID) + +extern const struct tee_mediator_desc _steemediator[], _eteemediator[]; +static const struct tee_mediator_ops *mediator_ops; + +/* Helper function to read UID returned by SMC */ +static void parse_uid(const register_t regs[4], xen_uuid_t *uid) +{ + uint8_t *bytes = uid->a; + int n; + + /* + * UID is returned in registers r0..r3, four bytes per register, + * first byte is stored in low-order bits of a register. + * (ARM DEN 0028B page 14) + */ + for (n = 0; n < 16; n++) + bytes[n] = (uint8_t)(regs[n/4] >> ((n & 3) * 8)); + +} + +void tee_init(void) +{ + const struct tee_mediator_desc *desc; + register_t resp[4]; + xen_uuid_t tee_uid; + int ret; + + /* Read UUID to determine which TEE is running */ + call_smccc_smc(ARM_SMCCC_FUNC_GET_TEE_UID, 0, 0, 0, 0, 0, 0, 0, resp); + if ( resp[0] == 0xFFFFFFFF ) { + printk("No TEE found\n"); + return; + } + + parse_uid(resp, &tee_uid); + + printk("TEE UID: %02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x\n", + tee_uid.a[0 ], tee_uid.a[1 ], tee_uid.a[2 ], tee_uid.a[3 ], + tee_uid.a[4 ], tee_uid.a[5 ], tee_uid.a[6 ], tee_uid.a[7 ], + tee_uid.a[8 ], tee_uid.a[9 ], tee_uid.a[10], tee_uid.a[11], + tee_uid.a[12], tee_uid.a[13], tee_uid.a[14], tee_uid.a[15]); + + for ( desc = _steemediator; desc != _eteemediator; desc++ ) + if ( memcmp(&desc->uid, &tee_uid, sizeof(xen_uuid_t)) == 0 ) + { + printk("Using TEE mediator for %sp\n", desc->name); + mediator_ops = desc->ops; + break; + } + + if ( !mediator_ops ) + return; + + ret = mediator_ops->init(); + if ( ret ) + { + printk("TEE mediator failed to initialize :%d\n", ret); + mediator_ops = NULL; + } +} + +bool tee_handle_smc(struct cpu_user_regs *regs) +{ + if ( !mediator_ops ) + return false; + + return mediator_ops->handle_smc(regs); +} + +void tee_domain_create(struct domain *d) +{ + if ( !mediator_ops ) + return; + + return mediator_ops->domain_create(d); +} + +void tee_domain_destroy(struct domain *d) +{ + if ( !mediator_ops ) + return; + + return mediator_ops->domain_destroy(d); +} + +void tee_remove(void) +{ + if ( !mediator_ops ) + return; + + return mediator_ops->remove(); +} + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * indent-tabs-mode: nil + * End: + */ diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c index 7bd6008..186e34b 100644 --- a/xen/arch/arm/vsmc.c +++ b/xen/arch/arm/vsmc.c @@ -22,6 +22,7 @@ #include <asm/psci.h> #include <asm/regs.h> #include <asm/smccc.h> +#include <asm/tee.h> #include <asm/traps.h> /* Number of functions currently supported by Hypervisor Service. */ @@ -288,6 +289,10 @@ static bool vsmccc_handle_call(struct cpu_user_regs *regs) case ARM_SMCCC_OWNER_STANDARD: handled = handle_sssc(regs); break; + case ARM_SMCCC_OWNER_TRUSTED_APP ... ARM_SMCCC_OWNER_TRUSTED_APP_END: + case ARM_SMCCC_OWNER_TRUSTED_OS ... ARM_SMCCC_OWNER_TRUSTED_OS_END: + handled = tee_handle_smc(regs); + break; } } diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S index c9b9546..b78b7f1 100644 --- a/xen/arch/arm/xen.lds.S +++ b/xen/arch/arm/xen.lds.S @@ -126,6 +126,13 @@ SECTIONS _aedevice = .; } :text + . = ALIGN(8); + .teemediator.info : { + _steemediator = .; + *(.teemediator.info) + _eteemediator = .; + } :text + . = ALIGN(PAGE_SIZE); /* Init code and data */ __init_begin = .; .init.text : { diff --git a/xen/include/asm-arm/tee.h b/xen/include/asm-arm/tee.h new file mode 100644 index 0000000..7f500ac --- /dev/null +++ b/xen/include/asm-arm/tee.h @@ -0,0 +1,79 @@ +/* + * xen/include/asm-arm/tee.h + * + * Generic part of TEE mediator subsystem + * + * Volodymyr Babchuk <volodymyr_babchuk@epam.com> + * Copyright (c) 2017 EPAM Systems. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef __ARCH_ARM_TEE_TEE_H__ +#define __ARCH_ARM_TEE_TEE_H__ + +#include <xen/lib.h> +#include <xen/types.h> +#include <asm/regs.h> + +#ifdef CONFIG_ARM_TEE + +struct tee_mediator_ops { + int (*init)(void); + void (*domain_create)(struct domain *d); + void (*domain_destroy)(struct domain *d); + bool (*handle_smc)(struct cpu_user_regs *regs); + void (*remove)(void); +}; + +struct tee_mediator_desc { + const char *name; + const xen_uuid_t uid; + const struct tee_mediator_ops *ops; +}; + +void tee_init(void); +bool tee_handle_smc(struct cpu_user_regs *regs); +void tee_domain_create(struct domain *d); +void tee_domain_destroy(struct domain *d); +void tee_remove(void); + +#define REGISTER_TEE_MEDIATOR(_name, _namestr, _uid, _ops) \ +static const struct tee_mediator_desc __tee_desc_##_name __used \ +__section(".teemediator.info") = { \ + .name = _namestr, \ + .uid = _uid, \ + .ops = _ops \ +} + +#else + +static inline void tee_init(void) {} +static inline bool tee_handle_smc(struct cpu_user_regs *regs) +{ + return false; +} +static inline void tee_domain_create(struct domain *d) {} +static inline tee_domain_destroy(struct domain *d) {} +static inline tee_remove(void) {} + +#endif /* CONFIG_ARM_TEE */ + +#endif /* __ARCH_ARM_TEE_TEE_H__ */ + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * indent-tabs-mode: nil + * End: + */
This patch adds basic framework for TEE mediators. Guests can't talk to TEE directly, we need some entity that will intercept request and decide what to do with them. "TEE mediaor" is a such entity. This is how it works: user can build XEN with multiple TEE mediators (see the next patches, where OP-TEE mediator is introduced). TEE mediator register self with REGISTER_TEE_MEDIATOR() macro in the same way, as device drivers use DT_DEVICE_START()/DT_DEVICE_END() macros. In runtime, during initialization, XEN issues standard SMC to read TEE UID. Using this UID it selects and initializes one of built-in mediators. Then generic vSMC handler will call selected mediator when it intercept SMC that belongs to TEE OS or TEE application. Also, there are hooks for domain construction and destruction, so TEE mediator can inform TEE about VM lifecycle. Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> --- MAINTAINERS | 5 ++ xen/arch/arm/Kconfig | 10 ++++ xen/arch/arm/Makefile | 1 + xen/arch/arm/domain.c | 7 +++ xen/arch/arm/setup.c | 4 ++ xen/arch/arm/tee/Kconfig | 0 xen/arch/arm/tee/Makefile | 1 + xen/arch/arm/tee/tee.c | 134 ++++++++++++++++++++++++++++++++++++++++++++++ xen/arch/arm/vsmc.c | 5 ++ xen/arch/arm/xen.lds.S | 7 +++ xen/include/asm-arm/tee.h | 79 +++++++++++++++++++++++++++ 11 files changed, 253 insertions(+) create mode 100644 xen/arch/arm/tee/Kconfig create mode 100644 xen/arch/arm/tee/Makefile create mode 100644 xen/arch/arm/tee/tee.c create mode 100644 xen/include/asm-arm/tee.h