[5/8] sunxi: board: Save the chosen DT name in the SPL header

This overwrites the name loaded from the SPL image. It will be different
if there was previously no name provided, or if a more accurate name was
determined by the board variant selection logic. This means that the DT
name in the SPL header now always matches the DT appended to U-Boot.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 board/sunxi/board.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

On 03/09/2020 06:07, Samuel Holland wrote:
> This overwrites the name loaded from the SPL image. It will be different
> if there was previously no name provided, or if a more accurate name was
> determined by the board variant selection logic. This means that the DT> name in the SPL header now always matches the DT appended to U-Boot.

That's a nice way of preserving all this fancy DT selection choices for
U-Boot proper!

> Signed-off-by: Samuel Holland <samuel@sholland.org>

One hint below, but nevertheless:

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

> ---
>  board/sunxi/board.c | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/board/sunxi/board.c b/board/sunxi/board.c
> index 3d64ed18664..eaa40a1ea96 100644
> --- a/board/sunxi/board.c
> +++ b/board/sunxi/board.c
> @@ -331,6 +331,21 @@ static const char *get_spl_dt_name(void)
>  	return NULL;
>  }
>  
> +static void set_spl_dt_name(const char *name)
> +{
> +	struct boot_file_head *spl = get_spl_header(SPL_ENV_HEADER_VERSION);
> +
> +	if (spl == INVALID_SPL_HEADER)
> +		return;
> +
> +	/* Promote the header version for U-Boot proper, if needed. */
> +	if (spl->spl_signature[3] < SPL_DT_HEADER_VERSION)
> +		spl->spl_signature[3] = SPL_DT_HEADER_VERSION;
> +
> +	strcpy((char *)&spl->string_pool, name);

Let's hope nobody ever optimises the strcpy() routine, as this might
break (when doing unaligned accesses) on device memory, as in this case.

Cheers,
Andre.

> +	spl->dt_name_offset = offsetof(struct boot_file_head, string_pool);
> +}
> +
>  int dram_init(void)
>  {
>  	struct boot_file_head *spl = get_spl_header(SPL_DRAM_HEADER_VERSION);
> @@ -904,6 +919,7 @@ int ft_board_setup(void *blob, struct bd_info *bd)
>  int board_fit_config_name_match(const char *name)
>  {
>  	const char *best_dt_name = get_spl_dt_name();
> +	int ret;
>  
>  #ifdef CONFIG_DEFAULT_DEVICE_TREE
>  	if (best_dt_name == NULL)
> @@ -941,6 +957,15 @@ int board_fit_config_name_match(const char *name)
>  	}
>  #endif
>  
> -	return strcmp(name, best_dt_name);
> +	ret = strcmp(name, best_dt_name);
> +
> +	/*
> +	 * If one of the FIT configurations matches the most accurate DT name,
> +	 * update the SPL header to provide that DT name to U-Boot proper.
> +	 */
> +	if (ret == 0)
> +		set_spl_dt_name(best_dt_name);
> +
> +	return ret;
>  }
>  #endif
>

On 9/21/20 7:41 PM, André Przywara wrote:
> On 03/09/2020 06:07, Samuel Holland wrote:
>> This overwrites the name loaded from the SPL image. It will be different
>> if there was previously no name provided, or if a more accurate name was
>> determined by the board variant selection logic. This means that the DT> name in the SPL header now always matches the DT appended to U-Boot.
> 
> That's a nice way of preserving all this fancy DT selection choices for
> U-Boot proper!
> 
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
> 
> One hint below, but nevertheless:
> 
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
> 
>> ---
>>  board/sunxi/board.c | 27 ++++++++++++++++++++++++++-
>>  1 file changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/board/sunxi/board.c b/board/sunxi/board.c
>> index 3d64ed18664..eaa40a1ea96 100644
>> --- a/board/sunxi/board.c
>> +++ b/board/sunxi/board.c
>> @@ -331,6 +331,21 @@ static const char *get_spl_dt_name(void)
>>  	return NULL;
>>  }
>>  
>> +static void set_spl_dt_name(const char *name)
>> +{
>> +	struct boot_file_head *spl = get_spl_header(SPL_ENV_HEADER_VERSION);
>> +
>> +	if (spl == INVALID_SPL_HEADER)
>> +		return;
>> +
>> +	/* Promote the header version for U-Boot proper, if needed. */
>> +	if (spl->spl_signature[3] < SPL_DT_HEADER_VERSION)
>> +		spl->spl_signature[3] = SPL_DT_HEADER_VERSION;
>> +
>> +	strcpy((char *)&spl->string_pool, name);
> 
> Let's hope nobody ever optimises the strcpy() routine, as this might
> break (when doing unaligned accesses) on device memory, as in this case.

Why would any part of SRAM A1 be mapped as device memory? If U-Boot maps its own
code as device memory, and then implements a strcpy that requires aligned
multi-byte access (i.e. cannot handle arbitrary naturally-aligned char *), then
this code here isn't the bug.

Cheers,
Samuel

> Cheers,
> Andre.
> 
>> +	spl->dt_name_offset = offsetof(struct boot_file_head, string_pool);
>> +}
>> +
>>  int dram_init(void)
>>  {
>>  	struct boot_file_head *spl = get_spl_header(SPL_DRAM_HEADER_VERSION);
>> @@ -904,6 +919,7 @@ int ft_board_setup(void *blob, struct bd_info *bd)
>>  int board_fit_config_name_match(const char *name)
>>  {
>>  	const char *best_dt_name = get_spl_dt_name();
>> +	int ret;
>>  
>>  #ifdef CONFIG_DEFAULT_DEVICE_TREE
>>  	if (best_dt_name == NULL)
>> @@ -941,6 +957,15 @@ int board_fit_config_name_match(const char *name)
>>  	}
>>  #endif
>>  
>> -	return strcmp(name, best_dt_name);
>> +	ret = strcmp(name, best_dt_name);
>> +
>> +	/*
>> +	 * If one of the FIT configurations matches the most accurate DT name,
>> +	 * update the SPL header to provide that DT name to U-Boot proper.
>> +	 */
>> +	if (ret == 0)
>> +		set_spl_dt_name(best_dt_name);
>> +
>> +	return ret;
>>  }
>>  #endif
>>
>

