[PATCH v5 09/11] spl: Convert semihosting to spl_load

[Sean Anderson]

On 8/3/23 04:36, Xavier Drudis Ferran wrote:
> El Mon, Jul 31, 2023 at 06:43:01PM -0400, Sean Anderson deia:
>> This converts the semihosting load method to use spl_load. As a result, it
>> also adds support for LOAD_FIT_FULL and IMX images.
>> 
>> Signed-off-by: Sean Anderson <sean.anderson at seco.com>
>> ---
>> 
>> Changes in v5:
>> - Rework to load header in spl_load
>> 
>> Changes in v2:
>> - New
>> 
>>  common/spl/spl_semihosting.c | 43 ++++++++++++------------------------
>>  1 file changed, 14 insertions(+), 29 deletions(-)
>> 
>> diff --git a/common/spl/spl_semihosting.c b/common/spl/spl_semihosting.c
>> index 5b5e842a11..e7cb9f4ce6 100644
>> --- a/common/spl/spl_semihosting.c
>> +++ b/common/spl/spl_semihosting.c
>> @@ -9,16 +9,16 @@
>>  #include <semihosting.h>
>>  #include <spl.h>
>>  
>> -static int smh_read_full(long fd, void *memp, size_t len)
>> +static ulong spl_smh_fit_read(struct spl_load_info *load, ulong sector,
>> +			      ulong count, void *buf)
>>  {
>> -	long read;
>> +	int ret, fd = *(int *)load->priv;
>>
> 
> should ret be long to hold smh_read() return value ?

Yes.

>> -	read = smh_read(fd, memp, len);
>> -	if (read < 0)
>> -		return read;
>> -	if (read != len)
>> -		return -EIO;
>> -	return 0;
>> +	if (smh_seek(fd, sector))
>> +		return 0;
>> +
> 
>                                                                                                                  
> I never used smh, but why would this not be :
> 
> +       ret = smh_seek(fd, sector);
> +       if (ret)
> +               return ret;

Because we always return the number of bytes read. So by returning 0 we
signal an error.

> (why does smh_seek(), smh_write(), smh_open(), smh_close() return
> long, by the way? the implementations return either 0 or smh_errno(),
> so int would do ?)

Only smh_seek/smh_close always returns 0 or smh_errno. The rest return
values from smh_trap. The reason why we use long instead of int is that
the semihosting ABI always uses native-register-sized values (aka long).
We could use int instead of long for those two functions, but I don't
think its very critical.

--Sean

>> +	ret = smh_read(fd, buf, count);
>> +	return ret < 0 ? 0 : ret;
>>  }
>>  
>>  static int spl_smh_load_image(struct spl_image_info *spl_image,
>> @@ -27,14 +27,17 @@ static int spl_smh_load_image(struct spl_image_info *spl_image,
>>  	const char *filename = CONFIG_SPL_FS_LOAD_PAYLOAD_NAME;
>>  	int ret;
>>  	long fd, len;
>> -	struct legacy_img_hdr *header =
>> -		spl_get_load_buffer(-sizeof(*header), sizeof(*header));
>> +	struct spl_load_info load = {
>> +		.bl_len = 1,
>> +		.read = spl_smh_fit_read,
>> +	};
>>  
>>  	fd = smh_open(filename, MODE_READ | MODE_BINARY);
>>  	if (fd < 0) {
>>  		log_debug("could not open %s: %ld\n", filename, fd);
>>  		return fd;
>>  	}
>> +	load.priv = &fd;
>>  
>>  	ret = smh_flen(fd);
>>  	if (ret < 0) {
>> @@ -43,25 +46,7 @@ static int spl_smh_load_image(struct spl_image_info *spl_image,
>>  	}
>>  	len = ret;
>>  
>> -	ret = smh_read_full(fd, header, sizeof(struct legacy_img_hdr));
>> -	if (ret) {
>> -		log_debug("could not read image header: %d\n", ret);
>> -		goto out;
>> -	}
>> -
>> -	ret = spl_parse_image_header(spl_image, bootdev, header);
>> -	if (ret) {
>> -		log_debug("failed to parse image header: %d\n", ret);
>> -		goto out;
>> -	}
>> -
>> -	ret = smh_seek(fd, 0);
>> -	if (ret) {
>> -		log_debug("could not seek to start of image: %d\n", ret);
>> -		goto out;
>> -	}
>> -
>> -	ret = smh_read_full(fd, (void *)spl_image->load_addr, len);
>> +	ret = spl_load(spl_image, bootdev, &load, len, 0);
>>  	if (ret)
>>  		log_debug("could not read %s: %d\n", filename, ret);
>>  out:
>> -- 
>> 2.40.1
>>