[PATCH 0/2] xen/arm: Support compressed uImages

[PATCH 0/2] xen/arm: Support compressed uImages

[Michal Orzel]

This series adds support for booting gzip compressed images with u-boot header.
Currently Xen does not support such images because we are trying to decompress
the kernel before probing uImage header.

The problem can be solved using 2 different approaches:
1) Split uImage probing into 2 stages. The first stage is called before
   decompression, does the usual probing and sets up correctly module start
   address and size by taking the uImage header size into account. The second
   stage is called after decompression to update the zimage.{kernel_addr,len}.

2) Call the decompression function from within the uImage probing to avoid the
   split and to make the function self-containing. This way the only case for
   falling through to try to probe other image types is when there is no u-boot
   header detected.

In this series the second approach is taken that results in a better looking
code.

Michal Orzel (2):
  xen/arm: Move kernel_uimage_probe definition after kernel_decompress
  xen/arm: Add support for booting gzip compressed uImages

 docs/misc/arm/booting.txt |   3 -
 xen/arch/arm/kernel.c     | 197 +++++++++++++++++++++++---------------
 2 files changed, 118 insertions(+), 82 deletions(-)

-- 
2.25.1

[PATCH 1/2] xen/arm: Move kernel_uimage_probe definition after kernel_decompress

[Michal Orzel]

In a follow-up patch, we will be calling kernel_decompress function from
within kernel_uimage_probe to support booting compressed images with
u-boot header. Therefore, move the kernel_uimage_probe definition after
kernel_decompress so that the latter is visible to the former.

No functional change intended.

Signed-off-by: Michal Orzel <michal.orzel@amd.com>
---
 xen/arch/arm/kernel.c | 146 +++++++++++++++++++++---------------------
 1 file changed, 73 insertions(+), 73 deletions(-)

diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index 36081e73f140..068fbf88e492 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -186,6 +186,79 @@ static void __init kernel_zimage_load(struct kernel_info *info)
     iounmap(kernel);
 }
 
+static __init uint32_t output_length(char *image, unsigned long image_len)
+{
+    return *(uint32_t *)&image[image_len - 4];
+}
+
+static __init int kernel_decompress(struct bootmodule *mod)
+{
+    char *output, *input;
+    char magic[2];
+    int rc;
+    unsigned int kernel_order_out;
+    paddr_t output_size;
+    struct page_info *pages;
+    mfn_t mfn;
+    int i;
+    paddr_t addr = mod->start;
+    paddr_t size = mod->size;
+
+    if ( size < 2 )
+        return -EINVAL;
+
+    copy_from_paddr(magic, addr, sizeof(magic));
+
+    /* only gzip is supported */
+    if ( !gzip_check(magic, size) )
+        return -EINVAL;
+
+    input = ioremap_cache(addr, size);
+    if ( input == NULL )
+        return -EFAULT;
+
+    output_size = output_length(input, size);
+    kernel_order_out = get_order_from_bytes(output_size);
+    pages = alloc_domheap_pages(NULL, kernel_order_out, 0);
+    if ( pages == NULL )
+    {
+        iounmap(input);
+        return -ENOMEM;
+    }
+    mfn = page_to_mfn(pages);
+    output = __vmap(&mfn, 1 << kernel_order_out, 1, 1, PAGE_HYPERVISOR, VMAP_DEFAULT);
+
+    rc = perform_gunzip(output, input, size);
+    clean_dcache_va_range(output, output_size);
+    iounmap(input);
+    vunmap(output);
+
+    if ( rc )
+    {
+        free_domheap_pages(pages, kernel_order_out);
+        return rc;
+    }
+
+    mod->start = page_to_maddr(pages);
+    mod->size = output_size;
+
+    /*
+     * Need to free pages after output_size here because they won't be
+     * freed by discard_initial_modules
+     */
+    i = PFN_UP(output_size);
+    for ( ; i < (1 << kernel_order_out); i++ )
+        free_domheap_page(pages + i);
+
+    /*
+     * Free the original kernel, update the pointers to the
+     * decompressed kernel
+     */
+    fw_unreserved_regions(addr, addr + size, init_domheap_pages, 0);
+
+    return 0;
+}
+
 /*
  * Uimage CPU Architecture Codes
  */
@@ -289,79 +362,6 @@ static int __init kernel_uimage_probe(struct kernel_info *info,
     return 0;
 }
 
