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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Tue, 8 Sep 2015, Ian Campbell wrote:
> 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@xxxxxxxxxxxxx>
> > Reviewed-by: Julien Grall <julien.grall@xxxxxxxxxx>
> > CC: ian.campbell@xxxxxxxxxx
> > 
> > ---
> > 
> > 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.

I replaced it all with vmap/vunmap.


> > +
> > +    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 mean collapsing the "nothing to do" and the "decompression
successful" cases into a single return value, which I think is not a
good idea. We would be losing information compared to what we have now.
I am quite happy to replace EINVAL with any other return value you think
is most appropriate though.


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

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