Re: [Xen-devel] [PATCH v4 01/10] xen/arm: add generic TEE mediator framework

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 07/03/2019 21:04, Volodymyr Babchuk wrote:
> From: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
>
> 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
s/In/At/

[...]

> 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 6dc633ed50..d1e2a3979d 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>
> @@ -705,6 +706,10 @@ int arch_domain_create(struct domain *d,
>       if ( (rc = domain_vtimer_init(d, &config->arch)) != 0 )
>           goto fail;
>   
> +    if ( config->arch.tee_type == XEN_DOMCTL_CONFIG_TEE_NATIVE )
Please sanitise tee_type in arch_sanitise_domain_config. Also, can't this check 
be pushed in tee_domain_init()?
This would allow us to deal with more tee_type in the future without having to 
extend the check here.
> +        if ( (rc = tee_domain_init(d)) != 0 )
> +            goto fail;
> +
>       update_domain_wallclock_time(d);
>   
>       /*
[...]

> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 444857a967..7602dd990c 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -56,6 +56,9 @@ struct bootinfo __initdata bootinfo;
>   
>   struct cpuinfo_arm __read_mostly boot_cpu_data;
>   
> +static bool __initdata opt_dom0_tee_enabled;
> +boolean_param("dom0_tee_enabled", opt_dom0_tee_enabled);
> +
>   #ifdef CONFIG_ACPI
>   bool __read_mostly acpi_disabled;
>   #endif
> @@ -889,6 +892,11 @@ void __init start_xen(unsigned long boot_phys_offset,
>       /* The vGIC for DOM0 is exactly emulating the hardware GIC */
>       dom0_cfg.arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE;
>       dom0_cfg.arch.nr_spis = gic_number_lines() - 32;
> +    if ( opt_dom0_tee_enabled )
> +        dom0_cfg.arch.tee_type = XEN_DOMCTL_CONFIG_TEE_NATIVE;
> +    else
> +        dom0_cfg.arch.tee_type = XEN_DOMCTL_CONFIG_TEE_NONE;
I don't like the idea to introduce a command line to turn OP-TEE on for Dom0. 
Dom0 should be able to use OP-TEE by default if mediator is present. Otherwise 
you had burden on the user.
We can then decide whether there are reason to allow the user to disable OP-TEE 
support for Dom0.
Also, if you disable OP-TEE to Dom0, then you need to make sure the OP-TEE node 
is not present in the DT. I don't see code doing that.
Lastly, any new option should be described in docs/misc/xen-command-line.pandoc.

> +
>       dom0_cfg.max_vcpus = dom0_max_vcpus();
>   
>       dom0 = domain_create(0, &dom0_cfg, true);
> 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..70432306b9
> --- /dev/null
> +++ b/xen/arch/arm/tee/tee.c
> @@ -0,0 +1,79 @@
> +/*
> + * 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/errno.h>
> +#include <xen/init.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 )
NIT: This could probably be an unlikely as if you end up calling TEE then you 
have more chance to have TEE present.
[...]


> +struct tee_mediator_desc {
> +    /* Name of the TEE. Just for debugging purposes. */
I would drop "Just for debugging purposes". You use it in non-debug build to 
tell what mediator has been used. This could also be useful if we decide to 
support multiple mediators at the same time.
> +    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_domain_init(struct domain *d);
> +int tee_relinquish_resources(struct domain *d);
> +void tee_domain_destroy(struct domain *d);
> +
> +#define REGISTER_TEE_MEDIATOR(_name, _namestr, _ops)          \
Please align \ with the one below.

> +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_domain_init(struct domain *d)
> +{
> +    return -ENODEV;
> +}
> +
> +static inline int tee_relinquish_resources(struct domain *d)
> +{
> +    return 0;
> +}
> +
> +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:
> + */
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index eb424e8286..02aa782e8e 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -304,10 +304,14 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_context_t);
>   #define XEN_DOMCTL_CONFIG_GIC_NATIVE    0
>   #define XEN_DOMCTL_CONFIG_GIC_V2        1
>   #define XEN_DOMCTL_CONFIG_GIC_V3        2
> +#define XEN_DOMCTL_CONFIG_TEE_NONE      0
> +#define XEN_DOMCTL_CONFIG_TEE_NATIVE    1
While native makes sense when you only have one TEE running on the platform. 
This is a bit more difficult to understand if you have multiple TEE in place 
(imagine S-EL2).
I am ok with using the word native in the DOMCTL API because it is only exposed 
between the hypervisor and the tools. But we should be careful on the naming 
used with the user (i.e xl interface or xen command line).
>   struct xen_arch_domainconfig {
>       /* IN/OUT */
>       uint8_t gic_version;
>       /* IN */
> +    uint8_t tee_type;
> +    /* IN */
>       uint32_t nr_spis;
>       /*
>        * OUT
Cheers,

--
Julien Grall

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