From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v3 2/2] xen/arm: support gzip compressed kernels Date: Thu, 3 Sep 2015 12:39:52 +0100 Message-ID: <55E83188.5050802@citrix.com> References: <1441193587-5209-2-git-send-email-stefano.stabellini@eu.citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1441193587-5209-2-git-send-email-stefano.stabellini@eu.citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Stefano Stabellini , xen-devel@lists.xensource.com Cc: ian.campbell@citrix.com List-Id: xen-devel@lists.xenproject.org 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 > 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 > #include > #include > +#include > +#include > > #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