Re: [RFC PATCH v2] arm: dom0less: add TEE support

[Julien Grall <julien@xxxxxxx>]

Hi Volodymyr,

On 31/05/2024 18:49, Volodymyr Babchuk wrote:
> Extend TEE mediator interface with two functions :
>
>   - tee_get_type_from_dts() returns TEE type based on input string
>   - tee_make_dtb_node() creates a DTB entry for the selected
>     TEE mediator
>
> Use those new functions to parse "xen,tee" DTS property for dom0less
> guests and enable appropriate TEE mediator.
>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
>
> ---
>
> This is still RFC because I am not happy how I decide if
> tee_make_dtb_node() needs to be called.
>
> TEE type is stored in d_cfg, but d_cfg is not passed to
> construct_domU()->prepare_dtb_domU(). So right now I am relying on
> fact that every TEE mediator initilizes d->arch.tee.
>
> Also I am sorry about previous completely botched version of this
> patch. I really messed it up, including absence of [RFC] tag :(
That's fine. We all sent botched patches at least once :). Some comments 
below on the series.
> ---
>   docs/misc/arm/device-tree/booting.txt | 17 +++++++++++++
>   xen/arch/arm/dom0less-build.c         | 19 +++++++++++++++
>   xen/arch/arm/include/asm/tee/tee.h    | 13 ++++++++++
>   xen/arch/arm/tee/ffa.c                |  8 ++++++
>   xen/arch/arm/tee/optee.c              | 35 +++++++++++++++++++++++++++
>   xen/arch/arm/tee/tee.c                | 21 ++++++++++++++++
>   6 files changed, 113 insertions(+)
>
> diff --git a/docs/misc/arm/device-tree/booting.txt 
> b/docs/misc/arm/device-tree/booting.txt
> index bbd955e9c2..711a6080b5 100644
> --- a/docs/misc/arm/device-tree/booting.txt
> +++ b/docs/misc/arm/device-tree/booting.txt
> @@ -231,6 +231,23 @@ with the following properties:
>       In the future other possible property values might be added to
>       enable only selected interfaces.
>   
> +- xen,tee
> +
> +    A string property that describes what TEE mediator should be enabled
> +    for the domain. Possible property values are:
> +
> +    - "none" (or missing property value)
> +    No TEE will be available in the VM.
> +
> +    - "OP-TEE"
> +    VM will have access to the OP-TEE using classic OP-TEE SMC interface.
> +
> +    - "FF-A"
> +    VM will have access to a TEE using generic FF-A interface.
I understand why you chose those name, but it also means we are using 
different name in XL and Dom0less. I would rather prefer if they are the 
same.
> +
> +    In the future other TEE mediators may be added, extending possible
> +    values for this property.
> +
>   - xen,domain-p2m-mem-mb
>   
>       Optional. A 32-bit integer specifying the amount of megabytes of RAM
> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
> index fb63ec6fd1..13fdd44eef 100644
> --- a/xen/arch/arm/dom0less-build.c
> +++ b/xen/arch/arm/dom0less-build.c
> @@ -15,6 +15,7 @@
>   #include <asm/domain_build.h>
>   #include <asm/static-memory.h>
>   #include <asm/static-shmem.h>
> +#include <asm/tee/tee.h>
>   
>   bool __init is_dom0less_mode(void)
>   {
> @@ -650,6 +651,10 @@ static int __init prepare_dtb_domU(struct domain *d, 
> struct kernel_info *kinfo)
>       if ( ret )
>           goto err;
>   
> +    /* We are making assumption that every mediator sets d->arch.tee */
> +    if ( d->arch.tee )
I think the assumption is Ok. I would consider to move this check in 
each TEE callback. IOW call tee_make_dtb_node() unconditionally.
> +        tee_make_dtb_node(kinfo->fdt);
AFAICT, tee_make_dtb_node() can return an error. So please check the 
return value.
> +
>       /*
>        * domain_handle_dtb_bootmodule has to be called before the rest of
>        * the device tree is generated because it depends on the value of
> @@ -871,6 +876,7 @@ void __init create_domUs(void)
>           unsigned int flags = 0U;
>           uint32_t val;
>           int rc;
> +        const char *tee_name;
>   
>           if ( !dt_device_is_compatible(node, "xen,domain") )
>               continue;
> @@ -881,6 +887,19 @@ void __init create_domUs(void)
>           if ( dt_find_property(node, "xen,static-mem", NULL) )
>               flags |= CDF_staticmem;
>   
> +        tee_name = dt_get_property(node, "xen,tee", NULL);
In the previous version, you used dt_property_read_property_string() 
which contained some sanity check. Can you explain why you switch to 
dt_get_property()?
> +        if ( tee_name )
> +        {
> +            rc = tee_get_type_from_dts(tee_name);
> +            if ( rc < 0) > +                panic("Can't enable requested TEE for domain: %d\n", 
rc);> +            d_cfg.arch.tee_type = rc;
> +        }
> +        else
> +        {
NIT: The parentheses are not necessary.

> +            d_cfg.arch.tee_type = XEN_DOMCTL_CONFIG_TEE_NONE;
> +        }
> +
>           if ( dt_property_read_bool(node, "direct-map") )
>           {
>               if ( !(flags & CDF_staticmem) )
Cheers,

---
Julien Grall