diff mbox series

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

Message ID 20230731224304.111081-10-sean.anderson@seco.com
State Changes Requested
Delegated to: Tom Rini
Headers show
Series spl: Use common function for loading/parsing images | expand

Commit Message

Sean Anderson July 31, 2023, 10:43 p.m. UTC 
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@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(-)

Comments

Xavier Drudis Ferran Aug. 3, 2023, 8:36 a.m. UTC | #1 
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@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 ?

> -	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;

(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 ?)
                   
> +	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
>
Sean Anderson Aug. 3, 2023, 3:29 p.m. UTC | #2 
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@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
>>
Heinrich Schuchardt Sept. 6, 2023, 12:10 p.m. UTC | #3 
On 03.08.23 10: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@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 ?
>
>> -	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;
>
> (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 ?)

The return type used should match the implementation on the host side.
The ARM documentation explicitly refers to register R0 which will match
the length of long (or unsigned long).

For smh_seek, smh_read(), smh_write()  we should investigate if the type
should not better be unsigned long to handle a number of bytes exceeding
2^31 on a 32bit emulation or system. But that is beyond the scope of
this patch series.

Best regards

Heinrich
diff mbox series

Patch

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;
 
-	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;
+
+	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: