Messages in this thread | | | Date | Thu, 17 Dec 2020 02:35:14 +0000 | From | Paul Cercueil <> | Subject | Re: [PATCH] MIPS: boot: Fix unaligned access with CONFIG_MIPS_RAW_APPENDED_DTB |
| |
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
| |