[U-Boot] [PATCH v2 1/2] RISCV: image: Add booti support.

[U-Boot] [PATCH v2 1/2] RISCV: image: Add booti support.

[Atish Patra]

This patch adds booti support for RISC-V Linux kernel. The existing
bootm method will also continue to work as it is.

It depends on the following kernel patch which adds the header to the
flat Image.

https://patchwork.kernel.org/patch/10913869/

Tested on HiFive Unleashed and QEMU.

Signed-off-by: Atish Patra <atish.patra@wdc.com>
---
 arch/riscv/lib/Makefile |  1 +
 arch/riscv/lib/image.c  | 53 +++++++++++++++++++++++++++++++++++++++++
 cmd/Kconfig             |  2 +-
 cmd/booti.c             |  8 +++++--
 4 files changed, 61 insertions(+), 3 deletions(-)
 create mode 100644 arch/riscv/lib/image.c

diff --git a/arch/riscv/lib/Makefile b/arch/riscv/lib/Makefile
index 1c332db436a9..6ae6ebbeafda 100644
--- a/arch/riscv/lib/Makefile
+++ b/arch/riscv/lib/Makefile
@@ -7,6 +7,7 @@
 # Rick Chen, Andes Technology Corporation <rick@andestech.com>
 
 obj-$(CONFIG_CMD_BOOTM) += bootm.o
+obj-$(CONFIG_CMD_BOOTI) += bootm.o image.o
 obj-$(CONFIG_CMD_GO) += boot.o
 obj-y	+= cache.o
 obj-$(CONFIG_RISCV_RDTIME) += rdtime.o
diff --git a/arch/riscv/lib/image.c b/arch/riscv/lib/image.c
new file mode 100644
index 000000000000..e8802007c446
--- /dev/null
+++ b/arch/riscv/lib/image.c
@@ -0,0 +1,53 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2019 Western Digital Corporation or its affiliates.
+ * Authors:
+ *	Atish Patra <atish.patra@wdc.com>
+ * Based on arm/lib/image.c
+ */
+
+#include <common.h>
+#include <mapmem.h>
+#include <errno.h>
+#include <linux/sizes.h>
+#include <linux/stddef.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+/* ASCII version of "RISCV" defined in Linux kernel */
+#define LINUX_RISCV_IMAGE_MAGIC 0x5643534952
+
+struct linux_image_h {
+	uint32_t	code0;		/* Executable code */
+	uint32_t	code1;		/* Executable code */
+	uint64_t	text_offset;	/* Image load offset */
+	uint64_t	image_size;	/* Effective Image size */
+	uint64_t	res1;		/* reserved */
+	uint64_t	magic;		/* Magic number */
+	uint32_t	res2;		/* reserved */
+	uint32_t	res3;		/* reserved */
+};
+
+int booti_setup(ulong image, ulong *relocated_addr, ulong *size,
+		bool force_reloc)
+{
+	struct linux_image_h *lhdr;
+
+	lhdr = (struct linux_image_h *)map_sysmem(image, 0);
+
+	if (lhdr->magic != LINUX_RISCV_IMAGE_MAGIC) {
+		puts("Bad Linux RISCV Image magic!\n");
+		return -EINVAL;
+	}
+
+	if (lhdr->image_size == 0) {
+		puts("Image lacks image_size field, error!\n");
+		return -EINVAL;
+	}
+	*size = lhdr->image_size;
+	*relocated_addr = gd->ram_base + lhdr->text_offset;
+
+	unmap_sysmem(lhdr);
+
+	return 0;
+}
diff --git a/cmd/Kconfig b/cmd/Kconfig
index 2bdbfcb3d091..d427b66d3714 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -212,7 +212,7 @@ config CMD_BOOTZ
 
 config CMD_BOOTI
 	bool "booti"
-	depends on ARM64
+	depends on ARM64 || RISCV
 	default y
 	help
 	  Boot an AArch64 Linux Kernel image from memory.
