sunxi: Fix boot of Cubietruk and al.
Subsystems (2)
v3 cover sunxi: Fix boot of Cubietruk and al.
Hi
Most featureful boards, such as the Cubietruck, have been broken since the release 2017.09 (the two variants of the Olinuxino-Lime2 and the cubietruck at least, possibly more since then).
This is due to a size increase of the binary that will trip us across the size we've been using in our default configuration since forever, and widely distributed through the u-boot-sunxi-with-spl.bin file.
We would have several ways to work around it. The first one would be to just increase the offset of the environment. However, since it would break all the environments of our users and possibly the custom partition scheme that they would have created, it doesn't really seem like a smart move.
The second one would be to move the environment to a filesystem file, which would also break all the existing users. This can be envisionned as a long term fix though.
Another one would be to start trimming down a bit our enabled options in order to reduce the size and to gain some extra space for users customisations. However, this will always result in pointless and endless discussions, so let's move away from that.
The final one that has been implemented would be to just build U-Boot using thumb2 to push back the issue until hopefully I'm no longer maintainer or the switch to the env filesystem would have been done.
I've also added a patch to make sure that the compilation breaks and that we can notice.
Maxime
Changes from v2:
- Reorder the patches
- Changed the wording of U-boot to U-Boot
- Changed the hardcoded value for CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR
Maxime Ripard (2):
sunxi: Enable THUMB build for the U-Boot binary
sunxi: binman: Add U-Boot binary size check
arch/arm/Kconfig | 1 + arch/arm/dts/sunxi-u-boot.dtsi | 12 ++++++++++++ 2 files changed, 13 insertions(+)
Signature · 1 line
2.14.2
PATCH v3 1/2 sunxi: Enable THUMB build for the U-Boot binary
We start to get to the limit of our main U-Boot binary size (with some boards even crossing it). Enable its build using thumb2 to get some extra room.
--- arch/arm/Kconfig | 1 + 1 file changed, 1 insertion(+)
arch/arm/Kconfig +1 -0
@@ -698,6 +698,7 @@ config ARCH_SUNXI select SPL_SYS_MALLOC_SIMPLE if SPL select SYS_NS16550 select SPL_SYS_THUMB_BUILD if !ARM64 + select SYS_THUMB_BUILD if !ARM64 select USB if DISTRO_DEFAULTS select USB_STORAGE if DISTRO_DEFAULTS select USB_KEYBOARD if DISTRO_DEFAULTS
Signature · 1 line
2.14.2
PATCH v3 1/2 sunxi: Enable THUMB build for the U-Boot binary
Quoted 6 lines · Maxime Ripard
We start to get to the limit of our main U-Boot binary size (with some boards even crossing it). Enable its build using thumb2 to get some extra room.
Suggested-by Siarhei Siamashka <[email protected]>Signed-off-by Maxime Ripard <[email protected]>
I quickly boot tested this on an OrangePi Zero and a BananaPi-M1, also Pine64 (fwiw).
Also buildman was happy (with the whole series) on all 123 sunxi boards (ignoring the warnings a new dtc pops up on the 64-bit boards).
Thanks!
Andre
Quoted 17 lines
--- arch/arm/Kconfig | 1 + 1 file changed, 1 insertion(+)arch/arm/Kconfig +1
-0@@ -698,6 +698,7 @@ config ARCH_SUNXI select SPL_SYS_MALLOC_SIMPLE if SPL select SYS_NS16550 select SPL_SYS_THUMB_BUILD if !ARM64 + select SYS_THUMB_BUILD if !ARM64 select USB if DISTRO_DEFAULTS select USB_STORAGE if DISTRO_DEFAULTS select USB_KEYBOARD if DISTRO_DEFAULTS
PATCH v3 2/2 sunxi: binman: Add U-Boot binary size check
In reply to Maxime RipardThe U-Boot binary may trip over its actual allocated size in the storage. In such a case, the environment will not be readable anymore (because corrupted when the new image was flashed), and any attempt at using saveenv to reconstruct the environment will result in a corrupted U-Boot binary.
--- arch/arm/dts/sunxi-u-boot.dtsi | 12 ++++++++++++ 1 file changed, 12 insertions(+)
arch/arm/dts/sunxi-u-boot.dtsi +12 -0
@@ -1,5 +1,14 @@ #include <config.h> +/* + * This is the maximum size the U-Boot binary can be, which is basically + * the start of the environment, minus the start of the U-Boot binary in + * the MMC. This makes the assumption that the MMC is using 512-bytes + * blocks, but devices using something other than that remains to be + * seen. + */ +#define UBOOT_MMC_MAX_SIZE (CONFIG_ENV_OFFSET - (CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR * 512)) + / { binman { filename = "u-boot-sunxi-with-spl.bin";
@@ -8,6 +17,9 @@ filename = "spl/sunxi-spl.bin"; }; u-boot-img { +#ifdef CONFIG_MMC + size = <UBOOT_MMC_MAX_SIZE>; +#endif pos = <CONFIG_SPL_PAD_TO>; }; };
Signature · 1 line
2.14.2
PATCH v3 2/2 sunxi: binman: Add U-Boot binary size check
Quoted 32 lines · Maxime Ripard
The U-Boot binary may trip over its actual allocated size in the storage. In such a case, the environment will not be readable anymore (because corrupted when the new image was flashed), and any attempt at using saveenv to reconstruct the environment will result in a corrupted U-Boot binary.
Reviewed-by Andre Przywara <[email protected]>Signed-off-by Maxime Ripard <[email protected]>--- arch/arm/dts/sunxi-u-boot.dtsi | 12 ++++++++++++ 1 file changed, 12 insertions(+)arch/arm/dts/sunxi-u-boot.dtsi +9
-0@@ -1,5 +1,14 @@ #include <config.h> +/* + * This is the maximum size the U-Boot binary can be, which is basically + * the start of the environment, minus the start of the U-Boot binary in + * the MMC. This makes the assumption that the MMC is using 512-bytes + * blocks, but devices using something other than that remains to be + * seen. + */ +#define UBOOT_MMC_MAX_SIZE (CONFIG_ENV_OFFSET - (CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR * 512)) + / { binman { filename = "u-boot-sunxi-with-spl.bin";
arch/arm/dts/sunxi-u-boot.dtsi line 25
Quoted 9 lines
arch/arm/dts/sunxi-u-boot.dtsi +3
-0@@ -8,6 +17,9 @@ filename = "spl/sunxi-spl.bin"; }; u-boot-img { +#ifdef CONFIG_MMC + size = <UBOOT_MMC_MAX_SIZE>; +#endif pos = <CONFIG_SPL_PAD_TO>; }; };
This is broken in (at least) two ways:
1. It is simply nonsensical if u-boot and env are on different devices or not on mmc even if mmc support is enabled.
2. It causes u-boot-sunxi-with-spl.bin to be padded to the env offset. At best, this is useless. If the env doesn't immediately follow u-boot, it really breaks things.
Please fix or revert, I don't really care which.
Signature · 1 line
Måns Rullgård
PATCH v3 2/2 sunxi: binman: Add U-Boot binary size check
Quoted 46 lines · On Tue, 01 May 2018 18:25:06 +0100 Måns Rullgård <[email protected]> wrote:
Quoted 32 lines · Maxime Ripard
The U-Boot binary may trip over its actual allocated size in the storage. In such a case, the environment will not be readable anymore (because corrupted when the new image was flashed), and any attempt at using saveenv to reconstruct the environment will result in a corrupted U-Boot binary.
Reviewed-by Andre Przywara <[email protected]>Signed-off-by Maxime Ripard <[email protected]>--- arch/arm/dts/sunxi-u-boot.dtsi | 12 ++++++++++++ 1 file changed, 12 insertions(+)arch/arm/dts/sunxi-u-boot.dtsi +9
-0@@ -1,5 +1,14 @@ #include <config.h> +/* + * This is the maximum size the U-Boot binary can be, which is basically + * the start of the environment, minus the start of the U-Boot binary in + * the MMC. This makes the assumption that the MMC is using 512-bytes + * blocks, but devices using something other than that remains to be + * seen. + */ +#define UBOOT_MMC_MAX_SIZE (CONFIG_ENV_OFFSET - (CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR * 512)) + / { binman { filename = "u-boot-sunxi-with-spl.bin";arch/arm/dts/sunxi-u-boot.dtsi line 25
Quoted 9 lines
arch/arm/dts/sunxi-u-boot.dtsi +3
-0@@ -8,6 +17,9 @@ filename = "spl/sunxi-spl.bin"; }; u-boot-img { +#ifdef CONFIG_MMC + size = <UBOOT_MMC_MAX_SIZE>; +#endif pos = <CONFIG_SPL_PAD_TO>; }; };This is broken in (at least) two ways:
Quoted 8 lines
1. It is simply nonsensical if u-boot and env are on different devices or not on mmc even if mmc support is enabled.
2. It causes u-boot-sunxi-with-spl.bin to be padded to the env offset. At best, this is useless. If the env doesn't immediately follow u-boot, it really breaks things.
Please fix or revert, I don't really care which.
The padding is not useless. It reserves space for future size expansions and makes it harder for the users to hurt themselves.
For example, if the padded U-Boot size is 1024K while the actual size is only ~800K, then adventurous users might be tempted to fit some of their data into this gap. Yay, ~200K of storage space for free! Except that the next U-Boot release may grow up to 900K without any warning and if the users are not careful enough, then their data would be corrupted during upgrade.
Could you please tell us what is your problem and why reverting this patch would fix it?
Signature · 2 lines
Best regards, Siarhei Siamashka
PATCH v3 2/2 sunxi: binman: Add U-Boot binary size check
Quoted 61 lines · Siarhei Siamashka
Quoted 46 lines · On Tue, 01 May 2018 18:25:06 +0100 M책ns Rullg책rd <[email protected]> wrote:
Quoted 32 lines · Maxime Ripard
The U-Boot binary may trip over its actual allocated size in the storage. In such a case, the environment will not be readable anymore (because corrupted when the new image was flashed), and any attempt at using saveenv to reconstruct the environment will result in a corrupted U-Boot binary.
Reviewed-by Andre Przywara <[email protected]>Signed-off-by Maxime Ripard <[email protected]>--- arch/arm/dts/sunxi-u-boot.dtsi | 12 ++++++++++++ 1 file changed, 12 insertions(+)arch/arm/dts/sunxi-u-boot.dtsi +9
-0@@ -1,5 +1,14 @@ #include <config.h> +/* + * This is the maximum size the U-Boot binary can be, which is basically + * the start of the environment, minus the start of the U-Boot binary in + * the MMC. This makes the assumption that the MMC is using 512-bytes + * blocks, but devices using something other than that remains to be + * seen. + */ +#define UBOOT_MMC_MAX_SIZE (CONFIG_ENV_OFFSET - (CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR * 512)) + / { binman { filename = "u-boot-sunxi-with-spl.bin";arch/arm/dts/sunxi-u-boot.dtsi line 25
Quoted 9 lines
arch/arm/dts/sunxi-u-boot.dtsi +3
-0@@ -8,6 +17,9 @@ filename = "spl/sunxi-spl.bin"; }; u-boot-img { +#ifdef CONFIG_MMC + size = <UBOOT_MMC_MAX_SIZE>; +#endif pos = <CONFIG_SPL_PAD_TO>; }; };This is broken in (at least) two ways:
Quoted 8 lines
1. It is simply nonsensical if u-boot and env are on different devices or not on mmc even if mmc support is enabled.
2. It causes u-boot-sunxi-with-spl.bin to be padded to the env offset. At best, this is useless. If the env doesn't immediately follow u-boot, it really breaks things.
Please fix or revert, I don't really care which.
The padding is not useless. It reserves space for future size expansions and makes it harder for the users to hurt themselves.
Quoted 6 lines
For example, if the padded U-Boot size is 1024K while the actual size is only ~800K, then adventurous users might be tempted to fit some of their data into this gap. Yay, ~200K of storage space for free! Except that the next U-Boot release may grow up to 900K without any warning and if the users are not careful enough, then their data would be corrupted during upgrade.
Do U-Boot users really need that level of hand-holding?
Quoted 2 lines
Could you please tell us what is your problem and why reverting this patch would fix it?
See above. Best case, it just wastes space in the created file. If the configuration is anything other than U-Boot immediately followed by env on the same device, it _will_ break things horribly. A few examples:
1. U-Boot and env are on different devices, e.g. U-Boot on mmc and env in SPI NOR flash. In this case, padding the file to the env offset makes no sense. Writing the image will corrupt anything stored after U-Boot at a smaller (but still safe) offset.
2. U-Boot at a non-zero offset, followed by env, but _not_ on mmc. If CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR, probably at its default value, is smaller than the offset of U-Boot in its actual device, the padding will be too large. Writing the image file will then corrupt a stored env.
3. U-Boot at start of device, env at end as indicated by a negative CONFIG_ENV_OFFSET. With this configuration, binman tries to pad the image to (nearly) 2^64 bytes and promptly fills up your storage device.
I keep running into variants of these, and I'm weary of it.
Signature · 1 line
M책ns Rullg책rd
PATCH v3 2/2 sunxi: binman: Add U-Boot binary size check
1;5201;0c
Quoted 63 lines · M책ns Rullg책rd
Quoted 61 lines · Siarhei Siamashka
Quoted 46 lines · On Tue, 01 May 2018 18:25:06 +0100 M책ns Rullg책rd <[email protected]> wrote:
Quoted 32 lines · Maxime Ripard
The U-Boot binary may trip over its actual allocated size in the storage. In such a case, the environment will not be readable anymore (because corrupted when the new image was flashed), and any attempt at using saveenv to reconstruct the environment will result in a corrupted U-Boot binary.
Reviewed-by Andre Przywara <[email protected]>Signed-off-by Maxime Ripard <[email protected]>--- arch/arm/dts/sunxi-u-boot.dtsi | 12 ++++++++++++ 1 file changed, 12 insertions(+)arch/arm/dts/sunxi-u-boot.dtsi +9
-0@@ -1,5 +1,14 @@ #include <config.h> +/* + * This is the maximum size the U-Boot binary can be, which is basically + * the start of the environment, minus the start of the U-Boot binary in + * the MMC. This makes the assumption that the MMC is using 512-bytes + * blocks, but devices using something other than that remains to be + * seen. + */ +#define UBOOT_MMC_MAX_SIZE (CONFIG_ENV_OFFSET - (CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR * 512)) + / { binman { filename = "u-boot-sunxi-with-spl.bin";arch/arm/dts/sunxi-u-boot.dtsi line 25
Quoted 9 lines
arch/arm/dts/sunxi-u-boot.dtsi +3
-0@@ -8,6 +17,9 @@ filename = "spl/sunxi-spl.bin"; }; u-boot-img { +#ifdef CONFIG_MMC + size = <UBOOT_MMC_MAX_SIZE>; +#endif pos = <CONFIG_SPL_PAD_TO>; }; };This is broken in (at least) two ways:
Quoted 8 lines
1. It is simply nonsensical if u-boot and env are on different devices or not on mmc even if mmc support is enabled.
2. It causes u-boot-sunxi-with-spl.bin to be padded to the env offset. At best, this is useless. If the env doesn't immediately follow u-boot, it really breaks things.
Please fix or revert, I don't really care which.
The padding is not useless. It reserves space for future size expansions and makes it harder for the users to hurt themselves.
Quoted 8 lines
Quoted 6 lines
For example, if the padded U-Boot size is 1024K while the actual size is only ~800K, then adventurous users might be tempted to fit some of their data into this gap. Yay, ~200K of storage space for free! Except that the next U-Boot release may grow up to 900K without any warning and if the users are not careful enough, then their data would be corrupted during upgrade.
Do U-Boot users really need that level of hand-holding?
Yes, and that's never a good argument to make, because...
Quoted 11 lines
Quoted 2 lines
Could you please tell us what is your problem and why reverting this patch would fix it?
See above. Best case, it just wastes space in the created file. If the configuration is anything other than U-Boot immediately followed by env on the same device, it _will_ break things horribly. A few examples:
1. U-Boot and env are on different devices, e.g. U-Boot on mmc and env in SPI NOR flash. In this case, padding the file to the env offset makes no sense. Writing the image will corrupt anything stored after U-Boot at a smaller (but still safe) offset.
.. I could make pretty much the same one for all your cases, which would be completely useless, and wouldn't fix anything.
I guess this one could be solved using an ifdef guard with ENV_IS_IN_MMC.
Quoted 5 lines
2. U-Boot at a non-zero offset, followed by env, but _not_ on mmc. If
CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR, probably at its default value, is smaller than the offset of U-Boot in its actual device, the padding will be too large. Writing the image file will then corrupt a stored env.
This one would be covered too.
Quoted 4 lines
3. U-Boot at start of device, env at end as indicated by a negative
CONFIG_ENV_OFFSET. With this configuration, binman tries to pad the image to (nearly) 2^64 bytes and promptly fills up your storage device.
I'm not too sure about how to fix this one though. Any suggestion?
Maxime
Signature · 10 lines
Maxime Ripard, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 833 bytes Desc: not available URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180502/d376f331/attachment.sig>
PATCH v3 2/2 sunxi: binman: Add U-Boot binary size check
Quoted 83 lines · Maxime Ripard
1;5201;0c
Quoted 63 lines · Måns Rullgård
Quoted 61 lines · Siarhei Siamashka
Quoted 46 lines · On Tue, 01 May 2018 18:25:06 +0100 Måns Rullgård <[email protected]> wrote:
Quoted 32 lines · Maxime Ripard
The U-Boot binary may trip over its actual allocated size in the storage. In such a case, the environment will not be readable anymore (because corrupted when the new image was flashed), and any attempt at using saveenv to reconstruct the environment will result in a corrupted U-Boot binary.
Reviewed-by Andre Przywara <[email protected]>Signed-off-by Maxime Ripard <[email protected]>--- arch/arm/dts/sunxi-u-boot.dtsi | 12 ++++++++++++ 1 file changed, 12 insertions(+)arch/arm/dts/sunxi-u-boot.dtsi +9
-0@@ -1,5 +1,14 @@ #include <config.h> +/* + * This is the maximum size the U-Boot binary can be, which is basically + * the start of the environment, minus the start of the U-Boot binary in + * the MMC. This makes the assumption that the MMC is using 512-bytes + * blocks, but devices using something other than that remains to be + * seen. + */ +#define UBOOT_MMC_MAX_SIZE (CONFIG_ENV_OFFSET - (CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR * 512)) + / { binman { filename = "u-boot-sunxi-with-spl.bin";arch/arm/dts/sunxi-u-boot.dtsi line 25
Quoted 9 lines
arch/arm/dts/sunxi-u-boot.dtsi +3
-0@@ -8,6 +17,9 @@ filename = "spl/sunxi-spl.bin"; }; u-boot-img { +#ifdef CONFIG_MMC + size = <UBOOT_MMC_MAX_SIZE>; +#endif pos = <CONFIG_SPL_PAD_TO>; }; };This is broken in (at least) two ways:
Quoted 8 lines
1. It is simply nonsensical if u-boot and env are on different devices or not on mmc even if mmc support is enabled.
2. It causes u-boot-sunxi-with-spl.bin to be padded to the env offset. At best, this is useless. If the env doesn't immediately follow u-boot, it really breaks things.
Please fix or revert, I don't really care which.
The padding is not useless. It reserves space for future size expansions and makes it harder for the users to hurt themselves.
Quoted 8 lines
Quoted 6 lines
For example, if the padded U-Boot size is 1024K while the actual size is only ~800K, then adventurous users might be tempted to fit some of their data into this gap. Yay, ~200K of storage space for free! Except that the next U-Boot release may grow up to 900K without any warning and if the users are not careful enough, then their data would be corrupted during upgrade.
Do U-Boot users really need that level of hand-holding?
Yes, and that's never a good argument to make, because...
Quoted 7 lines
Quoted 2 lines
Could you please tell us what is your problem and why reverting this patch would fix it?
See above. Best case, it just wastes space in the created file. If the configuration is anything other than U-Boot immediately followed by env on the same device, it _will_ break things horribly. A few examples:
Quoted 7 lines
Quoted 4 lines
1. U-Boot and env are on different devices, e.g. U-Boot on mmc and env in SPI NOR flash. In this case, padding the file to the env offset makes no sense. Writing the image will corrupt anything stored after U-Boot at a smaller (but still safe) offset.
.. I could make pretty much the same one for all your cases, which would be completely useless, and wouldn't fix anything.
Huh? I'm saying we shouldn't "helpfully" do things that actually break perfectly valid setups. U-Boot users are expected to know what they are doing, and shouldn't need that kind of help. In my opinion.
Quoted 2 lines
I guess this one could be solved using an ifdef guard with
ENV_IS_IN_MMC.
Not if env is on a different mmc device.
Quoted 6 lines
Quoted 5 lines
2. U-Boot at a non-zero offset, followed by env, but _not_ on mmc. If
CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR, probably at its default value, is smaller than the offset of U-Boot in its actual device, the padding will be too large. Writing the image file will then corrupt a stored env.
Quoted 8 lines
This one would be covered too.
Quoted 4 lines
3. U-Boot at start of device, env at end as indicated by a negative
CONFIG_ENV_OFFSET. With this configuration, binman tries to pad the image to (nearly) 2^64 bytes and promptly fills up your storage device.I'm not too sure about how to fix this one though. Any suggestion?
I just don't see the point in trying to pin down the very specific case of U-Boot and env being on the same device with only a (small) amount of padding between them. There are a million other ways for users to screw up, so why should we be making a half-hearted effort to prevent this one?
Signature · 1 line
Måns Rullgård
PATCH v3 2/2 sunxi: binman: Add U-Boot binary size check
Quoted 90 lines · Måns Rullgård
Quoted 88 lines · Maxime Ripard
1;5201;0c
Quoted 63 lines · Måns Rullgård
Quoted 61 lines · Siarhei Siamashka
Quoted 46 lines · On Tue, 01 May 2018 18:25:06 +0100 Måns Rullgård <[email protected]> wrote:
Quoted 32 lines · Maxime Ripard
The U-Boot binary may trip over its actual allocated size in the storage. In such a case, the environment will not be readable anymore (because corrupted when the new image was flashed), and any attempt at using saveenv to reconstruct the environment will result in a corrupted U-Boot binary.
Reviewed-by Andre Przywara <[email protected]>Signed-off-by Maxime Ripard <[email protected]>--- arch/arm/dts/sunxi-u-boot.dtsi | 12 ++++++++++++ 1 file changed, 12 insertions(+)arch/arm/dts/sunxi-u-boot.dtsi +9
-0@@ -1,5 +1,14 @@ #include <config.h> +/* + * This is the maximum size the U-Boot binary can be, which is basically + * the start of the environment, minus the start of the U-Boot binary in + * the MMC. This makes the assumption that the MMC is using 512-bytes + * blocks, but devices using something other than that remains to be + * seen. + */ +#define UBOOT_MMC_MAX_SIZE (CONFIG_ENV_OFFSET - (CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR * 512)) + / { binman { filename = "u-boot-sunxi-with-spl.bin";arch/arm/dts/sunxi-u-boot.dtsi line 25
Quoted 9 lines
arch/arm/dts/sunxi-u-boot.dtsi +3
-0@@ -8,6 +17,9 @@ filename = "spl/sunxi-spl.bin"; }; u-boot-img { +#ifdef CONFIG_MMC + size = <UBOOT_MMC_MAX_SIZE>; +#endif pos = <CONFIG_SPL_PAD_TO>; }; };This is broken in (at least) two ways:
Quoted 8 lines
1. It is simply nonsensical if u-boot and env are on different devices or not on mmc even if mmc support is enabled.
2. It causes u-boot-sunxi-with-spl.bin to be padded to the env offset. At best, this is useless. If the env doesn't immediately follow u-boot, it really breaks things.
Please fix or revert, I don't really care which.
The padding is not useless. It reserves space for future size expansions and makes it harder for the users to hurt themselves.
Quoted 8 lines
Quoted 6 lines
For example, if the padded U-Boot size is 1024K while the actual size is only ~800K, then adventurous users might be tempted to fit some of their data into this gap. Yay, ~200K of storage space for free! Except that the next U-Boot release may grow up to 900K without any warning and if the users are not careful enough, then their data would be corrupted during upgrade.
Do U-Boot users really need that level of hand-holding?
Yes, and that's never a good argument to make, because...
Quoted 11 lines
Quoted 2 lines
Could you please tell us what is your problem and why reverting this patch would fix it?
See above. Best case, it just wastes space in the created file. If the configuration is anything other than U-Boot immediately followed by env on the same device, it _will_ break things horribly. A few examples:
1. U-Boot and env are on different devices, e.g. U-Boot on mmc and env in SPI NOR flash. In this case, padding the file to the env offset makes no sense. Writing the image will corrupt anything stored after U-Boot at a smaller (but still safe) offset.
Quoted 6 lines
Quoted 2 lines
.. I could make pretty much the same one for all your cases, which would be completely useless, and wouldn't fix anything.
Huh? I'm saying we shouldn't "helpfully" do things that actually break perfectly valid setups. U-Boot users are expected to know what they are doing, and shouldn't need that kind of help. In my opinion.
My point is that this is a slippery slope. Anyway..
Quoted 4 lines
Quoted 2 lines
I guess this one could be solved using an ifdef guard with
ENV_IS_IN_MMC.Not if env is on a different mmc device.
Ah, right...
Quoted 13 lines
Quoted 13 lines
Quoted 5 lines
2. U-Boot at a non-zero offset, followed by env, but _not_ on mmc. If
CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR, probably at its default value, is smaller than the offset of U-Boot in its actual device, the padding will be too large. Writing the image file will then corrupt a stored env.This one would be covered too.
Quoted 4 lines
3. U-Boot at start of device, env at end as indicated by a negative
CONFIG_ENV_OFFSET. With this configuration, binman tries to pad the image to (nearly) 2^64 bytes and promptly fills up your storage device.
Quoted 7 lines
Quoted 1 line
I'm not too sure about how to fix this one though. Any suggestion?
I just don't see the point in trying to pin down the very specific case of U-Boot and env being on the same device with only a (small) amount of padding between them. There are a million other ways for users to screw up, so why should we be making a half-hearted effort to prevent this one?
Unfortunately, this isn't a very specific case this is the default we shipped for years. And we have people relying on it in the wild.
This is why we added that check in the first place, because the size of uboot increased so much that it was now overlapping with the environment.
I guess now that we transitioned with the release of 2018.05, we can remove that check entirely, since the default is the first partition, wherever that is.
Maxime
Signature · 3 lines
Maxime Ripard, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com
PATCH v3 2/2 sunxi: binman: Add U-Boot binary size check
Quoted 91 lines · Maxime Ripard
Quoted 90 lines · Måns Rullgård
Quoted 88 lines · Maxime Ripard
1;5201;0c
Quoted 63 lines · Måns Rullgård
Quoted 61 lines · Siarhei Siamashka
Quoted 46 lines · On Tue, 01 May 2018 18:25:06 +0100 Måns Rullgård <[email protected]> wrote:
Quoted 32 lines · Maxime Ripard
The U-Boot binary may trip over its actual allocated size in the storage. In such a case, the environment will not be readable anymore (because corrupted when the new image was flashed), and any attempt at using saveenv to reconstruct the environment will result in a corrupted U-Boot binary.
Reviewed-by Andre Przywara <[email protected]>Signed-off-by Maxime Ripard <[email protected]>--- arch/arm/dts/sunxi-u-boot.dtsi | 12 ++++++++++++ 1 file changed, 12 insertions(+)arch/arm/dts/sunxi-u-boot.dtsi +9
-0@@ -1,5 +1,14 @@ #include <config.h> +/* + * This is the maximum size the U-Boot binary can be, which is basically + * the start of the environment, minus the start of the U-Boot binary in + * the MMC. This makes the assumption that the MMC is using 512-bytes + * blocks, but devices using something other than that remains to be + * seen. + */ +#define UBOOT_MMC_MAX_SIZE (CONFIG_ENV_OFFSET - (CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR * 512)) + / { binman { filename = "u-boot-sunxi-with-spl.bin";arch/arm/dts/sunxi-u-boot.dtsi line 25
Quoted 9 lines
arch/arm/dts/sunxi-u-boot.dtsi +3
-0@@ -8,6 +17,9 @@ filename = "spl/sunxi-spl.bin"; }; u-boot-img { +#ifdef CONFIG_MMC + size = <UBOOT_MMC_MAX_SIZE>; +#endif pos = <CONFIG_SPL_PAD_TO>; }; };This is broken in (at least) two ways:
Quoted 8 lines
1. It is simply nonsensical if u-boot and env are on different devices or not on mmc even if mmc support is enabled.
2. It causes u-boot-sunxi-with-spl.bin to be padded to the env offset. At best, this is useless. If the env doesn't immediately follow u-boot, it really breaks things.
Please fix or revert, I don't really care which.
The padding is not useless. It reserves space for future size expansions and makes it harder for the users to hurt themselves.
Quoted 8 lines
Quoted 6 lines
For example, if the padded U-Boot size is 1024K while the actual size is only ~800K, then adventurous users might be tempted to fit some of their data into this gap. Yay, ~200K of storage space for free! Except that the next U-Boot release may grow up to 900K without any warning and if the users are not careful enough, then their data would be corrupted during upgrade.
Do U-Boot users really need that level of hand-holding?
Yes, and that's never a good argument to make, because...
Quoted 11 lines
Quoted 2 lines
Could you please tell us what is your problem and why reverting this patch would fix it?
See above. Best case, it just wastes space in the created file. If the configuration is anything other than U-Boot immediately followed by env on the same device, it _will_ break things horribly. A few examples:
1. U-Boot and env are on different devices, e.g. U-Boot on mmc and env in SPI NOR flash. In this case, padding the file to the env offset makes no sense. Writing the image will corrupt anything stored after U-Boot at a smaller (but still safe) offset.
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
.. I could make pretty much the same one for all your cases, which would be completely useless, and wouldn't fix anything.
Huh? I'm saying we shouldn't "helpfully" do things that actually break perfectly valid setups. U-Boot users are expected to know what they are doing, and shouldn't need that kind of help. In my opinion.
My point is that this is a slippery slope. Anyway..
Yes, trying to be overly helpful is indeed a slippery slope.
Quoted 22 lines
Quoted 4 lines
Quoted 2 lines
I guess this one could be solved using an ifdef guard with
ENV_IS_IN_MMC.Not if env is on a different mmc device.
Ah, right...
Quoted 15 lines
Quoted 14 lines
Quoted 5 lines
2. U-Boot at a non-zero offset, followed by env, but _not_ on mmc. If
CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR, probably at its default value, is smaller than the offset of U-Boot in its actual device, the padding will be too large. Writing the image file will then corrupt a stored env.This one would be covered too.
Quoted 4 lines
3. U-Boot at start of device, env at end as indicated by a negative
CONFIG_ENV_OFFSET. With this configuration, binman tries to pad the image to (nearly) 2^64 bytes and promptly fills up your storage device.I'm not too sure about how to fix this one though. Any suggestion?
Quoted 8 lines
Quoted 5 lines
I just don't see the point in trying to pin down the very specific case of U-Boot and env being on the same device with only a (small) amount of padding between them. There are a million other ways for users to screw up, so why should we be making a half-hearted effort to prevent this one?
Unfortunately, this isn't a very specific case this is the default we shipped for years.
Your default is still just one specific case of nearly infinite possibilities.
Quoted 1 line
And we have people relying on it in the wild.
How can anyone *rely* on that padding being there?
Quoted 3 lines
This is why we added that check in the first place, because the size of uboot increased so much that it was now overlapping with the environment.
I get that. Unfortunately, the check was flawed for all but your default use case.
Quoted 3 lines
I guess now that we transitioned with the release of 2018.05, we can remove that check entirely, since the default is the first partition, wherever that is.
I don't care how you justify it as long as you get rid of the broken code.
Signature · 1 line
Måns Rullgård
Maxime Ripard
Appliedv3 cover sunxi: Fix boot of Cubietruk and al.
In reply to Maxime RipardQuoted 25 lines · Maxime Ripard
Hi
Most featureful boards, such as the Cubietruck, have been broken since the release 2017.09 (the two variants of the Olinuxino-Lime2 and the cubietruck at least, possibly more since then).
This is due to a size increase of the binary that will trip us across the size we've been using in our default configuration since forever, and widely distributed through the u-boot-sunxi-with-spl.bin file.
We would have several ways to work around it. The first one would be to just increase the offset of the environment. However, since it would break all the environments of our users and possibly the custom partition scheme that they would have created, it doesn't really seem like a smart move.
The second one would be to move the environment to a filesystem file, which would also break all the existing users. This can be envisionned as a long term fix though.
Another one would be to start trimming down a bit our enabled options in order to reduce the size and to gain some extra space for users customisations. However, this will always result in pointless and endless discussions, so let's move away from that.
Quoted 6 lines
The final one that has been implemented would be to just build U-Boot using thumb2 to push back the issue until hopefully I'm no longer maintainer or the switch to the env filesystem would have been done.
I've also added a patch to make sure that the compilation breaks and that we can notice.
Applied both, thanks!
Maxime
Signature · 10 lines
Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 801 bytes Desc: not available URL: <http://lists.denx.de/pipermail/u-boot/attachments/20171024/0a515452/attachment.sig>