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.

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
 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)
+{
+    void *fdt = kinfo->fdt;
+    int res;
+
+    res = fdt_begin_node(fdt, "firmware");
+    if ( res )
+        return res;
+
+    res = fdt_begin_node(fdt, "optee");
+    if ( res )
+        return res;
+
+    res = fdt_property_string(fdt, "compatible", "linaro,optee-tz");
+    if ( res )
+        return res;
+
+    res = fdt_property_string(fdt, "method", "hvc");
+    if ( res )
+        return res;
+
+    /* end of "optee" */
+    res = fdt_end_node(fdt);
+    if ( res )
+        return res;
+
+    /* end of "firmware" */
+    res = fdt_end_node(fdt);
+    if ( res )
+        return res;
+
+    return 0;
+}
+#endif
+
 /*
  * 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
+    rc = dt_property_read_string(node, "xen,tee", &tee);
+    if ( rc == -EILSEQ ||
+         rc == -ENODATA ||
+         (rc == 0 && !strcmp(tee, "none")) )
+    {
+        if ( !hardware_domain )
+            kinfo.tee_type = XEN_DOMCTL_CONFIG_TEE_NONE;
+    }
+    else if ( rc == 0 && !strcmp(tee, "optee") )
+        kinfo.tee_type = XEN_DOMCTL_CONFIG_TEE_OPTEE;
+#endif
     if ( vcpu_create(d, 0) == NULL )
         return -ENOMEM;
 
@@ -824,6 +885,14 @@ static int __init construct_domU(struct domain *d,
             return rc;
     }
 
+#ifdef CONFIG_TEE
+    if ( kinfo.tee_type )
+    {
+        rc = tee_domain_init(d, kinfo.tee_type);
+        if ( rc < 0 )
+            return rc;
+    }
+#endif
     rc = prepare_dtb_domU(d, &kinfo);
     if ( rc < 0 )
         return rc;
diff --git a/xen/arch/arm/include/asm/kernel.h b/xen/arch/arm/include/asm/kernel.h
index 0a23e86c2d..7e7b3f4d56 100644
--- a/xen/arch/arm/include/asm/kernel.h
+++ b/xen/arch/arm/include/asm/kernel.h
@@ -57,6 +57,9 @@ struct kernel_info {
     /* Enable pl011 emulation */
     bool vpl011;
 
+    /* TEE type */
+    uint16_t tee_type;
+
     /* Enable/Disable PV drivers interfaces */
     uint16_t dom0less_feature;
 
-- 
2.45.1

Hi Volodymyr,

Can you clarify whether this is intended for the next release cycle?

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.

> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
>   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.

[...]

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

> +    }
> +    else if ( rc == 0 && !strcmp(tee, "optee") )
> +        kinfo.tee_type = XEN_DOMCTL_CONFIG_TEE_OPTEE;

All the other values should be rejected.

> +#endif
>       if ( vcpu_create(d, 0) == NULL )
>           return -ENOMEM;
>   
> @@ -824,6 +885,14 @@ static int __init construct_domU(struct domain *d,
>               return rc;
>       }
>   
> +#ifdef CONFIG_TEE
> +    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?

> +        if ( rc < 0 )
> +            return rc;
> +    }
> +#endif
>       rc = prepare_dtb_domU(d, &kinfo);
>       if ( rc < 0 )
>           return rc;
> diff --git a/xen/arch/arm/include/asm/kernel.h b/xen/arch/arm/include/asm/kernel.h
> index 0a23e86c2d..7e7b3f4d56 100644
> --- a/xen/arch/arm/include/asm/kernel.h
> +++ b/xen/arch/arm/include/asm/kernel.h
> @@ -57,6 +57,9 @@ struct kernel_info {
>       /* Enable pl011 emulation */
>       bool vpl011;
>   
> +    /* TEE type */
> +    uint16_t tee_type;
> +
>       /* Enable/Disable PV drivers interfaces */
>       uint16_t dom0less_feature;
>   

Cheers,

-- 
Julien Grall

Hi Julien,

Julien Grall <julien@xen.org> 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@epam.com>
>> ---
>>   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

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@epam.com> wrote:
> 
> 
> Hi Julien,
> 
> Julien Grall <julien@xen.org> 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@epam.com>
>>> ---
>>>  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

Hi Bertrand,

On 30/05/2024 10:40, Bertrand Marquis wrote:
>> 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.

This would work if the node /firmware doesn't exist. But how would you 
handle the case where it is already present?

I looked at the libfdt API and AFAICT the DTB creation needs to be 
linear. IOW, you can't add a subnode to an already created node.

There is an helper to create a placeholder, but AFAIK this is only for a 
property. You also need to know the size in advance.

Cheers,

-- 
Julien Grall

Hi Julien,

> On 30 May 2024, at 12:35, Julien Grall <julien@xen.org> wrote:
> 
> Hi Bertrand,
> 
> On 30/05/2024 10:40, Bertrand Marquis wrote:
>>> 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.
> 
> This would work if the node /firmware doesn't exist. But how would you handle the case where it is already present?
> 
> I looked at the libfdt API and AFAICT the DTB creation needs to be linear. IOW, you can't add a subnode to an already created node.
> 
> There is an helper to create a placeholder, but AFAIK this is only for a property. You also need to know the size in advance.

I thought it was possible but i definitely can be wrong.

As right now we have only one need for the node, we could delay a possible solution and just create it in the optee driver.
Designing a solution for a possible future case right now seems a bit complex without a use case.

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall

Hi Bertrand,

On 30/05/2024 14:22, Bertrand Marquis wrote:
>> On 30 May 2024, at 12:35, Julien Grall <julien@xen.org> wrote:
>>
>> Hi Bertrand,
>>
>> On 30/05/2024 10:40, Bertrand Marquis wrote:
>>>> 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.
>>
>> This would work if the node /firmware doesn't exist. But how would you handle the case where it is already present?
>>
>> I looked at the libfdt API and AFAICT the DTB creation needs to be linear. IOW, you can't add a subnode to an already created node.
>>
>> There is an helper to create a placeholder, but AFAIK this is only for a property. You also need to know the size in advance.
> 
> I thought it was possible but i definitely can be wrong.
> 
> As right now we have only one need for the node, we could delay a possible solution and just create it in the optee driver.

I am fine with that.

Cheers,

-- 
Julien Grall

On 29/05/2024 22:34, Volodymyr Babchuk wrote:
> 
> Hi Julien,

Hi Volodymyr,

> 
> Julien Grall <julien@xen.org> 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@epam.com>
>>> ---
>>>    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?

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

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

The only difference between a domain created from Xen and the tools and 
where the parsing of the configuration is done. Other than they the 
creation happens exactly the same way (e.g. domain_create() is called)

Cheers,

-- 
Julien Grall

Hi Julien,

> On 30 May 2024, at 09:59, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 29/05/2024 22:34, Volodymyr Babchuk wrote:
>> Hi Julien,
> 
> Hi Volodymyr,
> 
>> Julien Grall <julien@xen.org> 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@epam.com>
>>>> ---
>>>>   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