| Message ID | 20240306005230.2638972-2-volodymyr_babchuk@epam.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Caleb Connolly |
| Headers | show |
| Series | Add support for Qualcomm SA8155-ADP board | expand |
On Wed, 6 Mar 2024 at 06:23, Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> wrote: > > There are cases when previous bootloader stage leaves some seemingly > valid value in r0, which in fact does not point to valid FDT > blob. This behavior was encountered when trying to boot U-Boot as > "hyp" loader on SA8155P-ADP. > > To be sure that we really got the pointer to a device tree we need to > validate it with fdt_valid() function. > > Note: This approach is not 100% fool-proof, as get_prev_bl_fdt_addr() > theoretically can return a pointer to a region that is not physically > mapped and we will get data abort exception when "fdt_valid" will try > to access it. But at this early boot stage we don't know where RAM is > anyways so there is little we can do. > I suppose this approach allows us to reuse the same U-Boot binary when booted in EL2 more or when loaded by ABL. So I am fine with this approach. Reviewed-by: Sumit Garg <sumit.garg@linaro.org> -Sumit > Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> > > --- > > Changes in v2: > - New patch in v2 > > arch/arm/mach-snapdragon/board.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/mach-snapdragon/board.c b/arch/arm/mach-snapdragon/board.c > index f12f5791a1..10eec47ada 100644 > --- a/arch/arm/mach-snapdragon/board.c > +++ b/arch/arm/mach-snapdragon/board.c > @@ -24,6 +24,7 @@ > #include <linux/sizes.h> > #include <lmb.h> > #include <malloc.h> > +#include <fdt_support.h> > #include <usb.h> > #include <sort.h> > > @@ -93,7 +94,9 @@ void *board_fdt_blob_setup(int *err) > * try and use the FDT built into U-Boot if there is one... > * This avoids having a hard dependency on the previous stage bootloader > */ > - if (IS_ENABLED(CONFIG_OF_SEPARATE) && (!fdt || fdt != ALIGN(fdt, SZ_4K))) { > + > + if (IS_ENABLED(CONFIG_OF_SEPARATE) && (!fdt || fdt != ALIGN(fdt, SZ_4K) || > + !fdt_valid((void *)&fdt))) { > debug("%s: Using built in FDT, bootloader gave us %#llx\n", __func__, fdt); > return (void *)gd->fdt_blob; > } > -- > 2.43.0
On 06/03/2024 00:52, Volodymyr Babchuk wrote: > There are cases when previous bootloader stage leaves some seemingly > valid value in r0, which in fact does not point to valid FDT > blob. This behavior was encountered when trying to boot U-Boot as > "hyp" loader on SA8155P-ADP. > > To be sure that we really got the pointer to a device tree we need to > validate it with fdt_valid() function. Thanks for this! > > Note: This approach is not 100% fool-proof, as get_prev_bl_fdt_addr() > theoretically can return a pointer to a region that is not physically > mapped and we will get data abort exception when "fdt_valid" will try > to access it. But at this early boot stage we don't know where RAM is > anyways so there is little we can do. IME this hasn't been an issue in practise, but yes it is a risk. > > Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> > > --- > > Changes in v2: > - New patch in v2 > > arch/arm/mach-snapdragon/board.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/mach-snapdragon/board.c b/arch/arm/mach-snapdragon/board.c > index f12f5791a1..10eec47ada 100644 > --- a/arch/arm/mach-snapdragon/board.c > +++ b/arch/arm/mach-snapdragon/board.c > @@ -24,6 +24,7 @@ > #include <linux/sizes.h> > #include <lmb.h> > #include <malloc.h> > +#include <fdt_support.h> > #include <usb.h> > #include <sort.h> > > @@ -93,7 +94,9 @@ void *board_fdt_blob_setup(int *err) > * try and use the FDT built into U-Boot if there is one... > * This avoids having a hard dependency on the previous stage bootloader > */ > - if (IS_ENABLED(CONFIG_OF_SEPARATE) && (!fdt || fdt != ALIGN(fdt, SZ_4K))) { > + > + if (IS_ENABLED(CONFIG_OF_SEPARATE) && (!fdt || fdt != ALIGN(fdt, SZ_4K) || > + !fdt_valid((void *)&fdt))) { Please use fdt_check_header() here, fdt_valid() prints a bunch of stuff using printf() which isn't really useful here. > debug("%s: Using built in FDT, bootloader gave us %#llx\n", __func__, fdt); > return (void *)gd->fdt_blob; > }
diff --git a/arch/arm/mach-snapdragon/board.c b/arch/arm/mach-snapdragon/board.c index f12f5791a1..10eec47ada 100644 --- a/arch/arm/mach-snapdragon/board.c +++ b/arch/arm/mach-snapdragon/board.c @@ -24,6 +24,7 @@ #include <linux/sizes.h> #include <lmb.h> #include <malloc.h> +#include <fdt_support.h> #include <usb.h> #include <sort.h> @@ -93,7 +94,9 @@ void *board_fdt_blob_setup(int *err) * try and use the FDT built into U-Boot if there is one... * This avoids having a hard dependency on the previous stage bootloader */ - if (IS_ENABLED(CONFIG_OF_SEPARATE) && (!fdt || fdt != ALIGN(fdt, SZ_4K))) { + + if (IS_ENABLED(CONFIG_OF_SEPARATE) && (!fdt || fdt != ALIGN(fdt, SZ_4K) || + !fdt_valid((void *)&fdt))) { debug("%s: Using built in FDT, bootloader gave us %#llx\n", __func__, fdt); return (void *)gd->fdt_blob; }
There are cases when previous bootloader stage leaves some seemingly valid value in r0, which in fact does not point to valid FDT blob. This behavior was encountered when trying to boot U-Boot as "hyp" loader on SA8155P-ADP. To be sure that we really got the pointer to a device tree we need to validate it with fdt_valid() function. Note: This approach is not 100% fool-proof, as get_prev_bl_fdt_addr() theoretically can return a pointer to a region that is not physically mapped and we will get data abort exception when "fdt_valid" will try to access it. But at this early boot stage we don't know where RAM is anyways so there is little we can do. Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> --- Changes in v2: - New patch in v2 arch/arm/mach-snapdragon/board.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)