[U-Boot,RFC,1/3] sunxi: Extend SPL header versioning

On Allwinner SoCs we use some free bytes at the beginning of the SPL image
to store various information. We have a version byte to allow updates,
but changing this always requires all tools to be updated as well.

Introduce the concept of semantic versioning [1] to the SPL header:
The major part of the version number only changes on incompatible
updates, a minor number bump indicates backward compatibility.
This patch just documents the major/minor split, adds some comments
to the header file and uses the versioning information for the existing
users.

[1] https://semver.org

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arch/arm/include/asm/arch-sunxi/spl.h | 19 ++++++++++++++-----
 board/sunxi/board.c                   |  4 ++--
 2 files changed, 16 insertions(+), 7 deletions(-)

On Thu, 17 May 2018 09:16:59 +0100
Andre Przywara <andre.przywara@arm.com> wrote:

> On Allwinner SoCs we use some free bytes at the beginning of the SPL image
> to store various information. We have a version byte to allow updates,
> but changing this always requires all tools to be updated as well.

The tools do not need to be updated together with U-Boot even now.

U-Boot may freely increment the SPL version number as long as the new
header is a superset of the old one.

> Introduce the concept of semantic versioning [1] to the SPL header:
> The major part of the version number only changes on incompatible
> updates, a minor number bump indicates backward compatibility.
> This patch just documents the major/minor split, adds some comments
> to the header file and uses the versioning information for the existing
> users.
> 
> [1] https://semver.org

So basically you are implementing the versioning scheme that I proposed
back in 2015:
    https://lists.denx.de/pipermail/u-boot/2015-September/228727.html

Hans de Goede thought that the major/minor versioning was too complex
and unnecessary (if I remember correctly, we had several discussion
threads which concluded in the same way), so we did not implement it
explicitly back then. But a potential non-compatible SPL header upgrade
still could be handled, albeit less gracefully:

    Yes, we can also always change the SPL header signature in the case
    of introducing incompatible changes. But the down side is that the
    "fel" tool would treat this situation as "unknown bootloader"
    instead of "incompatible U-Boot".

> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

In general, improvements in this area are welcome. Just some
comments below.

> ---
>  arch/arm/include/asm/arch-sunxi/spl.h | 19 ++++++++++++++-----
>  board/sunxi/board.c                   |  4 ++--
>  2 files changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm/include/asm/arch-sunxi/spl.h b/arch/arm/include/asm/arch-sunxi/spl.h
> index 4277d836e5..7cf89c8db2 100644
> --- a/arch/arm/include/asm/arch-sunxi/spl.h
> +++ b/arch/arm/include/asm/arch-sunxi/spl.h
> @@ -9,7 +9,16 @@
>  
>  #define BOOT0_MAGIC		"eGON.BT0"
>  #define SPL_SIGNATURE		"SPL" /* marks "sunxi" SPL header */
> -#define SPL_HEADER_VERSION	2
> +#define SPL_MAJOR_BITS		3
> +#define SPL_MINOR_BITS		5
> +#define SPL_VERSION(maj, min)						\
> +	((((maj) & ((1U << SPL_MAJOR_BITS) - 1)) << SPL_MINOR_BITS) | \
> +	((min) & ((1U << SPL_MINOR_BITS) - 1)))
> +
> +#define SPL_HEADER_VERSION	SPL_VERSION(0, 2)
> +
> +#define SPL_ENV_HEADER_VERSION	SPL_VERSION(0, 1)
> +#define SPL_DT_HEADER_VERSION	SPL_VERSION(0, 2)
>  
>  #ifdef CONFIG_SUNXI_HIGH_SRAM
>  #define SPL_ADDR		0x10000
> @@ -49,14 +58,14 @@ struct boot_file_head {
>  		uint32_t pub_head_size;
>  		uint8_t spl_signature[4];
>  	};
> -	uint32_t fel_script_address;
> +	uint32_t fel_script_address;	/* since v0.1, set by sunxi-fel */

Thanks, it's nice to have these comments about the versions where these
features were introduced.

