On 11/22/2018 06:18 AM, Simon Goldschmidt wrote: > > > Am Do., 22. Nov. 2018, 03:00 hat Marek Vasut <ma...@denx.de > <mailto:ma...@denx.de>> geschrieben: > > On 11/21/2018 10:07 PM, Simon Goldschmidt wrote: > > On 21.11.2018 15:08, Marek Vasut wrote: > >> On 11/21/2018 06:09 AM, Simon Goldschmidt wrote: > >>> > >>> Am Mi., 21. Nov. 2018, 00:11 hat Marek Vasut <ma...@denx.de > <mailto:ma...@denx.de> > >>> <mailto:ma...@denx.de <mailto:ma...@denx.de>>> geschrieben: > >>> > >>> On 11/20/2018 09:54 PM, Simon Goldschmidt wrote: > >>> > On 20.11.2018 20:33, Marek Vasut wrote: > >>> >> On 11/20/2018 08:22 PM, Simon Goldschmidt wrote: > >>> >>> From: Simon Goldschmidt > <sgoldschm...@de.pepperl-fuchs.com > <mailto:sgoldschm...@de.pepperl-fuchs.com> > >>> <mailto:sgoldschm...@de.pepperl-fuchs.com > <mailto:sgoldschm...@de.pepperl-fuchs.com>>> > >>> >>> > >>> >>> On socfpga gen5, a warm reboot from Linux currently > triggers > >>> a warm > >>> >>> reset via reset manager ctrl register. > >>> >>> > >>> >>> This currently leads to the boot rom just jumping to > onchip ram > >>> >>> executing the SPL that is supposed to still be there. > This is > >>> >>> because we tell the boot rom to do so by writing a > magin value > >>> >>> the warmramgrp_enable register in arch_early_init_r(). > >>> >>> > >>> >>> However, this can lead to lockups on reboot if this > register > >>> still > >>> >>> contains its magic value but the SPL is not intact any more > >>> (e.g. > >>> >>> partly overwritten onchip ram). > >>> >>> > >>> >>> To fis this, store a crc calculated over SPL in sysmgr > >>> registers to > >>> >>> let the boot rom check it on next warm boot. If the crc is > >>> still > >>> >>> correct, SPL can be executd from onchip ram. If the crc > >>> fails, SPL > >>> >>> is loaded from original boot source. > >>> >>> > >>> >>> The crc that is written to the warmramgrp_crc register is > >>> the crc > >>> >>> found in the SPL sfp image but with one addiional u32 > added. > >>> For > >>> >>> this, we need to add a function to calculate the updated > >>> crc. This > >>> >>> is done as a bitwise calculation to keep the code increase > >>> small. > >>> >>> > >>> >>> This whole patch added 96 bytes to .text for SPL for > >>> >>> socfpga_socrates_defconfig. > >>> >>> > >>> >>> Signed-off-by: Simon Goldschmidt > >>> <simon.k.r.goldschm...@gmail.com > <mailto:simon.k.r.goldschm...@gmail.com> > >>> <mailto:simon.k.r.goldschm...@gmail.com > <mailto:simon.k.r.goldschm...@gmail.com>>> > >>> >>> --- > >>> >>> > >>> >>> arch/arm/mach-socfpga/misc_gen5.c | 9 ---- > >>> >>> arch/arm/mach-socfpga/spl_gen5.c | 73 > >>> +++++++++++++++++++++++++++++++ > >>> >>> 2 files changed, 73 insertions(+), 9 deletions(-) > >>> >>> > >>> >>> diff --git a/arch/arm/mach-socfpga/misc_gen5.c > >>> >>> b/arch/arm/mach-socfpga/misc_gen5.c > >>> >>> index 5fa40937c4..492a3082de 100644 > >>> >>> --- a/arch/arm/mach-socfpga/misc_gen5.c > >>> >>> +++ b/arch/arm/mach-socfpga/misc_gen5.c > >>> >>> @@ -204,15 +204,6 @@ int arch_early_init_r(void) > >>> >>> { > >>> >>> int i; > >>> >>> - /* > >>> >>> - * Write magic value into magic register to unlock > >>> support for > >>> >>> - * issuing warm reset. The ancient kernel code expects > >>> this > >>> >>> - * value to be written into the register by the > >>> bootloader, so > >>> >>> - * to support that old code, we write it here instead > >>> of in the > >>> >>> - * reset_cpu() function just before resetting the CPU. > >>> >>> - */ > >>> >>> - writel(0xae9efebc, > >>> &sysmgr_regs->romcodegrp_warmramgrp_enable); > >>> >>> - > >>> >>> for (i = 0; i < 8; i++) /* Cache initial SW > setting > >>> regs */ > >>> >>> iswgrp_handoff[i] = > >>> readl(&sysmgr_regs->iswgrp_handoff[i]); > >>> >>> diff --git a/arch/arm/mach-socfpga/spl_gen5.c > >>> >>> b/arch/arm/mach-socfpga/spl_gen5.c > >>> >>> index ccdc661d05..3416e19f79 100644 > >>> >>> --- a/arch/arm/mach-socfpga/spl_gen5.c > >>> >>> +++ b/arch/arm/mach-socfpga/spl_gen5.c > >>> >>> @@ -63,6 +63,76 @@ u32 spl_boot_mode(const u32 boot_device) > >>> >>> } > >>> >>> #endif > >>> >>> +/* This function calculates the CRC32 used by the > Cyclone > >>> 5 SoC > >>> >>> Boot Rom */ > >>> >>> +static u32 socfpga_boot_crc(u32 crc, const unsigned char > >>> *ptr, u32 > >>> >>> length) > >>> >>> +{ > >>> >>> + uint i; > >>> >>> + u8 bit; > >>> >>> + unsigned char data; > >>> >>> + const u32 poly = 0x02608edb; > >>> >>> + > >>> >>> + for (; length > 0; length--, ptr++) { > >>> >>> + data = *ptr; > >>> >>> + for (i = 0; i < 8; i++) { > >>> >>> + if (data & 0x80) > >>> >>> + bit = 1; > >>> >>> + else > >>> >>> + bit = 0; > >>> >>> + > >>> >>> + data = data << 1; > >>> >>> + if (crc & 0x80000000) > >>> >>> + bit = 1 - bit; > >>> >>> + > >>> >>> + if (bit) { > >>> >>> + crc ^= poly; > >>> >>> + crc = crc << 1; > >>> >>> + crc |= 1; > >>> >>> + } else { > >>> >>> + crc = crc << 1; > >>> >>> + } > >>> >>> + } > >>> >>> + } > >>> >>> + return crc; > >>> >>> +} > >>> >>> + > >>> >>> +/* > >>> >>> + * Write magic value into magic register to unlock support > >>> for the > >>> >>> boot rom to > >>> >>> + * execute spl from sram on warm reset. This may be > >>> required at > >>> >>> least on some > >>> >>> + * boards that start from qspi where the flash chip might > >>> be in a > >>> >>> state that > >>> >>> + * cannot be handled by the boot rom (e.g. 4 byte mode). > >>> >>> + * > >>> >>> + * To prevent just jumping to corrupted memory, a crc of > >>> the spl is > >>> >>> calculated. > >>> >>> + * This crc is loaded from the running image, but has > to be > >>> extended > >>> >>> by the > >>> >>> + * modified contents of the "datastart" register (i.e. > >>> 0xffff0000). > >>> >>> + */ > >>> >>> +static void spl_init_reboot_config(void) > >>> >>> +{ > >>> >>> + u32 spl_crc, spl_length; > >>> >>> + const u32 spl_start = (u32)__image_copy_start; > >>> >>> + const u32 spl_start_16 = spl_start & 0xffff; > >>> >>> + u32 spl_length_u32; > >>> >>> + > >>> >>> + /* load image length from sfp header (includes crc) */ > >>> >>> + spl_length_u32 = *(const u16 *)(spl_start + 0x46); > >>> >>> + /* subtract crc */ > >>> >>> + spl_length_u32--; > >>> >>> + /* get length in bytes */ > >>> >>> + spl_length = spl_length_u32 * 4; > >>> >>> + /* load crc */ > >>> >>> + spl_crc = *(const u32 *)(spl_start + spl_length); > >>> >>> + /* undo xor */ > >>> >>> + spl_crc ^= 0xffffffff; > >>> >>> + /* add contents of modified datastart register */ > >>> >>> + spl_crc = socfpga_boot_crc(spl_crc, (const u8 > >>> *)&spl_start, 4); > >>> >>> + /* finalize */ > >>> >>> + spl_crc ^= 0xffffffff; > >>> >>> + > >>> >>> + writel(0xae9efebc, > >>> &sysmgr_regs->romcodegrp_warmramgrp_enable); > >>> >>> + writel(spl_start_16, > >>> >>> &sysmgr_regs->romcodegrp_warmramgrp_datastart); > >>> >>> + writel(spl_length, > >>> &sysmgr_regs->romcodegrp_warmramgrp_length); > >>> >>> + writel(spl_crc, > &sysmgr_regs->romcodegrp_warmramgrp_crc); > >>> >>> +} > >>> >>> + > >>> >>> void board_init_f(ulong dummy) > >>> >>> { > >>> >>> const struct cm_config *cm_default_cfg = > >>> cm_get_default_config(); > >>> >>> @@ -82,6 +152,9 @@ void board_init_f(ulong dummy) > >>> >>> writel(SYSMGR_ECC_OCRAM_DERR | > SYSMGR_ECC_OCRAM_EN, > >>> >>> &sysmgr_regs->eccgrp_ocram); > >>> >>> + if (!socfpga_is_booting_from_fpga()) > >>> >>> + spl_init_reboot_config(); > >>> >>> + > >>> >>> memset(__bss_start, 0, __bss_end - __bss_start); > >>> >>> socfpga_sdram_remap_zero(); > >>> >>> > >>> >> Can't we use the library CRC32 function instead ? > >>> > > >>> > No, unfortunately, it's bit-reversed. Plus it uses a > table, which > >>> > increases the SPL binary by more than 2 KByte. > >>> > >>> Are you sure ? The uImage code also uses crc32, so I suspect > >>> that crc32 > >>> stuff is already in SPL. And the bit operation can probably be > >>> easily > >>> done. I might be wrong ... > >>> > >>> > >>> I wrote that as a result of testing it. The binary grew by 2k+. I'll > >>> have to check the uimage crc. > >> That's probably a good idea. > >> > >>> Bit reversing can be done, yes. I tested that too. It's a > rarther small > >>> function that could be added to some lib code (if it doesn't already > >>> exist, I haven't checked). > >> If we can reuse the CRC code, that'd be awesome. > > > > Ok, so I retested this and the result came as a bit of a shock to me. > > Binary size does increase by ~1.3kByte (not 2k+ as I wrote before) > when > > using lib/crc32.c to do the CRC check (with private code for reversing > > bits that works). This is 1K for the CRC table plus some code. That's > > probably ok, but: > > Thanks! > > > The shock was that I thought lib/crc32.c is what is used to check > uImage > > CRC and we shouldn't see any code size increasement. Turns out > that none > > of the files in common/spl (core or loaders) check the CRC of a > uImage! > > (Even with CONFIG_SPL_LOAD_FIT, the crc32 code isn't included.) > > Uh, how does the signature verification work then ? I guess it at least > includes the SHA support ? > > > Unless I'm mistaken, this means that SPL does not check the > validity of > > a U-Boot image at all (unless using FIT and enabling > > CONFIG_SPL_FIT_SIGNATURE, but this does not fit into 64k for > socfpga gen5). > > Yikes > > > Is this a known issue? Has it always been like this? Why do we > have a 2 > > CRCs in the U-Boot image then? > > The bootm command should check it. I was not aware SPL doesn't check it > and I don't believe that was intended, but I might be wrong. Can you > investigate ? (I'm pushing more things unto you since I'm under a lot of > pressure recently, too much stuff to do, and I have quite a bit of > confidence in you, in that you can figure it out) > > > Adding that CRC check to SPL is no big deal, I already did that to see > booting does fail when I invalidate the image. > > The thing I am concerned about is size increasement...
If it doesn't break any boards, that's actually a massive improvement. > I'll send a patch. Thanks -- Best regards, Marek Vasut _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
