On 02/20/2018 09:27 PM, Bryan O'Donoghue wrote:
On 19/02/18 15:44, Tom Rini wrote:
On Fri, Feb 02, 2018 at 04:56:57PM +0100, Dr. Philipp Tomsich wrote:
Bryan,
On 2 Feb 2018, at 16:37, Bryan O'Donoghue bryan.odonoghue@linaro.org wrote:
On 02/02/18 15:02, Dr. Philipp Tomsich wrote:
Where do we stand on this: can we reuse IH_TYPE_TEE, will be use IH_TYPE_OPTEE or will there be a new IH_TYPE_OPTEE_SPL?
I think because you aren't doing anything different with the image type you can reuse IH_TYPE_TEE
This
+#if CONFIG_IS_ENABLED(OPTEE) + case IH_OS_OP_TEE: + debug("Jumping to U-Boot via OP-TEE\n"); + spl_optee_entry(NULL, NULL, spl_image->fdt_addr, + (void *)spl_image.entry_point); + break; +#endif
could just as easily be this
+#if CONFIG_IS_ENABLED(OPTEE) + case IH_TYPE_TEE: + debug("Jumping to U-Boot via OP-TEE\n"); + spl_optee_entry(NULL, NULL, spl_image->fdt_addr, + (void *)spl_image.entry_point); + break; +#endif
should be a matter of just replacing the call to mkimage to use
mkimage -A arm -T tee
instead of
mkimage -A arm -T optee
and the suggested change above.. case IH_OS_OP_TEE -> case IH_TYPE_TEE
Thanks for summarising your suggestion.
However, my mail was intended to test the waters to see what the consensus was. A it appears that none has yet emerged between all the involved parties (including our colleagues from TI that had also chimed in on the discussion). So for now, I’ll sit back and wait until some sort of consensus (or at least a majority for one solution or the other) emerges.
Personally, I am not happy with having a ‘tee’ and an ‘optee’ both refering to OP-TEE and the upstream OP-TEE documentation suggesting that their envisioned boot process was to boot through the OP-TEE (i.e. what the ‘tee’ image type does). However, with the ‘tee’ image type already being defined, we seem to have already backed ourselves into a situation where the naming is non-intuitive.
Alright, I'm a little confused here. I guess first, there's a disconnect in upstream OP-TEE land about how 32bit ARM should be done? I guess we have three different implemented and upstreamed flows:
- SPL->U-Boot->OP-TEE->U-Boot->Linux
- SPL->U-Boot->OP-TEE->Linux
- SPL->OP-TEE->U-Boot->Linux
I'd call that
- SPL/BootROM->U-Boot->OP-TEE->U-Boot->Linux
- SPL/BootROM->U-Boot->OP-TEE->Linux
- SPL/BootROM->OP-TEE->U-Boot->Linux
But yes - fundamentally your flow is correct.
I think it works better to have the secure/non-secure sides labeled, here are two flows we support:
Non-Secure Secure
BootROM | ------------- | v SPL | v U-Boot ------> <----- OP-TEE | V Linux
Non-Secure Secure
BootROM | ------------- | v SPL -------> <----- OP-TEE | v U-Boot | V Linux
(Plus one were BootROM starts a TEE directly, but that is transparent to SPL/U-Boot)
Both of these are handled by the IH_TEE type, which behaves much like a regular function call in that it returns execution to were it was kicked off from.
So to me there is only a need for two types, one like the above, and one for when the TEE is a stage in the boot flow, whether in between SPL->U-Boot or U-Boot->Linux, U-Boot needs to know it is not coming back and so set it up like a regular OS payload.
And in this last case we have a combined image that is passed from SPL to OP-TEE.
Since all 3 of these flows are in upstream OP-TEE, we need to support them all.
Agreed.
The biggest constraint is that we have the first flow already in and named "tee" (my fault, I should have made sure everyone would use the same flow). So we need to have descriptive enough names for the other flows that we're going to add so that it's clear what's what. How about "tee-standalone" for "U-Boot starts OP-TEE, and is done"
"tee-combo" for "SPL gives OP-TEE an image to deal with". This could even in theory I suspect be SPL gives OP-TEE a Linux kernel to boot. I'm also happy to hear better suffixes but I don't want "tee" and "optee". And if we can cover two flows under the same name, that's good too, we just need to name the last flow "tee-something". Thanks all!
Yes I take your point "tee" and "optee" are the opposite of descriptive names.
- SPL/BootROM->U-Boot->OP-TEE->U-Boot->Linux
Currently called "tee" - has type IH_TEE - maintained as is for compatibility - deserves some documentation.
- SPL/BootROM->U-Boot->OP-TEE->Linux
New type "tee-standalone" this would be the type I've proposed in my patch set. New type IH_TYPE_TEE_STANDALONE
- SPL/BootROM->OP-TEE->U-Boot->Linux
New type "tee-combo" this would be Kever's type IH_TYPE_TEE_COMBO
The two above didn't make sense to me until I realized their BootROM starts SPL on the secure side, otherwise there would be no technical reason not to have TEE return to the same spot in SPL/U-Boot. If I'm not mistaken the two flows above look like this:
Non-Secure Secure
BootROM | V SPL | V OP-TEE | ----------- | V U-Boot | V Linux
Non-Secure Secure
BootROM | V SPL | V U-Boot | V OP-TEE | ---------- | V Linux
I've suggested to Kever that he doesn't actually need a separate type (though I could be wrong).
I resend my previous patchset renaming "optee" to "tee-standalone" and add the type IH_TYPE_TEE_STANDALONE.
I leave it to Kever to decide next steps for his patches.
bod