diff --git a/cmd/booti.c b/cmd/booti.c
index 04353b68eccc..c22ba9bae2e4 100644
--- a/cmd/booti.c
+++ b/cmd/booti.c
@@ -77,7 +77,11 @@ int do_booti(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	bootm_disable_interrupts();
 
 	images.os.os = IH_OS_LINUX;
+	#ifdef CONFIG_RISCV_SMODE
+		images.os.arch = IH_ARCH_RISCV;
+	#elif CONFIG_ARM64
 		images.os.arch = IH_ARCH_ARM64;
+	#endif
 	ret = do_bootm_states(cmdtp, flag, argc, argv,
 #ifdef CONFIG_SYS_BOOT_RAMDISK_HIGH
 			      BOOTM_STATE_RAMDISK |
@@ -92,7 +96,7 @@ int do_booti(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 #ifdef CONFIG_SYS_LONGHELP
 static char booti_help_text[] =
 	"[addr [initrd[:size]] [fdt]]\n"
-	"    - boot arm64 Linux Image stored in memory\n"
+	"    - boot arm64/riscv Linux Image stored in memory\n"
 	"\tThe argument 'initrd' is optional and specifies the address\n"
 	"\tof an initrd in memory. The optional parameter ':size' allows\n"
 	"\tspecifying the size of a RAW initrd.\n"
@@ -107,5 +111,5 @@ static char booti_help_text[] =
 
 U_BOOT_CMD(
 	booti,	CONFIG_SYS_MAXARGS,	1,	do_booti,
-	"boot arm64 Linux Image image from memory", booti_help_text
+	"boot arm64/riscv Linux Image image from memory", booti_help_text
 );
-- 
2.21.0

[U-Boot] [PATCH v2 2/2] RISCV: image: Parse Image.gz support in booti.

[Atish Patra]

Add gz parsing logic so that booti can parse both Image
and Image.gz.

Signed-off-by: Atish Patra <atish.patra@wdc.com>
---
 arch/riscv/lib/image.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/arch/riscv/lib/image.c b/arch/riscv/lib/image.c
index e8802007c446..73ebd0da3885 100644
--- a/arch/riscv/lib/image.c
+++ b/arch/riscv/lib/image.c
@@ -9,6 +9,8 @@
 #include <common.h>
 #include <mapmem.h>
 #include <errno.h>
+#include <bootm.h>
+#include <malloc.h>
 #include <linux/sizes.h>
 #include <linux/stddef.h>
 
@@ -16,6 +18,8 @@ DECLARE_GLOBAL_DATA_PTR;
 
 /* ASCII version of "RISCV" defined in Linux kernel */
 #define LINUX_RISCV_IMAGE_MAGIC 0x5643534952
+#define GZ_HEADER_0 0x1f
+#define GZ_HEADER_1 0x8b
 
 struct linux_image_h {
 	uint32_t	code0;		/* Executable code */
@@ -32,9 +36,31 @@ int booti_setup(ulong image, ulong *relocated_addr, ulong *size,
 		bool force_reloc)
 {
 	struct linux_image_h *lhdr;
+	uint8_t *temp;
+	void *dest;
+	ulong dest_end;
+	int ret;
+	/* TODO: Is there a way to figure out length of compressed Image.gz ?
+	 * Otherwise, set it to SYS_BOOTM_LEN which should be sufficient.
+	 */
+	int len = CONFIG_SYS_BOOTM_LEN;
+
+	temp = (uint8_t *)map_sysmem(image, 0);
 
-	lhdr = (struct linux_image_h *)map_sysmem(image, 0);
+	if (*(temp)  == GZ_HEADER_0 && *(temp+1) == GZ_HEADER_1) {
+		/* Need a temporary location to copy the uncompressed image */
+		dest = (void *)map_sysmem(image + 8 * CONFIG_SYS_BOOTM_LEN, 0);
+		ret = bootm_decomp_image(IH_COMP_GZIP, 0, image, IH_TYPE_KERNEL,
+					 dest, (void *)image, len,
+					 CONFIG_SYS_BOOTM_LEN, &dest_end);
+		if (ret)
+			return ret;
+		/* dest_end contains the uncompressed Image size */
+		memmove((void *) image, dest, dest_end);
+		unmap_sysmem(dest);
+	}
 
+	lhdr = (struct linux_image_h *)temp;
 	if (lhdr->magic != LINUX_RISCV_IMAGE_MAGIC) {
 		puts("Bad Linux RISCV Image magic!\n");
 		return -EINVAL;
-- 
2.21.0

[U-Boot] [PATCH v2 2/2] RISCV: image: Parse Image.gz support in booti.

[Marek Vasut]

On 4/25/19 9:56 PM, Atish Patra wrote:
> Add gz parsing logic so that booti can parse both Image
> and Image.gz.
> 
> Signed-off-by: Atish Patra <atish.patra@wdc.com>
> ---
>  arch/riscv/lib/image.c | 28 +++++++++++++++++++++++++++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/lib/image.c b/arch/riscv/lib/image.c
> index e8802007c446..73ebd0da3885 100644
> --- a/arch/riscv/lib/image.c
> +++ b/arch/riscv/lib/image.c
> @@ -9,6 +9,8 @@
>  #include <common.h>
>  #include <mapmem.h>
>  #include <errno.h>
> +#include <bootm.h>
> +#include <malloc.h>
>  #include <linux/sizes.h>
>  #include <linux/stddef.h>
>  
> @@ -16,6 +18,8 @@ DECLARE_GLOBAL_DATA_PTR;
>  
>  /* ASCII version of "RISCV" defined in Linux kernel */
>  #define LINUX_RISCV_IMAGE_MAGIC 0x5643534952
> +#define GZ_HEADER_0 0x1f
> +#define GZ_HEADER_1 0x8b
>  
>  struct linux_image_h {
>  	uint32_t	code0;		/* Executable code */
> @@ -32,9 +36,31 @@ int booti_setup(ulong image, ulong *relocated_addr, ulong *size,
>  		bool force_reloc)
>  {
>  	struct linux_image_h *lhdr;
> +	uint8_t *temp;
> +	void *dest;
> +	ulong dest_end;
> +	int ret;
> +	/* TODO: Is there a way to figure out length of compressed Image.gz ?
> +	 * Otherwise, set it to SYS_BOOTM_LEN which should be sufficient.
> +	 */

Presumably this is a RFC patch then ?

What happens if you have two (or more) gzip-ed files back-to-back ?
Wouldn't you then decompress both ? That might lead to all kinds of
problems.

> +	int len = CONFIG_SYS_BOOTM_LEN;
> +
> +	temp = (uint8_t *)map_sysmem(image, 0);

Is the type cast really needed ?

> -	lhdr = (struct linux_image_h *)map_sysmem(image, 0);
> +	if (*(temp)  == GZ_HEADER_0 && *(temp+1) == GZ_HEADER_1) {

Wrap the image in some fitImage or so contained, mark the image as gzip
compressed there and all this goes away.

> +		/* Need a temporary location to copy the uncompressed image */
> +		dest = (void *)map_sysmem(image + 8 * CONFIG_SYS_BOOTM_LEN, 0);
> +		ret = bootm_decomp_image(IH_COMP_GZIP, 0, image, IH_TYPE_KERNEL,
> +					 dest, (void *)image, len,
> +					 CONFIG_SYS_BOOTM_LEN, &dest_end);
> +		if (ret)
> +			return ret;
> +		/* dest_end contains the uncompressed Image size */
> +		memmove((void *) image, dest, dest_end);
> +		unmap_sysmem(dest);
> +	}
>  
> +	lhdr = (struct linux_image_h *)temp;
>  	if (lhdr->magic != LINUX_RISCV_IMAGE_MAGIC) {
>  		puts("Bad Linux RISCV Image magic!\n");
>  		return -EINVAL;
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2 2/2] RISCV: image: Parse Image.gz support in booti.

[Atish Patra]

On 4/25/19 10:33 PM, Marek Vasut wrote:
> On 4/25/19 9:56 PM, Atish Patra wrote:
>> Add gz parsing logic so that booti can parse both Image
>> and Image.gz.
>>
>> Signed-off-by: Atish Patra <atish.patra@wdc.com>
>> ---
>>   arch/riscv/lib/image.c | 28 +++++++++++++++++++++++++++-
>>   1 file changed, 27 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/riscv/lib/image.c b/arch/riscv/lib/image.c
>> index e8802007c446..73ebd0da3885 100644
>> --- a/arch/riscv/lib/image.c
>> +++ b/arch/riscv/lib/image.c
>> @@ -9,6 +9,8 @@
>>   #include <common.h>
>>   #include <mapmem.h>
>>   #include <errno.h>
>> +#include <bootm.h>
>> +#include <malloc.h>
>>   #include <linux/sizes.h>
>>   #include <linux/stddef.h>
>>   
>> @@ -16,6 +18,8 @@ DECLARE_GLOBAL_DATA_PTR;
>>   
>>   /* ASCII version of "RISCV" defined in Linux kernel */
>>   #define LINUX_RISCV_IMAGE_MAGIC 0x5643534952
>> +#define GZ_HEADER_0 0x1f
>> +#define GZ_HEADER_1 0x8b
>>   
>>   struct linux_image_h {
>>   	uint32_t	code0;		/* Executable code */
>> @@ -32,9 +36,31 @@ int booti_setup(ulong image, ulong *relocated_addr, ulong *size,
>>   		bool force_reloc)
>>   {
>>   	struct linux_image_h *lhdr;
>> +	uint8_t *temp;
>> +	void *dest;
>> +	ulong dest_end;
>> +	int ret;
>> +	/* TODO: Is there a way to figure out length of compressed Image.gz ?
>> +	 * Otherwise, set it to SYS_BOOTM_LEN which should be sufficient.
>> +	 */
> 
> Presumably this is a RFC patch then ?
> 
Yeah. I am not very sure if there is a better way to determine the size.
Hence the comment. I am hoping somebody here would suggest something ;).

> What happens if you have two (or more) gzip-ed files back-to-back ?
> Wouldn't you then decompress both ? That might lead to all kinds of
> problems.
> 

That will be catastrophic. But this was intended only for booti and the 
expectation was that only Image.gz must be loaded before this.

Having said that, if we can find a reliable way of figuring out the 
compressed file size here, we can get rid of this hack.

>> +	int len = CONFIG_SYS_BOOTM_LEN;
>> +
>> +	temp = (uint8_t *)map_sysmem(image, 0);
> 
> Is the type cast really needed ?
> 

Just wanted to be explicit. Will remove it.

>> -	lhdr = (struct linux_image_h *)map_sysmem(image, 0);
>> +	if (*(temp)  == GZ_HEADER_0 && *(temp+1) == GZ_HEADER_1) {
> 
> Wrap the image in some fitImage or so contained, mark the image as gzip
> compressed there and all this goes away.
> 

Yes. FIT image parsing can be done in that way. However, the idea was 
here to load Image.gz directly. Image.gz is default compressed Linux 
kernel image format in RISC-V.

Regards,
Atish
>> +		/* Need a temporary location to copy the uncompressed image */
>> +		dest = (void *)map_sysmem(image + 8 * CONFIG_SYS_BOOTM_LEN, 0);
>> +		ret = bootm_decomp_image(IH_COMP_GZIP, 0, image, IH_TYPE_KERNEL,
>> +					 dest, (void *)image, len,
>> +					 CONFIG_SYS_BOOTM_LEN, &dest_end);
>> +		if (ret)
>> +			return ret;
>> +		/* dest_end contains the uncompressed Image size */
>> +		memmove((void *) image, dest, dest_end);
>> +		unmap_sysmem(dest);
>> +	}
>>   
>> +	lhdr = (struct linux_image_h *)temp;
>>   	if (lhdr->magic != LINUX_RISCV_IMAGE_MAGIC) {
>>   		puts("Bad Linux RISCV Image magic!\n");
>>   		return -EINVAL;
>>
> 
> 

[U-Boot] [PATCH v2 2/2] RISCV: image: Parse Image.gz support in booti.

[Marek Vasut]

On 4/26/19 7:08 PM, Atish Patra wrote:
> On 4/25/19 10:33 PM, Marek Vasut wrote:
>> On 4/25/19 9:56 PM, Atish Patra wrote:
>>> Add gz parsing logic so that booti can parse both Image
>>> and Image.gz.
>>>
>>> Signed-off-by: Atish Patra <atish.patra@wdc.com>
>>> ---
>>>   arch/riscv/lib/image.c | 28 +++++++++++++++++++++++++++-
>>>   1 file changed, 27 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/riscv/lib/image.c b/arch/riscv/lib/image.c
>>> index e8802007c446..73ebd0da3885 100644
>>> --- a/arch/riscv/lib/image.c
>>> +++ b/arch/riscv/lib/image.c
>>> @@ -9,6 +9,8 @@
>>>   #include <common.h>
>>>   #include <mapmem.h>
>>>   #include <errno.h>
>>> +#include <bootm.h>
>>> +#include <malloc.h>
>>>   #include <linux/sizes.h>
>>>   #include <linux/stddef.h>
>>>   @@ -16,6 +18,8 @@ DECLARE_GLOBAL_DATA_PTR;
>>>     /* ASCII version of "RISCV" defined in Linux kernel */
>>>   #define LINUX_RISCV_IMAGE_MAGIC 0x5643534952
>>> +#define GZ_HEADER_0 0x1f
>>> +#define GZ_HEADER_1 0x8b
>>>     struct linux_image_h {
>>>       uint32_t    code0;        /* Executable code */
>>> @@ -32,9 +36,31 @@ int booti_setup(ulong image, ulong
>>> *relocated_addr, ulong *size,
>>>           bool force_reloc)
>>>   {
>>>       struct linux_image_h *lhdr;
>>> +    uint8_t *temp;
>>> +    void *dest;
>>> +    ulong dest_end;
>>> +    int ret;
>>> +    /* TODO: Is there a way to figure out length of compressed
>>> Image.gz ?
>>> +     * Otherwise, set it to SYS_BOOTM_LEN which should be sufficient.
>>> +     */
>>
>> Presumably this is a RFC patch then ?
>>
> Yeah. I am not very sure if there is a better way to determine the size.
> Hence the comment. I am hoping somebody here would suggest something ;).

Mark the patch as RFC next time please.

>> What happens if you have two (or more) gzip-ed files back-to-back ?
>> Wouldn't you then decompress both ? That might lead to all kinds of
>> problems.
>>
> 
> That will be catastrophic. But this was intended only for booti and the
> expectation was that only Image.gz must be loaded before this.

That also means it's horribly fragile.

> Having said that, if we can find a reliable way of figuring out the
> compressed file size here, we can get rid of this hack.

See below

>>> +    int len = CONFIG_SYS_BOOTM_LEN;
>>> +
>>> +    temp = (uint8_t *)map_sysmem(image, 0);
>>
>> Is the type cast really needed ?
>>
> 
> Just wanted to be explicit. Will remove it.
> 
>>> -    lhdr = (struct linux_image_h *)map_sysmem(image, 0);
>>> +    if (*(temp)  == GZ_HEADER_0 && *(temp+1) == GZ_HEADER_1) {
>>
>> Wrap the image in some fitImage or so contained, mark the image as gzip
>> compressed there and all this goes away.
>>
> 
> Yes. FIT image parsing can be done in that way. However, the idea was
> here to load Image.gz directly. Image.gz is default compressed Linux
> kernel image format in RISC-V.

Sigh, and the image header is compressed as well, so there's no way to
identify the image format, right ? And there's no decompressor, so the
dcompressing has to be done by bootloader, which would need some sort of
very smart way of figuring out which exact compression method is used ?

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2 2/2] RISCV: image: Parse Image.gz support in booti.

[Atish Patra]

On 4/26/19 11:05 AM, Marek Vasut wrote:
> On 4/26/19 7:08 PM, Atish Patra wrote:
>> On 4/25/19 10:33 PM, Marek Vasut wrote:
>>> On 4/25/19 9:56 PM, Atish Patra wrote:
>>>> Add gz parsing logic so that booti can parse both Image
>>>> and Image.gz.
>>>>
>>>> Signed-off-by: Atish Patra <atish.patra@wdc.com>
>>>> ---
>>>>    arch/riscv/lib/image.c | 28 +++++++++++++++++++++++++++-
>>>>    1 file changed, 27 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/riscv/lib/image.c b/arch/riscv/lib/image.c
>>>> index e8802007c446..73ebd0da3885 100644
>>>> --- a/arch/riscv/lib/image.c
>>>> +++ b/arch/riscv/lib/image.c
>>>> @@ -9,6 +9,8 @@
>>>>    #include <common.h>
>>>>    #include <mapmem.h>
>>>>    #include <errno.h>
>>>> +#include <bootm.h>
>>>> +#include <malloc.h>
>>>>    #include <linux/sizes.h>
>>>>    #include <linux/stddef.h>
>>>>    @@ -16,6 +18,8 @@ DECLARE_GLOBAL_DATA_PTR;
>>>>      /* ASCII version of "RISCV" defined in Linux kernel */
>>>>    #define LINUX_RISCV_IMAGE_MAGIC 0x5643534952
>>>> +#define GZ_HEADER_0 0x1f
>>>> +#define GZ_HEADER_1 0x8b
>>>>      struct linux_image_h {
>>>>        uint32_t    code0;        /* Executable code */
>>>> @@ -32,9 +36,31 @@ int booti_setup(ulong image, ulong
>>>> *relocated_addr, ulong *size,
>>>>            bool force_reloc)
>>>>    {
>>>>        struct linux_image_h *lhdr;
>>>> +    uint8_t *temp;
>>>> +    void *dest;
>>>> +    ulong dest_end;
>>>> +    int ret;
>>>> +    /* TODO: Is there a way to figure out length of compressed
>>>> Image.gz ?
>>>> +     * Otherwise, set it to SYS_BOOTM_LEN which should be sufficient.
>>>> +     */
>>>
>>> Presumably this is a RFC patch then ?
>>>
>> Yeah. I am not very sure if there is a better way to determine the size.
>> Hence the comment. I am hoping somebody here would suggest something ;).
> 
> Mark the patch as RFC next time please.
> 

Sure. Sorry for not marking RFC in the first time.

>>> What happens if you have two (or more) gzip-ed files back-to-back ?
>>> Wouldn't you then decompress both ? That might lead to all kinds of
>>> problems.
>>>
>>
>> That will be catastrophic. But this was intended only for booti and the
>> expectation was that only Image.gz must be loaded before this.
> 
> That also means it's horribly fragile.
> 
>> Having said that, if we can find a reliable way of figuring out the
>> compressed file size here, we can get rid of this hack.
> 
> See below
> 
>>>> +    int len = CONFIG_SYS_BOOTM_LEN;
>>>> +
>>>> +    temp = (uint8_t *)map_sysmem(image, 0);
>>>
>>> Is the type cast really needed ?
>>>
>>
>> Just wanted to be explicit. Will remove it.
>>
>>>> -    lhdr = (struct linux_image_h *)map_sysmem(image, 0);
>>>> +    if (*(temp)  == GZ_HEADER_0 && *(temp+1) == GZ_HEADER_1) {
>>>
>>> Wrap the image in some fitImage or so contained, mark the image as gzip
>>> compressed there and all this goes away.
>>>
>>
>> Yes. FIT image parsing can be done in that way. However, the idea was
>> here to load Image.gz directly. Image.gz is default compressed Linux
>> kernel image format in RISC-V.
> 
> Sigh, and the image header is compressed as well, so there's no way to
> identify the image format, right ? And there's no decompressor, so the
> dcompressing has to be done by bootloader, which would need some sort of
> very smart way of figuring out which exact compression method is used ?
> 
Yes. Image.gz is always gunzip. So checking first two bytes is enough to 
confirm that it is a gz file.

The tricky part is length of the compressed file. I took another look at 
the gunzip implementation in U-Boot. It looks like to me that compressed 
header length just to parse the header correctly. It doesn't actually 
use the "length" to decompress. In fact, it updates the length with 
uncompressed bytes after the decompression.


Regards,
Atish

[U-Boot] [PATCH v2 2/2] RISCV: image: Parse Image.gz support in booti.

[Marek Vasut]

On 4/30/19 3:27 AM, Atish Patra wrote:

[...]

>>> Yes. FIT image parsing can be done in that way. However, the idea was
>>> here to load Image.gz directly. Image.gz is default compressed Linux
>>> kernel image format in RISC-V.
>>
>> Sigh, and the image header is compressed as well, so there's no way to
>> identify the image format, right ? And there's no decompressor, so the
>> dcompressing has to be done by bootloader, which would need some sort of
>> very smart way of figuring out which exact compression method is used ?
>>
> Yes. Image.gz is always gunzip. So checking first two bytes is enough to
> confirm that it is a gz file.

What happens once people start feeding it more exotic compression
methods, like LZ4 or LZO or LZMA for example ?

> The tricky part is length of the compressed file. I took another look at
> the gunzip implementation in U-Boot. It looks like to me that compressed
> header length just to parse the header correctly. It doesn't actually
> use the "length" to decompress. In fact, it updates the length with
> uncompressed bytes after the decompression.

That's possible.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2 2/2] RISCV: image: Parse Image.gz support in booti.

[Atish Patra]

On 4/30/19 2:52 AM, Marek Vasut wrote:
> On 4/30/19 3:27 AM, Atish Patra wrote:
> 
> [...]
> 
>>>> Yes. FIT image parsing can be done in that way. However, the idea was
>>>> here to load Image.gz directly. Image.gz is default compressed Linux
>>>> kernel image format in RISC-V.
>>>
>>> Sigh, and the image header is compressed as well, so there's no way to
>>> identify the image format, right ? And there's no decompressor, so the
>>> dcompressing has to be done by bootloader, which would need some sort of
>>> very smart way of figuring out which exact compression method is used ?
>>>
>> Yes. Image.gz is always gunzip. So checking first two bytes is enough to
>> confirm that it is a gz file.
> 
> What happens once people start feeding it more exotic compression
> methods, like LZ4 or LZO or LZMA for example ?
> 

booti command help will clearly state that it can only boot kernel from 
Image or Image.gz.

static char booti_help_text[] =
  	"[addr [initrd[:size]] [fdt]]\n"
-	"    - boot arm64 Linux Image stored in memory\n"
+	"    - boot arm64 Linux Image or riscv Linux Image/Image.gz stored in 
memory\n"

(I will update the help text with Image.gz part)

Anything other than that, it will fail with following error.

"Bad Linux RISCV Image magic!"

>> The tricky part is length of the compressed file. I took another look at
>> the gunzip implementation in U-Boot. It looks like to me that compressed
>> header length just to parse the header correctly. It doesn't actually
>> use the "length" to decompress. In fact, it updates the length with
>> uncompressed bytes after the decompression.
> 
> That's possible.
> 

David suggested a better idea.

1. User can supply kernel_size and we can store in environment variable.
2. If the size is empty or greater than CONFIG_SYS_BOOTM_LEN, booti 
fails with appropriate error message.

We will update the documents to add the additional step for Image.gz

I am fine with either approach. Any preference ?

Regards,
Atish

[U-Boot] [PATCH v2 2/2] RISCV: image: Parse Image.gz support in booti.

[Marek Vasut]

On 4/30/19 8:13 PM, Atish Patra wrote:
> On 4/30/19 2:52 AM, Marek Vasut wrote:
>> On 4/30/19 3:27 AM, Atish Patra wrote:
>>
>> [...]
>>
>>>>> Yes. FIT image parsing can be done in that way. However, the idea was
>>>>> here to load Image.gz directly. Image.gz is default compressed Linux
>>>>> kernel image format in RISC-V.
>>>>
>>>> Sigh, and the image header is compressed as well, so there's no way to
>>>> identify the image format, right ? And there's no decompressor, so the
>>>> dcompressing has to be done by bootloader, which would need some
>>>> sort of
>>>> very smart way of figuring out which exact compression method is used ?
>>>>
>>> Yes. Image.gz is always gunzip. So checking first two bytes is enough to
>>> confirm that it is a gz file.
>>
>> What happens once people start feeding it more exotic compression
>> methods, like LZ4 or LZO or LZMA for example ?
>>
> 
> booti command help will clearly state that it can only boot kernel from
> Image or Image.gz.
> 
> static char booti_help_text[] =
>      "[addr [initrd[:size]] [fdt]]\n"
> -    "    - boot arm64 Linux Image stored in memory\n"
> +    "    - boot arm64 Linux Image or riscv Linux Image/Image.gz stored
> in memory\n"

Obvious question -- does this Image.gz stuff apply to arm64 ?

> 
> (I will update the help text with Image.gz part)
> 
> Anything other than that, it will fail with following error.
> 
> "Bad Linux RISCV Image magic!"

Right, so we're implementing file(1)-lite here to detect the format.

>>> The tricky part is length of the compressed file. I took another look at
>>> the gunzip implementation in U-Boot. It looks like to me that compressed
>>> header length just to parse the header correctly. It doesn't actually
>>> use the "length" to decompress. In fact, it updates the length with
>>> uncompressed bytes after the decompression.
>>
>> That's possible.
>>
> 
> David suggested a better idea.
> 
> 1. User can supply kernel_size and we can store in environment variable.
> 2. If the size is empty or greater than CONFIG_SYS_BOOTM_LEN, booti
> fails with appropriate error message.

You can keep decompressing until you reach $bootm_len, sure .

> We will update the documents to add the additional step for Image.gz
> 
> I am fine with either approach. Any preference ?
> 
> Regards,
> Atish


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2 2/2] RISCV: image: Parse Image.gz support in booti.

[Atish Patra]

On 4/30/19 12:11 PM, Marek Vasut wrote:
> On 4/30/19 8:13 PM, Atish Patra wrote:
>> On 4/30/19 2:52 AM, Marek Vasut wrote:
>>> On 4/30/19 3:27 AM, Atish Patra wrote:
>>>
>>> [...]
>>>
>>>>>> Yes. FIT image parsing can be done in that way. However, the idea was
>>>>>> here to load Image.gz directly. Image.gz is default compressed Linux
>>>>>> kernel image format in RISC-V.
>>>>>
>>>>> Sigh, and the image header is compressed as well, so there's no way to
>>>>> identify the image format, right ? And there's no decompressor, so the
>>>>> dcompressing has to be done by bootloader, which would need some
>>>>> sort of
>>>>> very smart way of figuring out which exact compression method is used ?
>>>>>
>>>> Yes. Image.gz is always gunzip. So checking first two bytes is enough to
>>>> confirm that it is a gz file.
>>>
>>> What happens once people start feeding it more exotic compression
>>> methods, like LZ4 or LZO or LZMA for example ?
>>>
>>
>> booti command help will clearly state that it can only boot kernel from
>> Image or Image.gz.
>>
>> static char booti_help_text[] =
>>       "[addr [initrd[:size]] [fdt]]\n"
>> -    "    - boot arm64 Linux Image stored in memory\n"
>> +    "    - boot arm64 Linux Image or riscv Linux Image/Image.gz stored
>> in memory\n"
> 
> Obvious question -- does this Image.gz stuff apply to arm64 ?
> 

arm64 builds Image.gz but booti for arm64 doesn't use it. I guess 
Image.gz can be used in FIT image for ARM64 but I am not sure which 
platform actually uses it.

This patch only enables support for RISC-V.

>>
>> (I will update the help text with Image.gz part)
>>
>> Anything other than that, it will fail with following error.
>>
>> "Bad Linux RISCV Image magic!"
> 
> Right, so we're implementing file(1)-lite here to detect the format.
> 
>>>> The tricky part is length of the compressed file. I took another look at
>>>> the gunzip implementation in U-Boot. It looks like to me that compressed
>>>> header length just to parse the header correctly. It doesn't actually
>>>> use the "length" to decompress. In fact, it updates the length with
>>>> uncompressed bytes after the decompression.
>>>
>>> That's possible.
>>>
>>
>> David suggested a better idea.
>>
>> 1. User can supply kernel_size and we can store in environment variable.
>> 2. If the size is empty or greater than CONFIG_SYS_BOOTM_LEN, booti
>> fails with appropriate error message.
> 
> You can keep decompressing until you reach $bootm_len, sure .

Decompression happens for actual uncompressed length which is encoded 
into the compressed file footer. So we don't have to worry about how 
much we decompress. But a correct file_size helps to avoid decompressing 
a possible corrupt compressed file.

I will update the patch as per David's suggestion.

Regards,
Atish
> 
>> We will update the documents to add the additional step for Image.gz
>>
>> I am fine with either approach. Any preference ?
>>
>> Regards,
>> Atish
> 
> 

[U-Boot] [PATCH v2 2/2] RISCV: image: Parse Image.gz support in booti.

[Marek Vasut]

On 4/30/19 10:38 PM, Atish Patra wrote:
> On 4/30/19 12:11 PM, Marek Vasut wrote:
>> On 4/30/19 8:13 PM, Atish Patra wrote:
>>> On 4/30/19 2:52 AM, Marek Vasut wrote:
>>>> On 4/30/19 3:27 AM, Atish Patra wrote:
>>>>
>>>> [...]
>>>>
>>>>>>> Yes. FIT image parsing can be done in that way. However, the idea
>>>>>>> was
>>>>>>> here to load Image.gz directly. Image.gz is default compressed Linux
>>>>>>> kernel image format in RISC-V.
>>>>>>
>>>>>> Sigh, and the image header is compressed as well, so there's no
>>>>>> way to
>>>>>> identify the image format, right ? And there's no decompressor, so
>>>>>> the
>>>>>> dcompressing has to be done by bootloader, which would need some
>>>>>> sort of
>>>>>> very smart way of figuring out which exact compression method is
>>>>>> used ?
>>>>>>
>>>>> Yes. Image.gz is always gunzip. So checking first two bytes is
>>>>> enough to
>>>>> confirm that it is a gz file.
>>>>
>>>> What happens once people start feeding it more exotic compression
>>>> methods, like LZ4 or LZO or LZMA for example ?
>>>>
>>>
>>> booti command help will clearly state that it can only boot kernel from
>>> Image or Image.gz.
>>>
>>> static char booti_help_text[] =
>>>       "[addr [initrd[:size]] [fdt]]\n"
>>> -    "    - boot arm64 Linux Image stored in memory\n"
>>> +    "    - boot arm64 Linux Image or riscv Linux Image/Image.gz stored
>>> in memory\n"
>>
>> Obvious question -- does this Image.gz stuff apply to arm64 ?
>>
> 
> arm64 builds Image.gz but booti for arm64 doesn't use it. I guess
> Image.gz can be used in FIT image for ARM64 but I am not sure which
> platform actually uses it.
> 
> This patch only enables support for RISC-V.

Can't this be made generic ?

[...]

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2 2/2] RISCV: image: Parse Image.gz support in booti.

[Atish Patra]

On 4/30/19 2:42 PM, Marek Vasut wrote:
> On 4/30/19 10:38 PM, Atish Patra wrote:
>> On 4/30/19 12:11 PM, Marek Vasut wrote:
>>> On 4/30/19 8:13 PM, Atish Patra wrote:
>>>> On 4/30/19 2:52 AM, Marek Vasut wrote:
>>>>> On 4/30/19 3:27 AM, Atish Patra wrote:
>>>>>
>>>>> [...]
>>>>>
>>>>>>>> Yes. FIT image parsing can be done in that way. However, the idea
>>>>>>>> was
>>>>>>>> here to load Image.gz directly. Image.gz is default compressed Linux
>>>>>>>> kernel image format in RISC-V.
>>>>>>>
>>>>>>> Sigh, and the image header is compressed as well, so there's no
>>>>>>> way to
>>>>>>> identify the image format, right ? And there's no decompressor, so
>>>>>>> the
>>>>>>> dcompressing has to be done by bootloader, which would need some
>>>>>>> sort of
>>>>>>> very smart way of figuring out which exact compression method is
>>>>>>> used ?
>>>>>>>
>>>>>> Yes. Image.gz is always gunzip. So checking first two bytes is
>>>>>> enough to
>>>>>> confirm that it is a gz file.
>>>>>
>>>>> What happens once people start feeding it more exotic compression
>>>>> methods, like LZ4 or LZO or LZMA for example ?
>>>>>
>>>>
>>>> booti command help will clearly state that it can only boot kernel from
>>>> Image or Image.gz.
>>>>
>>>> static char booti_help_text[] =
>>>>        "[addr [initrd[:size]] [fdt]]\n"
>>>> -    "    - boot arm64 Linux Image stored in memory\n"
>>>> +    "    - boot arm64 Linux Image or riscv Linux Image/Image.gz stored
>>>> in memory\n"
>>>
>>> Obvious question -- does this Image.gz stuff apply to arm64 ?
>>>
>>
>> arm64 builds Image.gz but booti for arm64 doesn't use it. I guess
>> Image.gz can be used in FIT image for ARM64 but I am not sure which
>> platform actually uses it.
>>
>> This patch only enables support for RISC-V.
> 
> Can't this be made generic ?
> 

I think so if I just move the gzip detection and decompression code to 
cmd/booti.c.

I will update the v3 patch with this.

Regards,
Atish

> [...]
> 

[U-Boot] [PATCH v2 2/2] RISCV: image: Parse Image.gz support in booti.

[Marek Vasut]

On 5/1/19 12:06 AM, Atish Patra wrote:
> On 4/30/19 2:42 PM, Marek Vasut wrote:
>> On 4/30/19 10:38 PM, Atish Patra wrote:
>>> On 4/30/19 12:11 PM, Marek Vasut wrote:
>>>> On 4/30/19 8:13 PM, Atish Patra wrote:
>>>>> On 4/30/19 2:52 AM, Marek Vasut wrote:
>>>>>> On 4/30/19 3:27 AM, Atish Patra wrote:
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>>>> Yes. FIT image parsing can be done in that way. However, the idea
>>>>>>>>> was
>>>>>>>>> here to load Image.gz directly. Image.gz is default compressed
>>>>>>>>> Linux
>>>>>>>>> kernel image format in RISC-V.
>>>>>>>>
>>>>>>>> Sigh, and the image header is compressed as well, so there's no
>>>>>>>> way to
>>>>>>>> identify the image format, right ? And there's no decompressor, so
>>>>>>>> the
>>>>>>>> dcompressing has to be done by bootloader, which would need some
>>>>>>>> sort of
>>>>>>>> very smart way of figuring out which exact compression method is
>>>>>>>> used ?
>>>>>>>>
>>>>>>> Yes. Image.gz is always gunzip. So checking first two bytes is
>>>>>>> enough to
>>>>>>> confirm that it is a gz file.
>>>>>>
>>>>>> What happens once people start feeding it more exotic compression
>>>>>> methods, like LZ4 or LZO or LZMA for example ?
>>>>>>
>>>>>
>>>>> booti command help will clearly state that it can only boot kernel
>>>>> from
>>>>> Image or Image.gz.
>>>>>
>>>>> static char booti_help_text[] =
>>>>>        "[addr [initrd[:size]] [fdt]]\n"
>>>>> -    "    - boot arm64 Linux Image stored in memory\n"
>>>>> +    "    - boot arm64 Linux Image or riscv Linux Image/Image.gz
>>>>> stored
>>>>> in memory\n"
>>>>
>>>> Obvious question -- does this Image.gz stuff apply to arm64 ?
>>>>
>>>
>>> arm64 builds Image.gz but booti for arm64 doesn't use it. I guess
>>> Image.gz can be used in FIT image for ARM64 but I am not sure which
>>> platform actually uses it.
>>>
>>> This patch only enables support for RISC-V.
>>
>> Can't this be made generic ?
>>
> 
> I think so if I just move the gzip detection and decompression code to
> cmd/booti.c.
> 
> I will update the v3 patch with this.

Nice, thanks.

But since you're basically implementing file(1)-lite, I wonder whether
there isn't similar code somewhere in u-boot already.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2 2/2] RISCV: image: Parse Image.gz support in booti.

[Atish Patra]

On 5/1/19 3:34 AM, Marek Vasut wrote:
> On 5/1/19 12:06 AM, Atish Patra wrote:
>> On 4/30/19 2:42 PM, Marek Vasut wrote:
>>> On 4/30/19 10:38 PM, Atish Patra wrote:
>>>> On 4/30/19 12:11 PM, Marek Vasut wrote:
>>>>> On 4/30/19 8:13 PM, Atish Patra wrote:
>>>>>> On 4/30/19 2:52 AM, Marek Vasut wrote:
>>>>>>> On 4/30/19 3:27 AM, Atish Patra wrote:
>>>>>>>
>>>>>>> [...]
>>>>>>>
>>>>>>>>>> Yes. FIT image parsing can be done in that way. However, the idea
>>>>>>>>>> was
>>>>>>>>>> here to load Image.gz directly. Image.gz is default compressed
>>>>>>>>>> Linux
>>>>>>>>>> kernel image format in RISC-V.
>>>>>>>>>
>>>>>>>>> Sigh, and the image header is compressed as well, so there's no
>>>>>>>>> way to
>>>>>>>>> identify the image format, right ? And there's no decompressor, so
>>>>>>>>> the
>>>>>>>>> dcompressing has to be done by bootloader, which would need some
>>>>>>>>> sort of
>>>>>>>>> very smart way of figuring out which exact compression method is
>>>>>>>>> used ?
>>>>>>>>>
>>>>>>>> Yes. Image.gz is always gunzip. So checking first two bytes is
>>>>>>>> enough to
>>>>>>>> confirm that it is a gz file.
>>>>>>>
>>>>>>> What happens once people start feeding it more exotic compression
>>>>>>> methods, like LZ4 or LZO or LZMA for example ?
>>>>>>>
>>>>>>
>>>>>> booti command help will clearly state that it can only boot kernel
>>>>>> from
>>>>>> Image or Image.gz.
>>>>>>
>>>>>> static char booti_help_text[] =
>>>>>>         "[addr [initrd[:size]] [fdt]]\n"
>>>>>> -    "    - boot arm64 Linux Image stored in memory\n"
>>>>>> +    "    - boot arm64 Linux Image or riscv Linux Image/Image.gz
>>>>>> stored
>>>>>> in memory\n"
>>>>>
>>>>> Obvious question -- does this Image.gz stuff apply to arm64 ?
>>>>>
>>>>
>>>> arm64 builds Image.gz but booti for arm64 doesn't use it. I guess
>>>> Image.gz can be used in FIT image for ARM64 but I am not sure which
>>>> platform actually uses it.
>>>>
>>>> This patch only enables support for RISC-V.
>>>
>>> Can't this be made generic ?
>>>
>>
>> I think so if I just move the gzip detection and decompression code to
>> cmd/booti.c.
>>
>> I will update the v3 patch with this.
> 
> Nice, thanks.
> 
> But since you're basically implementing file(1)-lite, I wonder whether
> there isn't similar code somewhere in u-boot already.
> 
I was hoping to find one as well. But I couldn't. I can add a generic gz 
detection function(same file(1)-lite approach) to a lib/gunzip.c

Regards,
Atish