[PATCH V2] mkimage: fit: Do not tail-pad fitImage with external data
There is no reason to tail-pad fitImage with external data to 4-bytes, while fitImage without external data does not have any such padding and is often unaligned. DT spec also does not mandate any such padding.
Moreover, the tail-pad fills the last few bytes with uninitialized data, which could lead to a potential information leak.
$ echo -n xy > /tmp/data ; \ ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \ hexdump -vC /tmp/fitImage | tail -n 3
before: 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| 00000270 7a 65 00 00 78 79 64 64 |ze..xydd| ^^ ^^ ^^ after: 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| 00000270 7a 65 00 78 79 |ze.xy|
Signed-off-by: Marek Vasut marex@denx.de Cc: Heinrich Schuchardt xypron.glpk@gmx.de Cc: Simon Glass sjg@chromium.org Cc: Tom Rini trini@konsulko.com --- V2: Rebase on u-boot/master, update commit message --- tools/fit_image.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/tools/fit_image.c b/tools/fit_image.c index 4aeabbcfe9..b859e7c0c0 100644 --- a/tools/fit_image.c +++ b/tools/fit_image.c @@ -434,7 +434,7 @@ static int fit_extract_data(struct image_tool_params *params, const char *fname) int image_number; int align_size;
- align_size = params->bl_len ? params->bl_len : 4; + align_size = params->bl_len ? params->bl_len : 1; fd = mmap_fdt(params->cmdname, fname, 0, &fdt, &sbuf, false, false); if (fd < 0) return -EIO; @@ -492,7 +492,6 @@ static int fit_extract_data(struct image_tool_params *params, const char *fname) fdt_pack(fdt);
new_size = fdt_totalsize(fdt); - new_size = ALIGN(new_size, align_size); fdt_set_totalsize(fdt, new_size); debug("Size reduced from %x to %x\n", fit_size, fdt_totalsize(fdt)); debug("External data size %x\n", buf_ptr);
On Fri, 1 May 2020 at 09:40, Marek Vasut marex@denx.de wrote:
There is no reason to tail-pad fitImage with external data to 4-bytes, while fitImage without external data does not have any such padding and is often unaligned. DT spec also does not mandate any such padding.
Moreover, the tail-pad fills the last few bytes with uninitialized data, which could lead to a potential information leak.
$ echo -n xy > /tmp/data ; \ ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \ hexdump -vC /tmp/fitImage | tail -n 3
before: 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| 00000270 7a 65 00 00 78 79 64 64 |ze..xydd| ^^ ^^ ^^ after: 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| 00000270 7a 65 00 78 79 |ze.xy|
Signed-off-by: Marek Vasut marex@denx.de Cc: Heinrich Schuchardt xypron.glpk@gmx.de Cc: Simon Glass sjg@chromium.org Cc: Tom Rini trini@konsulko.com
V2: Rebase on u-boot/master, update commit message
tools/fit_image.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote:
There is no reason to tail-pad fitImage with external data to 4-bytes, while fitImage without external data does not have any such padding and is often unaligned. DT spec also does not mandate any such padding.
Moreover, the tail-pad fills the last few bytes with uninitialized data, which could lead to a potential information leak.
$ echo -n xy > /tmp/data ; \ ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \ hexdump -vC /tmp/fitImage | tail -n 3
before: 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| 00000270 7a 65 00 00 78 79 64 64 |ze..xydd| ^^ ^^ ^^ after: 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| 00000270 7a 65 00 78 79 |ze.xy|
Signed-off-by: Marek Vasut marex@denx.de Reviewed-by: Simon Glass sjg@chromium.org Cc: Heinrich Schuchardt xypron.glpk@gmx.de Cc: Tom Rini trini@konsulko.com
Applied to u-boot/master, thanks!
On Mon, May 4, 2020 at 12:28 PM Tom Rini trini@konsulko.com wrote:
On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote:
There is no reason to tail-pad fitImage with external data to 4-bytes, while fitImage without external data does not have any such padding and is often unaligned. DT spec also does not mandate any such padding.
Moreover, the tail-pad fills the last few bytes with uninitialized data, which could lead to a potential information leak.
$ echo -n xy > /tmp/data ; \ ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \ hexdump -vC /tmp/fitImage | tail -n 3
before: 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| 00000270 7a 65 00 00 78 79 64 64 |ze..xydd| ^^ ^^ ^^ after: 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| 00000270 7a 65 00 78 79 |ze.xy|
Signed-off-by: Marek Vasut marex@denx.de Reviewed-by: Simon Glass sjg@chromium.org Cc: Heinrich Schuchardt xypron.glpk@gmx.de Cc: Tom Rini trini@konsulko.com
Applied to u-boot/master, thanks!
This breaks booting on my board (am3352, eMMC boot, FIT u-boot, CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it from eMMC I get nothing at all on the console, if I boot over ymodem it stalls at 420k, before continuing to 460k. My guess is there's some error going to the console at the 420k mark, but obviously it's lost in the ymodem... I have two DTBs in the FIT image, 420k would about align to the point between them.
On 5/5/20 3:22 PM, Alex Kiernan wrote:
On Mon, May 4, 2020 at 12:28 PM Tom Rini trini@konsulko.com wrote:
On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote:
There is no reason to tail-pad fitImage with external data to 4-bytes, while fitImage without external data does not have any such padding and is often unaligned. DT spec also does not mandate any such padding.
Moreover, the tail-pad fills the last few bytes with uninitialized data, which could lead to a potential information leak.
$ echo -n xy > /tmp/data ; \ ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \ hexdump -vC /tmp/fitImage | tail -n 3
before: 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| 00000270 7a 65 00 00 78 79 64 64 |ze..xydd| ^^ ^^ ^^ after: 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| 00000270 7a 65 00 78 79 |ze.xy|
Signed-off-by: Marek Vasut marex@denx.de Reviewed-by: Simon Glass sjg@chromium.org Cc: Heinrich Schuchardt xypron.glpk@gmx.de Cc: Tom Rini trini@konsulko.com
Applied to u-boot/master, thanks!
This breaks booting on my board (am3352, eMMC boot, FIT u-boot, CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it from eMMC I get nothing at all on the console, if I boot over ymodem it stalls at 420k, before continuing to 460k. My guess is there's some error going to the console at the 420k mark, but obviously it's lost in the ymodem... I have two DTBs in the FIT image, 420k would about align to the point between them.
My bet would be on some padding / unaligned access problem that this patch uncovered. Can you take a look ?
On Tue, May 5, 2020 at 2:28 PM Marek Vasut marex@denx.de wrote:
On 5/5/20 3:22 PM, Alex Kiernan wrote:
On Mon, May 4, 2020 at 12:28 PM Tom Rini trini@konsulko.com wrote:
On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote:
There is no reason to tail-pad fitImage with external data to 4-bytes, while fitImage without external data does not have any such padding and is often unaligned. DT spec also does not mandate any such padding.
Moreover, the tail-pad fills the last few bytes with uninitialized data, which could lead to a potential information leak.
$ echo -n xy > /tmp/data ; \ ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \ hexdump -vC /tmp/fitImage | tail -n 3
before: 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| 00000270 7a 65 00 00 78 79 64 64 |ze..xydd| ^^ ^^ ^^ after: 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| 00000270 7a 65 00 78 79 |ze.xy|
Signed-off-by: Marek Vasut marex@denx.de Reviewed-by: Simon Glass sjg@chromium.org Cc: Heinrich Schuchardt xypron.glpk@gmx.de Cc: Tom Rini trini@konsulko.com
Applied to u-boot/master, thanks!
This breaks booting on my board (am3352, eMMC boot, FIT u-boot, CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it from eMMC I get nothing at all on the console, if I boot over ymodem it stalls at 420k, before continuing to 460k. My guess is there's some error going to the console at the 420k mark, but obviously it's lost in the ymodem... I have two DTBs in the FIT image, 420k would about align to the point between them.
My bet would be on some padding / unaligned access problem that this patch uncovered. Can you take a look ?
Seems plausible. With this change my external data starts at 0x483 and everything after it is non-aligned:
/dts-v1/; // magic: 0xd00dfeed // totalsize: 0x483 (1155) // off_dt_struct: 0x38 // off_dt_strings: 0x3f8 // off_mem_rsvmap: 0x28 // version: 17 // last_comp_version: 2 // boot_cpuid_phys: 0x0 // size_dt_strings: 0x8b // size_dt_struct: 0x3c0
On 5/5/20 6:37 PM, Alex Kiernan wrote:
On Tue, May 5, 2020 at 2:28 PM Marek Vasut marex@denx.de wrote:
On 5/5/20 3:22 PM, Alex Kiernan wrote:
On Mon, May 4, 2020 at 12:28 PM Tom Rini trini@konsulko.com wrote:
On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote:
There is no reason to tail-pad fitImage with external data to 4-bytes, while fitImage without external data does not have any such padding and is often unaligned. DT spec also does not mandate any such padding.
Moreover, the tail-pad fills the last few bytes with uninitialized data, which could lead to a potential information leak.
$ echo -n xy > /tmp/data ; \ ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \ hexdump -vC /tmp/fitImage | tail -n 3
before: 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| 00000270 7a 65 00 00 78 79 64 64 |ze..xydd| ^^ ^^ ^^ after: 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| 00000270 7a 65 00 78 79 |ze.xy|
Signed-off-by: Marek Vasut marex@denx.de Reviewed-by: Simon Glass sjg@chromium.org Cc: Heinrich Schuchardt xypron.glpk@gmx.de Cc: Tom Rini trini@konsulko.com
Applied to u-boot/master, thanks!
This breaks booting on my board (am3352, eMMC boot, FIT u-boot, CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it from eMMC I get nothing at all on the console, if I boot over ymodem it stalls at 420k, before continuing to 460k. My guess is there's some error going to the console at the 420k mark, but obviously it's lost in the ymodem... I have two DTBs in the FIT image, 420k would about align to the point between them.
My bet would be on some padding / unaligned access problem that this patch uncovered. Can you take a look ?
Seems plausible. With this change my external data starts at 0x483 and everything after it is non-aligned:
Should the beginning of external data be aligned ?
On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote:
On 5/5/20 6:37 PM, Alex Kiernan wrote:
On Tue, May 5, 2020 at 2:28 PM Marek Vasut marex@denx.de wrote:
On 5/5/20 3:22 PM, Alex Kiernan wrote:
On Mon, May 4, 2020 at 12:28 PM Tom Rini trini@konsulko.com wrote:
On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote:
There is no reason to tail-pad fitImage with external data to 4-bytes, while fitImage without external data does not have any such padding and is often unaligned. DT spec also does not mandate any such padding.
Moreover, the tail-pad fills the last few bytes with uninitialized data, which could lead to a potential information leak.
$ echo -n xy > /tmp/data ; \ ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \ hexdump -vC /tmp/fitImage | tail -n 3
before: 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| 00000270 7a 65 00 00 78 79 64 64 |ze..xydd| ^^ ^^ ^^ after: 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| 00000270 7a 65 00 78 79 |ze.xy|
Signed-off-by: Marek Vasut marex@denx.de Reviewed-by: Simon Glass sjg@chromium.org Cc: Heinrich Schuchardt xypron.glpk@gmx.de Cc: Tom Rini trini@konsulko.com
Applied to u-boot/master, thanks!
This breaks booting on my board (am3352, eMMC boot, FIT u-boot, CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it from eMMC I get nothing at all on the console, if I boot over ymodem it stalls at 420k, before continuing to 460k. My guess is there's some error going to the console at the 420k mark, but obviously it's lost in the ymodem... I have two DTBs in the FIT image, 420k would about align to the point between them.
My bet would be on some padding / unaligned access problem that this patch uncovered. Can you take a look ?
Seems plausible. With this change my external data starts at 0x483 and everything after it is non-aligned:
Should the beginning of external data be aligned ?
If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does the problem go away? If so, that's not a fix outright, it means we need to dig back in to the libfdt thread and find the "make this work without killing performance everywhere all the time" option.
On 5/5/20 7:50 PM, Tom Rini wrote:
On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote:
On 5/5/20 6:37 PM, Alex Kiernan wrote:
On Tue, May 5, 2020 at 2:28 PM Marek Vasut marex@denx.de wrote:
On 5/5/20 3:22 PM, Alex Kiernan wrote:
On Mon, May 4, 2020 at 12:28 PM Tom Rini trini@konsulko.com wrote:
On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote:
> There is no reason to tail-pad fitImage with external data to 4-bytes, > while fitImage without external data does not have any such padding and > is often unaligned. DT spec also does not mandate any such padding. > > Moreover, the tail-pad fills the last few bytes with uninitialized data, > which could lead to a potential information leak. > > $ echo -n xy > /tmp/data ; \ > ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \ > hexdump -vC /tmp/fitImage | tail -n 3 > > before: > 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| > 00000270 7a 65 00 00 78 79 64 64 |ze..xydd| > ^^ ^^ ^^ > after: > 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| > 00000270 7a 65 00 78 79 |ze.xy| > > Signed-off-by: Marek Vasut marex@denx.de > Reviewed-by: Simon Glass sjg@chromium.org > Cc: Heinrich Schuchardt xypron.glpk@gmx.de > Cc: Tom Rini trini@konsulko.com
Applied to u-boot/master, thanks!
This breaks booting on my board (am3352, eMMC boot, FIT u-boot, CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it from eMMC I get nothing at all on the console, if I boot over ymodem it stalls at 420k, before continuing to 460k. My guess is there's some error going to the console at the 420k mark, but obviously it's lost in the ymodem... I have two DTBs in the FIT image, 420k would about align to the point between them.
My bet would be on some padding / unaligned access problem that this patch uncovered. Can you take a look ?
Seems plausible. With this change my external data starts at 0x483 and everything after it is non-aligned:
Should the beginning of external data be aligned ?
If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does the problem go away? If so, that's not a fix outright, it means we need to dig back in to the libfdt thread and find the "make this work without killing performance everywhere all the time" option.
Still, my question is, should the beginning of external data be aligned ? And if so, to what, 4 bytes like FDT entries OR 8 bytes to cater for arm64/rv64 ?
On Tue, May 05, 2020 at 07:53:42PM +0200, Marek Vasut wrote:
On 5/5/20 7:50 PM, Tom Rini wrote:
On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote:
On 5/5/20 6:37 PM, Alex Kiernan wrote:
On Tue, May 5, 2020 at 2:28 PM Marek Vasut marex@denx.de wrote:
On 5/5/20 3:22 PM, Alex Kiernan wrote:
On Mon, May 4, 2020 at 12:28 PM Tom Rini trini@konsulko.com wrote: > > On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote: > >> There is no reason to tail-pad fitImage with external data to 4-bytes, >> while fitImage without external data does not have any such padding and >> is often unaligned. DT spec also does not mandate any such padding. >> >> Moreover, the tail-pad fills the last few bytes with uninitialized data, >> which could lead to a potential information leak. >> >> $ echo -n xy > /tmp/data ; \ >> ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \ >> hexdump -vC /tmp/fitImage | tail -n 3 >> >> before: >> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| >> 00000270 7a 65 00 00 78 79 64 64 |ze..xydd| >> ^^ ^^ ^^ >> after: >> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| >> 00000270 7a 65 00 78 79 |ze.xy| >> >> Signed-off-by: Marek Vasut marex@denx.de >> Reviewed-by: Simon Glass sjg@chromium.org >> Cc: Heinrich Schuchardt xypron.glpk@gmx.de >> Cc: Tom Rini trini@konsulko.com > > Applied to u-boot/master, thanks! >
This breaks booting on my board (am3352, eMMC boot, FIT u-boot, CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it from eMMC I get nothing at all on the console, if I boot over ymodem it stalls at 420k, before continuing to 460k. My guess is there's some error going to the console at the 420k mark, but obviously it's lost in the ymodem... I have two DTBs in the FIT image, 420k would about align to the point between them.
My bet would be on some padding / unaligned access problem that this patch uncovered. Can you take a look ?
Seems plausible. With this change my external data starts at 0x483 and everything after it is non-aligned:
Should the beginning of external data be aligned ?
If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does the problem go away? If so, that's not a fix outright, it means we need to dig back in to the libfdt thread and find the "make this work without killing performance everywhere all the time" option.
Still, my question is, should the beginning of external data be aligned ? And if so, to what, 4 bytes like FDT entries OR 8 bytes to cater for arm64/rv64 ?
Is "external data" the kernel in this case? If so, I swear Linux mandates 8 byte alignment for arm32 as well.
On 5/5/20 7:55 PM, Tom Rini wrote:
On Tue, May 05, 2020 at 07:53:42PM +0200, Marek Vasut wrote:
On 5/5/20 7:50 PM, Tom Rini wrote:
On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote:
On 5/5/20 6:37 PM, Alex Kiernan wrote:
On Tue, May 5, 2020 at 2:28 PM Marek Vasut marex@denx.de wrote:
On 5/5/20 3:22 PM, Alex Kiernan wrote: > On Mon, May 4, 2020 at 12:28 PM Tom Rini trini@konsulko.com wrote: >> >> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote: >> >>> There is no reason to tail-pad fitImage with external data to 4-bytes, >>> while fitImage without external data does not have any such padding and >>> is often unaligned. DT spec also does not mandate any such padding. >>> >>> Moreover, the tail-pad fills the last few bytes with uninitialized data, >>> which could lead to a potential information leak. >>> >>> $ echo -n xy > /tmp/data ; \ >>> ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \ >>> hexdump -vC /tmp/fitImage | tail -n 3 >>> >>> before: >>> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| >>> 00000270 7a 65 00 00 78 79 64 64 |ze..xydd| >>> ^^ ^^ ^^ >>> after: >>> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| >>> 00000270 7a 65 00 78 79 |ze.xy| >>> >>> Signed-off-by: Marek Vasut marex@denx.de >>> Reviewed-by: Simon Glass sjg@chromium.org >>> Cc: Heinrich Schuchardt xypron.glpk@gmx.de >>> Cc: Tom Rini trini@konsulko.com >> >> Applied to u-boot/master, thanks! >> > > This breaks booting on my board (am3352, eMMC boot, FIT u-boot, > CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it > from eMMC I get nothing at all on the console, if I boot over ymodem > it stalls at 420k, before continuing to 460k. My guess is there's some > error going to the console at the 420k mark, but obviously it's lost > in the ymodem... I have two DTBs in the FIT image, 420k would about > align to the point between them.
My bet would be on some padding / unaligned access problem that this patch uncovered. Can you take a look ?
Seems plausible. With this change my external data starts at 0x483 and everything after it is non-aligned:
Should the beginning of external data be aligned ?
If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does the problem go away? If so, that's not a fix outright, it means we need to dig back in to the libfdt thread and find the "make this work without killing performance everywhere all the time" option.
Still, my question is, should the beginning of external data be aligned ? And if so, to what, 4 bytes like FDT entries OR 8 bytes to cater for arm64/rv64 ?
Is "external data" the kernel in this case? If so, I swear Linux mandates 8 byte alignment for arm32 as well.
External data can be anything, and if it is supposed to be 8 bytes, we already failed at that since forever.
On Tue, May 05, 2020 at 07:59:24PM +0200, Marek Vasut wrote:
On 5/5/20 7:55 PM, Tom Rini wrote:
On Tue, May 05, 2020 at 07:53:42PM +0200, Marek Vasut wrote:
On 5/5/20 7:50 PM, Tom Rini wrote:
On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote:
On 5/5/20 6:37 PM, Alex Kiernan wrote:
On Tue, May 5, 2020 at 2:28 PM Marek Vasut marex@denx.de wrote: > > On 5/5/20 3:22 PM, Alex Kiernan wrote: >> On Mon, May 4, 2020 at 12:28 PM Tom Rini trini@konsulko.com wrote: >>> >>> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote: >>> >>>> There is no reason to tail-pad fitImage with external data to 4-bytes, >>>> while fitImage without external data does not have any such padding and >>>> is often unaligned. DT spec also does not mandate any such padding. >>>> >>>> Moreover, the tail-pad fills the last few bytes with uninitialized data, >>>> which could lead to a potential information leak. >>>> >>>> $ echo -n xy > /tmp/data ; \ >>>> ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \ >>>> hexdump -vC /tmp/fitImage | tail -n 3 >>>> >>>> before: >>>> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| >>>> 00000270 7a 65 00 00 78 79 64 64 |ze..xydd| >>>> ^^ ^^ ^^ >>>> after: >>>> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| >>>> 00000270 7a 65 00 78 79 |ze.xy| >>>> >>>> Signed-off-by: Marek Vasut marex@denx.de >>>> Reviewed-by: Simon Glass sjg@chromium.org >>>> Cc: Heinrich Schuchardt xypron.glpk@gmx.de >>>> Cc: Tom Rini trini@konsulko.com >>> >>> Applied to u-boot/master, thanks! >>> >> >> This breaks booting on my board (am3352, eMMC boot, FIT u-boot, >> CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it >> from eMMC I get nothing at all on the console, if I boot over ymodem >> it stalls at 420k, before continuing to 460k. My guess is there's some >> error going to the console at the 420k mark, but obviously it's lost >> in the ymodem... I have two DTBs in the FIT image, 420k would about >> align to the point between them. > > My bet would be on some padding / unaligned access problem that this > patch uncovered. Can you take a look ?
Seems plausible. With this change my external data starts at 0x483 and everything after it is non-aligned:
Should the beginning of external data be aligned ?
If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does the problem go away? If so, that's not a fix outright, it means we need to dig back in to the libfdt thread and find the "make this work without killing performance everywhere all the time" option.
Still, my question is, should the beginning of external data be aligned ? And if so, to what, 4 bytes like FDT entries OR 8 bytes to cater for arm64/rv64 ?
Is "external data" the kernel in this case? If so, I swear Linux mandates 8 byte alignment for arm32 as well.
External data can be anything, and if it is supposed to be 8 bytes, we already failed at that since forever.
I would be entirely unsurprised at things working through a combination of luck and co-incidence in our previous padding working out. So, what typically is "external data" in this context?
On 5/5/20 8:06 PM, Tom Rini wrote:
On Tue, May 05, 2020 at 07:59:24PM +0200, Marek Vasut wrote:
On 5/5/20 7:55 PM, Tom Rini wrote:
On Tue, May 05, 2020 at 07:53:42PM +0200, Marek Vasut wrote:
On 5/5/20 7:50 PM, Tom Rini wrote:
On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote:
On 5/5/20 6:37 PM, Alex Kiernan wrote: > On Tue, May 5, 2020 at 2:28 PM Marek Vasut marex@denx.de wrote: >> >> On 5/5/20 3:22 PM, Alex Kiernan wrote: >>> On Mon, May 4, 2020 at 12:28 PM Tom Rini trini@konsulko.com wrote: >>>> >>>> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote: >>>> >>>>> There is no reason to tail-pad fitImage with external data to 4-bytes, >>>>> while fitImage without external data does not have any such padding and >>>>> is often unaligned. DT spec also does not mandate any such padding. >>>>> >>>>> Moreover, the tail-pad fills the last few bytes with uninitialized data, >>>>> which could lead to a potential information leak. >>>>> >>>>> $ echo -n xy > /tmp/data ; \ >>>>> ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \ >>>>> hexdump -vC /tmp/fitImage | tail -n 3 >>>>> >>>>> before: >>>>> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| >>>>> 00000270 7a 65 00 00 78 79 64 64 |ze..xydd| >>>>> ^^ ^^ ^^ >>>>> after: >>>>> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| >>>>> 00000270 7a 65 00 78 79 |ze.xy| >>>>> >>>>> Signed-off-by: Marek Vasut marex@denx.de >>>>> Reviewed-by: Simon Glass sjg@chromium.org >>>>> Cc: Heinrich Schuchardt xypron.glpk@gmx.de >>>>> Cc: Tom Rini trini@konsulko.com >>>> >>>> Applied to u-boot/master, thanks! >>>> >>> >>> This breaks booting on my board (am3352, eMMC boot, FIT u-boot, >>> CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it >>> from eMMC I get nothing at all on the console, if I boot over ymodem >>> it stalls at 420k, before continuing to 460k. My guess is there's some >>> error going to the console at the 420k mark, but obviously it's lost >>> in the ymodem... I have two DTBs in the FIT image, 420k would about >>> align to the point between them. >> >> My bet would be on some padding / unaligned access problem that this >> patch uncovered. Can you take a look ? > > Seems plausible. With this change my external data starts at 0x483 and > everything after it is non-aligned:
Should the beginning of external data be aligned ?
If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does the problem go away? If so, that's not a fix outright, it means we need to dig back in to the libfdt thread and find the "make this work without killing performance everywhere all the time" option.
Still, my question is, should the beginning of external data be aligned ? And if so, to what, 4 bytes like FDT entries OR 8 bytes to cater for arm64/rv64 ?
Is "external data" the kernel in this case? If so, I swear Linux mandates 8 byte alignment for arm32 as well.
External data can be anything, and if it is supposed to be 8 bytes, we already failed at that since forever.
I would be entirely unsurprised at things working through a combination of luck and co-incidence in our previous padding working out. So, what typically is "external data" in this context?
Anything you can bundle into the fitImage, -E just puts the content at the end of the fitImage and places a reference into the fitImage itself.
Hi Tom,
On Tue, 5 May 2020 at 11:50, Tom Rini trini@konsulko.com wrote:
On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote:
On 5/5/20 6:37 PM, Alex Kiernan wrote:
On Tue, May 5, 2020 at 2:28 PM Marek Vasut marex@denx.de wrote:
On 5/5/20 3:22 PM, Alex Kiernan wrote:
On Mon, May 4, 2020 at 12:28 PM Tom Rini trini@konsulko.com wrote:
On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote:
> There is no reason to tail-pad fitImage with external data to 4-bytes, > while fitImage without external data does not have any such padding and > is often unaligned. DT spec also does not mandate any such padding. > > Moreover, the tail-pad fills the last few bytes with uninitialized data, > which could lead to a potential information leak. > > $ echo -n xy > /tmp/data ; \ > ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \ > hexdump -vC /tmp/fitImage | tail -n 3 > > before: > 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| > 00000270 7a 65 00 00 78 79 64 64 |ze..xydd| > ^^ ^^ ^^ > after: > 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| > 00000270 7a 65 00 78 79 |ze.xy| > > Signed-off-by: Marek Vasut marex@denx.de > Reviewed-by: Simon Glass sjg@chromium.org > Cc: Heinrich Schuchardt xypron.glpk@gmx.de > Cc: Tom Rini trini@konsulko.com
Applied to u-boot/master, thanks!
This breaks booting on my board (am3352, eMMC boot, FIT u-boot, CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it from eMMC I get nothing at all on the console, if I boot over ymodem it stalls at 420k, before continuing to 460k. My guess is there's some error going to the console at the 420k mark, but obviously it's lost in the ymodem... I have two DTBs in the FIT image, 420k would about align to the point between them.
My bet would be on some padding / unaligned access problem that this patch uncovered. Can you take a look ?
Seems plausible. With this change my external data starts at 0x483 and everything after it is non-aligned:
Should the beginning of external data be aligned ?
If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does the problem go away? If so, that's not a fix outright, it means we need to dig back in to the libfdt thread and find the "make this work without killing performance everywhere all the time" option.
If it is a device tree, it must be 32-bit aligned.
But Marek's patch affects the FIT image itself, so I am not sure what would go after that.
Regards, Simon
Hi all,
Am 2020-05-05 20:41, schrieb Simon Glass:
Hi Tom,
On Tue, 5 May 2020 at 11:50, Tom Rini trini@konsulko.com wrote:
On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote:
On 5/5/20 6:37 PM, Alex Kiernan wrote:
On Tue, May 5, 2020 at 2:28 PM Marek Vasut marex@denx.de wrote:
On 5/5/20 3:22 PM, Alex Kiernan wrote:
On Mon, May 4, 2020 at 12:28 PM Tom Rini trini@konsulko.com wrote: > > On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote: > >> There is no reason to tail-pad fitImage with external data to 4-bytes, >> while fitImage without external data does not have any such padding and >> is often unaligned. DT spec also does not mandate any such padding. >> >> Moreover, the tail-pad fills the last few bytes with uninitialized data, >> which could lead to a potential information leak. >> >> $ echo -n xy > /tmp/data ; \ >> ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \ >> hexdump -vC /tmp/fitImage | tail -n 3 >> >> before: >> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| >> 00000270 7a 65 00 00 78 79 64 64 |ze..xydd| >> ^^ ^^ ^^ >> after: >> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| >> 00000270 7a 65 00 78 79 |ze.xy| >> >> Signed-off-by: Marek Vasut marex@denx.de >> Reviewed-by: Simon Glass sjg@chromium.org >> Cc: Heinrich Schuchardt xypron.glpk@gmx.de >> Cc: Tom Rini trini@konsulko.com > > Applied to u-boot/master, thanks! >
This breaks booting on my board (am3352, eMMC boot, FIT u-boot, CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it from eMMC I get nothing at all on the console, if I boot over ymodem it stalls at 420k, before continuing to 460k. My guess is there's some error going to the console at the 420k mark, but obviously it's lost in the ymodem... I have two DTBs in the FIT image, 420k would about align to the point between them.
My bet would be on some padding / unaligned access problem that this patch uncovered. Can you take a look ?
Seems plausible. With this change my external data starts at 0x483 and everything after it is non-aligned:
Should the beginning of external data be aligned ?
If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does the problem go away? If so, that's not a fix outright, it means we need to dig back in to the libfdt thread and find the "make this work without killing performance everywhere all the time" option.
If it is a device tree, it must be 32-bit aligned.
This commit actually breaks my board too (which I was just about to send upstream, but realized it was broken).
Said board uses SPL and main U-Boot. SPL runs fine and main u-boot doesn't output anything. The only difference which I found is that fit-dtb.blob is 2 bytes shorter. And the content is shifted by one byte although data-offset is the same. Strange. In the non-working case, the inner FDT magic isn't 4 byte aligned.
You can find the two fit-dtb.blobs here:
https://walle.cc/u-boot/fit-dtb.blob.working https://walle.cc/u-boot/fit-dtb.blob.non-working
Reverting e8c2d25845c72c7202a628a97d45e31beea40668 doesn't help (I might reverted it the wrong way, there is actually a conflict).
I'll dig deeper into that tomorrow, but maybe you have some pointers where to look.
For reference you can find the current patch here: https://github.com/mwalle/u-boot/tree/sl28-upstream
But Marek's patch affects the FIT image itself, so I am not sure what would go after that.
Regards, Simon
Am 2020-05-05 23:17, schrieb Michael Walle:
Hi all,
Am 2020-05-05 20:41, schrieb Simon Glass:
Hi Tom,
On Tue, 5 May 2020 at 11:50, Tom Rini trini@konsulko.com wrote:
On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote:
On 5/5/20 6:37 PM, Alex Kiernan wrote:
On Tue, May 5, 2020 at 2:28 PM Marek Vasut marex@denx.de wrote:
On 5/5/20 3:22 PM, Alex Kiernan wrote: > On Mon, May 4, 2020 at 12:28 PM Tom Rini trini@konsulko.com wrote: >> >> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote: >> >>> There is no reason to tail-pad fitImage with external data to 4-bytes, >>> while fitImage without external data does not have any such padding and >>> is often unaligned. DT spec also does not mandate any such padding. >>> >>> Moreover, the tail-pad fills the last few bytes with uninitialized data, >>> which could lead to a potential information leak. >>> >>> $ echo -n xy > /tmp/data ; \ >>> ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \ >>> hexdump -vC /tmp/fitImage | tail -n 3 >>> >>> before: >>> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| >>> 00000270 7a 65 00 00 78 79 64 64 |ze..xydd| >>> ^^ ^^ ^^ >>> after: >>> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| >>> 00000270 7a 65 00 78 79 |ze.xy| >>> >>> Signed-off-by: Marek Vasut marex@denx.de >>> Reviewed-by: Simon Glass sjg@chromium.org >>> Cc: Heinrich Schuchardt xypron.glpk@gmx.de >>> Cc: Tom Rini trini@konsulko.com >> >> Applied to u-boot/master, thanks! >> > > This breaks booting on my board (am3352, eMMC boot, FIT u-boot, > CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it > from eMMC I get nothing at all on the console, if I boot over ymodem > it stalls at 420k, before continuing to 460k. My guess is there's some > error going to the console at the 420k mark, but obviously it's lost > in the ymodem... I have two DTBs in the FIT image, 420k would about > align to the point between them.
My bet would be on some padding / unaligned access problem that this patch uncovered. Can you take a look ?
Seems plausible. With this change my external data starts at 0x483 and everything after it is non-aligned:
Should the beginning of external data be aligned ?
If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does the problem go away? If so, that's not a fix outright, it means we need to dig back in to the libfdt thread and find the "make this work without killing performance everywhere all the time" option.
If it is a device tree, it must be 32-bit aligned.
This commit actually breaks my board too (which I was just about to send upstream, but realized it was broken).
Said board uses SPL and main U-Boot. SPL runs fine and main u-boot doesn't output anything. The only difference which I found is that fit-dtb.blob is 2 bytes shorter. And the content is shifted by one byte although data-offset is the same. Strange. In the non-working case, the inner FDT magic isn't 4 byte aligned.
Here are the steps to reproduce it on the uboot master branch:
$ CROSS_COMPILE=arm-linux-gnueabihf- make pico-imx6_defconfig fit-dtb.blob tools $ tools/dumpimage -T flat_dt -p 1 -o fdt-1 fit-dtb.blob $ file fdt-1 fdt-1: data $ git revert 20a154f95bfe0a3b5bfba90bea7f001c58217536 $ git clean -fdx $ CROSS_COMPILE=arm-linux-gnueabihf- make pico-imx6_defconfig fit-dtb.blob tools $ tools/dumpimage -T flat_dt -p 1 -o fdt-1 fit-dtb.blob $ file fdt-1 fdt-1: Device Tree Blob version 17, size=34978, boot CPU=0, string block size=1746, DT structure block size=33176
-michael
On Tue, May 5, 2020 at 10:17 PM Michael Walle michael@walle.cc wrote:
Hi all,
Am 2020-05-05 20:41, schrieb Simon Glass:
Hi Tom,
On Tue, 5 May 2020 at 11:50, Tom Rini trini@konsulko.com wrote:
On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote:
On 5/5/20 6:37 PM, Alex Kiernan wrote:
On Tue, May 5, 2020 at 2:28 PM Marek Vasut marex@denx.de wrote:
On 5/5/20 3:22 PM, Alex Kiernan wrote: > On Mon, May 4, 2020 at 12:28 PM Tom Rini trini@konsulko.com wrote: >> >> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote: >> >>> There is no reason to tail-pad fitImage with external data to 4-bytes, >>> while fitImage without external data does not have any such padding and >>> is often unaligned. DT spec also does not mandate any such padding. >>> >>> Moreover, the tail-pad fills the last few bytes with uninitialized data, >>> which could lead to a potential information leak. >>> >>> $ echo -n xy > /tmp/data ; \ >>> ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \ >>> hexdump -vC /tmp/fitImage | tail -n 3 >>> >>> before: >>> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| >>> 00000270 7a 65 00 00 78 79 64 64 |ze..xydd| >>> ^^ ^^ ^^ >>> after: >>> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| >>> 00000270 7a 65 00 78 79 |ze.xy| >>> >>> Signed-off-by: Marek Vasut marex@denx.de >>> Reviewed-by: Simon Glass sjg@chromium.org >>> Cc: Heinrich Schuchardt xypron.glpk@gmx.de >>> Cc: Tom Rini trini@konsulko.com >> >> Applied to u-boot/master, thanks! >> > > This breaks booting on my board (am3352, eMMC boot, FIT u-boot, > CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it > from eMMC I get nothing at all on the console, if I boot over ymodem > it stalls at 420k, before continuing to 460k. My guess is there's some > error going to the console at the 420k mark, but obviously it's lost > in the ymodem... I have two DTBs in the FIT image, 420k would about > align to the point between them.
My bet would be on some padding / unaligned access problem that this patch uncovered. Can you take a look ?
Seems plausible. With this change my external data starts at 0x483 and everything after it is non-aligned:
Should the beginning of external data be aligned ?
If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does the problem go away? If so, that's not a fix outright, it means we need to dig back in to the libfdt thread and find the "make this work without killing performance everywhere all the time" option.
If it is a device tree, it must be 32-bit aligned.
This commit actually breaks my board too (which I was just about to send upstream, but realized it was broken).
Said board uses SPL and main U-Boot. SPL runs fine and main u-boot doesn't output anything. The only difference which I found is that fit-dtb.blob is 2 bytes shorter. And the content is shifted by one byte although data-offset is the same. Strange. In the non-working case, the inner FDT magic isn't 4 byte aligned.
You can find the two fit-dtb.blobs here:
https://walle.cc/u-boot/fit-dtb.blob.working https://walle.cc/u-boot/fit-dtb.blob.non-working
Reverting e8c2d25845c72c7202a628a97d45e31beea40668 doesn't help (I might reverted it the wrong way, there is actually a conflict).
I just did the same thing by manually extracting the patches and I agree, reverting that doesn't fix the problem.
I'll dig deeper into that tomorrow, but maybe you have some pointers where to look.
For reference you can find the current patch here: https://github.com/mwalle/u-boot/tree/sl28-upstream
But Marek's patch affects the FIT image itself, so I am not sure what would go after that.
Regards, Simon
-- -michael
On Tue, May 05, 2020 at 11:17:19PM +0200, Michael Walle wrote:
Hi all,
Am 2020-05-05 20:41, schrieb Simon Glass:
Hi Tom,
On Tue, 5 May 2020 at 11:50, Tom Rini trini@konsulko.com wrote:
On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote:
On 5/5/20 6:37 PM, Alex Kiernan wrote:
On Tue, May 5, 2020 at 2:28 PM Marek Vasut marex@denx.de wrote:
On 5/5/20 3:22 PM, Alex Kiernan wrote: > On Mon, May 4, 2020 at 12:28 PM Tom Rini trini@konsulko.com wrote: >> >> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote: >> >>> There is no reason to tail-pad fitImage with external data to 4-bytes, >>> while fitImage without external data does not have any such padding and >>> is often unaligned. DT spec also does not mandate any such padding. >>> >>> Moreover, the tail-pad fills the last few bytes with uninitialized data, >>> which could lead to a potential information leak. >>> >>> $ echo -n xy > /tmp/data ; \ >>> ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \ >>> hexdump -vC /tmp/fitImage | tail -n 3 >>> >>> before: >>> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| >>> 00000270 7a 65 00 00 78 79 64 64 |ze..xydd| >>> ^^ ^^ ^^ >>> after: >>> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| >>> 00000270 7a 65 00 78 79 |ze.xy| >>> >>> Signed-off-by: Marek Vasut marex@denx.de >>> Reviewed-by: Simon Glass sjg@chromium.org >>> Cc: Heinrich Schuchardt xypron.glpk@gmx.de >>> Cc: Tom Rini trini@konsulko.com >> >> Applied to u-boot/master, thanks! >> > > This breaks booting on my board (am3352, eMMC boot, FIT u-boot, > CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it > from eMMC I get nothing at all on the console, if I boot over ymodem > it stalls at 420k, before continuing to 460k. My guess is there's some > error going to the console at the 420k mark, but obviously it's lost > in the ymodem... I have two DTBs in the FIT image, 420k would about > align to the point between them.
My bet would be on some padding / unaligned access problem that this patch uncovered. Can you take a look ?
Seems plausible. With this change my external data starts at 0x483 and everything after it is non-aligned:
Should the beginning of external data be aligned ?
If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does the problem go away? If so, that's not a fix outright, it means we need to dig back in to the libfdt thread and find the "make this work without killing performance everywhere all the time" option.
If it is a device tree, it must be 32-bit aligned.
This commit actually breaks my board too (which I was just about to send upstream, but realized it was broken).
Said board uses SPL and main U-Boot. SPL runs fine and main u-boot doesn't output anything. The only difference which I found is that fit-dtb.blob is 2 bytes shorter. And the content is shifted by one byte although data-offset is the same. Strange. In the non-working case, the inner FDT magic isn't 4 byte aligned.
You can find the two fit-dtb.blobs here:
https://walle.cc/u-boot/fit-dtb.blob.working https://walle.cc/u-boot/fit-dtb.blob.non-working
Reverting e8c2d25845c72c7202a628a97d45e31beea40668 doesn't help (I might reverted it the wrong way, there is actually a conflict).
I'll dig deeper into that tomorrow, but maybe you have some pointers where to look.
For reference you can find the current patch here: https://github.com/mwalle/u-boot/tree/sl28-upstream
I think we have a few things to fix here. Marek's patch is breaking things and needs to be reverted. But it's showing a few underlying problems that need to be fixed too: - fit_extract_data() needs to use calloc() not malloc() so that we don't leak random data. - We need to 8-byte alignment on the external data. That's the requirement for Linux for device trees on both 32 and 64bit arm. Atish, does RISC-V require more than that? I don't see it in Linux's Documentation/riscv/boot-image-header.rst (and there's no booting.rst file like arm/arm64).
Thanks all!
On 5/6/20 3:48 PM, Tom Rini wrote:
On Tue, May 05, 2020 at 11:17:19PM +0200, Michael Walle wrote:
Hi all,
Am 2020-05-05 20:41, schrieb Simon Glass:
Hi Tom,
On Tue, 5 May 2020 at 11:50, Tom Rini trini@konsulko.com wrote:
On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote:
On 5/5/20 6:37 PM, Alex Kiernan wrote:
On Tue, May 5, 2020 at 2:28 PM Marek Vasut marex@denx.de wrote: > > On 5/5/20 3:22 PM, Alex Kiernan wrote: >> On Mon, May 4, 2020 at 12:28 PM Tom Rini trini@konsulko.com wrote: >>> >>> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote: >>> >>>> There is no reason to tail-pad fitImage with external data to 4-bytes, >>>> while fitImage without external data does not have any such padding and >>>> is often unaligned. DT spec also does not mandate any such padding. >>>> >>>> Moreover, the tail-pad fills the last few bytes with uninitialized data, >>>> which could lead to a potential information leak. >>>> >>>> $ echo -n xy > /tmp/data ; \ >>>> ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \ >>>> hexdump -vC /tmp/fitImage | tail -n 3 >>>> >>>> before: >>>> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| >>>> 00000270 7a 65 00 00 78 79 64 64 |ze..xydd| >>>> ^^ ^^ ^^ >>>> after: >>>> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| >>>> 00000270 7a 65 00 78 79 |ze.xy| >>>> >>>> Signed-off-by: Marek Vasut marex@denx.de >>>> Reviewed-by: Simon Glass sjg@chromium.org >>>> Cc: Heinrich Schuchardt xypron.glpk@gmx.de >>>> Cc: Tom Rini trini@konsulko.com >>> >>> Applied to u-boot/master, thanks! >>> >> >> This breaks booting on my board (am3352, eMMC boot, FIT u-boot, >> CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it >> from eMMC I get nothing at all on the console, if I boot over ymodem >> it stalls at 420k, before continuing to 460k. My guess is there's some >> error going to the console at the 420k mark, but obviously it's lost >> in the ymodem... I have two DTBs in the FIT image, 420k would about >> align to the point between them. > > My bet would be on some padding / unaligned access problem that this > patch uncovered. Can you take a look ?
Seems plausible. With this change my external data starts at 0x483 and everything after it is non-aligned:
Should the beginning of external data be aligned ?
If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does the problem go away? If so, that's not a fix outright, it means we need to dig back in to the libfdt thread and find the "make this work without killing performance everywhere all the time" option.
If it is a device tree, it must be 32-bit aligned.
This commit actually breaks my board too (which I was just about to send upstream, but realized it was broken).
Said board uses SPL and main U-Boot. SPL runs fine and main u-boot doesn't output anything. The only difference which I found is that fit-dtb.blob is 2 bytes shorter. And the content is shifted by one byte although data-offset is the same. Strange. In the non-working case, the inner FDT magic isn't 4 byte aligned.
You can find the two fit-dtb.blobs here:
https://walle.cc/u-boot/fit-dtb.blob.working https://walle.cc/u-boot/fit-dtb.blob.non-working
Reverting e8c2d25845c72c7202a628a97d45e31beea40668 doesn't help (I might reverted it the wrong way, there is actually a conflict).
I'll dig deeper into that tomorrow, but maybe you have some pointers where to look.
For reference you can find the current patch here: https://github.com/mwalle/u-boot/tree/sl28-upstream
I think we have a few things to fix here. Marek's patch is breaking things and needs to be reverted. But it's showing a few underlying problems that need to be fixed too:
- fit_extract_data() needs to use calloc() not malloc() so that we don't leak random data.
- We need to 8-byte alignment on the external data. That's the requirement for Linux for device trees on both 32 and 64bit arm. Atish, does RISC-V require more than that? I don't see it in Linux's Documentation/riscv/boot-image-header.rst (and there's no booting.rst file like arm/arm64).
Why 8-byte alignment ? The external data are copied into the target location, so why do they need to be padded in any way?
If the external data are used in place, then it's the same problem as arm64 fitImage with fdt_high=-1, which fails because the DT is aligned to 4 bytes and arm64 expects it at 8byte offset.
On Wed, May 06, 2020 at 04:17:35PM +0200, Marek Vasut wrote:
On 5/6/20 3:48 PM, Tom Rini wrote:
On Tue, May 05, 2020 at 11:17:19PM +0200, Michael Walle wrote:
Hi all,
Am 2020-05-05 20:41, schrieb Simon Glass:
Hi Tom,
On Tue, 5 May 2020 at 11:50, Tom Rini trini@konsulko.com wrote:
On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote:
On 5/5/20 6:37 PM, Alex Kiernan wrote: > On Tue, May 5, 2020 at 2:28 PM Marek Vasut marex@denx.de wrote: >> >> On 5/5/20 3:22 PM, Alex Kiernan wrote: >>> On Mon, May 4, 2020 at 12:28 PM Tom Rini trini@konsulko.com wrote: >>>> >>>> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote: >>>> >>>>> There is no reason to tail-pad fitImage with external data to 4-bytes, >>>>> while fitImage without external data does not have any such padding and >>>>> is often unaligned. DT spec also does not mandate any such padding. >>>>> >>>>> Moreover, the tail-pad fills the last few bytes with uninitialized data, >>>>> which could lead to a potential information leak. >>>>> >>>>> $ echo -n xy > /tmp/data ; \ >>>>> ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \ >>>>> hexdump -vC /tmp/fitImage | tail -n 3 >>>>> >>>>> before: >>>>> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| >>>>> 00000270 7a 65 00 00 78 79 64 64 |ze..xydd| >>>>> ^^ ^^ ^^ >>>>> after: >>>>> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| >>>>> 00000270 7a 65 00 78 79 |ze.xy| >>>>> >>>>> Signed-off-by: Marek Vasut marex@denx.de >>>>> Reviewed-by: Simon Glass sjg@chromium.org >>>>> Cc: Heinrich Schuchardt xypron.glpk@gmx.de >>>>> Cc: Tom Rini trini@konsulko.com >>>> >>>> Applied to u-boot/master, thanks! >>>> >>> >>> This breaks booting on my board (am3352, eMMC boot, FIT u-boot, >>> CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it >>> from eMMC I get nothing at all on the console, if I boot over ymodem >>> it stalls at 420k, before continuing to 460k. My guess is there's some >>> error going to the console at the 420k mark, but obviously it's lost >>> in the ymodem... I have two DTBs in the FIT image, 420k would about >>> align to the point between them. >> >> My bet would be on some padding / unaligned access problem that this >> patch uncovered. Can you take a look ? > > Seems plausible. With this change my external data starts at 0x483 and > everything after it is non-aligned:
Should the beginning of external data be aligned ?
If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does the problem go away? If so, that's not a fix outright, it means we need to dig back in to the libfdt thread and find the "make this work without killing performance everywhere all the time" option.
If it is a device tree, it must be 32-bit aligned.
This commit actually breaks my board too (which I was just about to send upstream, but realized it was broken).
Said board uses SPL and main U-Boot. SPL runs fine and main u-boot doesn't output anything. The only difference which I found is that fit-dtb.blob is 2 bytes shorter. And the content is shifted by one byte although data-offset is the same. Strange. In the non-working case, the inner FDT magic isn't 4 byte aligned.
You can find the two fit-dtb.blobs here:
https://walle.cc/u-boot/fit-dtb.blob.working https://walle.cc/u-boot/fit-dtb.blob.non-working
Reverting e8c2d25845c72c7202a628a97d45e31beea40668 doesn't help (I might reverted it the wrong way, there is actually a conflict).
I'll dig deeper into that tomorrow, but maybe you have some pointers where to look.
For reference you can find the current patch here: https://github.com/mwalle/u-boot/tree/sl28-upstream
I think we have a few things to fix here. Marek's patch is breaking things and needs to be reverted. But it's showing a few underlying problems that need to be fixed too:
- fit_extract_data() needs to use calloc() not malloc() so that we don't leak random data.
- We need to 8-byte alignment on the external data. That's the requirement for Linux for device trees on both 32 and 64bit arm. Atish, does RISC-V require more than that? I don't see it in Linux's Documentation/riscv/boot-image-header.rst (and there's no booting.rst file like arm/arm64).
Why 8-byte alignment ? The external data are copied into the target location, so why do they need to be padded in any way?
The start of the external data needs the alignment, to be clearer.
If the external data are used in place, then it's the same problem as arm64 fitImage with fdt_high=-1, which fails because the DT is aligned to 4 bytes and arm64 expects it at 8byte offset.
Except we're talking about cases where we can't relocate the data or it doesn't make any sense to.
That said, if you think the answer is that we need to ensure that when we use external data we first align it, please show us how that looks. Thanks!
On 5/6/20 4:27 PM, Tom Rini wrote:
On Wed, May 06, 2020 at 04:17:35PM +0200, Marek Vasut wrote:
On 5/6/20 3:48 PM, Tom Rini wrote:
On Tue, May 05, 2020 at 11:17:19PM +0200, Michael Walle wrote:
Hi all,
Am 2020-05-05 20:41, schrieb Simon Glass:
Hi Tom,
On Tue, 5 May 2020 at 11:50, Tom Rini trini@konsulko.com wrote:
On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote: > On 5/5/20 6:37 PM, Alex Kiernan wrote: >> On Tue, May 5, 2020 at 2:28 PM Marek Vasut marex@denx.de wrote: >>> >>> On 5/5/20 3:22 PM, Alex Kiernan wrote: >>>> On Mon, May 4, 2020 at 12:28 PM Tom Rini trini@konsulko.com wrote: >>>>> >>>>> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote: >>>>> >>>>>> There is no reason to tail-pad fitImage with external data to 4-bytes, >>>>>> while fitImage without external data does not have any such padding and >>>>>> is often unaligned. DT spec also does not mandate any such padding. >>>>>> >>>>>> Moreover, the tail-pad fills the last few bytes with uninitialized data, >>>>>> which could lead to a potential information leak. >>>>>> >>>>>> $ echo -n xy > /tmp/data ; \ >>>>>> ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \ >>>>>> hexdump -vC /tmp/fitImage | tail -n 3 >>>>>> >>>>>> before: >>>>>> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| >>>>>> 00000270 7a 65 00 00 78 79 64 64 |ze..xydd| >>>>>> ^^ ^^ ^^ >>>>>> after: >>>>>> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| >>>>>> 00000270 7a 65 00 78 79 |ze.xy| >>>>>> >>>>>> Signed-off-by: Marek Vasut marex@denx.de >>>>>> Reviewed-by: Simon Glass sjg@chromium.org >>>>>> Cc: Heinrich Schuchardt xypron.glpk@gmx.de >>>>>> Cc: Tom Rini trini@konsulko.com >>>>> >>>>> Applied to u-boot/master, thanks! >>>>> >>>> >>>> This breaks booting on my board (am3352, eMMC boot, FIT u-boot, >>>> CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it >>>> from eMMC I get nothing at all on the console, if I boot over ymodem >>>> it stalls at 420k, before continuing to 460k. My guess is there's some >>>> error going to the console at the 420k mark, but obviously it's lost >>>> in the ymodem... I have two DTBs in the FIT image, 420k would about >>>> align to the point between them. >>> >>> My bet would be on some padding / unaligned access problem that this >>> patch uncovered. Can you take a look ? >> >> Seems plausible. With this change my external data starts at 0x483 and >> everything after it is non-aligned: > > Should the beginning of external data be aligned ?
If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does the problem go away? If so, that's not a fix outright, it means we need to dig back in to the libfdt thread and find the "make this work without killing performance everywhere all the time" option.
If it is a device tree, it must be 32-bit aligned.
This commit actually breaks my board too (which I was just about to send upstream, but realized it was broken).
Said board uses SPL and main U-Boot. SPL runs fine and main u-boot doesn't output anything. The only difference which I found is that fit-dtb.blob is 2 bytes shorter. And the content is shifted by one byte although data-offset is the same. Strange. In the non-working case, the inner FDT magic isn't 4 byte aligned.
You can find the two fit-dtb.blobs here:
https://walle.cc/u-boot/fit-dtb.blob.working https://walle.cc/u-boot/fit-dtb.blob.non-working
Reverting e8c2d25845c72c7202a628a97d45e31beea40668 doesn't help (I might reverted it the wrong way, there is actually a conflict).
I'll dig deeper into that tomorrow, but maybe you have some pointers where to look.
For reference you can find the current patch here: https://github.com/mwalle/u-boot/tree/sl28-upstream
I think we have a few things to fix here. Marek's patch is breaking things and needs to be reverted. But it's showing a few underlying problems that need to be fixed too:
- fit_extract_data() needs to use calloc() not malloc() so that we don't leak random data.
- We need to 8-byte alignment on the external data. That's the requirement for Linux for device trees on both 32 and 64bit arm. Atish, does RISC-V require more than that? I don't see it in Linux's Documentation/riscv/boot-image-header.rst (and there's no booting.rst file like arm/arm64).
Why 8-byte alignment ? The external data are copied into the target location, so why do they need to be padded in any way?
The start of the external data needs the alignment, to be clearer.
Why ?
If the external data are used in place, then it's the same problem as arm64 fitImage with fdt_high=-1, which fails because the DT is aligned to 4 bytes and arm64 expects it at 8byte offset.
Except we're talking about cases where we can't relocate the data or it doesn't make any sense to.
Which cases? This really needs to be spelled out.
That said, if you think the answer is that we need to ensure that when we use external data we first align it, please show us how that looks.
I would like to understand the problem space first.
On Wed, May 06, 2020 at 04:33:37PM +0200, Marek Vasut wrote:
On 5/6/20 4:27 PM, Tom Rini wrote:
On Wed, May 06, 2020 at 04:17:35PM +0200, Marek Vasut wrote:
On 5/6/20 3:48 PM, Tom Rini wrote:
On Tue, May 05, 2020 at 11:17:19PM +0200, Michael Walle wrote:
Hi all,
Am 2020-05-05 20:41, schrieb Simon Glass:
Hi Tom,
On Tue, 5 May 2020 at 11:50, Tom Rini trini@konsulko.com wrote: > > On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote: >> On 5/5/20 6:37 PM, Alex Kiernan wrote: >>> On Tue, May 5, 2020 at 2:28 PM Marek Vasut marex@denx.de wrote: >>>> >>>> On 5/5/20 3:22 PM, Alex Kiernan wrote: >>>>> On Mon, May 4, 2020 at 12:28 PM Tom Rini trini@konsulko.com wrote: >>>>>> >>>>>> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote: >>>>>> >>>>>>> There is no reason to tail-pad fitImage with external data to 4-bytes, >>>>>>> while fitImage without external data does not have any such padding and >>>>>>> is often unaligned. DT spec also does not mandate any such padding. >>>>>>> >>>>>>> Moreover, the tail-pad fills the last few bytes with uninitialized data, >>>>>>> which could lead to a potential information leak. >>>>>>> >>>>>>> $ echo -n xy > /tmp/data ; \ >>>>>>> ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \ >>>>>>> hexdump -vC /tmp/fitImage | tail -n 3 >>>>>>> >>>>>>> before: >>>>>>> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| >>>>>>> 00000270 7a 65 00 00 78 79 64 64 |ze..xydd| >>>>>>> ^^ ^^ ^^ >>>>>>> after: >>>>>>> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| >>>>>>> 00000270 7a 65 00 78 79 |ze.xy| >>>>>>> >>>>>>> Signed-off-by: Marek Vasut marex@denx.de >>>>>>> Reviewed-by: Simon Glass sjg@chromium.org >>>>>>> Cc: Heinrich Schuchardt xypron.glpk@gmx.de >>>>>>> Cc: Tom Rini trini@konsulko.com >>>>>> >>>>>> Applied to u-boot/master, thanks! >>>>>> >>>>> >>>>> This breaks booting on my board (am3352, eMMC boot, FIT u-boot, >>>>> CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it >>>>> from eMMC I get nothing at all on the console, if I boot over ymodem >>>>> it stalls at 420k, before continuing to 460k. My guess is there's some >>>>> error going to the console at the 420k mark, but obviously it's lost >>>>> in the ymodem... I have two DTBs in the FIT image, 420k would about >>>>> align to the point between them. >>>> >>>> My bet would be on some padding / unaligned access problem that this >>>> patch uncovered. Can you take a look ? >>> >>> Seems plausible. With this change my external data starts at 0x483 and >>> everything after it is non-aligned: >> >> Should the beginning of external data be aligned ? > > If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does > the > problem go away? If so, that's not a fix outright, it means we need > to > dig back in to the libfdt thread and find the "make this work without > killing performance everywhere all the time" option.
If it is a device tree, it must be 32-bit aligned.
This commit actually breaks my board too (which I was just about to send upstream, but realized it was broken).
Said board uses SPL and main U-Boot. SPL runs fine and main u-boot doesn't output anything. The only difference which I found is that fit-dtb.blob is 2 bytes shorter. And the content is shifted by one byte although data-offset is the same. Strange. In the non-working case, the inner FDT magic isn't 4 byte aligned.
You can find the two fit-dtb.blobs here:
https://walle.cc/u-boot/fit-dtb.blob.working https://walle.cc/u-boot/fit-dtb.blob.non-working
Reverting e8c2d25845c72c7202a628a97d45e31beea40668 doesn't help (I might reverted it the wrong way, there is actually a conflict).
I'll dig deeper into that tomorrow, but maybe you have some pointers where to look.
For reference you can find the current patch here: https://github.com/mwalle/u-boot/tree/sl28-upstream
I think we have a few things to fix here. Marek's patch is breaking things and needs to be reverted. But it's showing a few underlying problems that need to be fixed too:
- fit_extract_data() needs to use calloc() not malloc() so that we don't leak random data.
- We need to 8-byte alignment on the external data. That's the requirement for Linux for device trees on both 32 and 64bit arm. Atish, does RISC-V require more than that? I don't see it in Linux's Documentation/riscv/boot-image-header.rst (and there's no booting.rst file like arm/arm64).
Why 8-byte alignment ? The external data are copied into the target location, so why do they need to be padded in any way?
The start of the external data needs the alignment, to be clearer.
Why ?
Given that things which end up in external data have alignment requirements, we need to align and meet those requirements. And I noted why 8 above.
If the external data are used in place, then it's the same problem as arm64 fitImage with fdt_high=-1, which fails because the DT is aligned to 4 bytes and arm64 expects it at 8byte offset.
Except we're talking about cases where we can't relocate the data or it doesn't make any sense to.
Which cases? This really needs to be spelled out.
That said, if you think the answer is that we need to ensure that when we use external data we first align it, please show us how that looks.
I would like to understand the problem space first.
Then please re-read this thread and come up with an alternative solution to the problem you're trying to solve, thanks!
On 5/6/20 4:37 PM, Tom Rini wrote:
On Wed, May 06, 2020 at 04:33:37PM +0200, Marek Vasut wrote:
On 5/6/20 4:27 PM, Tom Rini wrote:
On Wed, May 06, 2020 at 04:17:35PM +0200, Marek Vasut wrote:
On 5/6/20 3:48 PM, Tom Rini wrote:
On Tue, May 05, 2020 at 11:17:19PM +0200, Michael Walle wrote:
Hi all,
Am 2020-05-05 20:41, schrieb Simon Glass: > Hi Tom, > > On Tue, 5 May 2020 at 11:50, Tom Rini trini@konsulko.com wrote: >> >> On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote: >>> On 5/5/20 6:37 PM, Alex Kiernan wrote: >>>> On Tue, May 5, 2020 at 2:28 PM Marek Vasut marex@denx.de wrote: >>>>> >>>>> On 5/5/20 3:22 PM, Alex Kiernan wrote: >>>>>> On Mon, May 4, 2020 at 12:28 PM Tom Rini trini@konsulko.com wrote: >>>>>>> >>>>>>> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote: >>>>>>> >>>>>>>> There is no reason to tail-pad fitImage with external data to 4-bytes, >>>>>>>> while fitImage without external data does not have any such padding and >>>>>>>> is often unaligned. DT spec also does not mandate any such padding. >>>>>>>> >>>>>>>> Moreover, the tail-pad fills the last few bytes with uninitialized data, >>>>>>>> which could lead to a potential information leak. >>>>>>>> >>>>>>>> $ echo -n xy > /tmp/data ; \ >>>>>>>> ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \ >>>>>>>> hexdump -vC /tmp/fitImage | tail -n 3 >>>>>>>> >>>>>>>> before: >>>>>>>> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| >>>>>>>> 00000270 7a 65 00 00 78 79 64 64 |ze..xydd| >>>>>>>> ^^ ^^ ^^ >>>>>>>> after: >>>>>>>> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| >>>>>>>> 00000270 7a 65 00 78 79 |ze.xy| >>>>>>>> >>>>>>>> Signed-off-by: Marek Vasut marex@denx.de >>>>>>>> Reviewed-by: Simon Glass sjg@chromium.org >>>>>>>> Cc: Heinrich Schuchardt xypron.glpk@gmx.de >>>>>>>> Cc: Tom Rini trini@konsulko.com >>>>>>> >>>>>>> Applied to u-boot/master, thanks! >>>>>>> >>>>>> >>>>>> This breaks booting on my board (am3352, eMMC boot, FIT u-boot, >>>>>> CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it >>>>>> from eMMC I get nothing at all on the console, if I boot over ymodem >>>>>> it stalls at 420k, before continuing to 460k. My guess is there's some >>>>>> error going to the console at the 420k mark, but obviously it's lost >>>>>> in the ymodem... I have two DTBs in the FIT image, 420k would about >>>>>> align to the point between them. >>>>> >>>>> My bet would be on some padding / unaligned access problem that this >>>>> patch uncovered. Can you take a look ? >>>> >>>> Seems plausible. With this change my external data starts at 0x483 and >>>> everything after it is non-aligned: >>> >>> Should the beginning of external data be aligned ? >> >> If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does >> the >> problem go away? If so, that's not a fix outright, it means we need >> to >> dig back in to the libfdt thread and find the "make this work without >> killing performance everywhere all the time" option. > > If it is a device tree, it must be 32-bit aligned.
This commit actually breaks my board too (which I was just about to send upstream, but realized it was broken).
Said board uses SPL and main U-Boot. SPL runs fine and main u-boot doesn't output anything. The only difference which I found is that fit-dtb.blob is 2 bytes shorter. And the content is shifted by one byte although data-offset is the same. Strange. In the non-working case, the inner FDT magic isn't 4 byte aligned.
You can find the two fit-dtb.blobs here:
https://walle.cc/u-boot/fit-dtb.blob.working https://walle.cc/u-boot/fit-dtb.blob.non-working
Reverting e8c2d25845c72c7202a628a97d45e31beea40668 doesn't help (I might reverted it the wrong way, there is actually a conflict).
I'll dig deeper into that tomorrow, but maybe you have some pointers where to look.
For reference you can find the current patch here: https://github.com/mwalle/u-boot/tree/sl28-upstream
I think we have a few things to fix here. Marek's patch is breaking things and needs to be reverted. But it's showing a few underlying problems that need to be fixed too:
- fit_extract_data() needs to use calloc() not malloc() so that we don't leak random data.
- We need to 8-byte alignment on the external data. That's the requirement for Linux for device trees on both 32 and 64bit arm. Atish, does RISC-V require more than that? I don't see it in Linux's Documentation/riscv/boot-image-header.rst (and there's no booting.rst file like arm/arm64).
Why 8-byte alignment ? The external data are copied into the target location, so why do they need to be padded in any way?
The start of the external data needs the alignment, to be clearer.
Why ?
Given that things which end up in external data have alignment requirements, we need to align and meet those requirements. And I noted why 8 above.
If you end up with external data, then you need to move those blobs into their target location anyway. That's what you specify in the load = <> property in the .its .
If the external data are used in place, then it's the same problem as arm64 fitImage with fdt_high=-1, which fails because the DT is aligned to 4 bytes and arm64 expects it at 8byte offset.
Except we're talking about cases where we can't relocate the data or it doesn't make any sense to.
Which cases? This really needs to be spelled out.
That said, if you think the answer is that we need to ensure that when we use external data we first align it, please show us how that looks.
I would like to understand the problem space first.
Then please re-read this thread and come up with an alternative solution to the problem you're trying to solve, thanks!
Thank you for the detailed explanation.
On Wed, May 6, 2020 at 3:41 PM Marek Vasut marex@denx.de wrote:
On 5/6/20 4:37 PM, Tom Rini wrote:
On Wed, May 06, 2020 at 04:33:37PM +0200, Marek Vasut wrote:
On 5/6/20 4:27 PM, Tom Rini wrote:
On Wed, May 06, 2020 at 04:17:35PM +0200, Marek Vasut wrote:
On 5/6/20 3:48 PM, Tom Rini wrote:
On Tue, May 05, 2020 at 11:17:19PM +0200, Michael Walle wrote: > Hi all, > > Am 2020-05-05 20:41, schrieb Simon Glass: >> Hi Tom, >> >> On Tue, 5 May 2020 at 11:50, Tom Rini trini@konsulko.com wrote: >>> >>> On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote: >>>> On 5/5/20 6:37 PM, Alex Kiernan wrote: >>>>> On Tue, May 5, 2020 at 2:28 PM Marek Vasut marex@denx.de wrote: >>>>>> >>>>>> On 5/5/20 3:22 PM, Alex Kiernan wrote: >>>>>>> On Mon, May 4, 2020 at 12:28 PM Tom Rini trini@konsulko.com wrote: >>>>>>>> >>>>>>>> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote: >>>>>>>> >>>>>>>>> There is no reason to tail-pad fitImage with external data to 4-bytes, >>>>>>>>> while fitImage without external data does not have any such padding and >>>>>>>>> is often unaligned. DT spec also does not mandate any such padding. >>>>>>>>> >>>>>>>>> Moreover, the tail-pad fills the last few bytes with uninitialized data, >>>>>>>>> which could lead to a potential information leak. >>>>>>>>> >>>>>>>>> $ echo -n xy > /tmp/data ; \ >>>>>>>>> ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \ >>>>>>>>> hexdump -vC /tmp/fitImage | tail -n 3 >>>>>>>>> >>>>>>>>> before: >>>>>>>>> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| >>>>>>>>> 00000270 7a 65 00 00 78 79 64 64 |ze..xydd| >>>>>>>>> ^^ ^^ ^^ >>>>>>>>> after: >>>>>>>>> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| >>>>>>>>> 00000270 7a 65 00 78 79 |ze.xy| >>>>>>>>> >>>>>>>>> Signed-off-by: Marek Vasut marex@denx.de >>>>>>>>> Reviewed-by: Simon Glass sjg@chromium.org >>>>>>>>> Cc: Heinrich Schuchardt xypron.glpk@gmx.de >>>>>>>>> Cc: Tom Rini trini@konsulko.com >>>>>>>> >>>>>>>> Applied to u-boot/master, thanks! >>>>>>>> >>>>>>> >>>>>>> This breaks booting on my board (am3352, eMMC boot, FIT u-boot, >>>>>>> CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it >>>>>>> from eMMC I get nothing at all on the console, if I boot over ymodem >>>>>>> it stalls at 420k, before continuing to 460k. My guess is there's some >>>>>>> error going to the console at the 420k mark, but obviously it's lost >>>>>>> in the ymodem... I have two DTBs in the FIT image, 420k would about >>>>>>> align to the point between them. >>>>>> >>>>>> My bet would be on some padding / unaligned access problem that this >>>>>> patch uncovered. Can you take a look ? >>>>> >>>>> Seems plausible. With this change my external data starts at 0x483 and >>>>> everything after it is non-aligned: >>>> >>>> Should the beginning of external data be aligned ? >>> >>> If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does >>> the >>> problem go away? If so, that's not a fix outright, it means we need >>> to >>> dig back in to the libfdt thread and find the "make this work without >>> killing performance everywhere all the time" option. >> >> If it is a device tree, it must be 32-bit aligned. > > This commit actually breaks my board too (which I was just about to send > upstream, but realized it was broken). > > Said board uses SPL and main U-Boot. SPL runs fine and main u-boot doesn't > output anything. The only difference which I found is that fit-dtb.blob is > 2 bytes shorter. And the content is shifted by one byte although > data-offset is the same. Strange. In the non-working case, the inner > FDT magic isn't 4 byte aligned. > > You can find the two fit-dtb.blobs here: > > https://walle.cc/u-boot/fit-dtb.blob.working > https://walle.cc/u-boot/fit-dtb.blob.non-working > > > Reverting e8c2d25845c72c7202a628a97d45e31beea40668 doesn't help (I might > reverted it the wrong way, there is actually a conflict). > > I'll dig deeper into that tomorrow, but maybe you have some pointers where > to look. > > For reference you can find the current patch here: > https://github.com/mwalle/u-boot/tree/sl28-upstream
I think we have a few things to fix here. Marek's patch is breaking things and needs to be reverted. But it's showing a few underlying problems that need to be fixed too:
- fit_extract_data() needs to use calloc() not malloc() so that we don't leak random data.
- We need to 8-byte alignment on the external data. That's the requirement for Linux for device trees on both 32 and 64bit arm. Atish, does RISC-V require more than that? I don't see it in Linux's Documentation/riscv/boot-image-header.rst (and there's no booting.rst file like arm/arm64).
Why 8-byte alignment ? The external data are copied into the target location, so why do they need to be padded in any way?
The start of the external data needs the alignment, to be clearer.
Why ?
Given that things which end up in external data have alignment requirements, we need to align and meet those requirements. And I noted why 8 above.
If you end up with external data, then you need to move those blobs into their target location anyway. That's what you specify in the load = <> property in the .its .
Just reading common/spl/spl_fit.c, I think that'll try and parse in situ, rather than relocating it?
-- Alex Kiernan
On 5/6/20 5:43 PM, Alex Kiernan wrote:
On Wed, May 6, 2020 at 3:41 PM Marek Vasut marex@denx.de wrote:
On 5/6/20 4:37 PM, Tom Rini wrote:
On Wed, May 06, 2020 at 04:33:37PM +0200, Marek Vasut wrote:
On 5/6/20 4:27 PM, Tom Rini wrote:
On Wed, May 06, 2020 at 04:17:35PM +0200, Marek Vasut wrote:
On 5/6/20 3:48 PM, Tom Rini wrote: > On Tue, May 05, 2020 at 11:17:19PM +0200, Michael Walle wrote: >> Hi all, >> >> Am 2020-05-05 20:41, schrieb Simon Glass: >>> Hi Tom, >>> >>> On Tue, 5 May 2020 at 11:50, Tom Rini trini@konsulko.com wrote: >>>> >>>> On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote: >>>>> On 5/5/20 6:37 PM, Alex Kiernan wrote: >>>>>> On Tue, May 5, 2020 at 2:28 PM Marek Vasut marex@denx.de wrote: >>>>>>> >>>>>>> On 5/5/20 3:22 PM, Alex Kiernan wrote: >>>>>>>> On Mon, May 4, 2020 at 12:28 PM Tom Rini trini@konsulko.com wrote: >>>>>>>>> >>>>>>>>> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote: >>>>>>>>> >>>>>>>>>> There is no reason to tail-pad fitImage with external data to 4-bytes, >>>>>>>>>> while fitImage without external data does not have any such padding and >>>>>>>>>> is often unaligned. DT spec also does not mandate any such padding. >>>>>>>>>> >>>>>>>>>> Moreover, the tail-pad fills the last few bytes with uninitialized data, >>>>>>>>>> which could lead to a potential information leak. >>>>>>>>>> >>>>>>>>>> $ echo -n xy > /tmp/data ; \ >>>>>>>>>> ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \ >>>>>>>>>> hexdump -vC /tmp/fitImage | tail -n 3 >>>>>>>>>> >>>>>>>>>> before: >>>>>>>>>> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| >>>>>>>>>> 00000270 7a 65 00 00 78 79 64 64 |ze..xydd| >>>>>>>>>> ^^ ^^ ^^ >>>>>>>>>> after: >>>>>>>>>> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| >>>>>>>>>> 00000270 7a 65 00 78 79 |ze.xy| >>>>>>>>>> >>>>>>>>>> Signed-off-by: Marek Vasut marex@denx.de >>>>>>>>>> Reviewed-by: Simon Glass sjg@chromium.org >>>>>>>>>> Cc: Heinrich Schuchardt xypron.glpk@gmx.de >>>>>>>>>> Cc: Tom Rini trini@konsulko.com >>>>>>>>> >>>>>>>>> Applied to u-boot/master, thanks! >>>>>>>>> >>>>>>>> >>>>>>>> This breaks booting on my board (am3352, eMMC boot, FIT u-boot, >>>>>>>> CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it >>>>>>>> from eMMC I get nothing at all on the console, if I boot over ymodem >>>>>>>> it stalls at 420k, before continuing to 460k. My guess is there's some >>>>>>>> error going to the console at the 420k mark, but obviously it's lost >>>>>>>> in the ymodem... I have two DTBs in the FIT image, 420k would about >>>>>>>> align to the point between them. >>>>>>> >>>>>>> My bet would be on some padding / unaligned access problem that this >>>>>>> patch uncovered. Can you take a look ? >>>>>> >>>>>> Seems plausible. With this change my external data starts at 0x483 and >>>>>> everything after it is non-aligned: >>>>> >>>>> Should the beginning of external data be aligned ? >>>> >>>> If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does >>>> the >>>> problem go away? If so, that's not a fix outright, it means we need >>>> to >>>> dig back in to the libfdt thread and find the "make this work without >>>> killing performance everywhere all the time" option. >>> >>> If it is a device tree, it must be 32-bit aligned. >> >> This commit actually breaks my board too (which I was just about to send >> upstream, but realized it was broken). >> >> Said board uses SPL and main U-Boot. SPL runs fine and main u-boot doesn't >> output anything. The only difference which I found is that fit-dtb.blob is >> 2 bytes shorter. And the content is shifted by one byte although >> data-offset is the same. Strange. In the non-working case, the inner >> FDT magic isn't 4 byte aligned. >> >> You can find the two fit-dtb.blobs here: >> >> https://walle.cc/u-boot/fit-dtb.blob.working >> https://walle.cc/u-boot/fit-dtb.blob.non-working >> >> >> Reverting e8c2d25845c72c7202a628a97d45e31beea40668 doesn't help (I might >> reverted it the wrong way, there is actually a conflict). >> >> I'll dig deeper into that tomorrow, but maybe you have some pointers where >> to look. >> >> For reference you can find the current patch here: >> https://github.com/mwalle/u-boot/tree/sl28-upstream > > I think we have a few things to fix here. Marek's patch is breaking > things and needs to be reverted. But it's showing a few underlying > problems that need to be fixed too: > - fit_extract_data() needs to use calloc() not malloc() so that we don't > leak random data. > - We need to 8-byte alignment on the external data. That's the > requirement for Linux for device trees on both 32 and 64bit arm. > Atish, does RISC-V require more than that? I don't see it in Linux's > Documentation/riscv/boot-image-header.rst (and there's no booting.rst > file like arm/arm64).
Why 8-byte alignment ? The external data are copied into the target location, so why do they need to be padded in any way?
The start of the external data needs the alignment, to be clearer.
Why ?
Given that things which end up in external data have alignment requirements, we need to align and meet those requirements. And I noted why 8 above.
If you end up with external data, then you need to move those blobs into their target location anyway. That's what you specify in the load = <> property in the .its .
Just reading common/spl/spl_fit.c, I think that'll try and parse in situ, rather than relocating it?
And is that correct or is that the same problem as we have on arm64 with fitImage and fdt_high=-1 ? I think it's the later.
On Wed, May 06, 2020 at 05:52:45PM +0200, Marek Vasut wrote:
On 5/6/20 5:43 PM, Alex Kiernan wrote:
On Wed, May 6, 2020 at 3:41 PM Marek Vasut marex@denx.de wrote:
On 5/6/20 4:37 PM, Tom Rini wrote:
On Wed, May 06, 2020 at 04:33:37PM +0200, Marek Vasut wrote:
On 5/6/20 4:27 PM, Tom Rini wrote:
On Wed, May 06, 2020 at 04:17:35PM +0200, Marek Vasut wrote: > On 5/6/20 3:48 PM, Tom Rini wrote: >> On Tue, May 05, 2020 at 11:17:19PM +0200, Michael Walle wrote: >>> Hi all, >>> >>> Am 2020-05-05 20:41, schrieb Simon Glass: >>>> Hi Tom, >>>> >>>> On Tue, 5 May 2020 at 11:50, Tom Rini trini@konsulko.com wrote: >>>>> >>>>> On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote: >>>>>> On 5/5/20 6:37 PM, Alex Kiernan wrote: >>>>>>> On Tue, May 5, 2020 at 2:28 PM Marek Vasut marex@denx.de wrote: >>>>>>>> >>>>>>>> On 5/5/20 3:22 PM, Alex Kiernan wrote: >>>>>>>>> On Mon, May 4, 2020 at 12:28 PM Tom Rini trini@konsulko.com wrote: >>>>>>>>>> >>>>>>>>>> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote: >>>>>>>>>> >>>>>>>>>>> There is no reason to tail-pad fitImage with external data to 4-bytes, >>>>>>>>>>> while fitImage without external data does not have any such padding and >>>>>>>>>>> is often unaligned. DT spec also does not mandate any such padding. >>>>>>>>>>> >>>>>>>>>>> Moreover, the tail-pad fills the last few bytes with uninitialized data, >>>>>>>>>>> which could lead to a potential information leak. >>>>>>>>>>> >>>>>>>>>>> $ echo -n xy > /tmp/data ; \ >>>>>>>>>>> ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \ >>>>>>>>>>> hexdump -vC /tmp/fitImage | tail -n 3 >>>>>>>>>>> >>>>>>>>>>> before: >>>>>>>>>>> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| >>>>>>>>>>> 00000270 7a 65 00 00 78 79 64 64 |ze..xydd| >>>>>>>>>>> ^^ ^^ ^^ >>>>>>>>>>> after: >>>>>>>>>>> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| >>>>>>>>>>> 00000270 7a 65 00 78 79 |ze.xy| >>>>>>>>>>> >>>>>>>>>>> Signed-off-by: Marek Vasut marex@denx.de >>>>>>>>>>> Reviewed-by: Simon Glass sjg@chromium.org >>>>>>>>>>> Cc: Heinrich Schuchardt xypron.glpk@gmx.de >>>>>>>>>>> Cc: Tom Rini trini@konsulko.com >>>>>>>>>> >>>>>>>>>> Applied to u-boot/master, thanks! >>>>>>>>>> >>>>>>>>> >>>>>>>>> This breaks booting on my board (am3352, eMMC boot, FIT u-boot, >>>>>>>>> CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it >>>>>>>>> from eMMC I get nothing at all on the console, if I boot over ymodem >>>>>>>>> it stalls at 420k, before continuing to 460k. My guess is there's some >>>>>>>>> error going to the console at the 420k mark, but obviously it's lost >>>>>>>>> in the ymodem... I have two DTBs in the FIT image, 420k would about >>>>>>>>> align to the point between them. >>>>>>>> >>>>>>>> My bet would be on some padding / unaligned access problem that this >>>>>>>> patch uncovered. Can you take a look ? >>>>>>> >>>>>>> Seems plausible. With this change my external data starts at 0x483 and >>>>>>> everything after it is non-aligned: >>>>>> >>>>>> Should the beginning of external data be aligned ? >>>>> >>>>> If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does >>>>> the >>>>> problem go away? If so, that's not a fix outright, it means we need >>>>> to >>>>> dig back in to the libfdt thread and find the "make this work without >>>>> killing performance everywhere all the time" option. >>>> >>>> If it is a device tree, it must be 32-bit aligned. >>> >>> This commit actually breaks my board too (which I was just about to send >>> upstream, but realized it was broken). >>> >>> Said board uses SPL and main U-Boot. SPL runs fine and main u-boot doesn't >>> output anything. The only difference which I found is that fit-dtb.blob is >>> 2 bytes shorter. And the content is shifted by one byte although >>> data-offset is the same. Strange. In the non-working case, the inner >>> FDT magic isn't 4 byte aligned. >>> >>> You can find the two fit-dtb.blobs here: >>> >>> https://walle.cc/u-boot/fit-dtb.blob.working >>> https://walle.cc/u-boot/fit-dtb.blob.non-working >>> >>> >>> Reverting e8c2d25845c72c7202a628a97d45e31beea40668 doesn't help (I might >>> reverted it the wrong way, there is actually a conflict). >>> >>> I'll dig deeper into that tomorrow, but maybe you have some pointers where >>> to look. >>> >>> For reference you can find the current patch here: >>> https://github.com/mwalle/u-boot/tree/sl28-upstream >> >> I think we have a few things to fix here. Marek's patch is breaking >> things and needs to be reverted. But it's showing a few underlying >> problems that need to be fixed too: >> - fit_extract_data() needs to use calloc() not malloc() so that we don't >> leak random data. >> - We need to 8-byte alignment on the external data. That's the >> requirement for Linux for device trees on both 32 and 64bit arm. >> Atish, does RISC-V require more than that? I don't see it in Linux's >> Documentation/riscv/boot-image-header.rst (and there's no booting.rst >> file like arm/arm64). > > Why 8-byte alignment ? The external data are copied into the target > location, so why do they need to be padded in any way?
The start of the external data needs the alignment, to be clearer.
Why ?
Given that things which end up in external data have alignment requirements, we need to align and meet those requirements. And I noted why 8 above.
If you end up with external data, then you need to move those blobs into their target location anyway. That's what you specify in the load = <> property in the .its .
Just reading common/spl/spl_fit.c, I think that'll try and parse in situ, rather than relocating it?
And is that correct or is that the same problem as we have on arm64 with fitImage and fdt_high=-1 ? I think it's the later.
I'm not sure that it is. Can we easily/safely memmove the data to be aligned? Is that really a better option in this case than ensuring alignment within the file?
On 5/6/20 6:04 PM, Tom Rini wrote:
On Wed, May 06, 2020 at 05:52:45PM +0200, Marek Vasut wrote:
On 5/6/20 5:43 PM, Alex Kiernan wrote:
On Wed, May 6, 2020 at 3:41 PM Marek Vasut marex@denx.de wrote:
On 5/6/20 4:37 PM, Tom Rini wrote:
On Wed, May 06, 2020 at 04:33:37PM +0200, Marek Vasut wrote:
On 5/6/20 4:27 PM, Tom Rini wrote: > On Wed, May 06, 2020 at 04:17:35PM +0200, Marek Vasut wrote: >> On 5/6/20 3:48 PM, Tom Rini wrote: >>> On Tue, May 05, 2020 at 11:17:19PM +0200, Michael Walle wrote: >>>> Hi all, >>>> >>>> Am 2020-05-05 20:41, schrieb Simon Glass: >>>>> Hi Tom, >>>>> >>>>> On Tue, 5 May 2020 at 11:50, Tom Rini trini@konsulko.com wrote: >>>>>> >>>>>> On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote: >>>>>>> On 5/5/20 6:37 PM, Alex Kiernan wrote: >>>>>>>> On Tue, May 5, 2020 at 2:28 PM Marek Vasut marex@denx.de wrote: >>>>>>>>> >>>>>>>>> On 5/5/20 3:22 PM, Alex Kiernan wrote: >>>>>>>>>> On Mon, May 4, 2020 at 12:28 PM Tom Rini trini@konsulko.com wrote: >>>>>>>>>>> >>>>>>>>>>> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote: >>>>>>>>>>> >>>>>>>>>>>> There is no reason to tail-pad fitImage with external data to 4-bytes, >>>>>>>>>>>> while fitImage without external data does not have any such padding and >>>>>>>>>>>> is often unaligned. DT spec also does not mandate any such padding. >>>>>>>>>>>> >>>>>>>>>>>> Moreover, the tail-pad fills the last few bytes with uninitialized data, >>>>>>>>>>>> which could lead to a potential information leak. >>>>>>>>>>>> >>>>>>>>>>>> $ echo -n xy > /tmp/data ; \ >>>>>>>>>>>> ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \ >>>>>>>>>>>> hexdump -vC /tmp/fitImage | tail -n 3 >>>>>>>>>>>> >>>>>>>>>>>> before: >>>>>>>>>>>> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| >>>>>>>>>>>> 00000270 7a 65 00 00 78 79 64 64 |ze..xydd| >>>>>>>>>>>> ^^ ^^ ^^ >>>>>>>>>>>> after: >>>>>>>>>>>> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| >>>>>>>>>>>> 00000270 7a 65 00 78 79 |ze.xy| >>>>>>>>>>>> >>>>>>>>>>>> Signed-off-by: Marek Vasut marex@denx.de >>>>>>>>>>>> Reviewed-by: Simon Glass sjg@chromium.org >>>>>>>>>>>> Cc: Heinrich Schuchardt xypron.glpk@gmx.de >>>>>>>>>>>> Cc: Tom Rini trini@konsulko.com >>>>>>>>>>> >>>>>>>>>>> Applied to u-boot/master, thanks! >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> This breaks booting on my board (am3352, eMMC boot, FIT u-boot, >>>>>>>>>> CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it >>>>>>>>>> from eMMC I get nothing at all on the console, if I boot over ymodem >>>>>>>>>> it stalls at 420k, before continuing to 460k. My guess is there's some >>>>>>>>>> error going to the console at the 420k mark, but obviously it's lost >>>>>>>>>> in the ymodem... I have two DTBs in the FIT image, 420k would about >>>>>>>>>> align to the point between them. >>>>>>>>> >>>>>>>>> My bet would be on some padding / unaligned access problem that this >>>>>>>>> patch uncovered. Can you take a look ? >>>>>>>> >>>>>>>> Seems plausible. With this change my external data starts at 0x483 and >>>>>>>> everything after it is non-aligned: >>>>>>> >>>>>>> Should the beginning of external data be aligned ? >>>>>> >>>>>> If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does >>>>>> the >>>>>> problem go away? If so, that's not a fix outright, it means we need >>>>>> to >>>>>> dig back in to the libfdt thread and find the "make this work without >>>>>> killing performance everywhere all the time" option. >>>>> >>>>> If it is a device tree, it must be 32-bit aligned. >>>> >>>> This commit actually breaks my board too (which I was just about to send >>>> upstream, but realized it was broken). >>>> >>>> Said board uses SPL and main U-Boot. SPL runs fine and main u-boot doesn't >>>> output anything. The only difference which I found is that fit-dtb.blob is >>>> 2 bytes shorter. And the content is shifted by one byte although >>>> data-offset is the same. Strange. In the non-working case, the inner >>>> FDT magic isn't 4 byte aligned. >>>> >>>> You can find the two fit-dtb.blobs here: >>>> >>>> https://walle.cc/u-boot/fit-dtb.blob.working >>>> https://walle.cc/u-boot/fit-dtb.blob.non-working >>>> >>>> >>>> Reverting e8c2d25845c72c7202a628a97d45e31beea40668 doesn't help (I might >>>> reverted it the wrong way, there is actually a conflict). >>>> >>>> I'll dig deeper into that tomorrow, but maybe you have some pointers where >>>> to look. >>>> >>>> For reference you can find the current patch here: >>>> https://github.com/mwalle/u-boot/tree/sl28-upstream >>> >>> I think we have a few things to fix here. Marek's patch is breaking >>> things and needs to be reverted. But it's showing a few underlying >>> problems that need to be fixed too: >>> - fit_extract_data() needs to use calloc() not malloc() so that we don't >>> leak random data. >>> - We need to 8-byte alignment on the external data. That's the >>> requirement for Linux for device trees on both 32 and 64bit arm. >>> Atish, does RISC-V require more than that? I don't see it in Linux's >>> Documentation/riscv/boot-image-header.rst (and there's no booting.rst >>> file like arm/arm64). >> >> Why 8-byte alignment ? The external data are copied into the target >> location, so why do they need to be padded in any way? > > The start of the external data needs the alignment, to be clearer.
Why ?
Given that things which end up in external data have alignment requirements, we need to align and meet those requirements. And I noted why 8 above.
If you end up with external data, then you need to move those blobs into their target location anyway. That's what you specify in the load = <> property in the .its .
Just reading common/spl/spl_fit.c, I think that'll try and parse in situ, rather than relocating it?
And is that correct or is that the same problem as we have on arm64 with fitImage and fdt_high=-1 ? I think it's the later.
I'm not sure that it is. Can we easily/safely memmove the data to be aligned? Is that really a better option in this case than ensuring alignment within the file?
Can't we use the new mkimage -B option to enforce the alignment IFF and only IFF it is required ? Then we can enforce it separately for 32bit and 64bit platforms to 4 and 8 bytes respectively even.
On Wed, May 06, 2020 at 06:17:47PM +0200, Marek Vasut wrote:
On 5/6/20 6:04 PM, Tom Rini wrote:
On Wed, May 06, 2020 at 05:52:45PM +0200, Marek Vasut wrote:
On 5/6/20 5:43 PM, Alex Kiernan wrote:
On Wed, May 6, 2020 at 3:41 PM Marek Vasut marex@denx.de wrote:
On 5/6/20 4:37 PM, Tom Rini wrote:
On Wed, May 06, 2020 at 04:33:37PM +0200, Marek Vasut wrote: > On 5/6/20 4:27 PM, Tom Rini wrote: >> On Wed, May 06, 2020 at 04:17:35PM +0200, Marek Vasut wrote: >>> On 5/6/20 3:48 PM, Tom Rini wrote: >>>> On Tue, May 05, 2020 at 11:17:19PM +0200, Michael Walle wrote: >>>>> Hi all, >>>>> >>>>> Am 2020-05-05 20:41, schrieb Simon Glass: >>>>>> Hi Tom, >>>>>> >>>>>> On Tue, 5 May 2020 at 11:50, Tom Rini trini@konsulko.com wrote: >>>>>>> >>>>>>> On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote: >>>>>>>> On 5/5/20 6:37 PM, Alex Kiernan wrote: >>>>>>>>> On Tue, May 5, 2020 at 2:28 PM Marek Vasut marex@denx.de wrote: >>>>>>>>>> >>>>>>>>>> On 5/5/20 3:22 PM, Alex Kiernan wrote: >>>>>>>>>>> On Mon, May 4, 2020 at 12:28 PM Tom Rini trini@konsulko.com wrote: >>>>>>>>>>>> >>>>>>>>>>>> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote: >>>>>>>>>>>> >>>>>>>>>>>>> There is no reason to tail-pad fitImage with external data to 4-bytes, >>>>>>>>>>>>> while fitImage without external data does not have any such padding and >>>>>>>>>>>>> is often unaligned. DT spec also does not mandate any such padding. >>>>>>>>>>>>> >>>>>>>>>>>>> Moreover, the tail-pad fills the last few bytes with uninitialized data, >>>>>>>>>>>>> which could lead to a potential information leak. >>>>>>>>>>>>> >>>>>>>>>>>>> $ echo -n xy > /tmp/data ; \ >>>>>>>>>>>>> ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \ >>>>>>>>>>>>> hexdump -vC /tmp/fitImage | tail -n 3 >>>>>>>>>>>>> >>>>>>>>>>>>> before: >>>>>>>>>>>>> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| >>>>>>>>>>>>> 00000270 7a 65 00 00 78 79 64 64 |ze..xydd| >>>>>>>>>>>>> ^^ ^^ ^^ >>>>>>>>>>>>> after: >>>>>>>>>>>>> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| >>>>>>>>>>>>> 00000270 7a 65 00 78 79 |ze.xy| >>>>>>>>>>>>> >>>>>>>>>>>>> Signed-off-by: Marek Vasut marex@denx.de >>>>>>>>>>>>> Reviewed-by: Simon Glass sjg@chromium.org >>>>>>>>>>>>> Cc: Heinrich Schuchardt xypron.glpk@gmx.de >>>>>>>>>>>>> Cc: Tom Rini trini@konsulko.com >>>>>>>>>>>> >>>>>>>>>>>> Applied to u-boot/master, thanks! >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> This breaks booting on my board (am3352, eMMC boot, FIT u-boot, >>>>>>>>>>> CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it >>>>>>>>>>> from eMMC I get nothing at all on the console, if I boot over ymodem >>>>>>>>>>> it stalls at 420k, before continuing to 460k. My guess is there's some >>>>>>>>>>> error going to the console at the 420k mark, but obviously it's lost >>>>>>>>>>> in the ymodem... I have two DTBs in the FIT image, 420k would about >>>>>>>>>>> align to the point between them. >>>>>>>>>> >>>>>>>>>> My bet would be on some padding / unaligned access problem that this >>>>>>>>>> patch uncovered. Can you take a look ? >>>>>>>>> >>>>>>>>> Seems plausible. With this change my external data starts at 0x483 and >>>>>>>>> everything after it is non-aligned: >>>>>>>> >>>>>>>> Should the beginning of external data be aligned ? >>>>>>> >>>>>>> If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does >>>>>>> the >>>>>>> problem go away? If so, that's not a fix outright, it means we need >>>>>>> to >>>>>>> dig back in to the libfdt thread and find the "make this work without >>>>>>> killing performance everywhere all the time" option. >>>>>> >>>>>> If it is a device tree, it must be 32-bit aligned. >>>>> >>>>> This commit actually breaks my board too (which I was just about to send >>>>> upstream, but realized it was broken). >>>>> >>>>> Said board uses SPL and main U-Boot. SPL runs fine and main u-boot doesn't >>>>> output anything. The only difference which I found is that fit-dtb.blob is >>>>> 2 bytes shorter. And the content is shifted by one byte although >>>>> data-offset is the same. Strange. In the non-working case, the inner >>>>> FDT magic isn't 4 byte aligned. >>>>> >>>>> You can find the two fit-dtb.blobs here: >>>>> >>>>> https://walle.cc/u-boot/fit-dtb.blob.working >>>>> https://walle.cc/u-boot/fit-dtb.blob.non-working >>>>> >>>>> >>>>> Reverting e8c2d25845c72c7202a628a97d45e31beea40668 doesn't help (I might >>>>> reverted it the wrong way, there is actually a conflict). >>>>> >>>>> I'll dig deeper into that tomorrow, but maybe you have some pointers where >>>>> to look. >>>>> >>>>> For reference you can find the current patch here: >>>>> https://github.com/mwalle/u-boot/tree/sl28-upstream >>>> >>>> I think we have a few things to fix here. Marek's patch is breaking >>>> things and needs to be reverted. But it's showing a few underlying >>>> problems that need to be fixed too: >>>> - fit_extract_data() needs to use calloc() not malloc() so that we don't >>>> leak random data. >>>> - We need to 8-byte alignment on the external data. That's the >>>> requirement for Linux for device trees on both 32 and 64bit arm. >>>> Atish, does RISC-V require more than that? I don't see it in Linux's >>>> Documentation/riscv/boot-image-header.rst (and there's no booting.rst >>>> file like arm/arm64). >>> >>> Why 8-byte alignment ? The external data are copied into the target >>> location, so why do they need to be padded in any way? >> >> The start of the external data needs the alignment, to be clearer. > > Why ?
Given that things which end up in external data have alignment requirements, we need to align and meet those requirements. And I noted why 8 above.
If you end up with external data, then you need to move those blobs into their target location anyway. That's what you specify in the load = <> property in the .its .
Just reading common/spl/spl_fit.c, I think that'll try and parse in situ, rather than relocating it?
And is that correct or is that the same problem as we have on arm64 with fitImage and fdt_high=-1 ? I think it's the later.
I'm not sure that it is. Can we easily/safely memmove the data to be aligned? Is that really a better option in this case than ensuring alignment within the file?
Can't we use the new mkimage -B option to enforce the alignment IFF and only IFF it is required ?
Perhaps. But..
Then we can enforce it separately for 32bit and 64bit platforms to 4 and 8 bytes respectively even.
It's 8 bytes for both. It's possible that Linux doesn't hard fail if you only do 4 byte alignment but the documented requirement is 8, for arm32.
On 5/6/20 6:33 PM, Tom Rini wrote:
On Wed, May 06, 2020 at 06:17:47PM +0200, Marek Vasut wrote:
On 5/6/20 6:04 PM, Tom Rini wrote:
On Wed, May 06, 2020 at 05:52:45PM +0200, Marek Vasut wrote:
On 5/6/20 5:43 PM, Alex Kiernan wrote:
On Wed, May 6, 2020 at 3:41 PM Marek Vasut marex@denx.de wrote:
On 5/6/20 4:37 PM, Tom Rini wrote: > On Wed, May 06, 2020 at 04:33:37PM +0200, Marek Vasut wrote: >> On 5/6/20 4:27 PM, Tom Rini wrote: >>> On Wed, May 06, 2020 at 04:17:35PM +0200, Marek Vasut wrote: >>>> On 5/6/20 3:48 PM, Tom Rini wrote: >>>>> On Tue, May 05, 2020 at 11:17:19PM +0200, Michael Walle wrote: >>>>>> Hi all, >>>>>> >>>>>> Am 2020-05-05 20:41, schrieb Simon Glass: >>>>>>> Hi Tom, >>>>>>> >>>>>>> On Tue, 5 May 2020 at 11:50, Tom Rini trini@konsulko.com wrote: >>>>>>>> >>>>>>>> On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote: >>>>>>>>> On 5/5/20 6:37 PM, Alex Kiernan wrote: >>>>>>>>>> On Tue, May 5, 2020 at 2:28 PM Marek Vasut marex@denx.de wrote: >>>>>>>>>>> >>>>>>>>>>> On 5/5/20 3:22 PM, Alex Kiernan wrote: >>>>>>>>>>>> On Mon, May 4, 2020 at 12:28 PM Tom Rini trini@konsulko.com wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote: >>>>>>>>>>>>> >>>>>>>>>>>>>> There is no reason to tail-pad fitImage with external data to 4-bytes, >>>>>>>>>>>>>> while fitImage without external data does not have any such padding and >>>>>>>>>>>>>> is often unaligned. DT spec also does not mandate any such padding. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Moreover, the tail-pad fills the last few bytes with uninitialized data, >>>>>>>>>>>>>> which could lead to a potential information leak. >>>>>>>>>>>>>> >>>>>>>>>>>>>> $ echo -n xy > /tmp/data ; \ >>>>>>>>>>>>>> ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \ >>>>>>>>>>>>>> hexdump -vC /tmp/fitImage | tail -n 3 >>>>>>>>>>>>>> >>>>>>>>>>>>>> before: >>>>>>>>>>>>>> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| >>>>>>>>>>>>>> 00000270 7a 65 00 00 78 79 64 64 |ze..xydd| >>>>>>>>>>>>>> ^^ ^^ ^^ >>>>>>>>>>>>>> after: >>>>>>>>>>>>>> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| >>>>>>>>>>>>>> 00000270 7a 65 00 78 79 |ze.xy| >>>>>>>>>>>>>> >>>>>>>>>>>>>> Signed-off-by: Marek Vasut marex@denx.de >>>>>>>>>>>>>> Reviewed-by: Simon Glass sjg@chromium.org >>>>>>>>>>>>>> Cc: Heinrich Schuchardt xypron.glpk@gmx.de >>>>>>>>>>>>>> Cc: Tom Rini trini@konsulko.com >>>>>>>>>>>>> >>>>>>>>>>>>> Applied to u-boot/master, thanks! >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> This breaks booting on my board (am3352, eMMC boot, FIT u-boot, >>>>>>>>>>>> CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it >>>>>>>>>>>> from eMMC I get nothing at all on the console, if I boot over ymodem >>>>>>>>>>>> it stalls at 420k, before continuing to 460k. My guess is there's some >>>>>>>>>>>> error going to the console at the 420k mark, but obviously it's lost >>>>>>>>>>>> in the ymodem... I have two DTBs in the FIT image, 420k would about >>>>>>>>>>>> align to the point between them. >>>>>>>>>>> >>>>>>>>>>> My bet would be on some padding / unaligned access problem that this >>>>>>>>>>> patch uncovered. Can you take a look ? >>>>>>>>>> >>>>>>>>>> Seems plausible. With this change my external data starts at 0x483 and >>>>>>>>>> everything after it is non-aligned: >>>>>>>>> >>>>>>>>> Should the beginning of external data be aligned ? >>>>>>>> >>>>>>>> If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does >>>>>>>> the >>>>>>>> problem go away? If so, that's not a fix outright, it means we need >>>>>>>> to >>>>>>>> dig back in to the libfdt thread and find the "make this work without >>>>>>>> killing performance everywhere all the time" option. >>>>>>> >>>>>>> If it is a device tree, it must be 32-bit aligned. >>>>>> >>>>>> This commit actually breaks my board too (which I was just about to send >>>>>> upstream, but realized it was broken). >>>>>> >>>>>> Said board uses SPL and main U-Boot. SPL runs fine and main u-boot doesn't >>>>>> output anything. The only difference which I found is that fit-dtb.blob is >>>>>> 2 bytes shorter. And the content is shifted by one byte although >>>>>> data-offset is the same. Strange. In the non-working case, the inner >>>>>> FDT magic isn't 4 byte aligned. >>>>>> >>>>>> You can find the two fit-dtb.blobs here: >>>>>> >>>>>> https://walle.cc/u-boot/fit-dtb.blob.working >>>>>> https://walle.cc/u-boot/fit-dtb.blob.non-working >>>>>> >>>>>> >>>>>> Reverting e8c2d25845c72c7202a628a97d45e31beea40668 doesn't help (I might >>>>>> reverted it the wrong way, there is actually a conflict). >>>>>> >>>>>> I'll dig deeper into that tomorrow, but maybe you have some pointers where >>>>>> to look. >>>>>> >>>>>> For reference you can find the current patch here: >>>>>> https://github.com/mwalle/u-boot/tree/sl28-upstream >>>>> >>>>> I think we have a few things to fix here. Marek's patch is breaking >>>>> things and needs to be reverted. But it's showing a few underlying >>>>> problems that need to be fixed too: >>>>> - fit_extract_data() needs to use calloc() not malloc() so that we don't >>>>> leak random data. >>>>> - We need to 8-byte alignment on the external data. That's the >>>>> requirement for Linux for device trees on both 32 and 64bit arm. >>>>> Atish, does RISC-V require more than that? I don't see it in Linux's >>>>> Documentation/riscv/boot-image-header.rst (and there's no booting.rst >>>>> file like arm/arm64). >>>> >>>> Why 8-byte alignment ? The external data are copied into the target >>>> location, so why do they need to be padded in any way? >>> >>> The start of the external data needs the alignment, to be clearer. >> >> Why ? > > Given that things which end up in external data have alignment > requirements, we need to align and meet those requirements. And I noted > why 8 above.
If you end up with external data, then you need to move those blobs into their target location anyway. That's what you specify in the load = <> property in the .its .
Just reading common/spl/spl_fit.c, I think that'll try and parse in situ, rather than relocating it?
And is that correct or is that the same problem as we have on arm64 with fitImage and fdt_high=-1 ? I think it's the later.
I'm not sure that it is. Can we easily/safely memmove the data to be aligned? Is that really a better option in this case than ensuring alignment within the file?
Can't we use the new mkimage -B option to enforce the alignment IFF and only IFF it is required ?
Perhaps. But..
Then we can enforce it separately for 32bit and 64bit platforms to 4 and 8 bytes respectively even.
It's 8 bytes for both. It's possible that Linux doesn't hard fail if you only do 4 byte alignment but the documented requirement is 8, for arm32.
With Linux you usually need to move the kernel anyway, no ? It's 2 MiB for arm64 for example.
And what you usually parse in-place would be the DT then.
On Wed, May 06, 2020 at 06:35:52PM +0200, Marek Vasut wrote:
On 5/6/20 6:33 PM, Tom Rini wrote:
On Wed, May 06, 2020 at 06:17:47PM +0200, Marek Vasut wrote:
On 5/6/20 6:04 PM, Tom Rini wrote:
On Wed, May 06, 2020 at 05:52:45PM +0200, Marek Vasut wrote:
On 5/6/20 5:43 PM, Alex Kiernan wrote:
On Wed, May 6, 2020 at 3:41 PM Marek Vasut marex@denx.de wrote: > > On 5/6/20 4:37 PM, Tom Rini wrote: >> On Wed, May 06, 2020 at 04:33:37PM +0200, Marek Vasut wrote: >>> On 5/6/20 4:27 PM, Tom Rini wrote: >>>> On Wed, May 06, 2020 at 04:17:35PM +0200, Marek Vasut wrote: >>>>> On 5/6/20 3:48 PM, Tom Rini wrote: >>>>>> On Tue, May 05, 2020 at 11:17:19PM +0200, Michael Walle wrote: >>>>>>> Hi all, >>>>>>> >>>>>>> Am 2020-05-05 20:41, schrieb Simon Glass: >>>>>>>> Hi Tom, >>>>>>>> >>>>>>>> On Tue, 5 May 2020 at 11:50, Tom Rini trini@konsulko.com wrote: >>>>>>>>> >>>>>>>>> On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote: >>>>>>>>>> On 5/5/20 6:37 PM, Alex Kiernan wrote: >>>>>>>>>>> On Tue, May 5, 2020 at 2:28 PM Marek Vasut marex@denx.de wrote: >>>>>>>>>>>> >>>>>>>>>>>> On 5/5/20 3:22 PM, Alex Kiernan wrote: >>>>>>>>>>>>> On Mon, May 4, 2020 at 12:28 PM Tom Rini trini@konsulko.com wrote: >>>>>>>>>>>>>> >>>>>>>>>>>>>> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote: >>>>>>>>>>>>>> >>>>>>>>>>>>>>> There is no reason to tail-pad fitImage with external data to 4-bytes, >>>>>>>>>>>>>>> while fitImage without external data does not have any such padding and >>>>>>>>>>>>>>> is often unaligned. DT spec also does not mandate any such padding. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Moreover, the tail-pad fills the last few bytes with uninitialized data, >>>>>>>>>>>>>>> which could lead to a potential information leak. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> $ echo -n xy > /tmp/data ; \ >>>>>>>>>>>>>>> ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \ >>>>>>>>>>>>>>> hexdump -vC /tmp/fitImage | tail -n 3 >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> before: >>>>>>>>>>>>>>> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| >>>>>>>>>>>>>>> 00000270 7a 65 00 00 78 79 64 64 |ze..xydd| >>>>>>>>>>>>>>> ^^ ^^ ^^ >>>>>>>>>>>>>>> after: >>>>>>>>>>>>>>> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| >>>>>>>>>>>>>>> 00000270 7a 65 00 78 79 |ze.xy| >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Signed-off-by: Marek Vasut marex@denx.de >>>>>>>>>>>>>>> Reviewed-by: Simon Glass sjg@chromium.org >>>>>>>>>>>>>>> Cc: Heinrich Schuchardt xypron.glpk@gmx.de >>>>>>>>>>>>>>> Cc: Tom Rini trini@konsulko.com >>>>>>>>>>>>>> >>>>>>>>>>>>>> Applied to u-boot/master, thanks! >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> This breaks booting on my board (am3352, eMMC boot, FIT u-boot, >>>>>>>>>>>>> CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it >>>>>>>>>>>>> from eMMC I get nothing at all on the console, if I boot over ymodem >>>>>>>>>>>>> it stalls at 420k, before continuing to 460k. My guess is there's some >>>>>>>>>>>>> error going to the console at the 420k mark, but obviously it's lost >>>>>>>>>>>>> in the ymodem... I have two DTBs in the FIT image, 420k would about >>>>>>>>>>>>> align to the point between them. >>>>>>>>>>>> >>>>>>>>>>>> My bet would be on some padding / unaligned access problem that this >>>>>>>>>>>> patch uncovered. Can you take a look ? >>>>>>>>>>> >>>>>>>>>>> Seems plausible. With this change my external data starts at 0x483 and >>>>>>>>>>> everything after it is non-aligned: >>>>>>>>>> >>>>>>>>>> Should the beginning of external data be aligned ? >>>>>>>>> >>>>>>>>> If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does >>>>>>>>> the >>>>>>>>> problem go away? If so, that's not a fix outright, it means we need >>>>>>>>> to >>>>>>>>> dig back in to the libfdt thread and find the "make this work without >>>>>>>>> killing performance everywhere all the time" option. >>>>>>>> >>>>>>>> If it is a device tree, it must be 32-bit aligned. >>>>>>> >>>>>>> This commit actually breaks my board too (which I was just about to send >>>>>>> upstream, but realized it was broken). >>>>>>> >>>>>>> Said board uses SPL and main U-Boot. SPL runs fine and main u-boot doesn't >>>>>>> output anything. The only difference which I found is that fit-dtb.blob is >>>>>>> 2 bytes shorter. And the content is shifted by one byte although >>>>>>> data-offset is the same. Strange. In the non-working case, the inner >>>>>>> FDT magic isn't 4 byte aligned. >>>>>>> >>>>>>> You can find the two fit-dtb.blobs here: >>>>>>> >>>>>>> https://walle.cc/u-boot/fit-dtb.blob.working >>>>>>> https://walle.cc/u-boot/fit-dtb.blob.non-working >>>>>>> >>>>>>> >>>>>>> Reverting e8c2d25845c72c7202a628a97d45e31beea40668 doesn't help (I might >>>>>>> reverted it the wrong way, there is actually a conflict). >>>>>>> >>>>>>> I'll dig deeper into that tomorrow, but maybe you have some pointers where >>>>>>> to look. >>>>>>> >>>>>>> For reference you can find the current patch here: >>>>>>> https://github.com/mwalle/u-boot/tree/sl28-upstream >>>>>> >>>>>> I think we have a few things to fix here. Marek's patch is breaking >>>>>> things and needs to be reverted. But it's showing a few underlying >>>>>> problems that need to be fixed too: >>>>>> - fit_extract_data() needs to use calloc() not malloc() so that we don't >>>>>> leak random data. >>>>>> - We need to 8-byte alignment on the external data. That's the >>>>>> requirement for Linux for device trees on both 32 and 64bit arm. >>>>>> Atish, does RISC-V require more than that? I don't see it in Linux's >>>>>> Documentation/riscv/boot-image-header.rst (and there's no booting.rst >>>>>> file like arm/arm64). >>>>> >>>>> Why 8-byte alignment ? The external data are copied into the target >>>>> location, so why do they need to be padded in any way? >>>> >>>> The start of the external data needs the alignment, to be clearer. >>> >>> Why ? >> >> Given that things which end up in external data have alignment >> requirements, we need to align and meet those requirements. And I noted >> why 8 above. > > If you end up with external data, then you need to move those blobs into > their target location anyway. That's what you specify in the load = <> > property in the .its . >
Just reading common/spl/spl_fit.c, I think that'll try and parse in situ, rather than relocating it?
And is that correct or is that the same problem as we have on arm64 with fitImage and fdt_high=-1 ? I think it's the later.
I'm not sure that it is. Can we easily/safely memmove the data to be aligned? Is that really a better option in this case than ensuring alignment within the file?
Can't we use the new mkimage -B option to enforce the alignment IFF and only IFF it is required ?
Perhaps. But..
Then we can enforce it separately for 32bit and 64bit platforms to 4 and 8 bytes respectively even.
It's 8 bytes for both. It's possible that Linux doesn't hard fail if you only do 4 byte alignment but the documented requirement is 8, for arm32.
With Linux you usually need to move the kernel anyway, no ? It's 2 MiB for arm64 for example.
For arm64 you have to move it to where text_offset says it needs to be. For arm32 the common (always, practically?) case is you're firing off the zImage which does what's needed. But..
And what you usually parse in-place would be the DT then.
Yes, the practical case is that it's a DT and that needs 8 byte alignment. And we should just get back to aligning that correctly. Going back to the v1 thread, it turns out the answer to "why do we even have this padding?" is "we need the DT to be aligned".
On 5/6/20 7:02 PM, Tom Rini wrote:
On Wed, May 06, 2020 at 06:35:52PM +0200, Marek Vasut wrote:
On 5/6/20 6:33 PM, Tom Rini wrote:
On Wed, May 06, 2020 at 06:17:47PM +0200, Marek Vasut wrote:
On 5/6/20 6:04 PM, Tom Rini wrote:
On Wed, May 06, 2020 at 05:52:45PM +0200, Marek Vasut wrote:
On 5/6/20 5:43 PM, Alex Kiernan wrote: > On Wed, May 6, 2020 at 3:41 PM Marek Vasut marex@denx.de wrote: >> >> On 5/6/20 4:37 PM, Tom Rini wrote: >>> On Wed, May 06, 2020 at 04:33:37PM +0200, Marek Vasut wrote: >>>> On 5/6/20 4:27 PM, Tom Rini wrote: >>>>> On Wed, May 06, 2020 at 04:17:35PM +0200, Marek Vasut wrote: >>>>>> On 5/6/20 3:48 PM, Tom Rini wrote: >>>>>>> On Tue, May 05, 2020 at 11:17:19PM +0200, Michael Walle wrote: >>>>>>>> Hi all, >>>>>>>> >>>>>>>> Am 2020-05-05 20:41, schrieb Simon Glass: >>>>>>>>> Hi Tom, >>>>>>>>> >>>>>>>>> On Tue, 5 May 2020 at 11:50, Tom Rini trini@konsulko.com wrote: >>>>>>>>>> >>>>>>>>>> On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote: >>>>>>>>>>> On 5/5/20 6:37 PM, Alex Kiernan wrote: >>>>>>>>>>>> On Tue, May 5, 2020 at 2:28 PM Marek Vasut marex@denx.de wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> On 5/5/20 3:22 PM, Alex Kiernan wrote: >>>>>>>>>>>>>> On Mon, May 4, 2020 at 12:28 PM Tom Rini trini@konsulko.com wrote: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> There is no reason to tail-pad fitImage with external data to 4-bytes, >>>>>>>>>>>>>>>> while fitImage without external data does not have any such padding and >>>>>>>>>>>>>>>> is often unaligned. DT spec also does not mandate any such padding. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Moreover, the tail-pad fills the last few bytes with uninitialized data, >>>>>>>>>>>>>>>> which could lead to a potential information leak. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> $ echo -n xy > /tmp/data ; \ >>>>>>>>>>>>>>>> ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \ >>>>>>>>>>>>>>>> hexdump -vC /tmp/fitImage | tail -n 3 >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> before: >>>>>>>>>>>>>>>> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| >>>>>>>>>>>>>>>> 00000270 7a 65 00 00 78 79 64 64 |ze..xydd| >>>>>>>>>>>>>>>> ^^ ^^ ^^ >>>>>>>>>>>>>>>> after: >>>>>>>>>>>>>>>> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| >>>>>>>>>>>>>>>> 00000270 7a 65 00 78 79 |ze.xy| >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Signed-off-by: Marek Vasut marex@denx.de >>>>>>>>>>>>>>>> Reviewed-by: Simon Glass sjg@chromium.org >>>>>>>>>>>>>>>> Cc: Heinrich Schuchardt xypron.glpk@gmx.de >>>>>>>>>>>>>>>> Cc: Tom Rini trini@konsulko.com >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Applied to u-boot/master, thanks! >>>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> This breaks booting on my board (am3352, eMMC boot, FIT u-boot, >>>>>>>>>>>>>> CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it >>>>>>>>>>>>>> from eMMC I get nothing at all on the console, if I boot over ymodem >>>>>>>>>>>>>> it stalls at 420k, before continuing to 460k. My guess is there's some >>>>>>>>>>>>>> error going to the console at the 420k mark, but obviously it's lost >>>>>>>>>>>>>> in the ymodem... I have two DTBs in the FIT image, 420k would about >>>>>>>>>>>>>> align to the point between them. >>>>>>>>>>>>> >>>>>>>>>>>>> My bet would be on some padding / unaligned access problem that this >>>>>>>>>>>>> patch uncovered. Can you take a look ? >>>>>>>>>>>> >>>>>>>>>>>> Seems plausible. With this change my external data starts at 0x483 and >>>>>>>>>>>> everything after it is non-aligned: >>>>>>>>>>> >>>>>>>>>>> Should the beginning of external data be aligned ? >>>>>>>>>> >>>>>>>>>> If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does >>>>>>>>>> the >>>>>>>>>> problem go away? If so, that's not a fix outright, it means we need >>>>>>>>>> to >>>>>>>>>> dig back in to the libfdt thread and find the "make this work without >>>>>>>>>> killing performance everywhere all the time" option. >>>>>>>>> >>>>>>>>> If it is a device tree, it must be 32-bit aligned. >>>>>>>> >>>>>>>> This commit actually breaks my board too (which I was just about to send >>>>>>>> upstream, but realized it was broken). >>>>>>>> >>>>>>>> Said board uses SPL and main U-Boot. SPL runs fine and main u-boot doesn't >>>>>>>> output anything. The only difference which I found is that fit-dtb.blob is >>>>>>>> 2 bytes shorter. And the content is shifted by one byte although >>>>>>>> data-offset is the same. Strange. In the non-working case, the inner >>>>>>>> FDT magic isn't 4 byte aligned. >>>>>>>> >>>>>>>> You can find the two fit-dtb.blobs here: >>>>>>>> >>>>>>>> https://walle.cc/u-boot/fit-dtb.blob.working >>>>>>>> https://walle.cc/u-boot/fit-dtb.blob.non-working >>>>>>>> >>>>>>>> >>>>>>>> Reverting e8c2d25845c72c7202a628a97d45e31beea40668 doesn't help (I might >>>>>>>> reverted it the wrong way, there is actually a conflict). >>>>>>>> >>>>>>>> I'll dig deeper into that tomorrow, but maybe you have some pointers where >>>>>>>> to look. >>>>>>>> >>>>>>>> For reference you can find the current patch here: >>>>>>>> https://github.com/mwalle/u-boot/tree/sl28-upstream >>>>>>> >>>>>>> I think we have a few things to fix here. Marek's patch is breaking >>>>>>> things and needs to be reverted. But it's showing a few underlying >>>>>>> problems that need to be fixed too: >>>>>>> - fit_extract_data() needs to use calloc() not malloc() so that we don't >>>>>>> leak random data. >>>>>>> - We need to 8-byte alignment on the external data. That's the >>>>>>> requirement for Linux for device trees on both 32 and 64bit arm. >>>>>>> Atish, does RISC-V require more than that? I don't see it in Linux's >>>>>>> Documentation/riscv/boot-image-header.rst (and there's no booting.rst >>>>>>> file like arm/arm64). >>>>>> >>>>>> Why 8-byte alignment ? The external data are copied into the target >>>>>> location, so why do they need to be padded in any way? >>>>> >>>>> The start of the external data needs the alignment, to be clearer. >>>> >>>> Why ? >>> >>> Given that things which end up in external data have alignment >>> requirements, we need to align and meet those requirements. And I noted >>> why 8 above. >> >> If you end up with external data, then you need to move those blobs into >> their target location anyway. That's what you specify in the load = <> >> property in the .its . >> > > Just reading common/spl/spl_fit.c, I think that'll try and parse in > situ, rather than relocating it?
And is that correct or is that the same problem as we have on arm64 with fitImage and fdt_high=-1 ? I think it's the later.
I'm not sure that it is. Can we easily/safely memmove the data to be aligned? Is that really a better option in this case than ensuring alignment within the file?
Can't we use the new mkimage -B option to enforce the alignment IFF and only IFF it is required ?
Perhaps. But..
Then we can enforce it separately for 32bit and 64bit platforms to 4 and 8 bytes respectively even.
It's 8 bytes for both. It's possible that Linux doesn't hard fail if you only do 4 byte alignment but the documented requirement is 8, for arm32.
With Linux you usually need to move the kernel anyway, no ? It's 2 MiB for arm64 for example.
For arm64 you have to move it to where text_offset says it needs to be.
The alignment for compressed image is 2 MiB though, right ?
For arm32 the common (always, practically?) case is you're firing off the zImage which does what's needed. But..
And what you usually parse in-place would be the DT then.
Yes, the practical case is that it's a DT and that needs 8 byte alignment.
4-byte
And we should just get back to aligning that correctly. Going back to the v1 thread, it turns out the answer to "why do we even have this padding?" is "we need the DT to be aligned".
OK, let's assume you enforce 8-byte alignment on the external data. But then, there is fitImage with embedded data (mkimage called without -E), and the DT inside that fitImage will be aligned to 4 bytes. Then what?
On 5/6/20 12:02 PM, trini at konsulko.com (Tom Rini) wrote:
I'm not sure that it is. Can we easily/safely memmove the data to be aligned? Is that really a better option in this case than ensuring alignment within the file?
Can't we use the new mkimage -B option to enforce the alignment IFF and only IFF it is required ?
Perhaps. But..
Then we can enforce it separately for 32bit and 64bit platforms to 4 and 8 bytes respectively even.
It's 8 bytes for both. It's possible that Linux doesn't hard fail if you only do 4 byte alignment but the documented requirement is 8, for arm32.
With Linux you usually need to move the kernel anyway, no ? It's 2 MiB for arm64 for example.
For arm64 you have to move it to where text_offset says it needs to be. For arm32 the common (always, practically?) case is you're firing off the zImage which does what's needed. But..
And what you usually parse in-place would be the DT then.
Yes, the practical case is that it's a DT and that needs 8 byte alignment. And we should just get back to aligning that correctly. Going back to the v1 thread, it turns out the answer to "why do we even have this padding?" is "we need the DT to be aligned".
This change broke SPL booting for me on MACH_SUN50I as well. One thing that I haven't seen brought up yet is that SPL FIT code assumes exactly a 4-byte alignment of external data after the FIT. In spl_load_simple_fit():
/* * For FIT with external data, figure out where the external images * start. This is the base for the data-offset properties in each * image. */ size = fdt_totalsize(fit); size = (size + 3) & ~3; size = board_spl_fit_size_align(size); base_offset = (size + 3) & ~3;
And then later on in spl_load_fit_image():
if (!fit_image_get_data_position(fit, node, &offset)) { external_data = true; } else if (!fit_image_get_data_offset(fit, node, &offset)) { offset += base_offset; external_data = true; }
In my case, after this change, the FIT binary was 0x453 bytes long. But SPL rounded it up to 0x454, so the external data offsets were off by one, and the first byte of every loadable was cut off in RAM:
Trying to boot from MMC1 fit read sector 50, sectors=3, dst=49fff980, count=3, size=0x454 firmware: 'uboot' External data: dst=4a000000, offset=454, size=81208 src = 4a000054, dest = 4a000000 4a000000: 00 00 ea 35 00 00 14 00 00 00 00 00 00 00 00 00
If I remove both "(size + 3) & ~3" lines, I can boot successfully:
Trying to boot from MMC1
fit read sector 50, sectors=3, dst=49fff980, count=3, size=0x453 firmware: 'uboot'
External data: dst=4a000000, offset=453, size=81208 src = 4a000053, dest = 4a000000
4a000000: 1f 00 00 ea 35 00 00 14 00 00 00 00 00 00 00 00
In other words, it's not just FDTs that are affected by this change, but _all_ external data loaded from SPL.
So if you change the alignment to anything but 4 (be it 1 or 8 or something else), you must also update the assumptions made by SPL.
Regards, Samuel
On 5/7/20 10:46 PM, Samuel Holland wrote:
On 5/6/20 12:02 PM, trini at konsulko.com (Tom Rini) wrote:
I'm not sure that it is. Can we easily/safely memmove the data to be aligned? Is that really a better option in this case than ensuring alignment within the file?
Can't we use the new mkimage -B option to enforce the alignment IFF and only IFF it is required ?
Perhaps. But..
Then we can enforce it separately for 32bit and 64bit platforms to 4 and 8 bytes respectively even.
It's 8 bytes for both. It's possible that Linux doesn't hard fail if you only do 4 byte alignment but the documented requirement is 8, for arm32.
With Linux you usually need to move the kernel anyway, no ? It's 2 MiB for arm64 for example.
For arm64 you have to move it to where text_offset says it needs to be. For arm32 the common (always, practically?) case is you're firing off the zImage which does what's needed. But..
And what you usually parse in-place would be the DT then.
Yes, the practical case is that it's a DT and that needs 8 byte alignment. And we should just get back to aligning that correctly. Going back to the v1 thread, it turns out the answer to "why do we even have this padding?" is "we need the DT to be aligned".
This change broke SPL booting for me on MACH_SUN50I as well. One thing that I haven't seen brought up yet is that SPL FIT code assumes exactly a 4-byte alignment of external data after the FIT. In spl_load_simple_fit():
/*
- For FIT with external data, figure out where the external images
- start. This is the base for the data-offset properties in each
- image.
*/ size = fdt_totalsize(fit); size = (size + 3) & ~3; size = board_spl_fit_size_align(size); base_offset = (size + 3) & ~3;
Somehow this doesn't match the 8-byte alignment Tom was suggesting. And that only leads me to believe that we can either make assumptions about alignment, which would very likely fail one way or the other OR we can say that for SPL as a special case, we enforce some alignment.
But that in turn fails for fitImage with embedded data, where the embedded data are always aligned to 4 bytes, because that's how DTC aligns properties.
On Fri, May 08, 2020 at 03:37:01AM +0200, Marek Vasut wrote:
On 5/7/20 10:46 PM, Samuel Holland wrote:
On 5/6/20 12:02 PM, trini at konsulko.com (Tom Rini) wrote:
> I'm not sure that it is. Can we easily/safely memmove the data to be > aligned? Is that really a better option in this case than ensuring > alignment within the file?
Can't we use the new mkimage -B option to enforce the alignment IFF and only IFF it is required ?
Perhaps. But..
Then we can enforce it separately for 32bit and 64bit platforms to 4 and 8 bytes respectively even.
It's 8 bytes for both. It's possible that Linux doesn't hard fail if you only do 4 byte alignment but the documented requirement is 8, for arm32.
With Linux you usually need to move the kernel anyway, no ? It's 2 MiB for arm64 for example.
For arm64 you have to move it to where text_offset says it needs to be. For arm32 the common (always, practically?) case is you're firing off the zImage which does what's needed. But..
And what you usually parse in-place would be the DT then.
Yes, the practical case is that it's a DT and that needs 8 byte alignment. And we should just get back to aligning that correctly. Going back to the v1 thread, it turns out the answer to "why do we even have this padding?" is "we need the DT to be aligned".
This change broke SPL booting for me on MACH_SUN50I as well. One thing that I haven't seen brought up yet is that SPL FIT code assumes exactly a 4-byte alignment of external data after the FIT. In spl_load_simple_fit():
/*
- For FIT with external data, figure out where the external images
- start. This is the base for the data-offset properties in each
- image.
*/ size = fdt_totalsize(fit); size = (size + 3) & ~3; size = board_spl_fit_size_align(size); base_offset = (size + 3) & ~3;
Somehow this doesn't match the 8-byte alignment Tom was suggesting. And that only leads me to believe that we can either make assumptions about alignment, which would very likely fail one way or the other OR we can say that for SPL as a special case, we enforce some alignment.
It's likely the case that on arm32 as there's no natural alignment problem, even tho the kernel says 8 byte, 4 byte doesn't lead to failure and is rarely if ever given 4-but-not-8-byte-aligned addresses of the DTB. Which is why we should probably move the alignment here to 8 bytes instead of 4.
But that in turn fails for fitImage with embedded data, where the embedded data are always aligned to 4 bytes, because that's how DTC aligns properties.
I think the answer is that the use-case you're talking about is simply going to require data to be relocated.
On 5/8/20 8:47 PM, Tom Rini wrote:
On Fri, May 08, 2020 at 03:37:01AM +0200, Marek Vasut wrote:
On 5/7/20 10:46 PM, Samuel Holland wrote:
On 5/6/20 12:02 PM, trini at konsulko.com (Tom Rini) wrote:
>> I'm not sure that it is. Can we easily/safely memmove the data to be >> aligned? Is that really a better option in this case than ensuring >> alignment within the file? > > Can't we use the new mkimage -B option to enforce the alignment IFF and > only IFF it is required ?
Perhaps. But..
> Then we can enforce it separately for 32bit > and 64bit platforms to 4 and 8 bytes respectively even.
It's 8 bytes for both. It's possible that Linux doesn't hard fail if you only do 4 byte alignment but the documented requirement is 8, for arm32.
With Linux you usually need to move the kernel anyway, no ? It's 2 MiB for arm64 for example.
For arm64 you have to move it to where text_offset says it needs to be. For arm32 the common (always, practically?) case is you're firing off the zImage which does what's needed. But..
And what you usually parse in-place would be the DT then.
Yes, the practical case is that it's a DT and that needs 8 byte alignment. And we should just get back to aligning that correctly. Going back to the v1 thread, it turns out the answer to "why do we even have this padding?" is "we need the DT to be aligned".
This change broke SPL booting for me on MACH_SUN50I as well. One thing that I haven't seen brought up yet is that SPL FIT code assumes exactly a 4-byte alignment of external data after the FIT. In spl_load_simple_fit():
/*
- For FIT with external data, figure out where the external images
- start. This is the base for the data-offset properties in each
- image.
*/ size = fdt_totalsize(fit); size = (size + 3) & ~3; size = board_spl_fit_size_align(size); base_offset = (size + 3) & ~3;
Somehow this doesn't match the 8-byte alignment Tom was suggesting. And that only leads me to believe that we can either make assumptions about alignment, which would very likely fail one way or the other OR we can say that for SPL as a special case, we enforce some alignment.
It's likely the case that on arm32 as there's no natural alignment problem, even tho the kernel says 8 byte, 4 byte doesn't lead to failure and is rarely if ever given 4-but-not-8-byte-aligned addresses of the DTB. Which is why we should probably move the alignment here to 8 bytes instead of 4.
But that in turn fails for fitImage with embedded data, where the embedded data are always aligned to 4 bytes, because that's how DTC aligns properties.
I think the answer is that the use-case you're talking about is simply going to require data to be relocated.
I have a feeling that no matter how much you try to pad when generating fitImage from U-Boot, there will always be a case where that will fail. I listed at least two: - fitImage with embedded data, 4byte alignment due to DTC - Older fitImages, 4byte alignment, fails on arm64 - Someone can generate signed fitImage with older mkimage => fail
So that relocation logic or at least warning or something should be in there, no matter what.
On Fri, May 08, 2020 at 09:00:02PM +0200, Marek Vasut wrote:
On 5/8/20 8:47 PM, Tom Rini wrote:
On Fri, May 08, 2020 at 03:37:01AM +0200, Marek Vasut wrote:
On 5/7/20 10:46 PM, Samuel Holland wrote:
On 5/6/20 12:02 PM, trini at konsulko.com (Tom Rini) wrote:
>>> I'm not sure that it is. Can we easily/safely memmove the data to be >>> aligned? Is that really a better option in this case than ensuring >>> alignment within the file? >> >> Can't we use the new mkimage -B option to enforce the alignment IFF and >> only IFF it is required ? > > Perhaps. But.. > >> Then we can enforce it separately for 32bit >> and 64bit platforms to 4 and 8 bytes respectively even. > > It's 8 bytes for both. It's possible that Linux doesn't hard fail if > you only do 4 byte alignment but the documented requirement is 8, for > arm32.
With Linux you usually need to move the kernel anyway, no ? It's 2 MiB for arm64 for example.
For arm64 you have to move it to where text_offset says it needs to be. For arm32 the common (always, practically?) case is you're firing off the zImage which does what's needed. But..
And what you usually parse in-place would be the DT then.
Yes, the practical case is that it's a DT and that needs 8 byte alignment. And we should just get back to aligning that correctly. Going back to the v1 thread, it turns out the answer to "why do we even have this padding?" is "we need the DT to be aligned".
This change broke SPL booting for me on MACH_SUN50I as well. One thing that I haven't seen brought up yet is that SPL FIT code assumes exactly a 4-byte alignment of external data after the FIT. In spl_load_simple_fit():
/*
- For FIT with external data, figure out where the external images
- start. This is the base for the data-offset properties in each
- image.
*/ size = fdt_totalsize(fit); size = (size + 3) & ~3; size = board_spl_fit_size_align(size); base_offset = (size + 3) & ~3;
Somehow this doesn't match the 8-byte alignment Tom was suggesting. And that only leads me to believe that we can either make assumptions about alignment, which would very likely fail one way or the other OR we can say that for SPL as a special case, we enforce some alignment.
It's likely the case that on arm32 as there's no natural alignment problem, even tho the kernel says 8 byte, 4 byte doesn't lead to failure and is rarely if ever given 4-but-not-8-byte-aligned addresses of the DTB. Which is why we should probably move the alignment here to 8 bytes instead of 4.
But that in turn fails for fitImage with embedded data, where the embedded data are always aligned to 4 bytes, because that's how DTC aligns properties.
I think the answer is that the use-case you're talking about is simply going to require data to be relocated.
I have a feeling that no matter how much you try to pad when generating fitImage from U-Boot, there will always be a case where that will fail. I listed at least two:
- fitImage with embedded data, 4byte alignment due to DTC
- Older fitImages, 4byte alignment, fails on arm64
- Someone can generate signed fitImage with older mkimage => fail
So that relocation logic or at least warning or something should be in there, no matter what.
There's two distinct areas here, and they keep being conflated.
The case of SPL and a FIT image for U-Boot+DTB. We've always aligned this to 4 bytes and it's worked. I think if someone looked at the ARM ARM for aarch64 you could reason out that "4-but-not-8-byte aligned pointers are slow but work" as why this wasn't a hard fail on aarch64. We should adjust our current alignment up to cover that and move on.
The case of FIT images and "kernel_noload" / fdt_high=-1 / initrd_high=-1 and aarch64. If you load a FIT image in to memory and try and use it as-is, it will not work. It's not even possible in the general case as you would have to inspect the kernel, see what the text_offset is and build a FIT image that took that in to account, to not have to move the Image around. The device tree will almost certainly be misaligned and still need to be relocated. This is why a while back I sent out an email asking every maintainer of a board that disabled device tree relocation to stop that. Perhaps a run-time patch to scream about this rather than note it as we do today would help (see common/image-fdt.c::boot_relocate_fdt()).
On 5/8/20 9:21 PM, Tom Rini wrote:
On Fri, May 08, 2020 at 09:00:02PM +0200, Marek Vasut wrote:
On 5/8/20 8:47 PM, Tom Rini wrote:
On Fri, May 08, 2020 at 03:37:01AM +0200, Marek Vasut wrote:
On 5/7/20 10:46 PM, Samuel Holland wrote:
On 5/6/20 12:02 PM, trini at konsulko.com (Tom Rini) wrote:
>>>> I'm not sure that it is. Can we easily/safely memmove the data to be >>>> aligned? Is that really a better option in this case than ensuring >>>> alignment within the file? >>> >>> Can't we use the new mkimage -B option to enforce the alignment IFF and >>> only IFF it is required ? >> >> Perhaps. But.. >> >>> Then we can enforce it separately for 32bit >>> and 64bit platforms to 4 and 8 bytes respectively even. >> >> It's 8 bytes for both. It's possible that Linux doesn't hard fail if >> you only do 4 byte alignment but the documented requirement is 8, for >> arm32. > > With Linux you usually need to move the kernel anyway, no ? It's 2 MiB > for arm64 for example.
For arm64 you have to move it to where text_offset says it needs to be. For arm32 the common (always, practically?) case is you're firing off the zImage which does what's needed. But..
> And what you usually parse in-place would be the DT then.
Yes, the practical case is that it's a DT and that needs 8 byte alignment. And we should just get back to aligning that correctly. Going back to the v1 thread, it turns out the answer to "why do we even have this padding?" is "we need the DT to be aligned".
This change broke SPL booting for me on MACH_SUN50I as well. One thing that I haven't seen brought up yet is that SPL FIT code assumes exactly a 4-byte alignment of external data after the FIT. In spl_load_simple_fit():
/*
- For FIT with external data, figure out where the external images
- start. This is the base for the data-offset properties in each
- image.
*/ size = fdt_totalsize(fit); size = (size + 3) & ~3; size = board_spl_fit_size_align(size); base_offset = (size + 3) & ~3;
Somehow this doesn't match the 8-byte alignment Tom was suggesting. And that only leads me to believe that we can either make assumptions about alignment, which would very likely fail one way or the other OR we can say that for SPL as a special case, we enforce some alignment.
It's likely the case that on arm32 as there's no natural alignment problem, even tho the kernel says 8 byte, 4 byte doesn't lead to failure and is rarely if ever given 4-but-not-8-byte-aligned addresses of the DTB. Which is why we should probably move the alignment here to 8 bytes instead of 4.
But that in turn fails for fitImage with embedded data, where the embedded data are always aligned to 4 bytes, because that's how DTC aligns properties.
I think the answer is that the use-case you're talking about is simply going to require data to be relocated.
I have a feeling that no matter how much you try to pad when generating fitImage from U-Boot, there will always be a case where that will fail. I listed at least two:
- fitImage with embedded data, 4byte alignment due to DTC
- Older fitImages, 4byte alignment, fails on arm64
- Someone can generate signed fitImage with older mkimage => fail
So that relocation logic or at least warning or something should be in there, no matter what.
There's two distinct areas here, and they keep being conflated.
The case of SPL and a FIT image for U-Boot+DTB. We've always aligned this to 4 bytes and it's worked. I think if someone looked at the ARM ARM for aarch64 you could reason out that "4-but-not-8-byte aligned pointers are slow but work" as why this wasn't a hard fail on aarch64.
But we had hard-fault on arm64, see [PATCH] lib: rsa: Fix unaligned 64-bit fdt accesses
We should adjust our current alignment up to cover that and move on.
Adjust it to what, 8 bytes ? Or 16 in case RV128 happens ? Or what ? You will fail here either way, since if you build the fitImage with embedded data, the embedded data will be aligned to 4 bytes, because DT properties are aligned to 4 bytes.
The case of FIT images and "kernel_noload" / fdt_high=-1 / initrd_high=-1 and aarch64. If you load a FIT image in to memory and try and use it as-is, it will not work. It's not even possible in the general case as you would have to inspect the kernel, see what the text_offset is and build a FIT image that took that in to account, to not have to move the Image around. The device tree will almost certainly be misaligned and still need to be relocated. This is why a while back I sent out an email asking every maintainer of a board that disabled device tree relocation to stop that. Perhaps a run-time patch to scream about this rather than note it as we do today would help (see common/image-fdt.c::boot_relocate_fdt()).
I have a feeling we should do the relocation either way. And if there is some special limited case (like the SPL), we should warn about it and push mkimage with e.g. -B 8 flag to enforce the alignment.
On Sun, May 10, 2020 at 09:24:19PM +0200, Marek Vasut wrote:
On 5/8/20 9:21 PM, Tom Rini wrote:
On Fri, May 08, 2020 at 09:00:02PM +0200, Marek Vasut wrote:
On 5/8/20 8:47 PM, Tom Rini wrote:
On Fri, May 08, 2020 at 03:37:01AM +0200, Marek Vasut wrote:
On 5/7/20 10:46 PM, Samuel Holland wrote:
On 5/6/20 12:02 PM, trini at konsulko.com (Tom Rini) wrote: >>>>> I'm not sure that it is. Can we easily/safely memmove the data to be >>>>> aligned? Is that really a better option in this case than ensuring >>>>> alignment within the file? >>>> >>>> Can't we use the new mkimage -B option to enforce the alignment IFF and >>>> only IFF it is required ? >>> >>> Perhaps. But.. >>> >>>> Then we can enforce it separately for 32bit >>>> and 64bit platforms to 4 and 8 bytes respectively even. >>> >>> It's 8 bytes for both. It's possible that Linux doesn't hard fail if >>> you only do 4 byte alignment but the documented requirement is 8, for >>> arm32. >> >> With Linux you usually need to move the kernel anyway, no ? It's 2 MiB >> for arm64 for example. > > For arm64 you have to move it to where text_offset says it needs to be. > For arm32 the common (always, practically?) case is you're firing off > the zImage which does what's needed. But.. > >> And what you usually parse in-place would be the DT then. > > Yes, the practical case is that it's a DT and that needs 8 byte > alignment. And we should just get back to aligning that correctly. > Going back to the v1 thread, it turns out the answer to "why do we even > have this padding?" is "we need the DT to be aligned".
This change broke SPL booting for me on MACH_SUN50I as well. One thing that I haven't seen brought up yet is that SPL FIT code assumes exactly a 4-byte alignment of external data after the FIT. In spl_load_simple_fit():
/*
- For FIT with external data, figure out where the external images
- start. This is the base for the data-offset properties in each
- image.
*/ size = fdt_totalsize(fit); size = (size + 3) & ~3; size = board_spl_fit_size_align(size); base_offset = (size + 3) & ~3;
Somehow this doesn't match the 8-byte alignment Tom was suggesting. And that only leads me to believe that we can either make assumptions about alignment, which would very likely fail one way or the other OR we can say that for SPL as a special case, we enforce some alignment.
It's likely the case that on arm32 as there's no natural alignment problem, even tho the kernel says 8 byte, 4 byte doesn't lead to failure and is rarely if ever given 4-but-not-8-byte-aligned addresses of the DTB. Which is why we should probably move the alignment here to 8 bytes instead of 4.
But that in turn fails for fitImage with embedded data, where the embedded data are always aligned to 4 bytes, because that's how DTC aligns properties.
I think the answer is that the use-case you're talking about is simply going to require data to be relocated.
I have a feeling that no matter how much you try to pad when generating fitImage from U-Boot, there will always be a case where that will fail. I listed at least two:
- fitImage with embedded data, 4byte alignment due to DTC
- Older fitImages, 4byte alignment, fails on arm64
- Someone can generate signed fitImage with older mkimage => fail
So that relocation logic or at least warning or something should be in there, no matter what.
There's two distinct areas here, and they keep being conflated.
The case of SPL and a FIT image for U-Boot+DTB. We've always aligned this to 4 bytes and it's worked. I think if someone looked at the ARM ARM for aarch64 you could reason out that "4-but-not-8-byte aligned pointers are slow but work" as why this wasn't a hard fail on aarch64.
But we had hard-fault on arm64, see [PATCH] lib: rsa: Fix unaligned 64-bit fdt accesses
You're mixing the issues again. That's an example of "device tree properties are only 4 byte aligned" and we can't do what we were doing. I'm not even sure reverting e8c2d25845c7 would have helped in that case. It's also not the case we're talking about with respect to padding the start of embedded data.
We should adjust our current alignment up to cover that and move on.
Adjust it to what, 8 bytes ? Or 16 in case RV128 happens ? Or what ?
8 bytes is the defined requirement for everything today that defines a required _start_ alignment.
You will fail here either way, since if you build the fitImage with embedded data, the embedded data will be aligned to 4 bytes, because DT properties are aligned to 4 bytes.
Yes, there's the difference between "the device tree itself must be aligned to 8 bytes" and "device tree internally is 4 byte aligned". The latter means that some operations are simply not possible and a feature of the format.
The case of FIT images and "kernel_noload" / fdt_high=-1 / initrd_high=-1 and aarch64. If you load a FIT image in to memory and try and use it as-is, it will not work. It's not even possible in the general case as you would have to inspect the kernel, see what the text_offset is and build a FIT image that took that in to account, to not have to move the Image around. The device tree will almost certainly be misaligned and still need to be relocated. This is why a while back I sent out an email asking every maintainer of a board that disabled device tree relocation to stop that. Perhaps a run-time patch to scream about this rather than note it as we do today would help (see common/image-fdt.c::boot_relocate_fdt()).
I have a feeling we should do the relocation either way. And if there is some special limited case (like the SPL), we should warn about it and push mkimage with e.g. -B 8 flag to enforce the alignment.
Please send patches so we can see what that looks like, thanks!
On 5/11/20 9:36 PM, Tom Rini wrote:
On Sun, May 10, 2020 at 09:24:19PM +0200, Marek Vasut wrote:
On 5/8/20 9:21 PM, Tom Rini wrote:
On Fri, May 08, 2020 at 09:00:02PM +0200, Marek Vasut wrote:
On 5/8/20 8:47 PM, Tom Rini wrote:
On Fri, May 08, 2020 at 03:37:01AM +0200, Marek Vasut wrote:
On 5/7/20 10:46 PM, Samuel Holland wrote: > On 5/6/20 12:02 PM, trini at konsulko.com (Tom Rini) wrote: >>>>>> I'm not sure that it is. Can we easily/safely memmove the data to be >>>>>> aligned? Is that really a better option in this case than ensuring >>>>>> alignment within the file? >>>>> >>>>> Can't we use the new mkimage -B option to enforce the alignment IFF and >>>>> only IFF it is required ? >>>> >>>> Perhaps. But.. >>>> >>>>> Then we can enforce it separately for 32bit >>>>> and 64bit platforms to 4 and 8 bytes respectively even. >>>> >>>> It's 8 bytes for both. It's possible that Linux doesn't hard fail if >>>> you only do 4 byte alignment but the documented requirement is 8, for >>>> arm32. >>> >>> With Linux you usually need to move the kernel anyway, no ? It's 2 MiB >>> for arm64 for example. >> >> For arm64 you have to move it to where text_offset says it needs to be. >> For arm32 the common (always, practically?) case is you're firing off >> the zImage which does what's needed. But.. >> >>> And what you usually parse in-place would be the DT then. >> >> Yes, the practical case is that it's a DT and that needs 8 byte >> alignment. And we should just get back to aligning that correctly. >> Going back to the v1 thread, it turns out the answer to "why do we even >> have this padding?" is "we need the DT to be aligned". > > This change broke SPL booting for me on MACH_SUN50I as well. One thing that I > haven't seen brought up yet is that SPL FIT code assumes exactly a 4-byte > alignment of external data after the FIT. In spl_load_simple_fit(): > > /* > * For FIT with external data, figure out where the external images > * start. This is the base for the data-offset properties in each > * image. > */ > size = fdt_totalsize(fit); > size = (size + 3) & ~3; > size = board_spl_fit_size_align(size); > base_offset = (size + 3) & ~3;
Somehow this doesn't match the 8-byte alignment Tom was suggesting. And that only leads me to believe that we can either make assumptions about alignment, which would very likely fail one way or the other OR we can say that for SPL as a special case, we enforce some alignment.
It's likely the case that on arm32 as there's no natural alignment problem, even tho the kernel says 8 byte, 4 byte doesn't lead to failure and is rarely if ever given 4-but-not-8-byte-aligned addresses of the DTB. Which is why we should probably move the alignment here to 8 bytes instead of 4.
But that in turn fails for fitImage with embedded data, where the embedded data are always aligned to 4 bytes, because that's how DTC aligns properties.
I think the answer is that the use-case you're talking about is simply going to require data to be relocated.
I have a feeling that no matter how much you try to pad when generating fitImage from U-Boot, there will always be a case where that will fail. I listed at least two:
- fitImage with embedded data, 4byte alignment due to DTC
- Older fitImages, 4byte alignment, fails on arm64
- Someone can generate signed fitImage with older mkimage => fail
So that relocation logic or at least warning or something should be in there, no matter what.
There's two distinct areas here, and they keep being conflated.
The case of SPL and a FIT image for U-Boot+DTB. We've always aligned this to 4 bytes and it's worked. I think if someone looked at the ARM ARM for aarch64 you could reason out that "4-but-not-8-byte aligned pointers are slow but work" as why this wasn't a hard fail on aarch64.
But we had hard-fault on arm64, see [PATCH] lib: rsa: Fix unaligned 64-bit fdt accesses
You're mixing the issues again. That's an example of "device tree properties are only 4 byte aligned" and we can't do what we were doing. I'm not even sure reverting e8c2d25845c7 would have helped in that case. It's also not the case we're talking about with respect to padding the start of embedded data.
No, I am not mixing any issues again. These issues are all interconnected, which is why this is a valid example of the problems we have with the padding.
We should adjust our current alignment up to cover that and move on.
Adjust it to what, 8 bytes ? Or 16 in case RV128 happens ? Or what ?
8 bytes is the defined requirement for everything today that defines a required _start_ alignment.
Except DT properties are 4 byte aligned, so that's not true.
You will fail here either way, since if you build the fitImage with embedded data, the embedded data will be aligned to 4 bytes, because DT properties are aligned to 4 bytes.
Yes, there's the difference between "the device tree itself must be aligned to 8 bytes" and "device tree internally is 4 byte aligned". The latter means that some operations are simply not possible and a feature of the format.
So fitImage with embedded data should not be possible at all ?
The case of FIT images and "kernel_noload" / fdt_high=-1 / initrd_high=-1 and aarch64. If you load a FIT image in to memory and try and use it as-is, it will not work. It's not even possible in the general case as you would have to inspect the kernel, see what the text_offset is and build a FIT image that took that in to account, to not have to move the Image around. The device tree will almost certainly be misaligned and still need to be relocated. This is why a while back I sent out an email asking every maintainer of a board that disabled device tree relocation to stop that. Perhaps a run-time patch to scream about this rather than note it as we do today would help (see common/image-fdt.c::boot_relocate_fdt()).
I have a feeling we should do the relocation either way. And if there is some special limited case (like the SPL), we should warn about it and push mkimage with e.g. -B 8 flag to enforce the alignment.
Please send patches so we can see what that looks like, thanks!
Maybe we should remove support for embedded data from fitImage and then enforce padding ?
On Tue, May 5, 2020 at 7:41 PM Simon Glass sjg@chromium.org wrote:
Hi Tom,
On Tue, 5 May 2020 at 11:50, Tom Rini trini@konsulko.com wrote:
On Tue, May 05, 2020 at 06:39:58PM +0200, Marek Vasut wrote:
On 5/5/20 6:37 PM, Alex Kiernan wrote:
On Tue, May 5, 2020 at 2:28 PM Marek Vasut marex@denx.de wrote:
On 5/5/20 3:22 PM, Alex Kiernan wrote:
On Mon, May 4, 2020 at 12:28 PM Tom Rini trini@konsulko.com wrote: > > On Fri, May 01, 2020 at 05:40:25PM +0200, Marek Vasut wrote: > >> There is no reason to tail-pad fitImage with external data to 4-bytes, >> while fitImage without external data does not have any such padding and >> is often unaligned. DT spec also does not mandate any such padding. >> >> Moreover, the tail-pad fills the last few bytes with uninitialized data, >> which could lead to a potential information leak. >> >> $ echo -n xy > /tmp/data ; \ >> ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \ >> hexdump -vC /tmp/fitImage | tail -n 3 >> >> before: >> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| >> 00000270 7a 65 00 00 78 79 64 64 |ze..xydd| >> ^^ ^^ ^^ >> after: >> 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| >> 00000270 7a 65 00 78 79 |ze.xy| >> >> Signed-off-by: Marek Vasut marex@denx.de >> Reviewed-by: Simon Glass sjg@chromium.org >> Cc: Heinrich Schuchardt xypron.glpk@gmx.de >> Cc: Tom Rini trini@konsulko.com > > Applied to u-boot/master, thanks! >
This breaks booting on my board (am3352, eMMC boot, FIT u-boot, CONFIG_SPL_LOAD_FIT). Not got any useful diagnostics - if I boot it from eMMC I get nothing at all on the console, if I boot over ymodem it stalls at 420k, before continuing to 460k. My guess is there's some error going to the console at the 420k mark, but obviously it's lost in the ymodem... I have two DTBs in the FIT image, 420k would about align to the point between them.
My bet would be on some padding / unaligned access problem that this patch uncovered. Can you take a look ?
Seems plausible. With this change my external data starts at 0x483 and everything after it is non-aligned:
Should the beginning of external data be aligned ?
If in U-Boot we revert e8c2d25845c72c7202a628a97d45e31beea40668 does the problem go away? If so, that's not a fix outright, it means we need to dig back in to the libfdt thread and find the "make this work without killing performance everywhere all the time" option.
If it is a device tree, it must be 32-bit aligned.
But Marek's patch affects the FIT image itself, so I am not sure what would go after that.
Just reading the commit log, the example in there shows the padding after the FIT image being dropped:
$ echo -n xy > /tmp/data ; \ ./tools/mkimage -E -f auto -d /tmp/data /tmp/fitImage ; \ hexdump -vC /tmp/fitImage | tail -n 3
before: 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| 00000270 7a 65 00 00 78 79 64 64 |ze..xydd| ^^ ^^ ^^ after: 00000260 61 2d 6f 66 66 73 65 74 00 64 61 74 61 2d 73 69 |a-offset.data-si| 00000270 7a 65 00 78 79 |ze.xy|
Though I can't reproduce that result with or without the associated commit.
-- Alex Kiernan
participants (6)
-
Alex Kiernan -
Marek Vasut -
Michael Walle -
Samuel Holland -
Simon Glass -
Tom Rini