-static __init uint32_t output_length(char *image, unsigned long image_len)
-{
-    return *(uint32_t *)&image[image_len - 4];
-}
-
-static __init int kernel_decompress(struct bootmodule *mod)
-{
-    char *output, *input;
-    char magic[2];
-    int rc;
-    unsigned int kernel_order_out;
-    paddr_t output_size;
-    struct page_info *pages;
-    mfn_t mfn;
-    int i;
-    paddr_t addr = mod->start;
-    paddr_t size = mod->size;
-
-    if ( size < 2 )
-        return -EINVAL;
-
-    copy_from_paddr(magic, addr, sizeof(magic));
-
-    /* only gzip is supported */
-    if ( !gzip_check(magic, size) )
-        return -EINVAL;
-
-    input = ioremap_cache(addr, size);
-    if ( input == NULL )
-        return -EFAULT;
-
-    output_size = output_length(input, size);
-    kernel_order_out = get_order_from_bytes(output_size);
-    pages = alloc_domheap_pages(NULL, kernel_order_out, 0);
-    if ( pages == NULL )
-    {
-        iounmap(input);
-        return -ENOMEM;
-    }
-    mfn = page_to_mfn(pages);
-    output = __vmap(&mfn, 1 << kernel_order_out, 1, 1, PAGE_HYPERVISOR, VMAP_DEFAULT);
-
-    rc = perform_gunzip(output, input, size);
-    clean_dcache_va_range(output, output_size);
-    iounmap(input);
-    vunmap(output);
-
-    if ( rc )
-    {
-        free_domheap_pages(pages, kernel_order_out);
-        return rc;
-    }
-
-    mod->start = page_to_maddr(pages);
-    mod->size = output_size;
-
-    /*
-     * Need to free pages after output_size here because they won't be
-     * freed by discard_initial_modules
-     */
-    i = PFN_UP(output_size);
-    for ( ; i < (1 << kernel_order_out); i++ )
-        free_domheap_page(pages + i);
-
-    /*
-     * Free the original kernel, update the pointers to the
-     * decompressed kernel
-     */
-    fw_unreserved_regions(addr, addr + size, init_domheap_pages, 0);
-
-    return 0;
-}
-
 #ifdef CONFIG_ARM_64
 /*
  * Check if the image is a 64-bit Image.
-- 
2.25.1

Re: [PATCH 1/2] xen/arm: Move kernel_uimage_probe definition after kernel_decompress

[Ayan Kumar Halder]

On 31/01/2023 15:13, Michal Orzel wrote:
> In a follow-up patch, we will be calling kernel_decompress function from
> within kernel_uimage_probe to support booting compressed images with
> u-boot header. Therefore, move the kernel_uimage_probe definition after
> kernel_decompress so that the latter is visible to the former.
>
> No functional change intended.
>
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
Reviewed-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
>   xen/arch/arm/kernel.c | 146 +++++++++++++++++++++---------------------
>   1 file changed, 73 insertions(+), 73 deletions(-)
>
> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> index 36081e73f140..068fbf88e492 100644
> --- a/xen/arch/arm/kernel.c
> +++ b/xen/arch/arm/kernel.c
> @@ -186,6 +186,79 @@ static void __init kernel_zimage_load(struct kernel_info *info)
>       iounmap(kernel);
>   }
>   
> +static __init uint32_t output_length(char *image, unsigned long image_len)
> +{
> +    return *(uint32_t *)&image[image_len - 4];
> +}
> +
> +static __init int kernel_decompress(struct bootmodule *mod)
> +{
> +    char *output, *input;
> +    char magic[2];
> +    int rc;
> +    unsigned int kernel_order_out;
> +    paddr_t output_size;
> +    struct page_info *pages;
> +    mfn_t mfn;
> +    int i;
> +    paddr_t addr = mod->start;
> +    paddr_t size = mod->size;
> +
> +    if ( size < 2 )
> +        return -EINVAL;
> +
> +    copy_from_paddr(magic, addr, sizeof(magic));
> +
> +    /* only gzip is supported */
> +    if ( !gzip_check(magic, size) )
> +        return -EINVAL;
> +
> +    input = ioremap_cache(addr, size);
> +    if ( input == NULL )
> +        return -EFAULT;
> +
> +    output_size = output_length(input, size);
> +    kernel_order_out = get_order_from_bytes(output_size);
> +    pages = alloc_domheap_pages(NULL, kernel_order_out, 0);
> +    if ( pages == NULL )
> +    {
> +        iounmap(input);
> +        return -ENOMEM;
> +    }
> +    mfn = page_to_mfn(pages);
> +    output = __vmap(&mfn, 1 << kernel_order_out, 1, 1, PAGE_HYPERVISOR, VMAP_DEFAULT);
> +
> +    rc = perform_gunzip(output, input, size);
> +    clean_dcache_va_range(output, output_size);
> +    iounmap(input);
> +    vunmap(output);
> +
> +    if ( rc )
> +    {
> +        free_domheap_pages(pages, kernel_order_out);
> +        return rc;
> +    }
> +
> +    mod->start = page_to_maddr(pages);
> +    mod->size = output_size;
> +
> +    /*
> +     * Need to free pages after output_size here because they won't be
> +     * freed by discard_initial_modules
> +     */
> +    i = PFN_UP(output_size);
> +    for ( ; i < (1 << kernel_order_out); i++ )
> +        free_domheap_page(pages + i);
> +
> +    /*
> +     * Free the original kernel, update the pointers to the
> +     * decompressed kernel
> +     */
> +    fw_unreserved_regions(addr, addr + size, init_domheap_pages, 0);
> +
> +    return 0;
> +}
> +
>   /*
>    * Uimage CPU Architecture Codes
>    */
> @@ -289,79 +362,6 @@ static int __init kernel_uimage_probe(struct kernel_info *info,
>       return 0;
>   }
>   
> -static __init uint32_t output_length(char *image, unsigned long image_len)
> -{
> -    return *(uint32_t *)&image[image_len - 4];
> -}
> -
> -static __init int kernel_decompress(struct bootmodule *mod)
> -{
> -    char *output, *input;
> -    char magic[2];
> -    int rc;
> -    unsigned int kernel_order_out;
> -    paddr_t output_size;
> -    struct page_info *pages;
> -    mfn_t mfn;
> -    int i;
> -    paddr_t addr = mod->start;
> -    paddr_t size = mod->size;
> -
> -    if ( size < 2 )
> -        return -EINVAL;
> -
> -    copy_from_paddr(magic, addr, sizeof(magic));
> -
> -    /* only gzip is supported */
> -    if ( !gzip_check(magic, size) )
> -        return -EINVAL;
> -
> -    input = ioremap_cache(addr, size);
> -    if ( input == NULL )
> -        return -EFAULT;
> -
> -    output_size = output_length(input, size);
> -    kernel_order_out = get_order_from_bytes(output_size);
> -    pages = alloc_domheap_pages(NULL, kernel_order_out, 0);
> -    if ( pages == NULL )
> -    {
> -        iounmap(input);
> -        return -ENOMEM;
> -    }
> -    mfn = page_to_mfn(pages);
> -    output = __vmap(&mfn, 1 << kernel_order_out, 1, 1, PAGE_HYPERVISOR, VMAP_DEFAULT);
> -
> -    rc = perform_gunzip(output, input, size);
> -    clean_dcache_va_range(output, output_size);
> -    iounmap(input);
> -    vunmap(output);
> -
> -    if ( rc )
> -    {
> -        free_domheap_pages(pages, kernel_order_out);
> -        return rc;
> -    }
> -
> -    mod->start = page_to_maddr(pages);
> -    mod->size = output_size;
> -
> -    /*
> -     * Need to free pages after output_size here because they won't be
> -     * freed by discard_initial_modules
> -     */
> -    i = PFN_UP(output_size);
> -    for ( ; i < (1 << kernel_order_out); i++ )
> -        free_domheap_page(pages + i);
> -
> -    /*
> -     * Free the original kernel, update the pointers to the
> -     * decompressed kernel
> -     */
> -    fw_unreserved_regions(addr, addr + size, init_domheap_pages, 0);
> -
> -    return 0;
> -}
> -
>   #ifdef CONFIG_ARM_64
>   /*
>    * Check if the image is a 64-bit Image.

Re: [PATCH 1/2] xen/arm: Move kernel_uimage_probe definition after kernel_decompress

[Julien Grall]

Hi Michal,

On 31/01/2023 15:13, Michal Orzel wrote:
> In a follow-up patch, we will be calling kernel_decompress function from
> within kernel_uimage_probe to support booting compressed images with
> u-boot header. Therefore, move the kernel_uimage_probe definition after
> kernel_decompress so that the latter is visible to the former.
> 
> No functional change intended.
> 
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

[PATCH 2/2] xen/arm: Add support for booting gzip compressed uImages

[Michal Orzel]

At the moment, Xen does not support booting gzip compressed uImages.
This is because we are trying to decompress the kernel before probing
the u-boot header. This leads to a failure as the header always appears
at the top of the image (and therefore obscuring the gzip header).

Move the call to kernel_uimage_probe before kernel_decompress and make
the function self-containing by taking the following actions:
 - take a pointer to struct bootmodule as a parameter,
 - check the comp field of a u-boot header to determine compression type,
 - in case of compressed image, modify boot module start address and size
   by taking the header size into account and call kernel_decompress,
 - set up zimage.{kernel_addr,len} accordingly,
 - return -ENOENT in case of a u-boot header not found to distinguish it
   amongst other return values and make it the only case for falling
   through to try to probe other image types.

This is done to avoid splitting the uImage probing into 2 stages (executed
before and after decompression) which otherwise would be necessary to
properly update boot module start and size before decompression and
zimage.{kernel_addr,len} afterwards.

Remove the limitation from the booting.txt documentation.

Signed-off-by: Michal Orzel <michal.orzel@amd.com>
---
 docs/misc/arm/booting.txt |  3 ---
 xen/arch/arm/kernel.c     | 51 ++++++++++++++++++++++++++++++++++-----
 2 files changed, 45 insertions(+), 9 deletions(-)

diff --git a/docs/misc/arm/booting.txt b/docs/misc/arm/booting.txt
index bd7bfe7f284a..02f7bb65ec6d 100644
--- a/docs/misc/arm/booting.txt
+++ b/docs/misc/arm/booting.txt
@@ -50,9 +50,6 @@ Also, it is to be noted that if user provides the legacy image header on
 top of zImage or Image header, then Xen uses the attributes of legacy
 image header to determine the load address, entry point, etc.
 
-Known limitation: compressed kernels with a uboot headers are not
-working.
-
 
 Firmware/bootloader requirements
 --------------------------------
diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index 068fbf88e492..ea5f9618169e 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -265,11 +265,14 @@ static __init int kernel_decompress(struct bootmodule *mod)
 #define IH_ARCH_ARM             2       /* ARM          */
 #define IH_ARCH_ARM64           22      /* ARM64        */
 
