[U-Boot] [PATCH 21/24] tegra124: Implement spl_was_boot_source()

[Simon Glass]

Hi Stephen,

On 5 May 2015 at 10:10, Stephen Warren <swarren at wwwdotorg.org> wrote:
> On 05/05/2015 10:02 AM, Simon Glass wrote:
>>
>> Hi Stephen,
>>
>> On 5 May 2015 at 09:54, Stephen Warren <swarren at wwwdotorg.org> wrote:
>>>
>>> On 05/04/2015 11:31 AM, Simon Glass wrote:
>>>>
>>>>
>>>> Add an implementation of this function for Tegra.
>>>
>>>
>>>
>>>> diff --git a/arch/arm/mach-tegra/board.c b/arch/arm/mach-tegra/board.c
>>>
>>>
>>>
>>>> +#ifndef CONFIG_SPL_BUILD
>>>> +void save_boot_params(u32 r0, u32 r1, u32 r2, u32 r3)
>>>> +{
>>>> +       from_spl = r0 != SPL_RUNNING_FROM_UBOOT;
>>>> +       save_boot_params_ret();
>>>> +}
>>>> +#endif
>>>
>>>
>>>
>>> (Using terminology from:
>>> https://patchwork.ozlabs.org/patch/467771/
>>> arm: spl: Enable detecting when U-Boot is started from SPL
>>> )
>>>
>>> That doesn't look right. Surely (at least on Tegra), if the r/o U-Boot
>>> chain-loads to the r/w U-Boot, then the chain-loaded U-Boot has no SPL
>>> and
>>> is just the main CPU build of U-Boot.
>>>
>>> Hence, "SPL_RUNNING_FROM_UBOOT" seems incorrectly named, since the r/o
>>> U-Boot doesn't chain to SPL but rather to U-Boot.
>>
>>
>> What name do you suggest? I was trying to add a prefix indicating that
>> it relates to non-SPL start-up of U-Boot.
>
>
> Well, that name specifically states that it's SPL that's running, whereas
> the exact opposite is true.
>
> Perhaps UBOOT_CHAIN_LOADED_FROM_UBOOT?

I really want to say that it is not chain-loaded from SPL. Maybe
UBOOT_NOT_LOADED_FROM_SPL?

>
>>> This approach sounds a little brittle; what happens if r0 just happens to
>>> have that value. Won't the code get confused?
>>
>>
>> Yes, but SPL does not set that value in r0, and we have control over this.
>>
>>> Why does U-Boot care whether it's been chain-loaded? Shouldn't it always
>>> behave identically in all cases, so it's independent of what caused it to
>>> run?
>>
>>
>> In the case of read-only U-Boot it must find the read-write one to
>> jump to. In the case of read-write U-Boot it must boot a kernel.
>
>
> Surely that should be taken care of by placing the correct boot scripts into
> the U-Boot environment, rather than hard-coding specific boot behaviour into
> the U-Boot binary?

Two problems here:

1. The two U-Boot will use the same environment (as they are identical
after all)

2. Loading the environment is a security risk (since anyone can change
it in Linux, for example) so cannot be loaded.

> This feature seems really use-case-specific; I wonder if it's
> useful/generic-enough to upstream even?

I am keen to upstream this use case (upgrading U-Boot in a secure way)
as I think it has wide application.

Regards,
Simon