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

[Julien Grall <julien.grall@citrix.com>]

Hi Stefano,

The code looks good to me. Only few coding style comment and request to
improve the comments.

On 02/09/15 12:33, 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>
> CC: julien.grall@citrix.com
> CC: ian.campbell@citrix.com
> 
> ---
> 
> 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       |   66 +++++++++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/setup.c        |    2 +-
>  xen/include/asm-arm/setup.h |    2 ++
>  3 files changed, 69 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> index f641b12..f6da7c1 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,61 @@ static int kernel_uimage_probe(struct kernel_info *info,
>      return 0;
>  }
>  
> +static __init unsigned long output_length(char *image, unsigned long image_len)
> +{

Should not you return a uint32_t rather than an unsigned long?

> +    return *(uint32_t *)&image[image_len - 4];
> +}
> +
> +static int kernel_decompress(struct kernel_info *info,
> +                             paddr_t *addr, paddr_t *size)

static __init as you call a function which live in init section (i.e
output_lenght)

> +{
> +    char *output, *input, *end;
> +    char magic[2];
> +    int rc;
> +    unsigned kernel_order_out;
> +    paddr_t output_size;
> +    struct page_info *pages;
> +    
> +    if (*size < 2)

if ( ... ) as for all the "if" and "for" within this function. AFAICT,
only kernel_probe is using the wrong coding style within this file.

> +        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);
> +
> +

NIT: I think you can drop only line here.

> +    rc = perform_gunzip(output, input, *size);
> +    clean_dcache_va_range(output, output_size);
> +    iounmap(input);
> +
> +    *addr = virt_to_maddr(output);
> +    *size = output_size;
> +
> +    end = output + (1 << (kernel_order_out + PAGE_SHIFT));
> +    output += (output_size + PAGE_SIZE - 1) & PAGE_MASK;
> +    for (; output < end; output += PAGE_SIZE)

Can you add a comment to explain why you need to free pages that are unused?

> +    {
> +        free_domheap_page(virt_to_page(output));
> +    }

The brace are not necessary and it's missing a newline before the return.

> +    return 0;
> +}
> +
>  #ifdef CONFIG_ARM_64
>  /*
>   * Check if the image is a 64-bit Image.
> @@ -463,6 +520,15 @@ int kernel_probe(struct kernel_info *info)
>          printk("Loading ramdisk from boot module @ %"PRIpaddr"\n",
>                 info->initrd_bootmodule->start);
>  
> +    if (!kernel_decompress(info, &start, &size))

NIT: Maybe a comment to explain that we need to do it for everyone?

> +    {
> +        /* Free the original kernel, update the pointers to the
> +         * decompressed kernel */
> +        dt_unreserved_regions(mod->start, mod->start + mod->size,
> +                init_domheap_pages, 0);

The indentation looks wrong here. I think it should be

dt_unreserved_regions(....
                      init_domheap_pages, 0);

> +        mod->start = start;
> +        mod->size = size;
> +    }

NIT: I would add a newline here to make clear.

>  #ifdef CONFIG_ARM_64
>      rc = kernel_zimage64_probe(info, start, size);
>      if (rc < 0)

Regards,

-- 
Julien Grall