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>> 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>> >>> >>> >>> >>> 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>> >>> >>> --- >>> >>> >>> >>> 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) -- Best regards, Marek Vasut _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
