stm32mp: The purpose of "!tee_find_device()"

Etienne Carriere etienne.carriere at linaro.org
Wed Nov 4 22:28:07 CET 2020 

Hi Alex,

On Wed, 4 Nov 2020 at 20:55, Alex G. <mr.nuke.me at gmail.com> wrote:
>
>
>
> On 11/4/20 1:07 AM, Etienne Carriere wrote:
> > On Tue, 3 Nov 2020 at 16:53, Alex G. <mr.nuke.me at gmail.com> wrote:
> >>
> >> On 10/30/20 3:28 AM, Etienne Carriere wrote:
> >>> On Thu, 29 Oct 2020 at 15:33, Alex G. <mr.nuke.me at gmail.com> wrote:
> >>>>
> >>>> On 9/30/20 6:03 PM, Alex G. wrote:
> >>>>> Hi
> >>>>>
> >>>>> I'm trying to wrap my head around the purpose of the following lines in
> >>>>> ft_system_setup():
> >>>>>
> >>>>>        if (!CONFIG_IS_ENABLED(OPTEE) ||
> >>>>>            !tee_find_device(NULL, NULL, NULL, NULL))
> >>>>>            stm32_fdt_disable_optee(blob);


Here (in mach-stm32mp/fdt.c), maybe to skip runtime probe
when built for SPL.

