[PATCH 0/3] booti: Remove the SYS_BOOTM_LEN limit for booti

[PATCH 0/3] booti: Remove the SYS_BOOTM_LEN limit for booti

[Simon Glass]

This series restores the original behaviour of extlinux booting linux
'Image' files, which is to ignore CONFIG_SYS_BOOTM_LEN and instead uses
a limit of 10x the compressed size.

It also adds RISC-V support, since it uses a similar format to ARM64.

Future work should integrate the code in 'booti' into main 'bootm'
logic.


Simon Glass (3):
  boot: Add a function to check if a linux Image is supported
  bootm: Add RISC-V support in booti_is_supported()
  booti: Allow ignoring SYS_BOOTM_LEN with the booti command

 boot/bootm.c    | 31 +++++++++++++++++++++++++------
 cmd/booti.c     |  1 +
 include/bootm.h |  3 +++
 3 files changed, 29 insertions(+), 6 deletions(-)

-- 
2.43.0

base-commit: 5f31163c8f42c16f0035ae26d1e36e94e931fe6f
branch: fix-booti

[PATCH 1/3] boot: Add a function to check if a linux Image is supported

[Simon Glass]

Move this logic into a function so we can give it a sensible name.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 boot/bootm.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/boot/bootm.c b/boot/bootm.c
index 0e63dd4adf3..2ed78295ead 100644
--- a/boot/bootm.c
+++ b/boot/bootm.c
@@ -654,6 +654,21 @@ static int handle_decomp_error(int comp_type, size_t uncomp_size,
 #endif
 
 #ifndef USE_HOSTCC
+
+/**
+ * booti_is_supported() - Check whether a Linux 'Image' is supported
+ *
+ * @os: OS to check
+ * Return: true if CMD_BOOTI is enabled and the arch suports this format
+ */
+static bool booti_is_supported(struct image_info *os)
+{
+	if (!IS_ENABLED(CONFIG_CMD_BOOTI) || os->os != IH_OS_LINUX)
+		return false;
+
+	return os->arch == IH_ARCH_ARM64;
+}
+
 static int bootm_load_os(struct bootm_headers *images, int boot_progress)
 {
 	struct image_info os = images->os;
@@ -729,8 +744,7 @@ static int bootm_load_os(struct bootm_headers *images, int boot_progress)
 		}
 	}
 
-	if (IS_ENABLED(CONFIG_CMD_BOOTI) && images->os.arch == IH_ARCH_ARM64 &&
-	    images->os.os == IH_OS_LINUX) {
+	if (booti_is_supported(&images->os)) {
 		ulong relocated_addr;
 		ulong image_size;
 		int ret;
-- 
2.43.0

[PATCH 2/3] bootm: Add RISC-V support in booti_is_supported()

[Simon Glass]

RISC-V uses a similar linux 'Image' format to ARM64, so add support for
it in bootm_load_os()

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 boot/bootm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/boot/bootm.c b/boot/bootm.c
index 2ed78295ead..7be5ec796fc 100644
--- a/boot/bootm.c
+++ b/boot/bootm.c
@@ -666,7 +666,7 @@ static bool booti_is_supported(struct image_info *os)
 	if (!IS_ENABLED(CONFIG_CMD_BOOTI) || os->os != IH_OS_LINUX)
 		return false;
 
-	return os->arch == IH_ARCH_ARM64;
+	return os->arch == IH_ARCH_ARM64 || os->arch == IH_ARCH_RISCV;
 }
 
 static int bootm_load_os(struct bootm_headers *images, int boot_progress)
-- 
2.43.0

[PATCH 3/3] booti: Allow ignoring SYS_BOOTM_LEN with the booti command

[Simon Glass]

The booti command does not use the CONFIG_SYS_BOOTM_LEN value and
instead sets a maximum decompression-buffer size of 10x the size of the
compressed data.

Add this as an option in bootm_load_os() so that booting without the
command-line works in the same way as the 'booti' command.

Link: https://lore.kernel.org/u-boot/2ad3b1c5-b6e7-47e2-b225-834b821cc5c4@kwiboo.se/

Signed-off-by: Simon Glass <sjg@chromium.org>
Reported-by: Jonas Karlman <jonas@kwiboo.se>
Fixes: b13408021d3 ("boot: pxe: Use bootm_...() functions where possible")
---

 boot/bootm.c    | 13 +++++++++----
 cmd/booti.c     |  1 +
 include/bootm.h |  3 +++
 3 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/boot/bootm.c b/boot/bootm.c
index 7be5ec796fc..dae09060f0f 100644
--- a/boot/bootm.c
+++ b/boot/bootm.c
@@ -669,8 +669,9 @@ static bool booti_is_supported(struct image_info *os)
 	return os->arch == IH_ARCH_ARM64 || os->arch == IH_ARCH_RISCV;
 }
 
