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

Thu May 17 11:32:04 UTC 2018

Hi,

On 17/05/18 12:05, Siarhei Siamashka wrote:
> On Thu, 17 May 2018 09:16:59 +0100
> Andre Przywara <andre.przywara at 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 at 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)
> 