| Message ID | 20201216233956.280068-1-paul@crapouillou.net (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | MIPS: boot: Fix unaligned access with CONFIG_MIPS_RAW_APPENDED_DTB | expand |
On Wed, Dec 16, 2020 at 3:40 PM Paul Cercueil <paul@crapouillou.net> wrote: > > The compressed payload is not necesarily 4-byte aligned, at least when > compiling with Clang. In that case, the 4-byte value appended to the > compressed payload that corresponds to the uncompressed kernel image > size must be read using get_unaligned_le(). Should it be get_unaligned_le32()? > > This fixes Clang-built kernels not booting on MIPS (tested on a Ingenic > JZ4770 board). > > Fixes: b8f54f2cde78 ("MIPS: ZBOOT: copy appended dtb to the end of the kernel") > Cc: <stable@vger.kernel.org> # v4.7 > Signed-off-by: Paul Cercueil <paul@crapouillou.net> Hi Paul, thanks for the patch (and for testing with Clang)! Alternatively, we could re-align __image_end to the next 4B multiple via: diff --git a/arch/mips/boot/compressed/ld.script b/arch/mips/boot/compressed/ld.script index 0ebb667274d6..349919eff5fb 100644 --- a/arch/mips/boot/compressed/ld.script +++ b/arch/mips/boot/compressed/ld.script @@ -27,6 +27,7 @@ SECTIONS /* Put the compressed image here */ __image_begin = .; *(.image) + . = ALIGN(4); __image_end = .; CONSTRUCTORS . = ALIGN(16); The tradeoff being up to 3 wasted bytes of padding in the compressed image, vs fetching one value slower (assuming unaligned loads are slower than aligned loads MIPS, IDK). I doubt decompress_kernel is called repeatedly, so let's take the byte saving approach of yours by using unaligned loads! Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > --- > arch/mips/boot/compressed/decompress.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/mips/boot/compressed/decompress.c b/arch/mips/boot/compressed/decompress.c > index c61c641674e6..47c07990432b 100644 > --- a/arch/mips/boot/compressed/decompress.c > +++ b/arch/mips/boot/compressed/decompress.c > @@ -117,7 +117,7 @@ void decompress_kernel(unsigned long boot_heap_start) > dtb_size = fdt_totalsize((void *)&__appended_dtb); > > /* last four bytes is always image size in little endian */ > - image_size = le32_to_cpup((void *)&__image_end - 4); > + image_size = get_unaligned_le32((void *)&__image_end - 4); > > /* copy dtb to where the booted kernel will expect it */ > memcpy((void *)VMLINUX_LOAD_ADDRESS_ULL + image_size, > -- > 2.29.2 >
Hi Nick, Le mer. 16 déc. 2020 à 18:08, Nick Desaulniers <ndesaulniers@google.com> a écrit : > On Wed, Dec 16, 2020 at 3:40 PM Paul Cercueil <paul@crapouillou.net> > wrote: >> >> The compressed payload is not necesarily 4-byte aligned, at least >> when >> compiling with Clang. In that case, the 4-byte value appended to the >> compressed payload that corresponds to the uncompressed kernel image >> size must be read using get_unaligned_le(). > > Should it be get_unaligned_le32()? Indeed. >> >> This fixes Clang-built kernels not booting on MIPS (tested on a >> Ingenic >> JZ4770 board). >> >> Fixes: b8f54f2cde78 ("MIPS: ZBOOT: copy appended dtb to the end of >> the kernel") >> Cc: <stable@vger.kernel.org> # v4.7 >> Signed-off-by: Paul Cercueil <paul@crapouillou.net> > > Hi Paul, thanks for the patch (and for testing with Clang)! > Alternatively, we could re-align __image_end to the next 4B multiple > via: > > diff --git a/arch/mips/boot/compressed/ld.script > b/arch/mips/boot/compressed/ld.script > index 0ebb667274d6..349919eff5fb 100644 > --- a/arch/mips/boot/compressed/ld.script > +++ b/arch/mips/boot/compressed/ld.script > @@ -27,6 +27,7 @@ SECTIONS > /* Put the compressed image here */ > __image_begin = .; > *(.image) > + . = ALIGN(4); > __image_end = .; > CONSTRUCTORS > . = ALIGN(16); Actually that would not work (I did try that), since the 4-byte size appended to the compressed payload is inside the *(.image) section. The code that appends it (in scripts/Makefile.lib, I think) doesn't seem to take care about aligning it to a 4-byte offset. I have no idea why it does with GCC and doesn't with Clang, and I have no idea why the compressed payload's size isn't aligned either. > The tradeoff being up to 3 wasted bytes of padding in the compressed > image, vs fetching one value slower (assuming unaligned loads are > slower than aligned loads MIPS, IDK). I doubt decompress_kernel is > called repeatedly, so let's take the byte saving approach of yours by > using unaligned loads! > > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> Thanks. Cheers, -Paul >> --- >> arch/mips/boot/compressed/decompress.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/mips/boot/compressed/decompress.c >> b/arch/mips/boot/compressed/decompress.c >> index c61c641674e6..47c07990432b 100644 >> --- a/arch/mips/boot/compressed/decompress.c >> +++ b/arch/mips/boot/compressed/decompress.c >> @@ -117,7 +117,7 @@ void decompress_kernel(unsigned long >> boot_heap_start) >> dtb_size = fdt_totalsize((void *)&__appended_dtb); >> >> /* last four bytes is always image size in little >> endian */ >> - image_size = le32_to_cpup((void *)&__image_end - 4); >> + image_size = get_unaligned_le32((void >> *)&__image_end - 4); >> >> /* copy dtb to where the booted kernel will expect >> it */ >> memcpy((void *)VMLINUX_LOAD_ADDRESS_ULL + >> image_size, >> -- >> 2.29.2 >> > > > -- > Thanks, > ~Nick Desaulniers
On Thu, Dec 17, 2020 at 12:41 AM Paul Cercueil <paul@crapouillou.net> wrote: > > The compressed payload is not necesarily 4-byte aligned, at least when > compiling with Clang. In that case, the 4-byte value appended to the > compressed payload that corresponds to the uncompressed kernel image > size must be read using get_unaligned_le(). > > This fixes Clang-built kernels not booting on MIPS (tested on a Ingenic > JZ4770 board). > > Fixes: b8f54f2cde78 ("MIPS: ZBOOT: copy appended dtb to the end of the kernel") > Cc: <stable@vger.kernel.org> # v4.7 > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > --- > arch/mips/boot/compressed/decompress.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
On Wed, Dec 16, 2020 at 11:39:56PM +0000, Paul Cercueil wrote: > The compressed payload is not necesarily 4-byte aligned, at least when > compiling with Clang. In that case, the 4-byte value appended to the > compressed payload that corresponds to the uncompressed kernel image > size must be read using get_unaligned_le(). > > This fixes Clang-built kernels not booting on MIPS (tested on a Ingenic > JZ4770 board). > > Fixes: b8f54f2cde78 ("MIPS: ZBOOT: copy appended dtb to the end of the kernel") > Cc: <stable@vger.kernel.org> # v4.7 > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > --- > arch/mips/boot/compressed/decompress.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/mips/boot/compressed/decompress.c b/arch/mips/boot/compressed/decompress.c > index c61c641674e6..47c07990432b 100644 > --- a/arch/mips/boot/compressed/decompress.c > +++ b/arch/mips/boot/compressed/decompress.c > @@ -117,7 +117,7 @@ void decompress_kernel(unsigned long boot_heap_start) > dtb_size = fdt_totalsize((void *)&__appended_dtb); > > /* last four bytes is always image size in little endian */ > - image_size = le32_to_cpup((void *)&__image_end - 4); > + image_size = get_unaligned_le32((void *)&__image_end - 4); gives me following error arch/mips/boot/compressed/decompress.c:120:16: error: implicit declaration of function ‘get_unaligned_le32’ [-Werror=implicit-function-declaration] image_size = get_unaligned_le32((void *)&__image_end - 4); I've added #include <asm/unaligned.h> which fixes the compile error, but I'm wondering why the patch compiled for you ? Thomas.
Hi Thomas, Le lun. 28 déc. 2020 à 23:25, Thomas Bogendoerfer <tsbogend@alpha.franken.de> a écrit : > On Wed, Dec 16, 2020 at 11:39:56PM +0000, Paul Cercueil wrote: >> The compressed payload is not necesarily 4-byte aligned, at least >> when >> compiling with Clang. In that case, the 4-byte value appended to the >> compressed payload that corresponds to the uncompressed kernel image >> size must be read using get_unaligned_le(). >> >> This fixes Clang-built kernels not booting on MIPS (tested on a >> Ingenic >> JZ4770 board). >> >> Fixes: b8f54f2cde78 ("MIPS: ZBOOT: copy appended dtb to the end of >> the kernel") >> Cc: <stable@vger.kernel.org> # v4.7 >> Signed-off-by: Paul Cercueil <paul@crapouillou.net> >> --- >> arch/mips/boot/compressed/decompress.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/mips/boot/compressed/decompress.c >> b/arch/mips/boot/compressed/decompress.c >> index c61c641674e6..47c07990432b 100644 >> --- a/arch/mips/boot/compressed/decompress.c >> +++ b/arch/mips/boot/compressed/decompress.c >> @@ -117,7 +117,7 @@ void decompress_kernel(unsigned long >> boot_heap_start) >> dtb_size = fdt_totalsize((void *)&__appended_dtb); >> >> /* last four bytes is always image size in little endian */ >> - image_size = le32_to_cpup((void *)&__image_end - 4); >> + image_size = get_unaligned_le32((void *)&__image_end - 4); > > gives me following error > > arch/mips/boot/compressed/decompress.c:120:16: error: implicit > declaration of function ‘get_unaligned_le32’ > [-Werror=implicit-function-declaration] > image_size = get_unaligned_le32((void *)&__image_end - 4); > > I've added > > #include <asm/unaligned.h> > > which fixes the compile error, but I'm wondering why the patch > compiled > for you ? No idea - but it does compile fine without the include here. Probably a defconfig difference. Cheers, -Paul
On Mon, Dec 28, 2020 at 10:30:36PM +0000, Paul Cercueil wrote: > Le lun. 28 déc. 2020 à 23:25, Thomas Bogendoerfer > <tsbogend@alpha.franken.de> a écrit : > > On Wed, Dec 16, 2020 at 11:39:56PM +0000, Paul Cercueil wrote: > > > The compressed payload is not necesarily 4-byte aligned, at least > > > when > > > compiling with Clang. In that case, the 4-byte value appended to the > > > compressed payload that corresponds to the uncompressed kernel image > > > size must be read using get_unaligned_le(). > > > > > > This fixes Clang-built kernels not booting on MIPS (tested on a > > > Ingenic > > > JZ4770 board). > > > > > > Fixes: b8f54f2cde78 ("MIPS: ZBOOT: copy appended dtb to the end of > > > the kernel") > > > Cc: <stable@vger.kernel.org> # v4.7 > > > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > > > --- > > > arch/mips/boot/compressed/decompress.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/arch/mips/boot/compressed/decompress.c > > > b/arch/mips/boot/compressed/decompress.c > > > index c61c641674e6..47c07990432b 100644 > > > --- a/arch/mips/boot/compressed/decompress.c > > > +++ b/arch/mips/boot/compressed/decompress.c > > > @@ -117,7 +117,7 @@ void decompress_kernel(unsigned long > > > boot_heap_start) > > > dtb_size = fdt_totalsize((void *)&__appended_dtb); > > > > > > /* last four bytes is always image size in little endian */ > > > - image_size = le32_to_cpup((void *)&__image_end - 4); > > > + image_size = get_unaligned_le32((void *)&__image_end - 4); > > > > gives me following error > > > > arch/mips/boot/compressed/decompress.c:120:16: error: implicit > > declaration of function ‘get_unaligned_le32’ > > [-Werror=implicit-function-declaration] > > image_size = get_unaligned_le32((void *)&__image_end - 4); > > > > I've added > > > > #include <asm/unaligned.h> > > > > which fixes the compile error, but I'm wondering why the patch compiled > > for you ? > > No idea - but it does compile fine without the include here. Probably a > defconfig difference. # CONFIG_KERNEL_LZO is not set # CONFIG_KERNEL_LZ4 is not set this makes the difference. Both decompress.c files include asm/unaligned.h. I've added the #include, fixed the get_unaligned_le32 in the description and applied it to mips-fixes. Thomas.
Hi Thomas, Le mar. 29 déc. 2020 à 16:08, Thomas Bogendoerfer <tsbogend@alpha.franken.de> a écrit : > On Mon, Dec 28, 2020 at 10:30:36PM +0000, Paul Cercueil wrote: >> Le lun. 28 déc. 2020 à 23:25, Thomas Bogendoerfer >> <tsbogend@alpha.franken.de> a écrit : >> > On Wed, Dec 16, 2020 at 11:39:56PM +0000, Paul Cercueil wrote: >> > > The compressed payload is not necesarily 4-byte aligned, at >> least >> > > when >> > > compiling with Clang. In that case, the 4-byte value appended >> to the >> > > compressed payload that corresponds to the uncompressed kernel >> image >> > > size must be read using get_unaligned_le(). >> > > >> > > This fixes Clang-built kernels not booting on MIPS (tested on a >> > > Ingenic >> > > JZ4770 board). >> > > >> > > Fixes: b8f54f2cde78 ("MIPS: ZBOOT: copy appended dtb to the >> end of >> > > the kernel") >> > > Cc: <stable@vger.kernel.org> # v4.7 >> > > Signed-off-by: Paul Cercueil <paul@crapouillou.net> >> > > --- >> > > arch/mips/boot/compressed/decompress.c | 2 +- >> > > 1 file changed, 1 insertion(+), 1 deletion(-) >> > > >> > > diff --git a/arch/mips/boot/compressed/decompress.c >> > > b/arch/mips/boot/compressed/decompress.c >> > > index c61c641674e6..47c07990432b 100644 >> > > --- a/arch/mips/boot/compressed/decompress.c >> > > +++ b/arch/mips/boot/compressed/decompress.c >> > > @@ -117,7 +117,7 @@ void decompress_kernel(unsigned long >> > > boot_heap_start) >> > > dtb_size = fdt_totalsize((void *)&__appended_dtb); >> > > >> > > /* last four bytes is always image size in little endian */ >> > > - image_size = le32_to_cpup((void *)&__image_end - 4); >> > > + image_size = get_unaligned_le32((void *)&__image_end - 4); >> > >> > gives me following error >> > >> > arch/mips/boot/compressed/decompress.c:120:16: error: implicit >> > declaration of function ‘get_unaligned_le32’ >> > [-Werror=implicit-function-declaration] >> > image_size = get_unaligned_le32((void *)&__image_end - 4); >> > >> > I've added >> > >> > #include <asm/unaligned.h> >> > >> > which fixes the compile error, but I'm wondering why the patch >> compiled >> > for you ? >> >> No idea - but it does compile fine without the include here. >> Probably a >> defconfig difference. > > # CONFIG_KERNEL_LZO is not set > # CONFIG_KERNEL_LZ4 is not set > > this makes the difference. Both decompress.c files include > asm/unaligned.h. > > I've added the #include, fixed the get_unaligned_le32 in the > description > and applied it to mips-fixes. Alright, great! Thanks! Cheers, -Paul
diff --git a/arch/mips/boot/compressed/decompress.c b/arch/mips/boot/compressed/decompress.c index c61c641674e6..47c07990432b 100644 --- a/arch/mips/boot/compressed/decompress.c +++ b/arch/mips/boot/compressed/decompress.c @@ -117,7 +117,7 @@ void decompress_kernel(unsigned long boot_heap_start) dtb_size = fdt_totalsize((void *)&__appended_dtb); /* last four bytes is always image size in little endian */ - image_size = le32_to_cpup((void *)&__image_end - 4); + image_size = get_unaligned_le32((void *)&__image_end - 4); /* copy dtb to where the booted kernel will expect it */ memcpy((void *)VMLINUX_LOAD_ADDRESS_ULL + image_size,
The compressed payload is not necesarily 4-byte aligned, at least when compiling with Clang. In that case, the 4-byte value appended to the compressed payload that corresponds to the uncompressed kernel image size must be read using get_unaligned_le(). This fixes Clang-built kernels not booting on MIPS (tested on a Ingenic JZ4770 board). Fixes: b8f54f2cde78 ("MIPS: ZBOOT: copy appended dtb to the end of the kernel") Cc: <stable@vger.kernel.org> # v4.7 Signed-off-by: Paul Cercueil <paul@crapouillou.net> --- arch/mips/boot/compressed/decompress.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)