>  	/*
>  	 * If the fel_uEnv_length member below is set to a non-zero value,
>  	 * it specifies the size (byte count) of data at fel_script_address.
>  	 * At the same time this indicates that the data is in uEnv.txt
>  	 * compatible format, ready to be imported via "env import -t".
>  	 */
> -	uint32_t fel_uEnv_length;
> +	uint32_t fel_uEnv_length;	/* since v0.1, set by sunxi-fel */
>  	/*
>  	 * Offset of an ASCIIZ string (relative to the SPL header), which
>  	 * contains the default device tree name (CONFIG_DEFAULT_DEVICE_TREE).
> @@ -64,11 +73,11 @@ struct boot_file_head {
>  	 * by flash programming tools for providing nice informative messages
>  	 * to the users.
>  	 */
> -	uint32_t dt_name_offset;
> +	uint32_t dt_name_offset;	/* since v0.2, set by mksunxiboot */
>  	uint32_t reserved1;
>  	uint32_t boot_media;		/* written here by the boot ROM */
>  	/* A padding area (may be used for storing text strings) */
> -	uint32_t string_pool[13];
> +	uint32_t string_pool[13];	/* since v0.2, filled by mksunxiboot */

The 0.2 version of the SPL header does not specify the exact
format of this 'string_pool' padding area. So I think that this
comment is unnecessary.

In principle, the device tree name string can be stored even
somewhere in the const data section of the SPL binary and
referenced from the SPL header. Not that it makes any sense to
do this, but it is technically possible.

>  	/* The header must be a multiple of 32 bytes (for VBAR alignment) */
>  };
>  
> diff --git a/board/sunxi/board.c b/board/sunxi/board.c
> index 3d364c6db5..a105d0a5ab 100644
> --- a/board/sunxi/board.c
> +++ b/board/sunxi/board.c
> @@ -631,9 +631,9 @@ static void parse_spl_header(const uint32_t spl_addr)
>  		return; /* signature mismatch, no usable header */
>  
>  	uint8_t spl_header_version = spl->spl_signature[3];
> -	if (spl_header_version != SPL_HEADER_VERSION) {
> +	if (spl_header_version < SPL_ENV_HEADER_VERSION) {

We have already discussed this particular part of code earlier:
    https://lists.denx.de/pipermail/u-boot/2015-September/227999.html

Essentially, unless we have a real use case for mixing the SPL and the
main U-Boot parts built from different U-Boot versions, I don't see any
purpose for doing anything other than just exact version check here.

If this particular check fails (the SPL part does not match the main
U-Boot part), then something is already very wrong.

>  		printf("sunxi SPL version mismatch: expected %u, got %u\n",
> -		       SPL_HEADER_VERSION, spl_header_version);
> +		       SPL_ENV_HEADER_VERSION, spl_header_version);
>  		return;
>  	}
>  	if (!spl->fel_script_address)

于 2018年5月17日 GMT+08:00 下午7:05:15, Siarhei Siamashka <siarhei.siamashka@gmail.com> 写到:
>On Thu, 17 May 2018 09:16:59 +0100
>Andre Przywara <andre.przywara@arm.com> wrote:
>
>> On Allwinner SoCs we use some free bytes at the beginning of the SPL
>image
>> to store various information. We have a version byte to allow
>updates,
>> but changing this always requires all tools to be updated as well.
>
>The tools do not need to be updated together with U-Boot even now.
>
>U-Boot may freely increment the SPL version number as long as the new
>header is a superset of the old one.

But now sunxi-fel will work when SPL ver not recognized.

