Re: [PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data
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
Re: [PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data
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. > > 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. > You will fail here either way, since if you build the fi
Re: [PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data
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 > 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 ? 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. > 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
Re: [PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data
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. We should adjust our current alignment up to cover that and move on. 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()). -- Tom signature.asc Description: PGP signature
Re: [PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data
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.
Re: [PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data
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. -- Tom signature.asc Description: PGP signature
Re: [PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data
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. 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.
Re: [PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data
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;
And then later on in spl_load_fit_image():
if (!fit_image_get_data_position(fit, node, &offset)) {
external_data = true;
} else if (!fit_image_get_data_offset(fit, node, &offset)) {
offset += base_offset;
external_data = true;
}
In my case, after this change, the FIT binary was 0x453 bytes long. But SPL
rounded it up to 0x454, so the external data offsets were off by one, and the
first byte of every loadable was cut off in RAM:
Trying to boot from MMC1
fit read sector 50, sectors=3, dst=49fff980, count=3, size=0x454
firmware: 'uboot'
External data: dst=4a00, offset=454, size=81208
src = 4a54, dest = 4a00
4a00: 00 00 ea 35 00 00 14 00 00 00 00 00 00 00 00 00
If I remove both "(size + 3) & ~3" lines, I can boot successfully:
Trying to boot from MMC1
fit read sector 50, sectors=3, dst=49fff980, count=3, size=0x453
firmware: 'uboot'
External data: dst=4a00, offset=453, size=81208
src = 4a53, dest = 4a00
4a00: 1f 00 00 ea 35 00 00 14 00 00 00 00 00 00 00 00
In other words, it's not just FDTs that are affected by this change, but _all_
external data loaded from SPL.
So if you change the alignment to anything but 4 (be it 1 or 8 or something
else), you must also update the assumptions made by SPL.
Regards,
Samuel
Re: [PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data
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 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 >>> 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 >> wrote: >>> >>> On 5/5/20 3:22 PM, Alex Kiernan wrote: On Mon, May 4, 2020 at 12:28 PM Tom Rini 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: >> 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 >> 69 |a-offset.data-si| >> 0270 7a 65 00 00 78 79 64 64 >>|ze..xydd| >>^^ ^^ ^^ >> after: >> 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 >> 69 |a-offset.data-si| >> 0270 7a 65 00 78 79 >>|ze.xy| >> >> Signed-off-by: Marek Vasut >> Reviewed-by: Simon Glass >> Cc: Heinrich Schuchardt >> Cc: Tom Rini > > 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
Re: [PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data
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 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 > > 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 > wrote: > > > > On 5/5/20 3:22 PM, Alex Kiernan wrote: > >> On Mon, May 4, 2020 at 12:28 PM Tom Rini > >> 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: > 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 > 69 |a-offset.data-si| > 0270 7a 65 00 00 78 79 64 64 > |ze..xydd| > ^^ ^^ ^^ > after: > 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 > 69 |a-offset.data-si| > 0270 7a 65 00 78 79 > |ze.xy| > > Signed-off-by: Marek Vasut > Reviewed-by: Simon Glass > Cc: Heinrich Schuchardt > Cc: Tom Rini > >>> > >>> 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
Re: [PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data
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 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 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 wrote: > > On 5/5/20 3:22 PM, Alex Kiernan wrote: >> On Mon, May 4, 2020 at 12:28 PM Tom Rini >> 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: 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| 0270 7a 65 00 00 78 79 64 64 |ze..xydd| ^^ ^^ ^^ after: 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| 0270 7a 65 00 78 79 |ze.xy| Signed-off-by: Marek Vasut Reviewed-by: Simon Glass Cc: Heinrich Schuchardt Cc: Tom Rini >>> >>> 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 any
Re: [PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data
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 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 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 > >> wrote: > >>> > >>> On 5/5/20 3:22 PM, Alex Kiernan wrote: > On Mon, May 4, 2020 at 12:28 PM Tom Rini > 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: > >> 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 > >> |a-offset.data-si| > >> 0270 7a 65 00 00 78 79 64 64 > >> |ze..xydd| > >>^^ ^^ ^^ > >> after: > >> 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 > >> |a-offset.data-si| > >> 0270 7a 65 00 78 79 > >> |ze.xy| > >> > >> Signed-off-by: Marek Vasut > >> Reviewed-by: Simon Glass > >> Cc: Heinrich Schuchardt > >> Cc: Tom Rini > > > > 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 fou
Re: [PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data
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 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 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 wrote: >>> >>> On 5/5/20 3:22 PM, Alex Kiernan wrote: On Mon, May 4, 2020 at 12:28 PM Tom Rini 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: >> 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 >> |a-offset.data-si| >> 0270 7a 65 00 00 78 79 64 64 >> |ze..xydd| >>^^ ^^ ^^ >> after: >> 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 >> |a-offset.data-si| >> 0270 7a 65 00 78 79 >> |ze.xy| >> >> Signed-off-by: Marek Vasut >> Reviewed-by: Simon Glass >> Cc: Heinrich Schuchardt >> Cc: Tom Rini > > 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: >>
Re: [PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data
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 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 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 wrote: > > > > On 5/5/20 3:22 PM, Alex Kiernan wrote: > >> On Mon, May 4, 2020 at 12:28 PM Tom Rini > >> 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: > 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 > |a-offset.data-si| > 0270 7a 65 00 00 78 79 64 64 > |ze..xydd| > ^^ ^^ ^^ > after: > 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 > |a-offset.data-si| > 0270 7a 65 00 78 79 > |ze.xy| > > Signed-off-by: Marek Vasut > Reviewed-by: Simon Glass > Cc: Heinrich Schuchardt > Cc: Tom Rini > >>> > >>> 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/fi
Re: [PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data
On 5/6/20 5:43 PM, Alex Kiernan wrote: > On Wed, May 6, 2020 at 3:41 PM Marek Vasut 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 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 wrote: > > On 5/5/20 3:22 PM, Alex Kiernan wrote: >> On Mon, May 4, 2020 at 12:28 PM Tom Rini >> 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: 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| 0270 7a 65 00 00 78 79 64 64 |ze..xydd| ^^ ^^ ^^ after: 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| 0270 7a 65 00 78 79 |ze.xy| Signed-off-by: Marek Vasut Reviewed-by: Simon Glass Cc: Heinrich Schuchardt Cc: Tom Rini >>> >>> 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 pointer
Re: [PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data
On Wed, May 6, 2020 at 3:41 PM Marek Vasut 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 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 wrote: > >>> > >>> On 5/5/20 3:22 PM, Alex Kiernan wrote: > On Mon, May 4, 2020 at 12:28 PM Tom Rini > 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: > >> 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 > >> |a-offset.data-si| > >> 0270 7a 65 00 00 78 79 64 64 > >> |ze..xydd| > >>^^ ^^ ^^ > >> after: > >> 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 > >> |a-offset.data-si| > >> 0270 7a 65 00 78 79 > >> |ze.xy| > >> > >> Signed-off-by: Marek Vasut > >> Reviewed-by: Simon Glass > >> Cc: Heinrich Schuchardt > >> Cc: Tom Rini > > > > 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. > >
Re: [PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data
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 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 wrote: >>> >>> On 5/5/20 3:22 PM, Alex Kiernan wrote: On Mon, May 4, 2020 at 12:28 PM Tom Rini 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: >> 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 >> |a-offset.data-si| >> 0270 7a 65 00 00 78 79 64 64 >> |ze..xydd| >>^^ ^^ ^^ >> after: >> 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 >> |a-offset.data-si| >> 0270 7a 65 00 78 79 >> |ze.xy| >> >> Signed-off-by: Marek Vasut >> Reviewed-by: Simon Glass >> Cc: Heinrich Schuchardt >> Cc: Tom Rini > > 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()
Re: [PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data
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 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 wrote: > > > > On 5/5/20 3:22 PM, Alex Kiernan wrote: > >> On Mon, May 4, 2020 at 12:28 PM Tom Rini > >> 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: > 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 > |a-offset.data-si| > 0270 7a 65 00 00 78 79 64 64 > |ze..xydd| > ^^ ^^ ^^ > after: > 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 > |a-offset.data-si| > 0270 7a 65 00 78 79 > |ze.xy| > > Signed-off-by: Marek Vasut > Reviewed-by: Simon Glass > Cc: Heinrich Schuchardt > Cc: Tom Rini > >>> > >>> 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
Re: [PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data
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 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 wrote: > > On 5/5/20 3:22 PM, Alex Kiernan wrote: >> On Mon, May 4, 2020 at 12:28 PM Tom Rini 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: 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| 0270 7a 65 00 00 78 79 64 64 |ze..xydd| ^^ ^^ ^^ after: 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| 0270 7a 65 00 78 79|ze.xy| Signed-off-by: Marek Vasut Reviewed-by: Simon Glass Cc: Heinrich Schuchardt Cc: Tom Rini >>> >>> 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
Re: [PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data
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 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 wrote: > >>> > >>> On 5/5/20 3:22 PM, Alex Kiernan wrote: > On Mon, May 4, 2020 at 12:28 PM Tom Rini 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: > >> 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 > >> |a-offset.data-si| > >> 0270 7a 65 00 00 78 79 64 64 > >> |ze..xydd| > >>^^ ^^ ^^ > >> after: > >> 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 > >> |a-offset.data-si| > >> 0270 7a 65 00 78 79|ze.xy| > >> > >> Signed-off-by: Marek Vasut > >> Reviewed-by: Simon Glass > >> Cc: Heinrich Schuchardt > >> Cc: Tom Rini > > > > 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 th
Re: [PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data
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 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 wrote: >>> >>> On 5/5/20 3:22 PM, Alex Kiernan wrote: On Mon, May 4, 2020 at 12:28 PM Tom Rini 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: >> 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 >> |a-offset.data-si| >> 0270 7a 65 00 00 78 79 64 64 >> |ze..xydd| >>^^ ^^ ^^ >> after: >> 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 >> |a-offset.data-si| >> 0270 7a 65 00 78 79|ze.xy| >> >> Signed-off-by: Marek Vasut >> Reviewed-by: Simon Glass >> Cc: Heinrich Schuchardt >> Cc: Tom Rini > > 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? If the external data are used in place, then it's the same problem as arm64 fitImage with fdt_high=-1, which fails because the DT is aligned to 4 bytes and arm64 expects it at 8byte offset.
Re: [PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data
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 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 wrote: > > > > >> > > > > >> On 5/5/20 3:22 PM, Alex Kiernan wrote: > > > > >>> On Mon, May 4, 2020 at 12:28 PM Tom Rini 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: > > > > > 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 > > > > > |a-offset.data-si| > > > > > 0270 7a 65 00 00 78 79 64 64 > > > > > |ze..xydd| > > > > >^^ ^^ ^^ > > > > > after: > > > > > 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 > > > > > |a-offset.data-si| > > > > > 0270 7a 65 00 78 79 > > > > > |ze.xy| > > > > > > > > > > Signed-off-by: Marek Vasut > > > > > Reviewed-by: Simon Glass > > > > > Cc: Heinrich Schuchardt > > > > > Cc: Tom Rini > > > > > > > > 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). Thanks all! -- Tom signature.asc Description: PGP signature
Re: [PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data
On Tue, May 5, 2020 at 7:41 PM Simon Glass wrote: > > Hi Tom, > > On Tue, 5 May 2020 at 11:50, Tom Rini 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 wrote: > > > >> > > > >> On 5/5/20 3:22 PM, Alex Kiernan wrote: > > > >>> On Mon, May 4, 2020 at 12:28 PM Tom Rini 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: > > > > 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 > > > > |a-offset.data-si| > > > > 0270 7a 65 00 00 78 79 64 64 > > > > |ze..xydd| > > > >^^ ^^ ^^ > > > > after: > > > > 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 > > > > |a-offset.data-si| > > > > 0270 7a 65 00 78 79|ze.xy| > > > > > > > > Signed-off-by: Marek Vasut > > > > Reviewed-by: Simon Glass > > > > Cc: Heinrich Schuchardt > > > > Cc: Tom Rini > > > > > > 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. > > But Marek's patch affects the FIT image itself, so I am not sure what > would go after that. > Just reading the commit log, the example in there shows the padding after the FIT image being dropped: $ echo -n xy > /tmp/data ; \ ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \ hexdump -vC /tmp/fitImage | tail -n 3 before: 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| 0270 7a 65 00 00 78 79 64 64 |ze..xydd| ^^ ^^ ^^ after: 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| 0270 7a 65 00 78 79|ze.xy| Though I can't reproduce that result with or without the associated commit. -- Alex Kiernan
Re: [PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data
On Tue, May 5, 2020 at 10:17 PM 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 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 wrote: > >> > >> > >> > >> On 5/5/20 3:22 PM, Alex Kiernan wrote: > >> > >>> On Mon, May 4, 2020 at 12:28 PM Tom Rini 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: > >> > > 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 > >> > > |a-offset.data-si| > >> > > 0270 7a 65 00 00 78 79 64 64 > >> > > |ze..xydd| > >> > >^^ ^^ ^^ > >> > > after: > >> > > 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 > >> > > |a-offset.data-si| > >> > > 0270 7a 65 00 78 79|ze.xy| > >> > > > >> > > Signed-off-by: Marek Vasut > >> > > Reviewed-by: Simon Glass > >> > > Cc: Heinrich Schuchardt > >> > > Cc: Tom Rini > >> > > >> > 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 just did the same thing by manually extracting the patches and I agree, reverting that doesn't fix the problem. > 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 > > > > > But Marek's patch affects the FIT image itself, so I am not sure what > > would go after that. > > > > Regards, > > Simon > > -- > -michael -- Alex Kiernan
Re: [PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data
Am 2020-05-05 23:17, schrieb Michael Walle: Hi all, Am 2020-05-05 20:41, schrieb Simon Glass: Hi Tom, On Tue, 5 May 2020 at 11:50, Tom Rini 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 wrote: > >> > >> On 5/5/20 3:22 PM, Alex Kiernan wrote: > >>> On Mon, May 4, 2020 at 12:28 PM Tom Rini 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: > > 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| > > 0270 7a 65 00 00 78 79 64 64 |ze..xydd| > >^^ ^^ ^^ > > after: > > 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| > > 0270 7a 65 00 78 79|ze.xy| > > > > Signed-off-by: Marek Vasut > > Reviewed-by: Simon Glass > > Cc: Heinrich Schuchardt > > Cc: Tom Rini > > 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. Here are the steps to reproduce it on the uboot master branch: $ CROSS_COMPILE=arm-linux-gnueabihf- make pico-imx6_defconfig fit-dtb.blob tools $ tools/dumpimage -T flat_dt -p 1 -o fdt-1 fit-dtb.blob $ file fdt-1 fdt-1: data $ git revert 20a154f95bfe0a3b5bfba90bea7f001c58217536 $ git clean -fdx $ CROSS_COMPILE=arm-linux-gnueabihf- make pico-imx6_defconfig fit-dtb.blob tools $ tools/dumpimage -T flat_dt -p 1 -o fdt-1 fit-dtb.blob $ file fdt-1 fdt-1: Device Tree Blob version 17, size=34978, boot CPU=0, string block size=1746, DT structure block size=33176 -michael
Re: [PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data
Hi all, Am 2020-05-05 20:41, schrieb Simon Glass: Hi Tom, On Tue, 5 May 2020 at 11:50, Tom Rini 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 wrote: > >> > >> On 5/5/20 3:22 PM, Alex Kiernan wrote: > >>> On Mon, May 4, 2020 at 12:28 PM Tom Rini 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: > > 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| > > 0270 7a 65 00 00 78 79 64 64 |ze..xydd| > >^^ ^^ ^^ > > after: > > 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| > > 0270 7a 65 00 78 79|ze.xy| > > > > Signed-off-by: Marek Vasut > > Reviewed-by: Simon Glass > > Cc: Heinrich Schuchardt > > Cc: Tom Rini > > 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 But Marek's patch affects the FIT image itself, so I am not sure what would go after that. Regards, Simon -- -michael
Re: [PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data
Hi Tom, On Tue, 5 May 2020 at 11:50, Tom Rini 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 wrote: > > >> > > >> On 5/5/20 3:22 PM, Alex Kiernan wrote: > > >>> On Mon, May 4, 2020 at 12:28 PM Tom Rini 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: > > > 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 > > > |a-offset.data-si| > > > 0270 7a 65 00 00 78 79 64 64 |ze..xydd| > > >^^ ^^ ^^ > > > after: > > > 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 > > > |a-offset.data-si| > > > 0270 7a 65 00 78 79|ze.xy| > > > > > > Signed-off-by: Marek Vasut > > > Reviewed-by: Simon Glass > > > Cc: Heinrich Schuchardt > > > Cc: Tom Rini > > > > 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. But Marek's patch affects the FIT image itself, so I am not sure what would go after that. Regards, Simon
Re: [PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data
On 5/5/20 8:06 PM, Tom Rini wrote: > On Tue, May 05, 2020 at 07:59:24PM +0200, Marek Vasut wrote: >> On 5/5/20 7:55 PM, Tom Rini wrote: >>> On Tue, May 05, 2020 at 07:53:42PM +0200, Marek Vasut wrote: On 5/5/20 7:50 PM, Tom Rini 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 wrote: On 5/5/20 3:22 PM, Alex Kiernan wrote: > On Mon, May 4, 2020 at 12:28 PM Tom Rini 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: >>> 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 >>> |a-offset.data-si| >>> 0270 7a 65 00 00 78 79 64 64 >>> |ze..xydd| >>>^^ ^^ ^^ >>> after: >>> 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 >>> |a-offset.data-si| >>> 0270 7a 65 00 78 79|ze.xy| >>> >>> Signed-off-by: Marek Vasut >>> Reviewed-by: Simon Glass >>> Cc: Heinrich Schuchardt >>> Cc: Tom Rini >> >> 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. Still, my question is, should the beginning of external data be aligned ? And if so, to what, 4 bytes like FDT entries OR 8 bytes to cater for arm64/rv64 ? >>> >>> Is "external data" the kernel in this case? If so, I swear Linux >>> mandates 8 byte alignment for arm32 as well. >> >> External data can be anything, and if it is supposed to be 8 bytes, we >> already failed at that since forever. > > I would be entirely unsurprised at things working through a combination > of luck and co-incidence in our previous padding working out. So, what > typically is "external data" in this context? Anything you can bundle into the fitImage, -E just puts the content at the end of the fitImage and places a reference into the fitImage itself.
Re: [PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data
On Tue, May 05, 2020 at 07:59:24PM +0200, Marek Vasut wrote: > On 5/5/20 7:55 PM, Tom Rini wrote: > > On Tue, May 05, 2020 at 07:53:42PM +0200, Marek Vasut wrote: > >> On 5/5/20 7:50 PM, Tom Rini 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 wrote: > >> > >> On 5/5/20 3:22 PM, Alex Kiernan wrote: > >>> On Mon, May 4, 2020 at 12:28 PM Tom Rini 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: > > 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 > > |a-offset.data-si| > > 0270 7a 65 00 00 78 79 64 64 > > |ze..xydd| > >^^ ^^ ^^ > > after: > > 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 > > |a-offset.data-si| > > 0270 7a 65 00 78 79|ze.xy| > > > > Signed-off-by: Marek Vasut > > Reviewed-by: Simon Glass > > Cc: Heinrich Schuchardt > > Cc: Tom Rini > > 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. > >> > >> Still, my question is, should the beginning of external data be aligned > >> ? And if so, to what, 4 bytes like FDT entries OR 8 bytes to cater for > >> arm64/rv64 ? > > > > Is "external data" the kernel in this case? If so, I swear Linux > > mandates 8 byte alignment for arm32 as well. > > External data can be anything, and if it is supposed to be 8 bytes, we > already failed at that since forever. I would be entirely unsurprised at things working through a combination of luck and co-incidence in our previous padding working out. So, what typically is "external data" in this context? -- Tom signature.asc Description: PGP signature
Re: [PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data
On 5/5/20 7:55 PM, Tom Rini wrote: > On Tue, May 05, 2020 at 07:53:42PM +0200, Marek Vasut wrote: >> On 5/5/20 7:50 PM, Tom Rini 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 wrote: >> >> On 5/5/20 3:22 PM, Alex Kiernan wrote: >>> On Mon, May 4, 2020 at 12:28 PM Tom Rini 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: > 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 > |a-offset.data-si| > 0270 7a 65 00 00 78 79 64 64 |ze..xydd| >^^ ^^ ^^ > after: > 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 > |a-offset.data-si| > 0270 7a 65 00 78 79|ze.xy| > > Signed-off-by: Marek Vasut > Reviewed-by: Simon Glass > Cc: Heinrich Schuchardt > Cc: Tom Rini 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. >> >> Still, my question is, should the beginning of external data be aligned >> ? And if so, to what, 4 bytes like FDT entries OR 8 bytes to cater for >> arm64/rv64 ? > > Is "external data" the kernel in this case? If so, I swear Linux > mandates 8 byte alignment for arm32 as well. External data can be anything, and if it is supposed to be 8 bytes, we already failed at that since forever.
Re: [PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data
On Tue, May 05, 2020 at 07:53:42PM +0200, Marek Vasut wrote: > On 5/5/20 7:50 PM, Tom Rini 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 wrote: > > On 5/5/20 3:22 PM, Alex Kiernan wrote: > > On Mon, May 4, 2020 at 12:28 PM Tom Rini 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: > >>> 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 > >>> |a-offset.data-si| > >>> 0270 7a 65 00 00 78 79 64 64 |ze..xydd| > >>>^^ ^^ ^^ > >>> after: > >>> 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 > >>> |a-offset.data-si| > >>> 0270 7a 65 00 78 79|ze.xy| > >>> > >>> Signed-off-by: Marek Vasut > >>> Reviewed-by: Simon Glass > >>> Cc: Heinrich Schuchardt > >>> Cc: Tom Rini > >> > >> 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. > > Still, my question is, should the beginning of external data be aligned > ? And if so, to what, 4 bytes like FDT entries OR 8 bytes to cater for > arm64/rv64 ? Is "external data" the kernel in this case? If so, I swear Linux mandates 8 byte alignment for arm32 as well. -- Tom signature.asc Description: PGP signature
Re: [PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data
On 5/5/20 7:50 PM, Tom Rini 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 wrote: On 5/5/20 3:22 PM, Alex Kiernan wrote: > On Mon, May 4, 2020 at 12:28 PM Tom Rini 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: >>> 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 >>> |a-offset.data-si| >>> 0270 7a 65 00 00 78 79 64 64 |ze..xydd| >>>^^ ^^ ^^ >>> after: >>> 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 >>> |a-offset.data-si| >>> 0270 7a 65 00 78 79|ze.xy| >>> >>> Signed-off-by: Marek Vasut >>> Reviewed-by: Simon Glass >>> Cc: Heinrich Schuchardt >>> Cc: Tom Rini >> >> 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. Still, my question is, should the beginning of external data be aligned ? And if so, to what, 4 bytes like FDT entries OR 8 bytes to cater for arm64/rv64 ?
Re: [PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data
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 wrote: > >> > >> On 5/5/20 3:22 PM, Alex Kiernan wrote: > >>> On Mon, May 4, 2020 at 12:28 PM Tom Rini 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: > > 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 > > |a-offset.data-si| > > 0270 7a 65 00 00 78 79 64 64 |ze..xydd| > >^^ ^^ ^^ > > after: > > 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 > > |a-offset.data-si| > > 0270 7a 65 00 78 79|ze.xy| > > > > Signed-off-by: Marek Vasut > > Reviewed-by: Simon Glass > > Cc: Heinrich Schuchardt > > Cc: Tom Rini > > 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. -- Tom signature.asc Description: PGP signature
Re: [PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data
On 5/5/20 6:37 PM, Alex Kiernan wrote: > On Tue, May 5, 2020 at 2:28 PM Marek Vasut wrote: >> >> On 5/5/20 3:22 PM, Alex Kiernan wrote: >>> On Mon, May 4, 2020 at 12:28 PM Tom Rini 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: > 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 > |a-offset.data-si| > 0270 7a 65 00 00 78 79 64 64 |ze..xydd| >^^ ^^ ^^ > after: > 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 > |a-offset.data-si| > 0270 7a 65 00 78 79|ze.xy| > > Signed-off-by: Marek Vasut > Reviewed-by: Simon Glass > Cc: Heinrich Schuchardt > Cc: Tom Rini 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 ?
Re: [PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data
On Tue, May 5, 2020 at 2:28 PM Marek Vasut wrote: > > On 5/5/20 3:22 PM, Alex Kiernan wrote: > > On Mon, May 4, 2020 at 12:28 PM Tom Rini 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: > >>> 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 > >>> |a-offset.data-si| > >>> 0270 7a 65 00 00 78 79 64 64 |ze..xydd| > >>>^^ ^^ ^^ > >>> after: > >>> 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 > >>> |a-offset.data-si| > >>> 0270 7a 65 00 78 79|ze.xy| > >>> > >>> Signed-off-by: Marek Vasut > >>> Reviewed-by: Simon Glass > >>> Cc: Heinrich Schuchardt > >>> Cc: Tom Rini > >> > >> 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: /dts-v1/; // magic: 0xd00dfeed // totalsize: 0x483 (1155) // off_dt_struct: 0x38 // off_dt_strings: 0x3f8 // off_mem_rsvmap: 0x28 // version: 17 // last_comp_version: 2 // boot_cpuid_phys: 0x0 // size_dt_strings: 0x8b // size_dt_struct: 0x3c0 -- Alex Kiernan
Re: [PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data
On 5/5/20 3:22 PM, Alex Kiernan wrote: > On Mon, May 4, 2020 at 12:28 PM Tom Rini 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: >>> 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 >>> |a-offset.data-si| >>> 0270 7a 65 00 00 78 79 64 64 |ze..xydd| >>>^^ ^^ ^^ >>> after: >>> 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 >>> |a-offset.data-si| >>> 0270 7a 65 00 78 79|ze.xy| >>> >>> Signed-off-by: Marek Vasut >>> Reviewed-by: Simon Glass >>> Cc: Heinrich Schuchardt >>> Cc: Tom Rini >> >> 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 ?
Re: [PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data
On Mon, May 4, 2020 at 12:28 PM Tom Rini 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: > > 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 > > |a-offset.data-si| > > 0270 7a 65 00 00 78 79 64 64 |ze..xydd| > >^^ ^^ ^^ > > after: > > 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 > > |a-offset.data-si| > > 0270 7a 65 00 78 79|ze.xy| > > > > Signed-off-by: Marek Vasut > > Reviewed-by: Simon Glass > > Cc: Heinrich Schuchardt > > Cc: Tom Rini > > 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. -- Alex Kiernan
Re: [PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data
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: > 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| > 0270 7a 65 00 00 78 79 64 64 |ze..xydd| >^^ ^^ ^^ > after: > 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| > 0270 7a 65 00 78 79|ze.xy| > > Signed-off-by: Marek Vasut > Reviewed-by: Simon Glass > Cc: Heinrich Schuchardt > Cc: Tom Rini Applied to u-boot/master, thanks! -- Tom signature.asc Description: PGP signature
Re: [PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data
On Fri, 1 May 2020 at 09:40, 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: > 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| > 0270 7a 65 00 00 78 79 64 64 |ze..xydd| >^^ ^^ ^^ > after: > 0260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| > 0270 7a 65 00 78 79|ze.xy| > > Signed-off-by: Marek Vasut > Cc: Heinrich Schuchardt > Cc: Simon Glass > Cc: Tom Rini > --- > V2: Rebase on u-boot/master, update commit message > --- > tools/fit_image.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) Reviewed-by: Simon Glass
