1;5201;0c On Wed, May 02, 2018 at 10:34:49AM +0100, Måns Rullgård wrote: > Siarhei Siamashka <siarhei.siamas...@gmail.com> writes: > > > On Tue, 01 May 2018 18:25:06 +0100 > > Måns Rullgård <m...@mansr.com> wrote: > > > >> Maxime Ripard <maxime.rip...@free-electrons.com> writes: > >> > >> > 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 <andre.przyw...@arm.com> > >> > Signed-off-by: Maxime Ripard <maxime.rip...@free-electrons.com> > >> > --- > >> > arch/arm/dts/sunxi-u-boot.dtsi | 12 ++++++++++++ > >> > 1 file changed, 12 insertions(+) > >> > > >> > diff --git a/arch/arm/dts/sunxi-u-boot.dtsi > >> > b/arch/arm/dts/sunxi-u-boot.dtsi > >> > index 5adfd9bca2ec..72e95afd780e 100644 > >> > --- a/arch/arm/dts/sunxi-u-boot.dtsi > >> > +++ b/arch/arm/dts/sunxi-u-boot.dtsi > >> > @@ -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>; > >> > }; > >> > }; > >> > -- > >> > >> 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. > > > > 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. > > Do U-Boot users really need that level of hand-holding?
Yes, and that's never a good argument to make, because... > > 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. > 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. > 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 -- Maxime Ripard, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com
signature.asc
Description: PGP signature
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot