Hi Julien,
> On 30 May 2024, at 09:59, Julien Grall <[email protected]> wrote:
>
>
>
> On 29/05/2024 22:34, Volodymyr Babchuk wrote:
>> Hi Julien,
>
> Hi Volodymyr,
>
>> Julien Grall <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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
