From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alex G. Date: Mon, 19 Oct 2020 18:09:52 -0500 Subject: [PATCH] Revert "Fix data abort caused by mis-aligning FIT data" In-Reply-To: References: <20201019214026.888876-1-marex@denx.de> <669974501aec485c9263fe5127c4ccb5@4rf.com> <38fc6118-b7d7-8455-2c05-3cbe368c4f12@denx.de> <20201019224558.GR14816@bill-the-cat> <4f9c0170-1c52-0f2d-c6f8-972ed082cd36@denx.de> <20201019225819.GS14816@bill-the-cat> Message-ID: <3c989798-84a0-256b-e455-6ff2b5a9390f@gmail.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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)