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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Volodomyr,

First: thanks a lot for this as having a solution for selecting the tee
for guests on a dom0less configuration was something needed.

Let me answer a bit more on the rest of the patch,

> On 29 May 2024, at 23:34, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx> 
> wrote:
> 
> 
> 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.
> 

I think we need something in tee.c calling on the right tee implementation
depending on what is enabled for the guest as Julien suggested.


> 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.


Regarding the firmware part you can easily handle that by looking for /firmware
and create it if it does not exist before creating your sub-node and this should
be node in the optee node creation function not in tee.c.

> 
>>>  /*
>>>   * 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...

ack

>> 
>>> +    rc = dt_property_read_string(node, "xen,tee", &tee);
>> 
>> ... want to return an error if "xen,tee" exists because CONFIG_TEE is
>> not set.

ack

>> 
>>> +    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.

This is only called for domU anyway so i do not think this check makes sense.

More than that, the fact that we are enabling optee support by default in dom0
might need shortly to be revisited as one might want to have a Xen compiled
with both optee and FF-A so we should have a solution to select which one
(if any) is enabled in dom0 (command line argument or device tree entry).

> 
>> 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.

ack for the panic.

>> 
>>> +            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

Cheers
Bertrand