[U-Boot] [PATCH] arm: socfpga: crc-protect SPL on warm boot
From: Simon Goldschmidt sgoldschmidt@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.goldschmidt@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();
On 11/20/2018 08:22 PM, Simon Goldschmidt wrote:
From: Simon Goldschmidt sgoldschmidt@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.goldschmidt@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;elsebit = 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 ?
On 20.11.2018 20:33, Marek Vasut wrote:
On 11/20/2018 08:22 PM, Simon Goldschmidt wrote:
From: Simon Goldschmidt sgoldschmidt@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.goldschmidt@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;elsebit = 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.
Simon
On Tue, 2018-11-20 at 21:54 +0100, 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 sgoldschmidt@de.pepperl-fuchs.com
On socfpga gen5, a warm reboot from Linux currently triggers a warm reset via reset manager ctrl register.
is there any reason to not just promote these to cold resets?
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.goldschmidt@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;elsebit = 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.
Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
On 11/20/2018 10:55 PM, Dalon L Westergreen wrote:
On Tue, 2018-11-20 at 21:54 +0100, 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 sgoldschmidt@de.pepperl-fuchs.com
On socfpga gen5, a warm reboot from Linux currently triggers a warm reset via reset manager ctrl register.
is there any reason to not just promote these to cold resets?
Why did Altera opt for warm resets on Gen5 in the first place ? I guess to avoid interferring with the FPGA and/or to circumvent problems with board designs which do not connect reset properly (esp. for SPI NOR) ?
Am Mi., 21. Nov. 2018, 00:12 hat Marek Vasut marex@denx.de geschrieben:
On 11/20/2018 10:55 PM, Dalon L Westergreen wrote:
On Tue, 2018-11-20 at 21:54 +0100, 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 sgoldschmidt@de.pepperl-fuchs.com
On socfpga gen5, a warm reboot from Linux currently triggers a warm reset via reset manager ctrl register.
is there any reason to not just promote these to cold resets?
Why did Altera opt for warm resets on Gen5 in the first place ? I guess to avoid interferring with the FPGA and/or to circumvent problems with board designs which do not connect reset properly (esp. for SPI NOR) ?
I really don't know. But it's not a question of cold or warm reset. Even in warm reset, we could stop to execute spl from sram and always load it from flash.
However, I don't know if this is ok for all users. And this patch only fixes the current behaviour of starting it without checking it.
Simon
On 11/21/2018 06:13 AM, Simon Goldschmidt wrote:
Am Mi., 21. Nov. 2018, 00:12 hat Marek Vasut <marex@denx.de mailto:marex@denx.de> geschrieben:
On 11/20/2018 10:55 PM, Dalon L Westergreen wrote: > On Tue, 2018-11-20 at 21:54 +0100, 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 <sgoldschmidt@de.pepperl-fuchs.com <mailto:sgoldschmidt@de.pepperl-fuchs.com>> >>>> >>>> On socfpga gen5, a warm reboot from Linux currently triggers a warm >>>> reset via reset manager ctrl register. > > is there any reason to not just promote these to cold resets? Why did Altera opt for warm resets on Gen5 in the first place ? I guess to avoid interferring with the FPGA and/or to circumvent problems with board designs which do not connect reset properly (esp. for SPI NOR) ?I really don't know. But it's not a question of cold or warm reset. Even in warm reset, we could stop to execute spl from sram and always load it from flash.
I'd like some answer from Altera/Intel on that :-)
However, I don't know if this is ok for all users. And this patch only fixes the current behaviour of starting it without checking it.
I think it's nice
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 sgoldschmidt@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.goldschmidt@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 ...
Am Mi., 21. Nov. 2018, 00:11 hat Marek Vasut marex@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 sgoldschmidt@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.goldschmidt@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]);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;elsebit = 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.
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).
Simon
On 11/21/2018 06:09 AM, Simon Goldschmidt wrote:
Am Mi., 21. Nov. 2018, 00:11 hat Marek Vasut <marex@denx.de mailto:marex@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 <sgoldschmidt@de.pepperl-fuchs.com <mailto:sgoldschmidt@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.goldschmidt@gmail.com <mailto:simon.k.r.goldschmidt@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.
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 <marex@denx.de mailto:marex@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 <sgoldschmidt@de.pepperl-fuchs.com <mailto:sgoldschmidt@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.goldschmidt@gmail.com <mailto:simon.k.r.goldschmidt@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:
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.)
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).
Is this a known issue? Has it always been like this? Why do we have a 2 CRCs in the U-Boot image then?
Simon
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 <marex@denx.de mailto:marex@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 <sgoldschmidt@de.pepperl-fuchs.com mailto:sgoldschmidt@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.goldschmidt@gmail.com mailto:simon.k.r.goldschmidt@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)
Am Do., 22. Nov. 2018, 03:00 hat Marek Vasut marex@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 <marex@denx.de mailto:marex@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 <sgoldschmidt@de.pepperl-fuchs.com <mailto:sgoldschmidt@de.pepperl-fuchs.com>> >>> >>> On socfpga gen5, a warm reboot from Linux currently triggersa 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 registerstill >>> 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.goldschmidt@gmail.com mailto:simon.k.r.goldschmidt@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 suspectthat 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...
I'll send a patch.
Simon
On 11/22/2018 06:18 AM, Simon Goldschmidt wrote:
Am Do., 22. Nov. 2018, 03:00 hat Marek Vasut <marex@denx.de mailto:marex@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 <marex@denx.de <mailto:marex@denx.de> >>> <mailto:marex@denx.de <mailto:marex@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 <sgoldschmidt@de.pepperl-fuchs.com <mailto:sgoldschmidt@de.pepperl-fuchs.com> >>> <mailto:sgoldschmidt@de.pepperl-fuchs.com <mailto:sgoldschmidt@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.goldschmidt@gmail.com <mailto:simon.k.r.goldschmidt@gmail.com> >>> <mailto:simon.k.r.goldschmidt@gmail.com <mailto:simon.k.r.goldschmidt@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
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 <marex@denx.de mailto:marex@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 <sgoldschmidt@de.pepperl-fuchs.com <mailto:sgoldschmidt@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.goldschmidt@gmail.com <mailto:simon.k.r.goldschmidt@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 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
participants (3)
-
Dalon L Westergreen -
Marek Vasut -
Simon Goldschmidt