| Message ID | 20180613041333.17943-2-marek.vasut+renesas@gmail.com |
|---|---|
| State | Accepted |
| Commit | 487b5fa6deb1f02843dbc9a9ac792bb38ef4d52a |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [U-Boot,V4,1/2] ARM: image: Add option for ignoring ep bit 3 | expand |
On 13 June 2018 at 14:13, Marek Vasut <marek.vasut@gmail.com> wrote: > The ARM64 has 2 MiB alignment requirement for the kernel. When using > fitImage, this requirement may by violated, the kernel will thus be > executed from unaligned address and fail to boot. Do what booti does > and run booti_setup() for kernel_noload images on arm64 to obtain a > suitable aligned address to which the image shall be relocated. > > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > Cc: Bin Chen <bin.chen@linaro.org> > Cc: Masahiro Yamada <yamada.masahiro@socionext.com> > Cc: Tom Rini <trini@konsulko.com> > --- > V2: Protect the ARM64 booti bit with if IS_ENABLED(CMD_BOOTI) > V3: Use if() instead of #ifdef > V4: Switch force_reloc to bool > --- > common/bootm.c | 19 +++++++++++++++++-- > 1 file changed, 17 insertions(+), 2 deletions(-) > > diff --git a/common/bootm.c b/common/bootm.c > index e789f6818a..e517d9f118 100644 > --- a/common/bootm.c > +++ b/common/bootm.c > @@ -202,8 +202,23 @@ static int bootm_find_os(cmd_tbl_t *cmdtp, int flag, > int argc, > } > > if (images.os.type == IH_TYPE_KERNEL_NOLOAD) { > - images.os.load = images.os.image_start; > - images.ep += images.os.load; > + if (CONFIG_IS_ENABLED(CMD_BOOTI) && > + images.os.arch == IH_ARCH_ARM64) { > + ulong image_addr; > + ulong image_size; > + > + ret = booti_setup(images.os.image_start, > &image_addr, > + &image_size, true); > Is it guaranteed to be conflict free (with other images) by always moving the kernel to the start of the RAM? > + if (ret != 0) > + return 1; > Do you think it helps to add some debug/error message here as in other path that returns 1? > + > + images.os.type = IH_TYPE_KERNEL; > + images.os.load = image_addr; > + images.ep = image_addr; > + } else { > + images.os.load = images.os.image_start; > + images.ep += images.os.image_start; > I know this is same the orinigal code. I'm not famaliar the history/purpse of type IH_TYPE_KERNEL_NOLOAD (compared with IH_TYPE_KERNEL), so just for my understanding, why in this case we have to increment the images.ep by images.os.load, not just set images.ep to images.os.load. + } > } > > images.os.start = map_to_sysmem(os_hdr); > -- > 2.17.1 > >
On 06/13/2018 09:51 AM, Bin Chen wrote: > > > On 13 June 2018 at 14:13, Marek Vasut <marek.vasut@gmail.com > <mailto:marek.vasut@gmail.com>> wrote: > > The ARM64 has 2 MiB alignment requirement for the kernel. When using > fitImage, this requirement may by violated, the kernel will thus be > executed from unaligned address and fail to boot. Do what booti does > and run booti_setup() for kernel_noload images on arm64 to obtain a > suitable aligned address to which the image shall be relocated. > > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com > <mailto:marek.vasut%2Brenesas@gmail.com>> > Cc: Bin Chen <bin.chen@linaro.org <mailto:bin.chen@linaro.org>> > Cc: Masahiro Yamada <yamada.masahiro@socionext.com > <mailto:yamada.masahiro@socionext.com>> > Cc: Tom Rini <trini@konsulko.com <mailto:trini@konsulko.com>> > --- > V2: Protect the ARM64 booti bit with if IS_ENABLED(CMD_BOOTI) > V3: Use if() instead of #ifdef > V4: Switch force_reloc to bool > --- > common/bootm.c | 19 +++++++++++++++++-- > 1 file changed, 17 insertions(+), 2 deletions(-) > > diff --git a/common/bootm.c b/common/bootm.c > index e789f6818a..e517d9f118 100644 > --- a/common/bootm.c > +++ b/common/bootm.c > @@ -202,8 +202,23 @@ static int bootm_find_os(cmd_tbl_t *cmdtp, int > flag, int argc, > } > > if (images.os.type == IH_TYPE_KERNEL_NOLOAD) { > - images.os.load = images.os.image_start; > - images.ep += images.os.load; > + if (CONFIG_IS_ENABLED(CMD_BOOTI) && > + images.os.arch == IH_ARCH_ARM64) { > + ulong image_addr; > + ulong image_size; > + > + ret = booti_setup(images.os.image_start, > &image_addr, > + &image_size, true); > > > Is it guaranteed to be conflict free (with other images) by always > moving the kernel to the start of the RAM? Yes > > + if (ret != 0) > + return 1; > > > Do you think it helps to add some debug/error message here as in other > path that returns 1? No, what for ? > + > + images.os.type = IH_TYPE_KERNEL; > + images.os.load = image_addr; > + images.ep = image_addr; > + } else { > + images.os.load = images.os.image_start; > + images.ep += images.os.image_start; > > > I know this is same the orinigal code. I'm not famaliar the > history/purpse of type > IH_TYPE_KERNEL_NOLOAD (compared with IH_TYPE_KERNEL), so just for my > understanding, > why in this case we have to increment the images.ep by images.os.load, > not just set images.ep to > images.os.load. The ep is incremented by the offset from the start of the image. That's also the purpose of kernel noload -- don't copy the kernel to the load address and just execute it in place. Problem with arm64 is that it does not work so because the kernel needs to be at 2 MiB offset, which is hard to achieve with fitImage. Thus we work around that by placing the kernel at the start of RAM. It's overloading the kernel noload, but at least it works. [...]
On Wed, Jun 13, 2018 at 12:39:25PM +0200, Marek Vasut wrote: > On 06/13/2018 09:51 AM, Bin Chen wrote: > > > > > > On 13 June 2018 at 14:13, Marek Vasut <marek.vasut@gmail.com > > <mailto:marek.vasut@gmail.com>> wrote: > > > > The ARM64 has 2 MiB alignment requirement for the kernel. When using > > fitImage, this requirement may by violated, the kernel will thus be > > executed from unaligned address and fail to boot. Do what booti does > > and run booti_setup() for kernel_noload images on arm64 to obtain a > > suitable aligned address to which the image shall be relocated. > > > > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com > > <mailto:marek.vasut%2Brenesas@gmail.com>> > > Cc: Bin Chen <bin.chen@linaro.org <mailto:bin.chen@linaro.org>> > > Cc: Masahiro Yamada <yamada.masahiro@socionext.com > > <mailto:yamada.masahiro@socionext.com>> > > Cc: Tom Rini <trini@konsulko.com <mailto:trini@konsulko.com>> > > --- > > V2: Protect the ARM64 booti bit with if IS_ENABLED(CMD_BOOTI) > > V3: Use if() instead of #ifdef > > V4: Switch force_reloc to bool > > --- > > common/bootm.c | 19 +++++++++++++++++-- > > 1 file changed, 17 insertions(+), 2 deletions(-) > > > > diff --git a/common/bootm.c b/common/bootm.c > > index e789f6818a..e517d9f118 100644 > > --- a/common/bootm.c > > +++ b/common/bootm.c > > @@ -202,8 +202,23 @@ static int bootm_find_os(cmd_tbl_t *cmdtp, int > > flag, int argc, > > } > > > > if (images.os.type == IH_TYPE_KERNEL_NOLOAD) { > > - images.os.load = images.os.image_start; > > - images.ep += images.os.load; > > + if (CONFIG_IS_ENABLED(CMD_BOOTI) && > > + images.os.arch == IH_ARCH_ARM64) { > > + ulong image_addr; > > + ulong image_size; > > + > > + ret = booti_setup(images.os.image_start, > > &image_addr, > > + &image_size, true); > > > > > > Is it guaranteed to be conflict free (with other images) by always > > moving the kernel to the start of the RAM? > > Yes > > > > > + if (ret != 0) > > + return 1; > > > > > > Do you think it helps to add some debug/error message here as in other > > path that returns 1? > > No, what for ? > > > + > > + images.os.type = IH_TYPE_KERNEL; > > + images.os.load = image_addr; > > + images.ep = image_addr; > > + } else { > > + images.os.load = images.os.image_start; > > + images.ep += images.os.image_start; > > > > > > I know this is same the orinigal code. I'm not famaliar the > > history/purpse of type > > IH_TYPE_KERNEL_NOLOAD (compared with IH_TYPE_KERNEL), so just for my > > understanding, > > why in this case we have to increment the images.ep by images.os.load, > > not just set images.ep to > > images.os.load. > The ep is incremented by the offset from the start of the image. That's > also the purpose of kernel noload -- don't copy the kernel to the load > address and just execute it in place. Problem with arm64 is that it does > not work so because the kernel needs to be at 2 MiB offset, which is > hard to achieve with fitImage. Thus we work around that by placing the > kernel at the start of RAM. It's overloading the kernel noload, but at > least it works. And, we have also overloaded the term "noload", historically. It does mean "execute the contents where they are". But it also means (to humans) "You don't need to specify a load address, it will just work". So it is important to fix this so that a fitImage for aarch64 can work on multiple SoCs so long as of course it has the dtbs for each of those SoCs.
On Wed, Jun 13, 2018 at 06:13:33AM +0200, Marek Vasut wrote: > The ARM64 has 2 MiB alignment requirement for the kernel. When using > fitImage, this requirement may by violated, the kernel will thus be > executed from unaligned address and fail to boot. Do what booti does > and run booti_setup() for kernel_noload images on arm64 to obtain a > suitable aligned address to which the image shall be relocated. > > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > Cc: Bin Chen <bin.chen@linaro.org> > Cc: Masahiro Yamada <yamada.masahiro@socionext.com> > Cc: Tom Rini <trini@konsulko.com> Applied to u-boot/master, thanks!
diff --git a/common/bootm.c b/common/bootm.c index e789f6818a..e517d9f118 100644 --- a/common/bootm.c +++ b/common/bootm.c @@ -202,8 +202,23 @@ static int bootm_find_os(cmd_tbl_t *cmdtp, int flag, int argc, } if (images.os.type == IH_TYPE_KERNEL_NOLOAD) { - images.os.load = images.os.image_start; - images.ep += images.os.load; + if (CONFIG_IS_ENABLED(CMD_BOOTI) && + images.os.arch == IH_ARCH_ARM64) { + ulong image_addr; + ulong image_size; + + ret = booti_setup(images.os.image_start, &image_addr, + &image_size, true); + if (ret != 0) + return 1; + + images.os.type = IH_TYPE_KERNEL; + images.os.load = image_addr; + images.ep = image_addr; + } else { + images.os.load = images.os.image_start; + images.ep += images.os.image_start; + } } images.os.start = map_to_sysmem(os_hdr);
The ARM64 has 2 MiB alignment requirement for the kernel. When using fitImage, this requirement may by violated, the kernel will thus be executed from unaligned address and fail to boot. Do what booti does and run booti_setup() for kernel_noload images on arm64 to obtain a suitable aligned address to which the image shall be relocated. Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> Cc: Bin Chen <bin.chen@linaro.org> Cc: Masahiro Yamada <yamada.masahiro@socionext.com> Cc: Tom Rini <trini@konsulko.com> --- V2: Protect the ARM64 booti bit with if IS_ENABLED(CMD_BOOTI) V3: Use if() instead of #ifdef V4: Switch force_reloc to bool --- common/bootm.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)