>
>> Introduce the concept of semantic versioning [1] to the SPL header:
>> The major part of the version number only changes on incompatible
>> updates, a minor number bump indicates backward compatibility.
>> This patch just documents the major/minor split, adds some comments
>> to the header file and uses the versioning information for the
>existing
>> users.
>> 
>> [1] https://semver.org
>
>So basically you are implementing the versioning scheme that I proposed
>back in 2015:
>    https://lists.denx.de/pipermail/u-boot/2015-September/228727.html
>
>Hans de Goede thought that the major/minor versioning was too complex
>and unnecessary (if I remember correctly, we had several discussion
>threads which concluded in the same way), so we did not implement it
>explicitly back then. But a potential non-compatible SPL header upgrade
>still could be handled, albeit less gracefully:
>
>    Yes, we can also always change the SPL header signature in the case
>    of introducing incompatible changes. But the down side is that the
>    "fel" tool would treat this situation as "unknown bootloader"
>    instead of "incompatible U-Boot".
>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>
>In general, improvements in this area are welcome. Just some
>comments below.
>
>> ---
>>  arch/arm/include/asm/arch-sunxi/spl.h | 19 ++++++++++++++-----
>>  board/sunxi/board.c                   |  4 ++--
>>  2 files changed, 16 insertions(+), 7 deletions(-)
>> 
>> diff --git a/arch/arm/include/asm/arch-sunxi/spl.h
>b/arch/arm/include/asm/arch-sunxi/spl.h
>> index 4277d836e5..7cf89c8db2 100644
>> --- a/arch/arm/include/asm/arch-sunxi/spl.h
>> +++ b/arch/arm/include/asm/arch-sunxi/spl.h
>> @@ -9,7 +9,16 @@
>>  
>>  #define BOOT0_MAGIC		"eGON.BT0"
>>  #define SPL_SIGNATURE		"SPL" /* marks "sunxi" SPL header */
>> -#define SPL_HEADER_VERSION	2
>> +#define SPL_MAJOR_BITS		3
>> +#define SPL_MINOR_BITS		5
>> +#define SPL_VERSION(maj, min)						\
>> +	((((maj) & ((1U << SPL_MAJOR_BITS) - 1)) << SPL_MINOR_BITS) | \
>> +	((min) & ((1U << SPL_MINOR_BITS) - 1)))
>> +
>> +#define SPL_HEADER_VERSION	SPL_VERSION(0, 2)
>> +
>> +#define SPL_ENV_HEADER_VERSION	SPL_VERSION(0, 1)
>> +#define SPL_DT_HEADER_VERSION	SPL_VERSION(0, 2)
>>  
>>  #ifdef CONFIG_SUNXI_HIGH_SRAM
>>  #define SPL_ADDR		0x10000
>> @@ -49,14 +58,14 @@ struct boot_file_head {
>>  		uint32_t pub_head_size;
>>  		uint8_t spl_signature[4];
>>  	};
>> -	uint32_t fel_script_address;
>> +	uint32_t fel_script_address;	/* since v0.1, set by sunxi-fel */
>
>Thanks, it's nice to have these comments about the versions where these
>features were introduced.
>
>>  	/*
>>  	 * If the fel_uEnv_length member below is set to a non-zero value,
>>  	 * it specifies the size (byte count) of data at
>fel_script_address.
>>  	 * At the same time this indicates that the data is in uEnv.txt
>>  	 * compatible format, ready to be imported via "env import -t".
>>  	 */
>> -	uint32_t fel_uEnv_length;
>> +	uint32_t fel_uEnv_length;	/* since v0.1, set by sunxi-fel */
>>  	/*
>>  	 * Offset of an ASCIIZ string (relative to the SPL header), which
>>  	 * contains the default device tree name
>(CONFIG_DEFAULT_DEVICE_TREE).
>> @@ -64,11 +73,11 @@ struct boot_file_head {
>>  	 * by flash programming tools for providing nice informative
>messages
>>  	 * to the users.
>>  	 */
>> -	uint32_t dt_name_offset;
>> +	uint32_t dt_name_offset;	/* since v0.2, set by mksunxiboot */
>>  	uint32_t reserved1;
>>  	uint32_t boot_media;		/* written here by the boot ROM */
>>  	/* A padding area (may be used for storing text strings) */
>> -	uint32_t string_pool[13];
>> +	uint32_t string_pool[13];	/* since v0.2, filled by mksunxiboot */
>
>The 0.2 version of the SPL header does not specify the exact
>format of this 'string_pool' padding area. So I think that this
>comment is unnecessary.
>
>In principle, the device tree name string can be stored even
>somewhere in the const data section of the SPL binary and
>referenced from the SPL header. Not that it makes any sense to
>do this, but it is technically possible.
>
>>  	/* The header must be a multiple of 32 bytes (for VBAR alignment)
>*/
>>  };
>>  
>> diff --git a/board/sunxi/board.c b/board/sunxi/board.c
>> index 3d364c6db5..a105d0a5ab 100644
>> --- a/board/sunxi/board.c
>> +++ b/board/sunxi/board.c
>> @@ -631,9 +631,9 @@ static void parse_spl_header(const uint32_t
>spl_addr)
>>  		return; /* signature mismatch, no usable header */
>>  
>>  	uint8_t spl_header_version = spl->spl_signature[3];
>> -	if (spl_header_version != SPL_HEADER_VERSION) {
>> +	if (spl_header_version < SPL_ENV_HEADER_VERSION) {
>
>We have already discussed this particular part of code earlier:
>    https://lists.denx.de/pipermail/u-boot/2015-September/227999.html
>
>Essentially, unless we have a real use case for mixing the SPL and the
>main U-Boot parts built from different U-Boot versions, I don't see any
>purpose for doing anything other than just exact version check here.
>
>If this particular check fails (the SPL part does not match the main
>U-Boot part), then something is already very wrong.
>
>>  		printf("sunxi SPL version mismatch: expected %u, got %u\n",
>> -		       SPL_HEADER_VERSION, spl_header_version);
>> +		       SPL_ENV_HEADER_VERSION, spl_header_version);
>>  		return;
>>  	}
>>  	if (!spl->fel_script_address)

Hi,

On 17/05/18 12:05, Siarhei Siamashka wrote:
> On Thu, 17 May 2018 09:16:59 +0100
> Andre Przywara <andre.przywara@arm.com> wrote:
> 
>> On Allwinner SoCs we use some free bytes at the beginning of the SPL image
>> to store various information. We have a version byte to allow updates,
>> but changing this always requires all tools to be updated as well.
> 
> The tools do not need to be updated together with U-Boot even now.
> 
> U-Boot may freely increment the SPL version number as long as the new
> header is a superset of the old one.
> 
>> Introduce the concept of semantic versioning [1] to the SPL header:
>> The major part of the version number only changes on incompatible
>> updates, a minor number bump indicates backward compatibility.
>> This patch just documents the major/minor split, adds some comments
>> to the header file and uses the versioning information for the existing
>> users.
>>
>> [1] https://semver.org
> 
> So basically you are implementing the versioning scheme that I proposed
> back in 2015:
>     https://lists.denx.de/pipermail/u-boot/2015-September/228727.html

Ah, sorry, that predates my sunxi involvement ;-)

> Hans de Goede thought that the major/minor versioning was too complex
> and unnecessary (if I remember correctly, we had several discussion
> threads which concluded in the same way), so we did not implement it
> explicitly back then. But a potential non-compatible SPL header upgrade
> still could be handled, albeit less gracefully:
> 
>     Yes, we can also always change the SPL header signature in the case
>     of introducing incompatible changes. But the down side is that the
>     "fel" tool would treat this situation as "unknown bootloader"
>     instead of "incompatible U-Boot".
> 
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> 
> In general, improvements in this area are welcome. Just some
> comments below.
> 
>> ---
>>  arch/arm/include/asm/arch-sunxi/spl.h | 19 ++++++++++++++-----
>>  board/sunxi/board.c                   |  4 ++--
>>  2 files changed, 16 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/arch-sunxi/spl.h b/arch/arm/include/asm/arch-sunxi/spl.h
>> index 4277d836e5..7cf89c8db2 100644
>> --- a/arch/arm/include/asm/arch-sunxi/spl.h
>> +++ b/arch/arm/include/asm/arch-sunxi/spl.h
>> @@ -9,7 +9,16 @@
>>  
>>  #define BOOT0_MAGIC		"eGON.BT0"
>>  #define SPL_SIGNATURE		"SPL" /* marks "sunxi" SPL header */
>> -#define SPL_HEADER_VERSION	2
>> +#define SPL_MAJOR_BITS		3
>> +#define SPL_MINOR_BITS		5
>> +#define SPL_VERSION(maj, min)						\
>> +	((((maj) & ((1U << SPL_MAJOR_BITS) - 1)) << SPL_MINOR_BITS) | \
>> +	((min) & ((1U << SPL_MINOR_BITS) - 1)))
>> +
>> +#define SPL_HEADER_VERSION	SPL_VERSION(0, 2)
>> +
>> +#define SPL_ENV_HEADER_VERSION	SPL_VERSION(0, 1)
>> +#define SPL_DT_HEADER_VERSION	SPL_VERSION(0, 2)
>>  
>>  #ifdef CONFIG_SUNXI_HIGH_SRAM
>>  #define SPL_ADDR		0x10000
>> @@ -49,14 +58,14 @@ struct boot_file_head {
>>  		uint32_t pub_head_size;
>>  		uint8_t spl_signature[4];
>>  	};
>> -	uint32_t fel_script_address;
>> +	uint32_t fel_script_address;	/* since v0.1, set by sunxi-fel */
> 
> Thanks, it's nice to have these comments about the versions where these
> features were introduced.
> 
>>  	/*
>>  	 * If the fel_uEnv_length member below is set to a non-zero value,
>>  	 * it specifies the size (byte count) of data at fel_script_address.
>>  	 * At the same time this indicates that the data is in uEnv.txt
>>  	 * compatible format, ready to be imported via "env import -t".
>>  	 */
>> -	uint32_t fel_uEnv_length;
>> +	uint32_t fel_uEnv_length;	/* since v0.1, set by sunxi-fel */
>>  	/*
>>  	 * Offset of an ASCIIZ string (relative to the SPL header), which
>>  	 * contains the default device tree name (CONFIG_DEFAULT_DEVICE_TREE).
>> @@ -64,11 +73,11 @@ struct boot_file_head {
>>  	 * by flash programming tools for providing nice informative messages
>>  	 * to the users.
>>  	 */
>> -	uint32_t dt_name_offset;
>> +	uint32_t dt_name_offset;	/* since v0.2, set by mksunxiboot */
>>  	uint32_t reserved1;
>>  	uint32_t boot_media;		/* written here by the boot ROM */
>>  	/* A padding area (may be used for storing text strings) */
>> -	uint32_t string_pool[13];
>> +	uint32_t string_pool[13];	/* since v0.2, filled by mksunxiboot */
> 
> The 0.2 version of the SPL header does not specify the exact
> format of this 'string_pool' padding area. So I think that this
> comment is unnecessary.

