Re: [PATCH v5 2/2] xen/arm: support gzip compressed kernels

[Ian Campbell <ian.campbell@citrix.com>]

On Mon, 2015-09-07 at 15:25 +0100, Stefano Stabellini wrote:
> Free the memory used for the compressed kernel and update the relative
> mod->start and mod->size parameters with the uncompressed ones.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Reviewed-by: Julien Grall <julien.grall@citrix.com>
> CC: ian.campbell@citrix.com
> 
> ---
> 
> Changes in v5:
> - code style
> 
> Changes in v4:
> - return uint32_t from output_length
> - __init kernel_decompress
> - code style
> - add comment
> - if kernel_decompress fails with error, return
> 
> Changes in v3:
> - better error checks in kernel_decompress
> - free unneeded pages between output_size and kernel_order_out
> - alloc pages from domheap
> 
> Changes in v2:
> - use gzip_check
> - avoid useless casts
> - free original kernel image and update the mod with the new address and
> size
> - remove changes to common Makefile
> - remove CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
> ---
>  xen/arch/arm/kernel.c       |   75
> +++++++++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/setup.c        |    2 +-
>  xen/include/asm-arm/setup.h |    2 ++
>  3 files changed, 78 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> index f641b12..baa5717 100644
> --- a/xen/arch/arm/kernel.c
> +++ b/xen/arch/arm/kernel.c
> @@ -13,6 +13,8 @@
>  #include <asm/byteorder.h>
>  #include <asm/setup.h>
>  #include <xen/libfdt/libfdt.h>
> +#include <xen/gunzip.h>
> +#include <xen/vmap.h>
>  
>  #include "kernel.h"
>  
> @@ -257,6 +259,63 @@ static int 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 kernel_info *info,
> +                             paddr_t *addr, paddr_t *size)
> +{
> +    char *output, *input, *end;
> +    char magic[2];
> +    int rc;
> +    unsigned kernel_order_out;
> +    paddr_t output_size;
> +    struct page_info *pages;
> +
> +    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;
> +    }
> +    output = page_to_virt(pages);
> +
> +    rc = perform_gunzip(output, input, *size);
> +    clean_dcache_va_range(output, output_size);
> +    iounmap(input);
> +
> +    *addr = virt_to_maddr(output);

I don't think virt_to_maddr is strictly speaking valid (at the arch
interface level, our actual implementation may happen to cope) for domheap
pages, it's only valid for things which are in the linear 1:1 map (~=
xenheap).

I think you need page_to_maddr(pages) instead.


> +    *size = output_size;
> +
> +    end = output + (1 << (kernel_order_out + PAGE_SHIFT));
> +    /* 
> +     * Need to free pages after output_size here because they won't be
> +     * freed by discard_initial_modules
> +     */
> +    output += (output_size + PAGE_SIZE - 1) & PAGE_MASK;
> +    for ( ; output < end; output += PAGE_SIZE )
> +        free_domheap_page(virt_to_page(output));

And here again I don't think you can use virt_to_page.

> +
> +    return 0;
> +}
> +
>  #ifdef CONFIG_ARM_64
>  /*
>   * Check if the image is a 64-bit Image.
> @@ -463,6 +522,22 @@ int kernel_probe(struct kernel_info *info)
>          printk("Loading ramdisk from boot module @ %"PRIpaddr"\n",
>                 info->initrd_bootmodule->start);
>  
> +    /* if it is a gzip'ed image, 32bit or 64bit, uncompress it */
> +    rc = kernel_decompress(info, &start, &size);
> +    if (rc < 0 && rc != -EINVAL)

IMHO kernel_decompress should return success when the decompress is a nop
(as represented by EINVAL here) and an error only when the thing needs to
be decompressed but cannot be.

That would mean putting the free of the original kernel and the updates of
mod->* into kernel_decompress. But I think that also makes more sense
because it confines the "ol' switcheroo" to the one place instead of mostly
up there and then the tail end cleanup down here. i.e. you call
kernel_decompress with everything in a consistent and valid state and
return with everything also in a (possibly different) consistent and valid
state.

I wouldn't have bothered commenting on this if I weren't already commenting
on the above, since it's not a huge deal if you think this is the wrong
direction.

Ian.