| Message ID | 1430760687-28505-22-git-send-email-sjg@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Simon Glass |
| Headers | show |
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. This approach sounds a little brittle; what happens if r0 just happens to have that value. Won't the code get confused? 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?
Hi Stephen, On 5 May 2015 at 09:54, Stephen Warren <swarren@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. > > 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. Regards, Simon
On 05/05/2015 10:02 AM, Simon Glass wrote: > Hi Stephen, > > On 5 May 2015 at 09:54, Stephen Warren <swarren@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? >> 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? This feature seems really use-case-specific; I wonder if it's useful/generic-enough to upstream even?
Hi Stephen, On 5 May 2015 at 10:10, Stephen Warren <swarren@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@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
On 05/05/2015 10:19 AM, Simon Glass wrote: > Hi Stephen, > > On 5 May 2015 at 10:10, Stephen Warren <swarren@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@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? OK, that highlights that better. >>>> 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) That's a design decision. There's absolutely no need for that to be true. > 2. Loading the environment is a security risk (since anyone can change > it in Linux, for example) so cannot be loaded. Well, the environment could be the default/built-in environment and hence validated as part of the validation of the U-Boot binary. Or, even if loaded separately, could also be validated in the same way (but perhaps there's not much point in that, since a fall-back to the built-in environment would be required in case the external environment validation failed). >> 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. OK. I worry that there are many many possible ways of doing that, and the selection of the best option depends on the system use-cases, security model, and environments. We might not want to lock people into a specific method. So long as the existence of this code doesn't prevent doing things some other way if they need, or upstreaming support for other methods, nor make the code too complex, then it's probably fine.
Hi Stephen, On 5 May 2015 at 12:07, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 05/05/2015 10:19 AM, Simon Glass wrote: >> >> Hi Stephen, >> >> On 5 May 2015 at 10:10, Stephen Warren <swarren@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@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? > > > OK, that highlights that better. > >>>>> 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) > > > That's a design decision. There's absolutely no need for that to be true. At present U-Boot has a single CONFIG for the environment type/position. Therefore we can't have the same U-Boot behave differently. > >> 2. Loading the environment is a security risk (since anyone can change >> it in Linux, for example) so cannot be loaded. > > > Well, the environment could be the default/built-in environment and hence > validated as part of the validation of the U-Boot binary. Or, even if loaded > separately, could also be validated in the same way (but perhaps there's > not much point in that, since a fall-back to the built-in environment would > be required in case the external environment validation failed). That's why we have the load-env device tree option, which uses the default environment. But since the RO and RW U-Boot then have the same environment, this doesn't really help. Yes we could validate it, although it's a pain since it is one more thing to sign and hash and it's not in the FIT. > >>> 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. > > > OK. I worry that there are many many possible ways of doing that, and the > selection of the best option depends on the system use-cases, security > model, and environments. We might not want to lock people into a specific > method. So long as the existence of this code doesn't prevent doing things > some other way if they need, or upstreaming support for other methods, nor > make the code too complex, then it's probably fine. This seems pretty simple to me. Regards, Simon
diff --git a/arch/arm/mach-tegra/board.c b/arch/arm/mach-tegra/board.c index 222de6a..77be211 100644 --- a/arch/arm/mach-tegra/board.c +++ b/arch/arm/mach-tegra/board.c @@ -6,6 +6,7 @@ */ #include <common.h> +#include <spl.h> #include <asm/io.h> #include <asm/arch/clock.h> #include <asm/arch/funcmux.h> @@ -29,6 +30,21 @@ enum { UART_COUNT = 5, }; +static bool from_spl __attribute__ ((section(".data"))); + +#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 + +bool spl_was_boot_source(void) +{ + return from_spl; +} + #if defined(CONFIG_TEGRA_SUPPORT_NON_SECURE) #if !defined(CONFIG_TEGRA124) #error tegra_cpu_is_non_secure has only been validated on Tegra124
Add an implementation of this function for Tegra. Signed-off-by: Simon Glass <sjg@chromium.org> --- arch/arm/mach-tegra/board.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)