[U-Boot] [PATCH] arm: socfpga: crc-protect SPL on warm boot

[U-Boot] [PATCH] arm: socfpga: crc-protect SPL on warm boot

[Simon Goldschmidt]

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@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();
-- 
2.17.1

[U-Boot] [PATCH] arm: socfpga: crc-protect SPL on warm boot

[Marek Vasut]

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 ?

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH] arm: socfpga: crc-protect SPL on warm boot

[Simon Goldschmidt]

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.

Simon

[U-Boot] [PATCH] arm: socfpga: crc-protect SPL on warm boot

[Dalon L Westergreen]

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;
> > > +			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.
> 
> Simon
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH] arm: socfpga: crc-protect SPL on warm boot

[Marek Vasut]

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 ...

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH] arm: socfpga: crc-protect SPL on warm boot

[Marek Vasut]

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) ?

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH] arm: socfpga: crc-protect SPL on warm boot

[Simon Goldschmidt]

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]);
> >>>   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.

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

>

[U-Boot] [PATCH] arm: socfpga: crc-protect SPL on warm boot

[Simon Goldschmidt]

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

>

[U-Boot] [PATCH] arm: socfpga: crc-protect SPL on warm boot

[Marek Vasut]

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

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH] arm: socfpga: crc-protect SPL on warm boot

[Marek Vasut]

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.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH] arm: socfpga: crc-protect SPL on warm boot

[Simon Goldschmidt]

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

[U-Boot] [PATCH] arm: socfpga: crc-protect SPL on warm boot

[Marek Vasut]

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)

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH] arm: socfpga: crc-protect SPL on warm boot

[Simon Goldschmidt]

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 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)
>

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

>

[U-Boot] [PATCH] arm: socfpga: crc-protect SPL on warm boot

[Marek Vasut]

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 at 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

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH] arm: socfpga: crc-protect SPL on warm boot

[Simon Goldschmidt]

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