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

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hi Julien,

Julien Grall <julien@xxxxxxx> writes:

> Hi Volodymyr,
>
> Can you clarify whether this is intended for the next release cycle?

Well, I don't think that this patch should be committed ASAP if this is
what you are asking about.

> On 29/05/2024 21:43, Volodymyr Babchuk wrote:
>> Allow to provide TEE type for a Dom0less guest via "xen,tee"
>> property. Create appropriate nodes in the guests' device tree and
>> initialize tee subsystem for it.
>
> The new property needs to be documented in
> docs/misc/arm/device-tree/booting.txt.
>

Yes, missed that.

>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
>> ---
>>   xen/arch/arm/dom0less-build.c     | 69 +++++++++++++++++++++++++++++++
>>   xen/arch/arm/include/asm/kernel.h |  3 ++
>>   2 files changed, 72 insertions(+)
>> diff --git a/xen/arch/arm/dom0less-build.c
>> b/xen/arch/arm/dom0less-build.c
>> index fb63ec6fd1..1ea3ecc45c 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)
>>   {
>> @@ -277,6 +278,42 @@ static int __init make_vpl011_uart_node(struct 
>> kernel_info *kinfo)
>>   }
>>   #endif
>>   +#ifdef CONFIG_OPTEE
>> +static int __init make_optee_node(struct kernel_info *kinfo)
>
> Please introduce a callback in the TEE framework that will create the
> OPTEE node.

This is the reason why this is RFC. I wanted to discuss the right method
of doing this. "optee" node should reside in "/firmware/" node as per
device tree bindings. But "/firmware/" node can contain additional
entries, for example linux device tree bindings also define
"/firmware/sdei". So, probably correct solution is to implement function
"make_firmware_node()" in this file, which in turn will call TEE
framework.

But we are making assumption that all TEE implementation will have its
node inside "/firmware/". I am not 100% sure that this is correct. For
example I saw that Google Trusty uses "/trusty" node (directly inside
the DTS root). On other hand, it is not defined in dts bindings, as far
as I know.

>>   /*
>>    * Scan device tree properties for passthrough specific information.
>>    * Returns < 0 on error
>> @@ -650,6 +687,15 @@ static int __init prepare_dtb_domU(struct domain *d, 
>> struct kernel_info *kinfo)
>>       if ( ret )
>>           goto err;
>>   +#ifdef CONFIG_OPTEE
>> +    if ( kinfo->tee_type == XEN_DOMCTL_CONFIG_TEE_OPTEE)
>> +    {
>> +        ret = make_optee_node(kinfo);
>> +        if ( ret )
>> +            goto err;
>> +    }
>> +#endif
>> +
>>       /*
>>        * domain_handle_dtb_bootmodule has to be called before the rest of
>>        * the device tree is generated because it depends on the value of
>> @@ -743,6 +789,9 @@ static int __init construct_domU(struct domain *d,
>>   {
>>       struct kernel_info kinfo = {};
>>       const char *dom0less_enhanced;
>> +#ifdef CONFIG_TEE
>> +    const char *tee;
>> +#endif
>>       int rc;
>>       u64 mem;
>>       u32 p2m_mem_mb;
>> @@ -786,6 +835,18 @@ static int __init construct_domU(struct domain *d,
>>       else if ( rc == 0 && !strcmp(dom0less_enhanced, "no-xenstore") )
>>           kinfo.dom0less_feature = DOM0LESS_ENHANCED_NO_XS;
>>   +#ifdef CONFIG_TEE
>
> I would rather prefer if this code is implemented in tee.c. We also...
>
>> +    rc = dt_property_read_string(node, "xen,tee", &tee);
>
> ... want to return an error if "xen,tee" exists because CONFIG_TEE is
> not set.
>
>> +    if ( rc == -EILSEQ ||
>> +         rc == -ENODATA ||
>> +         (rc == 0 && !strcmp(tee, "none")) )
>> +    {
>> +        if ( !hardware_domain )
>
>
> I don't understand this check. Why would we require dom0 for OP-TEE?

OP-TEE is enabled for Dom0 unconditionally in create_dom0(void);

This is another topic I wanted to discuss, actually, Should we use the
same "xen,tee" for Dom0? In this case we might want to alter Dom0 DTB to
remove TEE entry if user wants it to be disabled.

> In any case we should avoid to hide a feature behind the user back. At
> minimum, we should print a message, but I would rather prefer a
> panic() because the user asks for a feature and we denied it.
>
>> +            kinfo.tee_type = XEN_DOMCTL_CONFIG_TEE_NONE;
> Why don't we have a else case? Are you assuming that tee_type is
> initialized to TEE_NONE (which luckily happens to be 0)? If so, why do
> we need the check above?

Yes, you are right, I'll rework this part.

[...]

>> +    if ( kinfo.tee_type )
>> +    {
>> +        rc = tee_domain_init(d, kinfo.tee_type);
>
> Can you explain why do you need to call tee_domain_init() rather than
> setting d_cfg.arch.tee_type just before domain_create() is called and
> rely on the latter to call the former?
>

Because I was not familiar with dom0less code good enough. Your proposal
is much better, I'll rework this.

-- 
WBR, Volodymyr