+        if (!CONFIG_IS_ENABLED(SPL_BUILD) &&
             (!CONFIG_IS_ENABLED(OPTEE) ||
              !tee_find_device(NULL, NULL, NULL, NULL))
                 stm32_fdt_disable_optee(blob);


> >>>> Hi! Me again! Do we have a (good) reason for this, or should I submit a
> >>>> patch to remove this problematic code?
> >>>>
> >>>> Alex
> >>>>
> >>>>> My interpretation is "if optee is not running, delete the FDT node".
> >>>>> The problem is that tee_find_device() invokes device_probe(). This in
> >>>>> turn does an SMC call. This call results in an abort and reboot if optee
> >>>>> is not running in the first place.
> >>>>>
> >>>>> So I don't think that tee_find_device() can be used as a check for "Is
> >>>>> optee running?". Exhibit B: Outside of mach-stm32mp, tee_find_device()
> >>>>> is used to obtain of a _working_ TEE node, not to ask if "is optee
> >>>>> running?".
> >>>>>
> >>>>>
> >>>>> My problem is that trying to start linux with CONFIG_OPTEE=y will cause
> >>>>> the bootm command to crash (log in appendix A):
> >>>>>
> >>>>>
> >>>>>        load mmc 0:7 $fdt_addr_r boot/stm32mp157c-dk2.dtb
> >>>>>        load mmc 0:7 0xc8000000 boot/utee
> >>>>>        setenv bootm_boot_mode sec
> >>>>>        bootm 0xc8000000 - $fdt_addr_r
> >>>>>
> >>>>> What is the intent of calling tee_find_device() in an FDT fixup
> >>>>> function?
> >>>
> >>> The scheme is the generic U-Boot implementation do copy OP-TEE
> >>> related nodes when found in its FDT to the FDT provided to Linux.
> >>> (called from common/image-fdt.c)
> >>>
> >>> However stm32mp1 can be used with or without OP-TEE installed. To
> >>> get a generic stm32mp1/U-Boot image that support both configurations
> >>> (with and w/o OP-TEE installed), U-Boot FDT and config for this plaform
> >>> do enable OP-TEE but, at u-boot runtime, if we find OP-TEE's not present,
> >>> we remove the FTD node so that Linux does get it and expect OP-TEE
> >>> is present.
> >>>
> >>>>> Do you have any ideas how to make it not crash (short of
> >>>>> commenting out the problem lines) ?
> >>>
> >>> The crash seems due to that there is no secure monitor by the time
> >>> you have this sequence called. Secure monitor is the code that
> >>> handles the SMC. If none installed, SMCs ends nowhere and
> >>> likely badly crash the systel. If OP-TEE is not running but there
> >>> is a secure monitor loaded, it should not crash.
> >>>
> >>> It seems to me that U-Boot does set up a secure monitor for
> >>> PSCI minimal support, so the U-Boot PSCI stack should
> >>> nicely handle the SMC to report that there is no OP-TEE installed.
> >>> Enabling CONFIG_ARMV7_PSCI should fix the issue I think.
> >>
> >> Hi Etienne. I understand the reasoning behind this, and I agree that
> >> things shouldn't break in theory. However,I just double-checked this
> >> with master (7a8ac9df5d). I think we have a bug on our hands:
> >>
> >> stm32mp15_basic_defconfig, with the following changes:
> >>
> >>          CONFIG_TEE=y
> >>          CONFIG_OPTEE=y
> >>          CONFIG_OPTEE_TZDRAM_SIZE=0x01000000
> >
> > My apologies, I did'nt notice you use the _basic_defconfig.
> > With this defconfig, U-Boot cannot invoke OP-TEE services since OP-TEE
> > is booted only once U-Boot execution is completed (once bootm
> > command jumps to OP-TEE boot entry). So in this config U-Boot cannot
> > find the OP-TEE node.
> >
> > Maybe ft_system_setup() (mach-stm32mp/fdt.c) should bypass OP-TEE
> > auto probing when CONFIG_BOOTM_OPTEE is enabled.
>
> I'm seeing the following premise:
>      (a) Start out with a "clean" devicetree
>      (b) Add the 'optee' node (generic code)
>      (c) Remove the 'optee' node (stm32mp1 specific)
>
> This seems odd to me for two reasons:
>      1. The code does more work than it needs to
>      2. stm32mp behaves differently than other platforms
>
>
> The way I would approach this is to adjust the heuristic in step (b).
> This can be found in include/tee/optee.h:
>
>         if CONFIG_OPTEE and CONFIG_OF_LIBFDT
>                 add optee node
>         else
>                 do nothing
>
> If there's no need to do step (b), why do both steps (b) and (c) instead
> of doing nothing?
>
> Given that there isn't a situation where step (c) makes sense, I propose
> that we remove stm32_fdt_disable_optee() entirely. The worst case is
> that the 'optee' node stays up, linux is booted in non-secure mode, and
> throws an -ENODEV in optee_probe() -- but otherwise the system boots.
>
> I can prepare a patch, pending Patrick's analysis.

Ack.

>
>
> > That would probably require to change stm32mp15_trusted_defconfig
> > to explicitly disable CONFIG_BOOTM_OPTEE, since this config
> > does not allow booting OP-TEE from U-Boot.
> >
> > Patrick, your opinion?
> >
> > FYI Alex, stm32mp15_basic_defconfig is not well suited to boot OP-TEE.
> > Consider using stm32mp15_trusted_defconfig instead. The "Trusted"
> > means U-Boot is booted after secure world. SPL could still be used
> > to load/boot OP-TEE but stm32mp15_trusted_defconfig does not
> > provide such support in current U-Boot. TF-A is an alternative to SPL
> > in this case.
>
> I have boot timing constraint of one to a graphical userspace
> application. The TF-A boot flow blows my boot time budget. I think
> trusted_defconfig assumes TF-A instead of SPL, so I didn't dig into it.

Ok. I agree it should work.

>
> The fast graphical boot is a use case that I don't think has been
> addressed for STM32MP. I'm currently implementing:
>
>         SPL -> OPTEE -> Linux -> Userspace graphics app
>
> This flow doesn't really require CONFIG_BOOTM_OPTEE. However, I'm still
> interested in a proper for the sake of consistency across platforms.
>

mach-stm32mp/fdt.c could be changed to not probe OP-TEE when it is built as SPL.
SPL would then still enable optee nodes in kernel DT when found
enabled in U-Boot FDT from generic path.
stm32mp15 would still leverage SMCCC protocol to identify OP-TEE
presence dynamically.

If I understand correctly, U-Boot generic adds OP-TEE nodes in kernel
FDT when OP-TEE is found in FDT
(upon fe contidions: optee node found and status="okay" in u-boot FDT,
no optee node already in kernel FDT)
It looks a good behaviour.

stm32mp15 leverage U-Boot can probe OP-TEE to dynamically check
whether OP-TEE is really installed
and prevent booted kernel failure/trace on OP-TEE absence.
It makes stm32mp15 U-Boot DTS files more flexible when the target
board runs or not an OP-TEE.
Obviously this dynamic test cannot be run from SPL.

br/
etienne

> Alex
>
> > br,
> > etienne
> >
> >
> >
> >
> >>          # CONFIG_SYSRESET_CMD_POWEROFF is not set
> >>
> >> I double-checked that CONFIG_ARMV7_PSCI is indeed set. The following
> >> sequence will cause a bad mode abort:
> >>
> >>          > load mmc 0:7 $loadaddr boot/uImage
> >>          > load mmc 0:7 $fdt_addr_r boot/stm32mp157c-dk2.dtb
> >>          > setenv bootargs console=ttySTM0 root=/dev/mmcblk0p7
> >>          > bootm $loadaddr - $fdt_addr_r
> >>
> >> Alex
> >>
> >>
> >>
> >>> Regards,
> >>> Etienne
> >>>
> >>>>>
> >>>>> Alex
> >>>>>
> >>>>>
> >>>>> Appendix A: u-boot log after bootm command
> >>>>>
> >>>>> ## Booting kernel from Legacy Image at c8000000 ...
> >>>>>       Image Name:
> >>>>>       Created:      2020-09-28  20:58:56 UTC
> >>>>>       Image Type:   ARM Trusted Execution Environment Kernel Image
> >>>>> (uncompressed)
> >>>>>       Data Size:    349276 Bytes = 341.1 KiB
> >>>>>       Load Address: fdffffe4
> >>>>>       Entry Point:  fe000000
> >>>>>       Verifying Checksum ... OK
> >>>>>       Loading Kernel Image
> >>>>> ## Flattened Device Tree blob at c4000000
> >>>>>       Booting using the fdt blob at 0xc4000000
> >>>>>       Loading Device Tree to cffef000, end cffff5e2 ... OK
> >>>>> <BOARD RESETS WITHOUT USER INPUT>
> >>>>> U-Boot SPL 2020.10-rc4 (Sep 20 2020 - 23:46:47 +0000)
> >>>>> Model: STMicroelectronics STM32MP157C-DK2 Discovery Board

More information about the U-Boot mailing list