[U-Boot] [PATCH v2 3/4] image: fdt: copy possible optee nodes to a loaded devicetree

[Patrick DELAUNAY]

Hi Heiko,

> 
> Hi Patrick,
> 
> Am Mittwoch, 23. Oktober 2019, 09:10:52 CEST schrieb Patrick DELAUNAY:
> > Hi Jens and Heiko,
> >
> > > From: U-Boot <u-boot-bounces at lists.denx.de> On Behalf Of Jens
> > > Wiklander
> > > Sent: mercredi 23 octobre 2019 08:46
> > >
> > > On Tue, Oct 22, 2019 at 09:04:27PM +0200, Heiko Stuebner wrote:
> > > > From: Heiko Stuebner <heiko.stuebner at theobroma-systems.com>
> > > >
> > > > The loading convention for optee or any other tee on arm64 is as
> > > > bl32 parameter to the trusted-firmware. So TF-A gets invoked with
> > > > the TEE as
> > > > bl32 and main u-boot as bl33. Once it has done its startup TF-A
> > > > jumps into the bl32 for the TEE startup, returns to TF-A and then jumps to
> bl33.
> > > >
> > > > All of them get passed a devicetree as parameter and all
> > > > components often get loaded from a FIT image.
> > > >
> > > > OP-TEE will create additional nodes in that devicetree namely a
> > > > firmware node and possibly multiple reserved-memory nodes.
> > > >
> > > > While this devicetree is used in main u-boot, in most cases it
> > > > won't be the one passed to the actual kernel. Instead most boot
> > > > commands will load a new devicetree from somewhere like mass
> > > > storage of the network, so if that happens u-boot should transfer
> > > > the optee nodes to that new
> > > devicetree.
> > > >
> > > > To make that happen introduce optee_copy_fdt_nodes() called from
> > > > the dt setup function in image-fdt which after checking for the
> > > > optee presence in the u-boot dt will make sure a optee node is
> > > > present in the kernel dt and transfer any reserved-memory regions it can find.
> > > >
> > > > Signed-off-by: Heiko Stuebner
> > > > <heiko.stuebner at theobroma-systems.com>
> > > > ---
> > > > changes in v2:
> > > > - don't create a new optee firmware-node, but instead copy the
> > > >   compatible+method properties from the old fdt blob.
> > > >
> > > >  common/image-fdt.c  |   8 +++
> > > >  include/tee/optee.h |   9 +++
> > > >  lib/optee/optee.c   | 132
> ++++++++++++++++++++++++++++++++++++++++++++
> > > >  3 files changed, 149 insertions(+)
> > > >
> > > [snip]
> >
> > On STM32MP1 platform (armv7 with TF-A support), we can use BL32 =
> > OP-TEE or spmin provide by TF-A
> 
> That is the same I'm hoping to have at some point for the RK3288 soc I'm also
> working on :-D .
> 
> But why do you need different handling then? Except BL32 being baked into the
> TF-A binary on arm32, shouldn't the loading patch be somewhat similar (SPL ->
> TF-A -> OP-Tee -> U-Boot) so that OP-Tee also can insert its nodes there?

We have official 2 boot chains support by STM32MP15 serie in U-Boot

ROM code => TF-A (BL2 & spmin) => U-Boot => kernel
= stm32mp15_trusted_defconfig

ROM code => TF-A (BL2) => OP-TEE => U-Boot => kernel
= stm32mp15_optee_defconfig

We have also SPL but only for U-Boot test and with 
minimal PSCI support in u-boot (no low-power mode,
no security support)

ROM code => SPL => U-Boot (install PSCI) => kernel
= stm32mp15_basic_defconfig

For details:
https://wiki.st.com/stm32mpu/index.php/Boot_chains_overview#STM32MP15_case


> (Haven't looked too deeply at this yet)
> 
> > So we are plan to have the same type of feature but with an inversed logical:
> >
> > - Op-Tee nodes (firmwares and reserved) are present in kernel device
> > tree
> > - U-Boot deactivated these nodes when TEE is not present, with the next
> function called in ft_system_setup:
> 
> Having the OP-Tee nodes hard-coded in the devicetree in kernel sources sounds
> somewhat counter intuitive to me.
> 
> For the firmware node it shouldn't matter much, except that there may also be
> other TEEs other than OP-Tee that people may want to load.
> 
> But when the reserved memory is hard-coded in the kernel, you bind yourself to
> one specific version of your TEE. I.e. what happens when a future version of your
> platform support wants to move the memory region OP-Tee wants to occupy. With
> OP-Tee handling the reservation this should be somewhat transparent to
> bootloader and kernel, as just the reserved-memory changes.

I don't check the code and architecture of OP-TEE since a long time
and with your answer I just discover the non-secure device tree 
modification done by the OP-Tee (even it is present since v2.1.0)

> 
> > static void stm32_fdt_disable_optee(void *blob) {
> > 	int off, node;
> >
> > 	off = fdt_node_offset_by_compatible(blob, -1, "linaro,optee-tz");
> > 	if (off >= 0 && fdtdec_get_is_enabled(blob, off))
> > 		fdt_status_disabled(blob, off);
> >
> > 	/* Disabled "optee at ..." reserved-memory node */
> > 	off = fdt_path_offset(blob, "/reserved-memory/");
> > 	if (off < 0)
> > 		return;
> > 	for (node = fdt_first_subnode(blob, off);
> > 	     node >= 0;
> > 	     node = fdt_next_subnode(blob, node)) {
> > 		if (!strncmp(fdt_get_name(blob, node, NULL), "optee@", 6))
> > 			fdt_status_disabled(blob, node);
> > 	}
> > }
> >
> > What is  the better for you ?
> > 1/ Copy the U-Boot op-tee nodes in kernel device tree (where op-tee
> > nodes was present) or 2/ Deactivate the op-tee nodes in kernel device
> > tree (where op-tee nodes are present)
> >
> > The advantage of us is to upstream a Linux Kernel device tree with the correct
> op-tee nodes...
> 
> In any case, the copying we're doing here should not affect any present OP-Tee
> nodes in the kernel dts and only proceed if both
> (a) U-Boot dts does contain an OP-Tee node
> (b) Kernel dts does _not yet_ contain an OP-Tee node
> 
> are valid.


Now I understood your patch....

I need to check to correctly handle this behavior on my side
as stm32mp15 TF-A don't using FIT and SPL is not used.

But I think that the U-Boot device tree need to be loaded by BL2 and no more integrated in
U-Boot image thus OP-Tee can modify it.

ROM code => TF-A (BL2) = Load TF-A spmin & U-Boot & Device Tree  
			   => spmin => U-Boot => kernel

ROM code => TF-A (BL2) = Load OP-TEE & U-Boot & Device Tree 
                                          => OP-TEE => U-Boot => kernel
 
> 
> Heiko
> 

Thanks again for the answer.

Patrick