[PATCH] Revert "Fix data abort caused by mis-aligning FIT data"

[Alex G.]

On 10/19/20 6:02 PM, Marek Vasut wrote:
> On 10/20/20 12:58 AM, Tom Rini wrote:
>> On Tue, Oct 20, 2020 at 12:54:35AM +0200, Marek Vasut wrote:
>>> On 10/20/20 12:45 AM, Tom Rini wrote:
>>>> On Mon, Oct 19, 2020 at 11:59:22PM +0200, Marek Vasut wrote:
>>>>> On 10/19/20 11:50 PM, Reuben Dowle wrote:
>>>>>> The alignment of 8 bytes would also work if code was expecting 4 byte alignment. So the explanation you give for reverting this does not make sense to me.
>>>>>
>>>>> Well, since U-Boot 2020.10-rc5, any STM32MP1 board does no longer boot
>>>>> and if I revert this patch, it works again (per git bisect). But this
>>>>> also applies to any other arm32 boards which load fitImage in SPL, all
>>>>> of those boards are broken in U-Boot 2020.10.
>>>>>
>>>>> It seems that the end of the U-Boot image is at 4-byte aligned offset on
>>>>> arm32, and that is where the DT is also loaded ; but your patch forces
>>>>> the alignment to 8-bytes, so suddenly the DT location is 4-bytes off.
>>>>
>>>> I think this needs some more investigation to figure out what's going
>>>> on and where the underlying bugs are.  This section of the code is where
>>>> U-Boot is saying it will copy the device tree to.  If we're using a
>>>> device tree in place that's NOT being copied (and someone else has
>>>> ensured 8 byte alignment of) we need to set the address of where the
>>>> device tree is, at that time.
>>>
>>> The problem is that the previous alignment was 4 byte, now it is 8 byte
>>> and that breaks all the other assumptions. So, this patch should be
>>> reverted to fix the platforms which used to work (or use ALIGN(...4),
>>> which is the same as reverting it really).
>>>
>>> And likely the signed image which caused the breakage should be
>>> generated with mkimage -E / -B 8, which would insure the alignment, so
>>> then there is no need to change anything in the code itself.
>>
>> 4 byte alignment is wrong.
> 
> Please elaborate why is 4 byte alignment wrong? 8 byte alignment
> obviously breaks existing boards which are in-tree and worked for
> multiple releases.

The reverted change linked to some kernel documentation that requires 
64-bit alignment. I agree with the alignment requirement.

Im my opinion, there are two things that need to be done:

First is to look at an ALIGNED address for the fdt. A summary inspection 
of board_fdt_blob_setup() tells us this is done via the "_end" linker 
symbol.

Second is to put things in the right place. For FIT, the code, as is, is 
correct, but this alignment is not guaranteed for legacy images. I think 
somebody mentioned changing the arguments to mkimage to achieve this.

I've tried to fix the first point by aligning the _end symbol (appendix 
A). Unfortunately, this is causing other build issues that I don't know 
how to deal with.

Alex


APPENDIX A:


-- a/arch/arm/cpu/u-boot.lds
+++ b/arch/arm/cpu/u-boot.lds
@@ -196,7 +196,6 @@ SECTIONS
          * for FIT images.
          * See common/spl/spl_fit.c: spl_fit_append_fdt
          */
+       . = ALIGN(8);
         .end :
         {
                 *(.__end)