Hi Julien,
Julien Grall <jul...@xen.org> 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
