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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Julien,

> On 30 May 2024, at 09:59, Julien Grall <julien@xxxxxxx> wrote:
> 
> 
> 
> On 29/05/2024 22:34, Volodymyr Babchuk wrote:
>> Hi Julien,
> 
> Hi Volodymyr,
> 
>> 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.
> 
> If this is meant an RFC, then I would recommend to tag your series with RFC. 
> Similarly...
> 
> 
>> I wanted to discuss the right method
>> of doing this.
> 
> ... if you have any open questions. Then please write them down after the 
> "---" (so they are not committed). So this is not a guessing game for the 
> reviewer.
> 
> For instance, if you hadn't asked the question, I wouldn't have realized you 
> were not entirely happy with your solution. To me it looked fine because this 
> is self-contained in an OP-TEE specific function.
> 
>> "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.
> 
> Longer term possibly. But at the moment, we have no implementation of the 
> "sdei" node and I am not aware of any future plan. So I don't think it is 
> necessary to split the work in two functions.
> 
>> 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.
> 
> TBH, if there is no official binding documentation, then Xen cannot sensibly 
> create those nodes because they are not "stable". So the first step would be 
> to have official binding.
> 
> 
> Bertrand, I couldn't find any documentation for the FFA binding. Do you know 
> if they will be created under /firmware?

There is not device tree entry needed for FF-A because it is detected through a 
FF-A call.

> 
>>>>   /*
>>>>    * 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);
> 
> I am sorry but this still doesn't make sense. AFAICT, this path is only used 
> by domU. In some dom0less setup, we may not have dom0 at all. So why do you 
> want to prevent OP-TEE for such case?
> 
> Or are you intending to check that "d" is not the hardware domain? If so, you 
> have the wrong check (you want to check is_hardware_domain(d) and AFAIK this 
> path is not called for dom0.
> 
>> 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.
> Is there any existing use case to disable OP-TEE in dom0? I am asking because 
> removing the nodes will make the code a bit more complicated. So if there is 
> no need, then my preference is to not do it.

I would say there are several:
- optee not supported in Xen (dom0 cannot access it anyway)
- optee to be used in a guest instead of dom0
- ff-a used to communicate with optee (in this case optee support is not used 
but ff-a is).

On this subject, I will not ask you to add support for FF-A for this but 
whatever you do please keep in mind
that we will probably add the same for FF-A so that we end up with something 
coherent where the tee can
be selected by configuration for guests or by device tree for dom0 or dom0less 
guests.

I will make a path on the next version of the patch for this.

Cheers
Bertrand