arm: socfpga: crc-protect SPL on warm boot
Subsystems (3)
PATCH 1/1 arm: socfpga: crc-protect SPL on warm boot
From: Simon Goldschmidt <[email protected]>
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.
---
arch/arm/mach-socfpga/misc_gen5.c | 9 ---- arch/arm/mach-socfpga/spl_gen5.c | 73 +++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 9 deletions(-)
arch/arm/mach-socfpga/misc_gen5.c +0 -9
@@ -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]);
arch/arm/mach-socfpga/spl_gen5.c +73 -0
85 lines not on this page · raw message
Signature · 1 line
2.17.1
PATCH 1/1 arm: socfpga: crc-protect SPL on warm boot
Quoted 137 lines · Simon Goldschmidt
From: Simon Goldschmidt <[email protected]>
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_enableregister inarch_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_crcregister 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 <[email protected]>---
arch/arm/mach-socfpga/misc_gen5.c | 9 ---- arch/arm/mach-socfpga/spl_gen5.c | 73 +++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 9 deletions(-)arch/arm/mach-socfpga/misc_gen5.c +0
-9@@ -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]);arch/arm/mach-socfpga/spl_gen5.c +70
-0@@ -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 ();
arch/arm/mach-socfpga/spl_gen5.c void board_init_f(ulong dummy) · line 160
Quoted 9 lines
arch/arm/mach-socfpga/spl_gen5.c +3
-0@@ -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 ?
Signature · 2 lines
Best regards, Marek Vasut
PATCH 1/1 arm: socfpga: crc-protect SPL on warm boot
Quoted 140 lines · Marek Vasut
Quoted 139 lines · Simon Goldschmidt
From: Simon Goldschmidt <[email protected]>
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_enableregister inarch_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_crcregister 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 <[email protected]>---
arch/arm/mach-socfpga/misc_gen5.c | 9 ---- arch/arm/mach-socfpga/spl_gen5.c | 73 +++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 9 deletions(-)arch/arm/mach-socfpga/misc_gen5.c +0
-9@@ -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]);arch/arm/mach-socfpga/spl_gen5.c +70
-0@@ -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);
Quoted 8 lines
arch/arm/mach-socfpga/spl_gen5.c void board_init_f(ulong dummy) · line 160
Quoted 7 lines
arch/arm/mach-socfpga/spl_gen5.c +3
-0+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
PATCH 1/1 arm: socfpga: crc-protect SPL on warm boot
Quoted 6 lines · Simon Goldschmidt
Quoted 5 lines · Marek Vasut
Quoted 4 lines · Simon Goldschmidt
From: Simon Goldschmidt <[email protected]>
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?
Quoted 164 lines
Quoted 147 lines
Quoted 147 lines
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_enableregister inarch_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_crcregister 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 <[email protected]>---
arch/arm/mach-socfpga/misc_gen5.c | 9 ---- arch/arm/mach-socfpga/spl_gen5.c | 73 +++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 9 deletions(-)arch/arm/mach- renamed from arch/arm/mach-socfpga/misc_gen5.c +0
-0
socfpga/misc_gen5.c
index5fa40937c4..492a3082de100644arch/arm/mach-socfpga/misc_gen5.c +0
-9@@ -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-Quoted 1 line
iswgrp_handoff[i]);
arch/arm/mach- renamed from arch/arm/mach-socfpga/spl_gen5.c +0
-0
socfpga/spl_gen5.c
indexccdc661d05..3416e19f79100644arch/arm/mach-socfpga/spl_gen5.c +34
-0@@ -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).
+ */arch/arm/mach-socfpga/spl_gen5.c +28
-0+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();arch/arm/mach-socfpga/spl_gen5.c +0
-0@@ -82,6 +152,9 @@ void board_init_f(ulong dummy)writel (SYSMGR_ECC_OCRAM_DERR |SYSMGR_ECC_OCRAM_EN , &sysmgr_regs->eccgrp_ocram);Quoted 8 lines
arch/arm/mach-socfpga/spl_gen5.c void board_init_f(ulong dummy) · line 160
Quoted 7 lines
arch/arm/mach-socfpga/spl_gen5.c +3
-0+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 at lists.denx.de
https://lists.denx.de/listinfo/u-boot
PATCH 1/1 arm: socfpga: crc-protect SPL on warm boot
Quoted 9 lines · Dalon L Westergreen
Quoted 6 lines · Simon Goldschmidt
Quoted 5 lines · Marek Vasut
Quoted 4 lines · Simon Goldschmidt
From: Simon Goldschmidt <[email protected]>
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) ?
Signature · 2 lines
Best regards, Marek Vasut
PATCH 1/1 arm: socfpga: crc-protect SPL on warm boot
Am Mi., 21. Nov. 2018, 00:12 hat Marek Vasut <[email protected]> geschrieben:
Quoted 6 lines · Dalon L Westergreen
Quoted 5 lines · Simon Goldschmidt
Quoted 4 lines · Marek Vasut
Quoted 3 lines · Simon Goldschmidt
Quoted 2 lines
From: Simon Goldschmidt <[email protected]>
Quoted 9 lines
Quoted 4 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
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
Quoted 1 line
PATCH 1/1 arm: socfpga: crc-protect SPL on warm boot
Quoted 17 lines · Simon Goldschmidt
Am Mi., 21. Nov. 2018, 00:12 hat Marek Vasut <[email protected] <mailto:[email protected]>> 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 <[email protected] <mailto:[email protected]>>
>>>>
>>>> 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?
Quoted 8 lines
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 :-)
Quoted 2 lines
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
Signature · 2 lines
Best regards, Marek Vasut
PATCH 1/1 arm: socfpga: crc-protect SPL on warm boot
In reply to Simon GoldschmidtQuoted 155 lines · Simon Goldschmidt
Quoted 145 lines · Marek Vasut
Quoted 144 lines · Simon Goldschmidt
From: Simon Goldschmidt <[email protected]>
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_enableregister inarch_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_crcregister 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 <[email protected]>---
arch/arm/mach-socfpga/misc_gen5.c| 9 ---- arch/arm/mach-socfpga/spl_gen5.c | 73 +++++++++++++++++++++++++++++++2 files changed, 73 insertions(+), 9 deletions(-)arch/arm/mach-socfpga/misc_gen5.c +0
-0@@ -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 --gita/arch/arm/mach-socfpga/spl_gen5.cb/arch/arm/mach-socfpga/spl_gen5.c
indexccdc661d05..3416e19f79100644arch/arm/mach-socfpga/spl_gen5.c +0
-0@@ -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 u32socfpga_boot_crc(u32 crc, const unsigned char *ptr, u32 length)arch/arm/mach-socfpga/spl_gen5.c +32
-0+{ + 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).
+ */arch/arm/mach-socfpga/spl_gen5.c +24
-0+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);arch/arm/mach-socfpga/spl_gen5.c +2
-0+} +void
board_init_f(ulong dummy)
{
const structcm_config*cm_default_cfg=cm_get_default_config();arch/arm/mach-socfpga/spl_gen5.c +0
-0@@ -82,6 +152,9 @@ void board_init_f(ulong dummy) writel(SYSMGR_ECC_OCRAM_DERR | SYSMGR_ECC_OCRAM_EN,Quoted 8 lines
arch/arm/mach-socfpga/spl_gen5.c void board_init_f(ulong dummy)
Quoted 7 lines
arch/arm/mach-socfpga/spl_gen5.c +2
-0&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 ?
Quoted 2 lines
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 ...
Signature · 2 lines
Best regards, Marek Vasut
PATCH 1/1 arm: socfpga: crc-protect SPL on warm boot
Am Mi., 21. Nov. 2018, 00:11 hat Marek Vasut <[email protected]> geschrieben:
Quoted 157 lines · Simon Goldschmidt
Quoted 26 lines · Marek Vasut
Quoted 25 lines · Simon Goldschmidt
Quoted 24 lines
From: Simon Goldschmidt <[email protected]>
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_enableregister inarch_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_crcregister 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.Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
This whole patch added 96 bytes to .text for SPL for
socfpga_socrates_defconfig.Signed-off-by Simon Goldschmidt <[email protected]>---
arch/arm/mach-socfpga/misc_gen5.c | 9 ---- arch/arm/mach-socfpga/spl_gen5.c | 73+++++++++++++++++++++++++++++++
Quoted 121 lines
Quoted 120 lines
Quoted 111 lines
2 files changed, 73 insertions(+), 9 deletions(-)arch/arm/mach-socfpga/misc_gen5.c +0
-8@@ -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
indexccdc661d05..3416e19f79100644arch/arm/mach-socfpga/spl_gen5.c +0
-0@@ -63,6 +63,76 @@ u32 spl_boot_mode(const u32 boot_device) }#endif +/* This function calculates the CRC32 used by the Cyclone 5 SoCBoot Rom */
+static u32socfpga_boot_crc(u32 crc, const unsigned char *ptr, u32 length)arch/arm/mach-socfpga/spl_gen5.c +32
-0+{ +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).
+ */arch/arm/mach-socfpga/spl_gen5.c +24
-0+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);arch/arm/mach-socfpga/spl_gen5.c +4
-0+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 ();arch/arm/mach-socfpga/spl_gen5.c void board_init_f(ulong dummy) · line 159
Quoted 8 lines
arch/arm/mach-socfpga/spl_gen5.c +2
-0@@ -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 ?
Quoted 7 lines
Quoted 2 lines
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
Quoted 1 line
PATCH 1/1 arm: socfpga: crc-protect SPL on warm boot
Quoted 176 lines · Simon Goldschmidt
Am Mi., 21. Nov. 2018, 00:11 hat Marek Vasut <[email protected] <mailto:[email protected]>> 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 <[email protected] <mailto:[email protected]>> >>> >>> 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 <[email protected] <mailto:[email protected]>> >>> --- >>> >>> 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 ...
Quoted 2 lines
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.
Quoted 3 lines
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.
Signature · 2 lines
Best regards, Marek Vasut
PATCH 1/1 arm: socfpga: crc-protect SPL on warm boot
Quoted 176 lines · Marek Vasut
Quoted 175 lines · Simon Goldschmidt
Am Mi., 21. Nov. 2018, 00:11 hat Marek Vasut <[email protected] <mailto:[email protected]>> 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 <[email protected] <mailto:[email protected]>> >>> >>> 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 <[email protected] <mailto:[email protected]>> >>> --- >>> >>> 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 ...
Quoted 8 lines
Quoted 2 lines
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.
Quoted 3 lines
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
PATCH 1/1 arm: socfpga: crc-protect SPL on warm boot
Quoted 212 lines · Simon Goldschmidt
Quoted 211 lines · Marek Vasut
Quoted 203 lines · Simon Goldschmidt
Am Mi., 21. Nov. 2018, 00:11 hat Marek Vasut <[email protected] <mailto:[email protected]>> 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 <[email protected] <mailto:[email protected]>>
>>>
>>> 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 >>> thewarmramgrp_enableregister inarch_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 thewarmramgrp_crcregister 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 <[email protected] <mailto:[email protected]>>
>>> ---
>>>
>>>arch/arm/mach-socfpga/misc_gen5.c| 9 ----
>>> arch/arm/mach-socfpga/spl_gen5.c | 73
+++++++++++++++++++++++++++++++
>>> 2 files changed, 73 insertions(+), 9 deletions(-) >>>
>>> diff --gita/arch/arm/mach-socfpga/misc_gen5.c
>>>b/arch/arm/mach-socfpga/misc_gen5.c
>>> index5fa40937c4..492a3082de100644
>>> ---a/arch/arm/mach-socfpga/misc_gen5.c
>>> +++b/arch/arm/mach-socfpga/misc_gen5.c
>>> @@ -204,15 +204,6 @@ intarch_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 --gita/arch/arm/mach-socfpga/spl_gen5.c
>>>b/arch/arm/mach-socfpga/spl_gen5.c
>>> indexccdc661d05..3416e19f79100644
>>> ---a/arch/arm/mach-socfpga/spl_gen5.c
>>> +++b/arch/arm/mach-socfpga/spl_gen5.c
>>> @@ -63,6 +63,76 @@ u32spl_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 voidspl_init_reboot_config(void)
>>> +{
>>> + u32spl_crc,spl_length;
>>> + const u32spl_start= (u32)__image_copy_start; >>> + const u32spl_start_16=spl_start&0xffff; >>> + u32spl_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); >>> +}
>>> +
>>> voidboard_init_f(ulong dummy)
>>> {
>>> const structcm_config*cm_default_cfg=cm_get_default_config();
>>> @@ -82,6 +152,9 @@ voidboard_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 ...Quoted 2 lines
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.
Quoted 3 lines
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).
Quoted 7 lines
Quoted 1 line
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.cto 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!
Quoted 4 lines
The shock was that I thought
lib/crc32.cis 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 withCONFIG_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 ?
Quoted 3 lines
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
Quoted 2 lines
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)
Signature · 2 lines
Best regards, Marek Vasut
PATCH 1/1 arm: socfpga: crc-protect SPL on warm boot
Am Do., 22. Nov. 2018, 03:00 hat Marek Vasut <[email protected]> geschrieben:
Quoted 240 lines · Simon Goldschmidt
Quoted 6 lines · Marek Vasut
Quoted 5 lines · Simon Goldschmidt
Quoted 4 lines
Am Mi., 21. Nov. 2018, 00:11 hat Marek Vasut <[email protected] <mailto:[email protected]>> geschrieben:
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines · Simon Goldschmidt
> On 20.11.2018 20:33, Marek Vasut wrote: >> On 11/20/2018 08:22 PM, Simon Goldschmidt wrote: >>> From: Simon Goldschmidt <[email protected] <mailto:[email protected]>> >>> >>> 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 onchipram
Quoted 171 lines
Quoted 171 lines
Quoted 171 lines
>>> 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_enableregister inarch_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 thewarmramgrp_crcregister 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 <[email protected] <mailto:[email protected]>> >>> --- >>> >>> 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 unlocksupport 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 Cyclone5 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 supportfor 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())Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
>>> + 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
Quoted 16 lines
Quoted 16 lines
Quoted 11 lines
> 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.
Quoted 3 lines
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).
Quoted 7 lines
Quoted 1 line
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.cto 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!
Quoted 4 lines
The shock was that I thought
lib/crc32.cis 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 withCONFIG_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 ?
Quoted 3 lines
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 socfpgagen5).
Yikes
Quoted 9 lines
Quoted 2 lines
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
Quoted 1 line
PATCH 1/1 arm: socfpga: crc-protect SPL on warm boot
Quoted 278 lines · Simon Goldschmidt
Am Do., 22. Nov. 2018, 03:00 hat Marek Vasut <[email protected] <mailto:[email protected]>> 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 <[email protected] <mailto:[email protected]> >>> <mailto:marex at denx.de <mailto:[email protected]>>> 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 <[email protected] <mailto:[email protected]> >>> <mailto:[email protected] <mailto:[email protected]>>> >>> >>> >>> >>> 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 >>> <[email protected] <mailto:[email protected]> >>> <mailto:[email protected] <mailto:[email protected]>>> >>> >>> --- >>> >>> >>> >>> 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)
Quoted 4 lines
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.
Quoted 1 line
I'll send a patch.
Thanks
Signature · 2 lines
Best regards, Marek Vasut
PATCH 1/1 arm: socfpga: crc-protect SPL on warm boot
In reply to Marek VasutQuoted 176 lines · Marek Vasut
Quoted 175 lines · Simon Goldschmidt
Am Mi., 21. Nov. 2018, 00:11 hat Marek Vasut <[email protected] <mailto:[email protected]>> 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 <[email protected] <mailto:[email protected]>> >>> >>> 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 <[email protected] <mailto:[email protected]>> >>> --- >>> >>> 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 ...
Quoted 8 lines
Quoted 2 lines
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.
Quoted 3 lines
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