Re: [Xen-devel] [PATCH v2 2/2] xen/arm: support gzip compressed kernels

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Thu, 13 Aug 2015, Julien Grall wrote:
> 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.

I'll make these changes


> > +
> > +    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.

Actually init_heap_pages is just a wrapper around free_heap_pages, like
free_xenheap_pages. I think it is safe to call init_heap_pages, on
memory allocated by alloc_xenheap_pages.


> 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.

That is a good point. I am going to write a simple loop to free the
remaining pages.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel