[PATCH 20/26 v6] spl: spl_nor: Move legacy image loading into spl_legacy.c

[Stefan Roese]

On 09.04.20 20:51, Daniel Schwierzeck wrote:
> 
> 
> Am 08.04.20 um 10:09 schrieb Stefan Roese:
>> Move the legacy image loading into spl_legacy.c. This makes it easier
>> to extend the legacy image handling with new features that other
>> SPL loaders might use (e.g. spl_spi.c etc).
>>
>> No functional change indended.
>>
>> Signed-off-by: Stefan Roese <sr at denx.de>
>> Cc: Weijie Gao <weijie.gao at mediatek.com>
>> Cc: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com>
>> ---
>> Changes in v6:
>> - New patch
>>
>>   common/spl/spl_legacy.c | 17 +++++++++++++++++
>>   common/spl/spl_nor.c    | 34 +++++++++++++++++-----------------
>>   include/spl.h           | 14 ++++++++++++++
>>   3 files changed, 48 insertions(+), 17 deletions(-)
>>
>> diff --git a/common/spl/spl_legacy.c b/common/spl/spl_legacy.c
>> index 772135193e..d0fc4b111e 100644
>> --- a/common/spl/spl_legacy.c
>> +++ b/common/spl/spl_legacy.c
>> @@ -51,3 +51,20 @@ int spl_parse_legacy_header(struct spl_image_info *spl_image,
>>   
>>   	return 0;
>>   }
>> +
>> +int spl_load_legacy_img(struct spl_image_info *spl_image,
>> +			const struct image_header *header,
>> +			uintptr_t dataptr, struct spl_load_info *info)
>> +{
>> +	int ret;
>> +
>> +	ret = spl_parse_image_header(spl_image, header);
>> +	if (ret)
>> +		return ret;
>> +
>> +	dataptr = (uintptr_t)header + sizeof(struct image_header);
> 
> how should this work? You call this function with
> 
> spl_load_legacy_img(spl_image, &hdr, dataptr, &load);
> 
> so "header" points to the copy on the stack. "dataptr" already holds the
> address of the SPL payload. If you overwrite "dataptr" now, it will
> point to the stack area after the header copy.

You are correct. I fat-fingered while rebasing the patches here. As you
will see, this assignment will be removed in the next patch [21/26].

I'll change it in v7.

> 
>> +	info->read(info, dataptr, spl_image->size,
>> +		   (void *)(unsigned long)spl_image->load_addr);
>> +
>> +	return 0;
>> +}
>> diff --git a/common/spl/spl_nor.c b/common/spl/spl_nor.c
>> index cb8e06fe1f..d55165b772 100644
>> --- a/common/spl/spl_nor.c
>> +++ b/common/spl/spl_nor.c
>> @@ -24,11 +24,8 @@ unsigned long __weak spl_nor_get_uboot_base(void)
>>   static int spl_nor_load_image(struct spl_image_info *spl_image,
>>   			      struct spl_boot_device *bootdev)
>>   {
>> -	int ret;
>>   	__maybe_unused const struct image_header *header;
>>   	__maybe_unused struct spl_load_info load;
>> -	struct image_header hdr;
>> -	uintptr_t dataptr;
>>   
>>   	/*
>>   	 * Loading of the payload to SDRAM is done with skipping of
>> @@ -45,6 +42,8 @@ static int spl_nor_load_image(struct spl_image_info *spl_image,
>>   		header = (const struct image_header *)CONFIG_SYS_OS_BASE;
>>   #ifdef CONFIG_SPL_LOAD_FIT
>>   		if (image_get_magic(header) == FDT_MAGIC) {
>> +			int ret;
>> +
>>   			debug("Found FIT\n");
>>   			load.bl_len = 1;
>>   			load.read = spl_nor_load_read;
>> @@ -63,6 +62,7 @@ static int spl_nor_load_image(struct spl_image_info *spl_image,
>>   #endif
>>   		if (image_get_os(header) == IH_OS_LINUX) {
>>   			/* happy - was a Linux */
>> +			int ret;
>>   
>>   			ret = spl_parse_image_header(spl_image, header);
>>   			if (ret)
>> @@ -95,11 +95,9 @@ static int spl_nor_load_image(struct spl_image_info *spl_image,
>>   		debug("Found FIT format U-Boot\n");
>>   		load.bl_len = 1;
>>   		load.read = spl_nor_load_read;
>> -		ret = spl_load_simple_fit(spl_image, &load,
>> -					  spl_nor_get_uboot_base(),
>> -					  (void *)header);
>> -
>> -		return ret;
>> +		return spl_load_simple_fit(spl_image, &load,
>> +					   spl_nor_get_uboot_base(),
>> +					   (void *)header);
>>   	}
>>   #endif
>>   	if (IS_ENABLED(CONFIG_SPL_LOAD_IMX_CONTAINER)) {
>> @@ -109,17 +107,19 @@ static int spl_nor_load_image(struct spl_image_info *spl_image,
>>   					      spl_nor_get_uboot_base());
>>   	}
>>   
>> -	ret = spl_parse_image_header(spl_image,
>> -			(const struct image_header *)spl_nor_get_uboot_base());
>> -	if (ret)
>> -		return ret;
>> +	if (IS_ENABLED(CONFIG_SPL_LEGACY_IMAGE_SUPPORT)) {
>> +		struct image_header hdr;
>> +		uintptr_t dataptr;
>>   
>> -	/* Payload image may not be aligned, so copy it for safety */
>> -	memcpy(&hdr, (void *)spl_nor_get_uboot_base(), sizeof(hdr));
>> -	dataptr = spl_nor_get_uboot_base() + sizeof(struct image_header);
>> +		/* Payload image may not be aligned, so copy it for safety */
>> +		memcpy(&hdr, (void *)spl_nor_get_uboot_base(), sizeof(hdr));
>> +		dataptr = spl_nor_get_uboot_base() + sizeof(hdr);
>>   
>> -	memcpy((void *)(unsigned long)spl_image->load_addr,
>> -	       (void *)dataptr, spl_image->size);
>> +		/* Legacy image handling */
>> +		load.bl_len = 1;
>> +		load.read = spl_nor_load_read;
>> +		return spl_load_legacy_img(spl_image, &hdr, dataptr, &load);
>> +	}
> 
> I don't understand it fully. Is the purpose of the header copy to avoid
> unaligned access to the header in spl_parse_image_header()?

Yes.

> If so then the copy should be made in spl_load_legacy_img() itself.

Thats what I had implemented in an internal version already. Not sure,
why I switched to not using it in this v6? I'll re-think about it and
will perhaps move it to spl_load_legacy_img() again.

Thanks,
Stefan