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

[Ayan Kumar Halder <ayankuma@amd.com>]

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