From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek Vasut Date: Tue, 26 May 2020 17:53:50 +0200 Subject: [PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data In-Reply-To: <20200511193625.GR12564@bill-the-cat> References: <2ed78efa-9ef2-06f5-20c3-767e9113603f@denx.de> <20200506163334.GP12564@bill-the-cat> <20200506170248.GQ12564@bill-the-cat> <4c10945e-8cc2-bd0b-8501-4c8c0c9faf87@sholland.org> <97d1a603-951a-2750-bc08-63bd3177070c@denx.de> <20200508184748.GQ12564@bill-the-cat> <65a1d328-f5f6-f77f-5d9b-15bf4c6dba1b@denx.de> <20200508192157.GR12564@bill-the-cat> <20200511193625.GR12564@bill-the-cat> Message-ID: <3bf548d9-17f6-2401-9eb9-0ad671708339@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 5/11/20 9:36 PM, Tom Rini wrote: > On Sun, May 10, 2020 at 09:24:19PM +0200, Marek Vasut wrote: >> On 5/8/20 9:21 PM, Tom Rini wrote: >>> On Fri, May 08, 2020 at 09:00:02PM +0200, Marek Vasut wrote: >>>> On 5/8/20 8:47 PM, Tom Rini wrote: >>>>> On Fri, May 08, 2020 at 03:37:01AM +0200, Marek Vasut wrote: >>>>>> On 5/7/20 10:46 PM, Samuel Holland wrote: >>>>>>> On 5/6/20 12:02 PM, trini at konsulko.com (Tom Rini) wrote: >>>>>>>>>>>> I'm not sure that it is. Can we easily/safely memmove the data to be >>>>>>>>>>>> aligned? Is that really a better option in this case than ensuring >>>>>>>>>>>> alignment within the file? >>>>>>>>>>> >>>>>>>>>>> Can't we use the new mkimage -B option to enforce the alignment IFF and >>>>>>>>>>> only IFF it is required ? >>>>>>>>>> >>>>>>>>>> Perhaps. But.. >>>>>>>>>> >>>>>>>>>>> Then we can enforce it separately for 32bit >>>>>>>>>>> and 64bit platforms to 4 and 8 bytes respectively even. >>>>>>>>>> >>>>>>>>>> It's 8 bytes for both. It's possible that Linux doesn't hard fail if >>>>>>>>>> you only do 4 byte alignment but the documented requirement is 8, for >>>>>>>>>> arm32. >>>>>>>>> >>>>>>>>> With Linux you usually need to move the kernel anyway, no ? It's 2 MiB >>>>>>>>> for arm64 for example. >>>>>>>> >>>>>>>> For arm64 you have to move it to where text_offset says it needs to be. >>>>>>>> For arm32 the common (always, practically?) case is you're firing off >>>>>>>> the zImage which does what's needed. But.. >>>>>>>> >>>>>>>>> And what you usually parse in-place would be the DT then. >>>>>>>> >>>>>>>> Yes, the practical case is that it's a DT and that needs 8 byte >>>>>>>> alignment. And we should just get back to aligning that correctly. >>>>>>>> Going back to the v1 thread, it turns out the answer to "why do we even >>>>>>>> have this padding?" is "we need the DT to be aligned". >>>>>>> >>>>>>> This change broke SPL booting for me on MACH_SUN50I as well. One thing that I >>>>>>> haven't seen brought up yet is that SPL FIT code assumes exactly a 4-byte >>>>>>> alignment of external data after the FIT. In spl_load_simple_fit(): >>>>>>> >>>>>>> /* >>>>>>> * For FIT with external data, figure out where the external images >>>>>>> * start. This is the base for the data-offset properties in each >>>>>>> * image. >>>>>>> */ >>>>>>> size = fdt_totalsize(fit); >>>>>>> size = (size + 3) & ~3; >>>>>>> size = board_spl_fit_size_align(size); >>>>>>> base_offset = (size + 3) & ~3; >>>>>> >>>>>> Somehow this doesn't match the 8-byte alignment Tom was suggesting. >>>>>> And that only leads me to believe that we can either make assumptions >>>>>> about alignment, which would very likely fail one way or the other OR we >>>>>> can say that for SPL as a special case, we enforce some alignment. >>>>> >>>>> It's likely the case that on arm32 as there's no natural alignment >>>>> problem, even tho the kernel says 8 byte, 4 byte doesn't lead to failure >>>>> and is rarely if ever given 4-but-not-8-byte-aligned addresses of the >>>>> DTB. Which is why we should probably move the alignment here to 8 bytes >>>>> instead of 4. >>>>> >>>>>> But that in turn fails for fitImage with embedded data, where the >>>>>> embedded data are always aligned to 4 bytes, because that's how DTC >>>>>> aligns properties. >>>>> >>>>> I think the answer is that the use-case you're talking about is simply >>>>> going to require data to be relocated. >>>> >>>> I have a feeling that no matter how much you try to pad when generating >>>> fitImage from U-Boot, there will always be a case where that will fail. >>>> I listed at least two: >>>> - fitImage with embedded data, 4byte alignment due to DTC >>>> - Older fitImages, 4byte alignment, fails on arm64 >>>> - Someone can generate signed fitImage with older mkimage => fail >>>> >>>> So that relocation logic or at least warning or something should be in >>>> there, no matter what. >>> >>> There's two distinct areas here, and they keep being conflated. >>> >>> The case of SPL and a FIT image for U-Boot+DTB. We've always aligned >>> this to 4 bytes and it's worked. I think if someone looked at the ARM >>> ARM for aarch64 you could reason out that "4-but-not-8-byte aligned >>> pointers are slow but work" as why this wasn't a hard fail on aarch64. >> >> But we had hard-fault on arm64, see >> [PATCH] lib: rsa: Fix unaligned 64-bit fdt accesses > > You're mixing the issues again. That's an example of "device tree > properties are only 4 byte aligned" and we can't do what we were doing. > I'm not even sure reverting e8c2d25845c7 would have helped in that case. > It's also not the case we're talking about with respect to padding the > start of embedded data. No, I am not mixing any issues again. These issues are all interconnected, which is why this is a valid example of the problems we have with the padding. >>> We should adjust our current alignment up to cover that and move on. >> >> Adjust it to what, 8 bytes ? Or 16 in case RV128 happens ? Or what ? > > 8 bytes is the defined requirement for everything today that defines a > required _start_ alignment. Except DT properties are 4 byte aligned, so that's not true. >> You will fail here either way, since if you build the fitImage with >> embedded data, the embedded data will be aligned to 4 bytes, because DT >> properties are aligned to 4 bytes. > > Yes, there's the difference between "the device tree itself must be > aligned to 8 bytes" and "device tree internally is 4 byte aligned". The > latter means that some operations are simply not possible and a feature > of the format. So fitImage with embedded data should not be possible at all ? >>> The case of FIT images and "kernel_noload" / fdt_high=-1 / >>> initrd_high=-1 and aarch64. If you load a FIT image in to memory and >>> try and use it as-is, it will not work. It's not even possible in the >>> general case as you would have to inspect the kernel, see what the >>> text_offset is and build a FIT image that took that in to account, to >>> not have to move the Image around. The device tree will almost >>> certainly be misaligned and still need to be relocated. This is why a >>> while back I sent out an email asking every maintainer of a board that >>> disabled device tree relocation to stop that. Perhaps a run-time patch >>> to scream about this rather than note it as we do today would help (see >>> common/image-fdt.c::boot_relocate_fdt()). >> >> I have a feeling we should do the relocation either way. And if there is >> some special limited case (like the SPL), we should warn about it and >> push mkimage with e.g. -B 8 flag to enforce the alignment. > > Please send patches so we can see what that looks like, thanks! Maybe we should remove support for embedded data from fitImage and then enforce padding ?