Re: [Xen-devel] [PATCH v3 01/11] arm: add generic TEE mediator framework

[Julien Grall <julien.grall@xxxxxxx>]

Hi Volodymyr,

On 12/18/18 9:11 PM, 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 mediator" 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 run-time, during initialization, framework calls probe() function
> for each available mediator driver to find which TEE is installed
> on the platform. Then generic vSMC handler will call selected mediator
> when it intercept SMC/HVC that belongs to TEE OS or TEE application.
>
> Currently TEE mediator is enabled only for Dom0.
>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
> ---
>   Changes from v2:
>    - Removed empty tee/Kconfig file
>
>   Changes from v1:
>    - Removed tee_remove() function
>    - CONFIG_TEE depends on EXPERT
>    - tee_domain_created() converted to tee_enable()
>    - tee_init() is called using initcall() mechanism
>    - tee_handle_smc() renamed to tee_handle_call()
>
>   Changes from "RFC" version:
>    - renamed CONFIG_ARM_TEE to CONFIG_TEE
>    - changed discovery mechanism: instead of UUID mathing, TEE-specific
>       probing is used
>
>   MAINTAINERS                   |  6 +++
>   xen/arch/arm/Kconfig          |  7 +++
>   xen/arch/arm/Makefile         |  1 +
>   xen/arch/arm/domain.c         |  4 ++
>   xen/arch/arm/domain_build.c   |  4 ++
>   xen/arch/arm/setup.c          |  1 +
>   xen/arch/arm/shutdown.c       |  1 +
>   xen/arch/arm/tee/Makefile     |  1 +
>   xen/arch/arm/tee/tee.c        | 69 ++++++++++++++++++++++++++
>   xen/arch/arm/vsmc.c           |  5 ++
>   xen/arch/arm/xen.lds.S        |  7 +++
>   xen/include/asm-arm/tee/tee.h | 91 +++++++++++++++++++++++++++++++++++
>   12 files changed, 197 insertions(+)
>   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/tee.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 96a0518f49..eac2b40fdf 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -376,6 +376,12 @@ F: config/Stubdom.mk.in
>   F:    m4/stubdom.m4
>   F:    stubdom/
>   
> +TEE MEDIATORS
> +M:     Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
> +S:     Supported
> +F:     xen/arch/arm/tee/
> +F:     xen/include/asm-arm/tee
> +
>   TOOLSTACK
>   M:    Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
>   M:    Wei Liu <wei.liu2@xxxxxxxxxx>
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index 581de67b6b..e527b2f885 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -105,6 +105,13 @@ config HARDEN_BRANCH_PREDICTOR
>   
>   	  If unsure, say Y.
>   
> +config TEE
> +       bool "Enable TEE mediators support" if EXPERT = "y"
> +       default n
> +       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"
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index cb902cb6fe..5c2aa34557 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -5,6 +5,7 @@ subdir-$(CONFIG_ACPI) += acpi
>   ifneq ($(CONFIG_NO_PLAT),y)
>   subdir-y += platforms
>   endif
> +subdir-$(CONFIG_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 1d926dcb29..11b618515b 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -32,6 +32,7 @@
>   #include <asm/platform.h>
>   #include <asm/procinfo.h>
>   #include <asm/regs.h>
> +#include <asm/tee/tee.h>
>   #include <asm/vfp.h>
>   #include <asm/vgic.h>
>   #include <asm/vtimer.h>
> @@ -931,6 +932,9 @@ int domain_relinquish_resources(struct domain *d)
>            */
>           domain_vpl011_deinit(d);
>   
> +        /* Free TEE mediator resources */
> +        tee_domain_destroy(d);
> +
AFAIR, OP-TEE may have a lot of resources attached to it. So I think it 
would be best if we introduce a new RELMEM_* for it and make 
tee_domain_destroy return an int to handle preemption.
>           d->arch.relmem = RELMEM_xen;
>           /* Fallthrough */
>   
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index b0ec3f0b72..39a887b505 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -24,6 +24,7 @@
>   #include <asm/setup.h>
>   #include <asm/cpufeature.h>
>   #include <asm/domain_build.h>
> +#include <asm/tee/tee.h>
>   
>   #include <xen/irq.h>
>   #include <xen/grant_table.h>
> @@ -1962,6 +1963,9 @@ static int __init construct_domain(struct domain *d, 
> struct kernel_info *kinfo)
>       set_current(saved_current);
>       p2m_restore_state(saved_current);
>   
> +    /* Enable TEE */
> +    tee_enable(d);
Why do you need to call TEE here? This should be done through 
arch_domain_create as for any other domain.
> +
>       memset(regs, 0, sizeof(*regs));
>   
>       regs->pc = (register_t)kinfo->entry;
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index e83221ab79..cad568d432 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -48,6 +48,7 @@
>   #include <asm/platform.h>
>   #include <asm/procinfo.h>
>   #include <asm/setup.h>
> +#include <asm/tee/tee.h>
You don't add specific TEE code in that file. So why do you need to 
include it?
>   #include <xsm/xsm.h>
>   #include <asm/acpi.h>
>   
> diff --git a/xen/arch/arm/shutdown.c b/xen/arch/arm/shutdown.c
> index b32f07ec0e..30c69506ff 100644
> --- a/xen/arch/arm/shutdown.c
> +++ b/xen/arch/arm/shutdown.c
> @@ -5,6 +5,7 @@
>   #include <xen/smp.h>
>   #include <asm/platform.h>
>   #include <asm/psci.h>
> +#include <asm/tee/tee.h>
Ditto.

>   
>   static void noreturn halt_this_cpu(void *arg)
>   {
> diff --git a/xen/arch/arm/tee/Makefile b/xen/arch/arm/tee/Makefile
> new file mode 100644
> index 0000000000..c54d4796ff
> --- /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 0000000000..7fd0148b8a
> --- /dev/null
> +++ b/xen/arch/arm/tee/tee.c
> @@ -0,0 +1,69 @@
> +/*
> + * xen/arch/arm/tee/tee.c
> + *
> + * Generic part of TEE mediator subsystem
> + *
> + * Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
> + * Copyright (c) 2018 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.
> + */
> +
> +#include <xen/init.h>
This should be after xen/errno.h

> +#include <xen/errno.h>
> +#include <xen/types.h>
> +#include <asm/tee/tee.h>
> +
> +extern const struct tee_mediator_desc _steemediator[], _eteemediator[];
> +static const struct tee_mediator_ops *mediator_ops;
> +
> +bool tee_handle_call(struct cpu_user_regs *regs)
> +{
> +    if ( !mediator_ops )
> +        return false;
> +
> +    return mediator_ops->handle_call(regs);
> +}
> +
> +int tee_enable(struct domain *d)
I would prefer if you stay consistent with the naming of 
tee_domain_destroy. So this should be called tee_domain_init.
> +    if ( !mediator_ops )
> +        return -ENODEV;
> +
> +    return mediator_ops->enable(d);
> +}
> +
> +void tee_domain_destroy(struct domain *d)
> +{
> +    if ( !mediator_ops )
> +        return;
> +
> +    return mediator_ops->domain_destroy(d);
> +}
> +
> +static int __init tee_init(void)
> +{
> +    const struct tee_mediator_desc *desc;
> +
> +    for ( desc = _steemediator; desc != _eteemediator; desc++ )
For clarity:

{

> +        if ( desc->ops->probe() )
> +        {
> +            printk(XENLOG_INFO "Using TEE mediator for %s\n", desc->name);
> +            mediator_ops = desc->ops;
> +            return 0;
> +        }
}

And add a newline before return.

> +    return 0;
> +}
> +
> +__initcall(tee_init);
> +
> +/*
> + * 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 c4ccae6030..d0199c7874 100644
> --- a/xen/arch/arm/vsmc.c
> +++ b/xen/arch/arm/vsmc.c
> @@ -23,6 +23,7 @@
>   #include <asm/monitor.h>
>   #include <asm/regs.h>
>   #include <asm/smccc.h>
> +#include <asm/tee/tee.h>
>   #include <asm/traps.h>
>   #include <asm/vpsci.h>
>   
> @@ -272,6 +273,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_call(regs);
> +            break;
>           }
>       }
>   
> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
> index 1e72906477..e664c4441a 100644
> --- a/xen/arch/arm/xen.lds.S
> +++ b/xen/arch/arm/xen.lds.S
> @@ -137,6 +137,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/tee.h b/xen/include/asm-arm/tee/tee.h
> new file mode 100644
> index 0000000000..0e8b576372
> --- /dev/null
> +++ b/xen/include/asm-arm/tee/tee.h
> @@ -0,0 +1,91 @@
> +/*
> + * xen/include/asm-arm/tee/tee.h
> + *
> + * Generic part of TEE mediator subsystem
> + *
> + * Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
> + * Copyright (c) 2018 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.
> + */
> +
> +#ifndef __ARCH_ARM_TEE_TEE_H__
> +#define __ARCH_ARM_TEE_TEE_H__
> +
> +#include <xen/lib.h>
> +#include <xen/types.h>
Newline here please.

> +#include <asm/regs.h>
> +
> +#ifdef CONFIG_TEE
> +
> +struct tee_mediator_ops {
See my comments above regarding some of the prototype.

> +    /*
> +     * Probe for TEE. Should return true if TEE found and
> +     * mediator is initialized.
> +     */
> +    bool (*probe)(void);
> +
> +    /*
> +     * Called during domain construction if toolstack requests to enable
> +     * TEE support so mediator can inform TEE about new
> +     * guest and create own structures for the new domain.
> +     */
> +    int (*enable)(struct domain *d);
> +
> +    /*
> +     * Called during domain destruction to inform TEE that guest is now dead
> +     * and to destroy all resources allocated for the domain being destroyed.
> +     */
> +    void (*domain_destroy)(struct domain *d);
> +
> +    /* Handle SMCCC call for current domain. */
> +    bool (*handle_call)(struct cpu_user_regs *regs);
> +};
> +
> +struct tee_mediator_desc {
> +    /* Name of the TEE. Just for debugging purposes. */
> +    const char *name;
> +
> +    /* Mediator callbacks as described above. */
> +    const struct tee_mediator_ops *ops;
> +};
> +
> +bool tee_handle_call(struct cpu_user_regs *regs);
> +int tee_enable(struct domain *d);
> +void tee_domain_destroy(struct domain *d);
> +
> +#define REGISTER_TEE_MEDIATOR(_name, _namestr, _ops)          \
> +static const struct tee_mediator_desc __tee_desc_##_name __used     \
> +__section(".teemediator.info") = {                                  \
> +    .name = _namestr,                                               \
> +    .ops = _ops                                                     \
> +}
> +
> +#else
> +
> +static inline bool tee_handle_call(struct cpu_user_regs *regs)
> +{
> +    return false;
> +}
> +
> +static inline int tee_enable(struct domain *d)
> +{
> +    return -ENODEV;
> +}
> +
> +static inline void tee_domain_destroy(struct domain *d) {}
> +
> +#endif  /* CONFIG_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,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel