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

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

[Bin Meng]

Since libfdt v1.6.1, a new requirement on the device tree address via:

  commit 5e735860c478 ("libfdt: Check for 8-byte address alignment in fdt_ro_probe_()")

must be met that the device tree must be loaded in to memory at an
8-byte aligned address.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 common/spl/spl_fit.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index f41abca0cc..9baf6aca9f 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -374,6 +374,12 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image,
 	 */
 	image_info.load_addr = spl_image->load_addr + spl_image->size;
 
+	/*
+	 * Since libfdt v1.6.1, the device tree must be loaded in to memory
+	 * at an 8-byte aligned address.
+	 */
+	image_info.load_addr = roundup(image_info.load_addr, 8);
+
 	/* Figure out which device tree the board wants to use */
 	node = spl_fit_get_image_node(ctx, FIT_FDT_PROP, index++);
 	if (node < 0) {
-- 
2.25.1

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

[Reuben Dowle]

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.

> -----Original Message-----
> From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Bin Meng
> Sent: Monday, 12 July 2021 3:53 pm
> To: Tom Rini <trini@konsulko.com>; Simon Glass <sjg@chromium.org>; u-
> boot@lists.denx.de
> Cc: Bin Meng <bmeng.cn@gmail.com>
> Subject: [PATCH] spl: Align device tree blob address at 8-byte boundary
> 
> Since libfdt v1.6.1, a new requirement on the device tree address via:
> 
>   commit 5e735860c478 ("libfdt: Check for 8-byte address alignment in
> fdt_ro_probe_()")
> 
> must be met that the device tree must be loaded in to memory at an 8-byte
> aligned address.
> 
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
> 
>  common/spl/spl_fit.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c index
> f41abca0cc..9baf6aca9f 100644
> --- a/common/spl/spl_fit.c
> +++ b/common/spl/spl_fit.c
> @@ -374,6 +374,12 @@ static int spl_fit_append_fdt(struct spl_image_info
> *spl_image,
>  	 */
>  	image_info.load_addr = spl_image->load_addr + spl_image->size;
> 
> +	/*
> +	 * Since libfdt v1.6.1, the device tree must be loaded in to memory
> +	 * at an 8-byte aligned address.
> +	 */
> +	image_info.load_addr = roundup(image_info.load_addr, 8);
> +
>  	/* Figure out which device tree the board wants to use */
>  	node = spl_fit_get_image_node(ctx, FIT_FDT_PROP, index++);
>  	if (node < 0) {
> --
> 2.25.1

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

[Bin Meng]

On Mon, Jul 12, 2021 at 1:21 PM Reuben Dowle <reuben.dowle@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?

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.

>
> -----Original Message-----
> > From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Bin Meng
> > Sent: Monday, 12 July 2021 3:53 pm
> > To: Tom Rini <trini@konsulko.com>; Simon Glass <sjg@chromium.org>; u-
> > boot@lists.denx.de
> > Cc: Bin Meng <bmeng.cn@gmail.com>
> > Subject: [PATCH] spl: Align device tree blob address at 8-byte boundary
> >
> > Since libfdt v1.6.1, a new requirement on the device tree address via:
> >
> >   commit 5e735860c478 ("libfdt: Check for 8-byte address alignment in
> > fdt_ro_probe_()")
> >
> > must be met that the device tree must be loaded in to memory at an 8-byte
> > aligned address.
> >
> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > ---
> >
> >  common/spl/spl_fit.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c index
> > f41abca0cc..9baf6aca9f 100644
> > --- a/common/spl/spl_fit.c
> > +++ b/common/spl/spl_fit.c
> > @@ -374,6 +374,12 @@ static int spl_fit_append_fdt(struct spl_image_info
> > *spl_image,
> >   */
> >  image_info.load_addr = spl_image->load_addr + spl_image->size;
> >
> > +/*
> > + * Since libfdt v1.6.1, the device tree must be loaded in to memory
> > + * at an 8-byte aligned address.
> > + */
> > +image_info.load_addr = roundup(image_info.load_addr, 8);
> > +
> >  /* Figure out which device tree the board wants to use */
> >  node = spl_fit_get_image_node(ctx, FIT_FDT_PROP, index++);
> >  if (node < 0) {
> > --

Regards,
Bin

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1949 bytes --]

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@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.

> 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.  It's just that on 32bit
platforms 4-but-not-8 byte alignment tends to not be fatal but on 64bit
platforms it is.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Marek Vasut]

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@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 ?

> It's just that on 32bit
> platforms 4-but-not-8 byte alignment tends to not be fatal but on 64bit
> platforms it is.

[...]

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2989 bytes --]

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@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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Marek Vasut]

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@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.
"

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 5130 bytes --]

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@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 and U-Boot was
violating this both with:
- All of the boards that use fdt_high=0xffffffff to disable relocation,
  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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [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@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.

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

[Alex G.]

On 7/12/21 10:15 AM, 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@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.

It's true that the flat devicetree spec requires an 8-byte alignment, 
even on 32-bit. The issues here are specific to u-boot.

SPL and u-boot have to agree where u-boot's FDT is located. We'll look 
at two cases:
	1) u-boot as a FIT (binary and FDT separately loaded)
	2) u-boot with embedded FDT

In case (1) SPL must place the FDT at a location where u-boot will find 
it. The current logic is
	SPL:	fdt = ALIGN_4(u_boot + u_boot_size)
	u-boot:	fdt = ALIGN_4(u_boot + u_boot_size)

In case (2), SPL's view of the FDT is not relevant, but instead the 
build system must place the FDT correctly:
	build:	fdt >> u-boot.bin
	u-boot:	fdt = ALIGN_4(u_boot + u_boot_size)

We have 3 places that must agree. A correct and complete patch could 
change all three, but one has to consider compatibility issues when 
crossing u-boot and SPL versions.

I had proposed in the revert discussion that SPL use r2 or similar 
mechanism to pass the location of the FDT to u-boot.

Alex

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

[Simon Glass]

Hi Alex,

On Mon, 12 Jul 2021 at 10:01, Alex G. <mr.nuke.me@gmail.com> wrote:
>
> On 7/12/21 10:15 AM, 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@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.
>
> It's true that the flat devicetree spec requires an 8-byte alignment,
> even on 32-bit. The issues here are specific to u-boot.
>
> SPL and u-boot have to agree where u-boot's FDT is located. We'll look
> at two cases:
>         1) u-boot as a FIT (binary and FDT separately loaded)
>         2) u-boot with embedded FDT
>
> In case (1) SPL must place the FDT at a location where u-boot will find
> it. The current logic is
>         SPL:    fdt = ALIGN_4(u_boot + u_boot_size)
>         u-boot: fdt = ALIGN_4(u_boot + u_boot_size)
>
> In case (2), SPL's view of the FDT is not relevant, but instead the
> build system must place the FDT correctly:
>         build:  fdt >> u-boot.bin
>         u-boot: fdt = ALIGN_4(u_boot + u_boot_size)
>
> We have 3 places that must agree. A correct and complete patch could
> change all three, but one has to consider compatibility issues when
> crossing u-boot and SPL versions.
>
> I had proposed in the revert discussion that SPL use r2 or similar
> mechanism to pass the location of the FDT to u-boot.

Just on that specific point, we should use the SPL handoff info in a bloblist.

Regards,
Simon

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

[Bin Meng]

On Tue, Jul 13, 2021 at 12:01 AM Alex G. <mr.nuke.me@gmail.com> wrote:
>
> On 7/12/21 10:15 AM, 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@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.
>
> It's true that the flat devicetree spec requires an 8-byte alignment,
> even on 32-bit. The issues here are specific to u-boot.
>
> SPL and u-boot have to agree where u-boot's FDT is located. We'll look
> at two cases:
>         1) u-boot as a FIT (binary and FDT separately loaded)
>         2) u-boot with embedded FDT

Is this CONFIG_OF_EMBED?

>
> In case (1) SPL must place the FDT at a location where u-boot will find
> it. The current logic is
>         SPL:    fdt = ALIGN_4(u_boot + u_boot_size)

I don't see there is even a ALIGN_4 in current SPL logic, but it
happens to be 4 in all cases I think.

>         u-boot: fdt = ALIGN_4(u_boot + u_boot_size)
>
> In case (2), SPL's view of the FDT is not relevant, but instead the
> build system must place the FDT correctly:
>         build:  fdt >> u-boot.bin
>         u-boot: fdt = ALIGN_4(u_boot + u_boot_size)
>
> We have 3 places that must agree. A correct and complete patch could
> change all three, but one has to consider compatibility issues when
> crossing u-boot and SPL versions.
>
> I had proposed in the revert discussion that SPL use r2 or similar
> mechanism to pass the location of the FDT to u-boot.

Regards,
Bin

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2858 bytes --]

On Mon, Jul 12, 2021 at 11:01:24AM -0500, Alex G. wrote:
> On 7/12/21 10:15 AM, 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@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.
> 
> It's true that the flat devicetree spec requires an 8-byte alignment, even
> on 32-bit. The issues here are specific to u-boot.
> 
> SPL and u-boot have to agree where u-boot's FDT is located. We'll look at
> two cases:
> 	1) u-boot as a FIT (binary and FDT separately loaded)
> 	2) u-boot with embedded FDT
> 
> In case (1) SPL must place the FDT at a location where u-boot will find it.
> The current logic is
> 	SPL:	fdt = ALIGN_4(u_boot + u_boot_size)
> 	u-boot:	fdt = ALIGN_4(u_boot + u_boot_size)
> 
> In case (2), SPL's view of the FDT is not relevant, but instead the build
> system must place the FDT correctly:
> 	build:	fdt >> u-boot.bin
> 	u-boot:	fdt = ALIGN_4(u_boot + u_boot_size)
> 
> We have 3 places that must agree. A correct and complete patch could change
> all three, but one has to consider compatibility issues when crossing u-boot
> and SPL versions.
> 
> I had proposed in the revert discussion that SPL use r2 or similar mechanism
> to pass the location of the FDT to u-boot.

I'm not sure that we need to worry too much about mix-and-match
SPL/U-Boot, but documenting what to go change if you must do it
somewhere under doc/ would be good.  I think we can just switch to
ALIGN(8) not ALIGN(4) and be done with it?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Marek Vasut]

On 7/13/21 3:47 PM, Tom Rini wrote:
> On Mon, Jul 12, 2021 at 11:01:24AM -0500, Alex G. wrote:
>> On 7/12/21 10:15 AM, 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@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.
>>
>> It's true that the flat devicetree spec requires an 8-byte alignment, even
>> on 32-bit. The issues here are specific to u-boot.
>>
>> SPL and u-boot have to agree where u-boot's FDT is located. We'll look at
>> two cases:
>> 	1) u-boot as a FIT (binary and FDT separately loaded)
>> 	2) u-boot with embedded FDT
>>
>> In case (1) SPL must place the FDT at a location where u-boot will find it.
>> The current logic is
>> 	SPL:	fdt = ALIGN_4(u_boot + u_boot_size)
>> 	u-boot:	fdt = ALIGN_4(u_boot + u_boot_size)
>>
>> In case (2), SPL's view of the FDT is not relevant, but instead the build
>> system must place the FDT correctly:
>> 	build:	fdt >> u-boot.bin
>> 	u-boot:	fdt = ALIGN_4(u_boot + u_boot_size)
>>
>> We have 3 places that must agree. A correct and complete patch could change
>> all three, but one has to consider compatibility issues when crossing u-boot
>> and SPL versions.
>>
>> I had proposed in the revert discussion that SPL use r2 or similar mechanism
>> to pass the location of the FDT to u-boot.
> 
> I'm not sure that we need to worry too much about mix-and-match
> SPL/U-Boot, but documenting what to go change if you must do it
> somewhere under doc/ would be good.  I think we can just switch to
> ALIGN(8) not ALIGN(4) and be done with it?

Remember, there is also falcon boot. And we definitely have to be able 
to have old u-boot (SPL) boot new fitImage and vice versa.

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3565 bytes --]

On Tue, Jul 13, 2021 at 04:35:38PM +0200, Marek Vasut wrote:
> On 7/13/21 3:47 PM, Tom Rini wrote:
> > On Mon, Jul 12, 2021 at 11:01:24AM -0500, Alex G. wrote:
> > > On 7/12/21 10:15 AM, 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@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.
> > > 
> > > It's true that the flat devicetree spec requires an 8-byte alignment, even
> > > on 32-bit. The issues here are specific to u-boot.
> > > 
> > > SPL and u-boot have to agree where u-boot's FDT is located. We'll look at
> > > two cases:
> > > 	1) u-boot as a FIT (binary and FDT separately loaded)
> > > 	2) u-boot with embedded FDT
> > > 
> > > In case (1) SPL must place the FDT at a location where u-boot will find it.
> > > The current logic is
> > > 	SPL:	fdt = ALIGN_4(u_boot + u_boot_size)
> > > 	u-boot:	fdt = ALIGN_4(u_boot + u_boot_size)
> > > 
> > > In case (2), SPL's view of the FDT is not relevant, but instead the build
> > > system must place the FDT correctly:
> > > 	build:	fdt >> u-boot.bin
> > > 	u-boot:	fdt = ALIGN_4(u_boot + u_boot_size)
> > > 
> > > We have 3 places that must agree. A correct and complete patch could change
> > > all three, but one has to consider compatibility issues when crossing u-boot
> > > and SPL versions.
> > > 
> > > I had proposed in the revert discussion that SPL use r2 or similar mechanism
> > > to pass the location of the FDT to u-boot.
> > 
> > I'm not sure that we need to worry too much about mix-and-match
> > SPL/U-Boot, but documenting what to go change if you must do it
> > somewhere under doc/ would be good.  I think we can just switch to
> > ALIGN(8) not ALIGN(4) and be done with it?
> 
> Remember, there is also falcon boot. And we definitely have to be able to
> have old u-boot (SPL) boot new fitImage and vice versa.

I don't follow you, sorry.  But since you seem to have the best
understanding of where all of the cases something could go wrong here,
can you perhaps post an RFC patch?  That is likely to be clearer than
another long thread here.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Marek Vasut]

On 7/13/21 4:41 PM, Tom Rini wrote:
> On Tue, Jul 13, 2021 at 04:35:38PM +0200, Marek Vasut wrote:
>> On 7/13/21 3:47 PM, Tom Rini wrote:
>>> On Mon, Jul 12, 2021 at 11:01:24AM -0500, Alex G. wrote:
>>>> On 7/12/21 10:15 AM, 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@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.
>>>>
>>>> It's true that the flat devicetree spec requires an 8-byte alignment, even
>>>> on 32-bit. The issues here are specific to u-boot.
>>>>
>>>> SPL and u-boot have to agree where u-boot's FDT is located. We'll look at
>>>> two cases:
>>>> 	1) u-boot as a FIT (binary and FDT separately loaded)
>>>> 	2) u-boot with embedded FDT
>>>>
>>>> In case (1) SPL must place the FDT at a location where u-boot will find it.
>>>> The current logic is
>>>> 	SPL:	fdt = ALIGN_4(u_boot + u_boot_size)
>>>> 	u-boot:	fdt = ALIGN_4(u_boot + u_boot_size)
>>>>
>>>> In case (2), SPL's view of the FDT is not relevant, but instead the build
>>>> system must place the FDT correctly:
>>>> 	build:	fdt >> u-boot.bin
>>>> 	u-boot:	fdt = ALIGN_4(u_boot + u_boot_size)
>>>>
>>>> We have 3 places that must agree. A correct and complete patch could change
>>>> all three, but one has to consider compatibility issues when crossing u-boot
>>>> and SPL versions.
>>>>
>>>> I had proposed in the revert discussion that SPL use r2 or similar mechanism
>>>> to pass the location of the FDT to u-boot.
>>>
>>> I'm not sure that we need to worry too much about mix-and-match
>>> SPL/U-Boot, but documenting what to go change if you must do it
>>> somewhere under doc/ would be good.  I think we can just switch to
>>> ALIGN(8) not ALIGN(4) and be done with it?
>>
>> Remember, there is also falcon boot. And we definitely have to be able to
>> have old u-boot (SPL) boot new fitImage and vice versa.
> 
> I don't follow you, sorry.  But since you seem to have the best
> understanding of where all of the cases something could go wrong here,
> can you perhaps post an RFC patch?  That is likely to be clearer than
> another long thread here.

I don't follow you, sorry. I believe the revert did the right thing and 
new systems should use mkimage -E when generating fitImages, to avoid 
the string alignment problem. That is all.

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

[Simon Glass]

Hi Marek,

On Tue, 13 Jul 2021 at 08:53, Marek Vasut <marex@denx.de> wrote:
>
> On 7/13/21 4:41 PM, Tom Rini wrote:
> > On Tue, Jul 13, 2021 at 04:35:38PM +0200, Marek Vasut wrote:
> >> On 7/13/21 3:47 PM, Tom Rini wrote:
> >>> On Mon, Jul 12, 2021 at 11:01:24AM -0500, Alex G. wrote:
> >>>> On 7/12/21 10:15 AM, 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@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.
> >>>>
> >>>> It's true that the flat devicetree spec requires an 8-byte alignment, even
> >>>> on 32-bit. The issues here are specific to u-boot.
> >>>>
> >>>> SPL and u-boot have to agree where u-boot's FDT is located. We'll look at
> >>>> two cases:
> >>>>    1) u-boot as a FIT (binary and FDT separately loaded)
> >>>>    2) u-boot with embedded FDT
> >>>>
> >>>> In case (1) SPL must place the FDT at a location where u-boot will find it.
> >>>> The current logic is
> >>>>    SPL:    fdt = ALIGN_4(u_boot + u_boot_size)
> >>>>    u-boot: fdt = ALIGN_4(u_boot + u_boot_size)
> >>>>
> >>>> In case (2), SPL's view of the FDT is not relevant, but instead the build
> >>>> system must place the FDT correctly:
> >>>>    build:  fdt >> u-boot.bin
> >>>>    u-boot: fdt = ALIGN_4(u_boot + u_boot_size)
> >>>>
> >>>> We have 3 places that must agree. A correct and complete patch could change
> >>>> all three, but one has to consider compatibility issues when crossing u-boot
> >>>> and SPL versions.
> >>>>
> >>>> I had proposed in the revert discussion that SPL use r2 or similar mechanism
> >>>> to pass the location of the FDT to u-boot.
> >>>
> >>> I'm not sure that we need to worry too much about mix-and-match
> >>> SPL/U-Boot, but documenting what to go change if you must do it
> >>> somewhere under doc/ would be good.  I think we can just switch to
> >>> ALIGN(8) not ALIGN(4) and be done with it?
> >>
> >> Remember, there is also falcon boot. And we definitely have to be able to
> >> have old u-boot (SPL) boot new fitImage and vice versa.
> >
> > I don't follow you, sorry.  But since you seem to have the best
> > understanding of where all of the cases something could go wrong here,
> > can you perhaps post an RFC patch?  That is likely to be clearer than
> > another long thread here.
>
> I don't follow you, sorry. I believe the revert did the right thing and
> new systems should use mkimage -E when generating fitImages, to avoid
> the string alignment problem. That is all.

Using -E should be optional and things really should work without it.

Regards,
Simon

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

[Marek Vasut]

On 7/13/21 6:47 PM, Simon Glass wrote:
> Hi Marek,
> 
> On Tue, 13 Jul 2021 at 08:53, Marek Vasut <marex@denx.de> wrote:
>>
>> On 7/13/21 4:41 PM, Tom Rini wrote:
>>> On Tue, Jul 13, 2021 at 04:35:38PM +0200, Marek Vasut wrote:
>>>> On 7/13/21 3:47 PM, Tom Rini wrote:
>>>>> On Mon, Jul 12, 2021 at 11:01:24AM -0500, Alex G. wrote:
>>>>>> On 7/12/21 10:15 AM, 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@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.
>>>>>>
>>>>>> It's true that the flat devicetree spec requires an 8-byte alignment, even
>>>>>> on 32-bit. The issues here are specific to u-boot.
>>>>>>
>>>>>> SPL and u-boot have to agree where u-boot's FDT is located. We'll look at
>>>>>> two cases:
>>>>>>     1) u-boot as a FIT (binary and FDT separately loaded)
>>>>>>     2) u-boot with embedded FDT
>>>>>>
>>>>>> In case (1) SPL must place the FDT at a location where u-boot will find it.
>>>>>> The current logic is
>>>>>>     SPL:    fdt = ALIGN_4(u_boot + u_boot_size)
>>>>>>     u-boot: fdt = ALIGN_4(u_boot + u_boot_size)
>>>>>>
>>>>>> In case (2), SPL's view of the FDT is not relevant, but instead the build
>>>>>> system must place the FDT correctly:
>>>>>>     build:  fdt >> u-boot.bin
>>>>>>     u-boot: fdt = ALIGN_4(u_boot + u_boot_size)
>>>>>>
>>>>>> We have 3 places that must agree. A correct and complete patch could change
>>>>>> all three, but one has to consider compatibility issues when crossing u-boot
>>>>>> and SPL versions.
>>>>>>
>>>>>> I had proposed in the revert discussion that SPL use r2 or similar mechanism
>>>>>> to pass the location of the FDT to u-boot.
>>>>>
>>>>> I'm not sure that we need to worry too much about mix-and-match
>>>>> SPL/U-Boot, but documenting what to go change if you must do it
>>>>> somewhere under doc/ would be good.  I think we can just switch to
>>>>> ALIGN(8) not ALIGN(4) and be done with it?
>>>>
>>>> Remember, there is also falcon boot. And we definitely have to be able to
>>>> have old u-boot (SPL) boot new fitImage and vice versa.
>>>
>>> I don't follow you, sorry.  But since you seem to have the best
>>> understanding of where all of the cases something could go wrong here,
>>> can you perhaps post an RFC patch?  That is likely to be clearer than
>>> another long thread here.
>>
>> I don't follow you, sorry. I believe the revert did the right thing and
>> new systems should use mkimage -E when generating fitImages, to avoid
>> the string alignment problem. That is all.
> 
> Using -E should be optional and things really should work without it.