-static int bootm_load_os(struct bootm_headers *images, int boot_progress)
+static int bootm_load_os(struct bootm_info *bmi, int boot_progress)
 {
+	struct bootm_headers *images = bmi->images;
 	struct image_info os = images->os;
 	ulong load = os.load;
 	ulong load_end;
@@ -681,6 +682,7 @@ static int bootm_load_os(struct bootm_headers *images, int boot_progress)
 	ulong flush_start = ALIGN_DOWN(load, ARCH_DMA_MINALIGN);
 	bool no_overlap;
 	void *load_buf, *image_buf;
+	ulong decomp_len;
 	int err;
 
 	/*
@@ -706,11 +708,12 @@ static int bootm_load_os(struct bootm_headers *images, int boot_progress)
 
 	load_buf = map_sysmem(load, 0);
 	image_buf = map_sysmem(os.image_start, image_len);
+	decomp_len = bmi->ignore_bootm_len ? image_len * 10 : bootm_len();
 	err = image_decomp(os.comp, load, os.image_start, os.type,
-			   load_buf, image_buf, image_len, bootm_len(),
+			   load_buf, image_buf, image_len, decomp_len,
 			   &load_end);
 	if (err) {
-		err = handle_decomp_error(os.comp, load_end - load, bootm_len(),
+		err = handle_decomp_error(os.comp, load_end - load, decomp_len,
 					  err);
 		bootstage_error(BOOTSTAGE_ID_DECOMP_IMAGE);
 		return err;
@@ -1073,7 +1076,7 @@ int bootm_run_states(struct bootm_info *bmi, int states)
 	if (!ret && (states & BOOTM_STATE_LOADOS)) {
 		iflag = bootm_disable_interrupts();
 		board_fixup_os(&images->os);
-		ret = bootm_load_os(images, 0);
+		ret = bootm_load_os(bmi, 0);
 		if (ret && ret != BOOTM_ERR_OVERLAP)
 			goto err;
 		else if (ret == BOOTM_ERR_OVERLAP)
@@ -1237,6 +1240,8 @@ int bootz_run(struct bootm_info *bmi)
 
 int booti_run(struct bootm_info *bmi)
 {
+	bmi->ignore_bootm_len = true;
+
 	return boot_run(bmi, "booti", BOOTM_STATE_START | BOOTM_STATE_FINDOS |
 			BOOTM_STATE_PRE_LOAD | BOOTM_STATE_FINDOTHER |
 			BOOTM_STATE_LOADOS);
diff --git a/cmd/booti.c b/cmd/booti.c
index 43e79e87201..f4f782da056 100644
--- a/cmd/booti.c
+++ b/cmd/booti.c
@@ -118,6 +118,7 @@ int do_booti(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 		bmi.conf_fdt = argv[2];
 	bmi.boot_progress = true;
 	bmi.cmd_name = "booti";
+	bmi.ignore_bootm_len = true;
 	/* do not set up argc and argv[] since nothing uses them */
 
 	if (booti_start(&bmi))
diff --git a/include/bootm.h b/include/bootm.h
index 82ab8e0e565..97f77bc1ded 100644
--- a/include/bootm.h
+++ b/include/bootm.h
@@ -44,6 +44,8 @@ struct cmd_tbl;
  * @argc: Number of arguments to the command (excluding the actual command).
  *	This is 0 if there are no arguments
  * @argv: NULL-terminated list of arguments, or NULL if there are no arguments
+ * @ignore_bootm_len: Ignore the value CONFIG_SYS_BOOTM_LEN and use 10x the
+ *	compressed length as the maximum uncompressed size
  *
  * For zboot:
  * @bzimage_addr: Address of the bzImage to boot, or 0 if the image has already
@@ -69,6 +71,7 @@ struct bootm_info {
 	const char *cmd_name;
 	int argc;
 	char *const *argv;
+	bool ignore_bootm_len;
 
 	/* zboot items */
 #ifdef CONFIG_X86
-- 
2.43.0

Re: [PATCH 0/3] booti: Remove the SYS_BOOTM_LEN limit for booti

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1740 bytes --]

On Fri, Apr 18, 2025 at 06:50:59AM -0600, Simon Glass wrote:

> This series restores the original behaviour of extlinux booting linux
> 'Image' files, which is to ignore CONFIG_SYS_BOOTM_LEN and instead uses
> a limit of 10x the compressed size.
> 
> It also adds RISC-V support, since it uses a similar format to ARM64.
> 
> Future work should integrate the code in 'booti' into main 'bootm'
> logic.
> 
> 
> Simon Glass (3):
>   boot: Add a function to check if a linux Image is supported
>   bootm: Add RISC-V support in booti_is_supported()
>   booti: Allow ignoring SYS_BOOTM_LEN with the booti command
> 
>  boot/bootm.c    | 31 +++++++++++++++++++++++++------
>  cmd/booti.c     |  1 +
>  include/bootm.h |  3 +++
>  3 files changed, 29 insertions(+), 6 deletions(-)

This is like pulling teeth. We aren't ignoring SYS_BOOTM_LEN so much as
it was never relevant here to start with. The documentation
(doc/usage/cmd/booti.rst) has explained that we use 10 x
kernel_comp_size for the limit. The "bootm" namespace has largely been
about uImage (and then FIT images when not using it's own namespace)
images and not applied elsewhere, which is why bootz/booti handled
things outside that namespace. And then, still, this series doesn't
unify cmd/booti.c with your changes, it just introduces duplicated
functionality, which is generally understood to be a bad thing.

So again, please do similar to what you did for cmd/bootz.c and that
report and move the existing functionality over. We can then look at
cleaning it up (which should look like / abstract what we do for
compressed FIT images) as a follow up, in order to make bisecting any
regressions here easier down the line. Thanks.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 0/3] booti: Remove the SYS_BOOTM_LEN limit for booti

[Simon Glass]

Hi Tom,

On Fri, 18 Apr 2025 at 07:33, Tom Rini <trini@konsulko.com> wrote:
>
> On Fri, Apr 18, 2025 at 06:50:59AM -0600, Simon Glass wrote:
>
> > This series restores the original behaviour of extlinux booting linux
> > 'Image' files, which is to ignore CONFIG_SYS_BOOTM_LEN and instead uses
> > a limit of 10x the compressed size.
> >
> > It also adds RISC-V support, since it uses a similar format to ARM64.
> >
> > Future work should integrate the code in 'booti' into main 'bootm'
> > logic.
> >
> >
> > Simon Glass (3):
> >   boot: Add a function to check if a linux Image is supported
> >   bootm: Add RISC-V support in booti_is_supported()
> >   booti: Allow ignoring SYS_BOOTM_LEN with the booti command
> >
> >  boot/bootm.c    | 31 +++++++++++++++++++++++++------
> >  cmd/booti.c     |  1 +
> >  include/bootm.h |  3 +++
> >  3 files changed, 29 insertions(+), 6 deletions(-)
>
> This is like pulling teeth. We aren't ignoring SYS_BOOTM_LEN so much as
> it was never relevant here to start with. The documentation
> (doc/usage/cmd/booti.rst) has explained that we use 10 x
> kernel_comp_size for the limit. The "bootm" namespace has largely been
> about uImage (and then FIT images when not using it's own namespace)
> images and not applied elsewhere, which is why bootz/booti handled
> things outside that namespace. And then, still, this series doesn't
> unify cmd/booti.c with your changes, it just introduces duplicated
> functionality, which is generally understood to be a bad thing.
>
> So again, please do similar to what you did for cmd/bootz.c and that
> report and move the existing functionality over. We can then look at
> cleaning it up (which should look like / abstract what we do for
> compressed FIT images) as a follow up, in order to make bisecting any
> regressions here easier down the line. Thanks.

Well, I still don't understand what you are getting at here.

The existing functionality for booti is already there. Marek added it
a few years back. Can you please be more specific as to what
functionality you think is missing?

Yes, the bootm implementation is separate from the booti command at
present. They both call booti_setup(). But the booti command uses
kernel_comp_addr_r which we shouldn't be using in bootm. In fact I'd
like to get rid of those vars, not proliferate them. There is
certainly some more common code which can be teased apart, but it's
not the subject of this bugfix. I have to stop somewhere.

Perhaps you should let me do this refactoring in the way that I
believe it should be done?

Regards,
Simon

Re: [PATCH 0/3] booti: Remove the SYS_BOOTM_LEN limit for booti

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3298 bytes --]

On Fri, Apr 18, 2025 at 07:46:59AM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Fri, 18 Apr 2025 at 07:33, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Fri, Apr 18, 2025 at 06:50:59AM -0600, Simon Glass wrote:
> >
> > > This series restores the original behaviour of extlinux booting linux
> > > 'Image' files, which is to ignore CONFIG_SYS_BOOTM_LEN and instead uses
> > > a limit of 10x the compressed size.
> > >
> > > It also adds RISC-V support, since it uses a similar format to ARM64.
> > >
> > > Future work should integrate the code in 'booti' into main 'bootm'
> > > logic.
> > >
> > >
> > > Simon Glass (3):
> > >   boot: Add a function to check if a linux Image is supported
> > >   bootm: Add RISC-V support in booti_is_supported()
> > >   booti: Allow ignoring SYS_BOOTM_LEN with the booti command
> > >
> > >  boot/bootm.c    | 31 +++++++++++++++++++++++++------
> > >  cmd/booti.c     |  1 +
> > >  include/bootm.h |  3 +++
> > >  3 files changed, 29 insertions(+), 6 deletions(-)
> >
> > This is like pulling teeth. We aren't ignoring SYS_BOOTM_LEN so much as
> > it was never relevant here to start with. The documentation
> > (doc/usage/cmd/booti.rst) has explained that we use 10 x
> > kernel_comp_size for the limit. The "bootm" namespace has largely been
> > about uImage (and then FIT images when not using it's own namespace)
> > images and not applied elsewhere, which is why bootz/booti handled
> > things outside that namespace. And then, still, this series doesn't
> > unify cmd/booti.c with your changes, it just introduces duplicated
> > functionality, which is generally understood to be a bad thing.
> >
> > So again, please do similar to what you did for cmd/bootz.c and that
> > report and move the existing functionality over. We can then look at
> > cleaning it up (which should look like / abstract what we do for
> > compressed FIT images) as a follow up, in order to make bisecting any
> > regressions here easier down the line. Thanks.
> 
> Well, I still don't understand what you are getting at here.
> 
> The existing functionality for booti is already there. Marek added it
> a few years back. Can you please be more specific as to what
> functionality you think is missing?

With:
https://patchwork.ozlabs.org/project/uboot/patch/20250409155723.431102-1-sjg@chromium.org/
You moved the functionality from cmd/bootz.c and over to boot/bootm.c

You need to do the same thing here.

> Yes, the bootm implementation is separate from the booti command at
> present. They both call booti_setup(). But the booti command uses
> kernel_comp_addr_r which we shouldn't be using in bootm. In fact I'd
> like to get rid of those vars, not proliferate them. There is
> certainly some more common code which can be teased apart, but it's
> not the subject of this bugfix. I have to stop somewhere.
> 
> Perhaps you should let me do this refactoring in the way that I
> believe it should be done?

One of the big lessons learned I've had from all of the code refactors
you've done over the years is we need to do them small and bisectable
because corner cases become a nightmare to understand and resolve. So
perhaps you should do the refactoring the way I believe it needs to be
done.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 0/3] booti: Remove the SYS_BOOTM_LEN limit for booti

[Simon Glass]

Hi Tom,

On Fri, 18 Apr 2025 at 08:08, Tom Rini <trini@konsulko.com> wrote:
>
> On Fri, Apr 18, 2025 at 07:46:59AM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Fri, 18 Apr 2025 at 07:33, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Fri, Apr 18, 2025 at 06:50:59AM -0600, Simon Glass wrote:
> > >
> > > > This series restores the original behaviour of extlinux booting linux
> > > > 'Image' files, which is to ignore CONFIG_SYS_BOOTM_LEN and instead uses
> > > > a limit of 10x the compressed size.
> > > >
> > > > It also adds RISC-V support, since it uses a similar format to ARM64.
> > > >
> > > > Future work should integrate the code in 'booti' into main 'bootm'
> > > > logic.
> > > >
> > > >
> > > > Simon Glass (3):
> > > >   boot: Add a function to check if a linux Image is supported
> > > >   bootm: Add RISC-V support in booti_is_supported()
> > > >   booti: Allow ignoring SYS_BOOTM_LEN with the booti command
> > > >
> > > >  boot/bootm.c    | 31 +++++++++++++++++++++++++------
> > > >  cmd/booti.c     |  1 +
> > > >  include/bootm.h |  3 +++
> > > >  3 files changed, 29 insertions(+), 6 deletions(-)
> > >
> > > This is like pulling teeth. We aren't ignoring SYS_BOOTM_LEN so much as
> > > it was never relevant here to start with. The documentation
> > > (doc/usage/cmd/booti.rst) has explained that we use 10 x
> > > kernel_comp_size for the limit. The "bootm" namespace has largely been
> > > about uImage (and then FIT images when not using it's own namespace)
> > > images and not applied elsewhere, which is why bootz/booti handled
> > > things outside that namespace. And then, still, this series doesn't
> > > unify cmd/booti.c with your changes, it just introduces duplicated
> > > functionality, which is generally understood to be a bad thing.
> > >
> > > So again, please do similar to what you did for cmd/bootz.c and that
> > > report and move the existing functionality over. We can then look at
> > > cleaning it up (which should look like / abstract what we do for
> > > compressed FIT images) as a follow up, in order to make bisecting any
> > > regressions here easier down the line. Thanks.
> >
> > Well, I still don't understand what you are getting at here.
> >
> > The existing functionality for booti is already there. Marek added it
> > a few years back. Can you please be more specific as to what
> > functionality you think is missing?
>
> With:
> https://patchwork.ozlabs.org/project/uboot/patch/20250409155723.431102-1-sjg@chromium.org/
> You moved the functionality from cmd/bootz.c and over to boot/bootm.c
>
> You need to do the same thing here.

That code is already there. Here is the code you are talking about, in
cmd/booti.c (which is a hack, BTW)

static int booti_start(struct bootm_info *bmi)
{
        struct bootm_headers *images = bmi->images;
        int ret;
        ulong ld;
        ulong relocated_addr;
        ulong image_size;
        uint8_t *temp;
        ulong dest;
        ulong dest_end;
        unsigned long comp_len;
        unsigned long decomp_len;
        int ctype;

        ret = bootm_run_states(bmi, BOOTM_STATE_START);

See booti_run(). bootm_run_states() is called by virtue of boot_run(),
since the BOOTM_STATE_START flag is set

        /* Setup Linux kernel Image entry point */
        if (!bmi->addr_img) {
                ld = image_load_addr;
                debug("*  kernel: default image load address = 0x%08lx\n",
                                image_load_addr);
        } else {
                ld = hextoul(bmi->addr_img, NULL);
                debug("*  kernel: cmdline image address = 0x%08lx\n", ld);
        }

bootm handles the default load address, etc.

The next chunk is:

        temp = map_sysmem(ld, 0);
        ctype = image_decomp_type(temp, 2);
        if (ctype > 0) {
                dest = env_get_ulong("kernel_comp_addr_r", 16, 0);
                comp_len = env_get_ulong("kernel_comp_size", 16, 0);


                if (!dest || !comp_len) {
                        puts("kernel_comp_addr_r or kernel_comp_size
is not provided!\n");
                        return -EINVAL;
                }
                if (dest < gd->ram_base || dest > gd->ram_top) {
                        puts("kernel_comp_addr_r is outside of DRAM range!\n");
                        return -EINVAL;
                }

which is in this function:

static int found_booti_os(enum image_comp_t comp)
{
        images.os.load = images.os.image_start;
        images.os.type = IH_TYPE_KERNEL;
        images.os.os = IH_OS_LINUX;
        images.os.comp = comp;
        if (IS_ENABLED(CONFIG_RISCV_SMODE))
                images.os.arch = IH_ARCH_RISCV;
        else if (IS_ENABLED(CONFIG_ARM64))
                images.os.arch = IH_ARCH_ARM64;

        log_debug("load %lx start %lx len %lx ep %lx os %x comp %x\n",
                  images.os.load, images.os.image_start, images.os.image_len,
                  images.ep, images.os.os, images.os.comp);
        if (comp != IH_COMP_NONE) {
                images.os.load = env_get_hex("kernel_comp_addr_r", 0);
                images.os.image_len = env_get_ulong("kernel_comp_size", 16, 0);
                if (!images.os.load || !images.os.image_len) {
                        puts("kernel_comp_addr_r or kernel_comp_size
is not provided!\n");
                        return -ENOTSUPP;
                }
                if (lmb_reserve(images.os.load, images.os.image_len) < 0)
                        return -EXDEV;
        }

        return 0;
}


The next chunk is:

                debug("kernel image compression type %d size = 0x%08lx
address = 0x%08lx\n",
                        ctype, comp_len, (ulong)dest);
                decomp_len = comp_len * 10;
                ret = image_decomp(ctype, 0, ld, IH_TYPE_KERNEL,
                                 (void *)dest, (void *)ld, comp_len,
                                 decomp_len, &dest_end);
                if (ret)
                        return ret;
                /* dest_end contains the uncompressed Image size */
                memmove((void *) ld, (void *)dest, dest_end);
        }
        unmap_sysmem((void *)ld);

which happens in bootm_load_os():

        load_buf = map_sysmem(load, 0);
        image_buf = map_sysmem(os.image_start, image_len);
        err = image_decomp(os.comp, load, os.image_start, os.type,
                           load_buf, image_buf, image_len, bootm_len(),
                           &load_end);
        if (err) {
                err = handle_decomp_error(os.comp, load_end - load, bootm_len(),
                                          err);
                bootstage_error(BOOTSTAGE_ID_DECOMP_IMAGE);
                return err;
        }


then we have the setup:

        ret = booti_setup(ld, &relocated_addr, &image_size, false);
        if (ret)
                return 1;

        /* Handle BOOTM_STATE_LOADOS */
        if (relocated_addr != ld) {
                printf("Moving Image from 0x%lx to 0x%lx, end=0x%lx\n", ld,
                       relocated_addr, relocated_addr + image_size);
                memmove((void *)relocated_addr, (void *)ld, image_size);
        }

        images->ep = relocated_addr;
        images->os.start = relocated_addr;
        images->os.end = relocated_addr + image_size;

        lmb_reserve(images->ep, le32_to_cpu(image_size));

which happens later in bootm_load_os():

        if (IS_ENABLED(CONFIG_CMD_BOOTI) && images->os.arch == IH_ARCH_ARM64 &&
            images->os.os == IH_OS_LINUX) {
                ulong relocated_addr;
                ulong image_size;
                int ret;

                ret = booti_setup(load, &relocated_addr, &image_size, false);
                if (ret) {
                        printf("Failed to prep arm64 kernel (err=%d)\n", ret);
                        return BOOTM_ERR_RESET;
                }

                /* Handle BOOTM_STATE_LOADOS */
                if (relocated_addr != load) {
                        printf("Moving Image from 0x%lx to 0x%lx, end=0x%lx\n",
                               load, relocated_addr,
                               relocated_addr + image_size);
                        memmove((void *)relocated_addr, load_buf, image_size);
                }

                images->ep = relocated_addr;
                images->os.start = relocated_addr;
                images->os.end = relocated_addr + image_size;
        }

        if (CONFIG_IS_ENABLED(LMB))
                lmb_reserve(images->os.load, (load_end - images->os.load));


Finally we have this:

        /*
         * Handle the BOOTM_STATE_FINDOTHER state ourselves as we do not
         * have a header that provide this informaiton.
         */
        if (bootm_find_images(image_load_addr, bmi->conf_ramdisk, bmi->conf_fdt,
                              relocated_addr, image_size))
                return 1;

        return 0;
}

which happens by virtue of the BOOTM_STATE_LOADOS flag passed in booi_run()

>
> > Yes, the bootm implementation is separate from the booti command at
> > present. They both call booti_setup(). But the booti command uses
> > kernel_comp_addr_r which we shouldn't be using in bootm. In fact I'd
> > like to get rid of those vars, not proliferate them. There is
> > certainly some more common code which can be teased apart, but it's
> > not the subject of this bugfix. I have to stop somewhere.
> >
> > Perhaps you should let me do this refactoring in the way that I
> > believe it should be done?
>
> One of the big lessons learned I've had from all of the code refactors
> you've done over the years is we need to do them small and bisectable
> because corner cases become a nightmare to understand and resolve. So
> perhaps you should do the refactoring the way I believe it needs to be
> done.

You have to understand that bootm works in defined stages. The booti
command was written entirely separately and as a result bootm did not
support booting the linux 'Image' format. I've slowly been sorting
that out and cleaning up the hacks.

For example the 'booti' command does most of the stages all at once in
booti_start(). EFI has the same issue as i discussed with Heinrich
yesterday. See this code at the top of do_bootm_efi():

if (flag != BOOTM_STATE_OS_GO)
        return 0;

So it does nothing for all the stages, then loads, decompresses, etc.
etc. right at the end at the stage where it is supposed to just be
running the image.

Regards,
Simon

Re: [PATCH 0/3] booti: Remove the SYS_BOOTM_LEN limit for booti

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3166 bytes --]

On Thu, May 01, 2025 at 07:06:03AM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Fri, 18 Apr 2025 at 08:08, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Fri, Apr 18, 2025 at 07:46:59AM -0600, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Fri, 18 Apr 2025 at 07:33, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Fri, Apr 18, 2025 at 06:50:59AM -0600, Simon Glass wrote:
> > > >
> > > > > This series restores the original behaviour of extlinux booting linux
> > > > > 'Image' files, which is to ignore CONFIG_SYS_BOOTM_LEN and instead uses
> > > > > a limit of 10x the compressed size.
> > > > >
> > > > > It also adds RISC-V support, since it uses a similar format to ARM64.
> > > > >
> > > > > Future work should integrate the code in 'booti' into main 'bootm'
> > > > > logic.
> > > > >
> > > > >
> > > > > Simon Glass (3):
> > > > >   boot: Add a function to check if a linux Image is supported
> > > > >   bootm: Add RISC-V support in booti_is_supported()
> > > > >   booti: Allow ignoring SYS_BOOTM_LEN with the booti command
> > > > >
> > > > >  boot/bootm.c    | 31 +++++++++++++++++++++++++------
> > > > >  cmd/booti.c     |  1 +
> > > > >  include/bootm.h |  3 +++
> > > > >  3 files changed, 29 insertions(+), 6 deletions(-)
> > > >
> > > > This is like pulling teeth. We aren't ignoring SYS_BOOTM_LEN so much as
> > > > it was never relevant here to start with. The documentation
> > > > (doc/usage/cmd/booti.rst) has explained that we use 10 x
> > > > kernel_comp_size for the limit. The "bootm" namespace has largely been
> > > > about uImage (and then FIT images when not using it's own namespace)
> > > > images and not applied elsewhere, which is why bootz/booti handled
> > > > things outside that namespace. And then, still, this series doesn't
> > > > unify cmd/booti.c with your changes, it just introduces duplicated
> > > > functionality, which is generally understood to be a bad thing.
> > > >
> > > > So again, please do similar to what you did for cmd/bootz.c and that
> > > > report and move the existing functionality over. We can then look at
> > > > cleaning it up (which should look like / abstract what we do for
> > > > compressed FIT images) as a follow up, in order to make bisecting any
> > > > regressions here easier down the line. Thanks.
> > >
> > > Well, I still don't understand what you are getting at here.
> > >
> > > The existing functionality for booti is already there. Marek added it
> > > a few years back. Can you please be more specific as to what
> > > functionality you think is missing?
> >
> > With:
> > https://patchwork.ozlabs.org/project/uboot/patch/20250409155723.431102-1-sjg@chromium.org/
> > You moved the functionality from cmd/bootz.c and over to boot/bootm.c
> >
> > You need to do the same thing here.
> 
> That code is already there. Here is the code you are talking about, in
> cmd/booti.c (which is a hack, BTW)

Hey, thanks for insulting my code, BTW. Best way to make sure I really want
to take your way of refactoring the code, rather than the way I asked
you to.

I'll look at this again tomorrow.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 0/3] booti: Remove the SYS_BOOTM_LEN limit for booti

[Simon Glass]

Hi Tom,

On Thu, 1 May 2025 at 08:59, Tom Rini <trini@konsulko.com> wrote:
>
> On Thu, May 01, 2025 at 07:06:03AM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Fri, 18 Apr 2025 at 08:08, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Fri, Apr 18, 2025 at 07:46:59AM -0600, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Fri, 18 Apr 2025 at 07:33, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Fri, Apr 18, 2025 at 06:50:59AM -0600, Simon Glass wrote:
> > > > >
> > > > > > This series restores the original behaviour of extlinux booting linux
> > > > > > 'Image' files, which is to ignore CONFIG_SYS_BOOTM_LEN and instead uses
> > > > > > a limit of 10x the compressed size.
> > > > > >
> > > > > > It also adds RISC-V support, since it uses a similar format to ARM64.
> > > > > >
> > > > > > Future work should integrate the code in 'booti' into main 'bootm'
> > > > > > logic.
> > > > > >
> > > > > >
> > > > > > Simon Glass (3):
> > > > > >   boot: Add a function to check if a linux Image is supported
> > > > > >   bootm: Add RISC-V support in booti_is_supported()
> > > > > >   booti: Allow ignoring SYS_BOOTM_LEN with the booti command
> > > > > >
> > > > > >  boot/bootm.c    | 31 +++++++++++++++++++++++++------
> > > > > >  cmd/booti.c     |  1 +
> > > > > >  include/bootm.h |  3 +++
> > > > > >  3 files changed, 29 insertions(+), 6 deletions(-)
> > > > >
> > > > > This is like pulling teeth. We aren't ignoring SYS_BOOTM_LEN so much as
> > > > > it was never relevant here to start with. The documentation
> > > > > (doc/usage/cmd/booti.rst) has explained that we use 10 x
> > > > > kernel_comp_size for the limit. The "bootm" namespace has largely been
> > > > > about uImage (and then FIT images when not using it's own namespace)
> > > > > images and not applied elsewhere, which is why bootz/booti handled
> > > > > things outside that namespace. And then, still, this series doesn't
> > > > > unify cmd/booti.c with your changes, it just introduces duplicated
> > > > > functionality, which is generally understood to be a bad thing.
> > > > >
> > > > > So again, please do similar to what you did for cmd/bootz.c and that
> > > > > report and move the existing functionality over. We can then look at
> > > > > cleaning it up (which should look like / abstract what we do for
> > > > > compressed FIT images) as a follow up, in order to make bisecting any
> > > > > regressions here easier down the line. Thanks.
> > > >
> > > > Well, I still don't understand what you are getting at here.
> > > >
> > > > The existing functionality for booti is already there. Marek added it
> > > > a few years back. Can you please be more specific as to what
> > > > functionality you think is missing?
> > >
> > > With:
> > > https://patchwork.ozlabs.org/project/uboot/patch/20250409155723.431102-1-sjg@chromium.org/
> > > You moved the functionality from cmd/bootz.c and over to boot/bootm.c
> > >
> > > You need to do the same thing here.
> >
> > That code is already there. Here is the code you are talking about, in
> > cmd/booti.c (which is a hack, BTW)
>
> Hey, thanks for insulting my code, BTW. Best way to make sure I really want
> to take your way of refactoring the code, rather than the way I asked
> you to.
>
> I'll look at this again tomorrow.

Oops, I didn't know it was your code, sorry.

Regards,
Simon

Re: [PATCH 0/3] booti: Remove the SYS_BOOTM_LEN limit for booti

[Peter Robinson]

On Thu, 1 May 2025 at 16:03, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Tom,
>
> On Thu, 1 May 2025 at 08:59, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Thu, May 01, 2025 at 07:06:03AM -0600, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Fri, 18 Apr 2025 at 08:08, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Fri, Apr 18, 2025 at 07:46:59AM -0600, Simon Glass wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Fri, 18 Apr 2025 at 07:33, Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Fri, Apr 18, 2025 at 06:50:59AM -0600, Simon Glass wrote:
> > > > > >
> > > > > > > This series restores the original behaviour of extlinux booting linux
> > > > > > > 'Image' files, which is to ignore CONFIG_SYS_BOOTM_LEN and instead uses
> > > > > > > a limit of 10x the compressed size.
> > > > > > >
> > > > > > > It also adds RISC-V support, since it uses a similar format to ARM64.
> > > > > > >
> > > > > > > Future work should integrate the code in 'booti' into main 'bootm'
> > > > > > > logic.
> > > > > > >
> > > > > > >
> > > > > > > Simon Glass (3):
> > > > > > >   boot: Add a function to check if a linux Image is supported
> > > > > > >   bootm: Add RISC-V support in booti_is_supported()
> > > > > > >   booti: Allow ignoring SYS_BOOTM_LEN with the booti command
> > > > > > >
> > > > > > >  boot/bootm.c    | 31 +++++++++++++++++++++++++------
> > > > > > >  cmd/booti.c     |  1 +
> > > > > > >  include/bootm.h |  3 +++
> > > > > > >  3 files changed, 29 insertions(+), 6 deletions(-)
> > > > > >
> > > > > > This is like pulling teeth. We aren't ignoring SYS_BOOTM_LEN so much as
> > > > > > it was never relevant here to start with. The documentation
> > > > > > (doc/usage/cmd/booti.rst) has explained that we use 10 x
> > > > > > kernel_comp_size for the limit. The "bootm" namespace has largely been
> > > > > > about uImage (and then FIT images when not using it's own namespace)
> > > > > > images and not applied elsewhere, which is why bootz/booti handled
> > > > > > things outside that namespace. And then, still, this series doesn't
> > > > > > unify cmd/booti.c with your changes, it just introduces duplicated
> > > > > > functionality, which is generally understood to be a bad thing.
> > > > > >
> > > > > > So again, please do similar to what you did for cmd/bootz.c and that
> > > > > > report and move the existing functionality over. We can then look at
> > > > > > cleaning it up (which should look like / abstract what we do for
> > > > > > compressed FIT images) as a follow up, in order to make bisecting any
> > > > > > regressions here easier down the line. Thanks.
> > > > >
> > > > > Well, I still don't understand what you are getting at here.
> > > > >
> > > > > The existing functionality for booti is already there. Marek added it
> > > > > a few years back. Can you please be more specific as to what
> > > > > functionality you think is missing?
> > > >
> > > > With:
> > > > https://patchwork.ozlabs.org/project/uboot/patch/20250409155723.431102-1-sjg@chromium.org/
> > > > You moved the functionality from cmd/bootz.c and over to boot/bootm.c
> > > >
> > > > You need to do the same thing here.
> > >
> > > That code is already there. Here is the code you are talking about, in
> > > cmd/booti.c (which is a hack, BTW)
> >
> > Hey, thanks for insulting my code, BTW. Best way to make sure I really want
> > to take your way of refactoring the code, rather than the way I asked
> > you to.
> >
> > I'll look at this again tomorrow.
>
> Oops, I didn't know it was your code, sorry.

Simon, does it matter who's code it is? You shouldn't be insulting
anyone's code!

Re: [PATCH 0/3] booti: Remove the SYS_BOOTM_LEN limit for booti

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 11820 bytes --]

On Thu, May 01, 2025 at 07:06:03AM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Fri, 18 Apr 2025 at 08:08, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Fri, Apr 18, 2025 at 07:46:59AM -0600, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Fri, 18 Apr 2025 at 07:33, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Fri, Apr 18, 2025 at 06:50:59AM -0600, Simon Glass wrote:
> > > >
> > > > > This series restores the original behaviour of extlinux booting linux
> > > > > 'Image' files, which is to ignore CONFIG_SYS_BOOTM_LEN and instead uses
> > > > > a limit of 10x the compressed size.
> > > > >
> > > > > It also adds RISC-V support, since it uses a similar format to ARM64.
> > > > >
> > > > > Future work should integrate the code in 'booti' into main 'bootm'
> > > > > logic.
> > > > >
> > > > >
> > > > > Simon Glass (3):
> > > > >   boot: Add a function to check if a linux Image is supported
> > > > >   bootm: Add RISC-V support in booti_is_supported()
> > > > >   booti: Allow ignoring SYS_BOOTM_LEN with the booti command
> > > > >
> > > > >  boot/bootm.c    | 31 +++++++++++++++++++++++++------
> > > > >  cmd/booti.c     |  1 +
> > > > >  include/bootm.h |  3 +++
> > > > >  3 files changed, 29 insertions(+), 6 deletions(-)
> > > >
> > > > This is like pulling teeth. We aren't ignoring SYS_BOOTM_LEN so much as
> > > > it was never relevant here to start with. The documentation
> > > > (doc/usage/cmd/booti.rst) has explained that we use 10 x
> > > > kernel_comp_size for the limit. The "bootm" namespace has largely been
> > > > about uImage (and then FIT images when not using it's own namespace)
> > > > images and not applied elsewhere, which is why bootz/booti handled
> > > > things outside that namespace. And then, still, this series doesn't
> > > > unify cmd/booti.c with your changes, it just introduces duplicated
> > > > functionality, which is generally understood to be a bad thing.
> > > >
> > > > So again, please do similar to what you did for cmd/bootz.c and that
> > > > report and move the existing functionality over. We can then look at
> > > > cleaning it up (which should look like / abstract what we do for
> > > > compressed FIT images) as a follow up, in order to make bisecting any
> > > > regressions here easier down the line. Thanks.
> > >
> > > Well, I still don't understand what you are getting at here.
> > >
> > > The existing functionality for booti is already there. Marek added it
> > > a few years back. Can you please be more specific as to what
> > > functionality you think is missing?
> >
> > With:
> > https://patchwork.ozlabs.org/project/uboot/patch/20250409155723.431102-1-sjg@chromium.org/
> > You moved the functionality from cmd/bootz.c and over to boot/bootm.c
> >
> > You need to do the same thing here.
> 
> That code is already there. Here is the code you are talking about, in
> cmd/booti.c (which is a hack, BTW)
> 
> static int booti_start(struct bootm_info *bmi)
> {
>         struct bootm_headers *images = bmi->images;
>         int ret;
>         ulong ld;
>         ulong relocated_addr;
>         ulong image_size;
>         uint8_t *temp;
>         ulong dest;
>         ulong dest_end;
>         unsigned long comp_len;
>         unsigned long decomp_len;
>         int ctype;
> 
>         ret = bootm_run_states(bmi, BOOTM_STATE_START);
> 
> See booti_run(). bootm_run_states() is called by virtue of boot_run(),
> since the BOOTM_STATE_START flag is set
> 
>         /* Setup Linux kernel Image entry point */
>         if (!bmi->addr_img) {
>                 ld = image_load_addr;
>                 debug("*  kernel: default image load address = 0x%08lx\n",
>                                 image_load_addr);
>         } else {
>                 ld = hextoul(bmi->addr_img, NULL);
>                 debug("*  kernel: cmdline image address = 0x%08lx\n", ld);
>         }
> 
> bootm handles the default load address, etc.
> 
> The next chunk is:
> 
>         temp = map_sysmem(ld, 0);
>         ctype = image_decomp_type(temp, 2);
>         if (ctype > 0) {
>                 dest = env_get_ulong("kernel_comp_addr_r", 16, 0);
>                 comp_len = env_get_ulong("kernel_comp_size", 16, 0);
> 
> 
>                 if (!dest || !comp_len) {
>                         puts("kernel_comp_addr_r or kernel_comp_size
> is not provided!\n");
>                         return -EINVAL;
>                 }
>                 if (dest < gd->ram_base || dest > gd->ram_top) {
>                         puts("kernel_comp_addr_r is outside of DRAM range!\n");
>                         return -EINVAL;
>                 }
> 
> which is in this function:
> 
> static int found_booti_os(enum image_comp_t comp)
> {
>         images.os.load = images.os.image_start;
>         images.os.type = IH_TYPE_KERNEL;
>         images.os.os = IH_OS_LINUX;
>         images.os.comp = comp;
>         if (IS_ENABLED(CONFIG_RISCV_SMODE))
>                 images.os.arch = IH_ARCH_RISCV;
>         else if (IS_ENABLED(CONFIG_ARM64))
>                 images.os.arch = IH_ARCH_ARM64;
> 
>         log_debug("load %lx start %lx len %lx ep %lx os %x comp %x\n",
>                   images.os.load, images.os.image_start, images.os.image_len,
>                   images.ep, images.os.os, images.os.comp);
>         if (comp != IH_COMP_NONE) {
>                 images.os.load = env_get_hex("kernel_comp_addr_r", 0);
>                 images.os.image_len = env_get_ulong("kernel_comp_size", 16, 0);
>                 if (!images.os.load || !images.os.image_len) {
>                         puts("kernel_comp_addr_r or kernel_comp_size
> is not provided!\n");
>                         return -ENOTSUPP;
>                 }
>                 if (lmb_reserve(images.os.load, images.os.image_len) < 0)
>                         return -EXDEV;
>         }
> 
>         return 0;
> }
> 
> 
> The next chunk is:
> 
>                 debug("kernel image compression type %d size = 0x%08lx
> address = 0x%08lx\n",
>                         ctype, comp_len, (ulong)dest);
>                 decomp_len = comp_len * 10;
>                 ret = image_decomp(ctype, 0, ld, IH_TYPE_KERNEL,
>                                  (void *)dest, (void *)ld, comp_len,
>                                  decomp_len, &dest_end);
>                 if (ret)
>                         return ret;
>                 /* dest_end contains the uncompressed Image size */
>                 memmove((void *) ld, (void *)dest, dest_end);
>         }
>         unmap_sysmem((void *)ld);
> 
> which happens in bootm_load_os():
> 
>         load_buf = map_sysmem(load, 0);
>         image_buf = map_sysmem(os.image_start, image_len);
>         err = image_decomp(os.comp, load, os.image_start, os.type,
>                            load_buf, image_buf, image_len, bootm_len(),
>                            &load_end);
>         if (err) {
>                 err = handle_decomp_error(os.comp, load_end - load, bootm_len(),
>                                           err);
>                 bootstage_error(BOOTSTAGE_ID_DECOMP_IMAGE);
>                 return err;
>         }
> 
> 
> then we have the setup:
> 
>         ret = booti_setup(ld, &relocated_addr, &image_size, false);
>         if (ret)
>                 return 1;
> 
>         /* Handle BOOTM_STATE_LOADOS */
>         if (relocated_addr != ld) {
>                 printf("Moving Image from 0x%lx to 0x%lx, end=0x%lx\n", ld,
>                        relocated_addr, relocated_addr + image_size);
>                 memmove((void *)relocated_addr, (void *)ld, image_size);
>         }
> 
>         images->ep = relocated_addr;
>         images->os.start = relocated_addr;
>         images->os.end = relocated_addr + image_size;
> 
>         lmb_reserve(images->ep, le32_to_cpu(image_size));
> 
> which happens later in bootm_load_os():
> 
>         if (IS_ENABLED(CONFIG_CMD_BOOTI) && images->os.arch == IH_ARCH_ARM64 &&
>             images->os.os == IH_OS_LINUX) {
>                 ulong relocated_addr;
>                 ulong image_size;
>                 int ret;
> 
>                 ret = booti_setup(load, &relocated_addr, &image_size, false);
>                 if (ret) {
>                         printf("Failed to prep arm64 kernel (err=%d)\n", ret);
>                         return BOOTM_ERR_RESET;
>                 }
> 
>                 /* Handle BOOTM_STATE_LOADOS */
>                 if (relocated_addr != load) {
>                         printf("Moving Image from 0x%lx to 0x%lx, end=0x%lx\n",
>                                load, relocated_addr,
>                                relocated_addr + image_size);
>                         memmove((void *)relocated_addr, load_buf, image_size);
>                 }
> 
>                 images->ep = relocated_addr;
>                 images->os.start = relocated_addr;
>                 images->os.end = relocated_addr + image_size;
>         }
> 
>         if (CONFIG_IS_ENABLED(LMB))
>                 lmb_reserve(images->os.load, (load_end - images->os.load));
> 
> 
> Finally we have this:
> 
>         /*
>          * Handle the BOOTM_STATE_FINDOTHER state ourselves as we do not
>          * have a header that provide this informaiton.
>          */
>         if (bootm_find_images(image_load_addr, bmi->conf_ramdisk, bmi->conf_fdt,
>                               relocated_addr, image_size))
>                 return 1;
> 
>         return 0;
> }
> 
> which happens by virtue of the BOOTM_STATE_LOADOS flag passed in booi_run()

Which brings me to the point I was making before, and am making again.
Why in this case are you happy about having duplicated code, especially
since for the bootz case you went and refactored things not to.

> > > Yes, the bootm implementation is separate from the booti command at
> > > present. They both call booti_setup(). But the booti command uses
> > > kernel_comp_addr_r which we shouldn't be using in bootm. In fact I'd
> > > like to get rid of those vars, not proliferate them. There is
> > > certainly some more common code which can be teased apart, but it's
> > > not the subject of this bugfix. I have to stop somewhere.
> > >
> > > Perhaps you should let me do this refactoring in the way that I
> > > believe it should be done?
> >
> > One of the big lessons learned I've had from all of the code refactors
> > you've done over the years is we need to do them small and bisectable
> > because corner cases become a nightmare to understand and resolve. So
> > perhaps you should do the refactoring the way I believe it needs to be
> > done.
> 
> You have to understand that bootm works in defined stages. The booti

Yes, I'm aware. I modeled this on bootz rather than include it in the
mess that was/is "bootm".

> command was written entirely separately and as a result bootm did not
> support booting the linux 'Image' format. I've slowly been sorting
> that out and cleaning up the hacks.
> 
> For example the 'booti' command does most of the stages all at once in
> booti_start(). EFI has the same issue as i discussed with Heinrich
> yesterday. See this code at the top of do_bootm_efi():
> 
> if (flag != BOOTM_STATE_OS_GO)
>         return 0;
> 
> So it does nothing for all the stages, then loads, decompresses, etc.
> etc. right at the end at the stage where it is supposed to just be
> running the image.

Yes, there wasn't a need / desire for bootz nor booti to have the
ability to call in to a specific state and run only that far, or resume
from there. It ended up being much more simple to do.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]