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 |
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 >
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 >>
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 --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:
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(-)