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

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hi Julien,

Julien Grall <julien@xxxxxxx> writes:

> Hi Volodymyr,
>
> On 31/05/2024 18:49, Volodymyr Babchuk wrote:
>> Extend TEE mediator interface with two functions :
>>   - tee_get_type_from_dts() returns TEE type based on input string
>>   - tee_make_dtb_node() creates a DTB entry for the selected
>>     TEE mediator
>> Use those new functions to parse "xen,tee" DTS property for dom0less
>> guests and enable appropriate TEE mediator.
[...]

>> +
>> +    A string property that describes what TEE mediator should be enabled
>> +    for the domain. Possible property values are:
>> +
>> +    - "none" (or missing property value)
>> +    No TEE will be available in the VM.
>> +
>> +    - "OP-TEE"
>> +    VM will have access to the OP-TEE using classic OP-TEE SMC interface.
>> +
>> +    - "FF-A"
>> +    VM will have access to a TEE using generic FF-A interface.
>
> I understand why you chose those name, but it also means we are using
> different name in XL and Dom0less. I would rather prefer if they are
> the same.
>

Well, my first idea was to introduce additional "const char *dts_name"
for a TEE mediator description. But it seems redundant. I can rename
existing mediators so their names will correspond to names used by libxl.

>> +
>> +    In the future other TEE mediators may be added, extending possible
>> +    values for this property.
>> +
>>   - xen,domain-p2m-mem-mb
>>         Optional. A 32-bit integer specifying the amount of
>> megabytes of RAM
>> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
>> index fb63ec6fd1..13fdd44eef 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)
>>   {
>> @@ -650,6 +651,10 @@ static int __init prepare_dtb_domU(struct domain *d, 
>> struct kernel_info *kinfo)
>>       if ( ret )
>>           goto err;
>>   +    /* We are making assumption that every mediator sets
>> d->arch.tee */
>> +    if ( d->arch.tee )
>
> I think the assumption is Ok. I would consider to move this check in
> each TEE callback. IOW call tee_make_dtb_node() unconditionally.
>

Ah, okay, makes sense.

>> +        tee_make_dtb_node(kinfo->fdt);
>
> AFAICT, tee_make_dtb_node() can return an error. So please check the
> return value.
>

Yes, you are right.

>> +
>>       /*
>>        * domain_handle_dtb_bootmodule has to be called before the rest of
>>        * the device tree is generated because it depends on the value of
>> @@ -871,6 +876,7 @@ void __init create_domUs(void)
>>           unsigned int flags = 0U;
>>           uint32_t val;
>>           int rc;
>> +        const char *tee_name;
>>             if ( !dt_device_is_compatible(node, "xen,domain") )
>>               continue;
>> @@ -881,6 +887,19 @@ void __init create_domUs(void)
>>           if ( dt_find_property(node, "xen,static-mem", NULL) )
>>               flags |= CDF_staticmem;
>>   +        tee_name = dt_get_property(node, "xen,tee", NULL);
>
> In the previous version, you used dt_property_read_property_string()
> which contained some sanity check. Can you explain why you switch to
> dt_get_property()?

Because I was confused by dt_property_read_string() return values.

I made a fresh look at it and now I understand that I need to test for
-EINVAL to determine that a property is not available and I should use
a default value. All other return codes should cause panic. I'll rework
this in the next version.

-- 
WBR, Volodymyr