[PATCH] spl: Align device tree blob address at 8-byte boundary

[Marek Vasut]

On 7/12/21 6:02 PM, Tom Rini wrote:
> On Mon, Jul 12, 2021 at 05:51:29PM +0200, Marek Vasut wrote:
>> On 7/12/21 5:43 PM, Tom Rini wrote:
>>> On Mon, Jul 12, 2021 at 05:38:33PM +0200, Marek Vasut wrote:
>>>> On 7/12/21 5:15 PM, Tom Rini wrote:
>>>>> On Mon, Jul 12, 2021 at 01:36:14PM +0800, Bin Meng wrote:
>>>>>> On Mon, Jul 12, 2021 at 1:21 PM Reuben Dowle <reuben.dowle at 4rf.com> wrote:
>>>>>>>
>>>>>>> I submitted an almost identical patch. See https://github.com/u-boot/u-boot/commit/eb39d8ba5f0d1468b01b89a2a464d18612d3ea76
>>>>>>>
>>>>>>> This patch eventually had to be reverted (https://github.com/u-boot/u-boot/commit/5675ed7cb645f5ec13958726992daeeed16fd114), because it was causing issues on some platforms that had FIT on 32 bit boundary. However I continue to use it in production code, as without it the boot on my platform aborts.
>>>>>>>
>>>>>>> I don't have time to investigate why this was happening, but you need to check this code won't just cause exactly the same faults.
>>>>>>
>>>>>> Thanks for your information.
>>>>>>
>>>>>> +Marek who did the revert
>>>>>>
>>>>>> The revert commit message says:
>>>>>>
>>>>>>        "The commit breaks booting of fitImage by SPL, the system simply
>>>>>> hangs. This is because on arm32, the fitImage and all of its content
>>>>>> can be aligned to 4 bytes and U-Boot expects just that."
>>>>>>
>>>>>> I don't understand this. If an address is aligned to 8, it is already
>>>>>> aligned to 4, so how did this commit make the system hang on arm32?
>>>>>
>>>>> I think this had something to do with embedding contents somewhere in
>>>>> the image?  There is a thread on the ML from then but I don't know how
>>>>> informative it will end up being.
>>>>
>>>> If I recall this correctly, DT node alignment is 4 byte and that is what DTC
>>>> emits. If you have fitImage with embedded data, you basically end up with
>>>>
>>>> / {
>>>>    prop1 = "string1";
>>>>    prop2 = "string2";
>>>> };
>>>>
>>>> where the "string2" is aligned to 4 bytes. And that is what U-Boot expects
>>>> when it tries to access those data in-place in SPL.
>>>>
>>>> The problem with the reverted patch was that it made U-Boot assume the
>>>> alignment is 8 bytes, and that actually works only if you use fitImage with
>>>> external data (mkimage -E), but with embedded data (mkimage default) not so
>>>> much. That caused off-by-4 error in some cases and that made the SPL hang.
>>>>
>>>>>> Note, as I indicated in this patch, now with libfdt 1.6.1, the
>>>>>> alignment to 8 byte is a must-have. So we have to do such alignment
>>>>>> anyway.
>>>>>>
>>>>>> @Tom may fill in why libfdt commit commit 5e735860c478 ("libfdt: Check
>>>>>> for 8-byte address alignment in fdt_ro_probe_()") was made to have the
>>>>>> 8-byte alignment requirement.
>>>>>
>>>>> Note that it's not so much since libfdt 1.6.1 but that since always the
>>>>> device tree has required 8 byte alignment.
>>>>
>>>> DT alignment was always 4 byte , no ?
>>>
>>> I'm pretty sure, no, 8 byte base alignment is a pretty much always
>>> thing.  I don't have a reference handy but I also know I couldn't have
>>> convinced dgibson to add the check otherwise.
>>
>> DTSpec rev 0.3 says the following and I think you got confused by section
>> 5.6 which talks about alignment of the entire tree, not its nodes:
>>
>> 5.4 Structure Block
>> "
>> Each token in the structure block, and thus the structure block itself,
>> shall be located at a 4-byte aligned offset from the
>> beginning of the devicetree blob (see 5.6).
>> "
>>
>> 5.4.2 Tree structure
>> "
>> For each property of the node:
>> ...
>> – FDT_PROP token
>> ...
>> * [zeroed padding bytes to align to a 4-byte boundary]
>> "
>>
>> 5.5 Strings Block
>> "
>> The strings block contains strings representing all the property names used
>> in the tree. These null terminated strings are
>> simply concatenated together in this section, and referred to from the
>> structure block by an offset into the strings block.
>> The strings block has no alignment constraints and may appear at any offset
>> from the beginning of the devicetree blob.
>> "
>>
>> 5.6 Alignment
>> "
>> As described in the previous sections, the structure and strings blocks
>> shall have aligned offsets from the beginning of
>> the devicetree blob. To ensure the in-memory alignment of the blocks, it is
>> sufficient to ensure that the devicetree as a
>> whole is loaded at an address aligned to the largest alignment of any of the
>> subblocks, that is, to an 8-byte boundary.
> 
> Right.  A device tree must start at an 8-byte boundary

Start, yes ; string alignment, no.

> and U-Boot was
> violating this both with:
> - All of the boards that use fdt_high=0xffffffff to disable relocation,

Not applicable to SPL case.

>    and then then place things at arbitrary spots in memory that may or
>    may not violate these requirements.
> - Perhaps some of the FIT internals where we have a device tree inside a
>    device tree?  And we need to fixup whatever we're doing there that's
>    wrong.

And that too. On arm32 and legacy setups this is fine, on arm64 and all 
new setups, use mkimage -E to avoid running into this.