On 10/20/20 4:32 PM, Tom Rini wrote:
> On Tue, Oct 20, 2020 at 04:29:36PM +0200, Marek Vasut wrote:
>> On 10/20/20 4:07 PM, Tom Rini wrote:
>>> On Tue, Oct 20, 2020 at 11:05:40AM +0200, Marek Vasut wrote:
>>>> On 10/20/20 2:27 AM, Reuben Dowle wrote:
>>>>>>> What assumptions? Any code that assumes 4 byte alignment will also work
>>>>>> on 8 byte alignment.
>>>>>>>
>>>>>>> Reverting is not the same as assuming ALIGN(...4) if incoming data is
>>>>>>> not
>>>>>> already aligned to 4 bytes (as was the case when I saw crashes).
>>>>>>
>>>>>> Can the incoming data _not_ be 4 byte aligned ?
>>>>>> How can this be replicated ?
>>>>>
>>>>> In my case I have an offline signing process (separate from build server
>>>>> to keep secure boot keys safe), and this runs a script which also patches
>>>>> the main uboot device tree with some extra properties, then updates main
>>>>> uboot dtb with kernel signature, then finally updates the spl dtb with
>>>>> the uboot signature. I think when mkimage patches the dtb with the
>>>>> signatures, this results in the alignment issues (the unsigned bootloader
>>>>> direct from the uboot make process does not experience this issue).
>>>>>
>>>>> Possibly using mkimage to add padding would also fix the alignment issue
>>>>> I see at boot time.
>>>>>
>>>>>>> Interesting. I had not noticed the -B parameter previously. I had
>>>>>>> originally
>>>>>> fixed this issue on an older version of uboot that did not have that
>>>>>> option,
>>>>>> and later rebased the fix to newer uboot. I would need to do some
>>>>>> testing to
>>>>>> see if this would fix it as well.
>>>>>>
>>>>>> I believe this is the way to handle this if you are building a custom DT
>>>>>> for U-
>>>>>> Boot -- just make sure it has the correct parameters. I think this is
>>>>>> also related
>>>>>> to:
>>>>>> 20a154f95b ("mkimage: fit: Do not tail-pad fitImage with external data")
>>>>>
>>>>> I will look into this, although unfortunately I am very busy with other
>>>>> projects right now so can't retest th
>>>>
>>>> In that case, I would propose to revert this to fix existing boards in
>>>> mainline, and if your tests are not successful, revisit this issue.
>>>>
>>>> I am rather sure what you are hitting is related to the mkimage patch
>>>> above, there was a lengthy discussion on that topic before.
>>>
>>> This gets back to what I was saying earlier in this thread. But to
>>> expand on it, we have been, but cannot, use the same variable for both
>>> "this is where we have the DTB at runtime to use" and "this is where the
>>> DTB happens to exist when we get here". For the case of "we copy the
>>> device tree to $address", $address must be 8 bytes aligned. For the
>>> case of "we use an externally provided DTB in place" I don't like the
>>> idea of, and worry a lot about, assuming it's going to be 8 byte
>>> aligned. But I can set that aside for the moment. That said, in that
>>> second case we need to set $address to where the device tree is.
>>
>> I don't think I understand this paragraph at all ?
>
> OK. Maybe I can better explain it after you walk through how changing
> the "copy the DTB to this address" to be 8 byte aligned is leading to
> failure, as I ask below.
>
>>> That all said, I'm still not quite sure how you're ending up in the
>>> place you're ending up. Which if/else paths in spl_fit_append_fdt() is
>>> your exact platform hitting, and where is what in memory?
>>
>> Is this a question for me or for Reuben ?
>
> For you, the person with the current failure. Please walk me through
> how / where that function is now failing. With address values
> (approximate if you can't get the exact ones).
I currently don't have time for that, sorry. But Alex (on CC) was
debugging it yesterday, so I will defer there.
