On 10/20/20 1:10 PM, Tom Rini wrote:
On Tue, Oct 20, 2020 at 12:01:02PM -0500, Alex G. wrote:
On 10/20/20 10:54 AM, Tom Rini wrote:
On Tue, Oct 20, 2020 at 09:38:52AM -0500, Alex G. wrote:
On 10/20/20 9:32 AM, 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.
(gdb) print gd->fdt_blob
$13 = (const void *) 0xc01cf85c
(gdb) x/4x gd->fdt_blob
0xc01cf85c: 0xc01b814c 0xedfe0dd0 0xa0670100 0x38000000
Notice how u-boot thinks fdt_blob is at 0xc01cf85c. However it was actually
loaded at 0xc01cf860, as evidenced by the doodfeed signature. This is with
the 8-byte alignment.
Ah, thanks. So we're in this part of the code (conditions changed to results):
if (TRUE) {
debug("%s: cannot find FDT node\n", __func__);
/*
* U-Boot did not find a device tree inside the FIT image. Use
* the U-Boot device tree instead.
*/
if (TRUE)
memcpy((void *)image_info.load_addr, gd->fdt_blob,
fdt_totalsize(gd->fdt_blob));
So, our destination is what changed but gd->fdt_blob is 4 lower than the
correct value. Isn't that the problem? Or am I missing something
still? Thanks!
That is incorrect:
node = 465;
...
} else {
ret = spl_load_fit_image(info, sector, fit, base_offset,
node,&image_info);
I think there is confusion between the writer of the FDT and the reader.
This code is in SPL, the writer. It writes the FDT to an address I shall
call x1. The reader, which is u-boot TPL, will look at address x2.
It's obvious that for the FDT to be passed successfully, x1 == x2
## For x1:
image_info.load_addr = ALIGN(spl_image->load_addr + spl_image->size, 8);
(gdb) print/x (spl_image->load_addr + spl_image->size)
$19 = 0xc01cf85c
(gdb) print/x image_info->load_addr
$20 = 0xc01cf860
x1 is 0xc01cf860
## For x2:
__weak void *board_fdt_blob_setup(void)
{
/* FDT is at end of image */
fdt_blob = (ulong *)&_end;
(gdb) print &_end
$22 = (char (*)[]) 0xc01cf85c
x2 = 0xc01cf85c
As I have hopefully demonstrated, SPL and TPL do _not_ agree on the location
of the FDT, hence the failure.
Thanks, that's helpful. I guess we have a few things to sort out.
First, we don't have a general good way to pass from prior stage to next
stage "the device tree to use is HERE". That's I think why we're in
this particular mess. Second, the comment in spl_fit_append_fdt() needs
to be made to reflect where the consumer will be looking for it. Third,
and harder, is that we need to make sure that we're using the device
tree at an 8 byte aligned address, due to the alignment requirements of
our overall next consumers.
I don't expect you (or Marek) to solve #1. For #2, a follow-up on the
revert to clarify the comment block would be much appreciated. For #3,
that too requires some harder work as to me the more stringent
requirement is that once full U-Boot is at the point of using and
validating the device tree, it needs to be 8-byte aligned.
Let's do this. Let's merge this revert, and un-break things.
Re-introduce the FDT alignment once the other issues are ironed out.
Regarding #1, why not use r2, like everyone else? At least on arm.
Alex
