Re: [PATCH] Revert "Fix data abort caused by mis-aligning FIT data"
On Mon, Oct 19, 2020 at 11:40:26PM +0200, Marek Vasut wrote: > This reverts commit eb39d8ba5f0d1468b01b89a2a464d18612d3ea76. > 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. > > Signed-off-by: Marek Vasut > Cc: Reuben Dowle > Cc: Tom Rini > Signed-off-by: Marek Vasut Applied to u-boot/master, thanks! -- Tom signature.asc Description: PGP signature
Re: [PATCH] Revert "Fix data abort caused by mis-aligning FIT data"
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 0x3800
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,_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;
Re: [PATCH] Revert "Fix data abort caused by mis-aligning FIT data"
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 0x3800
> > >
> > > 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.
> > */
> >
Re: [PATCH] Revert "Fix data abort caused by mis-aligning FIT data"
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 0x3800
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,_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
Re: [PATCH] Revert "Fix data abort caused by mis-aligning FIT data"
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 0x3800
>
> 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!
--
Tom
signature.asc
Description: PGP signature
Re: [PATCH] Revert "Fix data abort caused by mis-aligning FIT data"
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.
Re: [PATCH] Revert "Fix data abort caused by mis-aligning FIT data"
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 0x3800
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.
Alex
Re: [PATCH] Revert "Fix data abort caused by mis-aligning FIT data"
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).
--
Tom
signature.asc
Description: PGP signature
Re: [PATCH] Revert "Fix data abort caused by mis-aligning FIT data"
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 ?
> 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 ?
Re: [PATCH] Revert "Fix data abort caused by mis-aligning FIT data"
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.
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?
--
Tom
signature.asc
Description: PGP signature
Re: [PATCH] Revert "Fix data abort caused by mis-aligning FIT data"
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.
Re: [PATCH] Revert "Fix data abort caused by mis-aligning FIT data"
On 10/19/20 6:13 PM, Reuben Dowle wrote: 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. The linker script can only control padding of the executable, but won't affect the alignment of the fdt that can be appended to this later by mkimage. Which I've addressed in the second paragraph :) 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 The information in this email communication (inclusive of attachments) is confidential to 4RF Limited and the intended recipient(s). If you are not the intended recipient(s), please note that any use, disclosure, distribution or copying of this information or any part thereof is strictly prohibited and that the author accepts no liability for the consequences of any action taken on the basis of the information provided. If you have received this email in error, please notify the sender immediately by return email and then delete all instances of this email from your system. 4RF Limited will not accept responsibility for any consequences associated with the use of this email (including, but not limited to, damages sustained as a result of any viruses and/or any action or lack of action taken in reliance on it). Per your instructions, I have deleted this message and all copies of it from my computer.
RE: [PATCH] Revert "Fix data abort caused by mis-aligning FIT data"
> > 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
Re: [PATCH] Revert "Fix data abort caused by mis-aligning FIT data"
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)
RE: [PATCH] Revert "Fix data abort caused by mis-aligning FIT data"
> 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.
The linker script can only control padding of the executable, but won't affect
the alignment of the fdt that can be appended to this later by mkimage.
> 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)
Re: [PATCH] Revert "Fix data abort caused by mis-aligning FIT data"
On 10/20/20 1:02 AM, Reuben Dowle wrote:
>> 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).
>
> 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 ?
>> 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.
>
> 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")
RE: [PATCH] Revert "Fix data abort caused by mis-aligning FIT data"
> 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). 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). > > 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. 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.
Re: [PATCH] Revert "Fix data abort caused by mis-aligning FIT data"
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.
Re: [PATCH] Revert "Fix data abort caused by mis-aligning FIT data"
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. -- Tom signature.asc Description: PGP signature
Re: [PATCH] Revert "Fix data abort caused by mis-aligning FIT data"
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.
Re: [PATCH] Revert "Fix data abort caused by mis-aligning FIT data"
On 10/20/20 12:17 AM, Reuben Dowle 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. > > Well if it breaks these boards then we need to do something to fix this. > Maybe reverting this patch is a good idea to fix this breakage, especially > since others were not running into the same issue as me. But I would like to > address the issue I was facing, so we need to figure out how to do something > that works for my situation and supports those other boards. > >> 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. > > The issue I was facing is that spl_image->size was not a multiple of 4 bytes > (alignment depended on the contents of the DT, which got misaligned when it > was updated with secure boot information). The patch you posted aligns the load address to 8 bytes, not to 4 bytes. So why didn't the ALIGN use 4 byte alignment ? > Maybe an alternative way to address this would be to force alignment in the > image generation process (either dtc or mkimage). mkimage -E / -B already can do that for you. > Moving image_info.load_addr by a few bytes to ensure alignment should not > cause any following code to fail. Maybe the SPL is so close to size limit > that wasting the 4 bytes pushes it over the limit? No. If the DT which is supposed to be at the end of the spl_image->load_addr + spl_image->size is suddenly a few bytes off (due to the ALIGN()), then it is inaccessible and the boot fails. The spl_image->load_addr + spl_image->size should already be 4 byte aligned. >>> The version I use in production uses 4 byte alignment, but on advice of Tom >> Rini I extended to 8 bytes. Maybe we could switch to just forcing 4 bytes, >> although I can't see why 8 byte would not work? >>> >>> Note also that I was getting SPL data abort crashes on my arm32 target >> when image size was not 4 byte aligned. So reverting this patch will break my >> platform. >> >> Which platform is that ? > > I am using a custom platform which is based off of the clearfog. I have > changes to uboot to support our custom hardware, secure boot, etc. So, are you triggering the problem with custom-patched U-Boot, not with mainline ? Could it be that some of the custom patches triggered the problem instead ?
Re: [PATCH] Revert "Fix data abort caused by mis-aligning FIT data"
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. -- Tom signature.asc Description: PGP signature
RE: [PATCH] Revert "Fix data abort caused by mis-aligning FIT data"
> -Original Message- > From: Marek Vasut > Sent: Tuesday, 20 October 2020 10:59 am > To: Reuben Dowle ; u-boot@lists.denx.de > Cc: Tom Rini > Subject: Re: [PATCH] Revert "Fix data abort caused by mis-aligning FIT data" > > 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. Well if it breaks these boards then we need to do something to fix this. Maybe reverting this patch is a good idea to fix this breakage, especially since others were not running into the same issue as me. But I would like to address the issue I was facing, so we need to figure out how to do something that works for my situation and supports those other boards. > 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. The issue I was facing is that spl_image->size was not a multiple of 4 bytes (alignment depended on the contents of the DT, which got misaligned when it was updated with secure boot information). Maybe an alternative way to address this would be to force alignment in the image generation process (either dtc or mkimage). Moving image_info.load_addr by a few bytes to ensure alignment should not cause any following code to fail. Maybe the SPL is so close to size limit that wasting the 4 bytes pushes it over the limit? > > The version I use in production uses 4 byte alignment, but on advice of Tom > Rini I extended to 8 bytes. Maybe we could switch to just forcing 4 bytes, > although I can't see why 8 byte would not work? > > > > Note also that I was getting SPL data abort crashes on my arm32 target > when image size was not 4 byte aligned. So reverting this patch will break my > platform. > > Which platform is that ? I am using a custom platform which is based off of the clearfog. I have changes to uboot to support our custom hardware, secure boot, etc.
Re: [PATCH] Revert "Fix data abort caused by mis-aligning FIT data"
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. > The version I use in production uses 4 byte alignment, but on advice of Tom > Rini I extended to 8 bytes. Maybe we could switch to just forcing 4 bytes, > although I can't see why 8 byte would not work? > > Note also that I was getting SPL data abort crashes on my arm32 target when > image size was not 4 byte aligned. So reverting this patch will break my > platform. Which platform is that ? (also, please do not top-post) [...] > This reverts commit eb39d8ba5f0d1468b01b89a2a464d18612d3ea76. > 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. > > Signed-off-by: Marek Vasut > Cc: Reuben Dowle > Cc: Tom Rini > --- > common/spl/spl_fit.c | 7 ++- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c index > 0e27ad1d6a..a90d821c82 100644 > --- a/common/spl/spl_fit.c > +++ b/common/spl/spl_fit.c > @@ -349,12 +349,9 @@ static int spl_fit_append_fdt(struct spl_image_info > *spl_image, > > /* > * Use the address following the image as target address for the > - * device tree. Load address is aligned to 8 bytes to match the required > - * alignment specified for linux arm [1] and arm 64 [2] booting > - * [1]: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/arm/booting.rst#n126 > - * [2]: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/arm64/booting.rst#n45 > + * device tree. > */ > -image_info.load_addr = ALIGN(spl_image->load_addr + spl_image->size, 8); > +image_info.load_addr = spl_image->load_addr + spl_image->size; > > /* Figure out which device tree the board wants to use */ > node = spl_fit_get_image_node(fit, images, FIT_FDT_PROP, index++); > -- > 2.28.0 [...]
RE: [PATCH] Revert "Fix data abort caused by mis-aligning FIT data"
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. The version I use in production uses 4 byte alignment, but on advice of Tom Rini I extended to 8 bytes. Maybe we could switch to just forcing 4 bytes, although I can't see why 8 byte would not work? Note also that I was getting SPL data abort crashes on my arm32 target when image size was not 4 byte aligned. So reverting this patch will break my platform. -Original Message- From: Marek Vasut Sent: Tuesday, 20 October 2020 10:40 am To: u-boot@lists.denx.de Cc: Marek Vasut ; Reuben Dowle ; Tom Rini Subject: [PATCH] Revert "Fix data abort caused by mis-aligning FIT data" This reverts commit eb39d8ba5f0d1468b01b89a2a464d18612d3ea76. 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. Signed-off-by: Marek Vasut Cc: Reuben Dowle Cc: Tom Rini --- common/spl/spl_fit.c | 7 ++- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c index 0e27ad1d6a..a90d821c82 100644 --- a/common/spl/spl_fit.c +++ b/common/spl/spl_fit.c @@ -349,12 +349,9 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image, /* * Use the address following the image as target address for the -* device tree. Load address is aligned to 8 bytes to match the required -* alignment specified for linux arm [1] and arm 64 [2] booting -* [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/arm/booting.rst#n126 -* [2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/arm64/booting.rst#n45 +* device tree. */ - image_info.load_addr = ALIGN(spl_image->load_addr + spl_image->size, 8); + image_info.load_addr = spl_image->load_addr + spl_image->size; /* Figure out which device tree the board wants to use */ node = spl_fit_get_image_node(fit, images, FIT_FDT_PROP, index++); -- 2.28.0