See the DTSpec, I don't think that is possible unless you relocate 
fitImage components, and if you want fast boot time esp. in SPL, that is 
not good.

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 5193 bytes --]

On Tue, Jul 13, 2021 at 07:50:49PM +0200, Marek Vasut wrote:
> On 7/13/21 6:47 PM, Simon Glass wrote:
> > Hi Marek,
> > 
> > On Tue, 13 Jul 2021 at 08:53, Marek Vasut <marex@denx.de> wrote:
> > > 
> > > On 7/13/21 4:41 PM, Tom Rini wrote:
> > > > On Tue, Jul 13, 2021 at 04:35:38PM +0200, Marek Vasut wrote:
> > > > > On 7/13/21 3:47 PM, Tom Rini wrote:
> > > > > > On Mon, Jul 12, 2021 at 11:01:24AM -0500, Alex G. wrote:
> > > > > > > On 7/12/21 10:15 AM, 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@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.
> > > > > > > 
> > > > > > > It's true that the flat devicetree spec requires an 8-byte alignment, even
> > > > > > > on 32-bit. The issues here are specific to u-boot.
> > > > > > > 
> > > > > > > SPL and u-boot have to agree where u-boot's FDT is located. We'll look at
> > > > > > > two cases:
> > > > > > >     1) u-boot as a FIT (binary and FDT separately loaded)
> > > > > > >     2) u-boot with embedded FDT
> > > > > > > 
> > > > > > > In case (1) SPL must place the FDT at a location where u-boot will find it.
> > > > > > > The current logic is
> > > > > > >     SPL:    fdt = ALIGN_4(u_boot + u_boot_size)
> > > > > > >     u-boot: fdt = ALIGN_4(u_boot + u_boot_size)
> > > > > > > 
> > > > > > > In case (2), SPL's view of the FDT is not relevant, but instead the build
> > > > > > > system must place the FDT correctly:
> > > > > > >     build:  fdt >> u-boot.bin
> > > > > > >     u-boot: fdt = ALIGN_4(u_boot + u_boot_size)
> > > > > > > 
> > > > > > > We have 3 places that must agree. A correct and complete patch could change
> > > > > > > all three, but one has to consider compatibility issues when crossing u-boot
> > > > > > > and SPL versions.
> > > > > > > 
> > > > > > > I had proposed in the revert discussion that SPL use r2 or similar mechanism
> > > > > > > to pass the location of the FDT to u-boot.
> > > > > > 
> > > > > > I'm not sure that we need to worry too much about mix-and-match
> > > > > > SPL/U-Boot, but documenting what to go change if you must do it
> > > > > > somewhere under doc/ would be good.  I think we can just switch to
> > > > > > ALIGN(8) not ALIGN(4) and be done with it?
> > > > > 
> > > > > Remember, there is also falcon boot. And we definitely have to be able to
> > > > > have old u-boot (SPL) boot new fitImage and vice versa.
> > > > 
> > > > I don't follow you, sorry.  But since you seem to have the best
> > > > understanding of where all of the cases something could go wrong here,
> > > > can you perhaps post an RFC patch?  That is likely to be clearer than
> > > > another long thread here.
> > > 
> > > I don't follow you, sorry. I believe the revert did the right thing and
> > > new systems should use mkimage -E when generating fitImages, to avoid
> > > the string alignment problem. That is all.
> > 
> > Using -E should be optional and things really should work without it.
> 
> See the DTSpec, I don't think that is possible unless you relocate fitImage
> components, and if you want fast boot time esp. in SPL, that is not good.

This is why I've asked you to make up some patch to perhaps highlight
the problem.  Ensuring that the device tree, which is small, is also
8-byte aligned, shouldn't be a big problem nor performance hit.  I'm not
sure where the problem case is that isn't "user put things they control
in a bad spot, fail and tell them why" but I could just be missing a
case.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Marek Vasut]

On 7/13/21 8:11 PM, Tom Rini wrote:
> On Tue, Jul 13, 2021 at 07:50:49PM +0200, Marek Vasut wrote:
>> On 7/13/21 6:47 PM, Simon Glass wrote:
>>> Hi Marek,
>>>
>>> On Tue, 13 Jul 2021 at 08:53, Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> On 7/13/21 4:41 PM, Tom Rini wrote:
>>>>> On Tue, Jul 13, 2021 at 04:35:38PM +0200, Marek Vasut wrote:
>>>>>> On 7/13/21 3:47 PM, Tom Rini wrote:
>>>>>>> On Mon, Jul 12, 2021 at 11:01:24AM -0500, Alex G. wrote:
>>>>>>>> On 7/12/21 10:15 AM, 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@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.
>>>>>>>>
>>>>>>>> It's true that the flat devicetree spec requires an 8-byte alignment, even
>>>>>>>> on 32-bit. The issues here are specific to u-boot.
>>>>>>>>
>>>>>>>> SPL and u-boot have to agree where u-boot's FDT is located. We'll look at
>>>>>>>> two cases:
>>>>>>>>      1) u-boot as a FIT (binary and FDT separately loaded)
>>>>>>>>      2) u-boot with embedded FDT
>>>>>>>>
>>>>>>>> In case (1) SPL must place the FDT at a location where u-boot will find it.
>>>>>>>> The current logic is
>>>>>>>>      SPL:    fdt = ALIGN_4(u_boot + u_boot_size)
>>>>>>>>      u-boot: fdt = ALIGN_4(u_boot + u_boot_size)
>>>>>>>>
>>>>>>>> In case (2), SPL's view of the FDT is not relevant, but instead the build
>>>>>>>> system must place the FDT correctly:
>>>>>>>>      build:  fdt >> u-boot.bin
>>>>>>>>      u-boot: fdt = ALIGN_4(u_boot + u_boot_size)
>>>>>>>>
>>>>>>>> We have 3 places that must agree. A correct and complete patch could change
>>>>>>>> all three, but one has to consider compatibility issues when crossing u-boot
>>>>>>>> and SPL versions.
>>>>>>>>
>>>>>>>> I had proposed in the revert discussion that SPL use r2 or similar mechanism
>>>>>>>> to pass the location of the FDT to u-boot.
>>>>>>>
>>>>>>> I'm not sure that we need to worry too much about mix-and-match
>>>>>>> SPL/U-Boot, but documenting what to go change if you must do it
>>>>>>> somewhere under doc/ would be good.  I think we can just switch to
>>>>>>> ALIGN(8) not ALIGN(4) and be done with it?
>>>>>>
>>>>>> Remember, there is also falcon boot. And we definitely have to be able to
>>>>>> have old u-boot (SPL) boot new fitImage and vice versa.
>>>>>
>>>>> I don't follow you, sorry.  But since you seem to have the best
>>>>> understanding of where all of the cases something could go wrong here,
>>>>> can you perhaps post an RFC patch?  That is likely to be clearer than
>>>>> another long thread here.
>>>>
>>>> I don't follow you, sorry. I believe the revert did the right thing and
>>>> new systems should use mkimage -E when generating fitImages, to avoid
>>>> the string alignment problem. That is all.
>>>
>>> Using -E should be optional and things really should work without it.
>>
>> See the DTSpec, I don't think that is possible unless you relocate fitImage
>> components, and if you want fast boot time esp. in SPL, that is not good.
> 
> This is why I've asked you to make up some patch to perhaps highlight
> the problem.  Ensuring that the device tree, which is small, is also
> 8-byte aligned, shouldn't be a big problem nor performance hit.  I'm not
> sure where the problem case is that isn't "user put things they control
> in a bad spot, fail and tell them why" but I could just be missing a
> case.

The fail case is this:
- you update SPL with this 8 byte alignment change
- you have older kernel fitImage with embedded DT for falcon mode
- system no longer boots because there is off-by-4 error in the DT
   address passed to the kernel

I hope this is clear now.

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

[Alex G]

On 7/13/21 3:35 PM, Marek Vasut wrote:
> On 7/13/21 8:11 PM, Tom Rini wrote:
>> On Tue, Jul 13, 2021 at 07:50:49PM +0200, Marek Vasut wrote:
>>> On 7/13/21 6:47 PM, Simon Glass wrote:
>>>> Hi Marek,
>>>>
>>>> On Tue, 13 Jul 2021 at 08:53, Marek Vasut <marex@denx.de> wrote:
>>>>>
>>>>> On 7/13/21 4:41 PM, Tom Rini wrote:
>>>>>> On Tue, Jul 13, 2021 at 04:35:38PM +0200, Marek Vasut wrote:
>>>>>>> On 7/13/21 3:47 PM, Tom Rini wrote:
>>>>>>>> On Mon, Jul 12, 2021 at 11:01:24AM -0500, Alex G. wrote:
>>>>>>>>> On 7/12/21 10:15 AM, 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@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.
>>>>>>>>>
>>>>>>>>> It's true that the flat devicetree spec requires an 8-byte 
>>>>>>>>> alignment, even
>>>>>>>>> on 32-bit. The issues here are specific to u-boot.
>>>>>>>>>
>>>>>>>>> SPL and u-boot have to agree where u-boot's FDT is located. 
>>>>>>>>> We'll look at
>>>>>>>>> two cases:
>>>>>>>>>      1) u-boot as a FIT (binary and FDT separately loaded)
>>>>>>>>>      2) u-boot with embedded FDT
>>>>>>>>>
>>>>>>>>> In case (1) SPL must place the FDT at a location where u-boot 
>>>>>>>>> will find it.
>>>>>>>>> The current logic is
>>>>>>>>>      SPL:    fdt = ALIGN_4(u_boot + u_boot_size)
>>>>>>>>>      u-boot: fdt = ALIGN_4(u_boot + u_boot_size)
>>>>>>>>>
>>>>>>>>> In case (2), SPL's view of the FDT is not relevant, but instead 
>>>>>>>>> the build
>>>>>>>>> system must place the FDT correctly:
>>>>>>>>>      build:  fdt >> u-boot.bin
>>>>>>>>>      u-boot: fdt = ALIGN_4(u_boot + u_boot_size)
>>>>>>>>>
>>>>>>>>> We have 3 places that must agree. A correct and complete patch 
>>>>>>>>> could change
>>>>>>>>> all three, but one has to consider compatibility issues when 
>>>>>>>>> crossing u-boot
>>>>>>>>> and SPL versions.
>>>>>>>>>
>>>>>>>>> I had proposed in the revert discussion that SPL use r2 or 
>>>>>>>>> similar mechanism
>>>>>>>>> to pass the location of the FDT to u-boot.
>>>>>>>>
>>>>>>>> I'm not sure that we need to worry too much about mix-and-match
>>>>>>>> SPL/U-Boot, but documenting what to go change if you must do it
>>>>>>>> somewhere under doc/ would be good.  I think we can just switch to
>>>>>>>> ALIGN(8) not ALIGN(4) and be done with it?
>>>>>>>
>>>>>>> Remember, there is also falcon boot. And we definitely have to be 
>>>>>>> able to
>>>>>>> have old u-boot (SPL) boot new fitImage and vice versa.
>>>>>>
>>>>>> I don't follow you, sorry.  But since you seem to have the best
>>>>>> understanding of where all of the cases something could go wrong 
>>>>>> here,
>>>>>> can you perhaps post an RFC patch?  That is likely to be clearer than
>>>>>> another long thread here.
>>>>>
>>>>> I don't follow you, sorry. I believe the revert did the right thing 
>>>>> and
>>>>> new systems should use mkimage -E when generating fitImages, to avoid
>>>>> the string alignment problem. That is all.
>>>>
>>>> Using -E should be optional and things really should work without it.
>>>
>>> See the DTSpec, I don't think that is possible unless you relocate 
>>> fitImage
>>> components, and if you want fast boot time esp. in SPL, that is not 
>>> good.
>>
>> This is why I've asked you to make up some patch to perhaps highlight
>> the problem.  Ensuring that the device tree, which is small, is also
>> 8-byte aligned, shouldn't be a big problem nor performance hit.  I'm not
>> sure where the problem case is that isn't "user put things they control
>> in a bad spot, fail and tell them why" but I could just be missing a
>> case.
> 
> The fail case is this:
> - you update SPL with this 8 byte alignment change
> - you have older kernel fitImage with embedded DT for falcon mode
> - system no longer boots because there is off-by-4 error in the DT
>    address passed to the kernel

I'm not sure how falcon mode would break the kernel. It passes to the 
kernel the load address of the fdt. The concern here is loading u-boot.


> I hope this is clear now.

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 6536 bytes --]

On Tue, Jul 13, 2021 at 10:35:03PM +0200, Marek Vasut wrote:
> On 7/13/21 8:11 PM, Tom Rini wrote:
> > On Tue, Jul 13, 2021 at 07:50:49PM +0200, Marek Vasut wrote:
> > > On 7/13/21 6:47 PM, Simon Glass wrote:
> > > > Hi Marek,
> > > > 
> > > > On Tue, 13 Jul 2021 at 08:53, Marek Vasut <marex@denx.de> wrote:
> > > > > 
> > > > > On 7/13/21 4:41 PM, Tom Rini wrote:
> > > > > > On Tue, Jul 13, 2021 at 04:35:38PM +0200, Marek Vasut wrote:
> > > > > > > On 7/13/21 3:47 PM, Tom Rini wrote:
> > > > > > > > On Mon, Jul 12, 2021 at 11:01:24AM -0500, Alex G. wrote:
> > > > > > > > > On 7/12/21 10:15 AM, 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@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.
> > > > > > > > > 
> > > > > > > > > It's true that the flat devicetree spec requires an 8-byte alignment, even
> > > > > > > > > on 32-bit. The issues here are specific to u-boot.
> > > > > > > > > 
> > > > > > > > > SPL and u-boot have to agree where u-boot's FDT is located. We'll look at
> > > > > > > > > two cases:
> > > > > > > > >      1) u-boot as a FIT (binary and FDT separately loaded)
> > > > > > > > >      2) u-boot with embedded FDT
> > > > > > > > > 
> > > > > > > > > In case (1) SPL must place the FDT at a location where u-boot will find it.
> > > > > > > > > The current logic is
> > > > > > > > >      SPL:    fdt = ALIGN_4(u_boot + u_boot_size)
> > > > > > > > >      u-boot: fdt = ALIGN_4(u_boot + u_boot_size)
> > > > > > > > > 
> > > > > > > > > In case (2), SPL's view of the FDT is not relevant, but instead the build
> > > > > > > > > system must place the FDT correctly:
> > > > > > > > >      build:  fdt >> u-boot.bin
> > > > > > > > >      u-boot: fdt = ALIGN_4(u_boot + u_boot_size)
> > > > > > > > > 
> > > > > > > > > We have 3 places that must agree. A correct and complete patch could change
> > > > > > > > > all three, but one has to consider compatibility issues when crossing u-boot
> > > > > > > > > and SPL versions.
> > > > > > > > > 
> > > > > > > > > I had proposed in the revert discussion that SPL use r2 or similar mechanism
> > > > > > > > > to pass the location of the FDT to u-boot.
> > > > > > > > 
> > > > > > > > I'm not sure that we need to worry too much about mix-and-match
> > > > > > > > SPL/U-Boot, but documenting what to go change if you must do it
> > > > > > > > somewhere under doc/ would be good.  I think we can just switch to
> > > > > > > > ALIGN(8) not ALIGN(4) and be done with it?
> > > > > > > 
> > > > > > > Remember, there is also falcon boot. And we definitely have to be able to
> > > > > > > have old u-boot (SPL) boot new fitImage and vice versa.
> > > > > > 
> > > > > > I don't follow you, sorry.  But since you seem to have the best
> > > > > > understanding of where all of the cases something could go wrong here,
> > > > > > can you perhaps post an RFC patch?  That is likely to be clearer than
> > > > > > another long thread here.
> > > > > 
> > > > > I don't follow you, sorry. I believe the revert did the right thing and
> > > > > new systems should use mkimage -E when generating fitImages, to avoid
> > > > > the string alignment problem. That is all.
> > > > 
> > > > Using -E should be optional and things really should work without it.
> > > 
> > > See the DTSpec, I don't think that is possible unless you relocate fitImage
> > > components, and if you want fast boot time esp. in SPL, that is not good.
> > 
> > This is why I've asked you to make up some patch to perhaps highlight
> > the problem.  Ensuring that the device tree, which is small, is also
> > 8-byte aligned, shouldn't be a big problem nor performance hit.  I'm not
> > sure where the problem case is that isn't "user put things they control
> > in a bad spot, fail and tell them why" but I could just be missing a
> > case.
> 
> The fail case is this:
> - you update SPL with this 8 byte alignment change
> - you have older kernel fitImage with embedded DT for falcon mode
> - system no longer boots because there is off-by-4 error in the DT
>   address passed to the kernel

OK.  Then I think the answer is what I said recently in another part of
this thread, we need to split "find the fdt" from "align the fdt".  The
fdt can come to us with any alignment it happens to have, but we can't
use that fdt in-place unless it's correctly aligned.  In the case of
falcon mode, it needs to end up at CONFIG_SYS_SPL_ARGS_ADDR.  The case
of passing it on to U-Boot proper is where we have at best a hack right
now (as noted by fdt_hack in common/spl/spl.c).  That would be a place
to, as has been also suggested in this thread, pass along more correctly
where the device tree in memory is.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Bin Meng]

On Wed, Jul 14, 2021 at 5:11 AM Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Jul 13, 2021 at 10:35:03PM +0200, Marek Vasut wrote:
> > On 7/13/21 8:11 PM, Tom Rini wrote:
> > > On Tue, Jul 13, 2021 at 07:50:49PM +0200, Marek Vasut wrote:
> > > > On 7/13/21 6:47 PM, Simon Glass wrote:
> > > > > Hi Marek,
> > > > >
> > > > > On Tue, 13 Jul 2021 at 08:53, Marek Vasut <marex@denx.de> wrote:
> > > > > >
> > > > > > On 7/13/21 4:41 PM, Tom Rini wrote:
> > > > > > > On Tue, Jul 13, 2021 at 04:35:38PM +0200, Marek Vasut wrote:
> > > > > > > > On 7/13/21 3:47 PM, Tom Rini wrote:
> > > > > > > > > On Mon, Jul 12, 2021 at 11:01:24AM -0500, Alex G. wrote:
> > > > > > > > > > On 7/12/21 10:15 AM, 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@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.
> > > > > > > > > >
> > > > > > > > > > It's true that the flat devicetree spec requires an 8-byte alignment, even
> > > > > > > > > > on 32-bit. The issues here are specific to u-boot.
> > > > > > > > > >
> > > > > > > > > > SPL and u-boot have to agree where u-boot's FDT is located. We'll look at
> > > > > > > > > > two cases:
> > > > > > > > > >      1) u-boot as a FIT (binary and FDT separately loaded)
> > > > > > > > > >      2) u-boot with embedded FDT
> > > > > > > > > >
> > > > > > > > > > In case (1) SPL must place the FDT at a location where u-boot will find it.
> > > > > > > > > > The current logic is
> > > > > > > > > >      SPL:    fdt = ALIGN_4(u_boot + u_boot_size)
> > > > > > > > > >      u-boot: fdt = ALIGN_4(u_boot + u_boot_size)
> > > > > > > > > >
> > > > > > > > > > In case (2), SPL's view of the FDT is not relevant, but instead the build
> > > > > > > > > > system must place the FDT correctly:
> > > > > > > > > >      build:  fdt >> u-boot.bin
> > > > > > > > > >      u-boot: fdt = ALIGN_4(u_boot + u_boot_size)
> > > > > > > > > >
> > > > > > > > > > We have 3 places that must agree. A correct and complete patch could change
> > > > > > > > > > all three, but one has to consider compatibility issues when crossing u-boot
> > > > > > > > > > and SPL versions.
> > > > > > > > > >
> > > > > > > > > > I had proposed in the revert discussion that SPL use r2 or similar mechanism
> > > > > > > > > > to pass the location of the FDT to u-boot.
> > > > > > > > >
> > > > > > > > > I'm not sure that we need to worry too much about mix-and-match
> > > > > > > > > SPL/U-Boot, but documenting what to go change if you must do it
> > > > > > > > > somewhere under doc/ would be good.  I think we can just switch to
> > > > > > > > > ALIGN(8) not ALIGN(4) and be done with it?
> > > > > > > >
> > > > > > > > Remember, there is also falcon boot. And we definitely have to be able to
> > > > > > > > have old u-boot (SPL) boot new fitImage and vice versa.
> > > > > > >
> > > > > > > I don't follow you, sorry.  But since you seem to have the best
> > > > > > > understanding of where all of the cases something could go wrong here,
> > > > > > > can you perhaps post an RFC patch?  That is likely to be clearer than
> > > > > > > another long thread here.
> > > > > >
> > > > > > I don't follow you, sorry. I believe the revert did the right thing and
> > > > > > new systems should use mkimage -E when generating fitImages, to avoid
> > > > > > the string alignment problem. That is all.
> > > > >
> > > > > Using -E should be optional and things really should work without it.
> > > >
> > > > See the DTSpec, I don't think that is possible unless you relocate fitImage
> > > > components, and if you want fast boot time esp. in SPL, that is not good.
> > >
> > > This is why I've asked you to make up some patch to perhaps highlight
> > > the problem.  Ensuring that the device tree, which is small, is also
> > > 8-byte aligned, shouldn't be a big problem nor performance hit.  I'm not
> > > sure where the problem case is that isn't "user put things they control
> > > in a bad spot, fail and tell them why" but I could just be missing a
> > > case.
> >
> > The fail case is this:
> > - you update SPL with this 8 byte alignment change
> > - you have older kernel fitImage with embedded DT for falcon mode
> > - system no longer boots because there is off-by-4 error in the DT
> >   address passed to the kernel
>
> OK.  Then I think the answer is what I said recently in another part of
> this thread, we need to split "find the fdt" from "align the fdt".  The
> fdt can come to us with any alignment it happens to have, but we can't
> use that fdt in-place unless it's correctly aligned.  In the case of
> falcon mode, it needs to end up at CONFIG_SYS_SPL_ARGS_ADDR.  The case
> of passing it on to U-Boot proper is where we have at best a hack right
> now (as noted by fdt_hack in common/spl/spl.c).  That would be a place
> to, as has been also suggested in this thread, pass along more correctly
> where the device tree in memory is.

Where are we on this issue?

Regards,
Bin

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 7208 bytes --]

On Mon, Jul 26, 2021 at 09:26:26PM +0800, Bin Meng wrote:
> On Wed, Jul 14, 2021 at 5:11 AM Tom Rini <trini@konsulko.com> wrote:
> >
> > On Tue, Jul 13, 2021 at 10:35:03PM +0200, Marek Vasut wrote:
> > > On 7/13/21 8:11 PM, Tom Rini wrote:
> > > > On Tue, Jul 13, 2021 at 07:50:49PM +0200, Marek Vasut wrote:
> > > > > On 7/13/21 6:47 PM, Simon Glass wrote:
> > > > > > Hi Marek,
> > > > > >
> > > > > > On Tue, 13 Jul 2021 at 08:53, Marek Vasut <marex@denx.de> wrote:
> > > > > > >
> > > > > > > On 7/13/21 4:41 PM, Tom Rini wrote:
> > > > > > > > On Tue, Jul 13, 2021 at 04:35:38PM +0200, Marek Vasut wrote:
> > > > > > > > > On 7/13/21 3:47 PM, Tom Rini wrote:
> > > > > > > > > > On Mon, Jul 12, 2021 at 11:01:24AM -0500, Alex G. wrote:
> > > > > > > > > > > On 7/12/21 10:15 AM, 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@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.
> > > > > > > > > > >
> > > > > > > > > > > It's true that the flat devicetree spec requires an 8-byte alignment, even
> > > > > > > > > > > on 32-bit. The issues here are specific to u-boot.
> > > > > > > > > > >
> > > > > > > > > > > SPL and u-boot have to agree where u-boot's FDT is located. We'll look at
> > > > > > > > > > > two cases:
> > > > > > > > > > >      1) u-boot as a FIT (binary and FDT separately loaded)
> > > > > > > > > > >      2) u-boot with embedded FDT
> > > > > > > > > > >
> > > > > > > > > > > In case (1) SPL must place the FDT at a location where u-boot will find it.
> > > > > > > > > > > The current logic is
> > > > > > > > > > >      SPL:    fdt = ALIGN_4(u_boot + u_boot_size)
> > > > > > > > > > >      u-boot: fdt = ALIGN_4(u_boot + u_boot_size)
> > > > > > > > > > >
> > > > > > > > > > > In case (2), SPL's view of the FDT is not relevant, but instead the build
> > > > > > > > > > > system must place the FDT correctly:
> > > > > > > > > > >      build:  fdt >> u-boot.bin
> > > > > > > > > > >      u-boot: fdt = ALIGN_4(u_boot + u_boot_size)
> > > > > > > > > > >
> > > > > > > > > > > We have 3 places that must agree. A correct and complete patch could change
> > > > > > > > > > > all three, but one has to consider compatibility issues when crossing u-boot
> > > > > > > > > > > and SPL versions.
> > > > > > > > > > >
> > > > > > > > > > > I had proposed in the revert discussion that SPL use r2 or similar mechanism
> > > > > > > > > > > to pass the location of the FDT to u-boot.
> > > > > > > > > >
> > > > > > > > > > I'm not sure that we need to worry too much about mix-and-match
> > > > > > > > > > SPL/U-Boot, but documenting what to go change if you must do it
> > > > > > > > > > somewhere under doc/ would be good.  I think we can just switch to
> > > > > > > > > > ALIGN(8) not ALIGN(4) and be done with it?
> > > > > > > > >
> > > > > > > > > Remember, there is also falcon boot. And we definitely have to be able to
> > > > > > > > > have old u-boot (SPL) boot new fitImage and vice versa.
> > > > > > > >
> > > > > > > > I don't follow you, sorry.  But since you seem to have the best
> > > > > > > > understanding of where all of the cases something could go wrong here,
> > > > > > > > can you perhaps post an RFC patch?  That is likely to be clearer than
> > > > > > > > another long thread here.
> > > > > > >
> > > > > > > I don't follow you, sorry. I believe the revert did the right thing and
> > > > > > > new systems should use mkimage -E when generating fitImages, to avoid
> > > > > > > the string alignment problem. That is all.
> > > > > >
> > > > > > Using -E should be optional and things really should work without it.
> > > > >
> > > > > See the DTSpec, I don't think that is possible unless you relocate fitImage
> > > > > components, and if you want fast boot time esp. in SPL, that is not good.
> > > >
> > > > This is why I've asked you to make up some patch to perhaps highlight
> > > > the problem.  Ensuring that the device tree, which is small, is also
> > > > 8-byte aligned, shouldn't be a big problem nor performance hit.  I'm not
> > > > sure where the problem case is that isn't "user put things they control
> > > > in a bad spot, fail and tell them why" but I could just be missing a
> > > > case.
> > >
> > > The fail case is this:
> > > - you update SPL with this 8 byte alignment change
> > > - you have older kernel fitImage with embedded DT for falcon mode
> > > - system no longer boots because there is off-by-4 error in the DT
> > >   address passed to the kernel
> >
> > OK.  Then I think the answer is what I said recently in another part of
> > this thread, we need to split "find the fdt" from "align the fdt".  The
> > fdt can come to us with any alignment it happens to have, but we can't
> > use that fdt in-place unless it's correctly aligned.  In the case of
> > falcon mode, it needs to end up at CONFIG_SYS_SPL_ARGS_ADDR.  The case
> > of passing it on to U-Boot proper is where we have at best a hack right
> > now (as noted by fdt_hack in common/spl/spl.c).  That would be a place
> > to, as has been also suggested in this thread, pass along more correctly
> > where the device tree in memory is.
> 
> Where are we on this issue?

Waiting for someone to do what I suggested I think.  I've not yet had
time to take a look.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Alex G]

On 7/13/21 1:11 PM, Tom Rini wrote:
> On Tue, Jul 13, 2021 at 07:50:49PM +0200, Marek Vasut wrote:
>> On 7/13/21 6:47 PM, Simon Glass wrote:
>>> Hi Marek,
>>>
>>> On Tue, 13 Jul 2021 at 08:53, Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> On 7/13/21 4:41 PM, Tom Rini wrote:
>>>>> On Tue, Jul 13, 2021 at 04:35:38PM +0200, Marek Vasut wrote:
>>>>>> On 7/13/21 3:47 PM, Tom Rini wrote:
>>>>>>> On Mon, Jul 12, 2021 at 11:01:24AM -0500, Alex G. wrote:
>>>>>>>> On 7/12/21 10:15 AM, 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@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.
>>>>>>>>
>>>>>>>> It's true that the flat devicetree spec requires an 8-byte alignment, even
>>>>>>>> on 32-bit. The issues here are specific to u-boot.
>>>>>>>>
>>>>>>>> SPL and u-boot have to agree where u-boot's FDT is located. We'll look at
>>>>>>>> two cases:
>>>>>>>>      1) u-boot as a FIT (binary and FDT separately loaded)
>>>>>>>>      2) u-boot with embedded FDT
>>>>>>>>
>>>>>>>> In case (1) SPL must place the FDT at a location where u-boot will find it.
>>>>>>>> The current logic is
>>>>>>>>      SPL:    fdt = ALIGN_4(u_boot + u_boot_size)
>>>>>>>>      u-boot: fdt = ALIGN_4(u_boot + u_boot_size)
>>>>>>>>
>>>>>>>> In case (2), SPL's view of the FDT is not relevant, but instead the build
>>>>>>>> system must place the FDT correctly:
>>>>>>>>      build:  fdt >> u-boot.bin
>>>>>>>>      u-boot: fdt = ALIGN_4(u_boot + u_boot_size)
>>>>>>>>
>>>>>>>> We have 3 places that must agree. A correct and complete patch could change
>>>>>>>> all three, but one has to consider compatibility issues when crossing u-boot
>>>>>>>> and SPL versions.
>>>>>>>>
>>>>>>>> I had proposed in the revert discussion that SPL use r2 or similar mechanism
>>>>>>>> to pass the location of the FDT to u-boot.
>>>>>>>
>>>>>>> I'm not sure that we need to worry too much about mix-and-match
>>>>>>> SPL/U-Boot, but documenting what to go change if you must do it
>>>>>>> somewhere under doc/ would be good.  I think we can just switch to
>>>>>>> ALIGN(8) not ALIGN(4) and be done with it?
>>>>>>
>>>>>> Remember, there is also falcon boot. And we definitely have to be able to
>>>>>> have old u-boot (SPL) boot new fitImage and vice versa.
>>>>>
>>>>> I don't follow you, sorry.  But since you seem to have the best
>>>>> understanding of where all of the cases something could go wrong here,
>>>>> can you perhaps post an RFC patch?  That is likely to be clearer than
>>>>> another long thread here.
>>>>
>>>> I don't follow you, sorry. I believe the revert did the right thing and
>>>> new systems should use mkimage -E when generating fitImages, to avoid
>>>> the string alignment problem. That is all.
>>>
>>> Using -E should be optional and things really should work without it.
>>
>> See the DTSpec, I don't think that is possible unless you relocate fitImage
>> components, and if you want fast boot time esp. in SPL, that is not good.
> 
> This is why I've asked you to make up some patch to perhaps highlight
> the problem.  Ensuring that the device tree, which is small, is also
> 8-byte aligned, shouldn't be a big problem nor performance hit.  I'm not
> sure where the problem case is that isn't "user put things they control
> in a bad spot, fail and tell them why" but I could just be missing a
> case.
> 

As far as highlighting the problem, here's an excerpt from the previous 
discussion [1].


## SPL:

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

FDT is installed at 0xc01cf860


## u-boot:

	__weak void *board_fdt_blob_setup(void)
	{
		/* FDT is at end of image */

		fdt_blob = (ulong *)&_end;

	(gdb) print &_end
	$22 = (char (*)[]) 0xc01cf85c

FDT is expected at 0xc01cf85c


Alex

[1] https://lists.denx.de/pipermail/u-boot/2020-October/430066.html

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2989 bytes --]

On Mon, Jul 12, 2021 at 11:01:24AM -0500, Alex G. wrote:
> On 7/12/21 10:15 AM, 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@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.
> 
> It's true that the flat devicetree spec requires an 8-byte alignment, even
> on 32-bit. The issues here are specific to u-boot.
> 
> SPL and u-boot have to agree where u-boot's FDT is located. We'll look at
> two cases:
> 	1) u-boot as a FIT (binary and FDT separately loaded)
> 	2) u-boot with embedded FDT
> 
> In case (1) SPL must place the FDT at a location where u-boot will find it.
> The current logic is
> 	SPL:	fdt = ALIGN_4(u_boot + u_boot_size)
> 	u-boot:	fdt = ALIGN_4(u_boot + u_boot_size)
> 
> In case (2), SPL's view of the FDT is not relevant, but instead the build
> system must place the FDT correctly:
> 	build:	fdt >> u-boot.bin
> 	u-boot:	fdt = ALIGN_4(u_boot + u_boot_size)
> 
> We have 3 places that must agree. A correct and complete patch could change
> all three, but one has to consider compatibility issues when crossing u-boot
> and SPL versions.
> 
> I had proposed in the revert discussion that SPL use r2 or similar mechanism
> to pass the location of the FDT to u-boot.

Looking at this again, and trying to figure out what we can do here most
easily, I think we need to have spl_fit_append_fdt() perhaps be split in
to "find the fdt" and then either a new function, or more logic within
the function for "ensure fdt is aligned properly".  We had made the
assumption that we can use the fdt in place in memory but that's not
always true.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Bin Meng]

On Mon, Jul 12, 2021 at 11:15 PM Tom Rini <trini@konsulko.com> 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@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.
>
> > 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.  It's just that on 32bit
> platforms 4-but-not-8 byte alignment tends to not be fatal but on 64bit
> platforms it is.

But with libfdt v1.6.1 commit 5e735860c478 ("libfdt: Check for 8-byte
address alignment in fdt_ro_probe_()"), now this is fatal error if DT
address is not 8-bytes aligned, as that commit adds a check in
fdt_ro_probe_().

Regards,
Bin