From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Goldschmidt Date: Sat, 24 Nov 2018 20:18:45 +0100 Subject: [U-Boot] [PATCH] arm: socfpga: crc-protect SPL on warm boot In-Reply-To: <2d788bd7-16ec-7302-20c7-78d2136210cc@denx.de> References: <20181120192215.13647-1-simon.k.r.goldschmidt@gmail.com> <4d9df11c-6c41-15aa-b5dd-e66153038241@denx.de> <699ba8cd-9783-5c04-86ae-bd63484050ae@gmail.com> <2d788bd7-16ec-7302-20c7-78d2136210cc@denx.de> Message-ID: <48a717b2-73c8-3a4e-00dc-11029e674785@gmail.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de 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 > > 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 > > >> >>> >> >>> 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 >> > > >> >>> --- >> >>> >> >>>   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 have v2 running with bitrev and lib/crc32. However, on the socrates board, it does not run stable. Looks like a size issue, perhaps some of the memory is overwritten. I'll have to investigate further. I guess for this v1, it would be the same, depending on SPL size. So please do not push this v1, it probably wouldn't always work. Thanks, Simon