| Message ID | 20240531174915.1679443-1-volodymyr_babchuk@epam.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,v2] arm: dom0less: add TEE support | expand |
Hello all, Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> writes: > 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 > [..] > 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; > I forgot to add #ifdef CONFIG_TEE > + /* We are making assumption that every mediator sets d->arch.tee */ > + if ( d->arch.tee ) > + tee_make_dtb_node(kinfo->fdt); > + #endif So build fails if TEE is disabled. I'll fix this in the next version. Anyways, this is RFC. > /* > * 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; [...]
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. > > Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> > > --- > > This is still RFC because I am not happy how I decide if > tee_make_dtb_node() needs to be called. > > TEE type is stored in d_cfg, but d_cfg is not passed to > construct_domU()->prepare_dtb_domU(). So right now I am relying on > fact that every TEE mediator initilizes d->arch.tee. > > Also I am sorry about previous completely botched version of this > patch. I really messed it up, including absence of [RFC] tag :( That's fine. We all sent botched patches at least once :). Some comments below on the series. > > --- > docs/misc/arm/device-tree/booting.txt | 17 +++++++++++++ > xen/arch/arm/dom0less-build.c | 19 +++++++++++++++ > xen/arch/arm/include/asm/tee/tee.h | 13 ++++++++++ > xen/arch/arm/tee/ffa.c | 8 ++++++ > xen/arch/arm/tee/optee.c | 35 +++++++++++++++++++++++++++ > xen/arch/arm/tee/tee.c | 21 ++++++++++++++++ > 6 files changed, 113 insertions(+) > > diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt > index bbd955e9c2..711a6080b5 100644 > --- a/docs/misc/arm/device-tree/booting.txt > +++ b/docs/misc/arm/device-tree/booting.txt > @@ -231,6 +231,23 @@ with the following properties: > In the future other possible property values might be added to > enable only selected interfaces. > > +- xen,tee > + > + 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. > + > + 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. > + tee_make_dtb_node(kinfo->fdt); AFAICT, tee_make_dtb_node() can return an error. So please check the return value. > + > /* > * 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()? > + if ( tee_name ) > + { > + rc = tee_get_type_from_dts(tee_name); > + if ( rc < 0) > + panic("Can't enable requested TEE for domain: %d\n", rc);> + d_cfg.arch.tee_type = rc; > + } > + else > + { NIT: The parentheses are not necessary. > + d_cfg.arch.tee_type = XEN_DOMCTL_CONFIG_TEE_NONE; > + } > + > if ( dt_property_read_bool(node, "direct-map") ) > { > if ( !(flags & CDF_staticmem) ) Cheers, --- Julien Grall
Hi Julien, Julien Grall <julien@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.
Hi Volodymyr, On 04/06/2024 12:56, Volodymyr Babchuk wrote: > > Hi Julien, > > Julien Grall <julien@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. The field name is only used for printing. So I think it would be ok to rename the values. It would also be good to update the comment on top of the definition of the field "name" so it is clear the name will be used by dom0less. Cheers,
diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt index bbd955e9c2..711a6080b5 100644 --- a/docs/misc/arm/device-tree/booting.txt +++ b/docs/misc/arm/device-tree/booting.txt @@ -231,6 +231,23 @@ with the following properties: In the future other possible property values might be added to enable only selected interfaces. +- xen,tee + + 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. + + 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 ) + tee_make_dtb_node(kinfo->fdt); + /* * 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); + if ( tee_name ) + { + rc = tee_get_type_from_dts(tee_name); + if ( rc < 0) + panic("Can't enable requested TEE for domain: %d\n", rc); + d_cfg.arch.tee_type = rc; + } + else + { + d_cfg.arch.tee_type = XEN_DOMCTL_CONFIG_TEE_NONE; + } + if ( dt_property_read_bool(node, "direct-map") ) { if ( !(flags & CDF_staticmem) ) diff --git a/xen/arch/arm/include/asm/tee/tee.h b/xen/arch/arm/include/asm/tee/tee.h index da324467e1..9626667545 100644 --- a/xen/arch/arm/include/asm/tee/tee.h +++ b/xen/arch/arm/include/asm/tee/tee.h @@ -36,6 +36,9 @@ struct tee_mediator_ops { int (*domain_init)(struct domain *d); int (*domain_teardown)(struct domain *d); + /* Make DTB node that describes TEE. Used when creating a dom0less domain */ + int (*make_dtb_node)(void *fdt); + /* * Called during domain destruction to relinquish resources used * by mediator itself. This function can return -ERESTART to indicate @@ -65,7 +68,9 @@ bool tee_handle_call(struct cpu_user_regs *regs); int tee_domain_init(struct domain *d, uint16_t tee_type); int tee_domain_teardown(struct domain *d); int tee_relinquish_resources(struct domain *d); +int tee_make_dtb_node(void *fdt); uint16_t tee_get_type(void); +int tee_get_type_from_dts(const char* prop_value); #define REGISTER_TEE_MEDIATOR(_name, _namestr, _type, _ops) \ static const struct tee_mediator_desc __tee_desc_##_name __used \ @@ -105,6 +110,14 @@ static inline uint16_t tee_get_type(void) return XEN_DOMCTL_CONFIG_TEE_NONE; } +static inline int tee_get_type_from_dts(const char* prop_value) +{ + if ( !strcmp(prop_value, "none") ) + return XEN_DOMCTL_CONFIG_TEE_NONE; + + return -ENODEV; +} + #endif /* CONFIG_TEE */ #endif /* __ARCH_ARM_TEE_TEE_H__ */ diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c index 0793c1c758..f315d6eef6 100644 --- a/xen/arch/arm/tee/ffa.c +++ b/xen/arch/arm/tee/ffa.c @@ -1752,6 +1752,13 @@ err_free_ffa_rx: return false; } +static int ffa_make_dtb_node(void *fdt) +{ + /* FF-A is not configured via dtb */ + + return 0; +} + static const struct tee_mediator_ops ffa_ops = { .probe = ffa_probe, @@ -1759,6 +1766,7 @@ static const struct tee_mediator_ops ffa_ops = .domain_teardown = ffa_domain_teardown, .relinquish_resources = ffa_relinquish_resources, .handle_call = ffa_handle_call, + .make_dtb_node = ffa_make_dtb_node, }; REGISTER_TEE_MEDIATOR(ffa, "FF-A", XEN_DOMCTL_CONFIG_TEE_FFA, &ffa_ops); diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c index 5151bd90ed..5eb5d01813 100644 --- a/xen/arch/arm/tee/optee.c +++ b/xen/arch/arm/tee/optee.c @@ -28,6 +28,7 @@ #include <xen/domain_page.h> #include <xen/err.h> #include <xen/guest_access.h> +#include <xen/libfdt/libfdt.h> #include <xen/mm.h> #include <xen/sched.h> @@ -1722,6 +1723,39 @@ static bool optee_handle_call(struct cpu_user_regs *regs) } } +static int __init optee_make_dtb_node(void *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; +} + static const struct tee_mediator_ops optee_ops = { .probe = optee_probe, @@ -1729,6 +1763,7 @@ static const struct tee_mediator_ops optee_ops = .domain_teardown = optee_domain_teardown, .relinquish_resources = optee_relinquish_resources, .handle_call = optee_handle_call, + .make_dtb_node = optee_make_dtb_node, }; REGISTER_TEE_MEDIATOR(optee, "OP-TEE", XEN_DOMCTL_CONFIG_TEE_OPTEE, &optee_ops); diff --git a/xen/arch/arm/tee/tee.c b/xen/arch/arm/tee/tee.c index ddd17506a9..6388166e17 100644 --- a/xen/arch/arm/tee/tee.c +++ b/xen/arch/arm/tee/tee.c @@ -68,6 +68,14 @@ int tee_relinquish_resources(struct domain *d) return cur_mediator->ops->relinquish_resources(d); } +int tee_make_dtb_node(void *fdt) +{ + if ( !cur_mediator ) + return -ENODEV; + + return cur_mediator->ops->make_dtb_node(fdt); +} + uint16_t tee_get_type(void) { if ( !cur_mediator ) @@ -76,6 +84,19 @@ uint16_t tee_get_type(void) return cur_mediator->tee_type; } +int tee_get_type_from_dts(const char* prop_value) +{ + if ( !strcmp(prop_value, "none") ) + return XEN_DOMCTL_CONFIG_TEE_NONE; + + if ( !cur_mediator ) + return -ENODEV; + + if ( !strcmp(prop_value, cur_mediator->name) ) + return cur_mediator->tee_type; + + return -ENODEV; +} static int __init tee_init(void) {
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. Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> --- This is still RFC because I am not happy how I decide if tee_make_dtb_node() needs to be called. TEE type is stored in d_cfg, but d_cfg is not passed to construct_domU()->prepare_dtb_domU(). So right now I am relying on fact that every TEE mediator initilizes d->arch.tee. Also I am sorry about previous completely botched version of this patch. I really messed it up, including absence of [RFC] tag :( --- docs/misc/arm/device-tree/booting.txt | 17 +++++++++++++ xen/arch/arm/dom0less-build.c | 19 +++++++++++++++ xen/arch/arm/include/asm/tee/tee.h | 13 ++++++++++ xen/arch/arm/tee/ffa.c | 8 ++++++ xen/arch/arm/tee/optee.c | 35 +++++++++++++++++++++++++++ xen/arch/arm/tee/tee.c | 21 ++++++++++++++++ 6 files changed, 113 insertions(+)