On 22/09/2020 02:12, Samuel Holland wrote:

Hi,

> On 9/21/20 7:41 PM, André Przywara wrote:
>> On 03/09/2020 06:07, Samuel Holland wrote:
>>> This overwrites the name loaded from the SPL image. It will be different
>>> if there was previously no name provided, or if a more accurate name was
>>> determined by the board variant selection logic. This means that the DT> name in the SPL header now always matches the DT appended to U-Boot.
>>
>> That's a nice way of preserving all this fancy DT selection choices for
>> U-Boot proper!
>>
>>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>>
>> One hint below, but nevertheless:
>>
>> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
>>
>>> ---
>>>  board/sunxi/board.c | 27 ++++++++++++++++++++++++++-
>>>  1 file changed, 26 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/board/sunxi/board.c b/board/sunxi/board.c
>>> index 3d64ed18664..eaa40a1ea96 100644
>>> --- a/board/sunxi/board.c
>>> +++ b/board/sunxi/board.c
>>> @@ -331,6 +331,21 @@ static const char *get_spl_dt_name(void)
>>>  	return NULL;
>>>  }
>>>  
>>> +static void set_spl_dt_name(const char *name)
>>> +{
>>> +	struct boot_file_head *spl = get_spl_header(SPL_ENV_HEADER_VERSION);
>>> +
>>> +	if (spl == INVALID_SPL_HEADER)
>>> +		return;
>>> +
>>> +	/* Promote the header version for U-Boot proper, if needed. */
>>> +	if (spl->spl_signature[3] < SPL_DT_HEADER_VERSION)
>>> +		spl->spl_signature[3] = SPL_DT_HEADER_VERSION;
>>> +
>>> +	strcpy((char *)&spl->string_pool, name);
>>
>> Let's hope nobody ever optimises the strcpy() routine, as this might
>> break (when doing unaligned accesses) on device memory, as in this case.
> 
> Why would any part of SRAM A1 be mapped as device memory? 

Because we do so in arch/arm/mach-sunxi/board.c:
static struct mm_region sunxi_mem_map[] = {
        {
                /* SRAM, MMIO regions */
                .virt = 0x0UL,
                .phys = 0x0UL,
                .size = 0x40000000UL,
                .attrs = PTE_BLOCK_MEMTYPE(MT_DEVICE_NGNRNE) |
                         PTE_BLOCK_NON_SHARE
        }, {


> If U-Boot maps its own code as device memory,

DRAM is mapped as normal cacheable memory (the line after those above).
So U-Boot's code and its own data are safe.

> and then implements a strcpy that requires aligned
> multi-byte access (i.e. cannot handle arbitrary naturally-aligned char *), then
> this code here isn't the bug.

The problem is that letting the compiler generate code that does access
non-normal memory is broken. Device memory needs to be accessed via MMIO
accessors only, everything else is architecturally wrong.
We are somewhat saved by the fact that SRAM, by its very nature,
implements memory semantics, so it supports the GRE semantics that the
compiler relies on. But unaligned accesses trap on device memory
nevertheless.
As I said, it probably works at the moment, under the current
conditions, but is fragile.

Cheers,
Andre

>>> +	spl->dt_name_offset = offsetof(struct boot_file_head, string_pool);
>>> +}
>>> +
>>>  int dram_init(void)
>>>  {
>>>  	struct boot_file_head *spl = get_spl_header(SPL_DRAM_HEADER_VERSION);
>>> @@ -904,6 +919,7 @@ int ft_board_setup(void *blob, struct bd_info *bd)
>>>  int board_fit_config_name_match(const char *name)
>>>  {
>>>  	const char *best_dt_name = get_spl_dt_name();
>>> +	int ret;
>>>  
>>>  #ifdef CONFIG_DEFAULT_DEVICE_TREE
>>>  	if (best_dt_name == NULL)
>>> @@ -941,6 +957,15 @@ int board_fit_config_name_match(const char *name)
>>>  	}
>>>  #endif
>>>  
>>> -	return strcmp(name, best_dt_name);
>>> +	ret = strcmp(name, best_dt_name);
>>> +
>>> +	/*
>>> +	 * If one of the FIT configurations matches the most accurate DT name,
>>> +	 * update the SPL header to provide that DT name to U-Boot proper.
>>> +	 */
>>> +	if (ret == 0)
>>> +		set_spl_dt_name(best_dt_name);
>>> +
>>> +	return ret;
>>>  }
>>>  #endif
>>>
>>
>