[U-Boot,V4,2/2] bootm: Handle kernel_noload on arm64

Message ID 20180613041333.17943-2-marek.vasut+renesas@gmail.com
State Under Review
Delegated to: Tom Rini
Headers show
Series
  • [U-Boot,V4,1/2] ARM: image: Add option for ignoring ep bit 3
Related show

Commit Message

Marek Vasut June 13, 2018, 4:13 a.m.
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(-)

Comments

Bin Chen June 13, 2018, 7:51 a.m. | #1
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
>
>
Marek Vasut June 13, 2018, 10:39 a.m. | #2
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.

[...]
Tom Rini June 13, 2018, 12:58 p.m. | #3
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.
Tom Rini June 19, 2018, 6:42 p.m. | #4
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!

Patch

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);