On 5/6/20 7:02 PM, Tom Rini wrote: > On Wed, May 06, 2020 at 06:35:52PM +0200, Marek Vasut wrote: >> On 5/6/20 6:33 PM, Tom Rini wrote: >>> On Wed, May 06, 2020 at 06:17:47PM +0200, Marek Vasut wrote: >>>> On 5/6/20 6:04 PM, Tom Rini wrote: >>>>> On Wed, May 06, 2020 at 05:52:45PM +0200, Marek Vasut wrote: >>>>>> On 5/6/20 5:43 PM, Alex Kiernan wrote: >>>>>>> On Wed, May 6, 2020 at 3:41 PM Marek Vasut <ma...@denx.de> wrote: >>>>>>>> >>>>>>>> On 5/6/20 4:37 PM, Tom Rini wrote: >>>>>>>>> On Wed, May 06, 2020 at 04:33:37PM +0200, Marek Vasut wrote: >>>>>>>>>> On 5/6/20 4:27 PM, Tom Rini wrote: >>>>>>>>>>> On Wed, May 06, 2020 at 04:17:35PM +0200, Marek Vasut wrote: >>>>>>>>>>>> On 5/6/20 3:48 PM, Tom Rini wrote: >>>>>>>>>>>>> On Tue, May 05, 2020 at 11:17:19PM +0200, Michael Walle wrote: >>>>>>>>>>>>>> Hi all, >>>>>>>>>>>>>> >>>>>>>>>>>>>> Am 2020-05-05 20:41, schrieb Simon Glass: >>>>>>>>>>>>>>> Hi Tom, >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> On Tue, 5 May 2020 at 11:50, Tom Rini <tr...@konsulko.com> >>>>>>>>>>>>>>> wrote: >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote: >>>>>>>>>>>>>>>>> On 5/5/20 6:37 PM, Alex Kiernan wrote: >>>>>>>>>>>>>>>>>> On Tue, May 5, 2020 at 2:28 PM Marek Vasut <ma...@denx.de> >>>>>>>>>>>>>>>>>> wrote: >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> On 5/5/20 3:22 PM, Alex Kiernan wrote: >>>>>>>>>>>>>>>>>>>> On Mon, May 4, 2020 at 12:28 PM Tom Rini >>>>>>>>>>>>>>>>>>>> <tr...@konsulko.com> wrote: >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut >>>>>>>>>>>>>>>>>>>>> wrote: >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> There is no reason to tail-pad fitImage with external >>>>>>>>>>>>>>>>>>>>>> data to 4-bytes, >>>>>>>>>>>>>>>>>>>>>> while fitImage without external data does not have any >>>>>>>>>>>>>>>>>>>>>> such padding and >>>>>>>>>>>>>>>>>>>>>> is often unaligned. DT spec also does not mandate any >>>>>>>>>>>>>>>>>>>>>> such padding. >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> Moreover, the tail-pad fills the last few bytes with >>>>>>>>>>>>>>>>>>>>>> uninitialized data, >>>>>>>>>>>>>>>>>>>>>> which could lead to a potential information leak. >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> $ echo -n xy > /tmp/data ; \ >>>>>>>>>>>>>>>>>>>>>> ./tools/mkimage -E -f auto -d /tmp/data >>>>>>>>>>>>>>>>>>>>>> /tmp/fitImage ; \ >>>>>>>>>>>>>>>>>>>>>> hexdump -vC /tmp/fitImage | tail -n 3 >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> before: >>>>>>>>>>>>>>>>>>>>>> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 >>>>>>>>>>>>>>>>>>>>>> 69 |a-offset.data-si| >>>>>>>>>>>>>>>>>>>>>> 00000270 7a 65 00 00 78 79 64 64 >>>>>>>>>>>>>>>>>>>>>> |ze..xydd| >>>>>>>>>>>>>>>>>>>>>> ^^ ^^ ^^ >>>>>>>>>>>>>>>>>>>>>> after: >>>>>>>>>>>>>>>>>>>>>> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 >>>>>>>>>>>>>>>>>>>>>> 69 |a-offset.data-si| >>>>>>>>>>>>>>>>>>>>>> 00000270 7a 65 00 78 79 >>>>>>>>>>>>>>>>>>>>>> |ze.xy| >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> Signed-off-by: Marek Vasut <ma...@denx.de> >>>>>>>>>>>>>>>>>>>>>> Reviewed-by: Simon Glass <s...@chromium.org> >>>>>>>>>>>>>>>>>>>>>> Cc: Heinrich Schuchardt <xypron.g...@gmx.de> >>>>>>>>>>>>>>>>>>>>>> Cc: Tom Rini <tr...@konsulko.com> >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> Applied to u-boot/master, thanks! >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> This breaks booting on my board (am3352, eMMC boot, FIT >>>>>>>>>>>>>>>>>>>> u-boot, >>>>>>>>>>>>>>>>>>>> CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if >>>>>>>>>>>>>>>>>>>> I boot it >>>>>>>>>>>>>>>>>>>> from eMMC I get nothing at all on the console, if I boot >>>>>>>>>>>>>>>>>>>> over ymodem >>>>>>>>>>>>>>>>>>>> it stalls at 420k, before continuing to 460k. My guess is >>>>>>>>>>>>>>>>>>>> there's some >>>>>>>>>>>>>>>>>>>> error going to the console at the 420k mark, but obviously >>>>>>>>>>>>>>>>>>>> it's lost >>>>>>>>>>>>>>>>>>>> in the ymodem... I have two DTBs in the FIT image, 420k >>>>>>>>>>>>>>>>>>>> would about >>>>>>>>>>>>>>>>>>>> align to the point between them. >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> My bet would be on some padding / unaligned access problem >>>>>>>>>>>>>>>>>>> that this >>>>>>>>>>>>>>>>>>> patch uncovered. Can you take a look ? >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> Seems plausible. With this change my external data starts at >>>>>>>>>>>>>>>>>> 0x483 and >>>>>>>>>>>>>>>>>> everything after it is non-aligned: >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> Should the beginning of external data be aligned ? >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> If in U-Boot we revert >>>>>>>>>>>>>>>> e8c2d25845c72c7202a628a97d45e31beea40668 does >>>>>>>>>>>>>>>> the >>>>>>>>>>>>>>>> problem go away? If so, that's not a fix outright, it means >>>>>>>>>>>>>>>> we need >>>>>>>>>>>>>>>> to >>>>>>>>>>>>>>>> dig back in to the libfdt thread and find the "make this work >>>>>>>>>>>>>>>> without >>>>>>>>>>>>>>>> killing performance everywhere all the time" option. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> If it is a device tree, it must be 32-bit aligned. >>>>>>>>>>>>>> >>>>>>>>>>>>>> This commit actually breaks my board too (which I was just about >>>>>>>>>>>>>> to send >>>>>>>>>>>>>> upstream, but realized it was broken). >>>>>>>>>>>>>> >>>>>>>>>>>>>> Said board uses SPL and main U-Boot. SPL runs fine and main >>>>>>>>>>>>>> u-boot doesn't >>>>>>>>>>>>>> output anything. The only difference which I found is that >>>>>>>>>>>>>> fit-dtb.blob is >>>>>>>>>>>>>> 2 bytes shorter. And the content is shifted by one byte although >>>>>>>>>>>>>> data-offset is the same. Strange. In the non-working case, the >>>>>>>>>>>>>> inner >>>>>>>>>>>>>> FDT magic isn't 4 byte aligned. >>>>>>>>>>>>>> >>>>>>>>>>>>>> You can find the two fit-dtb.blobs here: >>>>>>>>>>>>>> >>>>>>>>>>>>>> https://walle.cc/u-boot/fit-dtb.blob.working >>>>>>>>>>>>>> https://walle.cc/u-boot/fit-dtb.blob.non-working >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> Reverting e8c2d25845c72c7202a628a97d45e31beea40668 doesn't help >>>>>>>>>>>>>> (I might >>>>>>>>>>>>>> reverted it the wrong way, there is actually a conflict). >>>>>>>>>>>>>> >>>>>>>>>>>>>> I'll dig deeper into that tomorrow, but maybe you have some >>>>>>>>>>>>>> pointers where >>>>>>>>>>>>>> to look. >>>>>>>>>>>>>> >>>>>>>>>>>>>> For reference you can find the current patch here: >>>>>>>>>>>>>> https://github.com/mwalle/u-boot/tree/sl28-upstream >>>>>>>>>>>>> >>>>>>>>>>>>> I think we have a few things to fix here. Marek's patch is >>>>>>>>>>>>> breaking >>>>>>>>>>>>> things and needs to be reverted. But it's showing a few >>>>>>>>>>>>> underlying >>>>>>>>>>>>> problems that need to be fixed too: >>>>>>>>>>>>> - fit_extract_data() needs to use calloc() not malloc() so that >>>>>>>>>>>>> we don't >>>>>>>>>>>>> leak random data. >>>>>>>>>>>>> - We need to 8-byte alignment on the external data. That's the >>>>>>>>>>>>> requirement for Linux for device trees on both 32 and 64bit arm. >>>>>>>>>>>>> Atish, does RISC-V require more than that? I don't see it in >>>>>>>>>>>>> Linux's >>>>>>>>>>>>> Documentation/riscv/boot-image-header.rst (and there's no >>>>>>>>>>>>> booting.rst >>>>>>>>>>>>> file like arm/arm64). >>>>>>>>>>>> >>>>>>>>>>>> Why 8-byte alignment ? The external data are copied into the target >>>>>>>>>>>> location, so why do they need to be padded in any way? >>>>>>>>>>> >>>>>>>>>>> The start of the external data needs the alignment, to be clearer. >>>>>>>>>> >>>>>>>>>> Why ? >>>>>>>>> >>>>>>>>> Given that things which end up in external data have alignment >>>>>>>>> requirements, we need to align and meet those requirements. And I >>>>>>>>> noted >>>>>>>>> why 8 above. >>>>>>>> >>>>>>>> If you end up with external data, then you need to move those blobs >>>>>>>> into >>>>>>>> their target location anyway. That's what you specify in the load = <> >>>>>>>> property in the .its . >>>>>>>> >>>>>>> >>>>>>> Just reading common/spl/spl_fit.c, I think that'll try and parse in >>>>>>> situ, rather than relocating it? >>>>>> >>>>>> And is that correct or is that the same problem as we have on arm64 with >>>>>> fitImage and fdt_high=-1 ? I think it's the later. >>>>> >>>>> 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.
The alignment for compressed image is 2 MiB though, right ? > 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. 4-byte > 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". OK, let's assume you enforce 8-byte alignment on the external data. But then, there is fitImage with embedded data (mkimage called without -E), and the DT inside that fitImage will be aligned to 4 bytes. Then what?