+/* uImage Compression Types */
+#define IH_COMP_GZIP            1
+
 /*
  * Check if the image is a uImage and setup kernel_info
  */
 static int __init kernel_uimage_probe(struct kernel_info *info,
-                                      paddr_t addr, paddr_t size)
+                                      struct bootmodule *mod)
 {
     struct {
         __be32 magic;   /* Image Header Magic Number */
@@ -287,6 +290,8 @@ static int __init kernel_uimage_probe(struct kernel_info *info,
     } uimage;
 
     uint32_t len;
+    paddr_t addr = mod->start;
+    paddr_t size = mod->size;
 
     if ( size < sizeof(uimage) )
         return -EINVAL;
@@ -294,13 +299,21 @@ static int __init kernel_uimage_probe(struct kernel_info *info,
     copy_from_paddr(&uimage, addr, sizeof(uimage));
 
     if ( be32_to_cpu(uimage.magic) != UIMAGE_MAGIC )
-        return -EINVAL;
+        return -ENOENT;
 
     len = be32_to_cpu(uimage.size);
 
     if ( len > size - sizeof(uimage) )
         return -EINVAL;
 
+    /* Only gzip compression is supported. */
+    if ( uimage.comp && uimage.comp != IH_COMP_GZIP )
+    {
+        printk(XENLOG_ERR
+               "Unsupported uImage compression type %"PRIu8"\n", uimage.comp);
+        return -EOPNOTSUPP;
+    }
+
     info->zimage.start = be32_to_cpu(uimage.load);
     info->entry = be32_to_cpu(uimage.ep);
 
@@ -330,8 +343,26 @@ static int __init kernel_uimage_probe(struct kernel_info *info,
         return -EINVAL;
     }
 
-    info->zimage.kernel_addr = addr + sizeof(uimage);
-    info->zimage.len = len;
+    if ( uimage.comp )
+    {
+        int rc;
+
+        /* Prepare start and size for decompression. */
+        mod->start += sizeof(uimage);
+        mod->size -= sizeof(uimage);
+
+        rc = kernel_decompress(mod);
+        if ( rc )
+            return rc;
+
+        info->zimage.kernel_addr = mod->start;
+        info->zimage.len = mod->size;
+    }
+    else
+    {
+        info->zimage.kernel_addr = addr + sizeof(uimage);
+        info->zimage.len = len;
+    }
 
     info->load = kernel_zimage_load;
 
@@ -561,6 +592,16 @@ int __init kernel_probe(struct kernel_info *info,
         printk("Loading ramdisk from boot module @ %"PRIpaddr"\n",
                info->initrd_bootmodule->start);
 
+    /*
+     * uImage header always appears at the top of the image (even compressed),
+     * so it needs to be probed first. Note that in case of compressed uImage,
+     * kernel_decompress is called from kernel_uimage_probe making the function
+     * self-containing (i.e. fall through only in case of a header not found).
+    */
+    rc = kernel_uimage_probe(info, mod);
+    if ( rc != -ENOENT )
+        return rc;
+
     /* if it is a gzip'ed image, 32bit or 64bit, uncompress it */
     rc = kernel_decompress(mod);
     if ( rc && rc != -EINVAL )
@@ -570,8 +611,6 @@ int __init kernel_probe(struct kernel_info *info,
     rc = kernel_zimage64_probe(info, mod->start, mod->size);
     if (rc < 0)
 #endif
-        rc = kernel_uimage_probe(info, mod->start, mod->size);
-    if (rc < 0)
         rc = kernel_zimage32_probe(info, mod->start, mod->size);
 
     return rc;
-- 
2.25.1

Re: [PATCH 2/2] xen/arm: Add support for booting gzip compressed uImages

[Ayan Kumar Halder]

On 31/01/2023 15:13, Michal Orzel wrote:
> At the moment, Xen does not support booting gzip compressed uImages.
> This is because we are trying to decompress the kernel before probing
> the u-boot header. This leads to a failure as the header always appears
> at the top of the image (and therefore obscuring the gzip header).
>
> Move the call to kernel_uimage_probe before kernel_decompress and make
> the function self-containing by taking the following actions:
>   - take a pointer to struct bootmodule as a parameter,
>   - check the comp field of a u-boot header to determine compression type,
>   - in case of compressed image, modify boot module start address and size
>     by taking the header size into account and call kernel_decompress,
>   - set up zimage.{kernel_addr,len} accordingly,
>   - return -ENOENT in case of a u-boot header not found to distinguish it
>     amongst other return values and make it the only case for falling
>     through to try to probe other image types.
>
> This is done to avoid splitting the uImage probing into 2 stages (executed
> before and after decompression) which otherwise would be necessary to
> properly update boot module start and size before decompression and
> zimage.{kernel_addr,len} afterwards.
>
> Remove the limitation from the booting.txt documentation.
>
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
LGTM, Reviewed-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
>   docs/misc/arm/booting.txt |  3 ---
>   xen/arch/arm/kernel.c     | 51 ++++++++++++++++++++++++++++++++++-----
>   2 files changed, 45 insertions(+), 9 deletions(-)
>
> diff --git a/docs/misc/arm/booting.txt b/docs/misc/arm/booting.txt
> index bd7bfe7f284a..02f7bb65ec6d 100644
> --- a/docs/misc/arm/booting.txt
> +++ b/docs/misc/arm/booting.txt
> @@ -50,9 +50,6 @@ Also, it is to be noted that if user provides the legacy image header on
>   top of zImage or Image header, then Xen uses the attributes of legacy
>   image header to determine the load address, entry point, etc.
>   
> -Known limitation: compressed kernels with a uboot headers are not
> -working.
> -
>   
>   Firmware/bootloader requirements
>   --------------------------------
> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> index 068fbf88e492..ea5f9618169e 100644
> --- a/xen/arch/arm/kernel.c
> +++ b/xen/arch/arm/kernel.c
> @@ -265,11 +265,14 @@ static __init int kernel_decompress(struct bootmodule *mod)
>   #define IH_ARCH_ARM             2       /* ARM          */
>   #define IH_ARCH_ARM64           22      /* ARM64        */
>   
> +/* uImage Compression Types */
> +#define IH_COMP_GZIP            1
> +
>   /*
>    * Check if the image is a uImage and setup kernel_info
>    */
>   static int __init kernel_uimage_probe(struct kernel_info *info,
> -                                      paddr_t addr, paddr_t size)
> +                                      struct bootmodule *mod)
>   {
>       struct {
>           __be32 magic;   /* Image Header Magic Number */
> @@ -287,6 +290,8 @@ static int __init kernel_uimage_probe(struct kernel_info *info,
>       } uimage;
>   
>       uint32_t len;
> +    paddr_t addr = mod->start;
> +    paddr_t size = mod->size;
>   
>       if ( size < sizeof(uimage) )
>           return -EINVAL;
> @@ -294,13 +299,21 @@ static int __init kernel_uimage_probe(struct kernel_info *info,
>       copy_from_paddr(&uimage, addr, sizeof(uimage));
>   
>       if ( be32_to_cpu(uimage.magic) != UIMAGE_MAGIC )
> -        return -EINVAL;
> +        return -ENOENT;
>   
>       len = be32_to_cpu(uimage.size);
>   
>       if ( len > size - sizeof(uimage) )
>           return -EINVAL;
>   
> +    /* Only gzip compression is supported. */
> +    if ( uimage.comp && uimage.comp != IH_COMP_GZIP )
> +    {
> +        printk(XENLOG_ERR
> +               "Unsupported uImage compression type %"PRIu8"\n", uimage.comp);
> +        return -EOPNOTSUPP;
> +    }
> +
>       info->zimage.start = be32_to_cpu(uimage.load);
>       info->entry = be32_to_cpu(uimage.ep);
>   
> @@ -330,8 +343,26 @@ static int __init kernel_uimage_probe(struct kernel_info *info,
>           return -EINVAL;
>       }
>   
> -    info->zimage.kernel_addr = addr + sizeof(uimage);
> -    info->zimage.len = len;
> +    if ( uimage.comp )
> +    {
> +        int rc;
> +
> +        /* Prepare start and size for decompression. */
> +        mod->start += sizeof(uimage);
> +        mod->size -= sizeof(uimage);
> +
> +        rc = kernel_decompress(mod);
> +        if ( rc )
> +            return rc;
> +
> +        info->zimage.kernel_addr = mod->start;
> +        info->zimage.len = mod->size;
> +    }
> +    else
> +    {
> +        info->zimage.kernel_addr = addr + sizeof(uimage);
> +        info->zimage.len = len;
> +    }
>   
>       info->load = kernel_zimage_load;
>   
> @@ -561,6 +592,16 @@ int __init kernel_probe(struct kernel_info *info,
>           printk("Loading ramdisk from boot module @ %"PRIpaddr"\n",
>                  info->initrd_bootmodule->start);
>   
> +    /*
> +     * uImage header always appears at the top of the image (even compressed),
> +     * so it needs to be probed first. Note that in case of compressed uImage,
> +     * kernel_decompress is called from kernel_uimage_probe making the function
> +     * self-containing (i.e. fall through only in case of a header not found).
> +    */
> +    rc = kernel_uimage_probe(info, mod);
> +    if ( rc != -ENOENT )
> +        return rc;
> +
>       /* if it is a gzip'ed image, 32bit or 64bit, uncompress it */
>       rc = kernel_decompress(mod);
I think the disadvantage of this approach is that kernel_decompress() 
now needs to be called from 2 different places. But, I think it is still 
preferable over splitting the

kernel_uimage_probe() function.

>       if ( rc && rc != -EINVAL )
> @@ -570,8 +611,6 @@ int __init kernel_probe(struct kernel_info *info,
>       rc = kernel_zimage64_probe(info, mod->start, mod->size);
>       if (rc < 0)
>   #endif
> -        rc = kernel_uimage_probe(info, mod->start, mod->size);
> -    if (rc < 0)
>           rc = kernel_zimage32_probe(info, mod->start, mod->size);
>   
>       return rc;
- Ayan

Re: [PATCH 2/2] xen/arm: Add support for booting gzip compressed uImages

[Julien Grall]

Hi,

On 31/01/2023 15:13, Michal Orzel wrote:
> At the moment, Xen does not support booting gzip compressed uImages.
> This is because we are trying to decompress the kernel before probing
> the u-boot header. This leads to a failure as the header always appears
> at the top of the image (and therefore obscuring the gzip header).
> 
> Move the call to kernel_uimage_probe before kernel_decompress and make
> the function self-containing by taking the following actions:
>   - take a pointer to struct bootmodule as a parameter,
>   - check the comp field of a u-boot header to determine compression type,
>   - in case of compressed image, modify boot module start address and size
>     by taking the header size into account and call kernel_decompress,
>   - set up zimage.{kernel_addr,len} accordingly,
>   - return -ENOENT in case of a u-boot header not found to distinguish it
>     amongst other return values and make it the only case for falling
>     through to try to probe other image types.
> 
> This is done to avoid splitting the uImage probing into 2 stages (executed
> before and after decompression) which otherwise would be necessary to
> properly update boot module start and size before decompression and
> zimage.{kernel_addr,len} afterwards.

AFAIU, it would be possible to have a zImage/Image header embedded in 
the uImage. So any reason to only handle a compressed binary?

> 
> Remove the limitation from the booting.txt documentation.
> 
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> ---
>   docs/misc/arm/booting.txt |  3 ---
>   xen/arch/arm/kernel.c     | 51 ++++++++++++++++++++++++++++++++++-----
>   2 files changed, 45 insertions(+), 9 deletions(-)
> 
> diff --git a/docs/misc/arm/booting.txt b/docs/misc/arm/booting.txt
> index bd7bfe7f284a..02f7bb65ec6d 100644
> --- a/docs/misc/arm/booting.txt
> +++ b/docs/misc/arm/booting.txt
> @@ -50,9 +50,6 @@ Also, it is to be noted that if user provides the legacy image header on
>   top of zImage or Image header, then Xen uses the attributes of legacy
>   image header to determine the load address, entry point, etc.
>   
> -Known limitation: compressed kernels with a uboot headers are not
> -working.
> -
>   
>   Firmware/bootloader requirements
>   --------------------------------
> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> index 068fbf88e492..ea5f9618169e 100644
> --- a/xen/arch/arm/kernel.c
> +++ b/xen/arch/arm/kernel.c
> @@ -265,11 +265,14 @@ static __init int kernel_decompress(struct bootmodule *mod)
>   #define IH_ARCH_ARM             2       /* ARM          */
>   #define IH_ARCH_ARM64           22      /* ARM64        */
>   
> +/* uImage Compression Types */
> +#define IH_COMP_GZIP            1
> +
>   /*
>    * Check if the image is a uImage and setup kernel_info
>    */
>   static int __init kernel_uimage_probe(struct kernel_info *info,
> -                                      paddr_t addr, paddr_t size)
> +                                      struct bootmodule *mod)
>   {
>       struct {
>           __be32 magic;   /* Image Header Magic Number */
> @@ -287,6 +290,8 @@ static int __init kernel_uimage_probe(struct kernel_info *info,
>       } uimage;
>   
>       uint32_t len;
> +    paddr_t addr = mod->start;
> +    paddr_t size = mod->size;
>   
>       if ( size < sizeof(uimage) )
>           return -EINVAL;
> @@ -294,13 +299,21 @@ static int __init kernel_uimage_probe(struct kernel_info *info,
>       copy_from_paddr(&uimage, addr, sizeof(uimage));
>   
>       if ( be32_to_cpu(uimage.magic) != UIMAGE_MAGIC )
> -        return -EINVAL;
> +        return -ENOENT;
>   
>       len = be32_to_cpu(uimage.size);
>   
>       if ( len > size - sizeof(uimage) )
>           return -EINVAL;
>   
> +    /* Only gzip compression is supported. */
> +    if ( uimage.comp && uimage.comp != IH_COMP_GZIP )
> +    {
> +        printk(XENLOG_ERR
> +               "Unsupported uImage compression type %"PRIu8"\n", uimage.comp);
> +        return -EOPNOTSUPP;
> +    }
> +
>       info->zimage.start = be32_to_cpu(uimage.load);
>       info->entry = be32_to_cpu(uimage.ep);
>   
> @@ -330,8 +343,26 @@ static int __init kernel_uimage_probe(struct kernel_info *info,
>           return -EINVAL;
>       }
>   
> -    info->zimage.kernel_addr = addr + sizeof(uimage);
> -    info->zimage.len = len;
> +    if ( uimage.comp )
> +    {
> +        int rc;
> +
> +        /* Prepare start and size for decompression. */
> +        mod->start += sizeof(uimage);
> +        mod->size -= sizeof(uimage);

kernel_decompress() will free the compressed module once it is 
decompressed. By updating the region it means the free page will be not 
be freed (assuming start was previously page-aligned).

> +
> +        rc = kernel_decompress(mod);
> +        if ( rc )
> +            return rc;
> +
> +        info->zimage.kernel_addr = mod->start;
> +        info->zimage.len = mod->size;
> +    }
> +    else
> +    {
> +        info->zimage.kernel_addr = addr + sizeof(uimage);
> +        info->zimage.len = len;
> +    }
>   
>       info->load = kernel_zimage_load;
>   
> @@ -561,6 +592,16 @@ int __init kernel_probe(struct kernel_info *info,
>           printk("Loading ramdisk from boot module @ %"PRIpaddr"\n",
>                  info->initrd_bootmodule->start);
>   
> +    /*
> +     * uImage header always appears at the top of the image (even compressed),
> +     * so it needs to be probed first. Note that in case of compressed uImage,
> +     * kernel_decompress is called from kernel_uimage_probe making the function
> +     * self-containing (i.e. fall through only in case of a header not found).
> +    */
> +    rc = kernel_uimage_probe(info, mod);
> +    if ( rc != -ENOENT )
> +        return rc;
> +
>       /* if it is a gzip'ed image, 32bit or 64bit, uncompress it */
>       rc = kernel_decompress(mod);
>       if ( rc && rc != -EINVAL )
> @@ -570,8 +611,6 @@ int __init kernel_probe(struct kernel_info *info,
>       rc = kernel_zimage64_probe(info, mod->start, mod->size);
>       if (rc < 0)
>   #endif
> -        rc = kernel_uimage_probe(info, mod->start, mod->size);
> -    if (rc < 0)
>           rc = kernel_zimage32_probe(info, mod->start, mod->size);
>   
>       return rc;
Cheers,

-- 
Julien Grall

Re: [PATCH 2/2] xen/arm: Add support for booting gzip compressed uImages

[Michal Orzel]

Hi Julien,

On 31/01/2023 21:20, Julien Grall wrote:
> 
> 
> Hi,
> 
> On 31/01/2023 15:13, Michal Orzel wrote:
>> At the moment, Xen does not support booting gzip compressed uImages.
>> This is because we are trying to decompress the kernel before probing
>> the u-boot header. This leads to a failure as the header always appears
>> at the top of the image (and therefore obscuring the gzip header).
>>
>> Move the call to kernel_uimage_probe before kernel_decompress and make
>> the function self-containing by taking the following actions:
>>   - take a pointer to struct bootmodule as a parameter,
>>   - check the comp field of a u-boot header to determine compression type,
>>   - in case of compressed image, modify boot module start address and size
>>     by taking the header size into account and call kernel_decompress,
>>   - set up zimage.{kernel_addr,len} accordingly,
>>   - return -ENOENT in case of a u-boot header not found to distinguish it
>>     amongst other return values and make it the only case for falling
>>     through to try to probe other image types.
>>
>> This is done to avoid splitting the uImage probing into 2 stages (executed
>> before and after decompression) which otherwise would be necessary to
>> properly update boot module start and size before decompression and
>> zimage.{kernel_addr,len} afterwards.
> 
> AFAIU, it would be possible to have a zImage/Image header embedded in
> the uImage. So any reason to only handle a compressed binary?
Not sure if I understand you correctly as what you say is already supported.
The split or moving decompression is only needed in case of compressed uImage,
as u-boot header (not being part of compression) appears before gzip header.
This is not the case for zImage/Image header that is embedded into image
and gzip header is at the top.

In case of uImage added on top of zImage/Image, the load address/entry point are taken
from uImage header so basically the zImage/Image header is not parsed (this is
documented in our booting.txt).

This patch makes the uImage compression works as the other combinations work fine already.
You can boot what you can already:
- zImage/Image
- compressed zImage/Image
- zImage/Image,raw with u-boot header
+ this patch allows to boot:
- compressed uImage (i.e. zImage/Image/raw compressed with u-boot header)

> 
>>
>> Remove the limitation from the booting.txt documentation.
>>
>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>> ---
>>   docs/misc/arm/booting.txt |  3 ---
>>   xen/arch/arm/kernel.c     | 51 ++++++++++++++++++++++++++++++++++-----
>>   2 files changed, 45 insertions(+), 9 deletions(-)
>>
>> diff --git a/docs/misc/arm/booting.txt b/docs/misc/arm/booting.txt
>> index bd7bfe7f284a..02f7bb65ec6d 100644
>> --- a/docs/misc/arm/booting.txt
>> +++ b/docs/misc/arm/booting.txt
>> @@ -50,9 +50,6 @@ Also, it is to be noted that if user provides the legacy image header on
>>   top of zImage or Image header, then Xen uses the attributes of legacy
>>   image header to determine the load address, entry point, etc.
>>
>> -Known limitation: compressed kernels with a uboot headers are not
>> -working.
>> -
>>
>>   Firmware/bootloader requirements
>>   --------------------------------
>> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
>> index 068fbf88e492..ea5f9618169e 100644
>> --- a/xen/arch/arm/kernel.c
>> +++ b/xen/arch/arm/kernel.c
>> @@ -265,11 +265,14 @@ static __init int kernel_decompress(struct bootmodule *mod)
>>   #define IH_ARCH_ARM             2       /* ARM          */
>>   #define IH_ARCH_ARM64           22      /* ARM64        */
>>
>> +/* uImage Compression Types */
>> +#define IH_COMP_GZIP            1
>> +
>>   /*
>>    * Check if the image is a uImage and setup kernel_info
>>    */
>>   static int __init kernel_uimage_probe(struct kernel_info *info,
>> -                                      paddr_t addr, paddr_t size)
>> +                                      struct bootmodule *mod)
>>   {
>>       struct {
>>           __be32 magic;   /* Image Header Magic Number */
>> @@ -287,6 +290,8 @@ static int __init kernel_uimage_probe(struct kernel_info *info,
>>       } uimage;
>>
>>       uint32_t len;
>> +    paddr_t addr = mod->start;
>> +    paddr_t size = mod->size;
>>
>>       if ( size < sizeof(uimage) )
>>           return -EINVAL;
>> @@ -294,13 +299,21 @@ static int __init kernel_uimage_probe(struct kernel_info *info,
>>       copy_from_paddr(&uimage, addr, sizeof(uimage));
>>
>>       if ( be32_to_cpu(uimage.magic) != UIMAGE_MAGIC )
>> -        return -EINVAL;
>> +        return -ENOENT;
>>
>>       len = be32_to_cpu(uimage.size);
>>
>>       if ( len > size - sizeof(uimage) )
>>           return -EINVAL;
>>
>> +    /* Only gzip compression is supported. */
>> +    if ( uimage.comp && uimage.comp != IH_COMP_GZIP )
>> +    {
>> +        printk(XENLOG_ERR
>> +               "Unsupported uImage compression type %"PRIu8"\n", uimage.comp);
>> +        return -EOPNOTSUPP;
>> +    }
>> +
>>       info->zimage.start = be32_to_cpu(uimage.load);
>>       info->entry = be32_to_cpu(uimage.ep);
>>
>> @@ -330,8 +343,26 @@ static int __init kernel_uimage_probe(struct kernel_info *info,
>>           return -EINVAL;
>>       }
>>
>> -    info->zimage.kernel_addr = addr + sizeof(uimage);
>> -    info->zimage.len = len;
>> +    if ( uimage.comp )
>> +    {
>> +        int rc;
>> +
>> +        /* Prepare start and size for decompression. */
>> +        mod->start += sizeof(uimage);
>> +        mod->size -= sizeof(uimage);
> 
> kernel_decompress() will free the compressed module once it is
> decompressed. By updating the region it means the free page will be not
> be freed (assuming start was previously page-aligned).
Ok, so the start address was previously page-aligned and by adding the uimage size
to it, it is no longer aligned. True. Do I understand you correctly that you refer
to the fw_unreserved_regions call from kernel_decompress where we will pass unaligned
address? This could be solved by doing (not harmful in my opinion for common case)
addr &= PAGE_MASK.

> 
>> +
>> +        rc = kernel_decompress(mod);
>> +        if ( rc )
>> +            return rc;
>> +
>> +        info->zimage.kernel_addr = mod->start;
>> +        info->zimage.len = mod->size;
>> +    }
>> +    else
>> +    {
>> +        info->zimage.kernel_addr = addr + sizeof(uimage);
>> +        info->zimage.len = len;
>> +    }
>>
>>       info->load = kernel_zimage_load;
>>
>> @@ -561,6 +592,16 @@ int __init kernel_probe(struct kernel_info *info,
>>           printk("Loading ramdisk from boot module @ %"PRIpaddr"\n",
>>                  info->initrd_bootmodule->start);
>>
>> +    /*
>> +     * uImage header always appears at the top of the image (even compressed),
>> +     * so it needs to be probed first. Note that in case of compressed uImage,
>> +     * kernel_decompress is called from kernel_uimage_probe making the function
>> +     * self-containing (i.e. fall through only in case of a header not found).
>> +    */
>> +    rc = kernel_uimage_probe(info, mod);
>> +    if ( rc != -ENOENT )
>> +        return rc;
>> +
>>       /* if it is a gzip'ed image, 32bit or 64bit, uncompress it */
>>       rc = kernel_decompress(mod);
>>       if ( rc && rc != -EINVAL )
>> @@ -570,8 +611,6 @@ int __init kernel_probe(struct kernel_info *info,
>>       rc = kernel_zimage64_probe(info, mod->start, mod->size);
>>       if (rc < 0)
>>   #endif
>> -        rc = kernel_uimage_probe(info, mod->start, mod->size);
>> -    if (rc < 0)
>>           rc = kernel_zimage32_probe(info, mod->start, mod->size);
>>
>>       return rc;
> Cheers,
> 
> --
> Julien Grall

~Michal

Re: [PATCH 2/2] xen/arm: Add support for booting gzip compressed uImages

[Julien Grall]

On 01/02/2023 09:48, Michal Orzel wrote:
> Hi Julien,

Hi Michal,

> 
> On 31/01/2023 21:20, Julien Grall wrote:
>>
>>
>> Hi,
>>
>> On 31/01/2023 15:13, Michal Orzel wrote:
>>> At the moment, Xen does not support booting gzip compressed uImages.
>>> This is because we are trying to decompress the kernel before probing
>>> the u-boot header. This leads to a failure as the header always appears
>>> at the top of the image (and therefore obscuring the gzip header).
>>>
>>> Move the call to kernel_uimage_probe before kernel_decompress and make
>>> the function self-containing by taking the following actions:
>>>    - take a pointer to struct bootmodule as a parameter,
>>>    - check the comp field of a u-boot header to determine compression type,
>>>    - in case of compressed image, modify boot module start address and size
>>>      by taking the header size into account and call kernel_decompress,
>>>    - set up zimage.{kernel_addr,len} accordingly,
>>>    - return -ENOENT in case of a u-boot header not found to distinguish it
>>>      amongst other return values and make it the only case for falling
>>>      through to try to probe other image types.
>>>
>>> This is done to avoid splitting the uImage probing into 2 stages (executed
>>> before and after decompression) which otherwise would be necessary to
>>> properly update boot module start and size before decompression and
>>> zimage.{kernel_addr,len} afterwards.
>>
>> AFAIU, it would be possible to have a zImage/Image header embedded in
>> the uImage. So any reason to only handle a compressed binary?
> Not sure if I understand you correctly as what you say is already supported.
> The split or moving decompression is only needed in case of compressed uImage,
> as u-boot header (not being part of compression) appears before gzip header.
> This is not the case for zImage/Image header that is embedded into image
> and gzip header is at the top.

[...]

> 
> In case of uImage added on top of zImage/Image, the load address/entry point are taken
> from uImage header so basically the zImage/Image header is not parsed (this is
> documented in our booting.txt).

This is the case I am talking about. I think we need to parrse 
zImage/Image because it may contain additional information about the 
placement (for instance Image has a field to indicate the real size in 
memory).

> 
> This patch makes the uImage compression works as the other combinations work fine already.
> You can boot what you can already:
> - zImage/Image
> - compressed zImage/Image
> - zImage/Image,raw with u-boot header
> + this patch allows to boot:
> - compressed uImage (i.e. zImage/Image/raw compressed with u-boot header)
> 
>>
>>>
>>> Remove the limitation from the booting.txt documentation.
>>>
>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>>> ---
>>>    docs/misc/arm/booting.txt |  3 ---
>>>    xen/arch/arm/kernel.c     | 51 ++++++++++++++++++++++++++++++++++-----
>>>    2 files changed, 45 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/docs/misc/arm/booting.txt b/docs/misc/arm/booting.txt
>>> index bd7bfe7f284a..02f7bb65ec6d 100644
>>> --- a/docs/misc/arm/booting.txt
>>> +++ b/docs/misc/arm/booting.txt
>>> @@ -50,9 +50,6 @@ Also, it is to be noted that if user provides the legacy image header on
>>>    top of zImage or Image header, then Xen uses the attributes of legacy
>>>    image header to determine the load address, entry point, etc.
>>>
>>> -Known limitation: compressed kernels with a uboot headers are not
>>> -working.
>>> -
>>>
>>>    Firmware/bootloader requirements
>>>    --------------------------------
>>> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
>>> index 068fbf88e492..ea5f9618169e 100644
>>> --- a/xen/arch/arm/kernel.c
>>> +++ b/xen/arch/arm/kernel.c
>>> @@ -265,11 +265,14 @@ static __init int kernel_decompress(struct bootmodule *mod)
>>>    #define IH_ARCH_ARM             2       /* ARM          */
>>>    #define IH_ARCH_ARM64           22      /* ARM64        */
>>>
>>> +/* uImage Compression Types */
>>> +#define IH_COMP_GZIP            1
>>> +
>>>    /*
>>>     * Check if the image is a uImage and setup kernel_info
>>>     */
>>>    static int __init kernel_uimage_probe(struct kernel_info *info,
>>> -                                      paddr_t addr, paddr_t size)
>>> +                                      struct bootmodule *mod)
>>>    {
>>>        struct {
>>>            __be32 magic;   /* Image Header Magic Number */
>>> @@ -287,6 +290,8 @@ static int __init kernel_uimage_probe(struct kernel_info *info,
>>>        } uimage;
>>>
>>>        uint32_t len;
>>> +    paddr_t addr = mod->start;
>>> +    paddr_t size = mod->size;
>>>
>>>        if ( size < sizeof(uimage) )
>>>            return -EINVAL;
>>> @@ -294,13 +299,21 @@ static int __init kernel_uimage_probe(struct kernel_info *info,
>>>        copy_from_paddr(&uimage, addr, sizeof(uimage));
>>>
>>>        if ( be32_to_cpu(uimage.magic) != UIMAGE_MAGIC )
>>> -        return -EINVAL;
>>> +        return -ENOENT;
>>>
>>>        len = be32_to_cpu(uimage.size);
>>>
>>>        if ( len > size - sizeof(uimage) )
>>>            return -EINVAL;
>>>
>>> +    /* Only gzip compression is supported. */
>>> +    if ( uimage.comp && uimage.comp != IH_COMP_GZIP )
>>> +    {
>>> +        printk(XENLOG_ERR
>>> +               "Unsupported uImage compression type %"PRIu8"\n", uimage.comp);
>>> +        return -EOPNOTSUPP;
>>> +    }
>>> +
>>>        info->zimage.start = be32_to_cpu(uimage.load);
>>>        info->entry = be32_to_cpu(uimage.ep);
>>>
>>> @@ -330,8 +343,26 @@ static int __init kernel_uimage_probe(struct kernel_info *info,
>>>            return -EINVAL;
>>>        }
>>>
>>> -    info->zimage.kernel_addr = addr + sizeof(uimage);
>>> -    info->zimage.len = len;
>>> +    if ( uimage.comp )
>>> +    {
>>> +        int rc;
>>> +
>>> +        /* Prepare start and size for decompression. */
>>> +        mod->start += sizeof(uimage);
>>> +        mod->size -= sizeof(uimage);
>>
>> kernel_decompress() will free the compressed module once it is
>> decompressed. By updating the region it means the free page will be not
>> be freed (assuming start was previously page-aligned).
> Ok, so the start address was previously page-aligned and by adding the uimage size
> to it, it is no longer aligned. True. Do I understand you correctly that you refer
> to the fw_unreserved_regions call from kernel_decompress where we will pass unaligned
> address? 

Correct.

> This could be solved by doing (not harmful in my opinion for common case)
> addr &= PAGE_MASK.
I don't quite understand your argument here. We need a check that work 
for everyone (not only in the common case).

Cheers,

-- 
Julien Grall

Re: [PATCH 2/2] xen/arm: Add support for booting gzip compressed uImages

[Michal Orzel]

Hi Julien,

On 01/02/2023 11:13, Julien Grall wrote:
> 
> 
> On 01/02/2023 09:48, Michal Orzel wrote:
>> Hi Julien,
> 
> Hi Michal,
> 
>>
>> On 31/01/2023 21:20, Julien Grall wrote:
>>>
>>>
>>> Hi,
>>>
>>> On 31/01/2023 15:13, Michal Orzel wrote:
>>>> At the moment, Xen does not support booting gzip compressed uImages.
>>>> This is because we are trying to decompress the kernel before probing
>>>> the u-boot header. This leads to a failure as the header always appears
>>>> at the top of the image (and therefore obscuring the gzip header).
>>>>
>>>> Move the call to kernel_uimage_probe before kernel_decompress and make
>>>> the function self-containing by taking the following actions:
>>>>    - take a pointer to struct bootmodule as a parameter,
>>>>    - check the comp field of a u-boot header to determine compression type,
>>>>    - in case of compressed image, modify boot module start address and size
>>>>      by taking the header size into account and call kernel_decompress,
>>>>    - set up zimage.{kernel_addr,len} accordingly,
>>>>    - return -ENOENT in case of a u-boot header not found to distinguish it
>>>>      amongst other return values and make it the only case for falling
>>>>      through to try to probe other image types.
>>>>
>>>> This is done to avoid splitting the uImage probing into 2 stages (executed
>>>> before and after decompression) which otherwise would be necessary to
>>>> properly update boot module start and size before decompression and
>>>> zimage.{kernel_addr,len} afterwards.
>>>
>>> AFAIU, it would be possible to have a zImage/Image header embedded in
>>> the uImage. So any reason to only handle a compressed binary?
>> Not sure if I understand you correctly as what you say is already supported.
>> The split or moving decompression is only needed in case of compressed uImage,
>> as u-boot header (not being part of compression) appears before gzip header.
>> This is not the case for zImage/Image header that is embedded into image
>> and gzip header is at the top.
> 
> [...]
> 
>>
>> In case of uImage added on top of zImage/Image, the load address/entry point are taken
>> from uImage header so basically the zImage/Image header is not parsed (this is
>> documented in our booting.txt).
> 
> This is the case I am talking about. I think we need to parrse
> zImage/Image because it may contain additional information about the
> placement (for instance Image has a field to indicate the real size in
> memory).
As it was said during discussion on Ayan's patch, adding u-boot header on top of zImage/Image
is not a common behavior and the purpose is questionable. Also I've never heard of any SW
that would parse both headers. After all that is why u-boot has booti (Image), bootz (zImage)
and bootm (uImage) commands. When using bootm to load Image/zImage with u-boot header, u-boot
does not read the Image/zImage header but only u-boot header (this is what Xen does at the moment
and was writeen by Ayan in documentation). If we really decide to do such a step (quite innovative :))
I would prefer not to do this in this series as the goals are different. This series is to remove
the known Xen limitation.

> 
>>
>> This patch makes the uImage compression works as the other combinations work fine already.
>> You can boot what you can already:
>> - zImage/Image
>> - compressed zImage/Image
>> - zImage/Image,raw with u-boot header
>> + this patch allows to boot:
>> - compressed uImage (i.e. zImage/Image/raw compressed with u-boot header)
>>
>>>
>>>>
>>>> Remove the limitation from the booting.txt documentation.
>>>>
>>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>>>> ---
>>>>    docs/misc/arm/booting.txt |  3 ---
>>>>    xen/arch/arm/kernel.c     | 51 ++++++++++++++++++++++++++++++++++-----
>>>>    2 files changed, 45 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/docs/misc/arm/booting.txt b/docs/misc/arm/booting.txt
>>>> index bd7bfe7f284a..02f7bb65ec6d 100644
>>>> --- a/docs/misc/arm/booting.txt
>>>> +++ b/docs/misc/arm/booting.txt
>>>> @@ -50,9 +50,6 @@ Also, it is to be noted that if user provides the legacy image header on
>>>>    top of zImage or Image header, then Xen uses the attributes of legacy
>>>>    image header to determine the load address, entry point, etc.
>>>>
>>>> -Known limitation: compressed kernels with a uboot headers are not
>>>> -working.
>>>> -
>>>>
>>>>    Firmware/bootloader requirements
>>>>    --------------------------------
>>>> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
>>>> index 068fbf88e492..ea5f9618169e 100644
>>>> --- a/xen/arch/arm/kernel.c
>>>> +++ b/xen/arch/arm/kernel.c
>>>> @@ -265,11 +265,14 @@ static __init int kernel_decompress(struct bootmodule *mod)
>>>>    #define IH_ARCH_ARM             2       /* ARM          */
>>>>    #define IH_ARCH_ARM64           22      /* ARM64        */
>>>>
>>>> +/* uImage Compression Types */
>>>> +#define IH_COMP_GZIP            1
>>>> +
>>>>    /*
>>>>     * Check if the image is a uImage and setup kernel_info
>>>>     */
>>>>    static int __init kernel_uimage_probe(struct kernel_info *info,
>>>> -                                      paddr_t addr, paddr_t size)
>>>> +                                      struct bootmodule *mod)
>>>>    {
>>>>        struct {
>>>>            __be32 magic;   /* Image Header Magic Number */
>>>> @@ -287,6 +290,8 @@ static int __init kernel_uimage_probe(struct kernel_info *info,
>>>>        } uimage;
>>>>
>>>>        uint32_t len;
>>>> +    paddr_t addr = mod->start;
>>>> +    paddr_t size = mod->size;
>>>>
>>>>        if ( size < sizeof(uimage) )
>>>>            return -EINVAL;
>>>> @@ -294,13 +299,21 @@ static int __init kernel_uimage_probe(struct kernel_info *info,
>>>>        copy_from_paddr(&uimage, addr, sizeof(uimage));
>>>>
>>>>        if ( be32_to_cpu(uimage.magic) != UIMAGE_MAGIC )
>>>> -        return -EINVAL;
>>>> +        return -ENOENT;
>>>>
>>>>        len = be32_to_cpu(uimage.size);
>>>>
>>>>        if ( len > size - sizeof(uimage) )
>>>>            return -EINVAL;
>>>>
>>>> +    /* Only gzip compression is supported. */
>>>> +    if ( uimage.comp && uimage.comp != IH_COMP_GZIP )
>>>> +    {
>>>> +        printk(XENLOG_ERR
>>>> +               "Unsupported uImage compression type %"PRIu8"\n", uimage.comp);
>>>> +        return -EOPNOTSUPP;
>>>> +    }
>>>> +
>>>>        info->zimage.start = be32_to_cpu(uimage.load);
>>>>        info->entry = be32_to_cpu(uimage.ep);
>>>>
>>>> @@ -330,8 +343,26 @@ static int __init kernel_uimage_probe(struct kernel_info *info,
>>>>            return -EINVAL;
>>>>        }
>>>>
>>>> -    info->zimage.kernel_addr = addr + sizeof(uimage);
>>>> -    info->zimage.len = len;
>>>> +    if ( uimage.comp )
>>>> +    {
>>>> +        int rc;
>>>> +
>>>> +        /* Prepare start and size for decompression. */
>>>> +        mod->start += sizeof(uimage);
>>>> +        mod->size -= sizeof(uimage);
>>>
>>> kernel_decompress() will free the compressed module once it is
>>> decompressed. By updating the region it means the free page will be not
>>> be freed (assuming start was previously page-aligned).
>> Ok, so the start address was previously page-aligned and by adding the uimage size
>> to it, it is no longer aligned. True. Do I understand you correctly that you refer
>> to the fw_unreserved_regions call from kernel_decompress where we will pass unaligned
>> address?
> 
> Correct.
> 
>> This could be solved by doing (not harmful in my opinion for common case)
>> addr &= PAGE_MASK.
> I don't quite understand your argument here. We need a check that work
> for everyone (not only in the common case).
As we assume the kernel module is at page aligned address (otherwise the issue you observed
can happen not only in uImage compressed case), having a check like
this is generic. I.e. for normal usecase (no uImage compressed), addr &= PAGE_MASK
does not modify the address. For uImage compressed usecase it causes the addr to get
back to the previous value (before we added header size to it).

Apart from the addr, we need to pass the correct size (as we extracted header size from it).
We could have the following (with appropriate comment):
size += addr - (addr & PAGE_MASK);
addr &= PAGE_MASK;
fw_unreserved_regions(addr, addr + size, init_domheap_pages, 0);

It does not look great but solves the uImage issue and does not modify
the previous behavior. Anyway, I'm open for suggestions.

> 
> Cheers,
> 
> --
> Julien Grall

~Michal

Re: [PATCH 2/2] xen/arm: Add support for booting gzip compressed uImages

[Julien Grall]

On 01/02/2023 11:01, Michal Orzel wrote:
> I would prefer not to do this in this series as the goals are different. This series is to remove
> the known Xen limitation.

The reason I am asking is it effectively change the way you would 
implement. If we were going to support zImage/Image within uImage, then 
we would need to fallthrough rather than calling kernel_decompress().

I am not aware of any at the moment. Better asking now than realizing 
after the fact that there is a need...

>>> This could be solved by doing (not harmful in my opinion for common case)
>>> addr &= PAGE_MASK.
>> I don't quite understand your argument here. We need a check that work
>> for everyone (not only in the common case).
> As we assume the kernel module is at page aligned address (otherwise the issue you observed
> can happen not only in uImage compressed case)

I am not aware of such assumption. It is allowed to use non page-aligned 
address and it is correct for Xen to not free the first part if it is 
not page aligned because the first part may contain data from another 
module (or else).

> , having a check like
> this is generic. I.e. for normal usecase (no uImage compressed), addr &= PAGE_MASK
> does not modify the address. For uImage compressed usecase it causes the addr to get
> back to the previous value (before we added header size to it).
> 
> Apart from the addr, we need to pass the correct size (as we extracted header size from it).
> We could have the following (with appropriate comment):
> size += addr - (addr & PAGE_MASK);
> addr &= PAGE_MASK;
> fw_unreserved_regions(addr, addr + size, init_domheap_pages, 0);
> 
> It does not look great but solves the uImage issue and does not modify
> the previous behavior. Anyway, I'm open for suggestions.

Two options:
  1) Move the call to fw_unreserved_regions() outside of 
kernel_decompress().
  2) Pass the offset of the gzip header to kernel_decompress(). 
Something like where it would be  sizeof(uimage) in the uImage case or 0 
otherwise.

I have a slight preference for the latter.

Cheers,

-- 
Julien Grall

Re: [PATCH 2/2] xen/arm: Add support for booting gzip compressed uImages

[Michal Orzel]

Hi Julien,

On 01/02/2023 12:20, Julien Grall wrote:
> 
> 
> On 01/02/2023 11:01, Michal Orzel wrote:
>> I would prefer not to do this in this series as the goals are different. This series is to remove
>> the known Xen limitation.
> 
> The reason I am asking is it effectively change the way you would
> implement. If we were going to support zImage/Image within uImage, then
> we would need to fallthrough rather than calling kernel_decompress().
> 
> I am not aware of any at the moment. Better asking now than realizing
> after the fact that there is a need...
We need uImage support as there is more and more need to support booting
raw images of some RTOSes (there is always additional concern \wrt booting requirements
but at least the header allows to try to boot them). We are not aware of any need
to do some special handling to parse more than one header + from what I said earlier,
this is an unusual approach not seen to be handled by anyone.

> 
>>>> This could be solved by doing (not harmful in my opinion for common case)
>>>> addr &= PAGE_MASK.
>>> I don't quite understand your argument here. We need a check that work
>>> for everyone (not only in the common case).
>> As we assume the kernel module is at page aligned address (otherwise the issue you observed
>> can happen not only in uImage compressed case)
> 
> I am not aware of such assumption. It is allowed to use non page-aligned
> address and it is correct for Xen to not free the first part if it is
> not page aligned because the first part may contain data from another
> module (or else).
> 
>> , having a check like
>> this is generic. I.e. for normal usecase (no uImage compressed), addr &= PAGE_MASK
>> does not modify the address. For uImage compressed usecase it causes the addr to get
>> back to the previous value (before we added header size to it).
>>
>> Apart from the addr, we need to pass the correct size (as we extracted header size from it).
>> We could have the following (with appropriate comment):
>> size += addr - (addr & PAGE_MASK);
>> addr &= PAGE_MASK;
>> fw_unreserved_regions(addr, addr + size, init_domheap_pages, 0);
>>
>> It does not look great but solves the uImage issue and does not modify
>> the previous behavior. Anyway, I'm open for suggestions.
> 
> Two options:
>   1) Move the call to fw_unreserved_regions() outside of
> kernel_decompress().
>   2) Pass the offset of the gzip header to kernel_decompress().
> Something like where it would be  sizeof(uimage) in the uImage case or 0
> otherwise.
> 
> I have a slight preference for the latter.
That is cool.
I'm in favor of this as well because it would allow not to set mod->start,size
from kernel_uimage_probe. Everything can be done in kernel_decompress:

Diff:

-static __init int kernel_decompress(struct bootmodule *mod)
+static __init int kernel_decompress(struct bootmodule *mod, uint32_t offset)
 {
     char *output, *input;
     char magic[2];
@@ -201,8 +201,14 @@ static __init int kernel_decompress(struct bootmodule *mod)
     struct page_info *pages;
     mfn_t mfn;
     int i;
-    paddr_t addr = mod->start;
-    paddr_t size = mod->size;
+
+    /*
+     * It might be that gzip header does not appear at the start address
+     * (i.e. in case of compressed uImage) so take into account offset to
+     * gzip header.
+    */
+    paddr_t addr = mod->start + offset;
+    paddr_t size = mod->size - offset;
 
     if ( size < 2 )
         return -EINVAL;
@@ -250,6 +256,13 @@ static __init int kernel_decompress(struct bootmodule *mod)
     for ( ; i < (1 << kernel_order_out); i++ )
         free_domheap_page(pages + i);
 
+    /*
+     * When freeing the kernel, we need to pass the module start address and
+     * size as they were before taking an offset to gzip header into account.
+     */
+    addr -= offset;
+    size += offset;
+
     /*
      * Free the original kernel, update the pointers to the
      * decompressed kernel

> 
> Cheers,
> 
> --
> Julien Grall

~Michal

Re: [PATCH 2/2] xen/arm: Add support for booting gzip compressed uImages

[Julien Grall]

On 01/02/2023 12:56, Michal Orzel wrote:
> Hi Julien,

Hi Michal,

> On 01/02/2023 12:20, Julien Grall wrote:
>>
>>
>> On 01/02/2023 11:01, Michal Orzel wrote:
>>> I would prefer not to do this in this series as the goals are different. This series is to remove
>>> the known Xen limitation.
>>
>> The reason I am asking is it effectively change the way you would
>> implement. If we were going to support zImage/Image within uImage, then
>> we would need to fallthrough rather than calling kernel_decompress().
>>
>> I am not aware of any at the moment. Better asking now than realizing
>> after the fact that there is a need...
> We need uImage support as there is more and more need to support booting
> raw images of some RTOSes (there is always additional concern \wrt booting requirements
> but at least the header allows to try to boot them). We are not aware of any need
> to do some special handling to parse more than one header + from what I said earlier,
> this is an unusual approach not seen to be handled by anyone.
> 
>>
>>>>> This could be solved by doing (not harmful in my opinion for common case)
>>>>> addr &= PAGE_MASK.
>>>> I don't quite understand your argument here. We need a check that work
>>>> for everyone (not only in the common case).
>>> As we assume the kernel module is at page aligned address (otherwise the issue you observed
>>> can happen not only in uImage compressed case)
>>
>> I am not aware of such assumption. It is allowed to use non page-aligned
>> address and it is correct for Xen to not free the first part if it is
>> not page aligned because the first part may contain data from another
>> module (or else).
>>
>>> , having a check like
>>> this is generic. I.e. for normal usecase (no uImage compressed), addr &= PAGE_MASK
>>> does not modify the address. For uImage compressed usecase it causes the addr to get
>>> back to the previous value (before we added header size to it).
>>>
>>> Apart from the addr, we need to pass the correct size (as we extracted header size from it).
>>> We could have the following (with appropriate comment):
>>> size += addr - (addr & PAGE_MASK);
>>> addr &= PAGE_MASK;
>>> fw_unreserved_regions(addr, addr + size, init_domheap_pages, 0);
>>>
>>> It does not look great but solves the uImage issue and does not modify
>>> the previous behavior. Anyway, I'm open for suggestions.
>>
>> Two options:
>>    1) Move the call to fw_unreserved_regions() outside of
>> kernel_decompress().
>>    2) Pass the offset of the gzip header to kernel_decompress().
>> Something like where it would be  sizeof(uimage) in the uImage case or 0
>> otherwise.
>>
>> I have a slight preference for the latter.
> That is cool.
> I'm in favor of this as well because it would allow not to set mod->start,size
> from kernel_uimage_probe. Everything can be done in kernel_decompress:
> 
> Diff:

A few comments because you resend the patch with it included.

> 
> -static __init int kernel_decompress(struct bootmodule *mod)
> +static __init int kernel_decompress(struct bootmodule *mod, uint32_t offset)
>   {
>       char *output, *input;
>       char magic[2];
> @@ -201,8 +201,14 @@ static __init int kernel_decompress(struct bootmodule *mod)
>       struct page_info *pages;
>       mfn_t mfn;
>       int i;
> -    paddr_t addr = mod->start;
> -    paddr_t size = mod->size;
> +
> +    /*
> +     * It might be that gzip header does not appear at the start address
> +     * (i.e. in case of compressed uImage) so take into account offset to

NIT: I would use 'e.g.' because in the future we may have multiple 
reason where the offset is not zero.

> +     * gzip header.
> +    */ > +    paddr_t addr = mod->start + offset;
> +    paddr_t size = mod->size - offset;

You want to check that mod->size is at least equal to offset.

>   
>       if ( size < 2 )
>           return -EINVAL;
> @@ -250,6 +256,13 @@ static __init int kernel_decompress(struct bootmodule *mod)
>       for ( ; i < (1 << kernel_order_out); i++ )
>           free_domheap_page(pages + i);
>   
> +    /*
> +     * When freeing the kernel, we need to pass the module start address and
> +     * size as they were before taking an offset to gzip header into account.
> +     */
> +    addr -= offset;
> +    size += offset;
> +
>       /*
>        * Free the original kernel, update the pointers to the
>        * decompressed kernel

Cheers,

-- 
Julien Grall

Re: [PATCH 2/2] xen/arm: Add support for booting gzip compressed uImages

[Michal Orzel]

Hi Julien,

On 01/02/2023 19:54, Julien Grall wrote:
> 
> 
> On 01/02/2023 12:56, Michal Orzel wrote:
>> Hi Julien,
> 
> Hi Michal,
> 
>> On 01/02/2023 12:20, Julien Grall wrote:
>>>
>>>
>>> On 01/02/2023 11:01, Michal Orzel wrote:
>>>> I would prefer not to do this in this series as the goals are different. This series is to remove
>>>> the known Xen limitation.
>>>
>>> The reason I am asking is it effectively change the way you would
>>> implement. If we were going to support zImage/Image within uImage, then
>>> we would need to fallthrough rather than calling kernel_decompress().
>>>
>>> I am not aware of any at the moment. Better asking now than realizing
>>> after the fact that there is a need...
>> We need uImage support as there is more and more need to support booting
>> raw images of some RTOSes (there is always additional concern \wrt booting requirements
>> but at least the header allows to try to boot them). We are not aware of any need
>> to do some special handling to parse more than one header + from what I said earlier,
>> this is an unusual approach not seen to be handled by anyone.
>>
>>>
>>>>>> This could be solved by doing (not harmful in my opinion for common case)
>>>>>> addr &= PAGE_MASK.
>>>>> I don't quite understand your argument here. We need a check that work
>>>>> for everyone (not only in the common case).
>>>> As we assume the kernel module is at page aligned address (otherwise the issue you observed
>>>> can happen not only in uImage compressed case)
>>>
>>> I am not aware of such assumption. It is allowed to use non page-aligned
>>> address and it is correct for Xen to not free the first part if it is
>>> not page aligned because the first part may contain data from another
>>> module (or else).
>>>
>>>> , having a check like
>>>> this is generic. I.e. for normal usecase (no uImage compressed), addr &= PAGE_MASK
>>>> does not modify the address. For uImage compressed usecase it causes the addr to get
>>>> back to the previous value (before we added header size to it).
>>>>
>>>> Apart from the addr, we need to pass the correct size (as we extracted header size from it).
>>>> We could have the following (with appropriate comment):
>>>> size += addr - (addr & PAGE_MASK);
>>>> addr &= PAGE_MASK;
>>>> fw_unreserved_regions(addr, addr + size, init_domheap_pages, 0);
>>>>
>>>> It does not look great but solves the uImage issue and does not modify
>>>> the previous behavior. Anyway, I'm open for suggestions.
>>>
>>> Two options:
>>>    1) Move the call to fw_unreserved_regions() outside of
>>> kernel_decompress().
>>>    2) Pass the offset of the gzip header to kernel_decompress().
>>> Something like where it would be  sizeof(uimage) in the uImage case or 0
>>> otherwise.
>>>
>>> I have a slight preference for the latter.
>> That is cool.
>> I'm in favor of this as well because it would allow not to set mod->start,size
>> from kernel_uimage_probe. Everything can be done in kernel_decompress:
>>
>> Diff:
> 
> A few comments because you resend the patch with it included.
> 
>>
>> -static __init int kernel_decompress(struct bootmodule *mod)
>> +static __init int kernel_decompress(struct bootmodule *mod, uint32_t offset)
>>   {
>>       char *output, *input;
>>       char magic[2];
>> @@ -201,8 +201,14 @@ static __init int kernel_decompress(struct bootmodule *mod)
>>       struct page_info *pages;
>>       mfn_t mfn;
>>       int i;
>> -    paddr_t addr = mod->start;
>> -    paddr_t size = mod->size;
>> +
>> +    /*
>> +     * It might be that gzip header does not appear at the start address
>> +     * (i.e. in case of compressed uImage) so take into account offset to
> 
> NIT: I would use 'e.g.' because in the future we may have multiple
> reason where the offset is not zero.
> 
>> +     * gzip header.
>> +    */ > +    paddr_t addr = mod->start + offset;
>> +    paddr_t size = mod->size - offset;
> 
> You want to check that mod->size is at least equal to offset.
Sounds reasonable. Thanks.

~Michal