Well, I understand that it *could* be everywhere, but I wanted to give a
heads up to the reader that the actual mksunxiboot implementation fills
that area, so it's not really vacant anymore. That may become of
importance if we need more fields.

> In principle, the device tree name string can be stored even
> somewhere in the const data section of the SPL binary and
> referenced from the SPL header. Not that it makes any sense to
> do this, but it is technically possible.
> 
>>  	/* The header must be a multiple of 32 bytes (for VBAR alignment) */
>>  };
>>  
>> diff --git a/board/sunxi/board.c b/board/sunxi/board.c
>> index 3d364c6db5..a105d0a5ab 100644
>> --- a/board/sunxi/board.c
>> +++ b/board/sunxi/board.c
>> @@ -631,9 +631,9 @@ static void parse_spl_header(const uint32_t spl_addr)
>>  		return; /* signature mismatch, no usable header */
>>  
>>  	uint8_t spl_header_version = spl->spl_signature[3];
>> -	if (spl_header_version != SPL_HEADER_VERSION) {
>> +	if (spl_header_version < SPL_ENV_HEADER_VERSION) {
> 
> We have already discussed this particular part of code earlier:
>     https://lists.denx.de/pipermail/u-boot/2015-September/227999.html
> 
> Essentially, unless we have a real use case for mixing the SPL and the
> main U-Boot parts built from different U-Boot versions, I don't see any
> purpose for doing anything other than just exact version check here.

For FEL booting we might trigger this. My personal use case is to have
FEL capable 32-bit SPL binaries for the ARMv8 SoCs lying around[2], and
using them with any version of U-Boot proper I want to test/debug.
So I hit this there. I see that this is quite a stretch, but this "<"
relation is what the code actually requires, especially now with the
semantic versioning, so I changed that.

Cheers,
Andre.

[2] https://github.com/apritzel/pine64/tree/master/binaries

> If this particular check fails (the SPL part does not match the main
> U-Boot part), then something is already very wrong.
> 
>>  		printf("sunxi SPL version mismatch: expected %u, got %u\n",
>> -		       SPL_HEADER_VERSION, spl_header_version);
>> +		       SPL_ENV_HEADER_VERSION, spl_header_version);
>>  		return;
>>  	}
>>  	if (!spl->fel_script_address)
>