From mboxrd@z Thu Jan  1 00:00:00 1970
From: Julien Grall <julien.grall@citrix.com>
Subject: Re: [PATCH v2 2/2] xen/arm: support gzip compressed
	kernels
Date: Thu, 13 Aug 2015 12:35:18 +0100
Message-ID: <55CC80F6.3030307@citrix.com>
References: <alpine.DEB.2.02.1508131217440.22657@kaball.uk.xensource.com>
	<1439464897-32609-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: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1439464897-32609-2-git-send-email-stefano.stabellini@eu.citrix.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Stefano Stabellini <stefano.stabellini@eu.citrix.com>, xen-devel@lists.xensource.com
Cc: Ian.Campbell@citrix.com
List-Id: xen-devel@lists.xenproject.org

Hi Stefano,

On 13/08/15 12:21, Stefano Stabellini wrote:
> +static int kernel_decompress(struct kernel_info *info,
> +                             paddr_t *addr, paddr_t *size)
> +{
> +    char *output, *input;
> +    char magic[2];
> +    int rc;
> +    unsigned kernel_order_in;
> +    unsigned kernel_order_out;
> +    paddr_t output_size;
> +    

Please check that the binary is not too small before reading the magic.

> +    copy_from_paddr(magic, *addr, sizeof(magic));
> +
> +    /* only gzip is supported */
> +    if (!gzip_check(magic, *size))
> +        return 0;
> +
> +    kernel_order_in = get_order_from_bytes(*size);
> +    input = ioremap_cache(*addr, *size);

The return of ioremap_cache should be check.

> +
> +    output_size = output_length(input, *size);
> +    kernel_order_out = get_order_from_bytes(output_size);
> +    output = alloc_xenheap_pages(kernel_order_out, 0);

Ditto.

> +
> +    rc = perform_gunzip(output, input, *size);
> +    clean_dcache_va_range(output, output_size);
> +    iounmap(input);
> +
> +    *addr = virt_to_maddr(output);
> +    *size = output_size;
> +    return 1;
> +}
> +
>  #ifdef CONFIG_ARM_64
>  /*
>   * Check if the image is a 64-bit Image.
> @@ -463,6 +502,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) > 0)
> +    {
> +        /* Free the original kernel, update the pointers to the
> +         * decompressed kernel */
> +        dt_unreserved_regions(mod->start, mod->start + mod->size,
> +                init_domheap_pages, 0);
> +        mod->start = start;
> +        mod->size = size;

I'm not sure to see how this would work. Any call to alloc_xenheap_pages
should be follow by a called to free_xenheap_pages.
But when freeing the modules, we are using init_heap_pages.
I don't think they are similar.

Furthermore, you may allocate more memory than necessary because you are
using an order but you never update the size. So we would have memory
loose forever.

Regards,

-- 
Julien Grall